Re: [PATCH v4 00/10] clk: Add kunit tests for fixed rate and parent data

[Stephen Boyd]

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.