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

[David Gow]

On Fri, 24 Mar 2023 at 14:11, Matti Vaittinen <mazziesaccount@xxxxxxxxx> 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

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature