Hi Mario, On 4 May 2018 at 01:14, Mario Six <mario....@gdsys.cc> wrote: > Hi Simon, > > On Tue, May 1, 2018 at 1:13 AM, Simon Glass <s...@chromium.org> wrote: >> Hi Mario, >> >> On 27 April 2018 at 06:51, Mario Six <mario....@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....@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 <s...@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.
Who will ever deallocate it? To me these cases are the same and I can't see why an existing property should be simply assigned, but a new property must be allocated. At the very lease that seems like a confusing thing to explain in the function comment you're going to add :-) > > 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. Hmm, I think just documenting behaviour clearly is good enough. How about we don't allocate the memory here. The caller can do it if necessary. Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot