Quoting David Gow (2024-05-01 01:08:11)
Thanks very much. I'm about halfway through reviewing these, and I like them a lot so far.
Most of my thoughts are just naming ideas. I fear some of them may be the reverse of previous suggestions, as we've since landed the KUnit device wrappers in include/kunit/device.h, which we decided would live as part of KUnit, not as part of the device infrastructure. I don't enormously mind if we make the opposite decision for these, though it does seem a bit inconsistent if we do 'devices' differently from 'platform_devices'. Thoughts?
Let's discuss on one of the patches.
The other thing I've noted so far is that the of_apply_kunit_platform_device and of_overlay_apply_kunit_cleanup tests fail (and BUG() with a NULL pointer) on powerpc:
[15:18:51] # of_overlay_apply_kunit_platform_device: EXPECTATION FAILED at drivers/of/overlay_test.c:47 [15:18:51] Expected pdev is not null, but is [15:18:51] BUG: Kernel NULL pointer dereference at 0x0000004c
This seems to be because pdev is NULL and we call put_device(&pdev->dev) on it. We could be nicer and have an 'if (pdev)' check there. I wonder if that fixes the other two below?
---8<--- diff --git a/drivers/of/overlay_test.c b/drivers/of/overlay_test.c index 223e5a5c23c5..85cfbe6bb132 100644 --- a/drivers/of/overlay_test.c +++ b/drivers/of/overlay_test.c @@ -45,7 +45,8 @@ static void of_overlay_apply_kunit_platform_device(struct kunit *test)
pdev = of_find_device_by_node(np); KUNIT_EXPECT_NOT_ERR_OR_NULL(test, pdev); - put_device(&pdev->dev); + if (pdev) + put_device(&pdev->dev); }
static int of_overlay_bus_match_compatible(struct device *dev, const void *data) @@ -77,8 +78,8 @@ static void of_overlay_apply_kunit_cleanup(struct kunit *test) KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
pdev = of_find_device_by_node(np); - put_device(&pdev->dev); /* Not derefing 'pdev' after this */ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev); + put_device(&pdev->dev); /* Not derefing 'pdev' after this */
/* Remove overlay */ kunit_cleanup(&fake); @@ -91,7 +92,8 @@ static void of_overlay_apply_kunit_cleanup(struct kunit *test) dev = bus_find_device(&platform_bus_type, NULL, kunit_compatible, of_overlay_bus_match_compatible); KUNIT_EXPECT_PTR_EQ(test, NULL, dev); - put_device(dev); + if (dev) + put_device(dev); }
static struct kunit_case of_overlay_apply_kunit_test_cases[] = {
[15:18:51] # of_overlay_apply_kunit_platform_device: try faulted: last line seen lib/kunit/resource.c:99 [15:18:51] # of_overlay_apply_kunit_platform_device: internal error occurred preventing test case from running: -4 [15:18:51] [FAILED] of_overlay_apply_kunit_platform_device
[15:18:51] BUG: Kernel NULL pointer dereference at 0x0000004c [15:18:51] note: kunit_try_catch[698] exited with irqs disabled [15:18:51] # of_overlay_apply_kunit_cleanup: try faulted: last line seen drivers/of/overlay_test.c:77 [15:18:51] # of_overlay_apply_kunit_cleanup: internal error occurred preventing test case from running: -4 [15:18:51] [FAILED] of_overlay_apply_kunit_cleanup
I've not had a chance to dig into it any further, yet, but it appears to work on all of the other architectures I tried.
Cool. I don't know why powerpc doesn't make devices. Maybe it has a similar design to sparc to create resources. I'll check it out.