| Message ID | 20241212003201.2098123-1-briannorris@chromium.org |
|---|---|
| Headers | show |
| Series | drivers: base: Don't match device with NULL of_node/fwnode/etc + tests | expand |
Hi, On Wed, Dec 11, 2024 at 04:31:41PM -0800, Brian Norris wrote: > 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> > --- > > Changes in v2: > * Keep "devm" and "match" tests in separate suites > > drivers/base/test/platform-device-test.c | 42 +++++++++++++++++++++++- > 1 file changed, 41 insertions(+), 1 deletion(-) > > diff --git a/drivers/base/test/platform-device-test.c b/drivers/base/test/platform-device-test.c > index ea05b8785743..c8d4b0a385f2 100644 > --- a/drivers/base/test/platform-device-test.c > +++ b/drivers/base/test/platform-device-test.c > @@ -3,6 +3,8 @@ > #include <kunit/resource.h> > > #include <linux/device.h> > +#include <linux/device/bus.h> > +#include <linux/of_platform.h> > #include <linux/platform_device.h> > > #define DEVICE_NAME "test" > @@ -217,7 +219,45 @@ static struct kunit_suite platform_device_devm_test_suite = { > .test_cases = platform_device_devm_tests, > }; > > -kunit_test_suite(platform_device_devm_test_suite); > +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); I *think* you have a bug there: if platform_device_add fails, KUNIT_ASSERT will stop the test execution and thus you will leak the platform_device you just allocated. You need to call platform_device_put in such a case, but if platform_device_add succeeds then you need to call platform_device_unregister instead. It would be better to use kunit_platform_device_alloc and kunit_platform_device_add that already deal with this. The rest looks good to me, once fixed: Reviewed-by: Maxime Ripard <mripard@kernel.org> Maxime
On Thu, Dec 12, 2024 at 1:32 AM Brian Norris <briannorris@chromium.org> wrote: > > 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> > Reviewed-by: Rob Herring (Arm) <robh@kernel.org> For the ACPI part Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > > Changes in v2: > * Add Rob's Reviewed-by > > 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 >
Hi Maxime, On Fri, Dec 13, 2024 at 12:59:57PM +0100, Maxime Ripard wrote: > On Wed, Dec 11, 2024 at 04:31:41PM -0800, Brian Norris wrote: > > --- a/drivers/base/test/platform-device-test.c > > +++ b/drivers/base/test/platform-device-test.c > > @@ -217,7 +219,45 @@ static struct kunit_suite platform_device_devm_test_suite = { > > .test_cases = platform_device_devm_tests, > > }; > > > > -kunit_test_suite(platform_device_devm_test_suite); > > +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); > > I *think* you have a bug there: if platform_device_add fails, > KUNIT_ASSERT will stop the test execution and thus you will leak the > platform_device you just allocated. > > You need to call platform_device_put in such a case, but if > platform_device_add succeeds then you need to call > platform_device_unregister instead. Hehe, well I'm imitating the existing leaks in the other tests in this file, then ;) But admittedly, those are a little more complex, because the unregistration is actually part of the test flow. > It would be better to use kunit_platform_device_alloc and > kunit_platform_device_add that already deal with this. Cool, thanks, I'll use those in v3 for my new test. > The rest looks good to me, once fixed: > Reviewed-by: Maxime Ripard <mripard@kernel.org> Thanks for the tips and review. Brian
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"). Changes in v2: * Add Rob's Reviewed-by * CC LKML (oops!) * Keep "devm" and "match" tests in separate suites Brian Norris (3): 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: 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, 46 insertions(+), 5 deletions(-)