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

  The creation of a dummy device in order to test managed interfaces is a
generally useful test feature. The drm test helpers
drm_kunit_helper_alloc_device() and drm_kunit_helper_free_device()
are doing exactly this. It makes no sense that each and every component
which intends to be testing managed interfaces will create similar
helpers so stole these for more generic use.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

---
Changes:
v4 => v5:
- Add accidentally dropped header and email recipients
- do not rename + move helpers from DRM but add temporary dublicates to
  simplify merging. (This patch does not touch DRM and can be merged
  separately. DRM patch and IIO test patch still depend on this one).

Please note that there's something similar ongoing in the CCF:
https://lore.kernel.org/all/20230302013822.1808711-1-sboyd@kernel.org/

I do like the simplicity of these DRM-originated helpers so I think
they're worth. I do equally like the Stephen's idea of having the
"dummy platform device" related helpers under drivers/base and the
header being in include/kunit/platform_device.h which is similar to real
platform device under include/linux/platform_device.h
---
 drivers/base/test/Kconfig             |  5 ++
 drivers/base/test/Makefile            |  2 +
 drivers/base/test/test_kunit_device.c | 83 +++++++++++++++++++++++++++
 include/kunit/platform_device.h       | 13 +++++
 4 files changed, 103 insertions(+)
 create mode 100644 drivers/base/test/test_kunit_device.c
 create mode 100644 include/kunit/platform_device.h
  

On Wed, Mar 22, 2023 at 11:05:55AM +0200, Matti Vaittinen wrote:
> --- /dev/null
> +++ b/include/kunit/platform_device.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef __KUNIT_PLATFORM_DEVICE__
> +#define __KUNIT_PLATFORM_DEVICE__
> +
> +#include <kunit/test.h>
> +
> +struct device;
> +
> +struct device *test_kunit_helper_alloc_device(struct kunit *test);
> +void test_kunit_helper_free_device(struct kunit *test, struct device *dev);

Why are you calling this a "platform_device" when it isn't a platform
device at all?

Why not just say "device.h" here?

thanks,

greg k-h
  

On Wed, Mar 22, 2023 at 11:05:55AM +0200, Matti Vaittinen wrote:
> --- /dev/null
> +++ b/drivers/base/test/test_kunit_device.c
> @@ -0,0 +1,83 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * These helpers have been extracted from drm test code at
> + * drm_kunit_helpers.c which was authored by
> + * Maxime Ripard <maxime@cerno.tech>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +
> +#include <kunit/platform_device.h>
> +
> +#define KUNIT_DEVICE_NAME	"test-kunit-mock-device"
> +
> +static int fake_probe(struct platform_device *pdev)

Please do not abuse platform devices and drivers for things that are not
actually platform devices and drivers.

> +{
> +	return 0;
> +}
> +
> +static int fake_remove(struct platform_device *pdev)
> +{
> +	return 0;
> +}
> +
> +static struct platform_driver fake_platform_driver = {
> +	.probe	= fake_probe,
> +	.remove	= fake_remove,
> +	.driver = {
> +		.name	= KUNIT_DEVICE_NAME,
> +	},
> +};

Why do you need this fake platform driver at all?

Why not just use a virtual device?

> +
> +/**
> + * 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?

Again, please do not abuse the platform device infrastructure for things
it was never ment to do (i.e. create fake devices that are not really a
platform device.)

thanks,

greg k-h
  

On Wed, Mar 22, 2023 at 11:05:55AM +0200, Matti Vaittinen wrote:
> The creation of a dummy device in order to test managed interfaces is a
> generally useful test feature. The drm test helpers
> drm_kunit_helper_alloc_device() and drm_kunit_helper_free_device()
> are doing exactly this. It makes no sense that each and every component
> which intends to be testing managed interfaces will create similar
> helpers so stole these for more generic use.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
> ---
> Changes:
> v4 => v5:
> - Add accidentally dropped header and email recipients
> - do not rename + move helpers from DRM but add temporary dublicates to
>   simplify merging. (This patch does not touch DRM and can be merged
>   separately. DRM patch and IIO test patch still depend on this one).
> 
> Please note that there's something similar ongoing in the CCF:
> https://lore.kernel.org/all/20230302013822.1808711-1-sboyd@kernel.org/
> 
> I do like the simplicity of these DRM-originated helpers so I think
> they're worth. I do equally like the Stephen's idea of having the
> "dummy platform device" related helpers under drivers/base and the
> header being in include/kunit/platform_device.h which is similar to real
> platform device under include/linux/platform_device.h
> ---
>  drivers/base/test/Kconfig             |  5 ++
>  drivers/base/test/Makefile            |  2 +
>  drivers/base/test/test_kunit_device.c | 83 +++++++++++++++++++++++++++

By putting files in this directory, you are asking me to maintain this
code, and right now, I can't agree with that, sorry.  See my review
comments on it for why.

thanks,

greg k-h
  

Hi Greg,

Thanks for looking at this.

On 3/22/23 14:07, Greg Kroah-Hartman wrote:
> On Wed, Mar 22, 2023 at 11:05:55AM +0200, Matti Vaittinen wrote:
>> --- /dev/null
>> +++ b/drivers/base/test/test_kunit_device.c
>> @@ -0,0 +1,83 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * These helpers have been extracted from drm test code at
>> + * drm_kunit_helpers.c which was authored by
>> + * Maxime Ripard <maxime@cerno.tech>
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/platform_device.h>
>> +
>> +#include <kunit/platform_device.h>
>> +
>> +#define KUNIT_DEVICE_NAME	"test-kunit-mock-device"
>> +
>> +static int fake_probe(struct platform_device *pdev)
> 
> Please do not abuse platform devices and drivers for things that are not
> actually platform devices and drivers.
> 
>> +{
>> +	return 0;
>> +}
>> +
>> +static int fake_remove(struct platform_device *pdev)
>> +{
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver fake_platform_driver = {
>> +	.probe	= fake_probe,
>> +	.remove	= fake_remove,
>> +	.driver = {
>> +		.name	= KUNIT_DEVICE_NAME,
>> +	},
>> +};
> 
> Why do you need this fake platform driver at all?
> 
> Why not just use a virtual device?

I can only answer on my behalf. In my case the answer to why I used 
platform_devices is practicality. I wanted to test devm_ APIs using 
KUnit tests and I was pointed to an existing implementation in DRM (seen 
in these patches). It didn't seem to make any sense to re-invent the 
wheel by writing another implementation for the existing in-tree 
functionality.

Maybe Maxime had a better reason to go with the platform devices.

>> +/**
>> + * 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.

The biggest thing for me is that I don't like the idea of creating own 
'test device' in <add subsystem here> while we already have some in DRM 
(or others). Thus, I do see value in adding generic helpers for 
supporting running KUnit tests on devm_* APIs. Hence it'd be good to 
have _some_ support for it. And having them in drivers/base/test seemed 
like a correct place to me. What I really don't know is if there are 
legitimate use-cases for using platform_devices in DRM tests. Perhaps 
Maxime can shed light on that.

Yours,
	-- Matti
  

On Wed, Mar 22, 2023 at 03:48:00PM +0200, Matti Vaittinen wrote:
> Hi Greg,
> 
> Thanks for looking at this.
> 
> On 3/22/23 14:07, Greg Kroah-Hartman wrote:
> > On Wed, Mar 22, 2023 at 11:05:55AM +0200, Matti Vaittinen wrote:
> > > --- /dev/null
> > > +++ b/drivers/base/test/test_kunit_device.c
> > > @@ -0,0 +1,83 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * These helpers have been extracted from drm test code at
> > > + * drm_kunit_helpers.c which was authored by
> > > + * Maxime Ripard <maxime@cerno.tech>
> > > + */
> > > +
> > > +#include <linux/device.h>
> > > +#include <linux/platform_device.h>
> > > +
> > > +#include <kunit/platform_device.h>
> > > +
> > > +#define KUNIT_DEVICE_NAME	"test-kunit-mock-device"
> > > +
> > > +static int fake_probe(struct platform_device *pdev)
> > 
> > Please do not abuse platform devices and drivers for things that are not
> > actually platform devices and drivers.
> > 
> > > +{
> > > +	return 0;
> > > +}
> > > +
> > > +static int fake_remove(struct platform_device *pdev)
> > > +{
> > > +	return 0;
> > > +}
> > > +
> > > +static struct platform_driver fake_platform_driver = {
> > > +	.probe	= fake_probe,
> > > +	.remove	= fake_remove,
> > > +	.driver = {
> > > +		.name	= KUNIT_DEVICE_NAME,
> > > +	},
> > > +};
> > 
> > Why do you need this fake platform driver at all?
> > 
> > Why not just use a virtual device?
> 
> I can only answer on my behalf. In my case the answer to why I used
> platform_devices is practicality. I wanted to test devm_ APIs using KUnit
> tests and I was pointed to an existing implementation in DRM (seen in these
> patches). It didn't seem to make any sense to re-invent the wheel by writing
> another implementation for the existing in-tree functionality.

That's fine, but please, let's do this right if it's going to be in the
driver core, that way we can actually test the driver core code as well.

> > > +/**
> > > + * 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.

> The biggest thing for me is that I don't like the idea of creating own 'test
> device' in <add subsystem here> while we already have some in DRM (or
> others). Thus, I do see value in adding generic helpers for supporting
> running KUnit tests on devm_* APIs. Hence it'd be good to have _some_
> support for it.

I agree, let's use a virtual device and a virtual bus (you can use the
auxbus code for this as that's all there for this type of thing) and
then you can test the devm_* code, _AND_ you can test the driver core at
the same time.

> And having them in drivers/base/test seemed like a correct
> place to me. What I really don't know is if there are legitimate use-cases
> for using platform_devices in DRM tests. Perhaps Maxime can shed light on
> that.

I agree that this could be in drivers/base/test/ but then let's test the
driver core, not just provide a dummy platform device.

If you want to test the platform driver/device api, that would be great
too, that can be plaform device/driver specific, but don't use one for
some other random driver core functionality please.

thanks,

greg k-h
  

On 3/22/23 20:57, Greg Kroah-Hartman wrote:
> On Wed, Mar 22, 2023 at 03:48:00PM +0200, Matti Vaittinen wrote:
>> Hi Greg,
>>
>> Thanks for looking at this.
>>
>> On 3/22/23 14:07, Greg Kroah-Hartman wrote:
>>> On Wed, Mar 22, 2023 at 11:05:55AM +0200, Matti Vaittinen 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.
> 
>> The biggest thing for me is that I don't like the idea of creating own 'test
>> device' in <add subsystem here> while we already have some in DRM (or
>> others). Thus, I do see value in adding generic helpers for supporting
>> running KUnit tests on devm_* APIs. Hence it'd be good to have _some_
>> support for it.
> 
> I agree, let's use a virtual device and a virtual bus (you can use the
> auxbus code for this as that's all there for this type of thing)

Hm. The auxiliary_devices require parent. What would be the best way to 
deal with that in KUnit tests?

> then you can test the devm_* code, _AND_ you can test the driver core at
> the same time.
> 
>> And having them in drivers/base/test seemed like a correct
>> place to me. What I really don't know is if there are legitimate use-cases
>> for using platform_devices in DRM tests. Perhaps Maxime can shed light on
>> that.
> 
> I agree that this could be in drivers/base/test/ but then let's test the
> driver core, not just provide a dummy platform device.
> 
> If you want to test the platform driver/device api, that would be great
> too, that can be plaform device/driver specific, but don't use one for
> some other random driver core functionality please.

I am very conservative what comes to adding unit tests due to the huge 
inertia they add to any further development. I usually only add tests to 
APIs which I know won't require changing (I don't know such in-kernel 
APIs) - or to functions which I think are error-prone. So, I am probably 
one of the last persons adding UTs to code I don't know :)

Yours,
	-- Matti

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

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

On Wed, 22 Mar 2023 at 17:06, Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>
> The creation of a dummy device in order to test managed interfaces is a
> generally useful test feature. The drm test helpers
> drm_kunit_helper_alloc_device() and drm_kunit_helper_free_device()
> are doing exactly this. It makes no sense that each and every component
> which intends to be testing managed interfaces will create similar
> helpers so stole these for more generic use.
>
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>
> ---
> Changes:
> v4 => v5:
> - Add accidentally dropped header and email recipients
> - do not rename + move helpers from DRM but add temporary dublicates to
>   simplify merging. (This patch does not touch DRM and can be merged
>   separately. DRM patch and IIO test patch still depend on this one).
>
> Please note that there's something similar ongoing in the CCF:
> https://lore.kernel.org/all/20230302013822.1808711-1-sboyd@kernel.org/
>
> I do like the simplicity of these DRM-originated helpers so I think
> they're worth. I do equally like the Stephen's idea of having the
> "dummy platform device" related helpers under drivers/base and the
> header being in include/kunit/platform_device.h which is similar to real
> platform device under include/linux/platform_device.h
> ---

Thanks for sending this my way.

It's clear we need some way of creating "fake" devices for KUnit
tests. Given that there are now (at least) three different drivers
looking to use this, we'll ultimately need something which works for
everyone.

I think the questions we therefore need to answer are:
- Do we specifically need a platform_device (or, any other specific
device struct), or would any old struct device work? I can see why we
would need a platform device for cases where we're testing things like
device-tree properties (or, in the future, having e.g. USB-specific
helpers which create usb_device). Most tests just use
root_device_register() thus far, though.
- What helpers do we need to make creating, using, and cleaning up
these devices as simple as possible.

I think that in this particular case, we don't actually need a struct
platform_device. Replacing these helpers with simple calls to
root_device_register() and root_device_unregister() seems to work fine
with this patch series for me. (It does break the
drm_test_managed_run_action test, though.) So I don't think having
these helpers actually help this series at the moment.

That being said, if they used the KUnit resource system to
automatically clean up the device when the test is finished (or
otherwise exits), that would seem like a real advantage. The clk and
drm examples both do this, and I'm hoping to add an API to make it
even simpler going forward. (With the work-in-progress API described
here[1], the whole thing should boil down to "struct device *dev =
root_device_register(name); kunit_defer(root_device_unregister,
dev);".)

So, I guess we have three cases we need to look at:
- A test just needs any old struct device. Tests thus far have
hardcoded, or had their own wrappers around root_device_register() for
this.
- A test needs a device attached to a bus (but doesn't care which
bus). Thus far, people have used struct platform_device for this (see
the DRM helpers, which use a platform device for managed resource
tests[2]). Maybe the right solution here is something like a struct
kunit_device?
- A test needs a device attached to a specific bus. We'll probably
need some more detailed faking of that bus. This covers cases like
having fake USB devices, devicetree integration, etc.

I'd suggest that, for the majority of cases which only care about the
first case, let's just use root_device_register() directly, or have a
thin wrapper like the old root_device-based version of the DRM
helpers[3]. This will probable serve us well enough while we work out
how to handle the other two cases properly (which is already being
looked at for the CLK/DeviceTree patches and the DRM stuff).

If the resulting helpers are generally useful enough, they can
probably sit in either drivers/base or lib/kunit. I'd rather not have
code that's really specific to certain busses sitting in lib/kunit
rather than alongside the device/bus code in drivers/base or some
other subsystem/driver path, but I can tolerate it for the very
generic struct device.

Regardless, I've left a few notes on the patch itself below.

Cheers,
-- David

[1]: https://kunit-review.googlesource.com/c/linux/+/5434/3/include/kunit/resource.h
[2]: https://lore.kernel.org/all/20221123-rpi-kunit-tests-v3-8-4615a663a84a@cerno.tech/
[3]: https://elixir.bootlin.com/linux/v6.2/source/drivers/gpu/drm/tests/drm_kunit_helpers.c#L39
>  drivers/base/test/Kconfig             |  5 ++
>  drivers/base/test/Makefile            |  2 +
>  drivers/base/test/test_kunit_device.c | 83 +++++++++++++++++++++++++++
>  include/kunit/platform_device.h       | 13 +++++
>  4 files changed, 103 insertions(+)
>  create mode 100644 drivers/base/test/test_kunit_device.c
>  create mode 100644 include/kunit/platform_device.h
>
> diff --git a/drivers/base/test/Kconfig b/drivers/base/test/Kconfig
> index 610a1ba7a467..642b5b183c10 100644
> --- a/drivers/base/test/Kconfig
> +++ b/drivers/base/test/Kconfig
> @@ -1,4 +1,9 @@
>  # SPDX-License-Identifier: GPL-2.0
> +
> +config TEST_KUNIT_DEVICE_HELPERS
> +       depends on KUNIT
> +       tristate
> +
>  config TEST_ASYNC_DRIVER_PROBE
>         tristate "Build kernel module to test asynchronous driver probing"
>         depends on m
> diff --git a/drivers/base/test/Makefile b/drivers/base/test/Makefile
> index 7f76fee6f989..49926524ec6f 100644
> --- a/drivers/base/test/Makefile
> +++ b/drivers/base/test/Makefile
> @@ -1,5 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_TEST_ASYNC_DRIVER_PROBE)  += test_async_driver_probe.o
>
> +obj-$(CONFIG_TEST_KUNIT_DEVICE_HELPERS) += test_kunit_device.o
> +
>  obj-$(CONFIG_DRIVER_PE_KUNIT_TEST) += property-entry-test.o
>  CFLAGS_property-entry-test.o += $(DISABLE_STRUCTLEAK_PLUGIN)
> diff --git a/drivers/base/test/test_kunit_device.c b/drivers/base/test/test_kunit_device.c
> new file mode 100644
> index 000000000000..75790e15b85c
> --- /dev/null
> +++ b/drivers/base/test/test_kunit_device.c
> @@ -0,0 +1,83 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * These helpers have been extracted from drm test code at
> + * drm_kunit_helpers.c which was authored by
> + * Maxime Ripard <maxime@cerno.tech>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +
> +#include <kunit/platform_device.h>
> +
> +#define KUNIT_DEVICE_NAME      "test-kunit-mock-device"

Personally, I'd really rather this be a name passed in by the test.
What if a test needs to create multiple distinct devices?

> +
> +static int fake_probe(struct platform_device *pdev)
> +{
> +       return 0;
> +}
> +
> +static int fake_remove(struct platform_device *pdev)
> +{
> +       return 0;
> +}
> +
> +static struct platform_driver fake_platform_driver = {
> +       .probe  = fake_probe,
> +       .remove = fake_remove,
> +       .driver = {
> +               .name   = KUNIT_DEVICE_NAME,
> +       },
> +};
> +
> +/**
> + * 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.
> + *
> + * Callers need to make sure test_kunit_helper_free_device() on the
> + * device when done.
> + *
> + * Returns:
> + * A pointer to the new device, or an ERR_PTR() otherwise.
> + */
> +struct device *test_kunit_helper_alloc_device(struct kunit *test)
> +{
> +       struct platform_device *pdev;
> +       int ret;
> +
> +       ret = platform_driver_register(&fake_platform_driver);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       pdev = platform_device_alloc(KUNIT_DEVICE_NAME, PLATFORM_DEVID_NONE);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
> +
> +       ret = platform_device_add(pdev);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       return &pdev->dev;
> +}
> +EXPORT_SYMBOL_GPL(test_kunit_helper_alloc_device);
> +
> +/**
> + * test_kunit_helper_free_device - Frees a mock device
> + * @test: The test context object
> + * @dev: The device to free
> + *
> + * Frees a device allocated with test_kunit_helper_alloc_device().
> + */

This really should be automatically called when the test exits,
probably using kunit reources. Ideally, there'd also be a function to
free it earlier, which can be done by calling kunit_remove_resource()
to lower the refcount.

> +void test_kunit_helper_free_device(struct kunit *test, struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +
> +       platform_device_unregister(pdev);
> +       platform_driver_unregister(&fake_platform_driver);
> +}
> +EXPORT_SYMBOL_GPL(test_kunit_helper_free_device);
> +
> +MODULE_AUTHOR("Matti Vaittinen <mazziesaccount@gmail.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/kunit/platform_device.h b/include/kunit/platform_device.h
> new file mode 100644
> index 000000000000..2a9c7bdd75eb
> --- /dev/null
> +++ b/include/kunit/platform_device.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef __KUNIT_PLATFORM_DEVICE__
> +#define __KUNIT_PLATFORM_DEVICE__
> +
> +#include <kunit/test.h>
> +
> +struct device;
> +
> +struct device *test_kunit_helper_alloc_device(struct kunit *test);
> +void test_kunit_helper_free_device(struct kunit *test, struct device *dev);

If these helpers are supposed to guarantee that the resulting device
is a platform device, let's reflect that in their names. Otherwise,
let's not put this in a platform_device.h header, but maybe something
more general, like kunit/device.h.

> +
> +#endif
> --
> 2.39.2
>
>
> --
> Matti Vaittinen, Linux device drivers
> ROHM Semiconductors, Finland SWDC
> Kiviharjunlenkki 1E
> 90220 OULU
> FINLAND
>
> ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
> Simon says - in Latin please.
> ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
> Thanks to Simon Glass for the translation =]
  

Hi David, all,

On 3/23/23 09:30, David Gow wrote:
> On Wed, 22 Mar 2023 at 17:06, Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>>
>> The creation of a dummy device in order to test managed interfaces is a
>> generally useful test feature. The drm test helpers
>> drm_kunit_helper_alloc_device() and drm_kunit_helper_free_device()
>> are doing exactly this. It makes no sense that each and every component
>> which intends to be testing managed interfaces will create similar
>> helpers so stole these for more generic use.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>
>> ---
>> Changes:
>> v4 => v5:
>> - Add accidentally dropped header and email recipients
>> - do not rename + move helpers from DRM but add temporary dublicates to
>>    simplify merging. (This patch does not touch DRM and can be merged
>>    separately. DRM patch and IIO test patch still depend on this one).
>>
>> Please note that there's something similar ongoing in the CCF:
>> https://lore.kernel.org/all/20230302013822.1808711-1-sboyd@kernel.org/
>>
>> I do like the simplicity of these DRM-originated helpers so I think
>> they're worth. I do equally like the Stephen's idea of having the
>> "dummy platform device" related helpers under drivers/base and the
>> header being in include/kunit/platform_device.h which is similar to real
>> platform device under include/linux/platform_device.h
>> ---
> 
> Thanks for sending this my way.
> 
> It's clear we need some way of creating "fake" devices for KUnit
> tests. Given that there are now (at least) three different drivers
> looking to use this, we'll ultimately need something which works for
> everyone.
> 
> I think the questions we therefore need to answer are:
> - Do we specifically need a platform_device (or, any other specific
> device struct), or would any old struct device work? I can see why we
> would need a platform device for cases where we're testing things like
> device-tree properties (or, in the future, having e.g. USB-specific
> helpers which create usb_device). Most tests just use
> root_device_register() thus far, though.

Funny timing. I just found the root_device_register() while wondering 
the parent for auxiliary_devices.

I think the root_device_[un]register() meets my current needs.

> - What helpers do we need to make creating, using, and cleaning up
> these devices as simple as possible.
> 
> I think that in this particular case, we don't actually need a struct
> platform_device. Replacing these helpers with simple calls to
> root_device_register() and root_device_unregister() seems to work fine
> with this patch series for me. (It does break the
> drm_test_managed_run_action test, though.) So I don't think having
> these helpers actually help this series at the moment.

I am afraid that further work with these helpers is out of the scope for 
me (at least for now). I'll drop the DRM and the helper patches from 
this series && go with the root_device_register(), 
root_device_unregister() in the IIO tests added in this series.

> That being said, if they used the KUnit resource system to
> automatically clean up the device when the test is finished (or
> otherwise exits),

My 10 cents to this is that yes, automatic unwinding at test exit would 
be simple - and also correct for most of the time. However, I think 
there might also be tests that would like to verify the unwinding 
process has really managed to do what it was intended to do. That, I 
think would require being able to manually drop the device in the middle 
of the test.

> So, I guess we have three cases we need to look at:
> - A test just needs any old struct device. Tests thus far have
> hardcoded, or had their own wrappers around root_device_register() for
> this.

As said above, my case fits this category.

> - A test needs a device attached to a bus (but doesn't care which
> bus). Thus far, people have used struct platform_device for this (see
> the DRM helpers, which use a platform device for managed resource
> tests[2]). Maybe the right solution here is something like a struct
> kunit_device?

This sounds like, how to put it, "architecturally correct". But...

> - A test needs a device attached to a specific bus. We'll probably
> need some more detailed faking of that bus. This covers cases like
> having fake USB devices, devicetree integration, etc.

...if we in any case need this, wouldn't the kunit_device just be 
unnecessary bloat because if the test does not care which bus it is 
sitting in, then it could probably use a bus-specific device as well?

> I'd suggest that, for the majority of cases which only care about the
> first case, let's just use root_device_register() directly,

After finding the root_device_register() - I agree.

> or have a
> thin wrapper like the old root_device-based version of the DRM
> helpers[3]. This will probable serve us well enough while we work out
> how to handle the other two cases properly (which is already being
> looked at for the CLK/DeviceTree patches and the DRM stuff).
> 
> If the resulting helpers are generally useful enough, they can
> probably sit in either drivers/base or lib/kunit. I'd rather not have
> code that's really specific to certain busses sitting in lib/kunit
> rather than alongside the device/bus code in drivers/base or some
> other subsystem/driver path, but I can tolerate it for the very
> generic struct device.
> 
> Regardless, I've left a few notes on the patch itself below.

Thanks but I guess I'll just drop this one :)

> 
> Cheers,
> -- David
> 
> [1]: https://kunit-review.googlesource.com/c/linux/+/5434/3/include/kunit/resource.h
> [2]: https://lore.kernel.org/all/20221123-rpi-kunit-tests-v3-8-4615a663a84a@cerno.tech/
> [3]: https://elixir.bootlin.com/linux/v6.2/source/drivers/gpu/drm/tests/drm_kunit_helpers.c#L39

Thanks for the thorough analysis and these links! This was enlightening :)

Yours,
	-- Matti
  

On Thu, Mar 23, 2023 at 07:17:40AM +0000, Vaittinen, Matti wrote:
> On 3/22/23 20:57, Greg Kroah-Hartman wrote:
> > On Wed, Mar 22, 2023 at 03:48:00PM +0200, Matti Vaittinen wrote:
> >> Hi Greg,
> >>
> >> Thanks for looking at this.
> >>
> >> On 3/22/23 14:07, Greg Kroah-Hartman wrote:
> >>> On Wed, Mar 22, 2023 at 11:05:55AM +0200, Matti Vaittinen 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.
> > 
> >> The biggest thing for me is that I don't like the idea of creating own 'test
> >> device' in <add subsystem here> while we already have some in DRM (or
> >> others). Thus, I do see value in adding generic helpers for supporting
> >> running KUnit tests on devm_* APIs. Hence it'd be good to have _some_
> >> support for it.
> > 
> > I agree, let's use a virtual device and a virtual bus (you can use the
> > auxbus code for this as that's all there for this type of thing)
> 
> Hm. The auxiliary_devices require parent. What would be the best way to 
> deal with that in KUnit tests?

If you use NULL as the parent, it goes into the root.

> > then you can test the devm_* code, _AND_ you can test the driver core at
> > the same time.
> > 
> >> And having them in drivers/base/test seemed like a correct
> >> place to me. What I really don't know is if there are legitimate use-cases
> >> for using platform_devices in DRM tests. Perhaps Maxime can shed light on
> >> that.
> > 
> > I agree that this could be in drivers/base/test/ but then let's test the
> > driver core, not just provide a dummy platform device.
> > 
> > If you want to test the platform driver/device api, that would be great
> > too, that can be plaform device/driver specific, but don't use one for
> > some other random driver core functionality please.
> 
> I am very conservative what comes to adding unit tests due to the huge 
> inertia they add to any further development. I usually only add tests to 
> APIs which I know won't require changing (I don't know such in-kernel 
> APIs)

So anything that is changing doesn't get a test?  If you only test
things that don't change then no tests fail, and so, why have the test
at all?

On the contrary, tests should be used to verify things that are changing
all the time, to ensure that we don't break things.  That's why we need
them, not to just validate that old code still is going ok.

The driver core is changing, and so, I would love to see tests for it to
ensure that I don't break anything over time.  That should NOT slow down
development but rather, speed it up as it ensures that things still work
properly.

> - or to functions which I think are error-prone. So, I am probably 
> one of the last persons adding UTs to code I don't know :)

That's fine, you don't have to add test code for stuff you don't know.

But again, do NOT abuse a platform device for this, that's not ok, and
the in-kernel code that does do this should be fixed up.

thanks,

greg k-h
  

On Thu, Mar 23, 2023 at 03:30:34PM +0800, David Gow wrote:
> On Wed, 22 Mar 2023 at 17:06, Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >
> > The creation of a dummy device in order to test managed interfaces is a
> > generally useful test feature. The drm test helpers
> > drm_kunit_helper_alloc_device() and drm_kunit_helper_free_device()
> > are doing exactly this. It makes no sense that each and every component
> > which intends to be testing managed interfaces will create similar
> > helpers so stole these for more generic use.
> >
> > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> >
> > ---
> > Changes:
> > v4 => v5:
> > - Add accidentally dropped header and email recipients
> > - do not rename + move helpers from DRM but add temporary dublicates to
> >   simplify merging. (This patch does not touch DRM and can be merged
> >   separately. DRM patch and IIO test patch still depend on this one).
> >
> > Please note that there's something similar ongoing in the CCF:
> > https://lore.kernel.org/all/20230302013822.1808711-1-sboyd@kernel.org/
> >
> > I do like the simplicity of these DRM-originated helpers so I think
> > they're worth. I do equally like the Stephen's idea of having the
> > "dummy platform device" related helpers under drivers/base and the
> > header being in include/kunit/platform_device.h which is similar to real
> > platform device under include/linux/platform_device.h
> > ---
> 
> Thanks for sending this my way.
> 
> It's clear we need some way of creating "fake" devices for KUnit
> tests. Given that there are now (at least) three different drivers
> looking to use this, we'll ultimately need something which works for
> everyone.
> 
> I think the questions we therefore need to answer are:
> - Do we specifically need a platform_device (or, any other specific
> device struct), or would any old struct device work? I can see why we
> would need a platform device for cases where we're testing things like
> device-tree properties (or, in the future, having e.g. USB-specific
> helpers which create usb_device). Most tests just use
> root_device_register() thus far, though.

Any struct device would work, so please do NOT use a platform device.

Use aux devices, or better yet, just a normal virtual device by calling
device_create().

> - What helpers do we need to make creating, using, and cleaning up
> these devices as simple as possible.

Becides what the driver core already provides you?  What exactly are you
wanting to do here?

> I think that in this particular case, we don't actually need a struct
> platform_device. Replacing these helpers with simple calls to
> root_device_register() and root_device_unregister() seems to work fine
> with this patch series for me. (It does break the
> drm_test_managed_run_action test, though.) So I don't think having
> these helpers actually help this series at the moment.

Why abuse root_device_*() for something that really is not a root device
in the system?  Why not use a virtual device?  Or better yet, a kunit
bus with devices on it that are just for testing?  That way you can
properly test the bus and driver and device code fully, which is what
you are implying is needed here.

> That being said, if they used the KUnit resource system to
> automatically clean up the device when the test is finished (or
> otherwise exits), that would seem like a real advantage. The clk and
> drm examples both do this, and I'm hoping to add an API to make it
> even simpler going forward. (With the work-in-progress API described
> here[1], the whole thing should boil down to "struct device *dev =
> root_device_register(name); kunit_defer(root_device_unregister,
> dev);".)
> 
> So, I guess we have three cases we need to look at:
> - A test just needs any old struct device. Tests thus far have
> hardcoded, or had their own wrappers around root_device_register() for
> this.

Again, make a kunit bus and devices, that might be "simplest" overall.

> - A test needs a device attached to a bus (but doesn't care which
> bus). Thus far, people have used struct platform_device for this (see
> the DRM helpers, which use a platform device for managed resource
> tests[2]). Maybe the right solution here is something like a struct
> kunit_device?

Yes.

> - A test needs a device attached to a specific bus. We'll probably
> need some more detailed faking of that bus. This covers cases like
> having fake USB devices, devicetree integration, etc.

Have your own bus.  No need to mess with USB or any real bus UNLESS you
want to actually test those busses, and if so, just create fake USB or
clk or whatever devices so that you can test them.

Or just rely on the testing that some busses have for their devices (USB
has this today and syzbot knows how to test it), as busses for hardware
can be complex and usually require a "real" driver to be written for
them to test things.

thanks,

gre gk-h
  

On 3/23/23 10:58, Greg Kroah-Hartman wrote:
> On Thu, Mar 23, 2023 at 07:17:40AM +0000, Vaittinen, Matti wrote:
>> On 3/22/23 20:57, Greg Kroah-Hartman wrote:
>>> On Wed, Mar 22, 2023 at 03:48:00PM +0200, Matti Vaittinen wrote:
>>>> Hi Greg,
>>>>
>>>> Thanks for looking at this.
>>>>
>>>> On 3/22/23 14:07, Greg Kroah-Hartman wrote:
>>>>> On Wed, Mar 22, 2023 at 11:05:55AM +0200, Matti Vaittinen wrote:

>> I am very conservative what comes to adding unit tests due to the huge
>> inertia they add to any further development. I usually only add tests to
>> APIs which I know won't require changing (I don't know such in-kernel
>> APIs)
> 
> So anything that is changing doesn't get a test? 

No. I think you misread me. I didn't say I don't like adding tests to 
code which changes. I said, I don't like adding tests to APIs which change.

  If you only test
> things that don't change then no tests fail, and so, why have the test
> at all?

Because implementation cascading into functions below an API may change 
even if the API stays unchanged.

> On the contrary, tests should be used to verify things that are changing
> all the time, to ensure that we don't break things.

This is only true when your test code stays valid. Problem with 
excessive amount of tests is that more we have callers for an API, 
harder changing that API becomes. I've seen a point where people stop 
fixing "unimportant" things just because the amount of work fixing all 
impacted UT-cases would take. I know that many things went wrong before 
that project ended up to the point - but what I picked up with me is 
that carelessly added UTs do really hinder further development.

  That's why we need
> them, not to just validate that old code still is going ok.
> 
> The driver core is changing, and so, I would love to see tests for it to
> ensure that I don't break anything over time.  That should NOT slow down
> development but rather, speed it up as it ensures that things still work
> properly.

I agree that there are cases where UTs are very handy and can add 
confidence that things work as intended. Still, my strong opinion is 
that people should consider what parts of code are really worth testing 
- and how to do the tests so that the amount of maintenance required by 
the tests stays low. It's definitely _not fun_ to do refactoring for 
minor improvement when 400+ unit-test cases break. It's a point when 
many developers start seeing fixing this minor culprit much less 
important... And when people stop fixing minor things ... major things 
start to be just around the corner.

> 
>> - or to functions which I think are error-prone. So, I am probably
>> one of the last persons adding UTs to code I don't know :)
> 
> That's fine, you don't have to add test code for stuff you don't know.
> 
> But again, do NOT abuse a platform device for this, that's not ok, and
> the in-kernel code that does do this should be fixed up.

As suggested by David in another mail - I'll go with the 
root_device_[un]register(). I'll drop this patch entirely. Thanks for 
help, this was once again very educating :)

Yours,
	-- Matti
  

On 3/23/23 10:58, Greg Kroah-Hartman wrote:
> On Thu, Mar 23, 2023 at 07:17:40AM +0000, Vaittinen, Matti wrote:
>> On 3/22/23 20:57, Greg Kroah-Hartman wrote:
>>> On Wed, Mar 22, 2023 at 03:48:00PM +0200, Matti Vaittinen wrote:
>>>> Hi Greg,
>>>>
>>>> Thanks for looking at this.
>>>>
>>>> On 3/22/23 14:07, Greg Kroah-Hartman wrote:
>>>>> On Wed, Mar 22, 2023 at 11:05:55AM +0200, Matti Vaittinen wrote:

>>>> The biggest thing for me is that I don't like the idea of creating own 'test
>>>> device' in <add subsystem here> while we already have some in DRM (or
>>>> others). Thus, I do see value in adding generic helpers for supporting
>>>> running KUnit tests on devm_* APIs. Hence it'd be good to have _some_
>>>> support for it.
>>>
>>> I agree, let's use a virtual device and a virtual bus (you can use the
>>> auxbus code for this as that's all there for this type of thing)
>>
>> Hm. The auxiliary_devices require parent. What would be the best way to
>> deal with that in KUnit tests?
> 
> If you use NULL as the parent, it goes into the root.

As far as I read this is not the case with auxiliary devices. Judging 
the docs they were intended to be representing some part of a (parent) 
device. I see the auxiliary_device_init() has explicit check for parent 
being populated:

int auxiliary_device_init(struct auxiliary_device *auxdev)
{
         struct device *dev = &auxdev->dev;

         if (!dev->parent) {
                 pr_err("auxiliary_device has a NULL dev->parent\n");
                 return -EINVAL;
         }

As I wrote in another mail, I thought of using a root_device for this 
IIO test as was suggested by David. To tell the truth, implementing a 
kunit bus device is starting to feel a bit overwhelming... I started 
just adding a driver for a light sensor, ended up adding a helper for 
IIO gain-time-scale conversions and I am slightly reluctant to going the 
extra-extra mile of adding some UT infrastructure in the context of this 
driver work...

Well, let's see. Maybe I change my mind after a good night's sleep :)

Yours,
	-- Matti
  

Hi David,

On Thu, Mar 23, 2023 at 03:30:34PM +0800, David Gow wrote:
> I think the questions we therefore need to answer are:
> - Do we specifically need a platform_device (or, any other specific
> device struct), or would any old struct device work? I can see why we
> would need a platform device for cases where we're testing things like
> device-tree properties (or, in the future, having e.g. USB-specific
> helpers which create usb_device). Most tests just use
> root_device_register() thus far, though.
> - What helpers do we need to make creating, using, and cleaning up
> these devices as simple as possible.
> 
> I think that in this particular case, we don't actually need a struct
> platform_device. Replacing these helpers with simple calls to
> root_device_register() and root_device_unregister() seems to work fine
> with this patch series for me. (It does break the
> drm_test_managed_run_action test, though.) So I don't think having
> these helpers actually help this series at the moment.

I'm not sure that a root_device is a good option, see below.

> That being said, if they used the KUnit resource system to
> automatically clean up the device when the test is finished (or
> otherwise exits), that would seem like a real advantage. The clk and
> drm examples both do this, and I'm hoping to add an API to make it
> even simpler going forward. (With the work-in-progress API described
> here[1], the whole thing should boil down to "struct device *dev =
> root_device_register(name); kunit_defer(root_device_unregister,
> dev);".)

Oh, yes, please make it happen :)

The current API is a bit of a pain compared to other managed APIs like
devm or drmm.

> So, I guess we have three cases we need to look at:
> - A test just needs any old struct device. Tests thus far have
> hardcoded, or had their own wrappers around root_device_register() for
> this.
> - A test needs a device attached to a bus (but doesn't care which
> bus). Thus far, people have used struct platform_device for this (see
> the DRM helpers, which use a platform device for managed resource
> tests[2]). Maybe the right solution here is something like a struct
> kunit_device?
> - A test needs a device attached to a specific bus. We'll probably
> need some more detailed faking of that bus. This covers cases like
> having fake USB devices, devicetree integration, etc.

Yeah, from the current discussion on the IIO and clk patchsets, and what
we've done in DRM, I guess there's two major use cases:

  * You test an (isolated) function that takes a device as an argument.
    Here you probably don't really care about what the device is as long
    as you can provide one. This is what is being done for the DRM
    helpers at the moment, even though it's not really isolated. The
    device is essentially mocked. This could be your points 1 and 2.

  * You want to probe the driver with a fake context. The device and
    drivers are very much real, but the device tree (or whatever) is
    mocked. This is what the clocks patches from Stephen are doing. This
    could be covered by your point 3.

> I'd suggest that, for the majority of cases which only care about the
> first case, let's just use root_device_register() directly, or have a
> thin wrapper like the old root_device-based version of the DRM
> helpers[3]. This will probable serve us well enough while we work out
> how to handle the other two cases properly (which is already being
> looked at for the CLK/DeviceTree patches and the DRM stuff).

I disagree, and I think your cases 1 and 2 should be merged. We were
initially using a root_device in DRM. We had to switch to
platform_device (but actually any device attached to a bus) because a
root device isn't attached to a bus and thus the devm resources are
never freed.

When a function takes a device as an argument, there's a good chance
that it will use devm nowadays, so we ended up exhausting resources
pools (like IDs iirc) when running a lot of tests in sequence.

So yeah, I think you can't just assume that a root device will do even
for a true unit test that would test an isolated function. It either
needs to be tied to a bus, or we need to force the devm resource release
when the root device is removed somehow.

Maxime
  

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

Maxime
  

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 :)

thanks,

greg k-h
  

On Thu, Mar 23, 2023 at 11:20:33AM +0200, Matti Vaittinen wrote:
> On 3/23/23 10:58, Greg Kroah-Hartman wrote:
> > On Thu, Mar 23, 2023 at 07:17:40AM +0000, Vaittinen, Matti wrote:
> > > On 3/22/23 20:57, Greg Kroah-Hartman wrote:
> > > > On Wed, Mar 22, 2023 at 03:48:00PM +0200, Matti Vaittinen wrote:
> > > > > Hi Greg,
> > > > > 
> > > > > Thanks for looking at this.
> > > > > 
> > > > > On 3/22/23 14:07, Greg Kroah-Hartman wrote:
> > > > > > On Wed, Mar 22, 2023 at 11:05:55AM +0200, Matti Vaittinen wrote:
> 
> > > I am very conservative what comes to adding unit tests due to the huge
> > > inertia they add to any further development. I usually only add tests to
> > > APIs which I know won't require changing (I don't know such in-kernel
> > > APIs)
> > 
> > So anything that is changing doesn't get a test?
> 
> No. I think you misread me. I didn't say I don't like adding tests to code
> which changes. I said, I don't like adding tests to APIs which change.

Then you should not be writing any in-kernel tests as all of our APIs
change all the time.

>  If you only test
> > things that don't change then no tests fail, and so, why have the test
> > at all?
> 
> Because implementation cascading into functions below an API may change even
> if the API stays unchanged.

Then it needs to be fixed.

> > On the contrary, tests should be used to verify things that are changing
> > all the time, to ensure that we don't break things.
> 
> This is only true when your test code stays valid. Problem with excessive
> amount of tests is that more we have callers for an API, harder changing
> that API becomes. I've seen a point where people stop fixing "unimportant"
> things just because the amount of work fixing all impacted UT-cases would
> take. I know that many things went wrong before that project ended up to the
> point - but what I picked up with me is that carelessly added UTs do really
> hinder further development.

Again, in-kernel apis change at any moment.  I just changed one that was
over 15 years old.  Don't get stuck into thinking that you can only
write tests for stuff that is "stable" as nothing in the kernel is
"stable" and can change at any point in time.  You fix up all the
in-kernel users of the api, and the tests, and all is good.  That's how
kernel development works.

>  That's why we need
> > them, not to just validate that old code still is going ok.
> > 
> > The driver core is changing, and so, I would love to see tests for it to
> > ensure that I don't break anything over time.  That should NOT slow down
> > development but rather, speed it up as it ensures that things still work
> > properly.
> 
> I agree that there are cases where UTs are very handy and can add confidence
> that things work as intended. Still, my strong opinion is that people should
> consider what parts of code are really worth testing - and how to do the
> tests so that the amount of maintenance required by the tests stays low.
> It's definitely _not fun_ to do refactoring for minor improvement when 400+
> unit-test cases break. It's a point when many developers start seeing fixing
> this minor culprit much less important... And when people stop fixing minor
> things ... major things start to be just around the corner.

If people stop fixing minor things then the kernel development process
is dead.  Based on all the changes that go into it right now, we are far
from having that problem.

So write valid tests, if we get to the point where we have too much of a
problem fixing up the tests than the real users of apis, then we can
revisit it.  But for now, that's not an issue.

And again, remember, and api can, and will, change at any moment in
time, you can never know what will be "stable" as we do not have such a
thing.

thanks,

greg k-h
  

On Thu, Mar 23, 2023 at 12:01:15PM +0200, Matti Vaittinen wrote:
> On 3/23/23 10:58, Greg Kroah-Hartman wrote:
> > On Thu, Mar 23, 2023 at 07:17:40AM +0000, Vaittinen, Matti wrote:
> > > On 3/22/23 20:57, Greg Kroah-Hartman wrote:
> > > > On Wed, Mar 22, 2023 at 03:48:00PM +0200, Matti Vaittinen wrote:
> > > > > Hi Greg,
> > > > > 
> > > > > Thanks for looking at this.
> > > > > 
> > > > > On 3/22/23 14:07, Greg Kroah-Hartman wrote:
> > > > > > On Wed, Mar 22, 2023 at 11:05:55AM +0200, Matti Vaittinen wrote:
> 
> > > > > The biggest thing for me is that I don't like the idea of creating own 'test
> > > > > device' in <add subsystem here> while we already have some in DRM (or
> > > > > others). Thus, I do see value in adding generic helpers for supporting
> > > > > running KUnit tests on devm_* APIs. Hence it'd be good to have _some_
> > > > > support for it.
> > > > 
> > > > I agree, let's use a virtual device and a virtual bus (you can use the
> > > > auxbus code for this as that's all there for this type of thing)
> > > 
> > > Hm. The auxiliary_devices require parent. What would be the best way to
> > > deal with that in KUnit tests?
> > 
> > If you use NULL as the parent, it goes into the root.
> 
> As far as I read this is not the case with auxiliary devices. Judging the
> docs they were intended to be representing some part of a (parent) device. I
> see the auxiliary_device_init() has explicit check for parent being
> populated:
> 
> int auxiliary_device_init(struct auxiliary_device *auxdev)
> {
>         struct device *dev = &auxdev->dev;
> 
>         if (!dev->parent) {
>                 pr_err("auxiliary_device has a NULL dev->parent\n");
>                 return -EINVAL;
>         }

Yes as it wants to "split" a device up into smaller devices.  So make a
real device that it can hang off of.

> As I wrote in another mail, I thought of using a root_device for this IIO
> test as was suggested by David. To tell the truth, implementing a kunit bus
> device is starting to feel a bit overwhelming... I started just adding a
> driver for a light sensor, ended up adding a helper for IIO gain-time-scale
> conversions and I am slightly reluctant to going the extra-extra mile of
> adding some UT infrastructure in the context of this driver work...

I think it is worth it as the driver core has no tests.  So it obviously
must be correct, right?  :)

thanks,

greg k-h
  

This is a low priority babbling - feel free to skip if busy.

On 3/23/23 12:25, Greg Kroah-Hartman wrote:
> On Thu, Mar 23, 2023 at 11:20:33AM +0200, Matti Vaittinen wrote:
>> On 3/23/23 10:58, Greg Kroah-Hartman wrote:
>>> On Thu, Mar 23, 2023 at 07:17:40AM +0000, Vaittinen, Matti wrote:
>>>> On 3/22/23 20:57, Greg Kroah-Hartman wrote:
>>>>> On Wed, Mar 22, 2023 at 03:48:00PM +0200, Matti Vaittinen wrote:
>>>>>> Hi Greg,
>>>>>>
>>>>>> Thanks for looking at this.
>>>>>>
>>>>>> On 3/22/23 14:07, Greg Kroah-Hartman wrote:
>>>>>>> On Wed, Mar 22, 2023 at 11:05:55AM +0200, Matti Vaittinen wrote:
>>
>>>> I am very conservative what comes to adding unit tests due to the huge
>>>> inertia they add to any further development. I usually only add tests to
>>>> APIs which I know won't require changing (I don't know such in-kernel
>>>> APIs)
>>>
>>> So anything that is changing doesn't get a test?
>>
>> No. I think you misread me. I didn't say I don't like adding tests to code
>> which changes. I said, I don't like adding tests to APIs which change.
> 
> Then you should not be writing any in-kernel tests as all of our APIs
> change all the time.
> 
>>   If you only test
>>> things that don't change then no tests fail, and so, why have the test
>>> at all?
>>
>> Because implementation cascading into functions below an API may change even
>> if the API stays unchanged.
> 
> Then it needs to be fixed.
> 
>>> On the contrary, tests should be used to verify things that are changing
>>> all the time, to ensure that we don't break things.
>>
>> This is only true when your test code stays valid. Problem with excessive
>> amount of tests is that more we have callers for an API, harder changing
>> that API becomes. I've seen a point where people stop fixing "unimportant"
>> things just because the amount of work fixing all impacted UT-cases would
>> take. I know that many things went wrong before that project ended up to the
>> point - but what I picked up with me is that carelessly added UTs do really
>> hinder further development.
> 
> Again, in-kernel apis change at any moment.

I agree. This is why I initially wrote:

 >>>> APIs which I know won't require changing (I don't know such in-kernel
 >>>> APIs)

> Don't get stuck into thinking that you can only
> write tests for stuff that is "stable" as nothing in the kernel is
> "stable" and can change at any point in time.

I don't. But I don't either think that UTs come with no cost. Thus I do 
only write tests when I see a _real need_ for one. If the APIs would be 
guaranteed not to change, then I would understand writing the tests for 
each and every "thing" without much of thinking if "the thing" is worth 
the test.

>  You fix up all the
> in-kernel users of the api, and the tests, and all is good.  That's how
> kernel development works.

Sure. This is how it works and how I think it should work. But I also 
have seen how this 'UT work overhead' has made people to decide not to 
touch things. Not in kernel but in other project. This is a real thing 
which can happen - many engineers like me are lazy bastards :)

>>   That's why we need
>>> them, not to just validate that old code still is going ok.
>>>
>>> The driver core is changing, and so, I would love to see tests for it to
>>> ensure that I don't break anything over time.  That should NOT slow down
>>> development but rather, speed it up as it ensures that things still work
>>> properly.
>>
>> I agree that there are cases where UTs are very handy and can add confidence
>> that things work as intended. Still, my strong opinion is that people should
>> consider what parts of code are really worth testing - and how to do the
>> tests so that the amount of maintenance required by the tests stays low.
>> It's definitely _not fun_ to do refactoring for minor improvement when 400+
>> unit-test cases break. It's a point when many developers start seeing fixing
>> this minor culprit much less important... And when people stop fixing minor
>> things ... major things start to be just around the corner.
> 
> If people stop fixing minor things then the kernel development process
> is dead.  Based on all the changes that go into it right now, we are far
> from having that problem.

And I am so happy for that. Kernel/drivers are still fun to work with. 
My personal preference is to keep it that way :)

> So write valid tests, if we get to the point where we have too much of a
> problem fixing up the tests than the real users of apis, then we can
> revisit it.  But for now, that's not an issue.

The beginning of your sentence hits the point. Write valid tests. I just 
encourage people to occasionally ask if the test they write is really a 
valid one. :)

> And again, remember, and api can, and will, change at any moment in
> time, you can never know what will be "stable" as we do not have such a
> thing.

We agree on this.
  

On 3/23/23 12:27, Greg Kroah-Hartman wrote:
> On Thu, Mar 23, 2023 at 12:01:15PM +0200, Matti Vaittinen wrote:
>> On 3/23/23 10:58, Greg Kroah-Hartman wrote:
>>> On Thu, Mar 23, 2023 at 07:17:40AM +0000, Vaittinen, Matti wrote:
>>>> On 3/22/23 20:57, Greg Kroah-Hartman wrote:
>>>>> On Wed, Mar 22, 2023 at 03:48:00PM +0200, Matti Vaittinen wrote:
>>>>>> Hi Greg,
>>>>>>
>>>>>> Thanks for looking at this.
>>>>>>
>>>>>> On 3/22/23 14:07, Greg Kroah-Hartman wrote:
>>>>>>> On Wed, Mar 22, 2023 at 11:05:55AM +0200, Matti Vaittinen wrote:
>>
>>>>>> The biggest thing for me is that I don't like the idea of creating own 'test
>>>>>> device' in <add subsystem here> while we already have some in DRM (or
>>>>>> others). Thus, I do see value in adding generic helpers for supporting
>>>>>> running KUnit tests on devm_* APIs. Hence it'd be good to have _some_
>>>>>> support for it.
>>>>>
>>>>> I agree, let's use a virtual device and a virtual bus (you can use the
>>>>> auxbus code for this as that's all there for this type of thing)
>>>>
>>>> Hm. The auxiliary_devices require parent. What would be the best way to
>>>> deal with that in KUnit tests?
>>>
>>> If you use NULL as the parent, it goes into the root.
>>
>> As far as I read this is not the case with auxiliary devices. Judging the
>> docs they were intended to be representing some part of a (parent) device. I
>> see the auxiliary_device_init() has explicit check for parent being
>> populated:
>>
>> int auxiliary_device_init(struct auxiliary_device *auxdev)
>> {
>>          struct device *dev = &auxdev->dev;
>>
>>          if (!dev->parent) {
>>                  pr_err("auxiliary_device has a NULL dev->parent\n");
>>                  return -EINVAL;
>>          }
> 
> Yes as it wants to "split" a device up into smaller devices.  So make a
> real device that it can hang off of.

Yep. This is what led me to the root_device_register()... :rolleyes: And 
seein the root-device alone could do what I need - adding auxiliary 
device on top of it just for the sake of adding one seems a bit of an 
over-engineering to me :)

>> As I wrote in another mail, I thought of using a root_device for this IIO
>> test as was suggested by David. To tell the truth, implementing a kunit bus
>> device is starting to feel a bit overwhelming... I started just adding a
>> driver for a light sensor, ended up adding a helper for IIO gain-time-scale
>> conversions and I am slightly reluctant to going the extra-extra mile of
>> adding some UT infrastructure in the context of this driver work...
> 
> I think it is worth it as the driver core has no tests.  So it obviously
> must be correct, right?  :)

Doh. Greg, I hate you :) How could one argue with something like this? I 
think I will submit the v6 with the root_device_register() due to the 
aux-device requiring it in any case. I know that will end up to your 
table still as IIO is going through your hands anyways.

I will however take a look at what Maxime said about devm unwinding not 
being done w/o a bus because I think I saw the unwinding done in these 
IIO tests even when using the root_device_register() 
root_device_unregister(). If the unwinding really is not done, then I 
will come back to this auxiliary device rehearsal
  

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

Yours,
	-- Matti
  

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
  

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
  

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
  

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.

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

Yours,
	-- Matti
  

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.

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
  

On 3/24/23 08:34, 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:

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

Ah. That's true. Being able to abort the test on error w/o being forced 
to do a clean-up dance for the dummy device would be convenient.

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

It's easy for me to agree.

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

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.

Yours,
	-- Matti
  

On Fri, 24 Mar 2023 at 14:51, Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>
> On 3/24/23 08:34, 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:
>
> >> 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).
>
> Ah. That's true. Being able to abort the test on error w/o being forced
> to do a clean-up dance for the dummy device would be convenient.
>
> > 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.
>
> It's easy for me to agree.
>
> >> 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.
>
> 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.

It's a little less explicit, though, so I could see the value in using
macros with "root_device" in the name to make the current
implementation clearer, and the eventual change more obvious.

Cheers,
-- David
  

On 3/24/23 11:52, David Gow wrote:
> On Fri, 24 Mar 2023 at 14:51, Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>>
>> On 3/24/23 08:34, David Gow wrote:
>>> On Fri, 24 Mar 2023 at 14:11, Matti Vaittinen <mazziesaccount@gmail.com> 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.
> 
> It's a little less explicit, though, so I could see the value in using
> macros with "root_device" in the name to make the current
> implementation clearer, and the eventual change more obvious.

I think it makes sense to avoid the mass-conversions if the signatures 
for kunit_device_register() and kunit_device_unregister() are expected 
to be known by now. If there is no objections I'll add those wrappers + 
the include/kunit/device.h to v6 of this series.

I think I'll try to hold back sending the v6 until next Monday - unless 
I get really bored during the weekend ;)

Yours,
	-- Matti
  

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 
>> <mazziesaccount@gmail.com> wrote:
>>>
>>> On 3/24/23 08:34, David Gow wrote:
>>>> On Fri, 24 Mar 2023 at 14:11, Matti Vaittinen 
>>>> <mazziesaccount@gmail.com> 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.

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.

Yours,
	-- Matti
  

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.

Maxime

-- >8 --
diff --git a/drivers/base/test/.kunitconfig b/drivers/base/test/.kunitconfig
new file mode 100644
index 000000000000..473923f0998b
--- /dev/null
+++ b/drivers/base/test/.kunitconfig
@@ -0,0 +1,2 @@
+CONFIG_KUNIT=y
+CONFIG_DM_KUNIT_TEST=y
diff --git a/drivers/base/test/Kconfig b/drivers/base/test/Kconfig
index 610a1ba7a467..9d42051f8f8e 100644
--- a/drivers/base/test/Kconfig
+++ b/drivers/base/test/Kconfig
@@ -9,6 +9,10 @@ config TEST_ASYNC_DRIVER_PROBE

 	  If unsure say N.

+config DM_KUNIT_TEST
+	tristate "KUnit Tests for the device model" if !KUNIT_ALL_TESTS
+	depends on KUNIT
+
 config DRIVER_PE_KUNIT_TEST
 	bool "KUnit Tests for property entry API" if !KUNIT_ALL_TESTS
 	depends on KUNIT=y
diff --git a/drivers/base/test/Makefile b/drivers/base/test/Makefile
index 7f76fee6f989..31bcaa743e94 100644
--- a/drivers/base/test/Makefile
+++ b/drivers/base/test/Makefile
@@ -1,5 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_TEST_ASYNC_DRIVER_PROBE)	+= test_async_driver_probe.o

+obj-$(CONFIG_DM_KUNIT_TEST)	+= test-devm-cleanup.o
+
 obj-$(CONFIG_DRIVER_PE_KUNIT_TEST) += property-entry-test.o
 CFLAGS_property-entry-test.o += $(DISABLE_STRUCTLEAK_PLUGIN)
diff --git a/drivers/base/test/test-devm-cleanup.c b/drivers/base/test/test-devm-cleanup.c
new file mode 100644
index 000000000000..bad3cb1385b1
--- /dev/null
+++ b/drivers/base/test/test-devm-cleanup.c
@@ -0,0 +1,353 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <kunit/resource.h>
+
+#include <linux/device.h>
+#include <linux/platform_device.h>
+
+#define DEVICE_NAME "test"
+
+struct test_priv {
+	bool probe_done;
+	bool release_done;
+	wait_queue_head_t release_wq;
+	struct device *dev;
+};
+
+static void devm_device_action(void *ptr)
+{
+	struct test_priv *priv = ptr;
+
+	priv->release_done = true;
+	wake_up_interruptible(&priv->release_wq);
+}
+
+static void devm_put_device_action(void *ptr)
+{
+	struct test_priv *priv = ptr;
+
+	put_device(priv->dev);
+	priv->release_done = true;
+	wake_up_interruptible(&priv->release_wq);
+}
+
+#define RELEASE_TIMEOUT_MS	500
+
+static void root_device_register_unregister_test(struct kunit *test)
+{
+	struct test_priv *priv;
+	int ret;
+
+	priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+	init_waitqueue_head(&priv->release_wq);
+
+	priv->dev = root_device_register(DEVICE_NAME);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->dev);
+
+	ret = devm_add_action_or_reset(priv->dev, devm_device_action, priv);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	root_device_unregister(priv->dev);
+
+	ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+					       msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+	KUNIT_EXPECT_GT(test, ret, 0);
+}
+
+static void root_device_register_get_put_unregister_test(struct kunit *test)
+{
+	struct test_priv *priv;
+	int ret;
+
+	priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+	init_waitqueue_head(&priv->release_wq);
+
+	priv->dev = root_device_register(DEVICE_NAME);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->dev);
+
+	get_device(priv->dev);
+
+	ret = devm_add_action_or_reset(priv->dev, devm_device_action, priv);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	put_device(priv->dev);
+
+	root_device_unregister(priv->dev);
+
+	ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+					       msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+	KUNIT_EXPECT_GT(test, ret, 0);
+}
+
+static void root_device_register_get_unregister_with_devm_test(struct kunit *test)
+{
+	struct test_priv *priv;
+	int ret;
+
+	priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+	init_waitqueue_head(&priv->release_wq);
+
+	priv->dev = root_device_register(DEVICE_NAME);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->dev);
+
+	get_device(priv->dev);
+
+	ret = devm_add_action_or_reset(priv->dev, devm_put_device_action, priv);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	root_device_unregister(priv->dev);
+
+	ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+					       msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+	KUNIT_EXPECT_GT(test, ret, 0);
+}
+
+static void platform_device_register_unregister_test(struct kunit *test)
+{
+	struct platform_device *pdev;
+	struct test_priv *priv;
+	int ret;
+
+	priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+	init_waitqueue_head(&priv->release_wq);
+
+	pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
+
+	ret = platform_device_add(pdev);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	priv->dev = &pdev->dev;
+
+	ret = devm_add_action_or_reset(priv->dev, devm_device_action, priv);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	platform_device_unregister(pdev);
+
+	ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+					       msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+	KUNIT_EXPECT_GT(test, ret, 0);
+}
+
+static void platform_device_register_get_put_unregister_test(struct kunit *test)
+{
+	struct platform_device *pdev;
+	struct test_priv *priv;
+	int ret;
+
+	priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+	init_waitqueue_head(&priv->release_wq);
+
+	pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
+
+	ret = platform_device_add(pdev);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	priv->dev = &pdev->dev;
+
+	get_device(priv->dev);
+
+	ret = devm_add_action_or_reset(priv->dev, devm_device_action, priv);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	put_device(priv->dev);
+
+	platform_device_unregister(pdev);
+
+	ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+					       msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+	KUNIT_EXPECT_GT(test, ret, 0);
+}
+
+static void platform_device_register_get_unregister_with_devm_test(struct kunit *test)
+{
+	struct platform_device *pdev;
+	struct test_priv *priv;
+	int ret;
+
+	priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+	init_waitqueue_head(&priv->release_wq);
+
+	pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
+
+	ret = platform_device_add(pdev);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	priv->dev = &pdev->dev;
+
+	get_device(priv->dev);
+
+	ret = devm_add_action_or_reset(priv->dev, devm_put_device_action, priv);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	platform_device_unregister(pdev);
+
+	ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+					       msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+	KUNIT_EXPECT_GT(test, ret, 0);
+}
+
+static int fake_probe(struct platform_device *pdev)
+{
+	struct test_priv *priv = platform_get_drvdata(pdev);
+
+	priv->probe_done = true;
+	wake_up_interruptible(&priv->release_wq);
+
+	return 0;
+}
+
+static struct platform_driver fake_driver = {
+	.probe	= fake_probe,
+	.driver = {
+		.name = DEVICE_NAME,
+	},
+};
+
+static void probed_platform_device_register_unregister_test(struct kunit *test)
+{
+	struct platform_device *pdev;
+	struct test_priv *priv;
+	int ret;
+
+	priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+	init_waitqueue_head(&priv->release_wq);
+
+	ret = platform_driver_register(&fake_driver);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
+
+	priv->dev = &pdev->dev;
+	platform_set_drvdata(pdev, priv);
+
+	ret = platform_device_add(pdev);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	ret = wait_event_interruptible_timeout(priv->release_wq, priv->probe_done,
+					       msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+	KUNIT_ASSERT_GT(test, ret, 0);
+
+	ret = devm_add_action_or_reset(priv->dev, devm_device_action, priv);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	platform_device_unregister(pdev);
+
+	ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+					       msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+	KUNIT_EXPECT_GT(test, ret, 0);
+
+	platform_driver_unregister(&fake_driver);
+}
+
+static void probed_platform_device_register_get_put_unregister_test(struct kunit *test)
+{
+	struct platform_device *pdev;
+	struct test_priv *priv;
+	int ret;
+
+	priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+	init_waitqueue_head(&priv->release_wq);
+
+	ret = platform_driver_register(&fake_driver);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
+
+	priv->dev = &pdev->dev;
+	platform_set_drvdata(pdev, priv);
+
+	ret = platform_device_add(pdev);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	ret = wait_event_interruptible_timeout(priv->release_wq, priv->probe_done,
+					       msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+	KUNIT_ASSERT_GT(test, ret, 0);
+
+	get_device(priv->dev);
+
+	ret = devm_add_action_or_reset(priv->dev, devm_device_action, priv);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	put_device(priv->dev);
+
+	platform_device_unregister(pdev);
+
+	ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+					       msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+	KUNIT_EXPECT_GT(test, ret, 0);
+
+	platform_driver_unregister(&fake_driver);
+}
+
+static void probed_platform_device_register_get_unregister_with_devm_test(struct kunit *test)
+{
+	struct platform_device *pdev;
+	struct test_priv *priv;
+	int ret;
+
+	priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+	init_waitqueue_head(&priv->release_wq);
+
+	ret = platform_driver_register(&fake_driver);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
+
+	priv->dev = &pdev->dev;
+	platform_set_drvdata(pdev, priv);
+
+	ret = platform_device_add(pdev);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	ret = wait_event_interruptible_timeout(priv->release_wq, priv->probe_done,
+					       msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+	KUNIT_ASSERT_GT(test, ret, 0);
+
+	get_device(priv->dev);
+
+	ret = devm_add_action_or_reset(priv->dev, devm_put_device_action, priv);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	platform_device_unregister(pdev);
+
+	ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+					       msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+	KUNIT_EXPECT_GT(test, ret, 0);
+
+	platform_driver_unregister(&fake_driver);
+}
+
+static struct kunit_case devm_inconsistencies_tests[] = {
+	KUNIT_CASE(root_device_register_unregister_test),
+	KUNIT_CASE(root_device_register_get_put_unregister_test),
+	KUNIT_CASE(root_device_register_get_unregister_with_devm_test),
+	KUNIT_CASE(platform_device_register_unregister_test),
+	KUNIT_CASE(platform_device_register_get_put_unregister_test),
+	KUNIT_CASE(platform_device_register_get_unregister_with_devm_test),
+	KUNIT_CASE(probed_platform_device_register_unregister_test),
+	KUNIT_CASE(probed_platform_device_register_get_put_unregister_test),
+	KUNIT_CASE(probed_platform_device_register_get_unregister_with_devm_test),
+	{}
+};
+
+static struct kunit_suite devm_inconsistencies_test_suite = {
+	.name = "devm-inconsistencies",
+	.test_cases = devm_inconsistencies_tests,
+};
+
+kunit_test_suite(devm_inconsistencies_test_suite);
-- >8 --
  

On Thu, Mar 23, 2023 at 11:21:58AM +0100, 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 :)

Please have a look at:
https://lore.kernel.org/linux-kselftest/20230324123157.bbwvfq4gsxnlnfwb@houat/

I couldn't get an answer on whether it was considered a bug or not last
time, but as you can see there's a clear difference between a root
device and a platform device that has probed when it comes to resource
cleanup.

Maxime
  

On Fri, Mar 24, 2023 at 01:36:32PM +0100, Maxime Ripard wrote:
> On Thu, Mar 23, 2023 at 11:21:58AM +0100, 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 :)
> 
> Please have a look at:
> https://lore.kernel.org/linux-kselftest/20230324123157.bbwvfq4gsxnlnfwb@houat/
> 
> I couldn't get an answer on whether it was considered a bug or not last
> time, but as you can see there's a clear difference between a root
> device and a platform device that has probed when it comes to resource
> cleanup.

Great, testing shows there are bugs!  :)

That's a great start of a test, how about submitting that in a way that
I can test it and we can go from there?

thanks,

greg k-h
  

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
  

On Fri, Mar 24, 2023 at 01:43:07PM +0100, Greg Kroah-Hartman wrote:
> On Fri, Mar 24, 2023 at 01:36:32PM +0100, Maxime Ripard wrote:
> > On Thu, Mar 23, 2023 at 11:21:58AM +0100, 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 :)
> > 
> > Please have a look at:
> > https://lore.kernel.org/linux-kselftest/20230324123157.bbwvfq4gsxnlnfwb@houat/
> > 
> > I couldn't get an answer on whether it was considered a bug or not last
> > time, but as you can see there's a clear difference between a root
> > device and a platform device that has probed when it comes to resource
> > cleanup.
> 
> Great, testing shows there are bugs!  :)

I mean, it wasn't clear to me that it was indeed a bug or the intent
behind devm was that it would only work when probed. Both seemed
reasonable.

> That's a great start of a test, how about submitting that in a way that
> I can test it and we can go from there?

Ack.

I guess I'd need to arrange them somewhat differently for it to be
useful and merge-able.

How would you prefer them to be submitted, in two different files
testing both the root devices and platform devices?

Maxime
  

On Fri, Mar 24, 2023 at 02:02:06PM +0100, Maxime Ripard wrote:
> On Fri, Mar 24, 2023 at 01:43:07PM +0100, Greg Kroah-Hartman wrote:
> > On Fri, Mar 24, 2023 at 01:36:32PM +0100, Maxime Ripard wrote:
> > > On Thu, Mar 23, 2023 at 11:21:58AM +0100, 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 :)
> > > 
> > > Please have a look at:
> > > https://lore.kernel.org/linux-kselftest/20230324123157.bbwvfq4gsxnlnfwb@houat/
> > > 
> > > I couldn't get an answer on whether it was considered a bug or not last
> > > time, but as you can see there's a clear difference between a root
> > > device and a platform device that has probed when it comes to resource
> > > cleanup.
> > 
> > Great, testing shows there are bugs!  :)
> 
> I mean, it wasn't clear to me that it was indeed a bug or the intent
> behind devm was that it would only work when probed. Both seemed
> reasonable.
> 
> > That's a great start of a test, how about submitting that in a way that
> > I can test it and we can go from there?
> 
> Ack.
> 
> I guess I'd need to arrange them somewhat differently for it to be
> useful and merge-able.
> 
> How would you prefer them to be submitted, in two different files
> testing both the root devices and platform devices?

root devices are rare, but yes, one for each would be good, thanks!

greg k-h
  

On Fri, 24 Mar 2023 at 18:17, Matti Vaittinen <mazziesaccount@gmail.com> 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
> >> <mazziesaccount@gmail.com> wrote:
> >>>
> >>> On 3/24/23 08:34, David Gow wrote:
> >>>> On Fri, 24 Mar 2023 at 14:11, Matti Vaittinen
> >>>> <mazziesaccount@gmail.com> 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/20230325043104.3761770-1-davidgow@google.com/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
  

On Fri, 24 Mar 2023 at 20:32, Maxime Ripard <maxime@cerno.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
  

On 3/25/23 06:35, David Gow wrote:
> On Fri, 24 Mar 2023 at 18:17, Matti Vaittinen <mazziesaccount@gmail.com> 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
>>>> <mazziesaccount@gmail.com> wrote:
>>>>>
>>>>> On 3/24/23 08:34, David Gow wrote:
>>>>>> On Fri, 24 Mar 2023 at 14:11, Matti Vaittinen
>>>>>> <mazziesaccount@gmail.com> 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/20230325043104.3761770-1-davidgow@google.com/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.

Thanks for letting me know. I did also prepare this commit yesterday:
https://github.com/M-Vaittinen/linux/commit/b784a90f8cc64ff83e802ec818e662fae1d0c264

It does use the existing kunit resources for clean-up. I am not sure if 
it is worth a shot or should I just drop it and use the root-device API 
for now. Any educated opinions on that? :)

Yours,
	-- Matti
  

On Fri, 24 Mar 2023 13:31:57 +0100
Maxime Ripard <maxime@cerno.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.

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.
  

On 3/25/23 10:50, Jonathan Cameron wrote:
> On Fri, 24 Mar 2023 13:31:57 +0100
> Maxime Ripard <maxime@cerno.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.
> 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, ...)` 
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.
  

Hi David,

On Sat, Mar 25, 2023 at 01:40:01PM +0800, David Gow wrote:
> On Fri, 24 Mar 2023 at 20:32, Maxime Ripard <maxime@cerno.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.

Yeah, as an outsider, I came to the same conclusion last time...

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

... but as someone that also have a limited knowledge of that part of
the code, I certainly wasn't sure it was the proper thing to do :)

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

I just submitted them.

Thanks!
Maxime
  

On Sat, Mar 25, 2023 at 05:50:44PM +0000, Jonathan Cameron wrote:
> On Fri, 24 Mar 2023 13:31:57 +0100
> Maxime Ripard <maxime@cerno.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.
> 
> 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
  

On Sun, 26 Mar 2023 10:16:54 -0700
Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 3/25/23 10:50, Jonathan Cameron wrote:
> > On Fri, 24 Mar 2023 13:31:57 +0100
> > Maxime Ripard <maxime@cerno.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.  
> > 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.
>
  

On Wed, 29 Mar 2023 21:46:09 +0200
Maxime Ripard <maxime@cerno.tech> 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 <maxime@cerno.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.  
> > 
> > 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