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

[Mario Six]

Hi Simon,

On Tue, May 1, 2018 at 1:13 AM, Simon Glass <sjg at chromium.org> wrote:
> 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.
>

Right, just the pointer in the property itself is allocated; just assigning the
pointer to the property value with the one passed in might lead to it being
deallocated.

Unfortunately a "free and malloc" or "realloc" won't work, since the unflatten
procedure in lib/of_live.c allocates the memory for the whole device tree as
one chunk, which cannot be partially freed. So the only choice would either be
only a "malloc" without prior freeing (which would leak memory if the property
is set multiple times), or require that the property value passed in is valid
forever (i.e. the caller has to malloc it himself), which would make the
interface more complicated to use, and also pushes the responsibility of
freeing the value onto the caller, who probably cannot safely decide when to
free it anyway.

Another idea would be to find out the size of the unflattened device tree; that
way one could decide whether the property value pointer points into the
allocated memory for the tree (in that case, just allocate a new property), or
if it points into a different memory region, which would indicate that the
property was changed before already (in that case, free and allocate new
property or reallocate). I don't know how complicated that would be, though.
I'll investigate.

>> +       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.
>

OK, will be documented in v3.

>> + * @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
>

OK, will be documented in v3.

>> + * @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
>

Best regards,
Mario