[PATCH] of/address: Add error logging for of_match_bus() in address translation path

[PATCH] of/address: Add error logging for of_match_bus() in address translation path

[Zhenhua Huang]

The change introduced in
commit 045b14ca5c36 ("of: WARN on deprecated #address-cells/#size-cells handling")
triggers a warning on the direct ancestor node when translating properties
like "iommu-addresses"/"reg". However, it fails to issue a warning if the
ancestor’s ancestor is missing the required cells.
For instance, if node_c lacks the necessary properties, no warning will be
generated. Potential issues will be trigger further.
node_c {
		//NO WARN
	node_b {
		//WARN on missing of "address-cells" and "size-cells"
		node_a {
			xxx = <memory_reion>  //contains "iommu-addresses"
		}
	}
}

Since of_match_bus() is now expected to succeed in traslation path,
routine __of_translate_address. Print an error message would help in
identifying cases where it fails, making such issues easier to diagnose.

Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com>
---
 drivers/of/address.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index f0f8f0dd191c..cd33ab64ccf3 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -515,8 +515,10 @@ static u64 __of_translate_address(struct device_node *node,
 	if (parent == NULL)
 		return OF_BAD_ADDR;
 	bus = of_match_bus(parent);
-	if (!bus)
+	if (!bus) {
+		pr_err("of_match_bus failed for device node(%pOF)\n", parent);
 		return OF_BAD_ADDR;
+	}
 
 	/* Count address cells & copy address locally */
 	bus->count_cells(dev, &na, &ns);
@@ -560,8 +562,10 @@ static u64 __of_translate_address(struct device_node *node,
 
 		/* Get new parent bus and counts */
 		pbus = of_match_bus(parent);
-		if (!pbus)
+		if (!pbus) {
+			pr_err("of_match_bus failed for device node(%pOF)\n", parent);
 			return OF_BAD_ADDR;
+		}
 		pbus->count_cells(dev, &pna, &pns);
 		if (!OF_CHECK_COUNTS(pna, pns)) {
 			pr_err("Bad cell count for %pOF\n", dev);
-- 
2.34.1

Re: [PATCH] of/address: Add error logging for of_match_bus() in address translation path

[Rob Herring]

On Mon, Aug 11, 2025 at 05:53:42PM +0800, Zhenhua Huang wrote:
> The change introduced in
> commit 045b14ca5c36 ("of: WARN on deprecated #address-cells/#size-cells handling")
> triggers a warning on the direct ancestor node when translating properties
> like "iommu-addresses"/"reg". However, it fails to issue a warning if the
> ancestor’s ancestor is missing the required cells.
> For instance, if node_c lacks the necessary properties, no warning will be
> generated. Potential issues will be trigger further.

The point of the WARN is to only to check the immediate ancestor.

> node_c {
> 		//NO WARN
> 	node_b {
> 		//WARN on missing of "address-cells" and "size-cells"
> 		node_a {
> 			xxx = <memory_reion>  //contains "iommu-addresses"
> 		}
> 	}
> }

Whether a warning is appropriate here depends on whether there's 
'ranges' properties or not. If your schemas are complete, then they 
should warn on missing 'ranges'. If ranges is present, then we should 
get warnings if #address-cells or #size-cells is missing.

> Since of_match_bus() is now expected to succeed in traslation path,

now expected? Nothing changed in that aspect.

> routine __of_translate_address. Print an error message would help in
> identifying cases where it fails, making such issues easier to diagnose.

For errors in the DT (as opposed to errors using the API), it would be 
better if we can check this at build time rather than run-time. And 
generally I think we should already, but there could be some corner case 
that we don't.

> 
> Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com>
> ---
>  drivers/of/address.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index f0f8f0dd191c..cd33ab64ccf3 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -515,8 +515,10 @@ static u64 __of_translate_address(struct device_node *node,
>  	if (parent == NULL)
>  		return OF_BAD_ADDR;
>  	bus = of_match_bus(parent);
> -	if (!bus)
> +	if (!bus) {
> +		pr_err("of_match_bus failed for device node(%pOF)\n", parent);
>  		return OF_BAD_ADDR;
> +	}
>  
>  	/* Count address cells & copy address locally */
>  	bus->count_cells(dev, &na, &ns);
> @@ -560,8 +562,10 @@ static u64 __of_translate_address(struct device_node *node,
>  
>  		/* Get new parent bus and counts */
>  		pbus = of_match_bus(parent);
> -		if (!pbus)
> +		if (!pbus) {
> +			pr_err("of_match_bus failed for device node(%pOF)\n", parent);
>  			return OF_BAD_ADDR;

If there's no case we expect of_match_bus() failing is correct 
operation, then the error msg should be in the of_match_bus() function 
rather than duplicated here. I'm not sure if there is any such case.

Rob

Re: [PATCH] of/address: Add error logging for of_match_bus() in address translation path

[Zhenhua Huang]

Hi Rob, Thanks for your review.

On 8/19/2025 12:49 AM, Rob Herring wrote:
> On Mon, Aug 11, 2025 at 05:53:42PM +0800, Zhenhua Huang wrote:
>> The change introduced in
>> commit 045b14ca5c36 ("of: WARN on deprecated #address-cells/#size-cells handling")
>> triggers a warning on the direct ancestor node when translating properties
>> like "iommu-addresses"/"reg". However, it fails to issue a warning if the
>> ancestor’s ancestor is missing the required cells.
>> For instance, if node_c lacks the necessary properties, no warning will be
>> generated. Potential issues will be trigger further.
> 
> The point of the WARN is to only to check the immediate ancestor.

Yes, that's exactly what I wanted to point out. In fact, during the 
translation phase, a warning should as well be issued when checking the 
node_c as described below, Otherwise, I noticed that the translation 
failure tends to go unnoticed in practice... which is further leading to 
other issues etc.

> 
>> node_c {
>> 		//NO WARN
>> 	node_b {
>> 		//WARN on missing of "address-cells" and "size-cells"
>> 		node_a {
>> 			xxx = <memory_reion>  //contains "iommu-addresses"
>> 		}
>> 	}
>> }
> 
> Whether a warning is appropriate here depends on whether there's
> 'ranges' properties or not. If your schemas are complete, then they
> should warn on missing 'ranges'. If ranges is present, then we should
> get warnings if #address-cells or #size-cells is missing.
> 
>> Since of_match_bus() is now expected to succeed in traslation path,
> 
> now expected? Nothing changed in that aspect.
My bad, The wording may have caused some confusion. What I intended to 
convey is that for example the of_device_alloc() path, as described 
below, of_match_bus() is not expected to always succeed.

> 
>> routine __of_translate_address. Print an error message would help in
>> identifying cases where it fails, making such issues easier to diagnose.
> 
> For errors in the DT (as opposed to errors using the API), it would be
> better if we can check this at build time rather than run-time. And
> generally I think we should already, but there could be some corner case
> that we don't.
> 
>>
>> Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com>
>> ---
>>   drivers/of/address.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/of/address.c b/drivers/of/address.c
>> index f0f8f0dd191c..cd33ab64ccf3 100644
>> --- a/drivers/of/address.c
>> +++ b/drivers/of/address.c
>> @@ -515,8 +515,10 @@ static u64 __of_translate_address(struct device_node *node,
>>   	if (parent == NULL)
>>   		return OF_BAD_ADDR;
>>   	bus = of_match_bus(parent);
>> -	if (!bus)
>> +	if (!bus) {
>> +		pr_err("of_match_bus failed for device node(%pOF)\n", parent);
>>   		return OF_BAD_ADDR;
>> +	}
>>   
>>   	/* Count address cells & copy address locally */
>>   	bus->count_cells(dev, &na, &ns);
>> @@ -560,8 +562,10 @@ static u64 __of_translate_address(struct device_node *node,
>>   
>>   		/* Get new parent bus and counts */
>>   		pbus = of_match_bus(parent);
>> -		if (!pbus)
>> +		if (!pbus) {
>> +			pr_err("of_match_bus failed for device node(%pOF)\n", parent);
>>   			return OF_BAD_ADDR;
> 
> If there's no case we expect of_match_bus() failing is correct
> operation, then the error msg should be in the of_match_bus() function
> rather than duplicated here. I'm not sure if there is any such case.
Yeah...
That’s what I initially did. However, in a case where the node doesn’t 
have a "reg" property (as with the "default" of_bus), the path below 
will call of_match_bus() and fail. In such scenarios, its failure should 
be considered expected ?
of_device_alloc
	of_address_count(np);
	..
		__of_get_address
			of_match_bus()

I moved the error checking into __of_translate_address then, limiting it 
to cases where actual address translation is being performed. Because it 
appears to be MUST successful in the scenario.

> 
> Rob