drivers/of/base.c | 28 ++++++++++++++++++++++------ drivers/of/fdt.c | 4 ++-- 2 files changed, 24 insertions(+), 8 deletions(-)
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(-)
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);
--
2.45.2Hi 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
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
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
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
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]);
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
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
"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 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/
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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
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
© 2016 - 2024 Red Hat, Inc.