Message-ID: <4a8abb5f501279de7907629f4dd6be24.sboyd@kernel.org>
Date: Thu, 03 Oct 2024 16:46:40 -0700
From: Stephen Boyd <sboyd@...nel.org>
To: Guenter Roeck <linux@...ck-us.net>, Shuah Khan <skhan@...uxfoundation.org>
Cc: Michael Turquette <mturquette@...libre.com>, linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org, patches@...ts.linux.dev, kunit-dev@...glegroups.com, linux-kselftest@...r.kernel.org, devicetree@...r.kernel.org, Brendan Higgins <brendan.higgins@...ux.dev>, David Gow <davidgow@...gle.com>, Rae Moar <rmoar@...gle.com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Rafael J . Wysocki <rafael@...nel.org>, Rob Herring <robh@...nel.org>, Saravana Kannan <saravanak@...gle.com>, Daniel Latypov <dlatypov@...gle.com>, Christian Marangi <ansuelsmth@...il.com>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, Maxime Ripard <maxime@...no.tech>, Geert Uytterhoeven <geert+renesas@...der.be>
Subject: Re: [PATCH v8 8/8] clk: Add KUnit tests for clks registered with struct clk_parent_data

Quoting Guenter Roeck (2024-09-28 14:32:35)
> On 9/28/24 12:27, Shuah Khan wrote:
> > On 9/28/24 11:54, Shuah Khan wrote:
> >> On 9/28/24 11:31, Guenter Roeck wrote:
> >>> On 9/27/24 17:08, Guenter Roeck wrote:
> >>>> On 9/27/24 13:45, Shuah Khan wrote:
> >>>>> On 9/27/24 10:19, Guenter Roeck wrote:
> >>>>>> Copying devicetree maintainers.
> >>>>>>
> >>>>>> On Thu, Sep 26, 2024 at 09:39:38PM -0700, Guenter Roeck wrote:
> >>>>>>> On Thu, Sep 26, 2024 at 09:14:11PM -0700, Guenter Roeck wrote:
> >>>>>>>> Hi Stephen,
> >>>>>>>>
> >>>>>>>> On Thu, Jul 18, 2024 at 02:05:07PM -0700, Stephen Boyd wrote:
> >>>>>>>>> Test that clks registered with 'struct clk_parent_data' work as
> >>>>>>>>> intended and can find their parents.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> When testing this on arm64, I see the error below. The error is only
> >>>>>>>> seen if I boot through efi, i.e., with "-bios QEMU_EFI-aarch64.fd"
> >>>>>>>> qemu parameter.
> >>>>>>>>
> >>>>>>>> Any idea what might cause the problem ?
> >>>>>>>>
> >>>>>>> I noticed that the new overlay tests fail as well, also with "path '/' not
> >>>>>>> found".
> >>>>>>>
> >>>>>>> [Maybe] answering my own question: I think the problem may be that there
> >>>>>>> is no devicetree file and thus no devicetree root when booting through
> >>>>>>> efi (in other words, of_root is NULL). Would it make sense to skip the
> >>>>>>> tests in that case ?
> >>>>>>>
> >>>>>>
> >>>>>> The problem is that of_root is not initialized in arm64 boots if ACPI
> >>>>>> is enabled.
> >>>>>>
> >>>>>>  From arch/arm64/kernel/setup.c:setup_arch():
> >>>>>>
> >>>>>>     if (acpi_disabled)
> >>>>>>         unflatten_device_tree();        // initializes of_root

Oof I forgot that Rob didn't apply the patch that let an empty root live
on ARM64 ACPI systems. See this thread[1] for all the details.

> >>>>>>
> >>>>>> ACPI is enabled if the system boots from EFI. This also affects
> >>>>>> CONFIG_OF_KUNIT_TEST, which explicitly checks if of_root exists and
> >>>>>> fails the test if it doesn't.
> >>>>>>
> >>>>>> I think those tests need to add a check for this condition, or affected
> >>>>>> machines won't be able to run those unit tests. The obvious solution would
> >>>>>> be to check if of_root is set, but then the associated test case in
> >>>>>> CONFIG_OF_KUNIT_TEST would not make sense.
> >>>>>>
> >>>>>> Any suggestions ?
> >>>>>>

I think that's the best we can do for now. Basically add a check like

	if (IS_ENABLED(CONFIG_ARM64) && acpi_disabled)
		kunit_skip(test, "ARM64 + ACPI rejects DT overlays");

to the overlay application function and the DT test.

> >>>>>
> >>>>> Would it work if these tests check if acpi_disabled and skip if it isn't
> >>>>> disabled? It might be low overhead condition to check from these tests.
> >>>>>
> >>>>> acpi_disabled is exported:
> >>>>>
> >>>>> arch/arm64/kernel/acpi.c:EXPORT_SYMBOL(acpi_disabled);
> >>>>> arch/loongarch/kernel/acpi.c:EXPORT_SYMBOL(acpi_disabled);
> >>>>> arch/riscv/kernel/acpi.c:EXPORT_SYMBOL(acpi_disabled);
> >>>>> arch/x86/kernel/acpi/boot.c:EXPORT_SYMBOL(acpi_disabled);
> >>>>>
> >>>>
> >>>> I don't think that would work. Looking through the use of acpi_init,
> >>>> I don't think that of_root is always NULL when acpi_init is false; that
> >>>> just happens to be the case on arm64 when booting through efi.
> >>>> However, even arm64 has the following code.
> >>>>
> >>>>          if (acpi_disabled)
> >>>>                  psci_dt_init();
> >>>>          else
> >>>>                  psci_acpi_init();
> >>>>
> >>>> While psci_dt_init() doesn't set of_root, it does try to do a devicetree
> >>>> match. So there must be some other condition where acpi_disabled is set
> >>>> but of_root is set anyway. I just have not found that code path.
> >>>>
> >>>
> >>> I ended up disabling all affected unit tests for arm64. I'll do the same
> >>> for other architectures if I encounter the problem there as well.
> >>>
> >>> Unfortunately that includes all clock unit tests because the tests requiring
> >>> devicetree support can not be enabled/disabled separately, but that can't be
> >>> helped and is still better than "mandatory" failures.
> >>>
> >>
> > 
> > of_root is set in drivers/of/pdt.c when it creates the root node.
> > This could be a definitive test for kunit tests that depend on
> > devicetree support.
> > 
> 
> That is not always the case, including arm64. It is primarily set in
> unflatten_devicetree(), which is not called on arm64 unless acpi_is disabled
> (see above).

> 
> > It is an exported symbol. drivers/of/base.c exports it.
> > 
> 
> Yes, checking if of_root is NULL and skipping the test in that case might help,
> but then there is the of_dtb_root_node_populates_of_root unit test which
> explicitly fails if of_root is NULL. The comment describing the test is
> 
> /*
>   * Test that the 'of_root' global variable is always populated when DT code is
>   * enabled. Remove this test once of_root is removed from global access.
>   */
> 
> The devicetree unit test code explicitly assumes that of_root is set if
> CONFIG_OF_EARLY_FLATTREE is enabled, but that is not always the case
> (again, of_root is NULL on arm64 unless acpi is disabled).
> 

That DT test has been there for a few releases. Is this the first time
those tests have been run on arm64+acpi? I didn't try after sending the
patches and forgot that the patch was dropped.

How are you running kunit tests? I installed the qemu-efi-aarch64 debian
package to get QEMU_EFI.fd but passing that to the kunit.py run command
with --qemu_args="-bios /usr/share/qemu-efi-aarch64/QEMU_EFI.fd" didn't
get me beyond the point that the EFI stub boots linux. I think the
serial console must not be working and thus the kunit wrapper waits for
something to show up but nothing ever does. I haven't dug any further
though, so maybe you have a working command.

Here's my command that isn't working:

./tools/testing/kunit/kunit.py run --arch=arm64 --kunitconfig=drivers/of --qemu_args="-bios /usr/share/qemu-efi-aarch64/QEMU_EFI.fd"	

[1] https://lore.kernel.org/all/20240217010557.2381548-6-sboyd@kernel.org/

Powered by blists - more mailing lists