On Tue, 28 Mar 2023 at 21:22, Matti Vaittinen mazziesaccount@gmail.com wrote:
Hi David & Greg and thanks for working with this!
On 3/28/23 15:45, David Gow wrote:
Thanks, Gred and Matti.
On Mon, 27 Mar 2023 at 20:38, Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:
On Mon, Mar 27, 2023 at 03:20:06PM +0300, Matti Vaittinen wrote:
On 3/27/23 15:01, Greg Kroah-Hartman wrote:
On Mon, Mar 27, 2023 at 02:34:02PM +0300, Matti Vaittinen wrote:
I'm happy to keep working on this, but would definitely appreciate your feedback.
I've put my work-in-progress code here: https://kunit.googlesource.com/linux/+/refs/heads/kunit/device-helpers%5E%21...
It creates a "kunit" bus, and adds a few helpers to create both devices and drivers on that bus, and clean them up when the test exits. It seems to work on all of the tests which used root_device_register so far (except those -- only test_iio_find_closest_gain_low so far -- which create multiple devices with the same name, as the driver name won't be unique),
I wouldn't worry about it for as long as it's just because an iio-gts test does something silly. Those tests are currently only in my personal playground and changing those tests should be pretty trivial.
Yeah: this isn't anything to worry about. It's as much my note as to "what works with this code as-is", rather than indicative of a more fundamental flaw.
And right after saying that - the test_iio_find_closest_gain_low test does
a) register a 'test' device b) perform test on devm_ API c) unregister the 'test' device
d) register a 'test' device (same name as at step a) e) perform test on devm_ API f) unregister the 'test' device
My assumption is that the test device would be gone after step c) because there should be no references to it anywhere. Hence, I wonder why registering at step d) fails? (Or did I misunderstand something?)
This is because I'm now creating a struct device_driver as well as a device, and it's not being cleaned up until the end of the test.
The two solutions here would be to either: - Add a way to clean up the driver earlier. (Shouldn't be too hard, I just haven't got around to it yet), or: - Use the same struct device_driver for both tests (there's a new kunit_device_register_with_driver which allows you to pass a custom driver in, rather than creating your own)
I think the latter's probably better, but I'll probably implement both as I clean up the API further.
and the drm tests work fine when ported to it as well.
There's still a lot of cleanup to do and questions which need answering, including:
- Working out how best to provide an owning module (it's currently
just kunit, but probably should be the module which contains the actual tests)
Maybe there is something I am not seeing but how about wrapping the kunit_device_register() in a macro and getting the THIS_MODULE in caller's context?
Yeah: that's probably what I'll do. The other possibility is to store the module pointer in the struct kunit context.
In any case, does this seem like the right way forward?
I am by no means an expert on this but this does look good to me. I would keep this as clean, lean and simple as possible in order to keep understanding / debugging the problems exposed by the tests as simple as possible. At some point someone is wondering why a test fails, and ends up looking through these helpers to ensure problem is no lurking there... Hence, I'd kept the code there in minimum - meaning, I might not add kunit class or even a driver until tests require that. (Even if it would not look as good in the sysfs - as far as I understand the kunit sysfs entries are a 'test feature' which should not be present in 'production systems'. This is not an excuse to make things bad - but (in my opinion) this is a good reason to prioritize simplicity.
I agree with you that it's best to avoid complexity for as long as we reasonably can. I think that there are enough things which would benefit from having the driver stuff to make it worth having _something_ there, particularly since it makes this a more "normal" device, so hopefully will be less surprising.
For sysfs, it's one of those things which shows up pretty rarely in day-to-day KUnit use, as most people are using the kunit.py script which has all of the tests built-in, and no userspace to look at sysfs from. That being said, that doesn't cover all use cases, and I definitely would rather not make sysfs unusably ugly for everyone else, so I'm happy to tidy it up. (I'm not planning to do a kunit class at the moment, though.)
I _think_ this approach (once the details of the API and implementation are tidied up a bit) should sit in about the sweet spot for complexity, assuming there's nothing horribly wrong with it I haven't noticed. I suspect we're better off leaving some of the more advanced use-cases to implement their own way of instantiating devices, at least for now, and focus on getting these common cases right.
Cheers, -- David