[PATCH v2 0/3] drivers: base: Add tests showing devm handling inconsistencies

[PATCH v2 0/3] drivers: base: Add tests showing devm handling inconsistencies

[Maxime Ripard]

Hi,

This follows the discussion here:
https://lore.kernel.org/linux-kselftest/20230324123157.bbwvfq4gsxnlnfwb@houat/

This shows a couple of inconsistencies with regard to how device-managed
resources are cleaned up. Basically, devm resources will only be cleaned up
if the device is attached to a bus and bound to a driver. Failing any of
these cases, a call to device_unregister will not end up in the devm
resources being released.

We had to work around it in DRM to provide helpers to create a device for
kunit tests, but the current discussion around creating similar, generic,
helpers for kunit resumed interest in fixing this.

This can be tested using the command:
./tools/testing/kunit/kunit.py run --kunitconfig=drivers/base/test/

I added the fix David suggested back in that discussion which does fix
the tests. The SoB is missing, since David didn't provide it back then.

Let me know what you think,
Maxime

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
Changes in v2:
- Use an init function
- Document the tests
- Add a fix for the bugs
- Link to v1: https://lore.kernel.org/r/20230329-kunit-devm-inconsistencies-test-v1-0-c33127048375@cerno.tech

---
David Gow (1):
      drivers: base: Free devm resources when unregistering a device

Maxime Ripard (2):
      drivers: base: Add basic devm tests for root devices
      drivers: base: Add basic devm tests for platform devices

 drivers/base/core.c                      |  11 ++
 drivers/base/test/.kunitconfig           |   2 +
 drivers/base/test/Kconfig                |   4 +
 drivers/base/test/Makefile               |   3 +
 drivers/base/test/platform-device-test.c | 220 +++++++++++++++++++++++++++++++
 drivers/base/test/root-device-test.c     | 108 +++++++++++++++
 6 files changed, 348 insertions(+)
---
base-commit: 53cdf865f90ba922a854c65ed05b519f9d728424
change-id: 20230329-kunit-devm-inconsistencies-test-5e5a7d01e60d

Best regards,
-- 
Maxime Ripard <mripard@kernel.org>

[PATCH v2 1/3] drivers: base: Add basic devm tests for root devices

[Maxime Ripard]

The root devices show some odd behaviours compared to regular "bus" devices
that have been probed through the usual mechanism, so let's create kunit
tests to exercise those paths and odd cases.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/base/test/.kunitconfig       |   2 +
 drivers/base/test/Kconfig            |   4 ++
 drivers/base/test/Makefile           |   2 +
 drivers/base/test/root-device-test.c | 110 +++++++++++++++++++++++++++++++++++
 4 files changed, 118 insertions(+)

diff --git a/drivers/base/test/.kunitconfig b/drivers/base/test/.kunitconfig
new file mode 100644
index 000000000000..473923f0998b
--- /dev/null
+++ b/drivers/base/test/.kunitconfig
@@ -0,0 +1,2 @@
+CONFIG_KUNIT=y
+CONFIG_DM_KUNIT_TEST=y
diff --git a/drivers/base/test/Kconfig b/drivers/base/test/Kconfig
index 610a1ba7a467..9d42051f8f8e 100644
--- a/drivers/base/test/Kconfig
+++ b/drivers/base/test/Kconfig
@@ -9,6 +9,10 @@ config TEST_ASYNC_DRIVER_PROBE
 
 	  If unsure say N.
 
+config DM_KUNIT_TEST
+	tristate "KUnit Tests for the device model" if !KUNIT_ALL_TESTS
+	depends on KUNIT
+
 config DRIVER_PE_KUNIT_TEST
 	bool "KUnit Tests for property entry API" if !KUNIT_ALL_TESTS
 	depends on KUNIT=y
diff --git a/drivers/base/test/Makefile b/drivers/base/test/Makefile
index 7f76fee6f989..d589ca3fa8fc 100644
--- a/drivers/base/test/Makefile
+++ b/drivers/base/test/Makefile
@@ -1,5 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_TEST_ASYNC_DRIVER_PROBE)	+= test_async_driver_probe.o
 
+obj-$(CONFIG_DM_KUNIT_TEST)	+= root-device-test.o
+
 obj-$(CONFIG_DRIVER_PE_KUNIT_TEST) += property-entry-test.o
 CFLAGS_property-entry-test.o += $(DISABLE_STRUCTLEAK_PLUGIN)
diff --git a/drivers/base/test/root-device-test.c b/drivers/base/test/root-device-test.c
new file mode 100644
index 000000000000..9a3e6cccae13
--- /dev/null
+++ b/drivers/base/test/root-device-test.c
@@ -0,0 +1,110 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright 2023 Maxime Ripard <mripard@kernel.org>
+
+#include <kunit/resource.h>
+
+#include <linux/device.h>
+
+#define DEVICE_NAME "test"
+
+struct test_priv {
+	bool probe_done;
+	bool release_done;
+	wait_queue_head_t release_wq;
+	struct device *dev;
+};
+
+static int root_device_devm_init(struct kunit *test)
+{
+	struct test_priv *priv;
+
+	priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+	init_waitqueue_head(&priv->release_wq);
+
+	test->priv = priv;
+
+	return 0;
+}
+
+static void devm_device_action(void *ptr)
+{
+	struct test_priv *priv = ptr;
+
+	priv->release_done = true;
+	wake_up_interruptible(&priv->release_wq);
+}
+
+#define RELEASE_TIMEOUT_MS	100
+
+/*
+ * Tests that a bus-less, non-probed device will run its device-managed
+ * actions when unregistered.
+ */
+static void root_device_devm_register_unregister_test(struct kunit *test)
+{
+	struct test_priv *priv = test->priv;
+	int ret;
+
+	priv->dev = root_device_register(DEVICE_NAME);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->dev);
+
+	ret = devm_add_action_or_reset(priv->dev, devm_device_action, priv);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	root_device_unregister(priv->dev);
+
+	ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+					       msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+	KUNIT_EXPECT_GT(test, ret, 0);
+}
+
+static void devm_put_device_action(void *ptr)
+{
+	struct test_priv *priv = ptr;
+
+	put_device(priv->dev);
+	priv->release_done = true;
+	wake_up_interruptible(&priv->release_wq);
+}
+
+/*
+ * Tests that a bus-less, non-probed device will run its device-managed
+ * actions when unregistered, even if someone still holds a reference to
+ * it.
+ */
+static void root_device_devm_register_get_unregister_with_devm_test(struct kunit *test)
+{
+	struct test_priv *priv = test->priv;
+	int ret;
+
+	kunit_skip(test, "This needs to be fixed in the core.");
+
+	priv->dev = root_device_register(DEVICE_NAME);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->dev);
+
+	get_device(priv->dev);
+
+	ret = devm_add_action_or_reset(priv->dev, devm_put_device_action, priv);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	root_device_unregister(priv->dev);
+
+	ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+					       msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+	KUNIT_EXPECT_GT(test, ret, 0);
+}
+
+static struct kunit_case root_device_devm_tests[] = {
+	KUNIT_CASE(root_device_devm_register_unregister_test),
+	KUNIT_CASE(root_device_devm_register_get_unregister_with_devm_test),
+	{}
+};
+
+static struct kunit_suite root_device_devm_test_suite = {
+	.name = "root-device-devm",
+	.init = root_device_devm_init,
+	.test_cases = root_device_devm_tests,
+};
+
+kunit_test_suite(root_device_devm_test_suite);

-- 
2.40.0

[PATCH v2 2/3] drivers: base: Add basic devm tests for platform devices

[Maxime Ripard]

Platform devices show some inconsistencies with how devm resources are
released when the device has been probed and when it hasn't. Let's add a
few tests to exercise thos paths and odd cases.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/base/test/Makefile               |   1 +
 drivers/base/test/platform-device-test.c | 222 +++++++++++++++++++++++++++++++
 2 files changed, 223 insertions(+)

diff --git a/drivers/base/test/Makefile b/drivers/base/test/Makefile
index d589ca3fa8fc..e321dfc7e922 100644
--- a/drivers/base/test/Makefile
+++ b/drivers/base/test/Makefile
@@ -2,6 +2,7 @@
 obj-$(CONFIG_TEST_ASYNC_DRIVER_PROBE)	+= test_async_driver_probe.o
 
 obj-$(CONFIG_DM_KUNIT_TEST)	+= root-device-test.o
+obj-$(CONFIG_DM_KUNIT_TEST)	+= platform-device-test.o
 
 obj-$(CONFIG_DRIVER_PE_KUNIT_TEST) += property-entry-test.o
 CFLAGS_property-entry-test.o += $(DISABLE_STRUCTLEAK_PLUGIN)
diff --git a/drivers/base/test/platform-device-test.c b/drivers/base/test/platform-device-test.c
new file mode 100644
index 000000000000..b6ebf1dcdffb
--- /dev/null
+++ b/drivers/base/test/platform-device-test.c
@@ -0,0 +1,222 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <kunit/resource.h>
+
+#include <linux/device.h>
+#include <linux/platform_device.h>
+
+#define DEVICE_NAME "test"
+
+struct test_priv {
+	bool probe_done;
+	bool release_done;
+	wait_queue_head_t probe_wq;
+	wait_queue_head_t release_wq;
+	struct device *dev;
+};
+
+static int platform_device_devm_init(struct kunit *test)
+{
+	struct test_priv *priv;
+
+	priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+	init_waitqueue_head(&priv->probe_wq);
+	init_waitqueue_head(&priv->release_wq);
+
+	test->priv = priv;
+
+	return 0;
+}
+
+static void devm_device_action(void *ptr)
+{
+	struct test_priv *priv = ptr;
+
+	priv->release_done = true;
+	wake_up_interruptible(&priv->release_wq);
+}
+
+static void devm_put_device_action(void *ptr)
+{
+	struct test_priv *priv = ptr;
+
+	put_device(priv->dev);
+	priv->release_done = true;
+	wake_up_interruptible(&priv->release_wq);
+}
+
+#define RELEASE_TIMEOUT_MS	100
+
+/*
+ * Tests that a platform bus, non-probed device will run its
+ * device-managed actions when unregistered.
+ */
+static void platform_device_devm_register_unregister_test(struct kunit *test)
+{
+	struct platform_device *pdev;
+	struct test_priv *priv = test->priv;
+	int ret;
+
+	pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
+
+	ret = platform_device_add(pdev);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	priv->dev = &pdev->dev;
+
+	ret = devm_add_action_or_reset(priv->dev, devm_device_action, priv);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	platform_device_unregister(pdev);
+
+	ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+					       msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+	KUNIT_EXPECT_GT(test, ret, 0);
+}
+
+/*
+ * Tests that a platform bus, non-probed device will run its
+ * device-managed actions when unregistered, even if someone still holds
+ * a reference to it.
+ */
+static void platform_device_devm_register_get_unregister_with_devm_test(struct kunit *test)
+{
+	struct platform_device *pdev;
+	struct test_priv *priv = test->priv;
+	int ret;
+
+	kunit_skip(test, "This needs to be fixed in the core.");
+
+	pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
+
+	ret = platform_device_add(pdev);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	priv->dev = &pdev->dev;
+
+	get_device(priv->dev);
+
+	ret = devm_add_action_or_reset(priv->dev, devm_put_device_action, priv);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	platform_device_unregister(pdev);
+
+	ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+					       msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+	KUNIT_EXPECT_GT(test, ret, 0);
+}
+
+static int fake_probe(struct platform_device *pdev)
+{
+	struct test_priv *priv = platform_get_drvdata(pdev);
+
+	priv->probe_done = true;
+	wake_up_interruptible(&priv->probe_wq);
+
+	return 0;
+}
+
+static struct platform_driver fake_driver = {
+	.probe	= fake_probe,
+	.driver = {
+		.name = DEVICE_NAME,
+	},
+};
+
+/*
+ * Tests that a platform bus, probed device will run its device-managed
+ * actions when unregistered.
+ */
+static void probed_platform_device_devm_register_unregister_test(struct kunit *test)
+{
+	struct platform_device *pdev;
+	struct test_priv *priv = test->priv;
+	int ret;
+
+	ret = platform_driver_register(&fake_driver);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
+
+	priv->dev = &pdev->dev;
+	platform_set_drvdata(pdev, priv);
+
+	ret = platform_device_add(pdev);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	ret = wait_event_interruptible_timeout(priv->probe_wq, priv->probe_done,
+					       msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+	KUNIT_ASSERT_GT(test, ret, 0);
+
+	ret = devm_add_action_or_reset(priv->dev, devm_device_action, priv);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	platform_device_unregister(pdev);
+
+	ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+					       msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+	KUNIT_EXPECT_GT(test, ret, 0);
+
+	platform_driver_unregister(&fake_driver);
+}
+
+/*
+ * Tests that a platform bus, probed device will run its device-managed
+ * actions when unregistered, even if someone still holds a reference to
+ * it.
+ */
+static void probed_platform_device_devm_register_get_unregister_with_devm_test(struct kunit *test)
+{
+	struct platform_device *pdev;
+	struct test_priv *priv = test->priv;
+	int ret;
+
+	ret = platform_driver_register(&fake_driver);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
+
+	priv->dev = &pdev->dev;
+	platform_set_drvdata(pdev, priv);
+
+	ret = platform_device_add(pdev);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	ret = wait_event_interruptible_timeout(priv->probe_wq, priv->probe_done,
+					       msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+	KUNIT_ASSERT_GT(test, ret, 0);
+
+	get_device(priv->dev);
+
+	ret = devm_add_action_or_reset(priv->dev, devm_put_device_action, priv);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	platform_device_unregister(pdev);
+
+	ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+					       msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+	KUNIT_EXPECT_GT(test, ret, 0);
+
+	platform_driver_unregister(&fake_driver);
+}
+
+static struct kunit_case platform_device_devm_tests[] = {
+	KUNIT_CASE(platform_device_devm_register_unregister_test),
+	KUNIT_CASE(platform_device_devm_register_get_unregister_with_devm_test),
+	KUNIT_CASE(probed_platform_device_devm_register_unregister_test),
+	KUNIT_CASE(probed_platform_device_devm_register_get_unregister_with_devm_test),
+	{}
+};
+
+static struct kunit_suite platform_device_devm_test_suite = {
+	.name = "platform-device-devm",
+	.init = platform_device_devm_init,
+	.test_cases = platform_device_devm_tests,
+};
+
+kunit_test_suite(platform_device_devm_test_suite);

-- 
2.40.0

[PATCH v2 3/3] drivers: base: Free devm resources when unregistering a device

[Maxime Ripard]

From: David Gow <davidgow@google.com>

In the current code, devres_release_all() only gets called if the device
has a bus and has been probed.

This leads to issues when using bus-less or driver-less devices where
the device might never get freed if a managed resource holds a reference
to the device. This is happening in the DRM framework for example.

We should thus call devres_release_all() in the device_del() function to
make sure that the device-managed actions are properly executed when the
device is unregistered, even if it has neither a bus nor a driver.

This is effectively the same change than commit 2f8d16a996da ("devres:
release resources on device_del()") that got reverted by commit
a525a3ddeaca ("driver core: free devres in device_release") over
use-after-free concerns.

It's not clear whether those concerns are legitimate though, but I would
expect drivers not to register new resources in their device-managed
actions.

Fixes: a525a3ddeaca ("driver core: free devres in device_release")
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/base/core.c                      | 11 +++++++++++
 drivers/base/test/platform-device-test.c |  2 --
 drivers/base/test/root-device-test.c     |  2 --
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 3dff5037943e..6ceaf50f5a67 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3817,6 +3817,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);
diff --git a/drivers/base/test/platform-device-test.c b/drivers/base/test/platform-device-test.c
index b6ebf1dcdffb..1ae5ce8bd366 100644
--- a/drivers/base/test/platform-device-test.c
+++ b/drivers/base/test/platform-device-test.c
@@ -87,8 +87,6 @@ static void platform_device_devm_register_get_unregister_with_devm_test(struct k
 	struct test_priv *priv = test->priv;
 	int ret;
 
-	kunit_skip(test, "This needs to be fixed in the core.");
-
 	pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
 
diff --git a/drivers/base/test/root-device-test.c b/drivers/base/test/root-device-test.c
index 9a3e6cccae13..780d07455f57 100644
--- a/drivers/base/test/root-device-test.c
+++ b/drivers/base/test/root-device-test.c
@@ -78,8 +78,6 @@ static void root_device_devm_register_get_unregister_with_devm_test(struct kunit
 	struct test_priv *priv = test->priv;
 	int ret;
 
-	kunit_skip(test, "This needs to be fixed in the core.");
-
 	priv->dev = root_device_register(DEVICE_NAME);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->dev);
 

-- 
2.40.0

Re: [PATCH v2 0/3] drivers: base: Add tests showing devm handling inconsistencies

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1030 bytes --]

On Wed, Jun 28, 2023 at 11:49:20AM +0200, Maxime Ripard wrote:
> Hi,
> 
> This follows the discussion here:
> https://lore.kernel.org/linux-kselftest/20230324123157.bbwvfq4gsxnlnfwb@houat/
> 
> This shows a couple of inconsistencies with regard to how device-managed
> resources are cleaned up. Basically, devm resources will only be cleaned up
> if the device is attached to a bus and bound to a driver. Failing any of
> these cases, a call to device_unregister will not end up in the devm
> resources being released.
> 
> We had to work around it in DRM to provide helpers to create a device for
> kunit tests, but the current discussion around creating similar, generic,
> helpers for kunit resumed interest in fixing this.
> 
> This can be tested using the command:
> ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/base/test/
> 
> I added the fix David suggested back in that discussion which does fix
> the tests. The SoB is missing, since David didn't provide it back then.

Ping?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 0/3] drivers: base: Add tests showing devm handling inconsistencies

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 2352 bytes --]

On Wed, 28 Jun 2023 at 17:50, Maxime Ripard <mripard@kernel.org> wrote:
>
> Hi,
>
> This follows the discussion here:
> https://lore.kernel.org/linux-kselftest/20230324123157.bbwvfq4gsxnlnfwb@houat/
>
> This shows a couple of inconsistencies with regard to how device-managed
> resources are cleaned up. Basically, devm resources will only be cleaned up
> if the device is attached to a bus and bound to a driver. Failing any of
> these cases, a call to device_unregister will not end up in the devm
> resources being released.
>
> We had to work around it in DRM to provide helpers to create a device for
> kunit tests, but the current discussion around creating similar, generic,
> helpers for kunit resumed interest in fixing this.
>
> This can be tested using the command:
> ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/base/test/
>
> I added the fix David suggested back in that discussion which does fix
> the tests. The SoB is missing, since David didn't provide it back then.
>
> Let me know what you think,
> Maxime
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Whoops, sorry. This managed to hide in my inbox. I think it looks
good, and am happy for you to add by SoB to the patch.

I've sent out reviews to the others with some small nitpicky things.

Thanks a bunch,
-- David

> Changes in v2:
> - Use an init function
> - Document the tests
> - Add a fix for the bugs
> - Link to v1: https://lore.kernel.org/r/20230329-kunit-devm-inconsistencies-test-v1-0-c33127048375@cerno.tech
>
> ---
> David Gow (1):
>       drivers: base: Free devm resources when unregistering a device
>
> Maxime Ripard (2):
>       drivers: base: Add basic devm tests for root devices
>       drivers: base: Add basic devm tests for platform devices
>
>  drivers/base/core.c                      |  11 ++
>  drivers/base/test/.kunitconfig           |   2 +
>  drivers/base/test/Kconfig                |   4 +
>  drivers/base/test/Makefile               |   3 +
>  drivers/base/test/platform-device-test.c | 220 +++++++++++++++++++++++++++++++
>  drivers/base/test/root-device-test.c     | 108 +++++++++++++++
>  6 files changed, 348 insertions(+)
> ---
> base-commit: 53cdf865f90ba922a854c65ed05b519f9d728424
> change-id: 20230329-kunit-devm-inconsistencies-test-5e5a7d01e60d
>
> Best regards,
> --
> Maxime Ripard <mripard@kernel.org>
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH v2 1/3] drivers: base: Add basic devm tests for root devices

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 6549 bytes --]

On Wed, 28 Jun 2023 at 17:49, Maxime Ripard <mripard@kernel.org> wrote:
>
> The root devices show some odd behaviours compared to regular "bus" devices
> that have been probed through the usual mechanism, so let's create kunit
> tests to exercise those paths and odd cases.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: David Gow <davidgow@google.com>

There's definitely an argument that root devices are not supposed to
be like regular devices, and so devm_ managed resources aren't
supposed to work with them. Either way:
- It needs to be documented somewhere (and this test makes for good
documentation, IMO).
- It should be consistent: if devm_ isn't to be used with root
devices, it should fail everywhere, and if it is, it should work in
all the cases below.

So I'm all in favour of including this test and making root devices work.

That being said, I am planning to send out a patchset adding a struct
kunit_device for use in tests, which will be attached to a "kunit"
bus. I think the combination of "fix devm_ with root devices" and
"don't recommend root devices as a 'fake' device for testing" is
probably the longer-term solution everyone can agree upon?

Cheers,
-- David


>  drivers/base/test/.kunitconfig       |   2 +
>  drivers/base/test/Kconfig            |   4 ++
>  drivers/base/test/Makefile           |   2 +
>  drivers/base/test/root-device-test.c | 110 +++++++++++++++++++++++++++++++++++
>  4 files changed, 118 insertions(+)
>
> diff --git a/drivers/base/test/.kunitconfig b/drivers/base/test/.kunitconfig
> new file mode 100644
> index 000000000000..473923f0998b
> --- /dev/null
> +++ b/drivers/base/test/.kunitconfig
> @@ -0,0 +1,2 @@
> +CONFIG_KUNIT=y
> +CONFIG_DM_KUNIT_TEST=y
> diff --git a/drivers/base/test/Kconfig b/drivers/base/test/Kconfig
> index 610a1ba7a467..9d42051f8f8e 100644
> --- a/drivers/base/test/Kconfig
> +++ b/drivers/base/test/Kconfig
> @@ -9,6 +9,10 @@ config TEST_ASYNC_DRIVER_PROBE
>
>           If unsure say N.
>
> +config DM_KUNIT_TEST
> +       tristate "KUnit Tests for the device model" if !KUNIT_ALL_TESTS
> +       depends on KUNIT

Could we add "default KUNIT_ALL_TESTS" here? Or if there's a good
reason to not make this run by default, remove the "if
!KUNIT_ALL_TESTS" condition above.


> +
>  config DRIVER_PE_KUNIT_TEST
>         bool "KUnit Tests for property entry API" if !KUNIT_ALL_TESTS
>         depends on KUNIT=y
> diff --git a/drivers/base/test/Makefile b/drivers/base/test/Makefile
> index 7f76fee6f989..d589ca3fa8fc 100644
> --- a/drivers/base/test/Makefile
> +++ b/drivers/base/test/Makefile
> @@ -1,5 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_TEST_ASYNC_DRIVER_PROBE)  += test_async_driver_probe.o
>
> +obj-$(CONFIG_DM_KUNIT_TEST)    += root-device-test.o
> +
>  obj-$(CONFIG_DRIVER_PE_KUNIT_TEST) += property-entry-test.o
>  CFLAGS_property-entry-test.o += $(DISABLE_STRUCTLEAK_PLUGIN)
> diff --git a/drivers/base/test/root-device-test.c b/drivers/base/test/root-device-test.c
> new file mode 100644
> index 000000000000..9a3e6cccae13
> --- /dev/null
> +++ b/drivers/base/test/root-device-test.c
> @@ -0,0 +1,110 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright 2023 Maxime Ripard <mripard@kernel.org>
> +
> +#include <kunit/resource.h>
> +
> +#include <linux/device.h>
> +
> +#define DEVICE_NAME "test"
> +
> +struct test_priv {
> +       bool probe_done;
> +       bool release_done;
> +       wait_queue_head_t release_wq;
> +       struct device *dev;
> +};
> +
> +static int root_device_devm_init(struct kunit *test)
> +{
> +       struct test_priv *priv;
> +
> +       priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
> +       init_waitqueue_head(&priv->release_wq);
> +
> +       test->priv = priv;
> +
> +       return 0;
> +}
> +
> +static void devm_device_action(void *ptr)
> +{
> +       struct test_priv *priv = ptr;
> +
> +       priv->release_done = true;
> +       wake_up_interruptible(&priv->release_wq);
> +}
> +
> +#define RELEASE_TIMEOUT_MS     100
> +
> +/*
> + * Tests that a bus-less, non-probed device will run its device-managed
> + * actions when unregistered.
> + */
> +static void root_device_devm_register_unregister_test(struct kunit *test)
> +{
> +       struct test_priv *priv = test->priv;
> +       int ret;
> +
> +       priv->dev = root_device_register(DEVICE_NAME);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->dev);
> +
> +       ret = devm_add_action_or_reset(priv->dev, devm_device_action, priv);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       root_device_unregister(priv->dev);
> +
> +       ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
> +                                              msecs_to_jiffies(RELEASE_TIMEOUT_MS));
> +       KUNIT_EXPECT_GT(test, ret, 0);
> +}
> +
> +static void devm_put_device_action(void *ptr)
> +{
> +       struct test_priv *priv = ptr;
> +
> +       put_device(priv->dev);
> +       priv->release_done = true;
> +       wake_up_interruptible(&priv->release_wq);
> +}
> +
> +/*
> + * Tests that a bus-less, non-probed device will run its device-managed
> + * actions when unregistered, even if someone still holds a reference to
> + * it.
> + */
> +static void root_device_devm_register_get_unregister_with_devm_test(struct kunit *test)
> +{
> +       struct test_priv *priv = test->priv;
> +       int ret;
> +
> +       kunit_skip(test, "This needs to be fixed in the core.");
> +
> +       priv->dev = root_device_register(DEVICE_NAME);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->dev);
> +
> +       get_device(priv->dev);
> +
> +       ret = devm_add_action_or_reset(priv->dev, devm_put_device_action, priv);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       root_device_unregister(priv->dev);
> +
> +       ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
> +                                              msecs_to_jiffies(RELEASE_TIMEOUT_MS));
> +       KUNIT_EXPECT_GT(test, ret, 0);
> +}
> +
> +static struct kunit_case root_device_devm_tests[] = {
> +       KUNIT_CASE(root_device_devm_register_unregister_test),
> +       KUNIT_CASE(root_device_devm_register_get_unregister_with_devm_test),
> +       {}
> +};
> +
> +static struct kunit_suite root_device_devm_test_suite = {
> +       .name = "root-device-devm",
> +       .init = root_device_devm_init,
> +       .test_cases = root_device_devm_tests,
> +};
> +
> +kunit_test_suite(root_device_devm_test_suite);
>
> --
> 2.40.0
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH v2 2/3] drivers: base: Add basic devm tests for platform devices

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 8971 bytes --]

On Wed, 28 Jun 2023 at 17:49, Maxime Ripard <mripard@kernel.org> wrote:
>
> Platform devices show some inconsistencies with how devm resources are
> released when the device has been probed and when it hasn't. Let's add a
> few tests to exercise thos paths and odd cases.

Nit: "these".

Also, it'd be nice to call out the case that fails explicitly in the
commit message here, so it's obvious what the "inconsistency" is.

>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

This looks good to me. I think this is, if anything, even more
obviously important than the root device issues, so we definitely need
to fix or document it.

Reviewed-by: David Gow <davidgow@google.com>


>  drivers/base/test/Makefile               |   1 +
>  drivers/base/test/platform-device-test.c | 222 +++++++++++++++++++++++++++++++
>  2 files changed, 223 insertions(+)
>
> diff --git a/drivers/base/test/Makefile b/drivers/base/test/Makefile
> index d589ca3fa8fc..e321dfc7e922 100644
> --- a/drivers/base/test/Makefile
> +++ b/drivers/base/test/Makefile
> @@ -2,6 +2,7 @@
>  obj-$(CONFIG_TEST_ASYNC_DRIVER_PROBE)  += test_async_driver_probe.o
>
>  obj-$(CONFIG_DM_KUNIT_TEST)    += root-device-test.o
> +obj-$(CONFIG_DM_KUNIT_TEST)    += platform-device-test.o
>
>  obj-$(CONFIG_DRIVER_PE_KUNIT_TEST) += property-entry-test.o
>  CFLAGS_property-entry-test.o += $(DISABLE_STRUCTLEAK_PLUGIN)
> diff --git a/drivers/base/test/platform-device-test.c b/drivers/base/test/platform-device-test.c
> new file mode 100644
> index 000000000000..b6ebf1dcdffb
> --- /dev/null
> +++ b/drivers/base/test/platform-device-test.c
> @@ -0,0 +1,222 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <kunit/resource.h>
> +
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +
> +#define DEVICE_NAME "test"
> +
> +struct test_priv {
> +       bool probe_done;
> +       bool release_done;
> +       wait_queue_head_t probe_wq;
> +       wait_queue_head_t release_wq;
> +       struct device *dev;
> +};
> +
> +static int platform_device_devm_init(struct kunit *test)
> +{
> +       struct test_priv *priv;
> +
> +       priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
> +       init_waitqueue_head(&priv->probe_wq);
> +       init_waitqueue_head(&priv->release_wq);
> +
> +       test->priv = priv;
> +
> +       return 0;
> +}
> +
> +static void devm_device_action(void *ptr)
> +{
> +       struct test_priv *priv = ptr;
> +
> +       priv->release_done = true;
> +       wake_up_interruptible(&priv->release_wq);
> +}
> +
> +static void devm_put_device_action(void *ptr)
> +{
> +       struct test_priv *priv = ptr;
> +
> +       put_device(priv->dev);
> +       priv->release_done = true;
> +       wake_up_interruptible(&priv->release_wq);
> +}
> +
> +#define RELEASE_TIMEOUT_MS     100
> +
> +/*
> + * Tests that a platform bus, non-probed device will run its
> + * device-managed actions when unregistered.
> + */
> +static void platform_device_devm_register_unregister_test(struct kunit *test)
> +{
> +       struct platform_device *pdev;
> +       struct test_priv *priv = test->priv;
> +       int ret;
> +
> +       pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
> +
> +       ret = platform_device_add(pdev);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       priv->dev = &pdev->dev;
> +
> +       ret = devm_add_action_or_reset(priv->dev, devm_device_action, priv);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       platform_device_unregister(pdev);
> +
> +       ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
> +                                              msecs_to_jiffies(RELEASE_TIMEOUT_MS));
> +       KUNIT_EXPECT_GT(test, ret, 0);
> +}
> +
> +/*
> + * Tests that a platform bus, non-probed device will run its
> + * device-managed actions when unregistered, even if someone still holds
> + * a reference to it.
> + */
> +static void platform_device_devm_register_get_unregister_with_devm_test(struct kunit *test)
> +{
> +       struct platform_device *pdev;
> +       struct test_priv *priv = test->priv;
> +       int ret;
> +
> +       kunit_skip(test, "This needs to be fixed in the core.");
> +
> +       pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
> +
> +       ret = platform_device_add(pdev);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       priv->dev = &pdev->dev;
> +
> +       get_device(priv->dev);
> +
> +       ret = devm_add_action_or_reset(priv->dev, devm_put_device_action, priv);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       platform_device_unregister(pdev);
> +
> +       ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
> +                                              msecs_to_jiffies(RELEASE_TIMEOUT_MS));
> +       KUNIT_EXPECT_GT(test, ret, 0);
> +}
> +
> +static int fake_probe(struct platform_device *pdev)
> +{
> +       struct test_priv *priv = platform_get_drvdata(pdev);
> +
> +       priv->probe_done = true;
> +       wake_up_interruptible(&priv->probe_wq);
> +
> +       return 0;
> +}
> +
> +static struct platform_driver fake_driver = {
> +       .probe  = fake_probe,
> +       .driver = {
> +               .name = DEVICE_NAME,
> +       },
> +};
> +
> +/*
> + * Tests that a platform bus, probed device will run its device-managed
> + * actions when unregistered.
> + */
> +static void probed_platform_device_devm_register_unregister_test(struct kunit *test)
> +{
> +       struct platform_device *pdev;
> +       struct test_priv *priv = test->priv;
> +       int ret;
> +
> +       ret = platform_driver_register(&fake_driver);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
> +
> +       priv->dev = &pdev->dev;
> +       platform_set_drvdata(pdev, priv);
> +
> +       ret = platform_device_add(pdev);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       ret = wait_event_interruptible_timeout(priv->probe_wq, priv->probe_done,
> +                                              msecs_to_jiffies(RELEASE_TIMEOUT_MS));
> +       KUNIT_ASSERT_GT(test, ret, 0);
> +
> +       ret = devm_add_action_or_reset(priv->dev, devm_device_action, priv);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       platform_device_unregister(pdev);
> +
> +       ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
> +                                              msecs_to_jiffies(RELEASE_TIMEOUT_MS));
> +       KUNIT_EXPECT_GT(test, ret, 0);
> +
> +       platform_driver_unregister(&fake_driver);
> +}
> +
> +/*
> + * Tests that a platform bus, probed device will run its device-managed
> + * actions when unregistered, even if someone still holds a reference to
> + * it.
> + */
> +static void probed_platform_device_devm_register_get_unregister_with_devm_test(struct kunit *test)
> +{
> +       struct platform_device *pdev;
> +       struct test_priv *priv = test->priv;
> +       int ret;
> +
> +       ret = platform_driver_register(&fake_driver);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
> +
> +       priv->dev = &pdev->dev;
> +       platform_set_drvdata(pdev, priv);
> +
> +       ret = platform_device_add(pdev);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       ret = wait_event_interruptible_timeout(priv->probe_wq, priv->probe_done,
> +                                              msecs_to_jiffies(RELEASE_TIMEOUT_MS));
> +       KUNIT_ASSERT_GT(test, ret, 0);
> +
> +       get_device(priv->dev);
> +
> +       ret = devm_add_action_or_reset(priv->dev, devm_put_device_action, priv);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       platform_device_unregister(pdev);
> +
> +       ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
> +                                              msecs_to_jiffies(RELEASE_TIMEOUT_MS));
> +       KUNIT_EXPECT_GT(test, ret, 0);
> +
> +       platform_driver_unregister(&fake_driver);
> +}
> +
> +static struct kunit_case platform_device_devm_tests[] = {
> +       KUNIT_CASE(platform_device_devm_register_unregister_test),
> +       KUNIT_CASE(platform_device_devm_register_get_unregister_with_devm_test),
> +       KUNIT_CASE(probed_platform_device_devm_register_unregister_test),
> +       KUNIT_CASE(probed_platform_device_devm_register_get_unregister_with_devm_test),
> +       {}
> +};
> +
> +static struct kunit_suite platform_device_devm_test_suite = {
> +       .name = "platform-device-devm",
> +       .init = platform_device_devm_init,
> +       .test_cases = platform_device_devm_tests,
> +};
> +
> +kunit_test_suite(platform_device_devm_test_suite);
>
> --
> 2.40.0
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH v2 3/3] drivers: base: Free devm resources when unregistering a device

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 4353 bytes --]

On Wed, 28 Jun 2023 at 17:50, Maxime Ripard <mripard@kernel.org> wrote:
>
> From: David Gow <davidgow@google.com>
>
> In the current code, devres_release_all() only gets called if the device
> has a bus and has been probed.
>
> This leads to issues when using bus-less or driver-less devices where
> the device might never get freed if a managed resource holds a reference
> to the device. This is happening in the DRM framework for example.
>
> We should thus call devres_release_all() in the device_del() function to
> make sure that the device-managed actions are properly executed when the
> device is unregistered, even if it has neither a bus nor a driver.
>
> This is effectively the same change than commit 2f8d16a996da ("devres:
> release resources on device_del()") that got reverted by commit
> a525a3ddeaca ("driver core: free devres in device_release") over
> use-after-free concerns.
>
> It's not clear whether those concerns are legitimate though, but I would
> expect drivers not to register new resources in their device-managed
> actions.

It might be clearer to notice that this patch effectively combines the
two patches above, freeing _both_ on device_del() and
device_release(). This should give us the best of both worlds. I'm not
aware of a use-after-free issue that could result here, though it's
possible there's a double free I'm missing now that we are freeing
things twice. My understanding is that commit a525a3ddeaca ("driver
core: free devres in device_release") was more to avoid a leak than a
use-after-free, but I could be wrong.


> Fixes: a525a3ddeaca ("driver core: free devres in device_release")
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---

Signed-off-by: David Gow <davidgow@google.com>

Personally, I feel that this is the right way to go, but I'm
definitely not an expert, so I'll let someone else review it in case
there's something I'm missing.

Cheers,
-- David



>  drivers/base/core.c                      | 11 +++++++++++
>  drivers/base/test/platform-device-test.c |  2 --
>  drivers/base/test/root-device-test.c     |  2 --
>  3 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 3dff5037943e..6ceaf50f5a67 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3817,6 +3817,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);
> diff --git a/drivers/base/test/platform-device-test.c b/drivers/base/test/platform-device-test.c
> index b6ebf1dcdffb..1ae5ce8bd366 100644
> --- a/drivers/base/test/platform-device-test.c
> +++ b/drivers/base/test/platform-device-test.c
> @@ -87,8 +87,6 @@ static void platform_device_devm_register_get_unregister_with_devm_test(struct k
>         struct test_priv *priv = test->priv;
>         int ret;
>
> -       kunit_skip(test, "This needs to be fixed in the core.");
> -
>         pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
>
> diff --git a/drivers/base/test/root-device-test.c b/drivers/base/test/root-device-test.c
> index 9a3e6cccae13..780d07455f57 100644
> --- a/drivers/base/test/root-device-test.c
> +++ b/drivers/base/test/root-device-test.c
> @@ -78,8 +78,6 @@ static void root_device_devm_register_get_unregister_with_devm_test(struct kunit
>         struct test_priv *priv = test->priv;
>         int ret;
>
> -       kunit_skip(test, "This needs to be fixed in the core.");
> -
>         priv->dev = root_device_register(DEVICE_NAME);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->dev);
>
>
> --
> 2.40.0
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH v2 1/3] drivers: base: Add basic devm tests for root devices

[Maxime Ripard]

Hi David,

On Wed, Jul 19, 2023 at 05:13:45PM +0800, David Gow wrote:
> On Wed, 28 Jun 2023 at 17:49, Maxime Ripard <mripard@kernel.org> wrote:
> >
> > The root devices show some odd behaviours compared to regular "bus" devices
> > that have been probed through the usual mechanism, so let's create kunit
> > tests to exercise those paths and odd cases.
> >
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> 
> Reviewed-by: David Gow <davidgow@google.com>

Thanks!

> There's definitely an argument that root devices are not supposed to
> be like regular devices, and so devm_ managed resources aren't
> supposed to work with them. Either way:
> - It needs to be documented somewhere (and this test makes for good
> documentation, IMO).
> - It should be consistent: if devm_ isn't to be used with root
> devices, it should fail everywhere, and if it is, it should work in
> all the cases below.
> 
> So I'm all in favour of including this test and making root devices work.

I agree 100%. I've reworded the commit log a bit to make it clearer
that's what we should strive for, and that this is what this patch is
doing.

> That being said, I am planning to send out a patchset adding a struct
> kunit_device for use in tests, which will be attached to a "kunit"
> bus. I think the combination of "fix devm_ with root devices" and
> "don't recommend root devices as a 'fake' device for testing" is
> probably the longer-term solution everyone can agree upon?

Yeah, that sounds reasonable to me

Maxime

Re: [PATCH v2 2/3] drivers: base: Add basic devm tests for platform devices

[Maxime Ripard]

Hi

On Wed, Jul 19, 2023 at 05:13:50PM +0800, David Gow wrote:
> On Wed, 28 Jun 2023 at 17:49, Maxime Ripard <mripard@kernel.org> wrote:
> >
> > Platform devices show some inconsistencies with how devm resources are
> > released when the device has been probed and when it hasn't. Let's add a
> > few tests to exercise thos paths and odd cases.
> 
> Nit: "these".
> 
> Also, it'd be nice to call out the case that fails explicitly in the
> commit message here, so it's obvious what the "inconsistency" is.

I've reworded the commit message.

> >
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> 
> This looks good to me. I think this is, if anything, even more
> obviously important than the root device issues, so we definitely need
> to fix or document it.
> 
> Reviewed-by: David Gow <davidgow@google.com>

Thanks!
Maxime

Re: [PATCH v2 3/3] drivers: base: Free devm resources when unregistering a device

[Maxime Ripard]

On Wed, Jul 19, 2023 at 05:13:58PM +0800, David Gow wrote:
> On Wed, 28 Jun 2023 at 17:50, Maxime Ripard <mripard@kernel.org> wrote:
> >
> > From: David Gow <davidgow@google.com>
> >
> > In the current code, devres_release_all() only gets called if the device
> > has a bus and has been probed.
> >
> > This leads to issues when using bus-less or driver-less devices where
> > the device might never get freed if a managed resource holds a reference
> > to the device. This is happening in the DRM framework for example.
> >
> > We should thus call devres_release_all() in the device_del() function to
> > make sure that the device-managed actions are properly executed when the
> > device is unregistered, even if it has neither a bus nor a driver.
> >
> > This is effectively the same change than commit 2f8d16a996da ("devres:
> > release resources on device_del()") that got reverted by commit
> > a525a3ddeaca ("driver core: free devres in device_release") over
> > use-after-free concerns.
> >
> > It's not clear whether those concerns are legitimate though, but I would
> > expect drivers not to register new resources in their device-managed
> > actions.
> 
> It might be clearer to notice that this patch effectively combines the
> two patches above, freeing _both_ on device_del() and
> device_release(). This should give us the best of both worlds.

You're right I'll add that part to the commit log.

> I'm not aware of a use-after-free issue that could result here, though
> it's possible there's a double free I'm missing now that we are
> freeing things twice. My understanding is that commit a525a3ddeaca
> ("driver core: free devres in device_release") was more to avoid a
> leak than a use-after-free, but I could be wrong.

Yeah, I'm not sure where I got the UAF from. I probably
misread/misremembered.

Maxime