Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1397052 > unrolled thread

[PATCH 0/4] of: dynamic: Changesets helpers & fixes

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4]  of: dynamic: Changesets helpers & fixes Pantelis Antoniou <pantelis.antoniou@konsulko.com> - 2016-05-09 15:30 +0200
    [PATCH 1/4] of: dynamic: changeset prop-update revert fix Pantelis Antoniou <pantelis.antoniou@konsulko.com> - 2016-05-09 15:30 +0200
    [PATCH 3/4] of: changesets: Introduce changeset helper methods Pantelis Antoniou <pantelis.antoniou@konsulko.com> - 2016-05-09 15:30 +0200
      Re: [PATCH 3/4] of: changesets: Introduce changeset helper methods Rob Herring <robherring2@gmail.com> - 2016-05-09 16:30 +0200
        Re: [PATCH 3/4] of: changesets: Introduce changeset helper methods Pantelis Antoniou <pantelis.antoniou@konsulko.com> - 2016-05-09 16:40 +0200
          Re: [PATCH 3/4] of: changesets: Introduce changeset helper methods Rob Herring <robherring2@gmail.com> - 2016-05-09 17:10 +0200

#1397052 — [PATCH 0/4] of: dynamic: Changesets helpers & fixes

This patchset introduces changeset helpers which makes working with
changeset much easier and less error prone.

The first patch fixes a bug when using an update property changeset operation
but the property does not exist.

The second exports an internal method which is used for the third patch
which contains the bulk of the changes.

Finally the last patch adds a unittest for the changeset helpers.

Pantelis Antoniou (4):
  of: dynamic: changeset prop-update revert fix
  of: dynamic: Add __of_node_dupv()
  of: changesets: Introduce changeset helper methods
  of: unittest: changeset helpers

 drivers/of/dynamic.c  | 535 +++++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/of/unittest.c |  54 +++++
 include/linux/of.h    | 143 ++++++++++++++
 3 files changed, 726 insertions(+), 6 deletions(-)

-- 
1.7.12

[toc] | [next] | [standalone]

#1397053 — [PATCH 1/4] of: dynamic: changeset prop-update revert fix

When reverting an update property changeset entry that created a
property the reverse operation is a remove property and not an update.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/of/dynamic.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index c647bd1..4145b44 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -497,6 +497,11 @@ static void __of_changeset_entry_invert(struct of_changeset_entry *ce,
 	case OF_RECONFIG_UPDATE_PROPERTY:
 		rce->old_prop = ce->prop;
 		rce->prop = ce->old_prop;
+		/* update was used but original property did not exist */
+		if (!rce->prop) {
+			rce->action = OF_RECONFIG_REMOVE_PROPERTY;
+			rce->prop = ce->prop;
+		}
 		break;
 	}
 }
-- 
1.7.12

[toc] | [prev] | [next] | [standalone]

#1397056 — [PATCH 3/4] of: changesets: Introduce changeset helper methods

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");

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/of/dynamic.c | 501 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of.h   | 143 +++++++++++++++
 2 files changed, 644 insertions(+)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index e4dea94..350b1cf 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -828,3 +828,504 @@ int of_changeset_action(struct of_changeset *ocs, unsigned long action,
 	return 0;
 }
 EXPORT_SYMBOL_GPL(of_changeset_action);
+
+/* changeset helpers */
+
+/**
+ * of_changeset_create_device_node - Create an empty device node
+ *
+ * @ocs:	changeset pointer
+ * @parent:	parent device node
+ * @fmt:	format string for the node's full_name
+ * @args:	argument list for the format string
+ *
+ * Create an empty device node, marking it as detached and allocated.
+ *
+ * Returns a device node on success, an error encoded pointer otherwise
+ */
+struct device_node *of_changeset_create_device_nodev(
+	struct of_changeset *ocs, struct device_node *parent,
+	const char *fmt, va_list vargs)
+{
+	struct device_node *node;
+
+	node = __of_node_dupv(NULL, fmt, vargs);
+	if (!node)
+		return ERR_PTR(-ENOMEM);
+
+	node->parent = parent;
+	return node;
+}
+
+/**
+ * of_changeset_create_device_node - Create an empty device node
+ *
+ * @ocs:	changeset pointer
+ * @parent:	parent device node
+ * @fmt:	Format string for the node's full_name
+ * ...		Arguments
+ *
+ * Create an empty device node, marking it as detached and allocated.
+ *
+ * Returns a device node on success, an error encoded pointer otherwise
+ */
+struct device_node *of_changeset_create_device_node(
+	struct of_changeset *ocs, struct device_node *parent,
+	const char *fmt, ...)
+{
+	va_list vargs;
+	struct device_node *node;
+
+	va_start(vargs, fmt);
+	node = of_changeset_create_device_nodev(ocs, parent, fmt, vargs);
+	va_end(vargs);
+	return node;
+}
+
+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;
+
+	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)
+		goto out_no_add;
+
+	return 0;
+
+out_no_add:
+	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);
+	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)
+{
+	return __of_changeset_add_update_property_copy(ocs, np, name, "", 0,
+			update);
+}
+
+/**
+ * of_changeset_add_property_copy - Create a new property copying name & value
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @value:	pointer to the value data
+ * @length:	length of the value in bytes
+ *
+ * Adds a property to the changeset by making copies of the name & value
+ * entries.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_add_property_copy(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const void *value,
+		int length)
+{
+	return __of_changeset_add_update_property_copy(ocs, np, name, value,
+			length, false);
+}
+
+/**
+ * of_changeset_add_property_string - Create a new string property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @str:	string property
+ *
+ * Adds a string property to the changeset by making copies of the name
+ * and the string value.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_add_property_string(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const char *str)
+{
+	return __of_changeset_add_update_property_string(ocs, np, name, str,
+			false);
+}
+
+/**
+ * of_changeset_add_property_stringf - Create a new formatted string property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @fmt:	format of string property
+ * ...		arguments of the format string
+ *
+ * Adds a string property to the changeset by making copies of the name
+ * and the formatted value.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_add_property_stringf(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const char *fmt, ...)
+{
+	va_list vargs;
+	int ret;
+
+	va_start(vargs, fmt);
+	ret = __of_changeset_add_update_property_stringv(ocs, np, name, fmt,
+			vargs, false);
+	va_end(vargs);
+	return ret;
+}
+
+/**
+ * of_changeset_add_property_string_list - Create a new string list property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @strs:	pointer to the string list
+ * @count:	string count
+ *
+ * Adds a string list property to the changeset.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_add_property_string_list(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const char **strs,
+		int count)
+{
+	return __of_changeset_add_update_property_string_list(ocs, np, name,
+			strs, count, false);
+}
+
+/**
+ * of_changeset_add_property_u32 - Create a new u32 property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @val:	value in host endian format
+ *
+ * Adds a u32 property to the changeset.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_add_property_u32(struct of_changeset *ocs,
+		struct device_node *np, const char *name, u32 val)
+{
+	return __of_changeset_add_update_property_u32(ocs, np, name,
+			val, false);
+}
+
+/**
+ * of_changeset_add_property_bool - Create a new u32 property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ *
+ * Adds a bool property to the changeset. Note that there is
+ * no option to set the value to false, since the property
+ * existing sets it to true.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_add_property_bool(struct of_changeset *ocs,
+		struct device_node *np, const char *name)
+{
+	return __of_changeset_add_update_property_bool(ocs, np, name, false);
+}
+
+/**
+ * of_changeset_update_property_copy - Update a property copying name & value
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @value:	pointer to the value data
+ * @length:	length of the value in bytes
+ *
+ * Update a property to the changeset by making copies of the name & value
+ * entries.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_update_property_copy(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const void *value,
+		int length)
+{
+	return __of_changeset_add_update_property_copy(ocs, np, name, value,
+			length, true);
+}
+
+/**
+ * of_changeset_update_property_string - Create a new string property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @str:	string property
+ *
+ * Updates a string property to the changeset by making copies of the name
+ * and the string value.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_update_property_string(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const char *str)
+{
+	return __of_changeset_add_update_property_string(ocs, np, name, str,
+			true);
+}
+
+/**
+ * of_changeset_update_property_stringf - Update formatted string property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @fmt:	format of string property
+ * ...		arguments of the format string
+ *
+ * Updates a string property to the changeset by making copies of the name
+ * and the formatted value.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_update_property_stringf(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const char *fmt, ...)
+{
+	va_list vargs;
+	int ret;
+
+	va_start(vargs, fmt);
+	ret = __of_changeset_add_update_property_stringv(ocs, np, name, fmt,
+			vargs, true);
+	va_end(vargs);
+	return ret;
+}
+
+/**
+ * of_changeset_update_property_string_list - Update string list property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @strs:	pointer to the string list
+ * @count:	string count
+ *
+ * Updates a string list property to the changeset.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_update_property_string_list(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const char **strs,
+		int count)
+{
+	return __of_changeset_add_update_property_string_list(ocs, np, name,
+			strs, count, true);
+}
+
+/**
+ * of_changeset_update_property_u32 - Update u32 property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @val:	value in host endian format
+ *
+ * Updates a u32 property to the changeset.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_update_property_u32(struct of_changeset *ocs,
+		struct device_node *np, const char *name, u32 val)
+{
+	return __of_changeset_add_update_property_u32(ocs, np, name,
+			val, true);
+}
+
+/**
+ * of_changeset_update_property_bool - Update a bool property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ *
+ * Updates a property to the changeset. Note that there is
+ * no option to set the value to false, since the property
+ * existing sets it to true.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_update_property_bool(struct of_changeset *ocs,
+		struct device_node *np, const char *name)
+{
+	return __of_changeset_add_update_property_bool(ocs, np, name, true);
+}
+
+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.
+ *
+ * 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);
+
 #else /* CONFIG_OF_DYNAMIC */
 static inline int of_reconfig_notifier_register(struct notifier_block *nb)
 {
@@ -1056,6 +1092,113 @@ static inline int of_reconfig_get_state_change(unsigned long action,
 {
 	return -EINVAL;
 }
+
+static inline int of_changeset_create_device_node(struct of_changeset *ocs,
+	struct device_node *parent, const char *fmt, ...)
+{
+	return -EINVAL;
+}
+
+int of_changeset_add_property_copy(struct of_changeset *ocs,
+	struct device_node *np, const char *name,
+	const void *value, int length)
+{
+	return -EINVAL;
+}
+
+int of_changeset_add_property_string(struct of_changeset *ocs,
+	struct device_node *np, const char *name, const char *str)
+{
+	return -EINVAL;
+}
+
+static inline struct device_node *of_changeset_create_device_nodev(
+	struct of_changeset *ocs, struct device_node *parent,
+	const char *fmt, va_list vargs)
+{
+	return ERR_PTR(-EINVAL);
+}
+
+static inline __printf(4, 5) struct device_node *
+	of_changeset_add_property_stringf(
+		struct of_changeset *ocs, struct device_node *np,
+		const char *name, const char *fmt, ...)
+{
+	return ERR_PTR(-EINVAL);
+}
+
+static inline int of_changeset_add_property_string_list(
+	struct of_changeset *ocs, struct device_node *np, const char *name,
+	const char **strs, int count)
+{
+	return -EINVAL;
+}
+
+static inline int of_changeset_add_property_u32(struct of_changeset *ocs,
+	struct device_node *np, const char *name, u32 val)
+{
+	return -EINVAL;
+}
+
+static inline int of_changeset_add_property_bool(struct of_changeset *ocs,
+	struct device_node *np, const char *name)
+{
+	return -EINVAL;
+}
+
+int of_changeset_update_property_copy(struct of_changeset *ocs,
+	struct device_node *np, const char *name,
+	const void *value, int length)
+{
+	return -EINVAL;
+}
+
+int of_changeset_update_property_string(struct of_changeset *ocs,
+	struct device_node *np, const char *name, const char *str)
+{
+	return -EINVAL;
+}
+
+static inline struct device_node *of_changeset_create_device_nodev(
+	struct of_changeset *ocs, struct device_node *parent,
+	const char *fmt, va_list vargs)
+{
+	return ERR_PTR(-EINVAL);
+}
+
+static inline __printf(4, 5) struct device_node *
+	of_changeset_update_property_stringf(
+		struct of_changeset *ocs, struct device_node *np,
+		const char *name, const char *fmt, ...)
+{
+	return ERR_PTR(-EINVAL);
+}
+
+static inline int of_changeset_update_property_string_list(
+	struct of_changeset *ocs, struct device_node *np, const char *name,
+	const char **strs, int count)
+{
+	return -EINVAL;
+}
+
+static inline int of_changeset_update_property_u32(struct of_changeset *ocs,
+	struct device_node *np, const char *name, u32 val)
+{
+	return -EINVAL;
+}
+
+static inline int of_changeset_update_property_bool(struct of_changeset *ocs,
+	struct device_node *np, const char *name)
+{
+	return -EINVAL;
+}
+
+static inline int of_changeset_node_move(struct of_changeset *ocs,
+		struct device_node *np, struct device_node *new_parent)
+{
+	return -EINVAL;
+}
+
 #endif /* CONFIG_OF_DYNAMIC */
 
 /* CONFIG_OF_RESOLVE api */
-- 
1.7.12

[toc] | [prev] | [next] | [standalone]

#1397090 — Re: [PATCH 3/4] of: changesets: Introduce changeset helper methods

On Mon, May 9, 2016 at 8:20 AM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> 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?

[...]

> +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.

> +
> +       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.

> +       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.

> + *
> + * 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.

Rob

[toc] | [prev] | [next] | [standalone]

#1397098 — Re: [PATCH 3/4] of: changesets: Introduce changeset helper methods

Hi Rob,

> On May 9, 2016, at 17:27 , Rob Herring <robherring2@gmail.com> wrote:
> 
> On Mon, May 9, 2016 at 8:20 AM, Pantelis Antoniou
> <pantelis.antoniou@konsulko.com> 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

[toc] | [prev] | [next] | [standalone]

#1397125 — Re: [PATCH 3/4] of: changesets: Introduce changeset helper methods

On Mon, May 9, 2016 at 9:34 AM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> Hi Rob,
>
>> On May 9, 2016, at 17:27 , Rob Herring <robherring2@gmail.com> wrote:
>>
>> On Mon, May 9, 2016 at 8:20 AM, Pantelis Antoniou
>> <pantelis.antoniou@konsulko.com> 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.

[...]

>>> +       /*
>>> +        * 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.

No you won't. "This" is the hunk above it. The error handling would
still be here:

>>> +       kfree(prop->value);
>>> +out_no_value:
>>> +       kfree(prop->name);
>>> +out_no_name:
>>> +       kfree(prop);
>>> +out_no_prop:
>>> +       return ret;
>>> +}

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web