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: <6d7c0eda-d93e-43b3-8b85-40b4ef094277@samsung.com>
Date: Fri, 8 Nov 2024 16:28:23 +0100
From: Marek Szyprowski <m.szyprowski@...sung.com>
To: Rob Herring <robh@...nel.org>
Cc: 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

Hi Rob,

On 08.11.2024 14:25, Rob Herring wrote:
> On Fri, Nov 8, 2024 at 5:04 AM Marek Szyprowski
> <m.szyprowski@...sung.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@...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):
> 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@...sung.com>
Tested-by: Marek Szyprowski <m.szyprowski@...sung.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