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: <3706d174-fadd-485f-be7b-f7ef4b11cf84@samsung.com>
Date: Fri, 8 Nov 2024 12:04:41 +0100
From: Marek Szyprowski <m.szyprowski@...sung.com>
To: "Rob Herring (Arm)" <robh@...nel.org>, Saravana Kannan
	<saravanak@...gle.com>, Krzysztof Kozlowski <krzk@...nel.org>
Cc: 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 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):

------------[ 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?


> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 20603d3c9931..39fb59b666f3 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -87,15 +87,25 @@ static bool __of_node_is_type(const struct device_node *np, const char *type)
>   	return np && match && type && !strcmp(match, type);
>   }
>   
> +#define EXCLUDED_DEFAULT_CELLS_PLATFORMS ( \
> +	IS_ENABLED(CONFIG_SPARC) \
> +)
> +
>   int of_bus_n_addr_cells(struct device_node *np)
>   {
>   	u32 cells;
>   
> -	for (; np; np = np->parent)
> +	for (; np; np = np->parent) {
>   		if (!of_property_read_u32(np, "#address-cells", &cells))
>   			return cells;
> -
> -	/* No #address-cells property for the root node */
> +		/*
> +		 * Default root value and walking parent nodes for "#address-cells"
> +		 * is deprecated. Any platforms which hit this warning should
> +		 * be added to the excluded list.
> +		 */
> +		WARN_ONCE(!EXCLUDED_DEFAULT_CELLS_PLATFORMS,
> +			  "Missing '#address-cells' in %pOF\n", np);
> +	}
>   	return OF_ROOT_NODE_ADDR_CELLS_DEFAULT;
>   }
>   
> @@ -112,11 +122,17 @@ int of_bus_n_size_cells(struct device_node *np)
>   {
>   	u32 cells;
>   
> -	for (; np; np = np->parent)
> +	for (; np; np = np->parent) {
>   		if (!of_property_read_u32(np, "#size-cells", &cells))
>   			return cells;
> -
> -	/* No #size-cells property for the root node */
> +		/*
> +		 * Default root value and walking parent nodes for "#size-cells"
> +		 * is deprecated. Any platforms which hit this warning should
> +		 * be added to the excluded list.
> +		 */
> +		WARN_ONCE(!EXCLUDED_DEFAULT_CELLS_PLATFORMS,
> +			  "Missing '#size-cells' in %pOF\n", np);
> +	}
>   	return OF_ROOT_NODE_SIZE_CELLS_DEFAULT;
>   }
>   
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 4d528c10df3a..d79707fb2eb1 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -938,12 +938,12 @@ int __init early_init_dt_scan_root(void)
>   	dt_root_addr_cells = OF_ROOT_NODE_ADDR_CELLS_DEFAULT;
>   
>   	prop = of_get_flat_dt_prop(node, "#size-cells", NULL);
> -	if (prop)
> +	if (!WARN(!prop, "No '#size-cells' in root node\n"))
>   		dt_root_size_cells = be32_to_cpup(prop);
>   	pr_debug("dt_root_size_cells = %x\n", dt_root_size_cells);
>   
>   	prop = of_get_flat_dt_prop(node, "#address-cells", NULL);
> -	if (prop)
> +	if (!WARN(!prop, "No '#address-cells' in root node\n"))
>   		dt_root_addr_cells = be32_to_cpup(prop);
>   	pr_debug("dt_root_addr_cells = %x\n", dt_root_addr_cells);
>   

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ