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

[David Gibson <david@gibson.dropbear.id.au>]

[-- 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 --]