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

[Maxime Ripard <maxime@cerno.tech>]

On Fri, Mar 24, 2023 at 02:34:19PM +0800, 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:
> > >>>
> > >>> 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.

Yeah, like I said you need a device that has been bound to a driver for
it to work at the moment.

I guess for driver mocks we could move to a setup where we get
kunit-specific drivers like what Stephen has been implementing for the
clocks but I guess we would need to register the kunit tests in the
driver probe which doesn't look like it's possible at the moment?

Maxime