by Matti Vaittinen

[permalink] [raw]

2023-03-23 10:18:55

On Thu, Mar 23, 2023 at 02:16:52PM +0200, Matti Vaittinen wrote:
> Hi Maxime, all
>
> On 3/23/23 12:21, Greg Kroah-Hartman wrote:
> > On Thu, Mar 23, 2023 at 11:12:16AM +0100, Maxime Ripard wrote:
> > > On Wed, Mar 22, 2023 at 07:57:10PM +0100, Greg Kroah-Hartman wrote:
> > > > > > > +/**
> > > > > > > + * test_kunit_helper_alloc_device - Allocate a mock device for a KUnit test
> > > > > > > + * @test: The test context object
> > > > > > > + *
> > > > > > > + * This allocates a fake struct &device to create a mock for a KUnit
> > > > > > > + * test. The device will also be bound to a fake driver. It will thus be
> > > > > > > + * able to leverage the usual infrastructure and most notably the
> > > > > > > + * device-managed resources just like a "real" device.
> > > > > >
> > > > > > What specific "usual infrastructure" are you wanting to access here?
> > > > > >
> > > > > > And again, if you want a fake device, make a virtual one, by just
> > > > > > calling device_create().
> > > > > >
> > > > > > Or are you wanting to do "more" with that device pointer than
> > > > > > device_create() can give you?
> > > > >
> > > > > Personally, I was (am) only interested in devm_ unwinding. I guess the
> > > > > device_create(), device_add(), device_remove()... (didn't study this
> > > > > sequence in details so sorry if there is errors) could've been sufficient
> > > > > for me. I haven't looked how much of the code that there is for 'platform
> > > > > devices' should be duplicated to support that sequence for testability
> > > > > purposes.
> > > >
> > > > Any device can access devm_ code, there's no need for it to be a
> > > > platform device at all.
> > >
> > > Sure but the resources are only released if the device is part of a bus,
> > > so it can't be a root_device (or bare device) either
> >
> > The resources are not cleaned up when the device is freed no matter if
> > it's on a bus or not? If so, then that's a bug that needs to be fixed,
> > and tested :)
>
> This is strange. I just ran a test on a beaglebone black using Linux
> 6.3.0-rc2 + the IIO patches we se here (but the IIO test patch modified to
> use the root_device_register() and root_device_unregister().
>
> I passed the device pointer from root_device_register() to the
> devm_iio_init_iio_gts()
>
> // snip
> dev = root_device_register(IIO_GTS_TEST_DEV);
> KUNIT_EXPECT_NOT_ERR_OR_NULL(test, dev);
> if (IS_ERR_OR_NULL(dev))
> return NULL;
>
> ret = devm_iio_init_iio_gts(dev, TEST_SCALE_1X, 0, g_table, num_g,
> i_table, num_i, gts);
>
> - and saw the tables for available scales allocated:
>
> if (gts.num_avail_all_scales)
> pr_info("GTS: table allocation succeeded\n");
> else
> pr_info("GTS: table allocation failed\n");
>
> pr_info("gts: num_avail_all_scales %d\n", gts.num_avail_all_scales);
>
> (this printed:
> [ 52.132966] # Subtest: iio-gain-time-scale
> [ 52.132982] 1..7
> [ 52.157455] GTS: table allocation succeeded
> [ 52.164077] gts: num_avail_all_scales 16
>
> Next I unregister the root-device and check if the unwinding code which
> frees the tables and zeroes the scale count was ran:
>
> root_device_unregister(dev);
> pr_info("gts: num_avail_all_scales %d\n", gts.num_avail_all_scales);
>
> if (gts.num_avail_all_scales)
> pr_info("devm unwinding not done\n");
> else
> pr_info("devm unwinding succeeded\n");
>
> Which printed:
> [ 52.168101] gts: num_avail_all_scales 0
> [ 52.171957] devm unwinding succeeded
>
> I can send patch(es) just for testing this on other machines if someone
> want's to see if the lack of devm unwinding is somehow architecture specific
> (which sounds very strange to me) - although using this IIO series just for
> checking the unwinding is a bit of an overkill. I just happened to have
> these tests at my hands / in my tree for testing.
>
> In any case, devm unwinding using root_device_[un]register() seems to "work
> on my machine".
>
> Naxime, what was the environment where you observed lack of unwinding? (Huh,
> I am so afraid of sending this post out - I've experienced too many "Oh, boy
> - how I didn't notice THAT" moments in the past and maybe I am again
> overlooking something...)

This is the description of what was happening:
https://lore.kernel.org/dri-devel/20221117165311.vovrc7usy4efiytl@houat/

Maxime

Attachments:

(No filename) (4.51 kB)
signature.asc (235.00 B)
Download all attachments

2023-03-23 13:26:10

by Matti Vaittinen

[permalink] [raw]

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

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?

If this is the case, then it sounds like a DRM specific issue to me.
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?

Well, It's almost the best season for ice-fishing in Finland so opening
a can of worms is not that bad, right? :)

Thanks for the education people! I did learn a thing or two Today.

Yours,
-- Matti

--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

2023-03-23 16:40:55

by Maxime Ripard

[permalink] [raw]

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

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.

> 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'm not sure how that helps. Those are
common helpers which should accommodate every framework, and your second
patch breaks the kunit tests for DRM anyway.

> 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.

Maxime

Attachments:

(No filename) (2.53 kB)
signature.asc (235.00 B)
Download all attachments

2023-03-24 06:14:56

by Matti Vaittinen

[permalink] [raw]

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

On Fri, 24 Mar 2023 at 18:17, Matti Vaittinen <[email protected]> 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
> >> <[email protected]> wrote:
> >>>
> >>> On 3/24/23 08:34, David Gow wrote:
> >>>> On Fri, 24 Mar 2023 at 14:11, Matti Vaittinen
> >>>> <[email protected]> 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/[email protected]/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

Attachments:

smime.p7s (3.91 kB)
S/MIME Cryptographic Signature

2023-03-25 05:59:56

On Sat, Mar 25, 2023 at 05:50:44PM +0000, Jonathan Cameron wrote:
> On Fri, 24 Mar 2023 13:31:57 +0100
> Maxime Ripard <[email protected]> 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.
>
> Lars-Peter Clausen (IIRC) fixed up the IIO handling of the similar cases a
> long time ago now. It's simpler that for some other subsystems as we don't
> have as many interdependencies as occur in DRM etc.
>
> I 'think' we are fine in general with the IIO approach to this (I think we
> did have one report of a theoretical race condition in the remove path that
> was never fully addressed).
>
> For IIO we also have fds that can be open but all accesses to them are proxied
> through the IIO core and one of the things iio_device_unregister() or the devm
> equivalent does is to set indio_dev->info = NULL (+ wake up anyone waiting on
> data etc). Alongside removing the callbacks, that is also used as a flag
> to indicate the device has gone.

Sorry if it came as trying to put IIO under a bad light, it certainly
wasn't my intention. I was trying to come up with possible explanations
as to why IIO's design was simpler than DRM is :)

> Note that we keep a reference to the struct indio_dev->dev (rather that the
> underlying device) so that is not freed until the last fd is closed.
> Thus, although devm unwinding has occurred that doesn't mean all the data
> that was allocated with devm_xx calls is cleared up immediately.

I'm not sure I get that part though. devm unwinding can happen even if the refcount is > 1

Maxime

Attachments:

(No filename) (4.50 kB)
signature.asc (235.00 B)
Download all attachments

2023-04-01 15:16:34

by Jonathan Cameron

[permalink] [raw]

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

On Sun, 26 Mar 2023 10:16:54 -0700
Lars-Peter Clausen <[email protected]> wrote:

> On 3/25/23 10:50, Jonathan Cameron wrote:
> > On Fri, 24 Mar 2023 13:31:57 +0100
> > Maxime Ripard <[email protected]> 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.
> > Lars-Peter Clausen (IIRC) fixed up the IIO handling of the similar cases a
> > long time ago now. It's simpler that for some other subsystems as we don't
> > have as many interdependencies as occur in DRM etc.
> >
> > I 'think' we are fine in general with the IIO approach to this (I think we
> > did have one report of a theoretical race condition in the remove path that
> > was never fully addressed).
> >
> > For IIO we also have fds that can be open but all accesses to them are proxied
> > through the IIO core and one of the things iio_device_unregister() or the devm
> > equivalent does is to set indio_dev->info = NULL (+ wake up anyone waiting on
> > data etc). Alongside removing the callbacks, that is also used as a flag
> > to indicate the device has gone.
> >
> > Note that we keep a reference to the struct indio_dev->dev (rather that the
> > underlying device) so that is not freed until the last fd is closed.
> > Thus, although devm unwinding has occurred that doesn't mean all the data
> > that was allocated with devm_xx calls is cleared up immediately.
>
> IIO is fully hot-plug and hot-unplug capable. And it will have the same
> issue. When using managed device registration that establishes a parent
> child relationship between the devices and in combination with a device
> where the managed unwinding does not happen on unbind, but rather on in
> the release callback you create a cyclic reference dependency. The child
> device holds a reference to the parent, but the reference is only
> released in the parents release callback. And since that release
> callback is not called until the last reference is dropped you end up
> with a resource leak.
>
> There are even some other places where IIO drivers run into this. E.g.
> any driver that does `devm_iio_trigger_register(&indio_dev->dev, ...)`

A driver should should not do that.
Should be devm_iio_trigger_registers(parent device, ...)

There is a corner where you need to detach the trigger from userspace
before release which is odd if it was attached by default. There are some
left over corners there that I think still cause trouble.

> creates a resource leak on the trigger and the IIO device. The indio_dev
> is not a bus device, hence no unbind and the trigger holds a reference
> so the release callback will never be called either.
>

2023-04-01 15:22:14

by Jonathan Cameron

[permalink] [raw]

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

On Wed, 29 Mar 2023 21:46:09 +0200
Maxime Ripard <[email protected]> wrote:

> On Sat, Mar 25, 2023 at 05:50:44PM +0000, Jonathan Cameron wrote:
> > On Fri, 24 Mar 2023 13:31:57 +0100
> > Maxime Ripard <[email protected]> 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.
> >
> > Lars-Peter Clausen (IIRC) fixed up the IIO handling of the similar cases a
> > long time ago now. It's simpler that for some other subsystems as we don't
> > have as many interdependencies as occur in DRM etc.
> >
> > I 'think' we are fine in general with the IIO approach to this (I think we
> > did have one report of a theoretical race condition in the remove path that
> > was never fully addressed).
> >
> > For IIO we also have fds that can be open but all accesses to them are proxied
> > through the IIO core and one of the things iio_device_unregister() or the devm
> > equivalent does is to set indio_dev->info = NULL (+ wake up anyone waiting on
> > data etc). Alongside removing the callbacks, that is also used as a flag
> > to indicate the device has gone.
>
> Sorry if it came as trying to put IIO under a bad light, it certainly
> wasn't my intention. I was trying to come up with possible explanations
> as to why IIO's design was simpler than DRM is :)

No problem :) I'm sure there are gremlins hiding there.
Part of the problem is that nothing prevents drivers doing 'wrong things'
other than us noticing when it happens.

>
> > Note that we keep a reference to the struct indio_dev->dev (rather that the
> > underlying device) so that is not freed until the last fd is closed.
> > Thus, although devm unwinding has occurred that doesn't mean all the data
> > that was allocated with devm_xx calls is cleared up immediately.
>
> I'm not sure I get that part though. devm unwinding can happen even if the refcount is > 1

No IIO driver should be using devm on the indio_dev->dev, they should be doing it on the
parent device. When the devm_iio_device_free() gets called, that doesn't actually free
the device. Just decrements a reference count (earlier on we already ensured that
it is just acting as a stub that provides no access to the underlying device for
open FDs.).

There are probably more problems hiding though!

Jonathan

>
> Maxime