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.
