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

[Simon Glass]

Hi Jianqun,

On Thu, 14 Jan 2021 at 19:21, Jianqun Xu <jay.xu at 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 at 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