[PATCH 0/4] drivers: base: Don't match device with NULL of_node/fwnode/etc + tests

[PATCH 0/4] drivers: base: Don't match device with NULL of_node/fwnode/etc + tests

[Brian Norris]

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

-- 
2.47.0.338.g60cca15819-goog

[PATCH 1/4] drivers: base: Don't match devices with NULL of_node/fwnode/etc

[Brian Norris]

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);
 
-- 
2.47.0.338.g60cca15819-goog

[PATCH 2/4] drivers: base: test: Enable device model tests with KUNIT_ALL_TESTS

[Brian Norris]

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
-- 
2.47.0.338.g60cca15819-goog

[PATCH 3/4] drivers: base: test: Drop "devm" from platform-device-test names

[Brian Norris]

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>");
-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH 3/4] drivers: base: test: Drop "devm" from platform-device-test names

[Maxime Ripard]

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

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

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

Re: [PATCH 3/4] drivers: base: test: Drop "devm" from platform-device-test names

[Brian Norris]

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

[PATCH 4/4] drivers: base: test: Add ...find_device_by...(... NULL) tests

[Brian Norris]

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),
 	{}
 };
 
-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH 0/4] drivers: base: Don't match device with NULL of_node/fwnode/etc + tests

[Rob Herring]

On Tue, Dec 10, 2024 at 1:14 PM Brian Norris <briannorris@chromium.org> wrote:
>
> 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(-)

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>