Re: [PATCH v2] of: WARN on deprecated #address-cells/#size-cells handling

[Steven Price <steven.price@arm.com>]

On 08/11/2024 14:04, Rob Herring wrote:
> On Fri, Nov 8, 2024 at 7:26 AM Steven Price <steven.price@arm.com> wrote:
>>
>> On 08/11/2024 11:04, Marek Szyprowski wrote:
>>> Hi Rob,
>>>
>>> On 06.11.2024 18:10, Rob Herring (Arm) wrote:
>>>> While OpenFirmware originally allowed walking parent nodes and default
>>>> root values for #address-cells and #size-cells, FDT has long required
>>>> explicit values. It's been a warning in dtc for the root node since the
>>>> beginning (2005) and for any parent node since 2007. Of course, not all
>>>> FDT uses dtc, but that should be the majority by far. The various
>>>> extracted OF devicetrees I have dating back to the 1990s (various
>>>> PowerMac, OLPC, PASemi Nemo) all have explicit root node properties. The
>>>> warning is disabled for Sparc as there are known systems relying on
>>>> default root node values.
>>>>
>>>> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
>>>> ---
>>>> v2:
>>>>   - Add a define for excluded platforms to help clarify the intent
>>>>     is to have an exclude list and make adding platforms easier.
>>>>   - Also warn when walking parent nodes.
>>>> ---
>>>>   drivers/of/base.c | 28 ++++++++++++++++++++++------
>>>>   drivers/of/fdt.c  |  4 ++--
>>>>   2 files changed, 24 insertions(+), 8 deletions(-)
>>>
>>> This patch landed in today's linux-next as commit 4b28a0dec185 ("of:
>>> WARN on deprecated #address-cells/#size-cells handling"). In my tests I
>>> found that it introduces warnings on almost all of my test systems. I
>>> took a look at the first one I got in my logs (Samsung Exynos Rinato
>>> board: arch/arm/boot/dts/samsung/exynos3250-rinato.dts):
>>
>> Just a "me too" for rk3288-firefly.dtb:
>>
>> [    0.138735] WARNING: CPU: 0 PID: 1 at drivers/of/base.c:106 of_bus_n_addr_cells+0x9c/0xd8
>> [    0.138776] Missing '#address-cells' in /power-management@ff730000
>>
>> I'm sure it's easy to fix up the DTB, but we shouldn't be breaking long existing DTBs.
> 
> What broke?

Nothing 'broke' as such (the board continued booting) but the WARN
shouldn't be happening. My CI treats the WARN as a failure as these
shouldn't occur unless there's a programming error.

> The intent here is to exclude any platforms/arch which actually need
> the deprecated behavior, not change DTBs. That's spelled out at the
> WARN which I assume people would read before fixing "Missing
> '#address-cells' in /power-management@ff730000". I tried to make the
> warn message indicate that on v1 with:
> 
> WARN_ONCE(!IS_ENABLED(CONFIG_SPARC), "Only listed platforms should
> rely on default '#address-cells'\n");

So one possibility is to include this platform in the exclusion list -
but I'm not sure how to do that, I assume including CONFIG_ARM in the
list would rather defeat the point of the patch. But my feeling is that
it would involve a lot of playing whack-a-mole to identify individual
platforms.

One obvious idea would be to look at the DTBs in the kernel tree and see
which are affected by this currently, that might be a good place to
start with an exclusion list.

You could also downgrade the warning to a pr_warn() or similar.

> But Conor thought that wasn't clear. So I'm open to suggestions...

I don't have any particular suggestions other than above, I just wanted
to report an existing DTB that triggers this WARN. We need to resolve
this one way or another before this patch can progress. For now I've
simply reverted this patch for my CI.

Steve