Received-SPF: pass (google.com: domain of linux-kernel+bounces-179184-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <5c919f0d3d72fe1592a11c45545e8a60.sboyd@kernel.org>
Content-Type: text/plain; charset="utf-8"
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <b822c6a5488c4098059b6d3c35eecbbd.sboyd@kernel.org>
References: <20240422232404.213174-1-sboyd@kernel.org> <CABVgOSmgUJp3FijpYGCphi1OzRUNvmYQmPDdL6mN59YnbkR2iQ@mail.gmail.com> <b822c6a5488c4098059b6d3c35eecbbd.sboyd@kernel.org>
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>, 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>, Rob Herring <robh@kernel.org>
Date: Tue, 14 May 2024 14:29:40 -0700
User-Agent: alot/0.10

Quoting Stephen Boyd (2024-05-02 18:27:42)
> Quoting David Gow (2024-05-01 01:08:11)
> >=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 =
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
>=20
> 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?
>=20
> ---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
> @@ -91,7 +92,8 @@ static void of_overlay_apply_kunit_cleanup(struct kunit=
 *test)
>         dev =3D bus_find_device(&platform_bus_type, NULL, kunit_compatibl=
e,
>                               of_overlay_bus_match_compatible);
>         KUNIT_EXPECT_PTR_EQ(test, NULL, dev);
> -       put_device(dev);
> +       if (dev)
> +               put_device(dev);
>  }

This last hunk isn't needed.

> =20
>  static struct kunit_case of_overlay_apply_kunit_test_cases[] =3D {
>=20
> > > [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 err=
or 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 li=
ne seen drivers/of/overlay_test.c:77
> > > [15:18:51]     # of_overlay_apply_kunit_cleanup: internal error occur=
red 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.
>=20
> 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.
>=20

powerpc doesn't mark the root node with OF_POPULATED_BUS. If I set that
in of_platform_default_populate_init() then the overlays can be applied.

---8<----
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 389d4ea6bfc1..fa7b439e9402 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -565,6 +565,10 @@ static int __init of_platform_default_populate_init(vo=
id)
 				of_platform_device_create(node, buf, NULL);
 		}
=20
+		node =3D of_find_node_by_path("/");
+		if (node)
+			of_node_set_flag(node, OF_POPULATED_BUS);
+		of_node_put(node);
 	} else {
 		/*
 		 * Handle certain compatibles explicitly, since we don't want to create

I'm guessing this is wrong though, because I see bunch of powerpc specific =
code
calling of_platform_bus_probe() which will set the flag on the actual platf=
orm
bus nodes. Maybe we should just allow overlays to create devices at the root
node regardless? Of course, the flag doc says "platform bus created for
children" and if we never populated the root then that isn't entirely accur=
ate.

Rob, can you point me in the right direction? Do we need to use simple-bus =
in
the test overlays and teach overlay code to populate that bus?