| 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
Geert Uytterhoeven <geert@linux-m68k.org> writes: > 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. OK. We could fix that up in prom_init() if necessary - there's already a bunch of workarounds in there for longtrail. cheers
On Thu, Nov 07, 2024 at 10:35:58PM +1100, Michael Ellerman 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). Many SUN systems won't have such superfluous properties. But does anyone use such systems at all anymore, and do people use dtc with those :-) Segher
On Thu, Nov 14, 2024 at 6:59 AM Segher Boessenkool <segher@kernel.crashing.org> wrote: > > On Thu, Nov 07, 2024 at 10:35:58PM +1100, Michael Ellerman 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). > > Many SUN systems won't have such superfluous properties. But does > anyone use such systems at all anymore, and do people use dtc with > those :-) There's still a few presumably. Sparc is omitted from this warning already because I suspected a problem which was confirmed on v1 thanks to the DT dumps here[1]. Rob [1] https://git.kernel.org/pub/scm/linux/kernel/git/davem/prtconfs.git/
Michael Ellerman <mpe@ellerman.id.au> writes: > "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. Turns out I was wrong. The warning about #size-cells hits on some powermacs, possible fixup patch here: https://lore.kernel.org/linuxppc-dev/20241126025710.591683-1-mpe@ellerman.id.au/ cheers
On Tue, Nov 26, 2024 at 02:36:32PM +1100, Michael Ellerman wrote: > Michael Ellerman <mpe@ellerman.id.au> writes: > > "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. > > Turns out I was wrong. > > The warning about #size-cells hits on some powermacs, possible fixup > patch here: > > https://lore.kernel.org/linuxppc-dev/20241126025710.591683-1-mpe@ellerman.id.au/ The Open Firmware specification is extremely clear that a "missing" "#size-cells" property means this bus has the default value of 1. https://www.openfirmware.info/data/docs/of1275.pdf (page 186). DTC or FDT might want to do things differently, but expecting decades older stuff to conform to its ill-conceived unnecessarily super wordy stuff is, well, not a plan that is likely to work very well :-) Segher
On Wed, Nov 27, 2024 at 3:47 PM Segher Boessenkool <segher@kernel.crashing.org> wrote: > > On Tue, Nov 26, 2024 at 02:36:32PM +1100, Michael Ellerman wrote: > > Michael Ellerman <mpe@ellerman.id.au> writes: > > > "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. > > > > Turns out I was wrong. > > > > The warning about #size-cells hits on some powermacs, possible fixup > > patch here: > > > > https://lore.kernel.org/linuxppc-dev/20241126025710.591683-1-mpe@ellerman.id.au/ > > The Open Firmware specification is extremely clear that a "missing" > "#size-cells" property means this bus has the default value of 1. And the default for #address-cells is 2, but yet every architecture except Sparc has that wrong. If I have a node without #size-cells, is the default of 1 used or do we check parent nodes? My read of the spec would be the former, but the kernel does the latter. > https://www.openfirmware.info/data/docs/of1275.pdf (page 186). > > DTC or FDT might want to do things differently, but expecting decades > older stuff to conform to its ill-conceived unnecessarily super wordy > stuff is, well, not a plan that is likely to work very well :-) That is not the intention. The intention is to identify what doesn't conform and exclude those systems from this check (or apply a fixup if that works). Rob
On Mon, Dec 02, 2024 at 08:18:22AM -0600, Rob Herring wrote: > On Wed, Nov 27, 2024 at 3:47 PM Segher Boessenkool > <segher@kernel.crashing.org> wrote: > > On Tue, Nov 26, 2024 at 02:36:32PM +1100, Michael Ellerman wrote: > > > Michael Ellerman <mpe@ellerman.id.au> writes: > > > > "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. > > > > > > Turns out I was wrong. > > > > > > The warning about #size-cells hits on some powermacs, possible fixup > > > patch here: > > > > > > https://lore.kernel.org/linuxppc-dev/20241126025710.591683-1-mpe@ellerman.id.au/ > > > > The Open Firmware specification is extremely clear that a "missing" > > "#size-cells" property means this bus has the default value of 1. > > And the default for #address-cells is 2, but yet every architecture > except Sparc has that wrong. ? Almost all architectures (that run Linux) use 64-bit addressing, both 32-bit and 64-bit architectures. > If I have a node without #size-cells, is the default of 1 used or do > we check parent nodes? My read of the spec would be the former, but > the kernel does the latter. The former is correct. The latter makes no sense at all! The whole point of the "bus" abstraction is that you get a new addressing domain there. Yes, these days you numerically find it most often with PCI sub-domains, but those are boring. In most cases you *do* have different adressing on your child busses, and even if the addressing is the same, addresses on the child bus are not normally a subset of those on the parent bus. > > https://www.openfirmware.info/data/docs/of1275.pdf (page 186). > > > > DTC or FDT might want to do things differently, but expecting decades > > older stuff to conform to its ill-conceived unnecessarily super wordy > > stuff is, well, not a plan that is likely to work very well :-) > > That is not the intention. The intention is to identify what doesn't > conform and exclude those systems from this check (or apply a fixup if > that works). So *always* use the OF definition, at least on OF systems? Where everything is meant to conform, but conform to OF, not conform to this "OF-like-but-very-different-in-crucial-spots" thing :-) Segher
On Mon, Dec 2, 2024 at 4:09 PM Segher Boessenkool <segher@kernel.crashing.org> wrote: > > On Mon, Dec 02, 2024 at 08:18:22AM -0600, Rob Herring wrote: > > On Wed, Nov 27, 2024 at 3:47 PM Segher Boessenkool > > <segher@kernel.crashing.org> wrote: > > > On Tue, Nov 26, 2024 at 02:36:32PM +1100, Michael Ellerman wrote: > > > > Michael Ellerman <mpe@ellerman.id.au> writes: > > > > > "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. > > > > > > > > Turns out I was wrong. > > > > > > > > The warning about #size-cells hits on some powermacs, possible fixup > > > > patch here: > > > > > > > > https://lore.kernel.org/linuxppc-dev/20241126025710.591683-1-mpe@ellerman.id.au/ > > > > > > The Open Firmware specification is extremely clear that a "missing" > > > "#size-cells" property means this bus has the default value of 1. > > > > And the default for #address-cells is 2, but yet every architecture > > except Sparc has that wrong. > > ? > > Almost all architectures (that run Linux) use 64-bit addressing, both > 32-bit and 64-bit architectures. I'm just telling you what Linux uses for defaults for at least 20 years. > > If I have a node without #size-cells, is the default of 1 used or do > > we check parent nodes? My read of the spec would be the former, but > > the kernel does the latter. > > The former is correct. The latter makes no sense at all! The whole > point of the "bus" abstraction is that you get a new addressing domain > there. I agree, but that's what the kernel does (again, for 20+ years). Walking the parents is really what I want to get rid of here. My choices were drop that behavior and see who I break, or add a warning and see who notices. I went the nicer route of a warning. > Yes, these days you numerically find it most often with PCI sub-domains, > but those are boring. In most cases you *do* have different adressing > on your child busses, and even if the addressing is the same, addresses > on the child bus are not normally a subset of those on the parent bus. > > > > https://www.openfirmware.info/data/docs/of1275.pdf (page 186). > > > > > > DTC or FDT might want to do things differently, but expecting decades > > > older stuff to conform to its ill-conceived unnecessarily super wordy > > > stuff is, well, not a plan that is likely to work very well :-) > > > > That is not the intention. The intention is to identify what doesn't > > conform and exclude those systems from this check (or apply a fixup if > > that works). > > So *always* use the OF definition, at least on OF systems? Where > everything is meant to conform, but conform to OF, not conform to this > "OF-like-but-very-different-in-crucial-spots" thing :-) I'm pretty sure there are OF systems that don't conform, so it is not that simple. There's this comment in of_irq_parse_raw() for example: /* Look for this #address-cells. We have to implement the old linux * trick of looking for the parent here as some device-trees rely on it */ Maybe that's from some system long dropped and we don't need it anymore. I have no idea. That's what I'm trying to find out with this patch. We also don't really have a way to distinguish OF from FDT (where we'd need to). It is somewhat just by arch, but PPC always passes an FDT to the kernel for both FDT and OF systems. Rob
On Mon, 2024-12-02 at 16:55 -0600, Rob Herring wrote: > /* Look for this #address-cells. We have to implement the old linux > * trick of looking for the parent here as some device-trees rely on it > */ > > Maybe that's from some system long dropped and we don't need it > anymore. I have no idea. That's what I'm trying to find out with this > patch. > > We also don't really have a way to distinguish OF from FDT (where we'd > need to). It is somewhat just by arch, but PPC always passes an FDT to > the kernel for both FDT and OF systems. I probably wrote that :-) The little I can remember, I think is there are many cases of old DTs (esp. old Apple ones) missing the "empty" ranges property in some nodes to indicate that the parent and child address spaces are identical. I honestly don't remember the specifics, but this is something I did hit a few times in the past. Cheers, Ben.
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(-)