Support ROHM BU27034 ALS sensor

include/kunit/platform_device.h new file +1 -0

@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */

drivers/base/test/test_kunit_device.c new file +5 -0

@@ -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 <[email protected]>

drivers/base/test/test_kunit_device.c new file +6 -0

+{
+	return 0;
+}
+
+static int fake_remove(struct platform_device *pdev)
+{

Quoted 8 lines · Matti Vaittinen

drivers/base/test/test_kunit_device.c new file +5 -0

@@ -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 <[email protected]>

drivers/base/test/test_kunit_device.c line 15

Quoted 10 lines

drivers/base/test/test_kunit_device.c new file +10 -0

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

Quoted 6 lines

drivers/base/test/test_kunit_device.c new file +6 -0

+{
+	return 0;
+}
+
+static int fake_remove(struct platform_device *pdev)
+{

drivers/base/test/test_kunit_device.c line 31

Quoted 10 lines

drivers/base/test/test_kunit_device.c new file +10 -0

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

drivers/base/test/test_kunit_device.c line 40

Quoted 8 lines

drivers/base/test/test_kunit_device.c new file +8 -0

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

Hi Greg,

Thanks for looking at this.

Quoted 42 lines · Greg Kroah-Hartman

Quoted 8 lines · Matti Vaittinen

drivers/base/test/test_kunit_device.c new file +5 -0

@@ -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 <[email protected]>

drivers/base/test/test_kunit_device.c line 15

Quoted 10 lines

drivers/base/test/test_kunit_device.c new file +10 -0

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

Quoted 6 lines

drivers/base/test/test_kunit_device.c new file +6 -0

+{
+	return 0;
+}
+
+static int fake_remove(struct platform_device *pdev)
+{

drivers/base/test/test_kunit_device.c line 31

Quoted 10 lines

drivers/base/test/test_kunit_device.c new file +10 -0

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

Quoted 1 line

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.

Quoted 9 lines

drivers/base/test/test_kunit_device.c line 40

Quoted 8 lines

drivers/base/test/test_kunit_device.c new file +8 -0

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

Quoted 7 lines

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.

Quoted 23 lines · Matti Vaittinen

Hi Greg,

Thanks for looking at this.

Quoted 10 lines · Greg Kroah-Hartman

Quoted 8 lines · Matti Vaittinen

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

Quoted 7 lines

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?

Quoted 6 lines

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.

Quoted 5 lines

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)

then you can test the devm_* code, _AND_ you can test the driver core at the same time.

Quoted 4 lines

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.

Quoted 40 lines · Greg Kroah-Hartman

Quoted 23 lines · Matti Vaittinen

Hi Greg,

Thanks for looking at this.

Quoted 10 lines · Greg Kroah-Hartman

Quoted 8 lines · Matti Vaittinen

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

Quoted 7 lines

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?

Quoted 6 lines

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.

Quoted 5 lines

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.

Quoted 2 lines

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?

Quoted 11 lines

then you can test the devm_* code, _AND_ you can test the driver core at the same time.

Quoted 4 lines

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.

Quoted 3 lines

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

Quoted 7 lines · Matti

Quoted 6 lines · Greg Kroah-Hartman

Quoted 5 lines · Matti Vaittinen

Hi Greg,

Thanks for looking at this.

On 3/22/23 14:07, Greg Kroah-Hartman wrote:

Quoted 6 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line · Matti Vaittinen

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?

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.

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.

Quoted 3 lines

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

Quoted 15 lines · Greg Kroah-Hartman

Quoted 13 lines · Matti

Quoted 7 lines · Greg Kroah-Hartman

Quoted 6 lines · Matti Vaittinen

Hi Greg,

Thanks for looking at this.

Quoted 1 line · Greg Kroah-Hartman

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)

Quoted 1 line

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

Quoted 2 lines

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.

Quoted 2 lines

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

Quoted 6 lines

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.

Quoted 16 lines · Matti Vaittinen

Quoted 15 lines · Greg Kroah-Hartman

Quoted 13 lines · Matti

Quoted 7 lines · Greg Kroah-Hartman

Quoted 6 lines · Matti Vaittinen

Hi Greg,

Thanks for looking at this.

Quoted 1 line · Greg Kroah-Hartman

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)

Quoted 4 lines

Quoted 1 line

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.

Quoted 6 lines

If you only test

Quoted 2 lines

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.

Quoted 10 lines

Quoted 2 lines

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.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

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.

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

Quoted 8 lines

That's why we need

Quoted 6 lines

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.

Quoted 8 lines

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.

Quoted 15 lines · Matti

Quoted 14 lines · Greg Kroah-Hartman

Quoted 12 lines · Matti Vaittinen

Hi Greg,

Thanks for looking at this.

Quoted 1 line · Greg Kroah-Hartman

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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 16 lines · Greg Kroah-Hartman

Quoted 15 lines · Matti

Quoted 14 lines · Greg Kroah-Hartman

Quoted 12 lines · Matti Vaittinen

Hi Greg,

Thanks for looking at this.

Quoted 1 line · Greg Kroah-Hartman

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.

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

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

Quoted 34 lines · Matti Vaittinen

Quoted 16 lines · Greg Kroah-Hartman

Quoted 15 lines · Matti

Quoted 14 lines · Greg Kroah-Hartman

Quoted 12 lines · Matti Vaittinen

Hi Greg,

Thanks for looking at this.

Quoted 1 line · Greg Kroah-Hartman

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.

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

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;

Quoted 4 lines

         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.

Quoted 6 lines

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

Quoted 17 lines

Quoted 9 lines

Quoted 8 lines

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

Quoted 7 lines

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?

Quoted 6 lines

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.

Quoted 24 lines · Greg Kroah-Hartman

Quoted 23 lines

Quoted 9 lines

Quoted 8 lines

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

Quoted 7 lines

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.

Quoted 2 lines

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

Quoted 28 lines · Maxime Ripard

Quoted 26 lines · Greg Kroah-Hartman

Quoted 17 lines

Quoted 9 lines

Quoted 8 lines

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

Quoted 7 lines

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?

Quoted 6 lines

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.

Quoted 2 lines

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

Hi Maxime, all

Quoted 29 lines · Greg Kroah-Hartman

Quoted 28 lines · Maxime Ripard

Quoted 26 lines · Greg Kroah-Hartman

Quoted 17 lines

Quoted 9 lines

Quoted 8 lines

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

Quoted 7 lines

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?

Quoted 6 lines

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.

Quoted 6 lines

Quoted 2 lines

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

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/

Quoted 4 lines · Maxime Ripard

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?

Quoted 10 lines · Matti Vaittinen

Quoted 4 lines · Maxime Ripard

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.

Quoted 2 lines

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.

Quoted 7 lines

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.

Quoted 20 lines · Maxime Ripard

Quoted 10 lines · Matti Vaittinen

Quoted 4 lines · Maxime Ripard

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

Quoted 5 lines

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

Quoted 5 lines

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 8 lines

Quoted 7 lines

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?

Quoted 7 lines

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.

Quoted 10 lines · Matti Vaittinen

Quoted 3 lines · Maxime Ripard

Quoted 2 lines · Matti Vaittinen

Quoted 1 line · Maxime Ripard

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

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.

Quoted 3 lines

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.

Quoted 12 lines · David Gow

Quoted 10 lines · Matti Vaittinen

Quoted 3 lines · Maxime Ripard

Quoted 2 lines · Matti Vaittinen

Quoted 1 line · Maxime Ripard

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.

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 7 lines

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.

Quoted 8 lines

Quoted 3 lines

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.

Quoted 11 lines · Matti Vaittinen

Quoted 6 lines · David Gow

On Fri, 24 Mar 2023 at 14:11, Matti Vaittinen <[email protected]> wrote:

I think that sounds like a good strategy for now, and we can work on a set of 'generic helpers' which have an associated bus and struct kunit_device in the meantime. If we can continue to use root_device_register until those are ready, that'd be very convenient.

Would it be a tiny bit more acceptable if we did add a very simple:

Quoted 9 lines

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

Quoted 32 lines · David Gow

Quoted 11 lines · Matti Vaittinen

Quoted 6 lines · David Gow

On Fri, 24 Mar 2023 at 14:11, Matti Vaittinen <[email protected]> wrote:

I think that sounds like a good strategy for now, and we can work on a set of 'generic helpers' which have an associated bus and struct kunit_device in the meantime. If we can continue to use root_device_register until those are ready, that'd be very convenient.

Would it be a tiny bit more acceptable if we did add a very simple:

Quoted 9 lines

#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);

Quoted 4 lines

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.

Quoted 37 lines · Matti Vaittinen

Quoted 36 lines · David Gow

Quoted 12 lines · Matti Vaittinen

Quoted 7 lines · David Gow

On Fri, 24 Mar 2023 at 14:11, Matti Vaittinen
<[email protected]> wrote:

I think that sounds like a good strategy for now, and we can work on a set of 'generic helpers' which have an associated bus and struct kunit_device in the meantime. If we can continue to use root_device_register until those are ready, that'd be very convenient.

Would it be a tiny bit more acceptable if we did add a very simple:

Quoted 9 lines

#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);

Quoted 4 lines

Quoted 4 lines

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.

Quoted 39 lines · Matti Vaittinen

Quoted 37 lines · Matti Vaittinen

Quoted 36 lines · David Gow

Quoted 12 lines · Matti Vaittinen

Quoted 7 lines · David Gow

On Fri, 24 Mar 2023 at 14:11, Matti Vaittinen
<[email protected]> wrote:

I think that sounds like a good strategy for now, and we can work on a set of 'generic helpers' which have an associated bus and struct kunit_device in the meantime. If we can continue to use root_device_register until those are ready, that'd be very convenient.

Would it be a tiny bit more acceptable if we did add a very simple:

Quoted 9 lines

#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);

Quoted 9 lines

Quoted 4 lines

Quoted 4 lines

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.

Quoted 5 lines

If these wrappers don't yet include the automatic device clean-up - then it probably makes more sense to just do the kunit_root_device_* defines because the tests are likely to need removing the explicit device clean-ups when proper APIs are finished.

I sent out my prototype implementation of this here, which does do the automatic cleanup:
https://lore.kernel.org/linux-kselftest/[email protected]/T/#mf797239a8bce11630875fdf60aab9ed627add1f0

It's probably overkill to squeeze into your patch series, though, given it also adds and uses a whole new kunit_defer() API.

Quoted 22 lines · Matti Vaittinen

Quoted 20 lines · Maxime Ripard

Quoted 10 lines · Matti Vaittinen

Quoted 4 lines · Maxime Ripard

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

Quoted 8 lines

Quoted 5 lines

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.

Quoted 14 lines

Quoted 5 lines

Quoted 2 lines

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.

Quoted 8 lines

Quoted 2 lines

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.

Quoted 20 lines · Maxime Ripard

Quoted 10 lines · Matti Vaittinen

Quoted 4 lines · Maxime Ripard

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

Quoted 5 lines

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

Quoted 5 lines

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 8 lines

Quoted 7 lines

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?

Quoted 7 lines

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.

Quoted 35 lines · Matti Vaittinen

Quoted 20 lines · Maxime Ripard

Quoted 10 lines · Matti Vaittinen

Quoted 4 lines · Maxime Ripard

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

Quoted 5 lines

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

Quoted 5 lines

Quoted 2 lines

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.

Quoted 7 lines

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.

Quoted 32 lines

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 8 lines

Quoted 7 lines

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?

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 115 lines · Maxime Ripard

Quoted 35 lines · Matti Vaittinen

Quoted 20 lines · Maxime Ripard

Quoted 10 lines · Matti Vaittinen

Quoted 4 lines · Maxime Ripard

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

Quoted 5 lines

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

Quoted 5 lines

Quoted 2 lines

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.

Quoted 7 lines

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.

Quoted 32 lines

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 8 lines

Quoted 7 lines

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?

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 7 lines

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

drivers/base/core.c +11 -1

index 6878dfcbf0d6..adfec6185014 100644
@@ -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.

Quoted 35 lines · Matti Vaittinen

Quoted 20 lines · Maxime Ripard

Quoted 10 lines · Matti Vaittinen

Quoted 4 lines · Maxime Ripard

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

Quoted 5 lines

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

Quoted 5 lines

Quoted 2 lines

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.

Quoted 7 lines

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.

Quoted 43 lines · On Fri, 24 Mar 2023 13:31:57 +0100 Maxime Ripard <[email protected]> wrote:

Quoted 30 lines · Matti Vaittinen

Quoted 18 lines · Maxime Ripard

Quoted 9 lines · Matti Vaittinen

Quoted 4 lines · Maxime Ripard

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

Quoted 5 lines

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

Quoted 4 lines

Quoted 2 lines

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.

Quoted 7 lines

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.

Quoted 7 lines

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.

Quoted 67 lines · Jonathan Cameron

Quoted 43 lines · On Fri, 24 Mar 2023 13:31:57 +0100 Maxime Ripard <[email protected]> wrote:

Quoted 30 lines · Matti Vaittinen

Quoted 18 lines · Maxime Ripard

Quoted 9 lines · Matti Vaittinen

Quoted 4 lines · Maxime Ripard

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

Quoted 5 lines

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

Quoted 4 lines

Quoted 2 lines

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.

Quoted 7 lines

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.

Quoted 7 lines

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.

Quoted 4 lines

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.

Quoted 49 lines · On Fri, 24 Mar 2023 13:31:57 +0100 Maxime Ripard <[email protected]> wrote:

Quoted 35 lines · Matti Vaittinen

Quoted 20 lines · Maxime Ripard

Quoted 10 lines · Matti Vaittinen

Quoted 4 lines · Maxime Ripard

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

Quoted 5 lines

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

Quoted 5 lines

Quoted 2 lines

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.

Quoted 7 lines

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.

Quoted 7 lines

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.

Quoted 68 lines · Jonathan Cameron

Quoted 49 lines · On Fri, 24 Mar 2023 13:31:57 +0100 Maxime Ripard <[email protected]> wrote:

Quoted 35 lines · Matti Vaittinen

Quoted 20 lines · Maxime Ripard

Quoted 10 lines · Matti Vaittinen

Quoted 4 lines · Maxime Ripard

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

Quoted 5 lines

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

Quoted 5 lines

Quoted 2 lines

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.

Quoted 7 lines

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.

Quoted 7 lines

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

Quoted 5 lines

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

Quoted 5 lines

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

Quoted 28 lines · Maxime Ripard

Quoted 26 lines · Greg Kroah-Hartman

Quoted 17 lines

Quoted 9 lines

Quoted 8 lines

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

Quoted 7 lines

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?

Quoted 6 lines

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.

Quoted 2 lines

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

Quoted 29 lines · Greg Kroah-Hartman

Quoted 28 lines · Maxime Ripard

Quoted 26 lines · Greg Kroah-Hartman

Quoted 17 lines

Quoted 9 lines

Quoted 8 lines

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

Quoted 7 lines

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?

Quoted 6 lines

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 40 lines · Maxime Ripard

Quoted 29 lines · Greg Kroah-Hartman

Quoted 28 lines · Maxime Ripard

Quoted 26 lines · Greg Kroah-Hartman

Quoted 17 lines

Quoted 9 lines

Quoted 8 lines

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

Quoted 7 lines

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?

Quoted 6 lines

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.

Quoted 6 lines

Quoted 2 lines

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/

Quoted 4 lines

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?

Quoted 41 lines · Greg Kroah-Hartman

Quoted 40 lines · Maxime Ripard

Quoted 29 lines · Greg Kroah-Hartman

Quoted 28 lines · Maxime Ripard

Quoted 26 lines · Greg Kroah-Hartman

Quoted 17 lines

Quoted 9 lines

Quoted 8 lines

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

Quoted 7 lines

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?

Quoted 6 lines

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.

Quoted 6 lines

Quoted 2 lines

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/

Quoted 6 lines

Quoted 4 lines

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.

Quoted 2 lines

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?

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 <[email protected]>

---
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/[email protected]/

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

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 <[email protected]>

---
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/[email protected]/

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

drivers/base/test/Kconfig +5 -0

index 610a1ba7a467..642b5b183c10 100644
@@ -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

drivers/base/test/Makefile +2 -0

index 7f76fee6f989..49926524ec6f 100644
@@ -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)

drivers/base/test/test_kunit_device.c new file +3 -0

new file mode 100644
index 000000000000..75790e15b85c
@@ -0,0 +1,83 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * These helpers have been extracted from drm test code at

drivers/base/test/test_kunit_device.c new file +49 -0

+
+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;

drivers/base/test/test_kunit_device.c new file +11 -0

+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 <[email protected]>");
+MODULE_LICENSE("GPL");

include/kunit/platform_device.h new file +1 -0

new file mode 100644
index 000000000000..2a9c7bdd75eb
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */

include/kunit/platform_device.h new file +2 -1

+
+#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 =]

Quoted 21 lines · Matti Vaittinen

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 <[email protected]>

---
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/[email protected]/

Quoted 6 lines

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

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/[email protected]/
[3]: https://elixir.bootlin.com/linux/v6.2/source/drivers/gpu/drm/tests/drm_kunit_helpers.c#L39

Quoted 21 lines · Matti Vaittinen

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 <[email protected]>

---
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/[email protected]/

Quoted 6 lines

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

Some light sensors can adjust both the HW-gain and integration time. There are cases where adjusting the integration time has similar impact to the scale of the reported values as gain setting has.

IIO users do typically expect to handle scale by a single writable 'scale' entry. Driver should then adjust the gain/time accordingly.

It however is difficult for a driver to know whether it should change gain or integration time to meet the requested scale. Usually it is preferred to have longer integration time which usually improves accuracy, but there may be use-cases where long measurement times can be an issue. Thus it can be preferable to allow also changing the integration time - but mitigate the scale impact by also changing the gain underneath. Eg, if integration time change doubles the measured values, the driver can reduce the HW-gain to half.

The theory of the computations of gain-time-scale is simple. However, some people (undersigned) got that implemented wrong for more than once. Hence some gain-time-scale helpers were introduced.

Add some simple tests to verify the most hairy functions.

Signed-off-by Matti Vaittinen <[email protected]>

---
Changes:
v4 => v5:
- remove empty lines from Kconfig
- adapt to drop of the non devm iio_init

ROHM BU27034 is an ambient light sensor with 3 channels and 3 photo diodes capable of detecting a very wide range of illuminance. Typical application is adjusting LCD and backlight power of TVs and mobile phones.

Add initial support for the ROHM BU27034 ambient light sensor.

NOTE:
- Driver exposes 4 channels. One IIO_LIGHT channel providing the calculated lux values based on measured data from diodes #0 and #1. In addition, 3 IIO_INTENSITY channels are emitting the raw register data from all diodes for more intense user-space computations.
- Sensor has GAIN values that can be adjusted from 1x to 4096x.
- Sensor has adjustible measurement times of 5, 55, 100, 200 and 400 mS. Driver does not support 5 mS which has special limitations.
- Driver exposes standard 'scale' adjustment which is implemented by:
1) Trying to adjust only the GAIN
2) If GAIN adjustment alone can't provide requested scale, adjusting both the time and the gain is attempted.
- Driver exposes writable INT_TIME property that can be used for adjusting the measurement time. Time adjustment will also cause the driver to try to adjust the GAIN so that the overall scale is kept as close to the original as possible.

Signed-off-by Matti Vaittinen <[email protected]>

drivers/iio/light/rohm-bu27034.c new file +8 -0

+
+static u64 bu27034_fixp_calc_t1(unsigned int coeff, unsigned int ch0,
+				unsigned int ch1, unsigned int gain0,
+				unsigned int gain1)
+{
+	unsigned int helper, tmp;
+	u64 helper64;
+

drivers/iio/light/rohm-bu27034.c new file +19 -0

+		return tmp / (gain1 * gain1) / ch0;
+
+	helper = gain1 * gain1;
+	if (helper > ch0) {
+		do_div(helper64, helper);
+
+		return gain_mul_div_helper(helper64, gain0, ch0);
+	}
+
+	do_div(helper64, ch0);
+
+	return gain_mul_div_helper(helper64, gain0, helper);
+}
+
+static u64 bu27034_fixp_calc_t23(unsigned int coeff, unsigned int ch,
+				 unsigned int gain)
+{
+	unsigned int helper;
+	u64 helper64;

drivers/iio/light/rohm-bu27034.c new file +3 -0

+	 * easy to spot from the buffers especially if raw-data channels show
+	 * valid values
+	 */

Quoted 27 lines · On Wed, 22 Mar 2023 11:07:56 +0200 Matti Vaittinen <[email protected]>…

ROHM BU27034 is an ambient light sensor with 3 channels and 3 photo diodes capable of detecting a very wide range of illuminance. Typical application is adjusting LCD and backlight power of TVs and mobile phones.

Add initial support for the ROHM BU27034 ambient light sensor.

NOTE:
- Driver exposes 4 channels. One IIO_LIGHT channel providing the calculated lux values based on measured data from diodes #0 and #1. In addition, 3 IIO_INTENSITY channels are emitting the raw register data from all diodes for more intense user-space computations.
- Sensor has GAIN values that can be adjusted from 1x to 4096x.
- Sensor has adjustible measurement times of 5, 55, 100, 200 and 400 mS. Driver does not support 5 mS which has special limitations.
- Driver exposes standard 'scale' adjustment which is implemented by:
1) Trying to adjust only the GAIN
2) If GAIN adjustment alone can't provide requested scale, adjusting both the time and the gain is attempted.
- Driver exposes writable INT_TIME property that can be used for adjusting the measurement time. Time adjustment will also cause the driver to try to adjust the GAIN so that the overall scale is kept as close to the original as possible.

Quoted 2 lines

Signed-off-by Matti Vaittinen <[email protected]>

Hi Matti,

A few minor comments inline. I'll take a closer look at the rest of the series when the discussions around the tests and devices to be used for them settle down.

Thanks,

Jonathan

Quoted 8 lines

drivers/iio/light/rohm-bu27034.c new file +8 -0

+
+static u64 bu27034_fixp_calc_t1(unsigned int coeff, unsigned int ch0,
+				unsigned int ch1, unsigned int gain0,
+				unsigned int gain1)
+{
+	unsigned int helper, tmp;
+	u64 helper64;
+

drivers/iio/light/rohm-bu27034.c line 833

Quoted 10 lines

drivers/iio/light/rohm-bu27034.c new file +10 -0

+	/*
+	 * Here we could overflow even the 64bit value. Hence we
+	 * multiply with gain0 only after the divisions - even though
+	 * it may result loss of accuracy
+	 */
+	helper64 = (u64)coeff * (u64)ch1 * (u64)ch1;
+	helper = coeff * ch1 * ch1;
+	tmp = helper * gain0;
+
+	if (helper == helper64 && (tmp / gain0 == helper))

Similar to below. Don't bother with the non 64 bit version.

Quoted 24 lines

drivers/iio/light/rohm-bu27034.c new file +24 -0

+		return tmp / (gain1 * gain1) / ch0;
+
+	helper = gain1 * gain1;
+	if (helper > ch0) {
+		do_div(helper64, helper);
+
+		return gain_mul_div_helper(helper64, gain0, ch0);
+	}
+
+	do_div(helper64, ch0);
+
+	return gain_mul_div_helper(helper64, gain0, helper);
+}
+
+static u64 bu27034_fixp_calc_t23(unsigned int coeff, unsigned int ch,
+				 unsigned int gain)
+{
+	unsigned int helper;
+	u64 helper64;
+
+	helper64 = (u64)coeff * (u64)ch;
+	helper = coeff * ch;
+
+	if (helper == helper64)

drivers/iio/light/rohm-bu27034.c line 862

Quoted 5 lines

drivers/iio/light/rohm-bu27034.c new file +5 -0

+		return helper / gain;
+
+	do_div(helper64, gain);
+
+	return helper64;

I suspect that this is a premature bit of optimization so I'd just do it in 64 bits always.

Also, if you did want to do this, check_mul_overflow() etc would help. (linux/overflow.h)

Revision history:
v4 => v5: Mostly fixes to review comments from Andy and Jonathan.
- more accurate change-log in individual patches

- copy code from DRM test helper instead of moving it to simplify merging

- document all exported GTS helpers.
- inline a few GTS helpers
- use again Milli lux for the bu27034 with RAW IIO_LIGHT channel and scale
- Fix bug from added in v4 bu27034 time setting.

Quoted 7 lines

Revision history:
v4 => v5: Mostly fixes to review comments from Andy and Jonathan.
- more accurate change-log in individual patches

- copy code from DRM test helper instead of moving it to simplify merging

1) Why do you think this is a problem?
2) How would we avoid spreading more copies of the same code in the future?

1) Merge conflicts is not a bad thing. It shows that people tested their code in isolation and stabilized it before submitting to the upper maintainer.

https://yarchive.net/comp/linux/git_merges_from_upstream.html

2) Spreading the same code when we _know_ this, should be very well justified. Merge conflict is an administrative point, and not a technical obstacle to avoid.

Andy Shevchenko <[email protected]> writes:

Hello Andy,

Quoted 21 lines · Matti Vaittinen

Quoted 7 lines

Revision history:
v4 => v5: Mostly fixes to review comments from Andy and Jonathan.
- more accurate change-log in individual patches

- copy code from DRM test helper instead of moving it to simplify merging

1) Why do you think this is a problem?
2) How would we avoid spreading more copies of the same code in the future?

1) Merge conflicts is not a bad thing. It shows that people tested their code in isolation and stabilized it before submitting to the upper maintainer.

https://yarchive.net/comp/linux/git_merges_from_upstream.html

2) Spreading the same code when we _know_ this, should be very well justified. Merge conflict is an administrative point, and not a technical obstacle to avoid.

I believe this was suggested by Maxime and the rationale is that by just copying the helpers for now, that would make it easier to land instead of requiring coordination between different subystems.

Otherwise the IIO tree will need to provide an inmutable branch for the DRM tree to merge and so on.

I agree with Maxime that a little bit of duplication (that can be cleaned up by each subsystem at their own pace) is the path of least resistance.

Quoted 1 line · Javier Martinez Canillas

Quoted 1 line · Matti Vaittinen

Quoted 7 lines

Quoted 7 lines

Quoted 2 lines

- copy code from DRM test helper instead of moving it to simplify merging

1) Why do you think this is a problem?
2) How would we avoid spreading more copies of the same code in the future?

Quoted 8 lines

Quoted 8 lines

1) Merge conflicts is not a bad thing. It shows that people tested their code in isolation and stabilized it before submitting to the upper maintainer.

https://yarchive.net/comp/linux/git_merges_from_upstream.html

2) Spreading the same code when we _know_ this, should be very well justified. Merge conflict is an administrative point, and not a technical obstacle to avoid.

I definitely agree. This is also why I did the renaming and not copying in the first version. In this version I did still add the subsequent patch 2/8
- which drops the duplicates from DRM tree.

Quoted 3 lines

I believe this was suggested by Maxime and the rationale is that by just copying the helpers for now, that would make it easier to land instead of requiring coordination between different subystems.

This is correct.

Quoted 2 lines

Otherwise the IIO tree will need to provide an inmutable branch for the DRM tree to merge and so on.

Or, if we carry the patch 1/8 via self-test tree, then we get even more players here.

Still, I am not opposing immutable branch because that would allow fast applying of the patch 2/8 as well. Longer that is delayed, more likely we will see more users of the DRM helpers and harder it gets to remove the duplicates later.

Quoted 2 lines

I agree with Maxime that a little bit of duplication (that can be cleaned up by each subsystem at their own pace) is the path of least resistance.

I'd say this depends. It probably is the path of least resistance for people maintaining the trees. It can also be the path of least resistance in general - but it depends on if there will be no new users for those DRM helpers while waiting the new APIs being merged in DRM tree. More users we see in DRM, more effort the clean-up requires.

I have no strong opinion on this specific case. I'd just be happy to see the IIO tests getting in preferably sooner than later - although 'soon' and 'late' does also depend on other factors besides these helpers...

Quoted 2 lines

I agree with Maxime that a little bit of duplication (that can be cleaned up by each subsystem at their own pace) is the path of least resistance.

I'd say this depends. It probably is the path of least resistance for people maintaining the trees. It can also be the path of least resistance in general - but it depends on if there will be no new users for those DRM helpers while waiting the new APIs being merged in DRM tree. More users we see in DRM, more effort the clean-up requires.