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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <994eb729-91d3-4e96-a63e-fa0ea49f4cb7@arm.com>
Date: Fri, 8 Nov 2024 15:29:48 +0000
From: Steven Price <steven.price@....com>
To: Rob Herring <robh@...nel.org>
Cc: Marek Szyprowski <m.szyprowski@...sung.com>,
 Saravana Kannan <saravanak@...gle.com>, Krzysztof Kozlowski
 <krzk@...nel.org>, linuxppc-dev@...ts.ozlabs.org,
 Conor Dooley <conor@...nel.org>, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org,
 Linux Samsung SOC <linux-samsung-soc@...r.kernel.org>
Subject: Re: [PATCH v2] of: WARN on deprecated #address-cells/#size-cells
 handling

On 08/11/2024 14:58, Rob Herring wrote:
> On Fri, Nov 8, 2024 at 8:33 AM Steven Price <steven.price@....com> wrote:
>>
>> On 08/11/2024 14:04, Rob Herring wrote:
>>> On Fri, Nov 8, 2024 at 7:26 AM Steven Price <steven.price@....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@...nel.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@...30000
>>>>
>>>> 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@...30000". 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.
> 
> Please see my posted fix in this thread. Things "broke" quite a bit
> more widely than anticipated.

Thanks for the pointer. Yes that fix seems to work for my board!

Thanks,
Steve

>> 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.
> 
> It's been a dtc warning since 2007, so I can say all of the in tree
> dts's are fine. The problem for these reported platforms is the
> kernel, not the DT.
> 
>> You could also downgrade the warning to a pr_warn() or similar.
> 
> I find that pr_warn() may or may not get noticed, but WARN for sure
> will which is what I want here.
> 
> Rob