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
