On Tue, May 14, 2024 at 4:29 PM Stephen Boyd <sboyd@xxxxxxxxxx> wrote:
>
> Quoting Stephen Boyd (2024-05-02 18:27:42)
> > Quoting David Gow (2024-05-01 01:08:11)
> > >
> > > 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
> > @@ -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);
> > }
>
> This last hunk isn't needed.
>
> >
> > 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.
> >
>
> 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(void)
> of_platform_device_create(node, buf, NULL);
> }
>
> + node = of_find_node_by_path("/");
> + if (node)
> + of_node_set_flag(node, OF_POPULATED_BUS);
I think you want to do this in of_platform_bus_probe() instead to
mirror of_platform_populate(). These are supposed to be the same
except that 'populate' only creates devices for nodes with compatible
while 'probe' will create devices for all child nodes. Looks like we
are missing some devlink stuff too. There may have been some issue for
PPC with it.
> + 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 platform
> 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 accurate.
>
> 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?
Overlays adding things to the root node might be suspect, but probably
there are some valid reasons to do so. If simple-bus makes sense here,
then yes, you should use it. But if what's on it is not MMIO devices,
don't. That's a warning in the schema now.
Rob
