Received-SPF: pass (google.com: domain of linux-kernel+bounces-167142-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <b822c6a5488c4098059b6d3c35eecbbd.sboyd@kernel.org>
Content-Type: text/plain; charset="utf-8"
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <CABVgOSmgUJp3FijpYGCphi1OzRUNvmYQmPDdL6mN59YnbkR2iQ@mail.gmail.com>
References: <20240422232404.213174-1-sboyd@kernel.org> <CABVgOSmgUJp3FijpYGCphi1OzRUNvmYQmPDdL6mN59YnbkR2iQ@mail.gmail.com>
Subject: Re: [PATCH v4 00/10] clk: Add kunit tests for fixed rate and parent data
From: Stephen Boyd <sboyd@kernel.org>
Cc: Michael Turquette <mturquette@baylibre.com>, linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org, patches@lists.linux.dev, kunit-dev@googlegroups.com, linux-kselftest@vger.kernel.org, devicetree@vger.kernel.org, Brendan Higgins <brendan.higgins@linux.dev>, Rae Moar <rmoar@google.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Rafael J . Wysocki <rafael@kernel.org>, Rob Herring <robh@kernel.org>, Saravana Kannan <saravanak@google.com>, Daniel Latypov <dlatypov@google.com>, Christian Marangi <ansuelsmth@gmail.com>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, Maxime Ripard <maxime@cerno.tech>
To: David Gow <davidgow@google.com>
Date: Thu, 02 May 2024 18:27:42 -0700
User-Agent: alot/0.10

Quoting David Gow (2024-05-01 01:08:11)
>=20
> Thanks very much. I'm about halfway through reviewing these, and I
> like them a lot so far.
>=20
> 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.

>=20
> 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 FA=
ILED 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)
=20
 	pdev =3D of_find_device_by_node(np);
 	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, pdev);
-	put_device(&pdev->dev);
+	if (pdev)
+		put_device(&pdev->dev);
 }
=20
 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);
=20
 	pdev =3D 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 */
=20
 	/* Remove overlay */
 	kunit_cleanup(&fake);
@@ -91,7 +92,8 @@ static void of_overlay_apply_kunit_cleanup(struct kunit *=
test)
 	dev =3D 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);
 }
=20
 static struct kunit_case of_overlay_apply_kunit_test_cases[] =3D {

> > [15:18:51]     # of_overlay_apply_kunit_platform_device: try faulted: l=
ast 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
>=20
> > [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 occurre=
d preventing test case from running: -4
> > [15:18:51] [FAILED] of_overlay_apply_kunit_cleanup
>=20
> 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.