Message-ID: <CABVgOSn8H=9pQfY7Exc-e37Nm6u299AJLYup6R-_97v5Fb4bpQ@mail.gmail.com>
Date:   Sat, 25 Mar 2023 13:40:01 +0800
From:   David Gow <davidgow@...gle.com>
To:     Maxime Ripard <maxime@...no.tech>
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, 24 Mar 2023 at 20:32, Maxime Ripard <maxime@...no.tech> wrote:
>
> On Fri, Mar 24, 2023 at 08:11:52AM +0200, Matti Vaittinen 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 :)
> >
> > >
> > > > 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.
>
> It depends really. The issue DRM is trying to solve is that, when a
> device is gone, some application might still have an open FD and could
> still poke into the kernel, while all the resources would have been
> free'd if it was using devm.
>
> So everything is kept around until the last fd is closed, so you still
> have a reference to the device (even though it's been removed from its
> bus) until that time.
>
> It could be possible that IIO just doesn't handle that case at all. I
> guess most of the devices aren't hotpluggable, and there's not much to
> interact with from a userspace PoV iirc, so it might be why.
>
> > > 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.
> >
> > > 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.
>
> So, I went around and created a bunch of kunit tests that shows the
> problem without DRM being involved at all.
>
> It does three things:
>
>  - It registers a device, attaches a devm action, unregisters the device
>    and then checks that the action has ran.
>
>  - It registers a device, gets a reference to it, attaches a devm
>    action, puts back the reference, unregisters the device and then
>    checks that the action has ran.
>
>  - It registers a device, gets a reference to it, attaches a devm action
>    that will put back the reference, unregisters the device and then
>    checks that the action has ran.
>
> And in three cases: first with a root_device, then platform_device, then
> a platform_device that has been bound to a driver.
>
> Once you've applied that patch, you can run it using:
>
> ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/base/test/ devm-inconsistencies
>
> You'll see that only the last case passes all the tests, even though the
> code itself is exactly the same.
>

This illustrates the problem very nicely, thanks.

I played around a bit with this, and I'm definitely leaning towards
this being a bug, rather than intentional behaviour. There's actually
an explicit call to devres_release_all() in device_release() which
seems to suggest that this should work:
/*
* Some platform devices are driven without driver attached
* and managed resources may have been acquired.  Make sure
* all resources are released.
*
* Drivers still can add resources into device after device
* is deleted but alive, so release devres here to avoid
* possible memory leak.
*/

My "solution" is just to call devres_release_all() in device_del() as
well, which fixes your tests (and the drm-test-managed one when ported
to use root_device_register() or my kunit_device_register() API[1]).

--8<--
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 6878dfcbf0d6..adfec6185014 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3778,6 +3778,17 @@ void device_del(struct device *dev)
       device_platform_notify_remove(dev);
       device_links_purge(dev);

+       /*
+        * If a device does not have a driver attached, we need to clean up any
+        * managed resources. We do this in device_release(), but it's never
+        * called (and we leak the device) if a managed resource holds a
+        * reference to the device. So release all managed resources here,
+        * like we do in driver_detach(). We still need to do so again in
+        * device_release() in case someone adds a new resource after this
+        * point, though.
+        */
+       devres_release_all(dev);
+
       bus_notify(dev, BUS_NOTIFY_REMOVED_DEVICE);
       kobject_uevent(&dev->kobj, KOBJ_REMOVE);
       glue_dir = get_glue_dir(dev);

-->8--

It doesn't _seem_ to break anything else, and I've managed to convince
myself that it's probably the correct fix. (Albeit, as someone with a
limited knowledge of this part of the code, who also hasn't had quite
enough sleep, so take that with some salt.)

Still, I'd agree with Greg that it'd be great to have versions of your
tests upstream before making any such radical changes.

Cheers,
-- David

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4003 bytes)

Powered by blists - more mailing lists