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

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

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

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.


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

>  }
>  WARNING(avoid_unnecessary_addr_size, check_avoid_unnecessary_addr_size, NULL, &avoid_default_addr_size);
>  

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