Re: [PATCHv3 1/2] powerpc/of: split out new_property() for reusing
On Sat, Mar 7, 2020 at 3:59 AM Nathan Lynch wrote: > > Hi, > > Pingfan Liu writes: > > Splitting out new_property() for coming reusing and moving it to > > of_helpers.c. > > [...] > > > +struct property *new_property(const char *name, const int length, > > + const unsigned char *value, struct property *last) > > +{ > > + struct property *new = kzalloc(sizeof(*new), GFP_KERNEL); > > + > > + if (!new) > > + return NULL; > > + > > + new->name = kstrdup(name, GFP_KERNEL); > > + if (!new->name) > > + goto cleanup; > > + new->value = kmalloc(length + 1, GFP_KERNEL); > > + if (!new->value) > > + goto cleanup; > > + > > + memcpy(new->value, value, length); > > + *(((char *)new->value) + length) = 0; > > + new->length = length; > > + new->next = last; > > + return new; > > + > > +cleanup: > > + kfree(new->name); > > + kfree(new->value); > > + kfree(new); > > + return NULL; > > +} > > This function in its current form isn't suitable for more general use: > > * It appears to be tailored to string properties - note the char * value > parameter, the length + 1 allocation and nul termination. > > * Most code shouldn't need the 'last' argument. The code where this > currently resides builds a list of properties and attaches it to a new > node, bypassing of_add_property(). > > Let's look at the call site you add in your next patch: > > + big = cpu_to_be64(p->bound_addr); > + property = new_property("bound-addr", sizeof(u64), (const > unsigned char *), > + NULL); > + of_add_property(dn, property); > > So you have to use a cast, and this is going to allocate (sizeof(u64) + 1) > for the value, is that what you want? > > I think you should leave that legacy pseries reconfig code undisturbed > (frankly that stuff should get deprecated and removed) and if you want a > generic helper it should look more like: > > struct property *of_property_new(const char *name, size_t length, > const void *value, gfp_t allocflags) > > __of_prop_dup() looks like a good model/guide here. Thanks for your good suggestion. I will re-code based on your suggestion, if [2/2] turns out acceptable. Regards, Pingfan
Re: [PATCHv3 1/2] powerpc/of: split out new_property() for reusing
Hi, Pingfan Liu writes: > Splitting out new_property() for coming reusing and moving it to > of_helpers.c. [...] > +struct property *new_property(const char *name, const int length, > + const unsigned char *value, struct property *last) > +{ > + struct property *new = kzalloc(sizeof(*new), GFP_KERNEL); > + > + if (!new) > + return NULL; > + > + new->name = kstrdup(name, GFP_KERNEL); > + if (!new->name) > + goto cleanup; > + new->value = kmalloc(length + 1, GFP_KERNEL); > + if (!new->value) > + goto cleanup; > + > + memcpy(new->value, value, length); > + *(((char *)new->value) + length) = 0; > + new->length = length; > + new->next = last; > + return new; > + > +cleanup: > + kfree(new->name); > + kfree(new->value); > + kfree(new); > + return NULL; > +} This function in its current form isn't suitable for more general use: * It appears to be tailored to string properties - note the char * value parameter, the length + 1 allocation and nul termination. * Most code shouldn't need the 'last' argument. The code where this currently resides builds a list of properties and attaches it to a new node, bypassing of_add_property(). Let's look at the call site you add in your next patch: + big = cpu_to_be64(p->bound_addr); + property = new_property("bound-addr", sizeof(u64), (const unsigned char *), + NULL); + of_add_property(dn, property); So you have to use a cast, and this is going to allocate (sizeof(u64) + 1) for the value, is that what you want? I think you should leave that legacy pseries reconfig code undisturbed (frankly that stuff should get deprecated and removed) and if you want a generic helper it should look more like: struct property *of_property_new(const char *name, size_t length, const void *value, gfp_t allocflags) __of_prop_dup() looks like a good model/guide here.
Re: [PATCHv3 1/2] powerpc/of: split out new_property() for reusing
On 4/3/20 7:47 pm, Pingfan Liu wrote: Splitting out new_property() for coming reusing and moving it to of_helpers.c. Also do some coding style cleanup. Signed-off-by: Pingfan Liu To: linuxppc-dev@lists.ozlabs.org Cc: Benjamin Herrenschmidt Cc: Paul Mackerras Cc: Michael Ellerman Cc: Hari Bathini Cc: Aneesh Kumar K.V Cc: Oliver O'Halloran Cc: Dan Williams Cc: Andrew Donnellan Cc: Christophe Leroy Cc: Rob Herring Cc: Frank Rowand Cc: ke...@lists.infradead.org Would this be useful to move into generic OF code? Also if more functions are moving into of_helpers.c perhaps there should be a common function name prefix. Otherwise your style cleanup looks good. Reviewed-by: Andrew Donnellan -- Andrew Donnellan OzLabs, ADL Canberra a...@linux.ibm.com IBM Australia Limited