| Message ID | 20241106171028.3830266-1-robh@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [v2] of: WARN on deprecated #address-cells/#size-cells handling | expand |
"Rob Herring (Arm)" <robh@kernel.org> writes: > 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. I have various old device trees that have been given to me over the years, and as far as I can tell they all have these properties (some of them are partial trees so it's hard to be 100% sure). So LGTM. cheers
On Thu, Nov 7, 2024 at 12:37 PM Michael Ellerman <mpe@ellerman.id.au> wrote: > "Rob Herring (Arm)" <robh@kernel.org> writes: > > 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. > > I have various old device trees that have been given to me over the > years, and as far as I can tell they all have these properties (some of > them are partial trees so it's hard to be 100% sure). Apparently CHRP LongTrail only had #address-cells in the root node. Interestingly, /cpus does have a (zero) @size-cells property. http://g33rt.be/migrated/Linux/PPC/root.html http://g33rt.be/migrated/Linux/PPC/DeviceTree.html No idea if any of them are still alive. > So LGTM. Indeed. Gr{oetje,eeting}s, Geert
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@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): ------------[ 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@10020000 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@10020000 { 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@12690000 { adc: adc@126c0000 { 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
On Fri, Nov 8, 2024 at 5:04 AM Marek Szyprowski <m.szyprowski@samsung.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@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. > ------------[ 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@10020000 > 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@10020000 { > 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@12690000 { > adc: adc@126c0000 { > 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]);
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@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): 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@ff730000 I'm sure it's easy to fix up the DTB, but we shouldn't be breaking long existing DTBs. Steve
On Fri, Nov 8, 2024 at 7:26 AM Steven Price <steven.price@arm.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@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): > > 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@ff730000 > > I'm sure it's easy to fix up the DTB, but we shouldn't be breaking long existing DTBs. What broke? 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@ff730000". 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"); But Conor thought that wasn't clear. So I'm open to suggestions... Rob
On 08/11/2024 14:04, Rob Herring wrote: > On Fri, Nov 8, 2024 at 7:26 AM Steven Price <steven.price@arm.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@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): >> >> 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@ff730000 >> >> 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@ff730000". 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. 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. You could also downgrade the warning to a pr_warn() or similar. > But Conor thought that wasn't clear. So I'm open to suggestions... I don't have any particular suggestions other than above, I just wanted to report an existing DTB that triggers this WARN. We need to resolve this one way or another before this patch can progress. For now I've simply reverted this patch for my CI. Steve
On Fri, Nov 8, 2024 at 8:33 AM Steven Price <steven.price@arm.com> wrote: > > On 08/11/2024 14:04, Rob Herring wrote: > > On Fri, Nov 8, 2024 at 7:26 AM Steven Price <steven.price@arm.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@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): > >> > >> 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@ff730000 > >> > >> 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@ff730000". 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. > 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
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
On 08/11/2024 14:58, Rob Herring wrote: > On Fri, Nov 8, 2024 at 8:33 AM Steven Price <steven.price@arm.com> wrote: >> >> On 08/11/2024 14:04, Rob Herring wrote: >>> On Fri, Nov 8, 2024 at 7:26 AM Steven Price <steven.price@arm.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@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): >>>> >>>> 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@ff730000 >>>> >>>> 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@ff730000". 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
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);
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(-)