Re: [PATCH] checks: Document possible false warning for graph child addresses

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

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

On Tue, Jul 08, 2025 at 09:51:55AM +0200, Niklas Söderlund wrote:
> Hi David,
> 
> Thanks for your comments.
> 
> On 2025-07-08 13:07:12 +1000, David Gibson wrote:
> > On Sun, Jul 06, 2025 at 02:26:38PM +0200, Niklas Söderlund wrote:
> > > The dtc graph_child_address check can't distinguish between bindings
> > > where there can only be a single endpoint, and cases where there can be
> > > multiple endpoints.
> > > 
> > > In cases where the bindings allow for multiple endpoints but only one is
> > > described false warnings about unnecessary #address-cells/#size-cells
> > > can be generated, but only if the endpoint described have an address of
> > > 0 (A), for single endpoints with a non-zero address (B) no warnings are
> > > generated.
> > > 
> > > A)
> > >     ports {
> > > 	#address-cells = <1>;
> > > 	#size-cells = <0>;
> > > 
> > > 	port@0 {
> > > 	    #address-cells = <1>;
> > > 	    #size-cells = <0>;
> > > 
> > > 	    sourceA: endpoint@0 {
> > > 		reg = <0>
> > > 	    };
> > > 	};
> > >     };
> > > 
> > > B)
> > >     ports {
> > > 	#address-cells = <1>;
> > > 	#size-cells = <0>;
> > > 
> > > 	port@0 {
> > > 	    #address-cells = <1>;
> > > 	    #size-cells = <0>;
> > > 
> > > 	    sourceB: endpoint@1 {
> > > 		reg = <1>
> > > 	    };
> > > 	};
> > >     };
> > > 
> > > Add a comment in the check to document this.
> > 
> > Hm.  I don't know the graph bindings at all well, so I'll take your
> > word for it on what's happening here.  But simply documenting this
> > within the code doesn't seem particularly useful.  Someone running dtc
> > will still see the bogus error, and they'd have a pretty long way to
> > go to find this explanation.
> 
> It would have been useful for me, I spent a lot of time questioning 
> myself on why my dts files produced warnings and where incorrect. I even 
> submitted patches to try and work around this issue before learning 
> these where false positives. A comment here would have saved me that 
> work :-)

Well, true, but you obviously had the wherewithal to track this to the
source in the first place, putting you well ahead of most people, I
think

> I think if the check stays the comment bring some value.

True, but I think we can do better.

> > Probably better to simply remove the check (and maybe comment that it
> > would be nice to check further, but we can't adequately it from a
> > valid case).
> 
> I'm OK with removing the check too. This comment was first posted 
> together with a change to demote this check to W=2 (instead of W=1) that 
> have now been posted separately [1]. I will wait for feedback on that 
> and let smarter people then me pick the best way forward.
> 
> 1.
> https://lore.kernel.org/all/20250706123243.1050718-1-niklas.soderlund%2Brenesas@ragnatech.se/

Right, that's more useful from the point of view of someone building
the kernel.  But the underlying fact here is that the check is Just
Plain Wrong - it's giving a warning on a perfectly valid situation.
It should go.

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