Thanks, Gred and Matti. On Mon, 27 Mar 2023 at 20:38, Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> 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: > > > > A few tests need to have a valid struct device. One such example is > > > > tests which want to be testing devm-managed interfaces. > > > > > > > > Add kunit wrapper for root_device_[un]register(), which create a root > > > > device and also add a kunit managed clean-up routine for the device > > > > destruction upon test exit. > > > > > > I really do not like this as a "root device" is a horrible hack and > > > should only be used if you have to hang other devices off of it and you > > > don't have a real device to tie those devices to. > > > > > > Here you are abusing it and attempting to treat it as a real device, > > > which it is not at all, because: > > > There's a tradeoff here in that we want to pull in as little code (and complexity) as possible into unit tests, both to make them as easy as possible to write, and to make them as targeted as possible. This leads to a lot of tests manually filling out structures with the bare minimum to get the code being tested to run, and creating "root devices" seems to have been a convenient way of doing that while only registering _one_ thing in a big global list per test. So having a "real device" is not something I'd consider a _necessity_ in designing these sorts of helpers: a convincing enough fake is sometimes better. That being said, now that I've got a bit of a better understanding of the device model, I agree that "root devices" are not an ideal solution, even if they are a convenient one. I'm still not thrilled by the prospect of having to register extra things like drivers to get these simple tests to work, but when wrapped behind helpers, it's a nice solution in practice. > > > > Special note: In some cases the device reference-count does not reach > > > > zero and devm-unwinding is not done if device is not sitting on a bus. > > > > The root_device_[un]register() are dealing with such devices and thus > > > > this interface may not be usable by all in its current form. More > > > > information can be found from: > > > > https://lore.kernel.org/dri-devel/20221117165311.vovrc7usy4efiytl@houat/ > > > > > > See, not a real device, doesn't follow normal "struct device" rules and > > > lifetimes, don't try to use it for a test as it will only cause problems > > > and you will be forced to work around that in a test. I think there's still some confusion around exactly what these issues are, and if they're indeed bugs or expected behaviour. I think it hangs off the question of what uses of a device with no driver attached are valid. My initial reading through of the various bits of the devres implementation seemed to imply that using it with such an unattached device was supported, but I'm less certain now. In any case, Maxime's tests in the other thread are a good starting point to clarify this behaviour, and if we use the bus-based KUnit helpers, it won't matter either way. > > > > Ok. I understood using the root-device has been a work-around in some other > > tests. Thus continuing use it for tests where we don't need the bus until we > > have a proper alternative was suggested by David. > > > > > Do the right thing here, create a fake bus and add devices to it. > > > > > > Heck, I'll even write that code if you want it, what's the requirement, > > > something like: > > > struct device *kunit_device_create(struct kunit *test, const char *name); > > > void kunit_device_destroy(struct device *dev); > > > > Thanks for the offer Greg. This, however, is being already worked on by > > David. I don't want to step on his toes by writing the same thing, nor do I > > think I should be pushing him to rush on his work. > > Ok, David, my offer stands, if you want me to write this I will be glad > to do so. 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/#F0 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), 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) - Improving the API around drivers: probably exposing the helper to create a driver, and add a way of cleaning it up early. - Properly testing it with modules, not just with kunit.py (including looking at what actually appears in sysfs) - Experimenting with using probe, etc, callbacks. - Cleaning up lots of ugly, still experimental code, documenting, testing, etc. The thought of trying to expand the match function to support, e.g., devicetree occurred to me, but I _think_ that devicetree-based tests are probably still better off using a platform_device. Regardless, it can probably wait to a follow-up In any case, does this seem like the right way forward? Cheers, -- David
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
