Export APIs to copy device properties & more

PATCH In review Dmitry Torokhov 5 participants 15 messages Feb 3, 2017 – Feb 9, 2017

Subsystems (4)

ACPI Driver core I2C Software nodes and device properties

lore

Share & tools

lore

Dmitry Torokhov

author

2017-02-03 01:41 UTC #0 lore raw

v5 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 #1.

If it seems useful I hope Rafael and Wolfram would figure a way to merge it :) and ideally give me a stable branch as I have more patches to platform/chrome and Atmel touchscreen driver depending on them.

v5:
- reshuffle order of patches so that "constness" ones are first
- factor out property_entry_free_data() and make sure we call it for properties already successfully copied when property_entry_copy_data() in property_entries_dup() fails
- dropped old Acks as patches changed enough

v4:
- do not clobber retrun value of property_copy_string_array() with -ENOMEM

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: allow to constify properties
device property: constify property arrays values
device property: export code duplicating array of property entries
i2c: allow specify device properties in i2c_board_info

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

Signature · 1 line

Dmitry

Dmitry Torokhov

author

2017-02-03 01:41 UTC #1 lore raw

PATCH v5 1/4 device property: allow to constify properties

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.

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

---
 drivers/base/property.c  | 41 +++++++++++++++++++++--------------------
 include/linux/property.h |  2 +-
 2 files changed, 22 insertions(+), 21 deletions(-)

drivers/base/property.c +21 ~~-20~~

index 43a36d68c3fd..e9fa75645d69 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)

@@ -776,7 +776,7 @@ 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 *props;
 	struct property_set *p;
 	size_t i, n = 0;

@@ -787,14 +787,14 @@ static struct property_set *pset_copy_set(const struct property_set *pset)
 	while (pset->properties[n].name)
 		n++;
 
-	p->properties = kcalloc(n + 1, sizeof(*entry), GFP_KERNEL);
+	p->properties = props = kcalloc(n + 1, sizeof(*props), GFP_KERNEL);
 	if (!p->properties) {
 		kfree(p);
 		return ERR_PTR(-ENOMEM);
 	}
 
 	for (i = 0; i < n; i++) {
-		int ret = pset_copy_entry(&p->properties[i],
+		int ret = pset_copy_entry(&props[i],
 					  &pset->properties[i]);
 		if (ret) {
 			pset_free_set(p);

@@ -847,7 +847,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 856e50b2140c..d37a4498b3ac 100644
@@ -242,7 +242,7 @@ struct property_entry {
 }
 
 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

Andy Shevchenko

reviewer Reviewed-by on v5 1/4

2017-02-03 11:40 UTC #2 lore raw

Re: [PATCH v5 1/4] device property: allow to constify properties

Quoted 10 lines · 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.

Looks good to me.

FWIW:

Reviewed-by Andy Shevchenko <[email protected]>

Though, nitpicks below.

drivers/base/property.c static int pset_copy_entry(struct property_entry *dst, · line 779

Quoted 5 lines

static struct property_set *pset_copy_set(const struct property_set *pset)
{
drivers/base/property.c +1 -1
-	const struct property_entry *entry;
+	struct property_entry *props;

Can we leave the name?

Quoted 13 lines

drivers/base/property.c +1 -1
-	p->properties = kcalloc(n + 1, sizeof(*entry), GFP_KERNEL);
 
+	p->properties = props = kcalloc(n + 1, sizeof(*props),
GFP_KERNEL);
if (!p->properties) {
kfree(p);
return ERR_PTR(-ENOMEM);
}

for (i = 0; i < n; i++) {
drivers/base/property.c +1 -1
-		int ret = pset_copy_entry(&p->properties[i],
+		int ret = pset_copy_entry(&props[i],

Quoted 1 line

&pset->properties[i]);

Do we need these changes?

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Dmitry Torokhov

author

2017-02-03 15:09 UTC #3 lore raw

Re: [PATCH v5 1/4] device property: allow to constify properties

Quoted 38 lines · Andy Shevchenko

Quoted 10 lines · 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.
Looks good to me.
FWIW:
Reviewed-by Andy Shevchenko <[email protected]>
Though, nitpicks below.
drivers/base/property.c static int pset_copy_entry(struct property_entry *dst, · line 779
Quoted 5 lines
static struct property_set *pset_copy_set(const struct property_set *pset)
{
drivers/base/property.c +1 -1
-	const struct property_entry *entry;
+	struct property_entry *props;
Can we leave the name?
Quoted 10 lines
drivers/base/property.c +1 -1
-	p->properties = kcalloc(n + 1, sizeof(*entry), GFP_KERNEL);
 
+	p->properties = props = kcalloc(n + 1, sizeof(*props),
GFP_KERNEL);
if (!p->properties) {
kfree(p);
return ERR_PTR(-ENOMEM);
}

for (i = 0; i < n; i++) {

Quoted 5 lines

Quoted 3 lines
drivers/base/property.c +1 -1
-		int ret = pset_copy_entry(&p->properties[i],
+		int ret = pset_copy_entry(&props[i],
&pset->properties[i]);
Do we need these changes?

Didn't want to wrap the line, the name is restored later anyway.

Thanks.

Signature · 1 line

Dmitry

Dmitry Torokhov

author

2017-02-03 01:41 UTC #4 lore raw

PATCH v5 2/4 device 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.

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

---
 drivers/base/property.c  | 10 +++++-----
 include/linux/property.h | 12 ++++++------
 2 files changed, 11 insertions(+), 11 deletions(-)

drivers/base/property.c +5 -5

index e9fa75645d69..31b942a29fdc 100644
@@ -718,7 +718,8 @@ static void pset_free_set(struct property_set *pset)
 static int pset_copy_entry(struct property_entry *dst,
 			   const struct property_entry *src)
 {
-	const char **d, **s;
+	const char * const *s;
+	char **d;
 	size_t i, nval;
 
 	dst->name = kstrdup(src->name, GFP_KERNEL);

@@ -731,12 +732,11 @@ static int pset_copy_entry(struct property_entry *dst,
 
 		if (src->is_string) {
 			nval = src->length / sizeof(const char *);
-			dst->pointer.str = kcalloc(nval, sizeof(const char *),
-						   GFP_KERNEL);
-			if (!dst->pointer.str)
+			d = kcalloc(nval, sizeof(const char *), GFP_KERNEL);
+			if (!d)
 				return -ENOMEM;
 
-			d = dst->pointer.str;
+			dst->pointer.raw_data = d;
 			s = src->pointer.str;
 			for (i = 0; i < nval; i++) {
 				d[i] = kstrdup(s[i], GFP_KERNEL);

include/linux/property.h +6 -6

index d37a4498b3ac..7a0a1cce5165 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

Andy Shevchenko

reviewer

2017-02-03 11:43 UTC #5 lore raw

Re: [PATCH v5 2/4] device property: constify property arrays values

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.

drivers/base/property.c static void pset_free_set(struct property_set · line 723

Quoted 7 lines

drivers/base/property.c +2 -1

@@ -718,7 +718,8 @@ static void pset_free_set(struct property_set
 *pset)
  static int pset_copy_entry(struct property_entry *dst,
  			   const struct property_entry *src)
  {
-	const char **d, **s;
+	const char * const *s;
+	char **d;

You removed const here

Quoted 13 lines

drivers/base/property.c +0 -0
  	size_t i, nval;
dst->name = kstrdup(src->name, GFP_KERNEL);
(quoted diff) +0 -0
@@ -731,12 +732,11 @@ static int pset_copy_entry(struct property_entry
 *dst,
if (src->is_string) {
nval = src->length / sizeof(const char *);
(quoted diff) +0 -1
-			dst->pointer.str = kcalloc(nval, sizeof(const
char *),
drivers/base/property.c +1 -2
-						   GFP_KERNEL);
-			if (!dst->pointer.str)
+			d = kcalloc(nval, sizeof(const char *),

Quoted 1 line

GFP_KERNEL);

But left it here. Do we need to remove const?

Quoted 6 lines

drivers/base/property.c +1 -0
+			if (!d)
return -ENOMEM;
drivers/base/property.c +1 -1
-			d = dst->pointer.str;
+			dst->pointer.raw_data = d;
s = src->pointer.str;

So, overall, do we need these changes at all? Nothing in commit message sheds a light on it.

Quoted 2 lines

for (i = 0; i < nval; i++) { d[i] = kstrdup(s[i], GFP_KERNEL);

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Dmitry Torokhov

author

2017-02-03 15:12 UTC #6 lore raw

Re: [PATCH v5 2/4] device property: constify property arrays values

Quoted 7 lines · Andy Shevchenko

Quoted 5 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.

Quoted 10 lines

drivers/base/property.c static void pset_free_set(struct property_set · line 723
Quoted 8 lines
drivers/base/property.c +2 -1
@@ -718,7 +718,8 @@ static void pset_free_set(struct property_set
 *pset)
  static int pset_copy_entry(struct property_entry *dst,
  			   const struct property_entry *src)
  {
-	const char **d, **s;
+	const char * const *s;
+	char **d;
You removed const here

Yes I did. It is hard to assign value to a constant otherwise.

Quoted 17 lines

Quoted 14 lines
drivers/base/property.c +0 -0
  	size_t i, nval;
dst->name = kstrdup(src->name, GFP_KERNEL);
(quoted diff) +0 -0
@@ -731,12 +732,11 @@ static int pset_copy_entry(struct property_entry
 *dst,
if (src->is_string) {
nval = src->length / sizeof(const char *);
(quoted diff) +0 -1
-			dst->pointer.str = kcalloc(nval, sizeof(const
char *),
drivers/base/property.c +1 -2
-						   GFP_KERNEL);
-			if (!dst->pointer.str)
+			d = kcalloc(nval, sizeof(const char *),
Quoted 1 line
GFP_KERNEL);
But left it here. Do we need to remove const?

I do not know why we had it in the first place: the size is the samei between constant and variable of the same type.

Ideally we'd use sizeof(*d), I can do it after this batch is accepted.

Quoted 10 lines

Quoted 7 lines
drivers/base/property.c +1 -0
+			if (!d)
return -ENOMEM;
drivers/base/property.c +1 -1
-			d = dst->pointer.str;
+			dst->pointer.raw_data = d;
s = src->pointer.str;
So, overall, do we need these changes at all? Nothing in commit message sheds a light on it.

The compiler insists in them though.

Quoted 3 lines

Quoted 3 lines
for (i = 0; i < nval; i++) { d[i] = kstrdup(s[i], GFP_KERNEL);

Thanks.

Signature · 1 line

Dmitry

Dmitry Torokhov

author

2017-02-03 01:41 UTC #7 lore raw

PATCH v5 3/4 device property: export code duplicating array of property entries

In reply to Dmitry Torokhov

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.

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

---
 drivers/base/property.c  | 194 +++++++++++++++++++++++++++++++----------------
 include/linux/property.h |   5 ++
 2 files changed, 134 insertions(+), 65 deletions(-)

drivers/base/property.c +129 ~~-65~~

index 31b942a29fdc..c458c63e353f 100644
@@ -682,77 +682,64 @@ 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)
+static int property_copy_string_array(struct property_entry *dst,
+				      const struct property_entry *src)
 {
-	const struct property_entry *prop;
-	size_t i, nval;
+	char **d;
+	size_t nval = src->length / sizeof(*d);
+	int i;
 
-	if (!pset)
-		return;
+	d = kcalloc(nval, sizeof(*d), GFP_KERNEL);
+	if (!d)
+		return -ENOMEM;
 
-	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);
+	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;
 		}
-		kfree(prop->name);
 	}
 
-	kfree(pset->properties);
-	kfree(pset);
+	dst->pointer.raw_data = d;
+	return 0;
 }
 
-static int pset_copy_entry(struct property_entry *dst,
-			   const struct property_entry *src)
+static int property_entry_copy_data(struct property_entry *dst,
+				    const struct property_entry *src)
 {
-	const char * const *s;
-	char **d;
-	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 *);
-			d = kcalloc(nval, sizeof(const char *), GFP_KERNEL);
-			if (!d)
-				return -ENOMEM;
-
-			dst->pointer.raw_data = d;
-			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;
 	}

@@ -762,6 +749,95 @@ static int pset_copy_entry(struct property_entry *dst,
 	dst->is_string = src->is_string;
 
 	return 0;
+
+out_free_name:
+	kfree(dst->name);
+	return error;
+}
+
+static void property_entry_free_data(const struct property_entry *p)
+{
+	size_t i, nval;
+
+	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);
+}
+
+/**
+ * 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_data(&p[i], &properties[i]);
+		if (ret) {
+			while (--i >= 0)
+				property_entry_free_data(&p[i]);
+			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;
+
+	for (p = properties; p->name; p++)
+		property_entry_free_data(p);
+
+	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);
 }
 
 /**

@@ -776,32 +852,20 @@ static int pset_copy_entry(struct property_entry *dst,
  */
 static struct property_set *pset_copy_set(const struct property_set *pset)
 {
-	struct property_entry *props;
+	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 = props = kcalloc(n + 1, sizeof(*props), 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(&props[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 7a0a1cce5165..64e3a9c6d95f 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,
 			  const struct property_entry *properties);
 void device_remove_properties(struct device *dev);

Signature · 1 line

2.11.0.483.g087da7b7c-goog

Andy Shevchenko

reviewer

2017-02-03 11:48 UTC #8 lore raw

Re: [PATCH v5 3/4] device property: export code duplicating array of property entries

Quoted 10 lines · Dmitry Torokhov

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.

Here you again rewrote the code you rewrote in previous patch. Please, remove those hunks in previous patch and rebase this one on top of the result.

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Dmitry Torokhov

author

2017-02-03 15:16 UTC #9 lore raw

Re: [PATCH v5 3/4] device property: export code duplicating array of property entries

Quoted 12 lines · Andy Shevchenko

Quoted 10 lines · Dmitry Torokhov
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.

Quoted 3 lines

Here you again rewrote the code you rewrote in previous patch. Please, remove those hunks in previous patch and rebase this one on top of the result.

I do no see the point really. If I am not exposing the new APIs then there is no point in touching the code. If I am creating new API then I do not want to go through contortion of producing something similar to the old code flow just to "fix" it in the subsequent patch.

Thanks.

Signature · 1 line

Dmitry

Dmitry Torokhov

author

2017-02-03 01:41 UTC #10 lore raw

PATCH v5 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

Andy Shevchenko

reviewer Reviewed-by on v5 4/4

2017-02-03 11:46 UTC #11 lore raw

Re: [PATCH v5 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.

FWIW:

Reviewed-by Andy Shevchenko <[email protected]>

Quoted 77 lines

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 +0 -0
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);
drivers/i2c/i2c-core.c +3 -0
+
+	if (info->properties) {
+		status = device_add_properties(&client->dev, info-
Quoted 1 line
properties);
drivers/i2c/i2c-core.c +3 -0
+		if (status) {
+			dev_err(&adap->dev,
+				"Failed to add properties to client
%s: %d\n",
drivers/i2c/i2c-core.c +5 -0
+				client->name, status);
+			goto out_err;
+		}
+	}
+
status = device_register(&client->dev);
if (status)
drivers/i2c/i2c-core.c +1 -1
-		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;
drivers/i2c/i2c-core.c +3 -0
+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 +0 -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) /*
include/linux/i2c.h +1 -0
@@ -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
include/linux/i2c.h +1 -0
@@ -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 · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Rafael J. Wysocki

maintainer

2017-02-07 12:44 UTC #12 lore raw

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

In reply to Dmitry Torokhov

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]>

Hi Wolfram,

Any objections here?

Quoted 61 lines

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

include/linux/i2c.h struct i2c_board_info {

Quoted 8 lines

include/linux/i2c.h +1 -1

@@ -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;
 };
 
--

Thanks,
Rafael

Wolfram Sang

Acked-by on v5 4/4

2017-02-07 13:25 UTC #13 lore raw

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

In reply to Dmitry Torokhov

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]>

Acked-by Wolfram Sang <[email protected]>

Rafael J. Wysocki

2017-02-09 13:56 UTC #14 lore raw

In reply to Dmitry Torokhov

Quoted 18 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.
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 #1.
If it seems useful I hope Rafael and Wolfram would figure a way to merge it :) and ideally give me a stable branch as I have more patches to platform/chrome and Atmel touchscreen driver depending on them.

Quoted 6 lines

v5:
- reshuffle order of patches so that "constness" ones are first
- factor out property_entry_free_data() and make sure we call it for properties already successfully copied when property_entry_copy_data() in property_entries_dup() fails
- dropped old Acks as patches changed enough

All [1-4/4] applied.

Thanks,
Rafael