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

Groups > linux.kernel > #1572846 > unrolled thread

[PATCH v5 0/4] Export APIs to copy device properties & more

Back to article view | Back to linux.kernel

Contents

  [PATCH v5 0/4] Export APIs to copy device properties & more Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-03 02:50 +0100
    [PATCH v5 3/4] device property: export code duplicating array of property entries Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-03 02:50 +0100
      Re: [PATCH v5 3/4] device property: export code duplicating array  of property entries Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-02-03 12:50 +0100
        Re: [PATCH v5 3/4] device property: export code duplicating array of  property entries Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-03 16:20 +0100
    Re: [PATCH v5 0/4] Export APIs to copy device properties & more "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-02-09 15:00 +0100

#1572846 — [PATCH v5 0/4] 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(-)

-- 
Dmitry

[toc] | [next] | [standalone]

#1572848 — [PATCH v5 3/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.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/property.c  | 194 +++++++++++++++++++++++++++++++----------------
 include/linux/property.h |   5 ++
 2 files changed, 134 insertions(+), 65 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 31b942a29fdc..c458c63e353f 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -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;
 }
 
diff --git a/include/linux/property.h b/include/linux/property.h
index 7a0a1cce5165..64e3a9c6d95f 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -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);
-- 
2.11.0.483.g087da7b7c-goog

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

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

On Thu, 2017-02-02 at 17:41 -0800, Dmitry Torokhov wrote:
> 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.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

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

On Fri, Feb 03, 2017 at 01:45:30PM +0200, Andy Shevchenko wrote:
> On Thu, 2017-02-02 at 17:41 -0800, Dmitry Torokhov wrote:
> > 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.

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.

-- 
Dmitry

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

#1577612

On Thursday, February 02, 2017 05:41:24 PM Dmitry Torokhov wrote:
> 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

All [1-4/4] applied.

Thanks,
Rafael

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web