device property: export code duplicating array of property entries

PATCH Superseded Dmitry Torokhov 4 participants 8 messages Jan 23, 2017 – Jan 30, 2017

Subsystems (2)

Driver core Software nodes and device properties

lore

Share & tools

lore

Dmitry Torokhov

author

2017-01-23 07:38 UTC #0 lore raw

PATCH 1/2 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 <[email protected]>

---
 drivers/base/property.c  | 124 +++++++++++++++++++++++++++--------------------
 include/linux/property.h |   3 ++
 2 files changed, 75 insertions(+), 52 deletions(-)

drivers/base/property.c +72 ~~-52~~

index 43a36d68c3fd..ee22ded63e06 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,71 @@ 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);
+
+/**
+ * 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);
+}
+
+/**
  * pset_copy_set - copies property set
  * @pset: Property set to copy
  *

@@ -776,32 +808,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 +3 -0

index 856e50b2140c..f7fa5891a8c3 100644
@@ -241,6 +241,9 @@ struct property_entry {
 	.name = _name_,				\
 }
 
+struct property_entry *property_entries_dup(
+				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

Dmitry Torokhov

author

2017-01-23 07:39 UTC #1 lore raw

PATCH 2/2 device property: allow 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  | 35 ++++++++++++++++++-----------------
 include/linux/property.h |  2 +-
 2 files changed, 19 insertions(+), 18 deletions(-)

drivers/base/property.c +18 ~~-17~~

index ee22ded63e06..4cbf99cb8ef6 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)

@@ -867,7 +867,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 f7fa5891a8c3..3aeb4d7acd68 100644
@@ -245,7 +245,7 @@ struct property_entry *property_entries_dup(
 				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

Mika Westerberg

Reviewed-by on v1 2/2

2017-01-23 14:40 UTC #2 lore raw

Re: [PATCH 2/2] device property: allow constify properties

Quoted 8 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.
Signed-off-by Dmitry Torokhov <[email protected]>

Reviewed-by Mika Westerberg <[email protected]>

Andy Shevchenko

reviewer Reviewed-by on v1 2/2

2017-01-23 15:07 UTC #3 lore raw

Re: [PATCH 2/2] device property: allow constify properties

In reply to Dmitry Torokhov

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.

Reviewed-by Andy Shevchenko <[email protected]>

Quoted 156 lines

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 +0 -0
index ee22ded63e06..4cbf99cb8ef6 100644
@@ -21,7 +21,7 @@
struct property_set {
struct fwnode_handle fwnode;
drivers/base/property.c +1 -1
-	struct property_entry *properties;
+	const struct property_entry *properties;
};

static inline bool is_pset_node(struct fwnode_handle *fwnode)
drivers/base/property.c +0 -0
@@ -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,
drivers/base/property.c +1 -1
-					    const char *name)
+static const struct property_entry *pset_prop_get(struct property_set
*pset,
drivers/base/property.c +1 -0
+						  const char *name)
{
drivers/base/property.c +1 -1
-	struct property_entry *prop;
+	const struct property_entry *prop;
if (!pset || !pset->properties)
return NULL;
drivers/base/property.c +0 -0
@@ -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,
drivers/base/property.c +2 -1
-			    size_t length)
+static const void *pset_prop_find(struct property_set *pset,
+				  const char *propname, size_t
length)
{
drivers/base/property.c +2 -2
-	struct property_entry *prop;
-	void *pointer;
+	const struct property_entry *prop;
+	const void *pointer;
prop = pset_prop_get(pset, propname);
if (!prop)
drivers/base/property.c +1 -1
@@ -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);
drivers/base/property.c +1 -1
@@ -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);
drivers/base/property.c +1 -1
@@ -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);
drivers/base/property.c +1 -1
@@ -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);
drivers/base/property.c +0 -0
@@ -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)
{
drivers/base/property.c +1 -1
-	struct property_entry *prop;
+	const struct property_entry *prop;
prop = pset_prop_get(pset, propname);
if (!prop)
drivers/base/property.c +0 -0
@@ -146,7 +146,7 @@ static int pset_prop_read_string_array(struct
 property_set *pset,
  				       const char *propname,
const char **strings, size_t nval)
{
drivers/base/property.c +1 -1
-	void *pointer;
+	const void *pointer;
size_t length = nval * sizeof(*strings);
pointer = pset_prop_find(pset, propname, length);
drivers/base/property.c +0 -0
@@ -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)
{
drivers/base/property.c +2 -2
-	struct property_entry *prop;
-	const char **pointer;
+	const struct property_entry *prop;
+	const char * const *pointer;
prop = pset_prop_get(pset, propname);
if (!prop)
drivers/base/property.c +2 -1
@@ -867,7 +867,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 +0 -0
index f7fa5891a8c3..3aeb4d7acd68 100644
@@ -245,7 +245,7 @@ struct property_entry *property_entries_dup(
const struct property_entry *properties);

int device_add_properties(struct device *dev,
include/linux/property.h +1 -1
-			  struct property_entry *properties);
+			  const struct property_entry *properties);
void device_remove_properties(struct device *dev);
bool device_dma_supported(struct device *dev);

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Mika Westerberg

Reviewed-by on v1 1/2

2017-01-23 14:39 UTC #4 lore raw

In reply to Dmitry Torokhov

Quoted 8 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.
Signed-off-by Dmitry Torokhov <[email protected]>

I think this is good idea, so please feel free to add my,

Reviewed-by Mika Westerberg <[email protected]>

Andy Shevchenko

reviewer Reviewed-by on v1 1/2

2017-01-23 15:02 UTC #5 lore raw

In reply to Dmitry Torokhov

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.

Looks good to me.

Reviewed-by Andy Shevchenko <[email protected]>

Couple of style nitpicks.

Quoted 4 lines

(quoted diff) +2 -0

+struct property_entry *property_entries_dup(
+				const struct property_entry

*properties)

Can we use

struct propert_entry *
property_entries_dup(...)

Quoted 4 lines

(quoted diff) +2 -0

+struct property_entry *property_entries_dup(
+				const struct property_entry

*properties);

Ditto.

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Dmitry Torokhov

author

2017-01-23 22:46 UTC #6 lore raw

Quoted 25 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.
Looks good to me.
Reviewed-by Andy Shevchenko <[email protected]>
Couple of style nitpicks.
Quoted 4 lines
(quoted diff) +2 -0
+struct property_entry *property_entries_dup(
+				const struct property_entry
*properties)
Can we use

Quoted 4 lines

struct propert_entry *
property_entries_dup(...)
?

Sure, will adjust. I also realized we'll need property_entries_free() for proper cleanups. I'll repost the series.

Quoted 12 lines

Quoted 6 lines
(quoted diff) +2 -0
+struct property_entry *property_entries_dup(
+				const struct property_entry
*properties);
Ditto.
Signature · 2 lines
Andy Shevchenko <[email protected]>
Intel Finland Oy

Signature · 1 line

Dmitry

Rafael J. Wysocki

2017-01-30 22:48 UTC #7 lore raw

Quoted 23 lines · Dmitry Torokhov

Quoted 22 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.
Looks good to me.
Reviewed-by Andy Shevchenko <[email protected]>
Couple of style nitpicks.
Quoted 3 lines
(quoted diff) +2 -0
+struct property_entry *property_entries_dup(
+				const struct property_entry
*properties)
Can we use

Quoted 6 lines

Quoted 4 lines
struct propert_entry *
property_entries_dup(...)
?
Sure, will adjust. I also realized we'll need property_entries_free() for proper cleanups. I'll repost the series.

Can you please CC it to linux-acpi while at that? It will help to handle it.

Thanks,
Rafael