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

[PATCH v3 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 <mripard@kernel.org>
---
Changes in v3:
- Reworded the commit logs according to David's feedback
- Rebased on current next
- Link to v2: https://lore.kernel.org/r/20230329-kunit-devm-inconsistencies-test-v2-0-19feb71e864b@kernel.org

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: c58c49dd89324b18a812762a2bfa5a0458e4f252
change-id: 20230329-kunit-devm-inconsistencies-test-5e5a7d01e60d

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

[PATCH v3 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.

It's not clear whether root devices are even allowed to use device
managed resources, but the fact that it works in some cases but not
others like shown in that test suite shouldn't happen either way: we
want to make it consistent and documented.

These tests will (after the following patches) ensure that consistency
and effectively document that it's allowed.

If it ever turns out to be a bad idea, we can always roll back and
modify the tests then.

Reviewed-by: David Gow <davidgow@google.com>
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 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.41.0

[PATCH v3 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.

In particular, we can register device-managed actions no matter if the
device has be bound to a driver or not, but devres_release_all() will
only be called if it was bound to a driver or if there's no reference
held to it anymore.

If it wasn't bound to a driver and we still have a reference,
devres_release_all() will never get called. This is surprising
considering that if the driver isn't bound but doesn't have any
reference to it anymore, that function will get called, and if it was
bound to a driver but still has references, that function will get
called as well.

Even if that case is fairly unusual, it can easily lead to memory leaks.

The plan is, with the next patch, to make it consistent and enforce that
devres_release_all() is called no matter what situation we're in. For
now, it just tests for the current behaviour and skips over the
inconsistencies.

Reviewed-by: David Gow <davidgow@google.com>
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 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.41.0

[PATCH v3 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
memory leaks concerns.

This patch effectively combines the two commits mentioned above to
release the resources both on device_del() and device_release() and get
the best of both worlds.

Fixes: a525a3ddeaca ("driver core: free devres in device_release")
Signed-off-by: David Gow <davidgow@google.com>
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.41.0

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

[Maxime Ripard]

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

On Thu, Jul 20, 2023 at 02:45:06PM +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.
> 
> Let me know what you think,
> Maxime
> 
> Signed-off-by: Maxime Ripard <mripard@kernel.org>

Ping?

Maxime

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

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

[Greg Kroah-Hartman]

On Mon, Jul 31, 2023 at 08:34:03AM +0200, Maxime Ripard wrote:
> On Thu, Jul 20, 2023 at 02:45:06PM +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.
> > 
> > Let me know what you think,
> > Maxime
> > 
> > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> 
> Ping?

It's in my review queue, still trying to catch up...

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

[Greg Kroah-Hartman]

On Mon, Jul 31, 2023 at 09:28:47AM +0200, Greg Kroah-Hartman wrote:
> On Mon, Jul 31, 2023 at 08:34:03AM +0200, Maxime Ripard wrote:
> > On Thu, Jul 20, 2023 at 02:45:06PM +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.
> > > 
> > > Let me know what you think,
> > > Maxime
> > > 
> > > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > 
> > Ping?
> 
> It's in my review queue, still trying to catch up...

I didn't make it here this week, sorry.  I kind of worry about encoding
the current "odd" functionality in a test as being the correct thing,
but will look at it closer next week.

thanks,

greg k-h

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

[Maxime Ripard]

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

Hi,

On Fri, Aug 04, 2023 at 05:01:50PM +0200, Greg Kroah-Hartman wrote:
> On Mon, Jul 31, 2023 at 09:28:47AM +0200, Greg Kroah-Hartman wrote:
> > On Mon, Jul 31, 2023 at 08:34:03AM +0200, Maxime Ripard wrote:
> > > On Thu, Jul 20, 2023 at 02:45:06PM +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.
> > > > 
> > > > Let me know what you think,
> > > > Maxime
> > > > 
> > > > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > > 
> > > Ping?
> > 
> > It's in my review queue, still trying to catch up...
> 
> I didn't make it here this week, sorry.

np, I just don't want that patch to disappear into the ether :)

> I kind of worry about encoding the current "odd" functionality in a
> test as being the correct thing, but will look at it closer next week.

I don't think I'm doing that? The tests we've added are all how we think
it should behave, the broken ones being skipped to avoid any failures.

The last patch drops the kunit_skip() to make sure that it's tested
going forward.

So we shouldn't encode the odd behaviour anywhere in this series, unless
I got you wrong?

Maxime

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

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

[Greg Kroah-Hartman]

On Mon, Aug 07, 2023 at 08:54:08AM +0200, Maxime Ripard wrote:
> Hi,
> 
> On Fri, Aug 04, 2023 at 05:01:50PM +0200, Greg Kroah-Hartman wrote:
> > On Mon, Jul 31, 2023 at 09:28:47AM +0200, Greg Kroah-Hartman wrote:
> > > On Mon, Jul 31, 2023 at 08:34:03AM +0200, Maxime Ripard wrote:
> > > > On Thu, Jul 20, 2023 at 02:45:06PM +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.
> > > > > 
> > > > > Let me know what you think,
> > > > > Maxime
> > > > > 
> > > > > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > > > 
> > > > Ping?
> > > 
> > > It's in my review queue, still trying to catch up...
> > 
> > I didn't make it here this week, sorry.
> 
> np, I just don't want that patch to disappear into the ether :)
> 
> > I kind of worry about encoding the current "odd" functionality in a
> > test as being the correct thing, but will look at it closer next week.
> 
> I don't think I'm doing that? The tests we've added are all how we think
> it should behave, the broken ones being skipped to avoid any failures.
> 
> The last patch drops the kunit_skip() to make sure that it's tested
> going forward.
> 
> So we shouldn't encode the odd behaviour anywhere in this series, unless
> I got you wrong?

No you are correct, I was mis-remembering things.

This looks good, thanks for sticking with it, all now applied to my
tree.

greg k-h