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

[Marek Szyprowski <m.szyprowski@samsung.com>]

Hi Rob,

On 08.11.2024 14:25, Rob Herring wrote:
> On Fri, Nov 8, 2024 at 5:04 AM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> 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):
> Thanks for the report. Let me know if any others have a different
> backtrace. Also, since it's a WARN_ONCE, fixing one case could expose
> others.

> >...

> I'm going to fold in the following fix which should fix the warning:

This fixes all the warnings I've observed.

Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>


> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 824bb449e007..f21f4699df7a 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -333,7 +333,8 @@ static unsigned int of_bus_isa_get_flags(const __be32 *addr)
>
>   static int of_bus_default_flags_match(struct device_node *np)
>   {
> -       return of_bus_n_addr_cells(np) == 3;
> +       /* Check for presence first since of_bus_n_addr_cells() will
> walk parents */
> +       return of_property_present(np, "#address-cells") &&
> (of_bus_n_addr_cells(np) == 3);
>   }
>
>   /*
> @@ -701,16 +702,16 @@ const __be32 *__of_get_address(struct
> device_node *dev, int index, int bar_no,
>          if (strcmp(bus->name, "pci") && (bar_no >= 0))
>                  return NULL;
>
> -       bus->count_cells(dev, &na, &ns);
> -       if (!OF_CHECK_ADDR_COUNT(na))
> -               return NULL;
> -
>          /* Get "reg" or "assigned-addresses" property */
>          prop = of_get_property(dev, bus->addresses, &psize);
>          if (prop == NULL)
>                  return NULL;
>          psize /= 4;
>
> +       bus->count_cells(dev, &na, &ns);
> +       if (!OF_CHECK_ADDR_COUNT(na))
> +               return NULL;
> +
>          onesize = na + ns;
>          for (i = 0; psize >= onesize; psize -= onesize, prop += onesize, i++) {
>                  u32 val = be32_to_cpu(prop[0]);
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland