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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 24 Mar 2023 13:46:48 +0100
From:   Maxime Ripard <maxime@...no.tech>
To:     David Gow <davidgow@...gle.com>
Cc:     Matti Vaittinen <mazziesaccount@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Brendan Higgins <brendan.higgins@...ux.dev>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
        kunit-dev@...glegroups.com, Stephen Boyd <sboyd@...nel.org>,
        Jonathan Cameron <jic23@...nel.org>, linux-iio@...r.kernel.org
Subject: Re: [PATCH v5 1/8] drivers: kunit: Generic helpers for test device
 creation

On Fri, Mar 24, 2023 at 02:34:19PM +0800, David Gow wrote:
> On Fri, 24 Mar 2023 at 14:11, Matti Vaittinen <mazziesaccount@...il.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