[v5,1/8] drivers: kunit: Generic helpers for test device creation

The creation of a dummy device in order to test managed interfaces is a
generally useful test feature. The drm test helpers
drm_kunit_helper_alloc_device() and drm_kunit_helper_free_device()
are doing exactly this. It makes no sense that each and every component
which intends to be testing managed interfaces will create similar
helpers so stole these for more generic use.

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

---
Changes:
v4 => v5:
- Add accidentally dropped header and email recipients
- do not rename + move helpers from DRM but add temporary dublicates to
  simplify merging. (This patch does not touch DRM and can be merged
  separately. DRM patch and IIO test patch still depend on this one).

Please note that there's something similar ongoing in the CCF:
https://lore.kernel.org/all/20230302013822.1808711-1-sboyd@kernel.org/

I do like the simplicity of these DRM-originated helpers so I think
they're worth. I do equally like the Stephen's idea of having the
"dummy platform device" related helpers under drivers/base and the
header being in include/kunit/platform_device.h which is similar to real
platform device under include/linux/platform_device.h
---
 drivers/base/test/Kconfig             |  5 ++
 drivers/base/test/Makefile            |  2 +
 drivers/base/test/test_kunit_device.c | 83 +++++++++++++++++++++++++++
 include/kunit/platform_device.h       | 13 +++++
 4 files changed, 103 insertions(+)
 create mode 100644 drivers/base/test/test_kunit_device.c
 create mode 100644 include/kunit/platform_device.h

On Fri, 24 Mar 2023 at 14:11, Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>
> On 3/23/23 18:36, Maxime Ripard wrote:
> > On Thu, Mar 23, 2023 at 03:02:03PM +0200, Matti Vaittinen wrote:
> >> On 3/23/23 14:29, Maxime Ripard wrote:
> >>> On Thu, Mar 23, 2023 at 02:16:52PM +0200, Matti Vaittinen wrote:
> >>>
> >>> This is the description of what was happening:
> >>> https://lore.kernel.org/dri-devel/20221117165311.vovrc7usy4efiytl@houat/
> >>
> >> Thanks Maxime. Do I read this correcty. The devm_ unwinding not being done
> >> when root_device_register() is used is not because root_device_unregister()
> >> would not trigger the unwinding - but rather because DRM code on top of this
> >> device keeps the refcount increased?
> >
> > There's a difference of behaviour between a root_device and any device
> > with a bus: the root_device will only release the devm resources when
> > it's freed (in device_release), but a bus device will also do it in
> > device_del (through bus_remove_device() -> device_release_driver() ->
> > device_release_driver_internal() -> __device_release_driver() ->
> > device_unbind_cleanup(), which are skipped (in multiple places) if
> > there's no bus and no driver attached to the device).
> >
> > It does affect DRM, but I'm pretty sure it will affect any framework
> > that deals with device hotplugging by deferring the framework structure
> > until the last (userspace) user closes its file descriptor. So I'd
> > assume that v4l2 and cec at least are also affected, and most likely
> > others.
>
> Thanks for the explanation and patience :)
>

Thanks from me as well -- this has certainly helped me understand some
of the details of the driver model that had slipped past me.

> >
> >> If this is the case, then it sounds like a DRM specific issue to me.
> >
> > I mean, I guess. One could also argue that it's because IIO doesn't
> > properly deal with hotplugging.
>
> I must say I haven't been testing the IIO registration API. I've only
> tested the helper API which is not backed up by any "IIO device". (This
> is fine for the helper because it must by design be cleaned-up only
> after the IIO-deregistration).
>
> After your explanation here, I am not convinced IIO wouldn't see the
> same issue if I was testing the devm_iio_device_alloc() & co.
>
> > I'm not sure how that helps. Those are
> > common helpers which should accommodate every framework,
>
> Ok. Fair enough. Besides, if the root-device was sufficient - then I
> would actually not see the need for a helper. People could in that case
> directly use the root_device_register(). So, if helpers are provided
> they should be backed up by a device with a bus then.
>

I think there is _some_ value in helpers even without a bus, but it's
much more limited:
- It's less confusing if KUnit test devices are using kunit labelled
structs and functions.
- Helpers could use KUnit's resource management API to ensure any
device created is properly unregistered and removed when the test
exits (particularly if it exits early due to, e.g., an assertion).

I've played around implementing those with a proper struct
kunit_device and the automatic cleanup on test failure, and thus far
it -- like root_device_register -- works for all of the tests except
the drm-test-managed one.

So if we really wanted to, we could use KUnit-specific helpers for
just those tests which currently work with root_device_register(), but
if we're going to try to implement a KUnit bus -- which I think is at
least worth investigating -- I'd rather not either hold up otherwise
good tests on helper development, or rush a helper out only to have to
change it a lot when we see exactly what the bus implementation would
look like.

> > and your second
> > patch breaks the kunit tests for DRM anyway.
>
> Oh, I must have made an error there. It was supposed to be just a
> refactoring with no functional changes. Sorry about that. Anyways, that
> patch can be forgotten as Greg opposes using the platform devices in
> generic helpers.
>
> >> Whether it is a feature or bug is beyond my knowledge. Still, I would
> >> not say using the root_device_[un]register() in generic code is not
> >> feasible - unless all other subsytems have similar refcount handling.
> >>
> >> Sure thing using root_device_register() root_device_unregister() in DRM does
> >> not work as such. This, however, does not mean the generic kunit helpers
> >> should use platform_devices to force unwinding?
> >
> > platform_devices were a quick way to get a device that would have a bus
> > and a driver bound to fall into the right patch above. We probably
> > shouldn't use platform_devices and a kunit_device sounds like the best
> > idea, but the test linked in the original mail I pointed you to should
> > work with whatever we come up with. It works with multiple (platform,
> > PCI, USB, etc) buses, so the mock we create should behave like their
> > real world equivalents.
> Thanks for the patience and the explanation. Now I understand a generic
> test device needs to sit on a bus.
>
> As I said, in my very specific IIO related test the test device does not
> need a bus. Hence I'll drop the 'generic helpers' from this series.
>

I think that sounds like a good strategy for now, and we can work on a
set of 'generic helpers' which have an associated bus and struct
kunit_device in the meantime. If we can continue to use
root_device_register until those are ready, that'd be very convenient.
Certainly, the helpers I'm playing with at the moment have a few other
dependencies I'd want to land first, so I suspect they wouldn't be
done in time for 6.4. It'd also make sense to see if we can make sure
any such helpers could either work well with (or at least not conflict
with) tests which use devicetree.

Regardless, thanks very much for putting all of the effort in to
working this out. I think we have a much better idea of the
requirements for this sort of thing now.

Cheers,
-- David

On Fri, 24 Mar 2023 at 14:51, Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>
> On 3/24/23 08:34, David Gow wrote:
> > On Fri, 24 Mar 2023 at 14:11, Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >>
> >> On 3/23/23 18:36, Maxime Ripard wrote:
> >>> On Thu, Mar 23, 2023 at 03:02:03PM +0200, Matti Vaittinen wrote:
> >>>> On 3/23/23 14:29, Maxime Ripard wrote:
> >>>>> On Thu, Mar 23, 2023 at 02:16:52PM +0200, Matti Vaittinen wrote:
>
> >> Ok. Fair enough. Besides, if the root-device was sufficient - then I
> >> would actually not see the need for a helper. People could in that case
> >> directly use the root_device_register(). So, if helpers are provided
> >> they should be backed up by a device with a bus then.
> >
> > I think there is _some_ value in helpers even without a bus, but it's
> > much more limited:
> > - It's less confusing if KUnit test devices are using kunit labelled
> > structs and functions.
> > - Helpers could use KUnit's resource management API to ensure any
> > device created is properly unregistered and removed when the test
> > exits (particularly if it exits early due to, e.g., an assertion).
>
> Ah. That's true. Being able to abort the test on error w/o being forced
> to do a clean-up dance for the dummy device would be convenient.
>
> > I've played around implementing those with a proper struct
> > kunit_device and the automatic cleanup on test failure, and thus far
> > it -- like root_device_register -- works for all of the tests except
> > the drm-test-managed one.
> >
> > So if we really wanted to, we could use KUnit-specific helpers for
> > just those tests which currently work with root_device_register(), but
> > if we're going to try to implement a KUnit bus -- which I think is at
> > least worth investigating -- I'd rather not either hold up otherwise
> > good tests on helper development, or rush a helper out only to have to
> > change it a lot when we see exactly what the bus implementation would
> > look like.
>
> It's easy for me to agree.
>
> >> As I said, in my very specific IIO related test the test device does not
> >> need a bus. Hence I'll drop the 'generic helpers' from this series.
> >>
> >
> > I think that sounds like a good strategy for now, and we can work on a
> > set of 'generic helpers' which have an associated bus and struct
> > kunit_device in the meantime. If we can continue to use
> > root_device_register until those are ready, that'd be very convenient.
>
> Would it be a tiny bit more acceptable if we did add a very simple:
>
> #define kunit_root_device_register(name) root_device_register(name)
> #define kunit_root_device_unregister(dev) root_device_unregister(dev)
>
> to include/kunit/device.h (or somesuch)
>
> This should help us later to at least spot the places where
> root_device_[un]register() is abused and (potentially mass-)covert them
> to use the proper helpers when they're available.
>

Great idea.

The code I've been playing with has the following in include/kunit/device.h:

/* Register a new device against a KUnit test. */
struct device *kunit_device_register(struct kunit *test, const char *name);
/* Unregister a device created by kunit_device_register() early (i.e.,
before test cleanup). */
void kunit_device_unregister(struct kunit *test, struct device *dev);

If we used the same names, and just forwarded them to
root_device_register() and root_device_unregister() for now
(discarding the struct kunit pointer), then I expect we could just
swap out the implementation to gain the extra functionality.

It's a little less explicit, though, so I could see the value in using
macros with "root_device" in the name to make the current
implementation clearer, and the eventual change more obvious.

Cheers,
-- David

On Fri, 24 Mar 2023 at 18:17, Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>
> On 3/24/23 12:05, Matti Vaittinen wrote:
> > On 3/24/23 11:52, David Gow wrote:
> >> On Fri, 24 Mar 2023 at 14:51, Matti Vaittinen
> >> <mazziesaccount@gmail.com> wrote:
> >>>
> >>> On 3/24/23 08:34, David Gow wrote:
> >>>> On Fri, 24 Mar 2023 at 14:11, Matti Vaittinen
> >>>> <mazziesaccount@gmail.com> wrote:
> >
> >>>> I think that sounds like a good strategy for now, and we can work on a
> >>>> set of 'generic helpers' which have an associated bus and struct
> >>>> kunit_device in the meantime. If we can continue to use
> >>>> root_device_register until those are ready, that'd be very convenient.
> >>>
> >>> Would it be a tiny bit more acceptable if we did add a very simple:
> >>>
> >>> #define kunit_root_device_register(name) root_device_register(name)
> >>> #define kunit_root_device_unregister(dev) root_device_unregister(dev)
> >>>
> >>> to include/kunit/device.h (or somesuch)
> >>>
> >>> This should help us later to at least spot the places where
> >>> root_device_[un]register() is abused and (potentially mass-)covert them
> >>> to use the proper helpers when they're available.
> >>>
> >>
> >> Great idea.
> >>
> >> The code I've been playing with has the following in
> >> include/kunit/device.h:
> >>
> >> /* Register a new device against a KUnit test. */
> >> struct device *kunit_device_register(struct kunit *test, const char
> >> *name);
> >> /* Unregister a device created by kunit_device_register() early (i.e.,
> >> before test cleanup). */
> >> void kunit_device_unregister(struct kunit *test, struct device *dev);
> >>
> >> If we used the same names, and just forwarded them to
> >> root_device_register() and root_device_unregister() for now
> >> (discarding the struct kunit pointer), then I expect we could just
> >> swap out the implementation to gain the extra functionality.
>
> There's one thing though. If the goal is to do a direct replacement and
> if automatic device deletion upon test completion / test abort is
> planned - then it should be there also for these initial wrappers.
>

Yeah, that's an excellent point. It's a pretty subtle change in
behaviour to suddenly introduce that, so changing it behind the scenes
is probably unwise.

> If these wrappers don't yet include the automatic device clean-up - then
> it probably makes more sense to just do the kunit_root_device_* defines
> because the tests are likely to need removing the explicit device
> clean-ups when proper APIs are finished.
>

I sent out my prototype implementation of this here, which does do the
automatic cleanup:
https://lore.kernel.org/linux-kselftest/20230325043104.3761770-1-davidgow@google.com/T/#mf797239a8bce11630875fdf60aab9ed627add1f0

It's probably overkill to squeeze into your patch series, though,
given it also adds and uses a whole new kunit_defer() API.

Cheers,
-- David

On 3/25/23 06:35, David Gow wrote:
> On Fri, 24 Mar 2023 at 18:17, Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>>
>> On 3/24/23 12:05, Matti Vaittinen wrote:
>>> On 3/24/23 11:52, David Gow wrote:
>>>> On Fri, 24 Mar 2023 at 14:51, Matti Vaittinen
>>>> <mazziesaccount@gmail.com> wrote:
>>>>>
>>>>> On 3/24/23 08:34, David Gow wrote:
>>>>>> On Fri, 24 Mar 2023 at 14:11, Matti Vaittinen
>>>>>> <mazziesaccount@gmail.com> wrote:
>>>
>>>>>> I think that sounds like a good strategy for now, and we can work on a
>>>>>> set of 'generic helpers' which have an associated bus and struct
>>>>>> kunit_device in the meantime. If we can continue to use
>>>>>> root_device_register until those are ready, that'd be very convenient.
>>>>>
>>>>> Would it be a tiny bit more acceptable if we did add a very simple:
>>>>>
>>>>> #define kunit_root_device_register(name) root_device_register(name)
>>>>> #define kunit_root_device_unregister(dev) root_device_unregister(dev)
>>>>>
>>>>> to include/kunit/device.h (or somesuch)
>>>>>
>>>>> This should help us later to at least spot the places where
>>>>> root_device_[un]register() is abused and (potentially mass-)covert them
>>>>> to use the proper helpers when they're available.
>>>>>
>>>>
>>>> Great idea.
>>>>
>>>> The code I've been playing with has the following in
>>>> include/kunit/device.h:
>>>>
>>>> /* Register a new device against a KUnit test. */
>>>> struct device *kunit_device_register(struct kunit *test, const char
>>>> *name);
>>>> /* Unregister a device created by kunit_device_register() early (i.e.,
>>>> before test cleanup). */
>>>> void kunit_device_unregister(struct kunit *test, struct device *dev);
>>>>
>>>> If we used the same names, and just forwarded them to
>>>> root_device_register() and root_device_unregister() for now
>>>> (discarding the struct kunit pointer), then I expect we could just
>>>> swap out the implementation to gain the extra functionality.
>>
>> There's one thing though. If the goal is to do a direct replacement and
>> if automatic device deletion upon test completion / test abort is
>> planned - then it should be there also for these initial wrappers.
>>
> 
> Yeah, that's an excellent point. It's a pretty subtle change in
> behaviour to suddenly introduce that, so changing it behind the scenes
> is probably unwise.
> 
>> If these wrappers don't yet include the automatic device clean-up - then
>> it probably makes more sense to just do the kunit_root_device_* defines
>> because the tests are likely to need removing the explicit device
>> clean-ups when proper APIs are finished.
>>
> 
> I sent out my prototype implementation of this here, which does do the
> automatic cleanup:
> https://lore.kernel.org/linux-kselftest/20230325043104.3761770-1-davidgow@google.com/T/#mf797239a8bce11630875fdf60aab9ed627add1f0
> 
> It's probably overkill to squeeze into your patch series, though,
> given it also adds and uses a whole new kunit_defer() API.

Thanks for letting me know. I did also prepare this commit yesterday:
https://github.com/M-Vaittinen/linux/commit/b784a90f8cc64ff83e802ec818e662fae1d0c264

It does use the existing kunit resources for clean-up. I am not sure if 
it is worth a shot or should I just drop it and use the root-device API 
for now. Any educated opinions on that? :)

Yours,
	-- Matti