Skip to contentSkip to conversation

Export APIs to copy device properties & more

PATCH Superseded Dmitry Torokhov 6 participants 19 messages Feb 1, 2017 – Feb 3, 2017

Subsystems (4)
ACPI Driver core I2C Software nodes and device properties

v3 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Dmitry Torokhov

author
#0 lore raw

v3 cover Export APIs to copy device properties & more

Hi,

Here is the refreshed series exporting APIs to copy statically declared device properties. The reason is that we want to augment ACPI-based devices with properties, and drivers usually have a largish DMI table for multiple models, so it is desirable to mark everything as __initdata/__initconst, and then copy only the entry matching the device we are running on and discard the rest.

The last patch is not really about device property APIs, but rather allowing users to attach properties to i2c_board_info, and have them attached to instantiated device(s). The reason it is included is because it depends on device_add_properties() taking const pointer, which is patch #2.

If it seems useful I hope Rafael and Wolfram would figure a way to merge it :).

v3:
- fix memory leak in property_copy_string_array() pointed out by Mika Westerberg

v2:
- addressed Andy's comments
- added property_entries_free()
- added patch to allow constify values of property arrays
- added i2c patch allowing to attach property to devices via board info

v1:
- initial posting

Dmitry Torokhov (4):
device property: export code duplicating array of property entries device property: allow to constify properties
driver property: constify property arrays values
i2c: allow specify device properties in i2c_board_info

 drivers/base/property.c  | 222 ++++++++++++++++++++++++++++++-----------------
 drivers/i2c/i2c-core.c   |  16 +++-
 include/linux/i2c.h      |   3 +
 include/linux/property.h |  19 ++--
 4 files changed, 171 insertions(+), 89 deletions(-)

Thanks.

Signature · 1 line
Dmitry
author
#1 lore raw

PATCH v3 1/4 device property: export code duplicating array of property entries

When augmenting ACPI-enumerated devices with additional property data based on DMI info, a module has often several potential property sets, with only one being active on a given box. In order to save memory it should be possible to mark everything and __initdata or __initconst, execute DMI match early, and duplicate relevant properties. Then kernel will discard the rest of them.

Reviewed-by Andy Shevchenko <[email protected]>
Reviewed-by Mika Westerberg <[email protected]>
Signed-off-by Dmitry Torokhov <[email protected]>
---
 drivers/base/property.c  | 137 +++++++++++++++++++++++++++++------------------
 include/linux/property.h |   5 ++
 2 files changed, 90 insertions(+), 52 deletions(-)
drivers/base/property.c +85 -52
index 43a36d68c3fd..934845fc72e5 100644
@@ -682,41 +682,8 @@ int fwnode_property_match_string(struct fwnode_handle *fwnode,
 }
 EXPORT_SYMBOL_GPL(fwnode_property_match_string);
 
-/**
- * pset_free_set - releases memory allocated for copied property set
- * @pset: Property set to release
- *
- * Function takes previously copied property set and releases all the
- * memory allocated to it.
- */
-static void pset_free_set(struct property_set *pset)
-{
-	const struct property_entry *prop;
-	size_t i, nval;
-
-	if (!pset)
-		return;
-
-	for (prop = pset->properties; prop->name; prop++) {
-		if (prop->is_array) {
-			if (prop->is_string && prop->pointer.str) {
-				nval = prop->length / sizeof(const char *);
-				for (i = 0; i < nval; i++)
-					kfree(prop->pointer.str[i]);
-			}
-			kfree(prop->pointer.raw_data);
-		} else if (prop->is_string) {
-			kfree(prop->value.str);
-		}
-		kfree(prop->name);
-	}
-
-	kfree(pset->properties);
-	kfree(pset);
-}
-
-static int pset_copy_entry(struct property_entry *dst,
-			   const struct property_entry *src)
+static int property_entry_copy(struct property_entry *dst,
+			       const struct property_entry *src)
 {
 	const char **d, **s;
 	size_t i, nval;

@@ -765,6 +732,84 @@ static int pset_copy_entry(struct property_entry *dst,
 }
 
 /**
+ * property_entries_dup - duplicate array of properties
+ * @properties: array of properties to copy
+ *
+ * This function creates a deep copy of the given NULL-terminated array
+ * of property entries.
+ */
+struct property_entry *
+property_entries_dup(const struct property_entry *properties)
+{
+	struct property_entry *p;
+	int i, n = 0;
+
+	while (properties[n].name)
+		n++;
+
+	p = kcalloc(n + 1, sizeof(*p), GFP_KERNEL);
+	if (!p)
+		return ERR_PTR(-ENOMEM);
+
+	for (i = 0; i < n; i++) {
+		int ret = property_entry_copy(&p[i], &properties[i]);
+		if (ret) {
+			kfree(p);
+			return ERR_PTR(ret);
+		}
+	}
+
+	return p;
+}
+EXPORT_SYMBOL_GPL(property_entries_dup);
+
+/**
+ * property_entries_free - free previously allocated array of properties
+ * @properties: array of properties to destroy
+ *
+ * This function frees given NULL-terminated array of property entries,
+ * along with their data.
+ */
+void property_entries_free(const struct property_entry *properties)
+{
+	const struct property_entry *p;
+	size_t i, nval;
+
+	for (p = properties; p->name; p++) {
+		if (p->is_array) {
+			if (p->is_string && p->pointer.str) {
+				nval = p->length / sizeof(const char *);
+				for (i = 0; i < nval; i++)
+					kfree(p->pointer.str[i]);
+			}
+			kfree(p->pointer.raw_data);
+		} else if (p->is_string) {
+			kfree(p->value.str);
+		}
+		kfree(p->name);
+	}
+
+	kfree(properties);
+}
+EXPORT_SYMBOL_GPL(property_entries_free);
+
+/**
+ * pset_free_set - releases memory allocated for copied property set
+ * @pset: Property set to release
+ *
+ * Function takes previously copied property set and releases all the
+ * memory allocated to it.
+ */
+static void pset_free_set(struct property_set *pset)
+{
+	if (!pset)
+		return;
+
+	property_entries_free(pset->properties);
+	kfree(pset);
+}
+
+/**
  * pset_copy_set - copies property set
  * @pset: Property set to copy
  *

@@ -776,32 +821,20 @@ static int pset_copy_entry(struct property_entry *dst,
  */
 static struct property_set *pset_copy_set(const struct property_set *pset)
 {
-	const struct property_entry *entry;
+	struct property_entry *properties;
 	struct property_set *p;
-	size_t i, n = 0;
 
 	p = kzalloc(sizeof(*p), GFP_KERNEL);
 	if (!p)
 		return ERR_PTR(-ENOMEM);
 
-	while (pset->properties[n].name)
-		n++;
-
-	p->properties = kcalloc(n + 1, sizeof(*entry), GFP_KERNEL);
-	if (!p->properties) {
+	properties = property_entries_dup(pset->properties);
+	if (IS_ERR(properties)) {
 		kfree(p);
-		return ERR_PTR(-ENOMEM);
-	}
-
-	for (i = 0; i < n; i++) {
-		int ret = pset_copy_entry(&p->properties[i],
-					  &pset->properties[i]);
-		if (ret) {
-			pset_free_set(p);
-			return ERR_PTR(ret);
-		}
+		return ERR_CAST(properties);
 	}
 
+	p->properties = properties;
 	return p;
 }
include/linux/property.h +5 -0
index 856e50b2140c..48f154ae7e99 100644
@@ -241,6 +241,11 @@ struct property_entry {
 	.name = _name_,				\
 }
 
+struct property_entry *
+property_entries_dup(const struct property_entry *properties);
+
+void property_entries_free(const struct property_entry *properties);
+
 int device_add_properties(struct device *dev,
 			  struct property_entry *properties);
 void device_remove_properties(struct device *dev);
Signature · 1 line
2.11.0.483.g087da7b7c-goog
author
#2 lore raw

PATCH v3 2/4 device property: allow to constify properties

In reply to Dmitry Torokhov

There is no reason why statically defined properties should be modifiable, so let's make device_add_properties() and the rest of pset_*() functions to take const pointers to properties.

This will allow us to mark properties as const/__initconst at definition sites.

Reviewed-by Mika Westerberg <[email protected]>
Reviewed-by Andy Shevchenko <[email protected]>
Signed-off-by Dmitry Torokhov <[email protected]>
---
 drivers/base/property.c  | 35 ++++++++++++++++++-----------------
 include/linux/property.h |  2 +-
 2 files changed, 19 insertions(+), 18 deletions(-)
drivers/base/property.c +18 -17
index 934845fc72e5..edc09854520b 100644
@@ -21,7 +21,7 @@
 
 struct property_set {
 	struct fwnode_handle fwnode;
-	struct property_entry *properties;
+	const struct property_entry *properties;
 };
 
 static inline bool is_pset_node(struct fwnode_handle *fwnode)

@@ -35,10 +35,10 @@ static inline struct property_set *to_pset_node(struct fwnode_handle *fwnode)
 		container_of(fwnode, struct property_set, fwnode) : NULL;
 }
 
-static struct property_entry *pset_prop_get(struct property_set *pset,
-					    const char *name)
+static const struct property_entry *pset_prop_get(struct property_set *pset,
+						  const char *name)
 {
-	struct property_entry *prop;
+	const struct property_entry *prop;
 
 	if (!pset || !pset->properties)
 		return NULL;

@@ -50,11 +50,11 @@ static struct property_entry *pset_prop_get(struct property_set *pset,
 	return NULL;
 }
 
-static void *pset_prop_find(struct property_set *pset, const char *propname,
-			    size_t length)
+static const void *pset_prop_find(struct property_set *pset,
+				  const char *propname, size_t length)
 {
-	struct property_entry *prop;
-	void *pointer;
+	const struct property_entry *prop;
+	const void *pointer;
 
 	prop = pset_prop_get(pset, propname);
 	if (!prop)

@@ -74,7 +74,7 @@ static int pset_prop_read_u8_array(struct property_set *pset,
 				   const char *propname,
 				   u8 *values, size_t nval)
 {
-	void *pointer;
+	const void *pointer;
 	size_t length = nval * sizeof(*values);
 
 	pointer = pset_prop_find(pset, propname, length);

@@ -89,7 +89,7 @@ static int pset_prop_read_u16_array(struct property_set *pset,
 				    const char *propname,
 				    u16 *values, size_t nval)
 {
-	void *pointer;
+	const void *pointer;
 	size_t length = nval * sizeof(*values);
 
 	pointer = pset_prop_find(pset, propname, length);

@@ -104,7 +104,7 @@ static int pset_prop_read_u32_array(struct property_set *pset,
 				    const char *propname,
 				    u32 *values, size_t nval)
 {
-	void *pointer;
+	const void *pointer;
 	size_t length = nval * sizeof(*values);
 
 	pointer = pset_prop_find(pset, propname, length);

@@ -119,7 +119,7 @@ static int pset_prop_read_u64_array(struct property_set *pset,
 				    const char *propname,
 				    u64 *values, size_t nval)
 {
-	void *pointer;
+	const void *pointer;
 	size_t length = nval * sizeof(*values);
 
 	pointer = pset_prop_find(pset, propname, length);

@@ -133,7 +133,7 @@ static int pset_prop_read_u64_array(struct property_set *pset,
 static int pset_prop_count_elems_of_size(struct property_set *pset,
 					 const char *propname, size_t length)
 {
-	struct property_entry *prop;
+	const struct property_entry *prop;
 
 	prop = pset_prop_get(pset, propname);
 	if (!prop)

@@ -146,7 +146,7 @@ static int pset_prop_read_string_array(struct property_set *pset,
 				       const char *propname,
 				       const char **strings, size_t nval)
 {
-	void *pointer;
+	const void *pointer;
 	size_t length = nval * sizeof(*strings);
 
 	pointer = pset_prop_find(pset, propname, length);

@@ -160,8 +160,8 @@ static int pset_prop_read_string_array(struct property_set *pset,
 static int pset_prop_read_string(struct property_set *pset,
 				 const char *propname, const char **strings)
 {
-	struct property_entry *prop;
-	const char **pointer;
+	const struct property_entry *prop;
+	const char * const *pointer;
 
 	prop = pset_prop_get(pset, propname);
 	if (!prop)

@@ -880,7 +880,8 @@ EXPORT_SYMBOL_GPL(device_remove_properties);
  * @dev as its secondary firmware node. The function takes a copy of
  * @properties.
  */
-int device_add_properties(struct device *dev, struct property_entry *properties)
+int device_add_properties(struct device *dev,
+			  const struct property_entry *properties)
 {
 	struct property_set *p, pset;
include/linux/property.h +1 -1
index 48f154ae7e99..5746e9927016 100644
@@ -247,7 +247,7 @@ property_entries_dup(const struct property_entry *properties);
 void property_entries_free(const struct property_entry *properties);
 
 int device_add_properties(struct device *dev,
-			  struct property_entry *properties);
+			  const struct property_entry *properties);
 void device_remove_properties(struct device *dev);
 
 bool device_dma_supported(struct device *dev);
Signature · 1 line
2.11.0.483.g087da7b7c-goog
author
#3 lore raw

PATCH v3 3/4 driver property: constify property arrays values

In reply to Dmitry Torokhov

Data that is fed into property arrays should not be modified, so let's mark relevant pointers as const. This will allow us making source arrays as const/__initconst.

Also fix memory leaks on errors in property_entry_copy().

Signed-off-by Dmitry Torokhov <[email protected]>
---
 drivers/base/property.c  | 66 +++++++++++++++++++++++++++++++++---------------
 include/linux/property.h | 12 ++++-----
 2 files changed, 52 insertions(+), 26 deletions(-)
drivers/base/property.c +46 -20
index edc09854520b..8cfbc94cafb1 100644
@@ -682,44 +682,66 @@ int fwnode_property_match_string(struct fwnode_handle *fwnode,
 }
 EXPORT_SYMBOL_GPL(fwnode_property_match_string);
 
+static int property_copy_string_array(struct property_entry *dst,
+				      const struct property_entry *src)
+{
+	char **d;
+	size_t nval = src->length / sizeof(*d);
+	size_t i;
+
+	d = kcalloc(nval, sizeof(*d), GFP_KERNEL);
+	if (!d)
+		return -ENOMEM;
+
+	for (i = 0; i < nval; i++) {
+		d[i] = kstrdup(src->pointer.str[i], GFP_KERNEL);
+		if (!d[i] && src->pointer.str[i]) {
+			while (--i >= 0)
+				kfree(d[i]);
+			kfree(d);
+			return -ENOMEM;
+		}
+	}
+
+	dst->pointer.str = (void *)d;
+	return 0;
+}
+
 static int property_entry_copy(struct property_entry *dst,
 			       const struct property_entry *src)
 {
-	const char **d, **s;
-	size_t i, nval;
+	int error;
 
 	dst->name = kstrdup(src->name, GFP_KERNEL);
 	if (!dst->name)
 		return -ENOMEM;
 
 	if (src->is_array) {
-		if (!src->length)
-			return -ENODATA;
+		if (!src->length) {
+			error = -ENODATA;
+			goto out_free_name;
+		}
 
 		if (src->is_string) {
-			nval = src->length / sizeof(const char *);
-			dst->pointer.str = kcalloc(nval, sizeof(const char *),
-						   GFP_KERNEL);
-			if (!dst->pointer.str)
-				return -ENOMEM;
-
-			d = dst->pointer.str;
-			s = src->pointer.str;
-			for (i = 0; i < nval; i++) {
-				d[i] = kstrdup(s[i], GFP_KERNEL);
-				if (!d[i] && s[i])
-					return -ENOMEM;
+			error = property_copy_string_array(dst, src);
+			if (error) {
+				error = -ENOMEM;
+				goto out_free_name;
 			}
 		} else {
 			dst->pointer.raw_data = kmemdup(src->pointer.raw_data,
 							src->length, GFP_KERNEL);
-			if (!dst->pointer.raw_data)
-				return -ENOMEM;
+			if (!dst->pointer.raw_data) {
+				error = -ENOMEM;
+				goto out_free_name;
+			}
 		}
 	} else if (src->is_string) {
 		dst->value.str = kstrdup(src->value.str, GFP_KERNEL);
-		if (!dst->value.str && src->value.str)
-			return -ENOMEM;
+		if (!dst->value.str && src->value.str) {
+			error = -ENOMEM;
+			goto out_free_name;
+		}
 	} else {
 		dst->value.raw_data = src->value.raw_data;
 	}

@@ -729,6 +751,10 @@ static int property_entry_copy(struct property_entry *dst,
 	dst->is_string = src->is_string;
 
 	return 0;
+
+out_free_name:
+	kfree(dst->name);
+	return error;
 }
 
 /**
include/linux/property.h +6 -6
index 5746e9927016..64e3a9c6d95f 100644
@@ -160,12 +160,12 @@ struct property_entry {
 	bool is_string;
 	union {
 		union {
-			void *raw_data;
-			u8 *u8_data;
-			u16 *u16_data;
-			u32 *u32_data;
-			u64 *u64_data;
-			const char **str;
+			const void *raw_data;
+			const u8 *u8_data;
+			const u16 *u16_data;
+			const u32 *u32_data;
+			const u64 *u64_data;
+			const char * const *str;
 		} pointer;
 		union {
 			unsigned long long raw_data;
Signature · 1 line
2.11.0.483.g087da7b7c-goog
#4 lore raw

Re: [PATCH v3 3/4] driver property: constify property arrays values

Quoted 3 lines · Dmitry Torokhov

Data that is fed into property arrays should not be modified, so let's mark relevant pointers as const. This will allow us making source arrays as const/__initconst.

trivia:

drivers/base/property.c

Quoted 1 line
drivers/base/property.c +0 -0

[]

Quoted 54 lines
drivers/base/property.c +30 -9
@@ -682,44 +682,66 @@ int fwnode_property_match_string(struct fwnode_handle *fwnode,
 }
 EXPORT_SYMBOL_GPL(fwnode_property_match_string);
 
+static int property_copy_string_array(struct property_entry *dst,
+				      const struct property_entry *src)
+{
+	char **d;
+	size_t nval = src->length / sizeof(*d);
+	size_t i;
+
+	d = kcalloc(nval, sizeof(*d), GFP_KERNEL);
+	if (!d)
+		return -ENOMEM;
+
+	for (i = 0; i < nval; i++) {
+		d[i] = kstrdup(src->pointer.str[i], GFP_KERNEL);
+		if (!d[i] && src->pointer.str[i]) {
+			while (--i >= 0)
+				kfree(d[i]);
+			kfree(d);
+			return -ENOMEM;
+		}
+	}
+
+	dst->pointer.str = (void *)d;
+	return 0;
+}
+
 static int property_entry_copy(struct property_entry *dst,
 			       const struct property_entry *src)
 {
-	const char **d, **s;
-	size_t i, nval;
+	int error;
 
 	dst->name = kstrdup(src->name, GFP_KERNEL);
 	if (!dst->name)
 		return -ENOMEM;
 
 	if (src->is_array) {
-		if (!src->length)
-			return -ENODATA;
+		if (!src->length) {
+			error = -ENODATA;
+			goto out_free_name;
+		}
 
 		if (src->is_string) {
-			nval = src->length / sizeof(const char *);
-			dst->pointer.str = kcalloc(nval, sizeof(const char *),
-						   GFP_KERNEL);
-			if (!dst->pointer.str)
-				return -ENOMEM;

drivers/base/property.c int fwnode_property_match_string(struct fwnode_handle *fwnode, · line 728

Quoted 10 lines
drivers/base/property.c +3 -7
-
-			d = dst->pointer.str;
-			s = src->pointer.str;
-			for (i = 0; i < nval; i++) {
-				d[i] = kstrdup(s[i], GFP_KERNEL);
-				if (!d[i] && s[i])
-					return -ENOMEM;
+			error = property_copy_string_array(dst, src);
+			if (error) {
+				error = -ENOMEM;

An unnecessary set as the return from the above
property_copy_string_array is either 0 or -ENOMEM

#5 lore raw

Re: [PATCH v3 3/4] driver property: constify property arrays values

Quoted 68 lines · Joe Perches
Quoted 3 lines · Dmitry Torokhov

Data that is fed into property arrays should not be modified, so let's mark relevant pointers as const. This will allow us making source arrays as const/__initconst.

trivia:

drivers/base/property.c

Quoted 1 line
drivers/base/property.c +0 -0

[]

Quoted 59 lines
drivers/base/property.c +30 -14
@@ -682,44 +682,66 @@ int fwnode_property_match_string(struct fwnode_handle *fwnode,
 }
 EXPORT_SYMBOL_GPL(fwnode_property_match_string);
 
+static int property_copy_string_array(struct property_entry *dst,
+				      const struct property_entry *src)
+{
+	char **d;
+	size_t nval = src->length / sizeof(*d);
+	size_t i;
+
+	d = kcalloc(nval, sizeof(*d), GFP_KERNEL);
+	if (!d)
+		return -ENOMEM;
+
+	for (i = 0; i < nval; i++) {
+		d[i] = kstrdup(src->pointer.str[i], GFP_KERNEL);
+		if (!d[i] && src->pointer.str[i]) {
+			while (--i >= 0)
+				kfree(d[i]);
+			kfree(d);
+			return -ENOMEM;
+		}
+	}
+
+	dst->pointer.str = (void *)d;
+	return 0;
+}
+
 static int property_entry_copy(struct property_entry *dst,
 			       const struct property_entry *src)
 {
-	const char **d, **s;
-	size_t i, nval;
+	int error;
 
 	dst->name = kstrdup(src->name, GFP_KERNEL);
 	if (!dst->name)
 		return -ENOMEM;
 
 	if (src->is_array) {
-		if (!src->length)
-			return -ENODATA;
+		if (!src->length) {
+			error = -ENODATA;
+			goto out_free_name;
+		}
 
 		if (src->is_string) {
-			nval = src->length / sizeof(const char *);
-			dst->pointer.str = kcalloc(nval, sizeof(const char *),
-						   GFP_KERNEL);
-			if (!dst->pointer.str)
-				return -ENOMEM;
-
-			d = dst->pointer.str;
-			s = src->pointer.str;
-			for (i = 0; i < nval; i++) {
-				d[i] = kstrdup(s[i], GFP_KERNEL);
Quoted 8 lines

drivers/base/property.c int fwnode_property_match_string(struct fwnode_handle *fwnode, · line 728

Quoted 5 lines
drivers/base/property.c +3 -2
-				if (!d[i] && s[i])
-					return -ENOMEM;
+			error = property_copy_string_array(dst, src);
+			if (error) {
+				error = -ENOMEM;

An unnecessary set as the return from the above
property_copy_string_array is either 0 or -ENOMEM

Even if it is not -ENOMEM, it still can be propagated to the caller.

I can fix this up unless Dmitry wants to send an updated patch.

Thanks,
Rafael

author
#6 lore raw

PATCH v4 3/4 device property: constify property arrays values

From: Dmitry Torokhov <[email protected]>

Data that is fed into property arrays should not be modified, so let's mark relevant pointers as const. This will allow us making source arrays as const/__initconst.

Also fix memory leaks on errors in property_entry_copy().

Signed-off-by Dmitry Torokhov <[email protected]>

---

Not sending the rest of the series as to not clutter mailing lists too much...

 drivers/base/property.c  |   66 +++++++++++++++++++++++++++++++---------------
 include/linux/property.h |   12 ++++----
 2 files changed, 51 insertions(+), 27 deletions(-)
drivers/base/property.c +45 -21
index edc09854520b..09fb9757e086 100644
@@ -682,44 +682,64 @@ int fwnode_property_match_string(struct fwnode_handle *fwnode,
 }
 EXPORT_SYMBOL_GPL(fwnode_property_match_string);
 
+static int property_copy_string_array(struct property_entry *dst,
+				      const struct property_entry *src)
+{
+	char **d;
+	size_t nval = src->length / sizeof(*d);
+	size_t i;
+
+	d = kcalloc(nval, sizeof(*d), GFP_KERNEL);
+	if (!d)
+		return -ENOMEM;
+
+	for (i = 0; i < nval; i++) {
+		d[i] = kstrdup(src->pointer.str[i], GFP_KERNEL);
+		if (!d[i] && src->pointer.str[i]) {
+			while (--i >= 0)
+				kfree(d[i]);
+			kfree(d);
+			return -ENOMEM;
+		}
+	}
+
+	dst->pointer.str = (void *)d;
+	return 0;
+}
+
 static int property_entry_copy(struct property_entry *dst,
 			       const struct property_entry *src)
 {
-	const char **d, **s;
-	size_t i, nval;
+	int error;
 
 	dst->name = kstrdup(src->name, GFP_KERNEL);
 	if (!dst->name)
 		return -ENOMEM;
 
 	if (src->is_array) {
-		if (!src->length)
-			return -ENODATA;
+		if (!src->length) {
+			error = -ENODATA;
+			goto out_free_name;
+		}
 
 		if (src->is_string) {
-			nval = src->length / sizeof(const char *);
-			dst->pointer.str = kcalloc(nval, sizeof(const char *),
-						   GFP_KERNEL);
-			if (!dst->pointer.str)
-				return -ENOMEM;
-
-			d = dst->pointer.str;
-			s = src->pointer.str;
-			for (i = 0; i < nval; i++) {
-				d[i] = kstrdup(s[i], GFP_KERNEL);
-				if (!d[i] && s[i])
-					return -ENOMEM;
-			}
+			error = property_copy_string_array(dst, src);
+			if (error)
+				goto out_free_name;
 		} else {
 			dst->pointer.raw_data = kmemdup(src->pointer.raw_data,
 							src->length, GFP_KERNEL);
-			if (!dst->pointer.raw_data)
-				return -ENOMEM;
+			if (!dst->pointer.raw_data) {
+				error = -ENOMEM;
+				goto out_free_name;
+			}
 		}
 	} else if (src->is_string) {
 		dst->value.str = kstrdup(src->value.str, GFP_KERNEL);
-		if (!dst->value.str && src->value.str)
-			return -ENOMEM;
+		if (!dst->value.str && src->value.str) {
+			error = -ENOMEM;
+			goto out_free_name;
+		}
 	} else {
 		dst->value.raw_data = src->value.raw_data;
 	}

@@ -729,6 +749,10 @@ static int property_entry_copy(struct property_entry *dst,
 	dst->is_string = src->is_string;
 
 	return 0;
+
+out_free_name:
+	kfree(dst->name);
+	return error;
 }
 
 /**
include/linux/property.h +6 -6
index 5746e9927016..64e3a9c6d95f 100644
@@ -160,12 +160,12 @@ struct property_entry {
 	bool is_string;
 	union {
 		union {
-			void *raw_data;
-			u8 *u8_data;
-			u16 *u16_data;
-			u32 *u32_data;
-			u64 *u64_data;
-			const char **str;
+			const void *raw_data;
+			const u8 *u8_data;
+			const u16 *u16_data;
+			const u32 *u32_data;
+			const u64 *u64_data;
+			const char * const *str;
 		} pointer;
 		union {
 			unsigned long long raw_data;
reviewer
#7 lore raw

Re: [PATCH v4 3/4] device property: constify property arrays values

Quoted 9 lines · Dmitry Torokhov

From: Dmitry Torokhov <[email protected]>

Data that is fed into property arrays should not be modified, so let's mark
relevant pointers as const. This will allow us making source arrays as const/__initconst.

Also fix memory leaks on errors in property_entry_copy().

While the code looks okay, I'm not sure what memory leaks you are referring to. The idea as far as I remember was to run *free() function if *copy() fails.

Quoted 142 lines
Signed-off-by Dmitry Torokhov <[email protected]>

---

Not sending the rest of the series as to not clutter mailing lists too much...

 drivers/base/property.c  |   66 +++++++++++++++++++++++++++++++----
-----------
 include/linux/property.h |   12 ++++----

 2 files changed, 51 insertions(+), 27 deletions(-)
drivers/base/property.c +0 -0
index edc09854520b..09fb9757e086 100644
@@ -682,44 +682,64 @@ int fwnode_property_match_string(struct
 fwnode_handle *fwnode,
  }
  EXPORT_SYMBOL_GPL(fwnode_property_match_string);

drivers/base/property.c +2 -0
+static int property_copy_string_array(struct property_entry *dst,
+				      const struct property_entry

*src)

drivers/base/property.c +23 -0
+{
+	char **d;
+	size_t nval = src->length / sizeof(*d);
+	size_t i;
+
+	d = kcalloc(nval, sizeof(*d), GFP_KERNEL);
+	if (!d)
+		return -ENOMEM;
+
+	for (i = 0; i < nval; i++) {
+		d[i] = kstrdup(src->pointer.str[i], GFP_KERNEL);
+		if (!d[i] && src->pointer.str[i]) {
+			while (--i >= 0)
+				kfree(d[i]);
+			kfree(d);
+			return -ENOMEM;
+		}
+	}
+
+	dst->pointer.str = (void *)d;
+	return 0;
+}
+

 static int property_entry_copy(struct property_entry *dst, const struct property_entry *src) {

drivers/base/property.c +1 -2
-	const char **d, **s;
-	size_t i, nval;
+	int error;


  dst->name = kstrdup(src->name, GFP_KERNEL); if (!dst->name)
  return -ENOMEM;

  if (src->is_array) {

drivers/base/property.c +4 -2
-		if (!src->length)
-			return -ENODATA;
+		if (!src->length) {
+			error = -ENODATA;
+			goto out_free_name;
+		}


  if (src->is_string) {

drivers/base/property.c +0 -2
-			nval = src->length / sizeof(const char *);
-			dst->pointer.str = kcalloc(nval, sizeof(const

char *),

drivers/base/property.c +3 -11
-						   GFP_KERNEL);
-			if (!dst->pointer.str)
-				return -ENOMEM;
-
-			d = dst->pointer.str;
-			s = src->pointer.str;
-			for (i = 0; i < nval; i++) {
-				d[i] = kstrdup(s[i], GFP_KERNEL);
-				if (!d[i] && s[i])
-					return -ENOMEM;
-			}
+			error = property_copy_string_array(dst, src);
+			if (error)
+				goto out_free_name;

  } else {
  dst->pointer.raw_data = kmemdup(src-

Quoted 1 line

pointer.raw_data,

  src->length, GFP_KERNEL);

drivers/base/property.c +4 -2
-			if (!dst->pointer.raw_data)
-				return -ENOMEM;
+			if (!dst->pointer.raw_data) {
+				error = -ENOMEM;
+				goto out_free_name;
+			}

  }
  } else if (src->is_string) {
  dst->value.str = kstrdup(src->value.str, GFP_KERNEL);

drivers/base/property.c +4 -2
-		if (!dst->value.str && src->value.str)
-			return -ENOMEM;
+		if (!dst->value.str && src->value.str) {
+			error = -ENOMEM;
+			goto out_free_name;
+		}

  } else {
  dst->value.raw_data = src->value.raw_data; }

drivers/base/property.c +0 -0
@@ -729,6 +749,10 @@ static int property_entry_copy(struct
 property_entry *dst,
  	dst->is_string = src->is_string;


  return 0;

drivers/base/property.c +4 -0
+
+out_free_name:
+	kfree(dst->name);
+	return error;

 }

 /**

include/linux/property.h +6 -6
index 5746e9927016..64e3a9c6d95f 100644
@@ -160,12 +160,12 @@ struct property_entry {
  	bool is_string;
  	union {
  		union {
-			void *raw_data;
-			u8 *u8_data;
-			u16 *u16_data;
-			u32 *u32_data;
-			u64 *u64_data;
-			const char **str;
+			const void *raw_data;
+			const u8 *u8_data;
+			const u16 *u16_data;
+			const u32 *u32_data;
+			const u64 *u64_data;
+			const char * const *str;
  		} pointer;
  		union {
  			unsigned long long raw_data;
Signature · 2 lines
Andy Shevchenko <[email protected]>
Intel Finland Oy
author
#8 lore raw

Re: [PATCH v4 3/4] device property: constify property arrays values

Quoted 8 lines · Andy Shevchenko
Quoted 3 lines · Dmitry Torokhov

From: Dmitry Torokhov <[email protected]>

Data that is fed into property arrays should not be modified, so

let's

Quoted 2 lines

mark
relevant pointers as const. This will allow us making source arrays

as

Quoted 8 lines
Quoted 4 lines

const/__initconst.

Also fix memory leaks on errors in property_entry_copy().

While the code looks okay, I'm not sure what memory leaks you are referring to. The idea as far as I remember was to run *free() function if *copy() fails.

That could have been OK for internal function, but will not work for public API, as it goes against normal pattern.

You will be old and grey and still correcting patches that would be getting it wrong :)

Thanks.

Signature · 1 line
Dmitry
reviewer
#9 lore raw

Re: [PATCH v4 3/4] device property: constify property arrays values

Quoted 22 lines · Dmitry Torokhov
Quoted 11 lines · Andy Shevchenko
Quoted 3 lines · Dmitry Torokhov

From: Dmitry Torokhov <[email protected]>

Data that is fed into property arrays should not be modified, so

let's

Quoted 3 lines

mark
relevant pointers as const. This will allow us making source arrays

as

Quoted 8 lines
Quoted 3 lines

const/__initconst.

Also fix memory leaks on errors in property_entry_copy().

While the code looks okay, I'm not sure what memory leaks you are referring to. The idea as far as I remember was to run *free() function
if *copy() fails.

Quoted 5 lines

That could have been OK for internal function, but will not work for public API, as it goes against normal pattern.

You will be old and grey and still correcting patches that would be getting it wrong :)

Yes, which sounds not exactly as "we have memory leaks and here we are fixing them". So, my comment regarding to phrasing of the commit message. Someone might mistakenly think that it needs to be ported as earlier as this had been introduced.

Signature · 2 lines
Andy Shevchenko <[email protected]>
Intel Finland Oy
author
#10 lore raw

Re: [PATCH v4 3/4] device property: constify property arrays values

Quoted 26 lines · Andy Shevchenko
Quoted 25 lines · Dmitry Torokhov
Quoted 11 lines · Andy Shevchenko
Quoted 3 lines · Dmitry Torokhov

From: Dmitry Torokhov <[email protected]>

Data that is fed into property arrays should not be modified, so

let's

Quoted 3 lines

mark
relevant pointers as const. This will allow us making source arrays

as

Quoted 8 lines
Quoted 3 lines

const/__initconst.

Also fix memory leaks on errors in property_entry_copy().

While the code looks okay, I'm not sure what memory leaks you are referring to. The idea as far as I remember was to run *free() function
if *copy() fails.

That could have been OK for internal function, but will not work for public API, as it goes against normal pattern.

Quoted 7 lines
Quoted 2 lines

You will be old and grey and still correcting patches that would be getting it wrong :)

Yes, which sounds not exactly as "we have memory leaks and here we are fixing them". So, my comment regarding to phrasing of the commit message. Someone might mistakenly think that it needs to be ported as earlier as this had been introduced.

OK, I'll leave it up to Rafael to massage the commit message as he sees fit.

Thanks.

Signature · 1 line
Dmitry
maintainer
#11 lore raw

Re: [PATCH v4 3/4] device property: constify property arrays values

Quoted 19 lines · Dmitry Torokhov
Quoted 18 lines · Andy Shevchenko
Quoted 17 lines · Dmitry Torokhov
Quoted 15 lines · Andy Shevchenko
Quoted 3 lines · Dmitry Torokhov

From: Dmitry Torokhov <[email protected]>

Data that is fed into property arrays should not be modified, so

let's

Quoted 3 lines

mark
relevant pointers as const. This will allow us making source arrays

as

Quoted 3 lines

const/__initconst.

Also fix memory leaks on errors in property_entry_copy().

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 4 lines

While the code looks okay, I'm not sure what memory leaks you are referring to. The idea as far as I remember was to run *free() function
if *copy() fails.

That could have been OK for internal function, but will not work for public API, as it goes against normal pattern.

But it is an internal function, isn't it?

Also its only caller does the right thing AFAICS.

Quoted 10 lines
Quoted 7 lines
Quoted 2 lines

You will be old and grey and still correcting patches that would be getting it wrong :)

Yes, which sounds not exactly as "we have memory leaks and here we are fixing them". So, my comment regarding to phrasing of the commit message. Someone might mistakenly think that it needs to be ported as earlier as this had been introduced.

OK, I'll leave it up to Rafael to massage the commit message as he sees fit.

To be precise, there are no memory leaks and this is just adding an unnecessary label along with some code around it, equally unnecessary.

Are you planning on making property_entry_copy() non-static?

Thanks,
Rafael

author
#12 lore raw

Re: [PATCH v4 3/4] device property: constify property arrays values

Quoted 26 lines · Rafael J. Wysocki
Quoted 24 lines · Dmitry Torokhov
Quoted 23 lines · Andy Shevchenko
Quoted 22 lines · Dmitry Torokhov
Quoted 19 lines · Andy Shevchenko
Quoted 3 lines · Dmitry Torokhov

From: Dmitry Torokhov <[email protected]>

Data that is fed into property arrays should not be modified, so

let's

Quoted 3 lines

mark
relevant pointers as const. This will allow us making source arrays

as

Quoted 3 lines

const/__initconst.

Also fix memory leaks on errors in property_entry_copy().

While the code looks okay, I'm not sure what memory leaks you are referring to. The idea as far as I remember was to run *free() function
if *copy() fails.

Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

That could have been OK for internal function, but will not work for public API, as it goes against normal pattern.

But it is an internal function, isn't it?

Also its only caller does the right thing AFAICS.

No, actually property_entries_dup() does not do the right thing anymore :(.

Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Quoted 3 lines

You will be old and grey and still correcting patches that would be getting it wrong :)

Yes, which sounds not exactly as "we have memory leaks and here we are fixing them". So, my comment regarding to phrasing of the commit message. Someone might mistakenly think that it needs to be ported as earlier as this had been introduced.

Quoted 7 lines
Quoted 2 lines

OK, I'll leave it up to Rafael to massage the commit message as he sees fit.

To be precise, there are no memory leaks and this is just adding an unnecessary label along with some code around it, equally unnecessary.

Are you planning on making property_entry_copy() non-static?

Maybe, but not yet. Still, I am uncomfortable with functions not cleaning up but rather requiring leaving failed property structure in such state that cleanup function will not crash on it. I think it is fragile and I'd rather rework it so we clean up on the spot.

Thanks.

Signature · 1 line
Dmitry
maintainer
#13 lore raw

Re: [PATCH v4 3/4] device property: constify property arrays values

Quoted 30 lines · Dmitry Torokhov
Quoted 29 lines · Rafael J. Wysocki
Quoted 26 lines · Dmitry Torokhov
Quoted 25 lines · Andy Shevchenko
Quoted 24 lines · Dmitry Torokhov
Quoted 11 lines · Andy Shevchenko
Quoted 3 lines · Dmitry Torokhov

From: Dmitry Torokhov <[email protected]>

Data that is fed into property arrays should not be modified, so

let's

Quoted 3 lines

mark
relevant pointers as const. This will allow us making source arrays

as

Quoted 8 lines
Quoted 3 lines

const/__initconst.

Also fix memory leaks on errors in property_entry_copy().

While the code looks okay, I'm not sure what memory leaks you are referring to. The idea as far as I remember was to run *free() function
if *copy() fails.

That could have been OK for internal function, but will not work for public API, as it goes against normal pattern.

Quoted 6 lines
Quoted 3 lines

But it is an internal function, isn't it?

Also its only caller does the right thing AFAICS.

No, actually property_entries_dup() does not do the right thing anymore :(.

Well, it looks like this is because of patch [1/4], so IMO the changes to clean up on errors in property_entry_copy() should be made in that patch as well.

Right now we seem to have potential memory leaks introduced in patch [1/4] and then fixed up in patch [3/4] in the same series which doesn't feel quite right to be honest.

Quoted 15 lines
Quoted 15 lines
Quoted 11 lines
Quoted 8 lines
Quoted 3 lines

You will be old and grey and still correcting patches that would be getting it wrong :)

Yes, which sounds not exactly as "we have memory leaks and here we are fixing them". So, my comment regarding to phrasing of the commit message. Someone might mistakenly think that it needs to be ported as earlier as this had been introduced.

OK, I'll leave it up to Rafael to massage the commit message as he sees fit.

To be precise, there are no memory leaks and this is just adding an unnecessary label along with some code around it, equally unnecessary.

Quoted 6 lines
Quoted 1 line

Are you planning on making property_entry_copy() non-static?

Maybe, but not yet. Still, I am uncomfortable with functions not cleaning up but rather requiring leaving failed property structure in such state that cleanup function will not crash on it. I think it is fragile and I'd rather rework it so we clean up on the spot.

Fair enough, but that should happen in patch [1/4] already IMO.

Thanks,
Rafael

author
#14 lore raw

Re: [PATCH v4 3/4] device property: constify property arrays values

Quoted 39 lines · Rafael J. Wysocki
Quoted 30 lines · Dmitry Torokhov
Quoted 29 lines · Rafael J. Wysocki
Quoted 26 lines · Dmitry Torokhov
Quoted 25 lines · Andy Shevchenko
Quoted 24 lines · Dmitry Torokhov
Quoted 11 lines · Andy Shevchenko
Quoted 3 lines · Dmitry Torokhov

From: Dmitry Torokhov <[email protected]>

Data that is fed into property arrays should not be modified, so

let's

Quoted 3 lines

mark
relevant pointers as const. This will allow us making source arrays

as

Quoted 8 lines
Quoted 3 lines

const/__initconst.

Also fix memory leaks on errors in property_entry_copy().

While the code looks okay, I'm not sure what memory leaks you are referring to. The idea as far as I remember was to run *free() function
if *copy() fails.

That could have been OK for internal function, but will not work for public API, as it goes against normal pattern.

Quoted 6 lines
Quoted 3 lines

But it is an internal function, isn't it?

Also its only caller does the right thing AFAICS.

No, actually property_entries_dup() does not do the right thing anymore :(.

Quoted 7 lines

Well, it looks like this is because of patch [1/4], so IMO the changes to clean up on errors in property_entry_copy() should be made in that patch as well.

Right now we seem to have potential memory leaks introduced in patch [1/4] and then fixed up in patch [3/4] in the same series which doesn't feel quite right to be honest.

Totally agree, I'm reshuffling and will repost the series in a few.

Signature · 1 line
Dmitry
Reviewed-by on v3 3/4
#15 lore raw

Re: [PATCH v3 3/4] driver property: constify property arrays values

In reply to Dmitry Torokhov
Quoted 7 lines · Dmitry Torokhov

Data that is fed into property arrays should not be modified, so let's mark relevant pointers as const. This will allow us making source arrays as const/__initconst.

Also fix memory leaks on errors in property_entry_copy().

Signed-off-by Dmitry Torokhov <[email protected]>
Reviewed-by Mika Westerberg <[email protected]>
author
#16 lore raw

PATCH v3 4/4 i2c: allow specify device properties in i2c_board_info

In reply to Dmitry Torokhov

With many drivers converting to using generic device properties, it is useful to provide array of device properties when instantiating new i2c client via i2c_board_info and have them automatically added to the device in question.

Signed-off-by Dmitry Torokhov <[email protected]>
---
 drivers/i2c/i2c-core.c | 16 +++++++++++++++-
 include/linux/i2c.h    |  3 +++
 2 files changed, 18 insertions(+), 1 deletion(-)
drivers/i2c/i2c-core.c +15 -1
index f009549f86a5..3897e78e5e9a 100644
@@ -1335,15 +1335,29 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
 	client->dev.fwnode = info->fwnode;
 
 	i2c_dev_set_name(adap, client);
+
+	if (info->properties) {
+		status = device_add_properties(&client->dev, info->properties);
+		if (status) {
+			dev_err(&adap->dev,
+				"Failed to add properties to client %s: %d\n",
+				client->name, status);
+			goto out_err;
+		}
+	}
+
 	status = device_register(&client->dev);
 	if (status)
-		goto out_err;
+		goto out_free_props;
 
 	dev_dbg(&adap->dev, "client [%s] registered with bus id %s\n",
 		client->name, dev_name(&client->dev));
 
 	return client;
 
+out_free_props:
+	if (info->properties)
+		device_remove_properties(&client->dev);
 out_err:
 	dev_err(&adap->dev,
 		"Failed to register i2c client %s at 0x%02x (%d)\n",
include/linux/i2c.h +3 -0
index 4aa7d244b38a..ec8f866a5656 100644
@@ -51,6 +51,7 @@ enum i2c_slave_event;
 typedef int (*i2c_slave_cb_t)(struct i2c_client *, enum i2c_slave_event, u8 *);
 
 struct module;
+struct property_entry;
 
 #if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
 /*

@@ -310,6 +311,7 @@ static inline int i2c_slave_event(struct i2c_client *client,
  * @archdata: copied into i2c_client.dev.archdata
  * @of_node: pointer to OpenFirmware device node
  * @fwnode: device node supplied by the platform firmware
+ * @properties: additional device properties for the device
  * @irq: stored in i2c_client.irq
  *
  * I2C doesn't actually support hardware probing, although controllers and

@@ -331,6 +333,7 @@ struct i2c_board_info {
 	struct dev_archdata	*archdata;
 	struct device_node *of_node;
 	struct fwnode_handle *fwnode;
+	const struct property_entry *properties;
 	int		irq;
 };
Signature · 1 line
2.11.0.483.g087da7b7c-goog
Reviewed-by on v3 4/4
#17 lore raw

Re: [PATCH v3 4/4] i2c: allow specify device properties in i2c_board_info

Quoted 6 lines · Dmitry Torokhov

With many drivers converting to using generic device properties, it is useful to provide array of device properties when instantiating new i2c client via i2c_board_info and have them automatically added to the device in question.

Signed-off-by Dmitry Torokhov <[email protected]>
Reviewed-by Mika Westerberg <[email protected]>
author
#18 lore raw
In reply to Dmitry Torokhov
Quoted 9 lines · Dmitry Torokhov

Hi,

Here is the refreshed series exporting APIs to copy statically declared device properties. The reason is that we want to augment ACPI-based devices with properties, and drivers usually have a largish DMI table for multiple models, so it is desirable to mark everything as __initdata/__initconst, and then copy only the entry matching the device we are running on and discard the rest.

Quoted 7 lines

The last patch is not really about device property APIs, but rather allowing users to attach properties to i2c_board_info, and have them attached to instantiated device(s). The reason it is included is because it depends on device_add_properties() taking const pointer, which is patch #2.

If it seems useful I hope Rafael and Wolfram would figure a way to merge it :).

By the way, if this series is accepted, it would be very helpful to have it in a stable branch off 4.10-rcX as I have more patches to platform/chrome and Atmel touchscreen driver depending on it.

Quoted 32 lines

v3:
- fix memory leak in property_copy_string_array() pointed out by Mika Westerberg

v2:
- addressed Andy's comments
- added property_entries_free()
- added patch to allow constify values of property arrays
- added i2c patch allowing to attach property to devices via board info

v1:
- initial posting

Dmitry Torokhov (4):
device property: export code duplicating array of property entries device property: allow to constify properties
driver property: constify property arrays values
i2c: allow specify device properties in i2c_board_info

 drivers/base/property.c  | 222 ++++++++++++++++++++++++++++++-----------------
 drivers/i2c/i2c-core.c   |  16 +++-
 include/linux/i2c.h      |   3 +
 include/linux/property.h |  19 ++--
 4 files changed, 171 insertions(+), 89 deletions(-)

Thanks.

Signature · 1 line
Dmitry
Signature · 1 line
Dmitry