From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mario Six Date: Wed, 13 Jun 2018 10:31:26 +0200 Subject: [U-Boot] [PATCH v2 1/4] core: Add functions to set properties in live-tree In-Reply-To: References: <20180427125149.794-1-mario.six@gdsys.cc> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Simon, On Fri, May 4, 2018 at 11:37 PM, Simon Glass wrote: > Hi Mario, > > On 4 May 2018 at 01:14, Mario Six wrote: >> Hi Simon, >> >> On Tue, May 1, 2018 at 1:13 AM, Simon Glass wrote: >>> Hi Mario, >>> >>> On 27 April 2018 at 06:51, Mario Six 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 >>>> >>>> --- >>>> >>>> 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 >>> >>> 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