[U-Boot] [PATCH v2 1/4] core: Add functions to set properties in live-tree

[Simon Glass]

Hi Mario,

On 27 April 2018 at 06:51, Mario Six <mario.six at gdsys.cc> wrote:
>
> Implement a set of functions to manipulate properties in a live device
> tree:
>
> * ofnode_set_property() to set generic properties of a node
> * ofnode_write_string() to set string properties of a node
> * ofnode_set_enabled() to either enable or disable a node
>
> Signed-off-by: Mario Six <mario.six at gdsys.cc>
>
> ---
>
> v1 -> v2:
> * Fix potential NULL pointer dereference in ofnode_write_property
>
> ---
>  drivers/core/ofnode.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/dm/ofnode.h   | 37 ++++++++++++++++++++++++++++++++
>  2 files changed, 95 insertions(+)

Reviewed-by: Simon Glass <sjg at chromium.org>

But please see below.

>
> diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
> index 5909a25f85..a55aa75e5b 100644
> --- a/drivers/core/ofnode.c
> +++ b/drivers/core/ofnode.c
> @@ -687,3 +687,61 @@ u64 ofnode_translate_address(ofnode node, const fdt32_t *in_addr)
>         else
>                 return fdt_translate_address(gd->fdt_blob, ofnode_to_offset(node), in_addr);
>  }
> +
> +#ifdef CONFIG_OF_LIVE
> +int ofnode_write_property(ofnode node, const char *propname, int len,
> +                         const void *value)
> +{
> +       const struct device_node *np = ofnode_to_np(node);
> +       struct property *pp;
> +       struct property *pp_last = NULL;
> +       struct property *new;
> +
> +       if (!np)
> +               return -EINVAL;
> +
> +       for (pp = np->properties; pp; pp = pp->next) {
> +               if (strcmp(pp->name, propname) == 0) {
> +                       /* Property exists -> change value */
> +                       pp->value = (void *)value;
> +                       pp->length = len;
> +                       return 0;
> +               }
> +               pp_last = pp;
> +       }
> +
> +       if (!pp_last)
> +               return -ENOENT;
> +
> +       /* Property does not exist -> append new property */
> +       new = malloc(sizeof(struct property));
> +
> +       new->name = strdup(propname);
> +       new->value = malloc(len);
> +       memcpy(new->value, value, len);

To me it seems odd that you allocate space for the value here, but
above (in the loop) you just assign it.

> +       new->length = len;
> +       new->next = NULL;
> +
> +       pp_last->next = new;
> +
> +       return 0;
> +}
> +
> +int ofnode_write_string(ofnode node, const char *propname, const char *value)
> +{
> +       assert(ofnode_valid(node));
> +       debug("%s: %s = %s", __func__, propname, value);
> +
> +       return ofnode_write_property(node, propname, strlen(value) + 1, value);
> +}
> +
> +int ofnode_set_enabled(ofnode node, bool value)
> +{
> +       assert(ofnode_valid(node));
> +
> +       if (value)
> +               return ofnode_write_string(node, "status", "okay");
> +       else
> +               return ofnode_write_string(node, "status", "disable");
> +}
> +#endif
> diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h
> index 0d008404f9..9224d583fc 100644
> --- a/include/dm/ofnode.h
> +++ b/include/dm/ofnode.h
> @@ -681,4 +681,41 @@ int ofnode_read_resource_byname(ofnode node, const char *name,
>   * @return the translated address; OF_BAD_ADDR on error
>   */
>  u64 ofnode_translate_address(ofnode node, const fdt32_t *in_addr);
> +
> +/**
> + * ofnode_write_property() - Set a property of a ofnode
> + *
> + * @node:      The node for whose property should be set
> + * @propname:  The name of the property to set
> + * @len:       The length of the new value of the property
> + * @value:     The new value of the property

Need to mention if this is copied or if the pointer needs to remain
valid forever.

> + * @return 0 if successful, -ve on error
> + */
> +int ofnode_write_property(ofnode node, const char *propname, int len,
> +                         const void *value);
> +
> +/**
> + * ofnode_write_string() - Set a string property of a ofnode
> + *
> + * @node:      The node for whose string property should be set
> + * @propname:  The name of the string property to set
> + * @value:     The new value of the string property

Same here

> + * @return 0 if successful, -ve on error
> + */
> +int ofnode_write_string(ofnode node, const char *propname, const char *value);
> +
> +/**
> + * ofnode_set_enabled() - Enable or disable a device tree node given by its ofnode
> + *
> + * This function effectively sets the node's "status" property to either "okay"
> + * or "disable", hence making it available for driver model initialization or
> + * not.
> + *
> + * @node:      The node to enable
> + * @value:     Flag that tells the function to either disable or enable the
> + *             node
> + * @return 0 if successful, -ve on error
> + */
> +int ofnode_set_enabled(ofnode node, bool value);
> +
>  #endif
> --
> 2.16.1
>

Regards,
Simon