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: <CAL_JsqKpv73RXLhdgox5gjxRUdaP7TVQyMs5rur+Ac=S4OtEFw@mail.gmail.com>
Date: Fri, 8 Nov 2024 07:25:45 -0600
From: Rob Herring <robh@...nel.org>
To: Marek Szyprowski <m.szyprowski@...sung.com>
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

On Fri, Nov 8, 2024 at 5:04 AM Marek Szyprowski
<m.szyprowski@...sung.com> 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):

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.

> ------------[ cut here ]------------
> WARNING: CPU: 1 PID: 1 at drivers/of/base.c:107
> of_bus_n_addr_cells+0x98/0xcc
> Missing '#address-cells' in /soc/system-controller@...20000
> Modules linked in:
> CPU: 1 UID: 0 PID: 1 Comm: swapper/0 Not tainted
> 6.12.0-rc6-next-20241108 #9360
> Hardware name: Samsung Exynos (Flattened Device Tree)
> Call trace:
>   unwind_backtrace from show_stack+0x10/0x14
>   show_stack from dump_stack_lvl+0x68/0x88
>   dump_stack_lvl from __warn+0x150/0x1dc
>   __warn from warn_slowpath_fmt+0x128/0x1b0
>   warn_slowpath_fmt from of_bus_n_addr_cells+0x98/0xcc
>   of_bus_n_addr_cells from of_bus_default_flags_match+0x8/0x18
>   of_bus_default_flags_match from of_match_bus+0x28/0x58
>   of_match_bus from __of_get_address+0x3c/0x1c8
>   __of_get_address from __of_address_to_resource.constprop.2+0x3c/0x1e8
>   __of_address_to_resource.constprop.2 from of_device_alloc+0x54/0x164
>   of_device_alloc from of_platform_device_create_pdata+0x60/0xfc
>   of_platform_device_create_pdata from of_platform_bus_create+0x1b0/0x4dc
>   of_platform_bus_create from of_platform_populate+0x80/0x114
>   of_platform_populate from devm_of_platform_populate+0x50/0x98
>   devm_of_platform_populate from exynos_pmu_probe+0x12c/0x284
>   exynos_pmu_probe from platform_probe+0x80/0xc0
>   platform_probe from really_probe+0x154/0x3ac
>   really_probe from __driver_probe_device+0xa0/0x1d4
>   __driver_probe_device from driver_probe_device+0x30/0xd0
>   driver_probe_device from __device_attach_driver+0xbc/0x11c
>   __device_attach_driver from bus_for_each_drv+0x74/0xc0
>   bus_for_each_drv from __device_attach+0xec/0x1b4
>   __device_attach from bus_probe_device+0x8c/0x90
>   bus_probe_device from device_add+0x56c/0x77c
>   device_add from of_platform_device_create_pdata+0xac/0xfc
>   of_platform_device_create_pdata from of_platform_bus_create+0x1b0/0x4dc
>   of_platform_bus_create from of_platform_bus_create+0x218/0x4dc
>   of_platform_bus_create from of_platform_populate+0x80/0x114
>   of_platform_populate from of_platform_default_populate_init+0xc0/0xd0
>   of_platform_default_populate_init from do_one_initcall+0x6c/0x328
>   do_one_initcall from kernel_init_freeable+0x1c8/0x218
>   kernel_init_freeable from kernel_init+0x1c/0x12c
>   kernel_init from ret_from_fork+0x14/0x28
> Exception stack(0xe0045fb0 to 0xe0045ff8)
> ...
> ---[ end trace 0000000000000000 ]---
>
> To silence the above warning and the rest of them on this board I had to
> do the following change:
>
> diff --git a/arch/arm/boot/dts/samsung/exynos3250.dtsi
> b/arch/arm/boot/dts/samsung/exynos3250.dtsi
> index b6c3826a9424..c09afbcd1cab 100644
> --- a/arch/arm/boot/dts/samsung/exynos3250.dtsi
> +++ b/arch/arm/boot/dts/samsung/exynos3250.dtsi
> @@ -347,6 +347,8 @@ pmu_system_controller: system-controller@...20000 {
>                          reg = <0x10020000 0x4000>;
>                          interrupt-controller;
>                          #interrupt-cells = <3>;
> +                       #size-cells = <0>;
> +                       #address-cells = <0>;
>                          interrupt-parent = <&gic>;
>                          clock-names = "clkout8";
>                          clocks = <&cmu CLK_FIN_PLL>;
> @@ -615,6 +617,8 @@ pdma1: dma-controller@...90000 {
>                  adc: adc@...c0000 {
>                          compatible = "samsung,exynos3250-adc";
>                          reg = <0x126c0000 0x100>;
> +                       #size-cells = <0>;
> +                       #address-cells = <0>;
>                          interrupts = <GIC_SPI 137 IRQ_TYPE_LEVEL_HIGH>;
>                          clock-names = "adc", "sclk";
>                          clocks = <&cmu CLK_TSADC>, <&cmu CLK_SCLK_TSADC>;
>
> I'm not a device tree expert, but for me this size/address cells
> requirement for nodes, which don't have addressable children looks a bit
> excessive. I bet, that if it was really needed from specification point
> of view, Krzysztof would already add it to Samsung Exynos dtsi/dts, as
> he spent quite a lot of time making them conformant with the spec.
> Krzysztof, could you comment on this?

No, you shouldn't need them and that's in fact a warning if you do.

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

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]);

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ