Re: [PATCH] dm: uclass: allow phandle < -1 with u32 type

Hi Jianqun,

On Thu, 14 Jan 2021 at 19:21, Jianqun Xu jay.xu@rock-chips.com wrote:

First, uclass try to get device node phandle by calling dev_read_u32_default, which always return the third argument def, the function codes as following:

    ofnode_read_u32_default(ofnode, *propname, u32 def)
            ofnode_read_u32(node, propname, &def);
            return def;

In this case, the 'def' has been intalized to '-1'.

Second, the phandle of fdt is generated by get_node_phandle in livetree, which is part of dtc script tool, as following:

It is confusing, but the livetree module in dtc is not actually used by U-Boot. We have our own implementation mostly lifted from Linux - e.g. see of_access.c

    get_node_phandle(struct node *root, struct node *node)
            static cell_t phandle = 1;
            if ((node->phandle != 0) && (node->phandle != -1))
                    return node->phandle;
            while (get_node_by_phandle(root, phandle))
                    phandle++;

The cell_t is defined by 'typedef uint32_t cell_t'.

So a valid fdt phandle is from 1 to -2 in uint type.

If the dev_read_u32_default return value -1 that means it is failed to read fdt and the 'def' argument still keep its intalized value.

Signed-off-by: Jianqun Xu jay.xu@rock-chips.com

drivers/core/uclass.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

I am not quite sure what the bug is here. It would be better the put the problem right at the start of the commit, before your explanation.

Valid phandles are >=1. The value 0 is an invalid phandle. Any negative value is also invalid since the values are allocated consecutively and an unsigned value it is much larger than the number of phandles in the file.

Are you able to write a sandbox test for this that shows the bug and the fix? e.g. in test/dm/core.c

diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c index c3f1b73cd6..86cc04ecac 100644 --- a/drivers/core/uclass.c +++ b/drivers/core/uclass.c @@ -385,12 +385,12 @@ int uclass_find_device_by_phandle(enum uclass_id id, struct udevice *parent, { struct udevice *dev; struct uclass *uc;

•   int find_phandle;
  

•   u32 find_phandle;
    int ret;
  
    *devp = NULL;
    find_phandle = dev_read_u32_default(parent, name, -1);
  

•   if (find_phandle <= 0)
  

•   if (find_phandle == -1)
            return -ENOENT;
    ret = uclass_get(id, &uc);
    if (ret)
  

-- 2.25.1

Regards, Simon