Re: [PATCH 1/3] dm: core: Check ofnode_to_offset() return value

[Marek Vasut <marek.vasut@mailbox.org>]

On 11/13/25 11:46 PM, Simon Glass wrote:

Hello Simon,

>>>> diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
>>>> index 071d998a0a5..55c2b3a10bb 100644
>>>> --- a/drivers/core/ofnode.c
>>>> +++ b/drivers/core/ofnode.c
>>>> @@ -335,7 +335,7 @@ bool ofnode_name_eq_unit(ofnode node, const char *name)
>>>>    int ofnode_read_u8(ofnode node, const char *propname, u8 *outp)
>>>>    {
>>>>           const u8 *cell;
>>>> -       int len;
>>>> +       int len, off;
>>>>
>>>>           assert(ofnode_valid(node));
>>>>           log_debug("%s: %s: ", __func__, propname);
>>>> @@ -343,8 +343,13 @@ int ofnode_read_u8(ofnode node, const char *propname, u8 *outp)
>>>>           if (ofnode_is_np(node))
>>>>                   return of_read_u8(ofnode_to_np(node), propname, outp);
>>>>
>>>> -       cell = fdt_getprop(gd->fdt_blob, ofnode_to_offset(node), propname,
>>>> -                          &len);
>>>> +       off = ofnode_to_offset(node);
>>>> +       if (off < 0) {
>>>> +               log_debug("(not valid)\n");
>>>> +               return -EINVAL;
>>>> +       }
>>>
>>> Ugh this is really horrible, sorry :-)
>>>
>>> Better to put a wrapper around it than add all these checks, extra
>>> code size, etc. Also in many cases the ofnode is known to be valid(TM)
>>> so it might just be a waste.
>>
>> How would you propose the wrapper should look like ?
>>
>> Keep in mind, not everything is fdt_getprop( ... ofnode_to_offset() ...)
>> in this patch.
> 
> I was thinking of
> 
> my_fdt_getprop(fdt, node, propname, lenp)
> {
> if (node is not valid)
>     return NULL;
> return fdt_getprop();
> }

We would need a wrapper for almost every libfdt call, which isn't really 
better.

> What does fdt_getprop() return when the offset is -1 ?
If can_assume(VALID_DTB) , then crash on NULL pointer dereference .
If not, I think FDT_END (premature end) or an error code.