[v6,3/7] kunit: Add kunit wrappers for (root) device creation

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.

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/

The use of root-devices in the kunit helpers is intended to be an
intermediate solution to allow tests which do not require device to sit
on a bus avoid directly abusing the root_device_[un]register() while
proper kunit device solution is being worked on. Related discussion can be
found from:
https://lore.kernel.org/lkml/CABVgOSmx3A4Vwos2_8xO-XQrQAw5gvY0nc5zLpLmcJ7FtA-dTQ@mail.gmail.com/

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

---
Change history:
v5 => v6:
- Kunit resource-managed root_device creation wrapper (new patch)

Please note: This patch uses root-devices (as was suggested) until there
is a proper dummy device creation mechanism added in kunit. The root
devices are embedded in kunit wrappers to simplify replacing the
root-devices with proper solution when it is available.

David Gow has sent out an RFC[1] which should implement these helpers
using not-yet-in-tree deferring API. This RFC aims to support
kunit_device which should be _the right thing to do_. I added this
implementation here because it may (or may not) take a while for the David's
RFC to make it's way in-kernel. So, in order to not delay this series I
added these helpers which use the existing kunit resource management for
clean-up while the new deferring kunit API is not yet in place.

[1] https://lore.kernel.org/linux-kselftest/20230325043104.3761770-1-davidgow@google.com/T/#mf797239a8bce11630875fdf60aab9ed627add1f0

---
 include/kunit/device.h | 18 ++++++++++++++++++
 lib/kunit/Makefile     |  3 ++-
 lib/kunit/device.c     | 36 ++++++++++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+), 1 deletion(-)
 create mode 100644 include/kunit/device.h
 create mode 100644 lib/kunit/device.c

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:
> 
>> 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.

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.

> Why do you want a "match" function?  You don't provide documentation
> here for it so I have no idea.
> 
> Anything else needed?
> 
>> The use of root-devices in the kunit helpers is intended to be an
>> intermediate solution to allow tests which do not require device to sit
>> on a bus avoid directly abusing the root_device_[un]register() while
>> proper kunit device solution is being worked on. Related discussion can be
>> found from:
>> https://lore.kernel.org/lkml/CABVgOSmx3A4Vwos2_8xO-XQrQAw5gvY0nc5zLpLmcJ7FtA-dTQ@mail.gmail.com/
> 
> Again, no, please let's not get this wrong now and say "we will fix this
> later" as that's not how kernel development should work...

Ok. In that case I need to drop the tests from the series until we get 
the new APIs in place. It really sucks but I guess I understand the 
rationale for not wanting to "intermediate" solutions merged. Yes, I 
hoped it'd be Ok as David is already working on it - but I was still 
kind of expecting your response. This is why I made it very clear in the 
cover-letter and this commit message what is suggested here.

Jonathan, should I re-spin the series without patches 3/7 and 5/7 or can 
you please review this and I'll just drop those for the next version?

Thanks for the review Greg, I think this case is now "closed".

Yours,
	-- Matti

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:
> > > > 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

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/#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),

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.

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?)

> 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?

> 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.

Anyways, thanks for the work!

Yours,
	-- Matti

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/#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),
>
> 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