Hi Simon,
On Fri, May 4, 2018 at 11:37 PM, Simon Glass sjg@chromium.org wrote:
Hi Mario,
On 4 May 2018 at 01:14, Mario Six mario.six@gdsys.cc wrote:
Hi Simon,
On Tue, May 1, 2018 at 1:13 AM, Simon Glass sjg@chromium.org wrote:
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.
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.
OK, should not be too difficult. I'll use that approach for v3.
Regards, Simon
Best regards, Mario