[PATCH] of: WARN on using default root node #address-cells/#size-cells

[PATCH] of: WARN on using default root node #address-cells/#size-cells

[Rob Herring (Arm)]

While OpenFirmware originally allowed default values of #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 no idea what exists for Sparc, so disabling the warning for it.
If any other platforms hit the warning, then the warning can be
disabled for them.

Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
Sparc folks, If anyone can dump DTs from some Sparc systems it would be
helpful.
---
 drivers/of/base.c | 2 ++
 drivers/of/fdt.c  | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 61fff13bbee5..6930aa29fec1 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -96,6 +96,7 @@ int of_bus_n_addr_cells(struct device_node *np)
 			return cells;
 
 	/* No #address-cells property for the root node */
+	WARN_ONCE(!IS_ENABLED(CONFIG_SPARC), "Only listed platforms should rely on default '#address-cells'\n");
 	return OF_ROOT_NODE_ADDR_CELLS_DEFAULT;
 }
 
@@ -116,6 +117,7 @@ int of_bus_n_size_cells(struct device_node *np)
 			return cells;
 
 	/* No #size-cells property for the root node */
+	WARN_ONCE(!IS_ENABLED(CONFIG_SPARC), "Only listed platforms should rely on default '#size-cells'\n");
 	return OF_ROOT_NODE_SIZE_CELLS_DEFAULT;
 }
 
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index a8a04f27915b..568a3fca4c27 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -961,11 +961,13 @@ int __init early_init_dt_scan_root(void)
 	prop = of_get_flat_dt_prop(node, "#size-cells", NULL);
 	if (prop)
 		dt_root_size_cells = be32_to_cpup(prop);
+	WARN(!prop, "No '#size-cells' in root node\n");
 	pr_debug("dt_root_size_cells = %x\n", dt_root_size_cells);
 
 	prop = of_get_flat_dt_prop(node, "#address-cells", NULL);
 	if (prop)
 		dt_root_addr_cells = be32_to_cpup(prop);
+	WARN(!prop, "No '#address-cells' in root node\n");
 	pr_debug("dt_root_addr_cells = %x\n", dt_root_addr_cells);
 
 	return 0;
-- 
2.43.0

Re: [PATCH] of: WARN on using default root node #address-cells/#size-cells

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1562 bytes --]

On Thu, May 30, 2024 at 01:50:48PM -0500, Rob Herring (Arm) wrote:
> While OpenFirmware originally allowed default values of #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 no idea what exists for Sparc, so disabling the warning for it.
> If any other platforms hit the warning, then the warning can be
> disabled for them.
> 
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
> Sparc folks, If anyone can dump DTs from some Sparc systems it would be
> helpful.
> ---
>  drivers/of/base.c | 2 ++
>  drivers/of/fdt.c  | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 61fff13bbee5..6930aa29fec1 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -96,6 +96,7 @@ int of_bus_n_addr_cells(struct device_node *np)
>  			return cells;
>  
>  	/* No #address-cells property for the root node */
> +	WARN_ONCE(!IS_ENABLED(CONFIG_SPARC), "Only listed platforms should rely on default '#address-cells'\n");

I assume "listed platforms" means things in the first parameter of
WARN_ONCE()? Since that's only SPARC, why not just say it? The error
message is rather obtuse as-is I think.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] of: WARN on using default root node #address-cells/#size-cells

[Sam Ravnborg]

Hi Rob.

On Thu, May 30, 2024 at 01:50:48PM -0500, Rob Herring (Arm) wrote:
> While OpenFirmware originally allowed default values of #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 no idea what exists for Sparc, so disabling the warning for it.
> If any other platforms hit the warning, then the warning can be
> disabled for them.
> 
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
> Sparc folks, If anyone can dump DTs from some Sparc systems it would be
> helpful.

For sparc the format looks much different - see:
git://git.kernel.org/pub/scm/linux/kernel/git/davem/prtconfs.git

This is dumps from the prtconf tool found in Solaris.

Looking at for example t1000 it looks like #size-cells, #address-cells
are used properly.

Looking at the older ss20 I see no use of these.
Looking at sb100 (old sparc64 machine) I see inconsistent use.

My best guess is that sparc32 machines see little to no use of them.
sparc64 use them, but on older machines the usage is inconsistent.

	Sam

Re: [PATCH] of: WARN on using default root node #address-cells/#size-cells

[Rob Herring]

On Thu, May 30, 2024 at 2:21 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Thu, May 30, 2024 at 01:50:48PM -0500, Rob Herring (Arm) wrote:
> > While OpenFirmware originally allowed default values of #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 no idea what exists for Sparc, so disabling the warning for it.
> > If any other platforms hit the warning, then the warning can be
> > disabled for them.
> >
> > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> > ---
> > Sparc folks, If anyone can dump DTs from some Sparc systems it would be
> > helpful.
> > ---
> >  drivers/of/base.c | 2 ++
> >  drivers/of/fdt.c  | 2 ++
> >  2 files changed, 4 insertions(+)
> >
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index 61fff13bbee5..6930aa29fec1 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -96,6 +96,7 @@ int of_bus_n_addr_cells(struct device_node *np)
> >                       return cells;
> >
> >       /* No #address-cells property for the root node */
> > +     WARN_ONCE(!IS_ENABLED(CONFIG_SPARC), "Only listed platforms should rely on default '#address-cells'\n");
>
> I assume "listed platforms" means things in the first parameter of
> WARN_ONCE()? Since that's only SPARC, why not just say it? The error
> message is rather obtuse as-is I think.

My intent is if you hit this warning, add the platform here. I imagine
it will be older stuff we can't or don't want to fix. Maybe I should
just say that as a comment instead.

Rob

Re: [PATCH] of: WARN on using default root node #address-cells/#size-cells

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2259 bytes --]

On Thu, May 30, 2024 at 07:33:57PM -0500, Rob Herring wrote:
> On Thu, May 30, 2024 at 2:21 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Thu, May 30, 2024 at 01:50:48PM -0500, Rob Herring (Arm) wrote:
> > > While OpenFirmware originally allowed default values of #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 no idea what exists for Sparc, so disabling the warning for it.
> > > If any other platforms hit the warning, then the warning can be
> > > disabled for them.
> > >
> > > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> > > ---
> > > Sparc folks, If anyone can dump DTs from some Sparc systems it would be
> > > helpful.
> > > ---
> > >  drivers/of/base.c | 2 ++
> > >  drivers/of/fdt.c  | 2 ++
> > >  2 files changed, 4 insertions(+)
> > >
> > > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > > index 61fff13bbee5..6930aa29fec1 100644
> > > --- a/drivers/of/base.c
> > > +++ b/drivers/of/base.c
> > > @@ -96,6 +96,7 @@ int of_bus_n_addr_cells(struct device_node *np)
> > >                       return cells;
> > >
> > >       /* No #address-cells property for the root node */
> > > +     WARN_ONCE(!IS_ENABLED(CONFIG_SPARC), "Only listed platforms should rely on default '#address-cells'\n");
> >
> > I assume "listed platforms" means things in the first parameter of
> > WARN_ONCE()? Since that's only SPARC, why not just say it? The error
> > message is rather obtuse as-is I think.
> 
> My intent is if you hit this warning, add the platform here.

Aye, I figured as much. My point was mostly that if you see this warning
during boot etc the message doesn't make that much sense. It only really
makes sense when you look at the kernel sources.

> I imagine
> it will be older stuff we can't or don't want to fix. Maybe I should
> just say that as a comment instead.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]