Re: [PATCH 1/3] of: dynamic: add of_property_alloc() and of_property_free()

[Clément Léger]

Le Thu, 5 May 2022 07:30:47 +0000,
Christophe Leroy <christophe.leroy@xxxxxxxxxx> a écrit :

> >   	/*
> > -	 * NOTE: There is no check for zero length value.
> > -	 * In case of a boolean property, this will allocate a value
> > -	 * of zero bytes. We do this to work around the use
> > -	 * of of_get_property() calls on boolean values.
> > +	 * Even if the property has no value, it must be set to a
> > +	 * non-null value since of_get_property() is used to check
> > +	 * some values that might or not have a values (ranges for
> > +	 * instance). Moreover, when the node is released, prop->value
> > +	 * is kfreed so the memory must come from kmalloc.
> >   	 */
> > -	new->name = kstrdup(prop->name, allocflags);
> > -	new->value = kmemdup(prop->value, prop->length, allocflags);
> > -	new->length = prop->length;
> > -	if (!new->name || !new->value)
> > -		goto err_free;
> > +	if (!alloc_len)
> > +		alloc_len = 1;
> >   
> > -	/* mark the property as dynamic */
> > -	of_property_set_flag(new, OF_DYNAMIC);
> > +	prop->value = kzalloc(alloc_len, allocflags);
> > +	if (!prop->value)
> > +		goto out_err;
> >   
> > -	return new;
> > +	if (value)
> > +		memcpy(prop->value, value, value_len);  
> 
> Could you use kmemdup() instead of kzalloc+memcpy ?

I could but then, we won't be able to allocate a property value that is
larger than the original one. This is used by the powerpc code to
recopy an existing value and add some extra space after it.

> > diff --git a/include/linux/of.h b/include/linux/of.h
> > index 04971e85fbc9..6b345eb71c19 100644
> > --- a/include/linux/of.h
> > +++ b/include/linux/of.h
> > @@ -1463,6 +1463,11 @@ enum of_reconfig_change {
> >   };
> >   
> >   #ifdef CONFIG_OF_DYNAMIC
> > +extern struct property *of_property_alloc(const char *name, const void *value,
> > +					  int value_len, int len,
> > +					  gfp_t allocflags);
> > +extern void of_property_free(const struct property *prop);
> > +  
> 
> 'extern' is pointless for function prototypes, you should not add new 
> ones. Checkpatch complain about it:

I did so that, but I kept that since the existing code is full of them.
Since you mention it, I'll remove the extern.

> 
> CHECK: extern prototypes should be avoided in .h files
> #172: FILE: include/linux/of.h:1466:
> +extern struct property *of_property_alloc(const char *name, const void 
> *value,
> 
> CHECK: extern prototypes should be avoided in .h files
> #175: FILE: include/linux/of.h:1469:
> +extern void of_property_free(const struct property *prop);
> 
> 
> 
> 
> >   extern int of_reconfig_notifier_register(struct notifier_block *);
> >   extern int of_reconfig_notifier_unregister(struct notifier_block *);
> >   extern int of_reconfig_notify(unsigned long, struct of_reconfig_data *rd);
> > @@ -1507,6 +1512,17 @@ static inline int of_changeset_update_property(struct of_changeset *ocs,
> >   	return of_changeset_action(ocs, OF_RECONFIG_UPDATE_PROPERTY, np, prop);
> >   }
> >   #else /* CONFIG_OF_DYNAMIC */
> > +
> > +static inline struct property *of_property_alloc(const char *name,
> > +						 const void *value,
> > +						 int value_len, int len,
> > +						 gfp_t allocflags)  
> 
> Can that fit on less lines ?
> 
> May be:
> 
> static inline struct property
> *of_property_alloc(const char *name, const void *value, int value_len,
> 		   int len, gfp_t allocflags)

Yes, that seems a better split.

Thanks,


-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com