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

Hi Mario,

On 27 April 2018 at 06:51, Mario Six mario.six@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@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@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