Re: [PATCH] of: property: Fix of_fwnode_get_reference_args() with negative index

[Krzysztof Kozlowski <krzk@kernel.org>]

On 15/06/2026 11:33, Alban Bedel wrote:
> On Mon, 15 Jun 2026 08:33:32 +0200
> Krzysztof Kozlowski <krzk@kernel.org> wrote:
> 
>> On Thu, Jun 11, 2026 at 12:28:06PM +0200, Alban Bedel wrote:
>>> fwnode_property_get_reference_args() should return -ENOENT when an out
>>> of bound index is passed. An issue arised with the OF backend because
>>> the OF API use signed indexes while the fwnode API use unsigned ones.
>>> When an index value greater the INT_MAX was passed to the OF backend
>>> it got casted to a negative value and it returned -EINVAL instead of  
>>
>> INT_MAX is not out of bound for this function. It is invalid value,
>> because OF code expects signed.
> 
> But this is fwnode code, it use the OF API but it should implement the
> fwnode API which, unlike the OF API, use unsigned index.
>>> -ENOENT. This patch add a check to of_fwnode_get_reference_args() to
>>> catch negative index before they are passed to the OF API and return
>>> -ENOENT right away.  
>>
>> I do not understand why are you fixing this issue that way. For this
>> API, the INT_MAX is correct value, but you claim that it is wrong and
>> should be ENOENT (even if there is entry).
>>
>> Fine, if this is not a correct value, then EINVAL.
> 
> Indices larger than INT_MAX are valid in the fwnode API, so returning
> -EINVAL is not appropriate here.
> 

Then neither ENOENT are.

But really, EINVAL is correct here. This is OF implementation, so this
implementation decides what is EINVAL and what is right. Not fwnode API.


>> But more important I think this should be just fixed in different way -
>> why index in OF calls is signed in the first place? All indices are
>> supposed to be unsigned in general, because that is both logical and
>> readable when accessing arrays.
>  
> I agree that would be the better fix in the long run. But that would
> impact a lot more code and I found it difficult to ensure it would not
> potentially break some of its users. 

I don't see how this fixes anything. You basically replace correct
return value to a different one.

Best regards,
Krzysztof