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

4 views
Skip to first unread message

Maxime Ripard

unread,
Mar 29, 2023, 3:40:29 PM3/29/23
to Greg Kroah-Hartman, Rafael J. Wysocki, Brendan Higgins, David Gow, linux-k...@vger.kernel.org, kuni...@googlegroups.com, linux-...@vger.kernel.org, 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/

Let me know what you think,
Maxime

Signed-off-by: Maxime Ripard <max...@cerno.tech>
---
Maxime Ripard (2):
drivers: base: Add basic devm tests for root devices
drivers: base: Add basic devm tests for platform devices

drivers/base/test/.kunitconfig | 2 +
drivers/base/test/Kconfig | 4 +
drivers/base/test/Makefile | 3 +
drivers/base/test/platform-device-test.c | 278 +++++++++++++++++++++++++++++++
drivers/base/test/root-device-test.c | 120 +++++++++++++
5 files changed, 407 insertions(+)
---
base-commit: a6faf7ea9fcb7267d06116d4188947f26e00e57e
change-id: 20230329-kunit-devm-inconsistencies-test-5e5a7d01e60d

Best regards,
--
Maxime Ripard <max...@cerno.tech>

Maxime Ripard

unread,
Mar 29, 2023, 3:40:30 PM3/29/23
to Greg Kroah-Hartman, Rafael J. Wysocki, Brendan Higgins, David Gow, linux-k...@vger.kernel.org, kuni...@googlegroups.com, linux-...@vger.kernel.org, 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 <max...@cerno.tech>
---
drivers/base/test/.kunitconfig | 2 +
drivers/base/test/Kconfig | 4 ++
drivers/base/test/Makefile | 2 +
drivers/base/test/root-device-test.c | 120 +++++++++++++++++++++++++++++++++++
4 files changed, 128 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..fcb55d8882aa
--- /dev/null
+++ b/drivers/base/test/root-device-test.c
@@ -0,0 +1,120 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright 2023 Maxime Ripard <mri...@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 void devm_device_action(void *ptr)
+{
+ struct test_priv *priv = ptr;
+
+ priv->release_done = true;
+ wake_up_interruptible(&priv->release_wq);
+}
+
+static void devm_put_device_action(void *ptr)
+{
+ struct test_priv *priv = ptr;
+
+ put_device(priv->dev);
+ priv->release_done = true;
+ wake_up_interruptible(&priv->release_wq);
+}
+
+#define RELEASE_TIMEOUT_MS 500
+
+static void root_device_devm_register_unregister_test(struct kunit *test)
+{
+ struct test_priv *priv;
+ int ret;
+
+ priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+ init_waitqueue_head(&priv->release_wq);
+
+ priv->dev = root_device_register(DEVICE_NAME);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->dev);
+
+ ret = devm_add_action_or_reset(priv->dev, devm_device_action, priv);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ root_device_unregister(priv->dev);
+
+ ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+ msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+ KUNIT_EXPECT_GT(test, ret, 0);
+}
+
+static void root_device_devm_register_get_put_unregister_test(struct kunit *test)
+{
+ struct test_priv *priv;
+ int ret;
+
+ priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+ init_waitqueue_head(&priv->release_wq);
+
+ priv->dev = root_device_register(DEVICE_NAME);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->dev);
+
+ get_device(priv->dev);
+
+ ret = devm_add_action_or_reset(priv->dev, devm_device_action, priv);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ put_device(priv->dev);
+
+ root_device_unregister(priv->dev);
+
+ ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+ msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+ KUNIT_EXPECT_GT(test, ret, 0);
+}
+
+static void root_device_devm_register_get_unregister_with_devm_test(struct kunit *test)
+{
+ struct test_priv *priv;
+ int ret;
+
+ priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+ init_waitqueue_head(&priv->release_wq);
+
+ priv->dev = root_device_register(DEVICE_NAME);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->dev);
+
+ get_device(priv->dev);
+
+ ret = devm_add_action_or_reset(priv->dev, devm_put_device_action, priv);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ root_device_unregister(priv->dev);
+
+ ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+ msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+ KUNIT_EXPECT_GT(test, ret, 0);
+}
+
+static struct kunit_case root_device_devm_tests[] = {
+ KUNIT_CASE(root_device_devm_register_unregister_test),
+ KUNIT_CASE(root_device_devm_register_get_put_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",
+ .test_cases = root_device_devm_tests,
+};
+
+kunit_test_suite(root_device_devm_test_suite);

--
2.40.0

Maxime Ripard

unread,
Mar 29, 2023, 3:40:33 PM3/29/23
to Greg Kroah-Hartman, Rafael J. Wysocki, Brendan Higgins, David Gow, linux-k...@vger.kernel.org, kuni...@googlegroups.com, linux-...@vger.kernel.org, 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 <max...@cerno.tech>
---
drivers/base/test/Makefile | 1 +
drivers/base/test/platform-device-test.c | 278 +++++++++++++++++++++++++++++++
2 files changed, 279 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..2779c6dfac06
--- /dev/null
+++ b/drivers/base/test/platform-device-test.c
@@ -0,0 +1,278 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <kunit/resource.h>
+
+#include <linux/device.h>
+#include <linux/platform_device.h>
+static void platform_device_devm_register_unregister_test(struct kunit *test)
+{
+ struct platform_device *pdev;
+ struct test_priv *priv;
+ int ret;
+
+ priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+ init_waitqueue_head(&priv->release_wq);
+
+ pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
+
+ ret = platform_device_add(pdev);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ priv->dev = &pdev->dev;
+
+ ret = devm_add_action_or_reset(priv->dev, devm_device_action, priv);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ platform_device_unregister(pdev);
+
+ ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+ msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+ KUNIT_EXPECT_GT(test, ret, 0);
+}
+
+static void platform_device_devm_register_get_put_unregister_test(struct kunit *test)
+{
+ struct platform_device *pdev;
+ struct test_priv *priv;
+ int ret;
+
+ priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+ init_waitqueue_head(&priv->release_wq);
+
+ pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
+
+ ret = platform_device_add(pdev);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ priv->dev = &pdev->dev;
+
+ get_device(priv->dev);
+
+ ret = devm_add_action_or_reset(priv->dev, devm_device_action, priv);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ put_device(priv->dev);
+
+ platform_device_unregister(pdev);
+
+ ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+ msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+ KUNIT_EXPECT_GT(test, ret, 0);
+}
+
+static void platform_device_devm_register_get_unregister_with_devm_test(struct kunit *test)
+{
+ struct platform_device *pdev;
+ struct test_priv *priv;
+ int ret;
+
+ priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+ init_waitqueue_head(&priv->release_wq);
+
+ pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
+
+ ret = platform_device_add(pdev);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ priv->dev = &pdev->dev;
+
+ get_device(priv->dev);
+
+ ret = devm_add_action_or_reset(priv->dev, devm_put_device_action, priv);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ platform_device_unregister(pdev);
+
+ ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+ msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+ KUNIT_EXPECT_GT(test, ret, 0);
+}
+
+static int fake_probe(struct platform_device *pdev)
+{
+ struct test_priv *priv = platform_get_drvdata(pdev);
+
+ priv->probe_done = true;
+ wake_up_interruptible(&priv->release_wq);
+
+ return 0;
+}
+
+static struct platform_driver fake_driver = {
+ .probe = fake_probe,
+ .driver = {
+ .name = DEVICE_NAME,
+ },
+};
+
+static void probed_platform_device_devm_register_unregister_test(struct kunit *test)
+{
+ struct platform_device *pdev;
+ struct test_priv *priv;
+ int ret;
+
+ priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+ init_waitqueue_head(&priv->release_wq);
+
+ ret = platform_driver_register(&fake_driver);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
+
+ priv->dev = &pdev->dev;
+ platform_set_drvdata(pdev, priv);
+
+ ret = platform_device_add(pdev);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ ret = wait_event_interruptible_timeout(priv->release_wq, priv->probe_done,
+ msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+ KUNIT_ASSERT_GT(test, ret, 0);
+
+ ret = devm_add_action_or_reset(priv->dev, devm_device_action, priv);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ platform_device_unregister(pdev);
+
+ ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+ msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+ KUNIT_EXPECT_GT(test, ret, 0);
+
+ platform_driver_unregister(&fake_driver);
+}
+
+static void probed_platform_device_devm_register_get_put_unregister_test(struct kunit *test)
+{
+ struct platform_device *pdev;
+ struct test_priv *priv;
+ int ret;
+
+ priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+ init_waitqueue_head(&priv->release_wq);
+
+ ret = platform_driver_register(&fake_driver);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
+
+ priv->dev = &pdev->dev;
+ platform_set_drvdata(pdev, priv);
+
+ ret = platform_device_add(pdev);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ ret = wait_event_interruptible_timeout(priv->release_wq, priv->probe_done,
+ msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+ KUNIT_ASSERT_GT(test, ret, 0);
+
+ get_device(priv->dev);
+
+ ret = devm_add_action_or_reset(priv->dev, devm_device_action, priv);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ put_device(priv->dev);
+
+ platform_device_unregister(pdev);
+
+ ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+ msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+ KUNIT_EXPECT_GT(test, ret, 0);
+
+ platform_driver_unregister(&fake_driver);
+}
+
+static void probed_platform_device_devm_register_get_unregister_with_devm_test(struct kunit *test)
+{
+ struct platform_device *pdev;
+ struct test_priv *priv;
+ int ret;
+
+ priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+ init_waitqueue_head(&priv->release_wq);
+
+ ret = platform_driver_register(&fake_driver);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
+
+ priv->dev = &pdev->dev;
+ platform_set_drvdata(pdev, priv);
+
+ ret = platform_device_add(pdev);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ ret = wait_event_interruptible_timeout(priv->release_wq, priv->probe_done,
+ msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+ KUNIT_ASSERT_GT(test, ret, 0);
+
+ get_device(priv->dev);
+
+ ret = devm_add_action_or_reset(priv->dev, devm_put_device_action, priv);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ platform_device_unregister(pdev);
+
+ ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+ msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+ KUNIT_EXPECT_GT(test, ret, 0);
+
+ platform_driver_unregister(&fake_driver);
+}
+
+static struct kunit_case platform_device_devm_tests[] = {
+ KUNIT_CASE(platform_device_devm_register_unregister_test),
+ KUNIT_CASE(platform_device_devm_register_get_put_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_put_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",
+ .test_cases = platform_device_devm_tests,
+};
+
+kunit_test_suite(platform_device_devm_test_suite);

--
2.40.0

kernel test robot

unread,
Mar 29, 2023, 5:44:20 PM3/29/23
to Maxime Ripard, Greg Kroah-Hartman, Rafael J. Wysocki, oe-kbu...@lists.linux.dev, Brendan Higgins, David Gow, linux-k...@vger.kernel.org, kuni...@googlegroups.com, linux-...@vger.kernel.org, Maxime Ripard
Hi Maxime,

I love your patch! Perhaps something to improve:

[auto build test WARNING on a6faf7ea9fcb7267d06116d4188947f26e00e57e]

url: https://github.com/intel-lab-lkp/linux/commits/Maxime-Ripard/drivers-base-Add-basic-devm-tests-for-root-devices/20230330-034149
base: a6faf7ea9fcb7267d06116d4188947f26e00e57e
patch link: https://lore.kernel.org/r/20230329-kunit-devm-inconsistencies-test-v1-1-c33127048375%40cerno.tech
patch subject: [PATCH 1/2] drivers: base: Add basic devm tests for root devices
config: arc-randconfig-r043-20230329 (https://download.01.org/0day-ci/archive/20230330/202303300531...@intel.com/config)
compiler: arc-elf-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/a6c0627f4059cf0565eec2bb99fc7453e20b5c51
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Maxime-Ripard/drivers-base-Add-basic-devm-tests-for-root-devices/20230330-034149
git checkout a6c0627f4059cf0565eec2bb99fc7453e20b5c51
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <l...@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303300531...@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/base/test/.kunitconfig: warning: ignored by one of the .gitignore files
drivers/clk/.kunitconfig: warning: ignored by one of the .gitignore files
drivers/gpu/drm/tests/.kunitconfig: warning: ignored by one of the .gitignore files
drivers/gpu/drm/vc4/tests/.kunitconfig: warning: ignored by one of the .gitignore files
drivers/hid/.kunitconfig: warning: ignored by one of the .gitignore files
fs/ext4/.kunitconfig: warning: ignored by one of the .gitignore files
fs/fat/.kunitconfig: warning: ignored by one of the .gitignore files
kernel/kcsan/.kunitconfig: warning: ignored by one of the .gitignore files
lib/kunit/.kunitconfig: warning: ignored by one of the .gitignore files
mm/kfence/.kunitconfig: warning: ignored by one of the .gitignore files
net/sunrpc/.kunitconfig: warning: ignored by one of the .gitignore files
tools/testing/selftests/arm64/tags/.gitignore: warning: ignored by one of the .gitignore files
tools/testing/selftests/arm64/tags/Makefile: warning: ignored by one of the .gitignore files
tools/testing/selftests/arm64/tags/run_tags_test.sh: warning: ignored by one of the .gitignore files
tools/testing/selftests/arm64/tags/tags_test.c: warning: ignored by one of the .gitignore files
tools/testing/selftests/kvm/.gitignore: warning: ignored by one of the .gitignore files
tools/testing/selftests/kvm/Makefile: warning: ignored by one of the .gitignore files
tools/testing/selftests/kvm/config: warning: ignored by one of the .gitignore files
tools/testing/selftests/kvm/settings: warning: ignored by one of the .gitignore files

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Maxime Ripard

unread,
Apr 17, 2023, 12:12:30 PM4/17/23
to Greg Kroah-Hartman, Rafael J. Wysocki, Brendan Higgins, David Gow, linux-k...@vger.kernel.org, kuni...@googlegroups.com, linux-...@vger.kernel.org
Hi,

On Wed, Mar 29, 2023 at 08:38:30PM +0100, 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/
>
> Let me know what you think,
> Maxime
>
> Signed-off-by: Maxime Ripard <max...@cerno.tech>

Is there any news on this?

Maxime
signature.asc

Maxime Ripard

unread,
May 11, 2023, 4:44:14 AM5/11/23
to Greg Kroah-Hartman, Rafael J. Wysocki, Brendan Higgins, David Gow, linux-k...@vger.kernel.org, kuni...@googlegroups.com, linux-...@vger.kernel.org
On Wed, Mar 29, 2023 at 08:38:30PM +0100, 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/
>
> Let me know what you think,
> Maxime
>
> Signed-off-by: Maxime Ripard <max...@cerno.tech>

Ping?

How can we move this forward?

Maxime
signature.asc

Greg Kroah-Hartman

unread,
May 31, 2023, 3:20:20 PM5/31/23
to Maxime Ripard, Rafael J. Wysocki, Brendan Higgins, David Gow, linux-k...@vger.kernel.org, kuni...@googlegroups.com, linux-...@vger.kernel.org
Can you resend, last I saw the kernel test robot had a problem, which
causes any patch series like that to be ignored by me.

thanks,

greg k-h

Maxime Ripard

unread,
Jun 2, 2023, 11:20:50 AM6/2/23
to Greg Kroah-Hartman, Rafael J. Wysocki, Brendan Higgins, David Gow, linux-k...@vger.kernel.org, kuni...@googlegroups.com, linux-...@vger.kernel.org, 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/

Let me know what you think,
Maxime

Signed-off-by: Maxime Ripard <max...@cerno.tech>
---
Maxime Ripard (2):
drivers: base: Add basic devm tests for root devices
drivers: base: Add basic devm tests for platform devices

drivers/base/test/.kunitconfig | 2 +
drivers/base/test/Kconfig | 4 +
drivers/base/test/Makefile | 3 +
drivers/base/test/platform-device-test.c | 278 +++++++++++++++++++++++++++++++
drivers/base/test/root-device-test.c | 120 +++++++++++++
5 files changed, 407 insertions(+)
---
base-commit: a6faf7ea9fcb7267d06116d4188947f26e00e57e
change-id: 20230329-kunit-devm-inconsistencies-test-5e5a7d01e60d

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

Maxime Ripard

unread,
Jun 2, 2023, 11:20:53 AM6/2/23
to Greg Kroah-Hartman, Rafael J. Wysocki, Brendan Higgins, David Gow, linux-k...@vger.kernel.org, kuni...@googlegroups.com, linux-...@vger.kernel.org, Maxime Ripard
From: Maxime Ripard <max...@cerno.tech>

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 <max...@cerno.tech>
---
drivers/base/test/.kunitconfig | 2 +
drivers/base/test/Kconfig | 4 ++
drivers/base/test/Makefile | 2 +
drivers/base/test/root-device-test.c | 120 +++++++++++++++++++++++++++++++++++
4 files changed, 128 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..fcb55d8882aa
--- /dev/null
+++ b/drivers/base/test/root-device-test.c
@@ -0,0 +1,120 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright 2023 Maxime Ripard <mri...@kernel.org>
+
+#include <kunit/resource.h>
+
+#include <linux/device.h>
+static void root_device_devm_register_unregister_test(struct kunit *test)
+{
+ struct test_priv *priv;
+ int ret;
+
+ priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+ init_waitqueue_head(&priv->release_wq);
+
+ priv->dev = root_device_register(DEVICE_NAME);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->dev);
+
+ ret = devm_add_action_or_reset(priv->dev, devm_device_action, priv);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ root_device_unregister(priv->dev);
+
+ ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+ msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+ KUNIT_EXPECT_GT(test, ret, 0);
+}
+
+static void root_device_devm_register_get_put_unregister_test(struct kunit *test)
+{
+ struct test_priv *priv;
+ int ret;
+
+ priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+ init_waitqueue_head(&priv->release_wq);
+
+ priv->dev = root_device_register(DEVICE_NAME);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->dev);
+
+ get_device(priv->dev);
+
+ ret = devm_add_action_or_reset(priv->dev, devm_device_action, priv);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ put_device(priv->dev);
+
+ root_device_unregister(priv->dev);
+
+ ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+ msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+ KUNIT_EXPECT_GT(test, ret, 0);
+}
+
+static void root_device_devm_register_get_unregister_with_devm_test(struct kunit *test)
+{
+ struct test_priv *priv;
+ int ret;
+
+ priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+ init_waitqueue_head(&priv->release_wq);
+
+ priv->dev = root_device_register(DEVICE_NAME);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->dev);
+
+ get_device(priv->dev);
+
+ ret = devm_add_action_or_reset(priv->dev, devm_put_device_action, priv);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ root_device_unregister(priv->dev);
+
+ ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+ msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+ KUNIT_EXPECT_GT(test, ret, 0);
+}
+
+static struct kunit_case root_device_devm_tests[] = {
+ KUNIT_CASE(root_device_devm_register_unregister_test),
+ KUNIT_CASE(root_device_devm_register_get_put_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",
+ .test_cases = root_device_devm_tests,
+};
+
+kunit_test_suite(root_device_devm_test_suite);

--
2.40.1

Maxime Ripard

unread,
Jun 2, 2023, 11:20:57 AM6/2/23
to Greg Kroah-Hartman, Rafael J. Wysocki, Brendan Higgins, David Gow, linux-k...@vger.kernel.org, kuni...@googlegroups.com, linux-...@vger.kernel.org, Maxime Ripard
From: Maxime Ripard <max...@cerno.tech>

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 <max...@cerno.tech>
---
drivers/base/test/Makefile | 1 +
drivers/base/test/platform-device-test.c | 278 +++++++++++++++++++++++++++++++
2 files changed, 279 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..2779c6dfac06
--- /dev/null
+++ b/drivers/base/test/platform-device-test.c
@@ -0,0 +1,278 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <kunit/resource.h>
+
+#include <linux/device.h>
+#include <linux/platform_device.h>
+static void platform_device_devm_register_unregister_test(struct kunit *test)
+{
+ struct platform_device *pdev;
+ struct test_priv *priv;
+ int ret;
+
+ priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+ init_waitqueue_head(&priv->release_wq);
+
+ pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
+
+ ret = platform_device_add(pdev);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ priv->dev = &pdev->dev;
+
+ ret = devm_add_action_or_reset(priv->dev, devm_device_action, priv);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ platform_device_unregister(pdev);
+
+ ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+ msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+ KUNIT_EXPECT_GT(test, ret, 0);
+}
+
+static void platform_device_devm_register_get_put_unregister_test(struct kunit *test)
+{
+ struct platform_device *pdev;
+ struct test_priv *priv;
+ int ret;
+
+ priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+ init_waitqueue_head(&priv->release_wq);
+
+ pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
+
+ ret = platform_device_add(pdev);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ priv->dev = &pdev->dev;
+
+ get_device(priv->dev);
+
+ ret = devm_add_action_or_reset(priv->dev, devm_device_action, priv);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ put_device(priv->dev);
+
+ platform_device_unregister(pdev);
+
+ ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+ msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+ KUNIT_EXPECT_GT(test, ret, 0);
+}
+
+static void platform_device_devm_register_get_unregister_with_devm_test(struct kunit *test)
+{
+ struct platform_device *pdev;
+ struct test_priv *priv;
+ int ret;
+
+ priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+ init_waitqueue_head(&priv->release_wq);
+
+ pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
+
+ ret = platform_device_add(pdev);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ priv->dev = &pdev->dev;
+
+ get_device(priv->dev);
+
+ ret = devm_add_action_or_reset(priv->dev, devm_put_device_action, priv);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ platform_device_unregister(pdev);
+
+ ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+ msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+ KUNIT_EXPECT_GT(test, ret, 0);
+}
+
+static int fake_probe(struct platform_device *pdev)
+{
+ struct test_priv *priv = platform_get_drvdata(pdev);
+
+ priv->probe_done = true;
+ wake_up_interruptible(&priv->release_wq);
+
+ return 0;
+}
+
+static struct platform_driver fake_driver = {
+ .probe = fake_probe,
+ .driver = {
+ .name = DEVICE_NAME,
+ },
+};
+
+static void probed_platform_device_devm_register_unregister_test(struct kunit *test)
+{
+ struct platform_device *pdev;
+ struct test_priv *priv;
+ int ret;
+
+ priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+ init_waitqueue_head(&priv->release_wq);
+
+ ret = platform_driver_register(&fake_driver);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
+
+ priv->dev = &pdev->dev;
+ platform_set_drvdata(pdev, priv);
+
+ ret = platform_device_add(pdev);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ ret = wait_event_interruptible_timeout(priv->release_wq, priv->probe_done,
+ msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+ KUNIT_ASSERT_GT(test, ret, 0);
+
+ ret = devm_add_action_or_reset(priv->dev, devm_device_action, priv);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ platform_device_unregister(pdev);
+
+ ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+ msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+ KUNIT_EXPECT_GT(test, ret, 0);
+
+ platform_driver_unregister(&fake_driver);
+}
+
+static void probed_platform_device_devm_register_get_put_unregister_test(struct kunit *test)
+{
+ struct platform_device *pdev;
+ struct test_priv *priv;
+ int ret;
+
+ priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+ init_waitqueue_head(&priv->release_wq);
+
+ ret = platform_driver_register(&fake_driver);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
+
+ priv->dev = &pdev->dev;
+ platform_set_drvdata(pdev, priv);
+
+ ret = platform_device_add(pdev);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ ret = wait_event_interruptible_timeout(priv->release_wq, priv->probe_done,
+ msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+ KUNIT_ASSERT_GT(test, ret, 0);
+
+ get_device(priv->dev);
+
+ ret = devm_add_action_or_reset(priv->dev, devm_device_action, priv);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ put_device(priv->dev);
+
+ platform_device_unregister(pdev);
+
+ ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+ msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+ KUNIT_EXPECT_GT(test, ret, 0);
+
+ platform_driver_unregister(&fake_driver);
+}
+
+static void probed_platform_device_devm_register_get_unregister_with_devm_test(struct kunit *test)
+{
+ struct platform_device *pdev;
+ struct test_priv *priv;
+ int ret;
+
+ priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+ init_waitqueue_head(&priv->release_wq);
+
+ ret = platform_driver_register(&fake_driver);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
+
+ priv->dev = &pdev->dev;
+ platform_set_drvdata(pdev, priv);
+
+ ret = platform_device_add(pdev);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ ret = wait_event_interruptible_timeout(priv->release_wq, priv->probe_done,
+ msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+ KUNIT_ASSERT_GT(test, ret, 0);
+
+ get_device(priv->dev);
+
+ ret = devm_add_action_or_reset(priv->dev, devm_put_device_action, priv);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ platform_device_unregister(pdev);
+
+ ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+ msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+ KUNIT_EXPECT_GT(test, ret, 0);
+
+ platform_driver_unregister(&fake_driver);
+}
+
+static struct kunit_case platform_device_devm_tests[] = {
+ KUNIT_CASE(platform_device_devm_register_unregister_test),
+ KUNIT_CASE(platform_device_devm_register_get_put_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_put_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",
+ .test_cases = platform_device_devm_tests,
+};
+
+kunit_test_suite(platform_device_devm_test_suite);

--
2.40.1

Maxime Ripard

unread,
Jun 2, 2023, 11:27:09 AM6/2/23
to Greg Kroah-Hartman, Rafael J. Wysocki, Brendan Higgins, David Gow, linux-k...@vger.kernel.org, kuni...@googlegroups.com, linux-...@vger.kernel.org
Yeah, this was due to .kunitconfig being ignored by .gitignore, which
triggers a warning for the bot.

It's now fixed (at least in next), and I just resent the patches

Thanks!
Maxime
signature.asc

kernel test robot

unread,
Jun 2, 2023, 2:04:23 PM6/2/23
to Maxime Ripard, Greg Kroah-Hartman, Rafael J. Wysocki, oe-kbu...@lists.linux.dev, Brendan Higgins, David Gow, linux-k...@vger.kernel.org, kuni...@googlegroups.com, linux-...@vger.kernel.org, Maxime Ripard
Hi Maxime,

kernel test robot noticed the following build warnings:

[auto build test WARNING on a6faf7ea9fcb7267d06116d4188947f26e00e57e]

url: https://github.com/intel-lab-lkp/linux/commits/Maxime-Ripard/drivers-base-Add-basic-devm-tests-for-root-devices/20230602-232247
base: a6faf7ea9fcb7267d06116d4188947f26e00e57e
patch link: https://lore.kernel.org/r/20230329-kunit-devm-inconsistencies-test-v1-1-015b1574d673%40kernel.org
patch subject: [PATCH RESEND 1/2] drivers: base: Add basic devm tests for root devices
config: m68k-randconfig-r013-20230601 (https://download.01.org/0day-ci/archive/20230603/202306030141...@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.3.0
reproduce (this is a W=1 build):
mkdir -p ~/bin
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/f685d9ffe8ed7605cf0edbfb05a7e65611216b21
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Maxime-Ripard/drivers-base-Add-basic-devm-tests-for-root-devices/20230602-232247
git checkout f685d9ffe8ed7605cf0edbfb05a7e65611216b21
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=m68k olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <l...@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306030141...@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/base/test/.kunitconfig: warning: ignored by one of the .gitignore files
drivers/clk/.kunitconfig: warning: ignored by one of the .gitignore files
drivers/gpu/drm/tests/.kunitconfig: warning: ignored by one of the .gitignore files
drivers/gpu/drm/vc4/tests/.kunitconfig: warning: ignored by one of the .gitignore files
drivers/hid/.kunitconfig: warning: ignored by one of the .gitignore files
fs/ext4/.kunitconfig: warning: ignored by one of the .gitignore files
fs/fat/.kunitconfig: warning: ignored by one of the .gitignore files
kernel/kcsan/.kunitconfig: warning: ignored by one of the .gitignore files
lib/kunit/.kunitconfig: warning: ignored by one of the .gitignore files
mm/kfence/.kunitconfig: warning: ignored by one of the .gitignore files
net/sunrpc/.kunitconfig: warning: ignored by one of the .gitignore files
tools/testing/selftests/arm64/tags/.gitignore: warning: ignored by one of the .gitignore files
tools/testing/selftests/arm64/tags/Makefile: warning: ignored by one of the .gitignore files
tools/testing/selftests/arm64/tags/run_tags_test.sh: warning: ignored by one of the .gitignore files
tools/testing/selftests/arm64/tags/tags_test.c: warning: ignored by one of the .gitignore files
tools/testing/selftests/kvm/.gitignore: warning: ignored by one of the .gitignore files
tools/testing/selftests/kvm/Makefile: warning: ignored by one of the .gitignore files
tools/testing/selftests/kvm/config: warning: ignored by one of the .gitignore files
tools/testing/selftests/kvm/settings: warning: ignored by one of the .gitignore files

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Daniel Latypov

unread,
Jun 2, 2023, 5:09:44 PM6/2/23
to Maxime Ripard, Greg Kroah-Hartman, Rafael J. Wysocki, Brendan Higgins, David Gow, linux-k...@vger.kernel.org, kuni...@googlegroups.com, linux-...@vger.kernel.org, Maxime Ripard
On Fri, Jun 2, 2023 at 8:20 AM Maxime Ripard <mri...@kernel.org> wrote:

One small suggestion below
<snip>

> +static void root_device_devm_register_unregister_test(struct kunit *test)
> +{
> + struct test_priv *priv;
> + int ret;
> +
> + priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
> + init_waitqueue_head(&priv->release_wq);


Note: should we use an init function to handle this setup?
We can store it in test->priv instead.

static int my_init(struct kunit *test)
{
struct test_priv *priv;

priv = kunit_kzalloc(test, sizeof(test_priv), GFP_KERNEL);
if (!priv) return -ENOMEM;
// N.B. I think you could probably still use assert instead

init_waitqueue_head(&priv->release_wq);

priv->dev = root_device_register(DEVICE_NAME);
if (!priv->dev) return -ENOMEM;

test->priv = priv;
}

...
static struct kunit_suite root_device_devm_test_suite = {
.name = "root-device-devm",
.init = my_init,
.test_cases = root_device_devm_tests,
};

Daniel

Greg Kroah-Hartman

unread,
Jun 3, 2023, 10:39:01 AM6/3/23
to Maxime Ripard, Rafael J. Wysocki, Brendan Higgins, David Gow, linux-k...@vger.kernel.org, kuni...@googlegroups.com, linux-...@vger.kernel.org, Maxime Ripard
On Fri, Jun 02, 2023 at 05:20:44PM +0200, Maxime Ripard wrote:
> From: Maxime Ripard <max...@cerno.tech>
>
> 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.

"those" :)

And what are those inconsistencies? Where are they documented? Here or
somewhere else?

thanks,

greg k-h

Greg Kroah-Hartman

unread,
Jun 3, 2023, 10:43:56 AM6/3/23
to Maxime Ripard, Rafael J. Wysocki, Brendan Higgins, David Gow, linux-k...@vger.kernel.org, kuni...@googlegroups.com, linux-...@vger.kernel.org, Maxime Ripard
Why are you incrementing the reference here?

> +
> + ret = devm_add_action_or_reset(priv->dev, devm_device_action, priv);
> + KUNIT_ASSERT_EQ(test, ret, 0);
> +
> + put_device(priv->dev);

And then dropping it here?

What did that accomplish? You shouldn't have needed to do that at all,
right?

THat's all the difference from the previous function? What is this
testing?
I can't figure out what you are trying to test here at all, which
doesn't bode well for this patchset.

Can you document it better? What should be happening (or not happening)
that you are trying to ensure works properly?

All I see is a register/devm_something/unregister sequence and then wait
for the device to be freed. Am I missing something else?

thanks,

greg k-h

Greg Kroah-Hartman

unread,
Jun 3, 2023, 10:44:25 AM6/3/23
to Maxime Ripard, Rafael J. Wysocki, Brendan Higgins, David Gow, linux-k...@vger.kernel.org, kuni...@googlegroups.com, linux-...@vger.kernel.org
Still triggered the bot :(

Maxime Ripard

unread,
Jun 4, 2023, 4:31:47 AM6/4/23
to Greg Kroah-Hartman, Rafael J. Wysocki, Brendan Higgins, David Gow, linux-k...@vger.kernel.org, kuni...@googlegroups.com, linux-...@vger.kernel.org
Hi,
So I guess most of the context was dropped since I first posted that
series (and I believe that the following will also answer the comment on
the other patch).

It spawned from the discussion here:
https://lore.kernel.org/linux-kselftest/20230324123157.bbwvfq4gsxnlnfwb@houat/

Basically, depending on the bus (platform vs root devices), and whether
a driver was bound to the device or not, the device managed actions
might or might not run.

This lead us in DRM to create helpers that will register a platform
device and bind it to a dumb driver so that we can have the proper
behaviour (ie, when we free the device, the device managed actions are
executed).

We wanted to create generic helpers for kunit to create a new device
instance to run a test on, and you were (not surprisingly) not really
along with it. We discussed the above fact that the bus and bind-ness of
a device was affecting device managed actions, I provided a bunch of
kunit tests showing the inconsistencies that led to what we did in DRM,
and you offered to fix it if I submitted the tests.

https://lore.kernel.org/linux-kselftest/ZB2a291P...@kroah.com/

And so here we are :)

Those tests are not doing much indeed but checking whether a device
managed action would run in various scenarii. If you run them, you'll
end up with:

$ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/base/test/
[10:28:39] Configuring KUnit Kernel ...
Regenerating .config ...
Populating config with:
$ make ARCH=um O=.kunit olddefconfig
[10:28:40] Building KUnit Kernel ...
Populating config with:
$ make ARCH=um O=.kunit olddefconfig
Building with:
$ make ARCH=um O=.kunit --jobs=32
[10:28:50] Starting KUnit Kernel (1/1)...
[10:28:50] ============================================================
[10:28:50] ============== root-device-devm (3 subtests) ===============
[10:28:50] [PASSED] root_device_devm_register_unregister_test
[10:28:50] [PASSED] root_device_devm_register_get_put_unregister_test
[10:28:50] # root_device_devm_register_get_unregister_with_devm_test: EXPECTATION FAILED at drivers/base/test/root-device-test.c:105
[10:28:50] Expected ret > 0, but
[10:28:50] ret == 0 (0x0)
[10:28:50] [FAILED] root_device_devm_register_get_unregister_with_devm_test
[10:28:50] # root-device-devm: pass:2 fail:1 skip:0 total:3
[10:28:50] # Totals: pass:2 fail:1 skip:0 total:3
[10:28:50] ================ [FAILED] root-device-devm =================
[10:28:50] ============ platform-device-devm (6 subtests) =============
[10:28:50] [PASSED] platform_device_devm_register_unregister_test
[10:28:51] [PASSED] platform_device_devm_register_get_put_unregister_test
[10:28:51] # platform_device_devm_register_get_unregister_with_devm_test: EXPECTATION FAILED at drivers/base/test/platform-device-test.c:123
[10:28:51] Expected ret > 0, but
[10:28:51] ret == 0 (0x0)
[10:28:51] [FAILED] platform_device_devm_register_get_unregister_with_devm_test
[10:28:51] [PASSED] probed_platform_device_devm_register_unregister_test
[10:28:51] [PASSED] probed_platform_device_devm_register_get_put_unregister_test
[10:28:51] [PASSED] probed_platform_device_devm_register_get_unregister_with_devm_test
[10:28:51] # platform-device-devm: pass:5 fail:1 skip:0 total:6
[10:28:51] # Totals: pass:5 fail:1 skip:0 total:6
[10:28:51] ============== [FAILED] platform-device-devm ===============
[10:28:51] ============================================================
[10:28:51] Testing complete. Ran 9 tests: passed: 7, failed: 2
[10:28:51] Elapsed time: 11.701s total, 0.979s configuring, 9.601s building, 1.087s running

So you can see (and test) those inconsistencies: if you're using devm,
you need to have a "bus" device bound to a driver. Failing that, devm
actions will not run, which we all believed was a bug in that thread
above.

Maxime
signature.asc

Greg Kroah-Hartman

unread,
Jun 7, 2023, 3:14:20 PM6/7/23
to Maxime Ripard, Rafael J. Wysocki, Brendan Higgins, David Gow, linux-k...@vger.kernel.org, kuni...@googlegroups.com, linux-...@vger.kernel.org
You have to have the context in the patch changelog itself, otherwise it
is useless (remember, some of us review hundreds of patches a week, and
have the short-term-memory of a squirrel.)

> It spawned from the discussion here:
> https://lore.kernel.org/linux-kselftest/20230324123157.bbwvfq4gsxnlnfwb@houat/
>
> Basically, depending on the bus (platform vs root devices), and whether
> a driver was bound to the device or not, the device managed actions
> might or might not run.

And is that correct? I don't remember if we said it was or not.

So why test something we don't know if it should be?

> This lead us in DRM to create helpers that will register a platform
> device and bind it to a dumb driver so that we can have the proper
> behaviour (ie, when we free the device, the device managed actions are
> executed).
>
> We wanted to create generic helpers for kunit to create a new device
> instance to run a test on, and you were (not surprisingly) not really
> along with it. We discussed the above fact that the bus and bind-ness of
> a device was affecting device managed actions, I provided a bunch of
> kunit tests showing the inconsistencies that led to what we did in DRM,
> and you offered to fix it if I submitted the tests.
>
> https://lore.kernel.org/linux-kselftest/ZB2a291P...@kroah.com/
>
> And so here we are :)
>
> Those tests are not doing much indeed but checking whether a device
> managed action would run in various scenarii. If you run them, you'll
> end up with:

Then document them please. You can't have tests that aren't obvious
what they are actually supposed to be testing, otherwise we have no idea
if the test is correct or not (or if the code it is testing is correct.)
So, what is the correct thing to do here? Fix the driver core? Don't
fix the driver core but document it? Something else? I don't think it
is to create an undocumented test :)

thanks,

greg k-h

Maxime Ripard

unread,
Jun 8, 2023, 3:59:57 AM6/8/23
to Greg Kroah-Hartman, Rafael J. Wysocki, Brendan Higgins, David Gow, linux-k...@vger.kernel.org, kuni...@googlegroups.com, linux-...@vger.kernel.org
It wasn't really supposed to be merged either (in my mind at least). It
started as a PoC showing the inconcistencies and then you asked for it
to be submitted so you could play with it more easily.

> > It spawned from the discussion here:
> > https://lore.kernel.org/linux-kselftest/20230324123157.bbwvfq4gsxnlnfwb@houat/
> >
> > Basically, depending on the bus (platform vs root devices), and whether
> > a driver was bound to the device or not, the device managed actions
> > might or might not run.
>
> And is that correct? I don't remember if we said it was or not.
>
> So why test something we don't know if it should be?

You never really confirmed it was, and David and I couldn't really judge
whether it was expected or not. I think it's one, David too:
https://lore.kernel.org/linux-kselftest/CABVgOSn8H=9pQfY7Exc-e37Nm6u299...@mail.gmail.com/

I don't have much experience with the core kernel, so I'm not definitive.
I have no idea. I have a very limited knowledge of the driver core so I
certainly can't say whether it was intentional or not, and thus how we
should address it.

It certainly look to me like we should fix it though.

> I don't think it is to create an undocumented test :)

I mean, yeah, definitely. But it was supposed to be patches for you to
test so you could see these issues for yourself after you asked for it,
not really something that should be merged as is.

Maxime
signature.asc
Reply all
Reply to author
Forward
0 new messages