Re: [PATCH] checks: Relax avoid_unnecessary_addr_size check to allow child ranges properties

[Philipp Zabel <p.zabel@pengutronix.de>]

On Mo, 2024-10-28 at 13:42 +1100, David Gibson wrote:
> On Fri, Oct 25, 2024 at 06:13:07PM +0200, Philipp Zabel wrote:
> > Do not fail the unnecessary #address-cells/#size-cells check if any
> > children of the node have a "ranges" property.
> 
> I think this is correct, but I had to think abuot it for a while,
> because it's subtler than it looks.
> 
> If there is no 'ranges' in the node itself, then the child devices'
> address space is not mapped into the parent bus.  Of course, you can
> still establish a local address space for them that (e.g.) could be
> accessed indirectly via registers in this bridge device.
> 
> Having a child device which acts as a bridge from this local address
> to another subordinate address space, but no children with any
> registers directly on the local bus seems odd... but it is logically
> possible.
> 
> Given the subtlety, it would be pretty nice to add an explanatory
> comment about what this is check for and what some of the edge cases
> are.

Thank you. Can I steal your wording and add the following comment
inside the for_each_child() loop:

 /*
  * Even if the child devices' address space is not mapped into
  * the parent bus (no 'ranges' property on node), children can
  * still have registers on a local bus, or map local addresses
  * to another subordinate address space. The properties on the
  * child nodes then make #address-cells/#size-cells necessary:
  */

The specific reason for this patch is a PCI device tree overlay [1]
where the '#address-cells'/'#size-cells' properties are on the
__overlay__ node and a child simple-bus node contains the 'ranges'
property mapping between BAR space and the local bus.

[1] https://lore.kernel.org/all/20241014124636.24221-2-herve.codina@bootlin.com/

> > Suggested-by: Rob Herring <robh@kernel.org>
> > Link: https://lore.kernel.org/all/CAL_JsqKebRL454poAYZ9i=sCsHqGzmocLy0psQcng-79UWJB-A@mail.gmail.com/
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >  checks.c | 10 +++-------
> >  1 file changed, 3 insertions(+), 7 deletions(-)
> > 
> > diff --git a/checks.c b/checks.c
> > index 6e06aeab5503..76fdee2ed030 100644
> > --- a/checks.c
> > +++ b/checks.c
> > @@ -1217,9 +1217,7 @@ WARNING(avoid_default_addr_size, check_avoid_default_addr_size, NULL,
> >  static void check_avoid_unnecessary_addr_size(struct check *c, struct dt_info *dti,
> >  					      struct node *node)
> >  {
> > -	struct property *prop;
> >  	struct node *child;
> > -	bool has_reg = false;
> >  
> >  	if (!node->parent || node->addr_cells < 0 || node->size_cells < 0)
> >  		return;
> > @@ -1228,13 +1226,11 @@ static void check_avoid_unnecessary_addr_size(struct check *c, struct dt_info *d
> >  		return;
> >  
> >  	for_each_child(node, child) {
> > -		prop = get_property(child, "reg");
> > -		if (prop)
> > -			has_reg = true;
> > +		if (get_property(child, "reg") || get_property(child, "ranges"))
> > +			return;
> >  	}
> >  
> > -	if (!has_reg)
> > -		FAIL(c, dti, node, "unnecessary #address-cells/#size-cells without \"ranges\", \"dma-ranges\" or child \"reg\" property");
> > +	FAIL(c, dti, node, "unnecessary #address-cells/#size-cells without \"ranges\", \"dma-ranges\" or child \"reg\" property");
> 
> ..also this message needs updating to reference child "ranges" as well.

I'll change it to:

  "unnecessary #address-cells/#size-cells without \"ranges\", \"dma-ranges\" or child \"reg\" or \"ranges\" property"


regards
Philipp