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

[Nathan Lynch <nathanl@linux.ibm.com>]

Hi,

Pingfan Liu <kernelfans@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.