Hi Rob, > On May 9, 2016, at 17:27 , Rob Herring <robherring2@xxxxxxxxx> wrote: > > On Mon, May 9, 2016 at 8:20 AM, Pantelis Antoniou > <pantelis.antoniou@xxxxxxxxxxxx> wrote: >> Changesets are very powerful, but the lack of a helper API >> makes using them cumbersome. Introduce a simple copy based >> API that makes things considerably easier. >> >> To wit, adding a property using the raw API. >> >> struct property *prop; >> prop = kzalloc(sizeof(*prop)), GFP_KERNEL); >> prop->name = kstrdup("compatible"); >> prop->value = kstrdup("foo,bar"); >> prop->length = strlen(prop->value) + 1; >> of_changeset_add_property(ocs, np, prop); >> >> while using the helper API >> >> of_changeset_add_property_string(ocs, np, "compatible", >> "foo,bar"); > > Seems useful. Do we not have any users that can be converted? > There is a user for this, which will be shortly posted. > [...] > >> +static int __of_changeset_add_update_property_copy(struct of_changeset *ocs, >> + struct device_node *np, const char *name, const void *value, >> + int length, bool update) >> +{ >> + struct property *prop; >> + char *new_name; >> + void *new_value; >> + int ret = -ENOMEM; >> + >> + prop = kzalloc(sizeof(*prop), GFP_KERNEL); >> + if (!prop) >> + goto out_no_prop; > > Just return here. > OK >> + >> + new_name = kstrdup(name, GFP_KERNEL); >> + if (!new_name) >> + goto out_no_name; >> + >> + /* >> + * 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. >> + */ >> + new_value = kmemdup(value, length, GFP_KERNEL); >> + if (!new_value) >> + goto out_no_value; >> + >> + of_property_set_flag(prop, OF_DYNAMIC); >> + >> + prop->name = new_name; >> + prop->value = new_value; >> + prop->length = length; >> + >> + if (!update) >> + ret = of_changeset_add_property(ocs, np, prop); >> + else >> + ret = of_changeset_update_property(ocs, np, prop); >> + >> + if (ret != 0) > > if (!ret) > return 0; > > >> + goto out_no_add; >> + >> + return 0; >> + >> +out_no_add: > > ... and remove all this. > Err, there’s an exit path here from kmemdup (goto err_no_value). We’ll be leaking memory on error. >> + kfree(prop->value); >> +out_no_value: >> + kfree(prop->name); >> +out_no_name: >> + kfree(prop); >> +out_no_prop: >> + return ret; >> +} >> + >> +static int __of_changeset_add_update_property_string(struct of_changeset *ocs, >> + struct device_node *np, const char *name, const char *str, >> + bool update) >> +{ >> + return __of_changeset_add_update_property_copy(ocs, np, name, str, >> + strlen(str) + 1, update); >> +} >> + >> +static int __of_changeset_add_update_property_stringv(struct of_changeset *ocs, >> + struct device_node *np, const char *name, >> + const char *fmt, va_list vargs, bool update) >> +{ >> + char *str; >> + int ret; >> + >> + str = kvasprintf(GFP_KERNEL, fmt, vargs); >> + if (!str) >> + return -ENOMEM; >> + >> + ret = __of_changeset_add_update_property_string(ocs, np, name, str, update); >> + >> + kfree(str); >> + >> + return ret; >> +} >> + >> +static int __of_changeset_add_update_property_string_list( >> + struct of_changeset *ocs, struct device_node *np, const char *name, >> + const char **strs, int count, bool update) >> +{ >> + int total = 0, i, ret; >> + char *value, *s; >> + >> + for (i = 0; i < count; i++) { >> + /* check if it's NULL */ >> + if (!strs[i]) >> + return -EINVAL; >> + total += strlen(strs[i]) + 1; >> + } >> + >> + value = kmalloc(total, GFP_KERNEL); >> + if (!value) >> + return -ENOMEM; >> + >> + for (i = 0, s = value; i < count; i++) { >> + /* no need to check for NULL, check above */ >> + strcpy(s, strs[i]); >> + s += strlen(strs[i]) + 1; >> + } >> + >> + ret = __of_changeset_add_update_property_copy(ocs, np, name, value, >> + total, update); >> + >> + kfree(value); >> + >> + return ret; >> +} >> + >> +static int __of_changeset_add_update_property_u32(struct of_changeset *ocs, >> + struct device_node *np, const char *name, u32 val, bool update) >> +{ >> + /* in place */ >> + val = cpu_to_be32(val); > > Kill this function and move this to the 2 callers. > >> + return __of_changeset_add_update_property_copy(ocs, np, name, &val, >> + sizeof(val), update); >> +} >> + >> +static int __of_changeset_add_update_property_bool(struct of_changeset *ocs, >> + struct device_node *np, const char *name, bool update) >> +{ > > I think this intermediate function could be killed off too. > >> + return __of_changeset_add_update_property_copy(ocs, np, name, "", 0, >> + update); >> +} > > [...] > >> +static struct device_node * >> +__of_changeset_node_move_one(struct of_changeset *ocs, >> + struct device_node *np, struct device_node *new_parent) >> +{ >> + struct device_node *np2; >> + const char *unitname; >> + int err; >> + >> + err = of_changeset_detach_node(ocs, np); >> + if (err) >> + return ERR_PTR(err); >> + >> + unitname = strrchr(np->full_name, '/'); >> + if (!unitname) >> + unitname = np->full_name; >> + >> + np2 = __of_node_dup(np, "%s/%s", >> + new_parent->full_name, unitname); >> + if (!np2) >> + return ERR_PTR(-ENOMEM); >> + np2->parent = new_parent; >> + >> + err = of_changeset_attach_node(ocs, np2); >> + if (err) >> + return ERR_PTR(err); >> + >> + return np2; >> +} >> + >> +/** >> + * of_changeset_node_move_to - Moves a subtree to a new place in >> + * the tree >> + * >> + * @ocs: changeset pointer >> + * @np: device node pointer to be moved >> + * @to: device node of the new parent >> + * >> + * Moves a subtree to a new place in the tree. >> + * Note that a move is a safe operation because the phandles >> + * remain valid. > > I'm having a hard time imagining a usecase for this one. I would drop > until we have a user in tree. > There’s a use case - it will be apparent with the next patches. >> + * >> + * Returns zero on success, a negative error value otherwise. >> + */ >> +int of_changeset_node_move(struct of_changeset *ocs, >> + struct device_node *np, struct device_node *new_parent) >> +{ >> + struct device_node *npc, *nppc; >> + >> + /* move the root first */ >> + nppc = __of_changeset_node_move_one(ocs, np, new_parent); >> + if (IS_ERR(nppc)) >> + return PTR_ERR(nppc); >> + >> + /* move the subtrees next */ >> + for_each_child_of_node(np, npc) { >> + nppc = __of_changeset_node_move_one(ocs, npc, nppc); >> + if (IS_ERR(nppc)) { >> + of_node_put(npc); >> + return PTR_ERR(nppc); >> + } >> + } >> + >> + return 0; >> +} >> diff --git a/include/linux/of.h b/include/linux/of.h >> index 3175803..f015911 100644 >> --- a/include/linux/of.h >> +++ b/include/linux/of.h >> @@ -1037,6 +1037,42 @@ static inline int of_changeset_update_property(struct of_changeset *ocs, >> { >> return of_changeset_action(ocs, OF_RECONFIG_UPDATE_PROPERTY, np, prop); >> } >> + >> +struct device_node *of_changeset_create_device_nodev( >> + struct of_changeset *ocs, struct device_node *parent, >> + const char *fmt, va_list vargs); >> +__printf(3, 4) struct device_node *of_changeset_create_device_node( >> + struct of_changeset *ocs, struct device_node *parent, >> + const char *fmt, ...); >> +int of_changeset_add_property_copy(struct of_changeset *ocs, >> + struct device_node *np, const char *name, >> + const void *value, int length); >> +int of_changeset_add_property_string(struct of_changeset *ocs, >> + struct device_node *np, const char *name, const char *str); >> +__printf(4, 5) int of_changeset_add_property_stringf(struct of_changeset *ocs, >> + struct device_node *np, const char *name, const char *fmt, ...); >> +int of_changeset_add_property_string_list(struct of_changeset *ocs, >> + struct device_node *np, const char *name, const char **strs, int count); >> +int of_changeset_add_property_u32(struct of_changeset *ocs, >> + struct device_node *np, const char *name, u32 val); >> +int of_changeset_add_property_bool(struct of_changeset *ocs, >> + struct device_node *np, const char *name); >> +int of_changeset_update_property_copy(struct of_changeset *ocs, >> + struct device_node *np, const char *name, >> + const void *value, int length); >> +int of_changeset_update_property_string(struct of_changeset *ocs, >> + struct device_node *np, const char *name, const char *str); >> +__printf(4, 5) int of_changeset_update_property_stringf(struct of_changeset *ocs, >> + struct device_node *np, const char *name, const char *fmt, ...); >> +int of_changeset_update_property_string_list(struct of_changeset *ocs, >> + struct device_node *np, const char *name, const char **strs, int count); >> +int of_changeset_update_property_u32(struct of_changeset *ocs, >> + struct device_node *np, const char *name, u32 val); >> +int of_changeset_update_property_bool(struct of_changeset *ocs, >> + struct device_node *np, const char *name); >> +int of_changeset_node_move(struct of_changeset *ocs, >> + struct device_node *np, struct device_node *new_parent); > > A bunch of these can be static inline wrappers. > > You need EXPORT_SYMBOL_GPL on these as well. > I will add the exports. > Rob Regards — Pantelis -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html