[PATCHv3 1/2] powerpc/of: split out new_property() for reusing

[Pingfan Liu]

On Sat, Mar 7, 2020 at 3:59 AM Nathan Lynch <nathanl at linux.ibm.com> wrote:
>
> Hi,
>
> Pingfan Liu <kernelfans at gmail.com> 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 *)&big,
> +                       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