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

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

On Mi, 2024-11-06 at 10:55 +1100, David Gibson wrote:
> On Mon, Nov 04, 2024 at 02:32:54PM +0100, Philipp Zabel wrote:
> > 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:
> >   */
> 
> Sure!

Done in v2.

> > 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/
> 
> Hrm... two things strike me as suspicious about that example.
> 
> 1) The __overlay__ node is the PCI bus root... which should already
> have #address-cells and #size-cells in the base tree; it's PCI so
> those values are known and standard.

dtc doesn't know about this when compiling the overlay.
Removing the #address-cells/#size-cells properties from the __overlay__
node would cause these compiler warnings:

  DTC     drivers/misc/lan966x_pci.dtbo
../drivers/misc/lan966x_pci.dtso:59.5-60.52: Warning (ranges_format): /fragment@0/__overlay__/pci-ep-bus@0:ranges: "ranges" property has invalid length (40 bytes) (parent #address-cells == 2, child #address-cells == 1, #size-cells == 1)
../drivers/misc/lan966x_pci.dtso:50.17-174.6: Warning (avoid_default_addr_size): /fragment@0/__overlay__/pci-ep-bus@0: Relying on default #address-cells value
../drivers/misc/lan966x_pci.dtso:50.17-174.6: Warning (avoid_default_addr_size): /fragment@0/__overlay__/pci-ep-bus@0: Relying on default #size-cells value

> 2) The PCI<->simple-bus bridge must surely have at least the normal
> configuration space registers, and so should have a 'reg' property as
> well as 'ranges'.

This I don't know about.

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

Done in v2.

regards
Philipp