[PATCH] checks: Warn about missing #address-cells for interrupt parents

[PATCH] checks: Warn about missing #address-cells for interrupt parents

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

The device tree specification (v0.4) suggests that #address-cells is
mandatory for interrupt parent nodes. If this property is missing, Linux
will default to the value of 0.

A number of device tree files rely on Linux' fallback and don't specify
an explicit #address-cells as suggested by the specification. This can
cause issues when these device trees are passed to software with a more
pedantic interpretation of the DT spec.

Add a warning when this case is detected so that device tree files can
be fixed.

Reported-by: Brad Griffis <bgriffis@nvidia.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Applying this to Linux' copy reports on the order of 1300 issues that
originate from 29 .dtsi files. If this proposal is acceptable/useful, I
volunteer to patch those up so we don't get spammed by them.
---
 checks.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/checks.c b/checks.c
index 9e6a7b6a3e97..123f2eb425f4 100644
--- a/checks.c
+++ b/checks.c
@@ -1676,6 +1676,10 @@ static void check_interrupt_map(struct check *c,
 		cellprop = get_property(provider_node, "#address-cells");
 		if (cellprop)
 			parent_cellsize += propval_cell(cellprop);
+		else
+			FAIL_PROP(c, dti, node, irq_map_prop,
+				"Missing property '#address-cells' in node %s, using 0 as fallback",
+				provider_node->fullpath);
 
 		cell += 1 + parent_cellsize;
 		if (cell > map_cells)
-- 
2.47.0

Re: [PATCH] checks: Warn about missing #address-cells for interrupt parents

[Rob Herring]

On Fri, Dec 13, 2024 at 8:14 AM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> From: Thierry Reding <treding@nvidia.com>
>
> The device tree specification (v0.4) suggests that #address-cells is
> mandatory for interrupt parent nodes. If this property is missing, Linux
> will default to the value of 0.
>
> A number of device tree files rely on Linux' fallback and don't specify
> an explicit #address-cells as suggested by the specification. This can
> cause issues when these device trees are passed to software with a more
> pedantic interpretation of the DT spec.
>
> Add a warning when this case is detected so that device tree files can
> be fixed.
>
> Reported-by: Brad Griffis <bgriffis@nvidia.com>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Applying this to Linux' copy reports on the order of 1300 issues that
> originate from 29 .dtsi files. If this proposal is acceptable/useful, I
> volunteer to patch those up so we don't get spammed by them.

I could have sworn I did a check for this, but it seems not.

> ---
>  checks.c | 4 ++++
>  1 file changed, 4 insertions(+)

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

Re: [PATCH] checks: Warn about missing #address-cells for interrupt parents

[Brad Griffis]

> From: Thierry Reding <treding@nvidia.com>
> 
> The device tree specification (v0.4) suggests that #address-cells is
> mandatory for interrupt parent nodes. If this property is missing, Linux
> will default to the value of 0.

Just to clarify, this relates to interrupt-map specifically. In that 
scenario the device tree spec requires that both the child node and 
parent node specify #address-cells and #interrupt-cells. It further 
specifies if a unit address component is not needed then it must be 
explicitly defined as zero. In other words, this does not seem to be 
just a suggestion, but more of a firm requirement.

> A number of device tree files rely on Linux' fallback and don't specify
> an explicit #address-cells as suggested by the specification. This can
> cause issues when these device trees are passed to software with a more
> pedantic interpretation of the DT spec.

The device tree spec also says that in the case where #address-cells is 
not specified that a value of 2 should be assumed. So in this context, I 
find the kernel's current practice of assuming #address-cells = <0> also 
violates the spec.

> Add a warning when this case is detected so that device tree files can
> be fixed.

I think a warning is reasonable, but perhaps we should consider making 
it an outright error. Though given the number of impacted device trees, 
perhaps that needs to be done in a couple of steps.

Re: [PATCH] checks: Warn about missing #address-cells for interrupt parents

[David Gibson]

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

On Tue, Dec 17, 2024 at 06:01:16PM -0800, Brad Griffis wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > The device tree specification (v0.4) suggests that #address-cells is
> > mandatory for interrupt parent nodes. If this property is missing, Linux
> > will default to the value of 0.
> 
> Just to clarify, this relates to interrupt-map specifically. In that
> scenario the device tree spec requires that both the child node and parent
> node specify #address-cells and #interrupt-cells. It further specifies if a
> unit address component is not needed then it must be explicitly defined as
> zero. In other words, this does not seem to be just a suggestion, but more
> of a firm requirement.
> 
> > A number of device tree files rely on Linux' fallback and don't specify
> > an explicit #address-cells as suggested by the specification. This can
> > cause issues when these device trees are passed to software with a more
> > pedantic interpretation of the DT spec.
> 
> The device tree spec also says that in the case where #address-cells is not
> specified that a value of 2 should be assumed. So in this context, I find
> the kernel's current practice of assuming #address-cells = <0> also violates
> the spec.

Possibly.  However the assumed value of 2 is generally in the context
of 'ranges' and 'reg' properties, so there's at least some argument
that the kernel doing something different in the context of interrupt
maps is acceptable.

> > Add a warning when this case is detected so that device tree files can
> > be fixed.
> 
> I think a warning is reasonable, but perhaps we should consider making it an
> outright error. Though given the number of impacted device trees, perhaps
> that needs to be done in a couple of steps.

So, the suggested implementation adds it as another part of the
check_interrupt_map() logic, which is a warning (by default).  There's
no way of different sub-tests within a single named "check" have
different priority levels.  To do that you'd need to implement this
specific test as a separate check with its own entry in the table.
That wouldn't be an unreasonable thing to do, but it would be trickier
and probably require duplicating some other logic, so I can't really
blame Thierry for doing it this way.

Note that you can switch the interrupt map check as a whole to be a
full error with the command line option -Einterrupt_map

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

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

Re: [PATCH] checks: Warn about missing #address-cells for interrupt parents

[David Gibson]

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

On Mon, Dec 16, 2024 at 01:37:01PM -0600, Rob Herring wrote:
> On Fri, Dec 13, 2024 at 8:14 AM Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > From: Thierry Reding <treding@nvidia.com>
> >
> > The device tree specification (v0.4) suggests that #address-cells is
> > mandatory for interrupt parent nodes. If this property is missing, Linux
> > will default to the value of 0.
> >
> > A number of device tree files rely on Linux' fallback and don't specify
> > an explicit #address-cells as suggested by the specification. This can
> > cause issues when these device trees are passed to software with a more
> > pedantic interpretation of the DT spec.
> >
> > Add a warning when this case is detected so that device tree files can
> > be fixed.
> >
> > Reported-by: Brad Griffis <bgriffis@nvidia.com>
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > Applying this to Linux' copy reports on the order of 1300 issues that
> > originate from 29 .dtsi files. If this proposal is acceptable/useful, I
> > volunteer to patch those up so we don't get spammed by them.
> 
> I could have sworn I did a check for this, but it seems not.
> 
> > ---
> >  checks.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> 
> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

Makes sense to me.  Applied.

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

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

Re: [PATCH] checks: Warn about missing #address-cells for interrupt parents

[Rob Herring]

On Tue, Dec 17, 2024 at 8:01 PM Brad Griffis <bgriffis@nvidia.com> wrote:
>
> > From: Thierry Reding <treding@nvidia.com>
> >
> > The device tree specification (v0.4) suggests that #address-cells is
> > mandatory for interrupt parent nodes. If this property is missing, Linux
> > will default to the value of 0.
>
> Just to clarify, this relates to interrupt-map specifically.

Yes. That is the only case that really cares and needs it. We used to
warn on all interrupt-controller nodes if #address-cells was missing.
But that turns out to be way too many cases to fix, so we dropped that
part.

> In that
> scenario the device tree spec requires that both the child node and
> parent node specify #address-cells and #interrupt-cells. It further
> specifies if a unit address component is not needed then it must be
> explicitly defined as zero. In other words, this does not seem to be
> just a suggestion, but more of a firm requirement.

unit-address being 0 and #address-cells being 0 are 2 different things.

> > A number of device tree files rely on Linux' fallback and don't specify
> > an explicit #address-cells as suggested by the specification. This can
> > cause issues when these device trees are passed to software with a more
> > pedantic interpretation of the DT spec.
>
> The device tree spec also says that in the case where #address-cells is
> not specified that a value of 2 should be assumed. So in this context, I
> find the kernel's current practice of assuming #address-cells = <0> also
> violates the spec.

Relying on defaults is not good practice and deprecated for longer
than we've had DT support on Arm. The default for the kernel on PPC is
1 which doesn't agree with dtc nor OpenFirmware. The kernel walking up
all parents to find #address-cells/#size-cells is not behavior defined
in any spec either.

6.13 now warns if parent nodes or defaults are used, but not in the
case of interrupt parsing.

> > Add a warning when this case is detected so that device tree files can
> > be fixed.
>
> I think a warning is reasonable, but perhaps we should consider making
> it an outright error. Though given the number of impacted device trees,
> perhaps that needs to be done in a couple of steps.

I'm more inclined to change the spec if needed. We only need
#address-cells if we have interrupt-map, so that's all we should
check.

If a given binding wants to be stricter, we can do that in its schema.

Rob

Re: [PATCH] checks: Warn about missing #address-cells for interrupt parents

[David Gibson]

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

On Wed, Dec 18, 2024 at 09:50:27AM -0600, Rob Herring wrote:
> On Tue, Dec 17, 2024 at 8:01 PM Brad Griffis <bgriffis@nvidia.com> wrote:
> >
> > > From: Thierry Reding <treding@nvidia.com>
> > >
> > > The device tree specification (v0.4) suggests that #address-cells is
> > > mandatory for interrupt parent nodes. If this property is missing, Linux
> > > will default to the value of 0.
> >
> > Just to clarify, this relates to interrupt-map specifically.
> 
> Yes. That is the only case that really cares and needs it. We used to
> warn on all interrupt-controller nodes if #address-cells was missing.
> But that turns out to be way too many cases to fix, so we dropped that
> part.
> 
> > In that
> > scenario the device tree spec requires that both the child node and
> > parent node specify #address-cells and #interrupt-cells. It further
> > specifies if a unit address component is not needed then it must be
> > explicitly defined as zero. In other words, this does not seem to be
> > just a suggestion, but more of a firm requirement.
> 
> unit-address being 0 and #address-cells being 0 are 2 different
> things.

It's poorly phrased, but I think that was meaning to say that if the
unit address is not needed then #address-cells must be explicitlyzero,
which makes sense.

> > > A number of device tree files rely on Linux' fallback and don't specify
> > > an explicit #address-cells as suggested by the specification. This can
> > > cause issues when these device trees are passed to software with a more
> > > pedantic interpretation of the DT spec.
> >
> > The device tree spec also says that in the case where #address-cells is
> > not specified that a value of 2 should be assumed. So in this context, I
> > find the kernel's current practice of assuming #address-cells = <0> also
> > violates the spec.
> 
> Relying on defaults is not good practice and deprecated for longer
> than we've had DT support on Arm. The default for the kernel on PPC is
> 1 which doesn't agree with dtc nor OpenFirmware. The kernel walking up
> all parents to find #address-cells/#size-cells is not behavior defined
> in any spec either.

Yes, although I believe that behaviour is *very* old, and copied from
/ made to work with old Apple Open Firmwares.

> 6.13 now warns if parent nodes or defaults are used, but not in the
> case of interrupt parsing.
> 
> > > Add a warning when this case is detected so that device tree files can
> > > be fixed.
> >
> > I think a warning is reasonable, but perhaps we should consider making
> > it an outright error. Though given the number of impacted device trees,
> > perhaps that needs to be done in a couple of steps.
> 
> I'm more inclined to change the spec if needed. We only need
> #address-cells if we have interrupt-map, so that's all we should
> check.
> 
> If a given binding wants to be stricter, we can do that in its schema.
> 
> Rob
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

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