This series: 1. makes the behavior of_find_device_by_node(), bus_find_device_by_of_node(), bus_find_device_by_fwnode(), etc., more consistent when provided with a NULL node/handle; 2. adds kunit tests to validate the new NULL-argument behavior; and 3. makes some related improvements and refactoring for the drivers/base/ kunit tests.
This series aims to prevent problems like the ones resolved in commit 5c8418cf4025 ("PCI/pwrctrl: Unregister platform device only if one actually exists").
Brian Norris (4): drivers: base: Don't match devices with NULL of_node/fwnode/etc drivers: base: test: Enable device model tests with KUNIT_ALL_TESTS drivers: base: test: Drop "devm" from platform-device-test names drivers: base: test: Add ...find_device_by...(... NULL) tests
drivers/base/core.c | 8 ++--- drivers/base/test/Kconfig | 1 + drivers/base/test/platform-device-test.c | 42 ++++++++++++++++++++---- 3 files changed, 40 insertions(+), 11 deletions(-)
of_find_device_by_node(), bus_find_device_by_of_node(), bus_find_device_by_fwnode(), ..., all produce arbitrary results when provided with a NULL of_node, fwnode, ACPI handle, etc. This is counterintuitive, and the source of a few bugs, such as the one fixed by commit 5c8418cf4025 ("PCI/pwrctrl: Unregister platform device only if one actually exists").
It's hard to imagine a good reason that these device_match_*() APIs should return 'true' for a NULL argument. Augment these to return 0 (false).
Signed-off-by: Brian Norris briannorris@chromium.org ---
drivers/base/core.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c index 94865c9d8adc..2b7b13fc36d7 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -5246,13 +5246,13 @@ EXPORT_SYMBOL_GPL(device_match_name);
int device_match_of_node(struct device *dev, const void *np) { - return dev->of_node == np; + return np && dev->of_node == np; } EXPORT_SYMBOL_GPL(device_match_of_node);
int device_match_fwnode(struct device *dev, const void *fwnode) { - return dev_fwnode(dev) == fwnode; + return fwnode && dev_fwnode(dev) == fwnode; } EXPORT_SYMBOL_GPL(device_match_fwnode);
@@ -5264,13 +5264,13 @@ EXPORT_SYMBOL_GPL(device_match_devt);
int device_match_acpi_dev(struct device *dev, const void *adev) { - return ACPI_COMPANION(dev) == adev; + return adev && ACPI_COMPANION(dev) == adev; } EXPORT_SYMBOL(device_match_acpi_dev);
int device_match_acpi_handle(struct device *dev, const void *handle) { - return ACPI_HANDLE(dev) == handle; + return handle && ACPI_HANDLE(dev) == handle; } EXPORT_SYMBOL(device_match_acpi_handle);
Per commit bebe94b53eb7 ("drivers: base: default KUNIT_* fragments to KUNIT_ALL_TESTS"), it seems like we should default to KUNIT_ALL_TESTS.
This enables these platform_device tests for common configurations, such as with: ./tools/testing/kunit/kunit.py run
Signed-off-by: Brian Norris briannorris@chromium.org ---
drivers/base/test/Kconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/base/test/Kconfig b/drivers/base/test/Kconfig index 5c7fac80611c..2756870615cc 100644 --- a/drivers/base/test/Kconfig +++ b/drivers/base/test/Kconfig @@ -12,6 +12,7 @@ config TEST_ASYNC_DRIVER_PROBE config DM_KUNIT_TEST tristate "KUnit Tests for the device model" if !KUNIT_ALL_TESTS depends on KUNIT + default KUNIT_ALL_TESTS
config DRIVER_PE_KUNIT_TEST tristate "KUnit Tests for property entry API" if !KUNIT_ALL_TESTS
This is a reasonably-helpful base for generic platform_device tests, and I'd like to add more tests that aren't specifically about "devm" functions. Drop the devm namings for the suite, for clarity.
Signed-off-by: Brian Norris briannorris@chromium.org ---
drivers/base/test/platform-device-test.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/base/test/platform-device-test.c b/drivers/base/test/platform-device-test.c index ea05b8785743..fd871bb9e143 100644 --- a/drivers/base/test/platform-device-test.c +++ b/drivers/base/test/platform-device-test.c @@ -15,7 +15,7 @@ struct test_priv { struct device *dev; };
-static int platform_device_devm_init(struct kunit *test) +static int platform_device_init(struct kunit *test) { struct test_priv *priv;
@@ -203,7 +203,7 @@ static void probed_platform_device_devm_register_get_unregister_with_devm_test(s platform_driver_unregister(&fake_driver); }
-static struct kunit_case platform_device_devm_tests[] = { +static struct kunit_case platform_device_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), @@ -211,13 +211,13 @@ static struct kunit_case platform_device_devm_tests[] = { {} };
-static struct kunit_suite platform_device_devm_test_suite = { - .name = "platform-device-devm", - .init = platform_device_devm_init, - .test_cases = platform_device_devm_tests, +static struct kunit_suite platform_device_test_suite = { + .name = "platform-device", + .init = platform_device_init, + .test_cases = platform_device_tests, };
-kunit_test_suite(platform_device_devm_test_suite); +kunit_test_suite(platform_device_test_suite);
MODULE_DESCRIPTION("Test module for platform devices"); MODULE_AUTHOR("Maxime Ripard mripard@kernel.org");
Hi,
On Tue, Dec 10, 2024 at 11:13:32AM -0800, Brian Norris wrote:
This is a reasonably-helpful base for generic platform_device tests, and I'd like to add more tests that aren't specifically about "devm" functions. Drop the devm namings for the suite, for clarity.
Signed-off-by: Brian Norris briannorris@chromium.org
drivers/base/test/platform-device-test.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/base/test/platform-device-test.c b/drivers/base/test/platform-device-test.c index ea05b8785743..fd871bb9e143 100644 --- a/drivers/base/test/platform-device-test.c +++ b/drivers/base/test/platform-device-test.c @@ -15,7 +15,7 @@ struct test_priv { struct device *dev; }; -static int platform_device_devm_init(struct kunit *test) +static int platform_device_init(struct kunit *test) { struct test_priv *priv; @@ -203,7 +203,7 @@ static void probed_platform_device_devm_register_get_unregister_with_devm_test(s platform_driver_unregister(&fake_driver); } -static struct kunit_case platform_device_devm_tests[] = { +static struct kunit_case platform_device_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), @@ -211,13 +211,13 @@ static struct kunit_case platform_device_devm_tests[] = { {} }; -static struct kunit_suite platform_device_devm_test_suite = {
- .name = "platform-device-devm",
- .init = platform_device_devm_init,
- .test_cases = platform_device_devm_tests,
+static struct kunit_suite platform_device_test_suite = {
- .name = "platform-device",
- .init = platform_device_init,
- .test_cases = platform_device_tests,
};
The rest of the patches look ok to me, but it still seems like it tests something different (ie, devm actions) so I don't see why we should group them in the same test suite.
Maxime
Hi Maxime,
On Wed, Dec 11, 2024 at 06:05:49PM +0100, Maxime Ripard wrote:
On Tue, Dec 10, 2024 at 11:13:32AM -0800, Brian Norris wrote:
This is a reasonably-helpful base for generic platform_device tests, and I'd like to add more tests that aren't specifically about "devm" functions. Drop the devm namings for the suite, for clarity.
Signed-off-by: Brian Norris briannorris@chromium.org
drivers/base/test/platform-device-test.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/base/test/platform-device-test.c b/drivers/base/test/platform-device-test.c index ea05b8785743..fd871bb9e143 100644 --- a/drivers/base/test/platform-device-test.c +++ b/drivers/base/test/platform-device-test.c
@@ -211,13 +211,13 @@ static struct kunit_case platform_device_devm_tests[] = { {} }; -static struct kunit_suite platform_device_devm_test_suite = {
- .name = "platform-device-devm",
- .init = platform_device_devm_init,
- .test_cases = platform_device_devm_tests,
+static struct kunit_suite platform_device_test_suite = {
- .name = "platform-device",
- .init = platform_device_init,
- .test_cases = platform_device_tests,
};
The rest of the patches look ok to me, but it still seems like it tests something different (ie, devm actions) so I don't see why we should group them in the same test suite.
My goal was to avoid adding a new test file for every sub-topic of "test platform devices". Would adding a second suite in this file make more sense, then? If so, I'll just drop this patch, and do that when adding the test.
(I'm not that familiar with kunit conventions yet.)
Brian
We recently updated these device_match*() (and therefore, various *find_device_by*()) functions to return a consistent 'false' value when trying to match a NULL handle. Add tests for this.
This provides regression-testing coverage for the sorts of bugs that underly commit 5c8418cf4025 ("PCI/pwrctrl: Unregister platform device only if one actually exists").
Signed-off-by: Brian Norris briannorris@chromium.org ---
drivers/base/test/platform-device-test.c | 28 ++++++++++++++++++++++++ 1 file changed, 28 insertions(+)
diff --git a/drivers/base/test/platform-device-test.c b/drivers/base/test/platform-device-test.c index fd871bb9e143..f9d1a269a479 100644 --- a/drivers/base/test/platform-device-test.c +++ b/drivers/base/test/platform-device-test.c @@ -2,7 +2,9 @@
#include <kunit/resource.h>
+#include <linux/device/bus.h> #include <linux/device.h> +#include <linux/of_platform.h> #include <linux/platform_device.h>
#define DEVICE_NAME "test" @@ -203,11 +205,37 @@ static void probed_platform_device_devm_register_get_unregister_with_devm_test(s platform_driver_unregister(&fake_driver); }
+static void platform_device_find_by_null_test(struct kunit *test) +{ + struct platform_device *pdev; + 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); + + KUNIT_EXPECT_PTR_EQ(test, of_find_device_by_node(NULL), NULL); + + KUNIT_EXPECT_PTR_EQ(test, bus_find_device_by_of_node(&platform_bus_type, NULL), NULL); + KUNIT_EXPECT_PTR_EQ(test, bus_find_device_by_fwnode(&platform_bus_type, NULL), NULL); + KUNIT_EXPECT_PTR_EQ(test, bus_find_device_by_acpi_dev(&platform_bus_type, NULL), NULL); + + KUNIT_EXPECT_FALSE(test, device_match_of_node(&pdev->dev, NULL)); + KUNIT_EXPECT_FALSE(test, device_match_fwnode(&pdev->dev, NULL)); + KUNIT_EXPECT_FALSE(test, device_match_acpi_dev(&pdev->dev, NULL)); + KUNIT_EXPECT_FALSE(test, device_match_acpi_handle(&pdev->dev, NULL)); + + platform_device_unregister(pdev); +} + static struct kunit_case platform_device_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), + KUNIT_CASE(platform_device_find_by_null_test), {} };
On Tue, Dec 10, 2024 at 1:14 PM Brian Norris briannorris@chromium.org wrote:
This series:
- makes the behavior of_find_device_by_node(), bus_find_device_by_of_node(), bus_find_device_by_fwnode(), etc., more consistent when provided with a NULL node/handle;
- adds kunit tests to validate the new NULL-argument behavior; and
- makes some related improvements and refactoring for the drivers/base/ kunit tests.
This series aims to prevent problems like the ones resolved in commit 5c8418cf4025 ("PCI/pwrctrl: Unregister platform device only if one actually exists").
Brian Norris (4): drivers: base: Don't match devices with NULL of_node/fwnode/etc drivers: base: test: Enable device model tests with KUNIT_ALL_TESTS drivers: base: test: Drop "devm" from platform-device-test names drivers: base: test: Add ...find_device_by...(... NULL) tests
drivers/base/core.c | 8 ++--- drivers/base/test/Kconfig | 1 + drivers/base/test/platform-device-test.c | 42 ++++++++++++++++++++---- 3 files changed, 40 insertions(+), 11 deletions(-)
Reviewed-by: Rob Herring (Arm) robh@kernel.org
linux-kselftest-mirror@lists.linaro.org
-
Brian Norris -
Maxime Ripard -
Rob Herring