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

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

[Niklas Söderlund]

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.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
Hello,

This was previously part of a patch posted to devicetree@vger.kernel.org
[1], but as Rob's points out in that thread it should have been posted
separately to devicetree-compiler@vger.kernel.org. Sorry for not
realising that dtc changes go to thru a separate tree.

1.  https://lore.kernel.org/all/20250702085008.689727-1-niklas.soderlund%2Brenesas@ragnatech.se/
---
 checks.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/checks.c b/checks.c
index 123f2eb425f4..52d09fcf8d3a 100644
--- a/checks.c
+++ b/checks.c
@@ -1913,6 +1913,11 @@ static void check_graph_child_address(struct check *c, struct dt_info *dti,
 		cnt++;
 	}
 
+	/*
+	 * This check can produce false warnings if the bindings allow for more
+	 * then one endpoint in the node but only one is present and it has a
+	 * unit address of zero.
+	 */
 	if (cnt == 1 && node->addr_cells != -1)
 		FAIL(c, dti, node, "graph node has single child node '%s', #address-cells/#size-cells are not necessary",
 		     node->children->name);
-- 
2.50.0

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

[David Gibson]

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

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.

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).

> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
> Hello,
> 
> This was previously part of a patch posted to devicetree@vger.kernel.org
> [1], but as Rob's points out in that thread it should have been posted
> separately to devicetree-compiler@vger.kernel.org. Sorry for not
> realising that dtc changes go to thru a separate tree.
> 
> 1.  https://lore.kernel.org/all/20250702085008.689727-1-niklas.soderlund%2Brenesas@ragnatech.se/
> ---
>  checks.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/checks.c b/checks.c
> index 123f2eb425f4..52d09fcf8d3a 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -1913,6 +1913,11 @@ static void check_graph_child_address(struct check *c, struct dt_info *dti,
>  		cnt++;
>  	}
>  
> +	/*
> +	 * This check can produce false warnings if the bindings allow for more
> +	 * then one endpoint in the node but only one is present and it has a
> +	 * unit address of zero.
> +	 */
>  	if (cnt == 1 && node->addr_cells != -1)
>  		FAIL(c, dti, node, "graph node has single child node '%s', #address-cells/#size-cells are not necessary",
>  		     node->children->name);

-- 
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: Document possible false warning for graph child addresses

[Niklas Söderlund]

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 :-)

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

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

> 
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> > Hello,
> > 
> > This was previously part of a patch posted to devicetree@vger.kernel.org
> > [1], but as Rob's points out in that thread it should have been posted
> > separately to devicetree-compiler@vger.kernel.org. Sorry for not
> > realising that dtc changes go to thru a separate tree.
> > 
> > 1.  https://lore.kernel.org/all/20250702085008.689727-1-niklas.soderlund%2Brenesas@ragnatech.se/
> > ---
> >  checks.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/checks.c b/checks.c
> > index 123f2eb425f4..52d09fcf8d3a 100644
> > --- a/checks.c
> > +++ b/checks.c
> > @@ -1913,6 +1913,11 @@ static void check_graph_child_address(struct check *c, struct dt_info *dti,
> >  		cnt++;
> >  	}
> >  
> > +	/*
> > +	 * This check can produce false warnings if the bindings allow for more
> > +	 * then one endpoint in the node but only one is present and it has a
> > +	 * unit address of zero.
> > +	 */
> >  	if (cnt == 1 && node->addr_cells != -1)
> >  		FAIL(c, dti, node, "graph node has single child node '%s', #address-cells/#size-cells are not necessary",
> >  		     node->children->name);
> 
> -- 
> 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



-- 
Kind Regards,
Niklas Söderlund

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

[Rob Herring]

On Tue, Jul 8, 2025 at 2:52 AM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> 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 :-)
>
> I think if the check stays the comment bring some value.
>
> >
> > 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.

I'm okay with removing it. It's somewhat redundant now with the schemas.

Another option though would be making it default off.

Rob

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

[David Gibson]

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

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

[Niklas Söderlund]

Hi David,

Thanks for your feedback.

On 2025-07-18 16:35:56 +1000, David Gibson wrote:
> 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.

As Rob came to the same conclusion I posted a patch for that to dtc [1].  
When two people more involved then me suggest the same thing, it's 
likely the better way forward ;-)

1. 20250708161547.149599-1-niklas.soderlund+renesas@ragnatech.se

-- 
Kind Regards,
Niklas Söderlund