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

Groups > linux.kernel > #1571819 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 0/4] Export APIs to copy device properties & more Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-01 18:40 +0100
    [PATCH v3 3/4] driver property: constify property arrays values Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-01 18:40 +0100
      Re: [PATCH v3 3/4] driver property: constify property arrays values Joe Perches <joe@perches.com> - 2017-02-02 00:40 +0100
        Re: [PATCH v3 3/4] driver property: constify property arrays values "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-02-02 12:30 +0100
          [PATCH v4 3/4] device property: constify property arrays values Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-02 17:40 +0100
            Re: [PATCH v4 3/4] device property: constify property arrays values Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-02-02 18:00 +0100
              Re: [PATCH v4 3/4] device property: constify property arrays values Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-02 18:10 +0100
                Re: [PATCH v4 3/4] device property: constify property arrays values Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-02-02 19:00 +0100
                  Re: [PATCH v4 3/4] device property: constify property arrays values Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-02 20:00 +0100
                    Re: [PATCH v4 3/4] device property: constify property arrays values "Rafael J. Wysocki" <rafael@kernel.org> - 2017-02-03 00:20 +0100
                      Re: [PATCH v4 3/4] device property: constify property arrays values Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-03 01:20 +0100
                        Re: [PATCH v4 3/4] device property: constify property arrays values "Rafael J. Wysocki" <rafael@kernel.org> - 2017-02-03 01:40 +0100
                          Re: [PATCH v4 3/4] device property: constify property arrays values Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-03 02:10 +0100
      Re: [PATCH v3 3/4] driver property: constify property arrays values Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-02-02 11:10 +0100
    [PATCH v3 2/4] device property: allow to constify properties Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-01 18:40 +0100
    Re: [PATCH v3 0/4] Export APIs to copy device properties & more Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-02 00:00 +0100

#1571819 — [PATCH v3 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 #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.

-- 
Dmitry

[toc] | [next] | [standalone]

#1571823 — [PATCH v3 3/4] driver property: constify property arrays values

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 <dmitry.torokhov@gmail.com>
---
 drivers/base/property.c  | 66 +++++++++++++++++++++++++++++++++---------------
 include/linux/property.h | 12 ++++-----
 2 files changed, 52 insertions(+), 26 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index edc09854520b..8cfbc94cafb1 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -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;
 }
 
 /**
diff --git a/include/linux/property.h b/include/linux/property.h
index 5746e9927016..64e3a9c6d95f 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -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;
-- 
2.11.0.483.g087da7b7c-goog

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

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

On Wed, 2017-02-01 at 09:31 -0800, Dmitry Torokhov wrote:
> 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:

> diff --git a/drivers/base/property.c b/drivers/base/property.c
[]
> @@ -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;

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

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

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

On Wednesday, February 01, 2017 03:30:03 PM Joe Perches wrote:
> On Wed, 2017-02-01 at 09:31 -0800, Dmitry Torokhov wrote:
> > 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:
> 
> > diff --git a/drivers/base/property.c b/drivers/base/property.c
> []
> > @@ -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;
> 
> 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

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

#1572506 — [PATCH v4 3/4] device property: constify property arrays values

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

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 <dmitry.torokhov@gmail.com>
---

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(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index edc09854520b..09fb9757e086 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -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;
 }
 
 /**
diff --git a/include/linux/property.h b/include/linux/property.h
index 5746e9927016..64e3a9c6d95f 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -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;

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

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

On Thu, 2017-02-02 at 08:39 -0800, Dmitry Torokhov wrote:
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> 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.

> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> 
> 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(-)
> 
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index edc09854520b..09fb9757e086 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -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;
>  }
>  
>  /**
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 5746e9927016..64e3a9c6d95f 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -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;

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

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

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

On February 2, 2017 8:48:30 AM PST, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>On Thu, 2017-02-02 at 08:39 -0800, Dmitry Torokhov wrote:
>> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> 
>> 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.

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.

-- 
Dmitry

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

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

On Thu, 2017-02-02 at 09:07 -0800, Dmitry Torokhov wrote:
> On February 2, 2017 8:48:30 AM PST, Andy Shevchenko <andriy.shevchenko
> @linux.intel.com> wrote:
> > On Thu, 2017-02-02 at 08:39 -0800, Dmitry Torokhov wrote:
> > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > 
> > > 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.
> 
> 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.


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

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

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

On Thu, Feb 02, 2017 at 07:52:58PM +0200, Andy Shevchenko wrote:
> On Thu, 2017-02-02 at 09:07 -0800, Dmitry Torokhov wrote:
> > On February 2, 2017 8:48:30 AM PST, Andy Shevchenko <andriy.shevchenko
> > @linux.intel.com> wrote:
> > > On Thu, 2017-02-02 at 08:39 -0800, Dmitry Torokhov wrote:
> > > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > > 
> > > > 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.
> > 
> > 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.

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

Thanks.

-- 
Dmitry

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

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

On Thu, Feb 2, 2017 at 7:38 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Thu, Feb 02, 2017 at 07:52:58PM +0200, Andy Shevchenko wrote:
>> On Thu, 2017-02-02 at 09:07 -0800, Dmitry Torokhov wrote:
>> > On February 2, 2017 8:48:30 AM PST, Andy Shevchenko <andriy.shevchenko
>> > @linux.intel.com> wrote:
>> > > On Thu, 2017-02-02 at 08:39 -0800, Dmitry Torokhov wrote:
>> > > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> > > >
>> > > > 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.
>> >
>> > 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.

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

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

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

On Fri, Feb 03, 2017 at 12:16:29AM +0100, Rafael J. Wysocki wrote:
> On Thu, Feb 2, 2017 at 7:38 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Thu, Feb 02, 2017 at 07:52:58PM +0200, Andy Shevchenko wrote:
> >> On Thu, 2017-02-02 at 09:07 -0800, Dmitry Torokhov wrote:
> >> > On February 2, 2017 8:48:30 AM PST, Andy Shevchenko <andriy.shevchenko
> >> > @linux.intel.com> wrote:
> >> > > On Thu, 2017-02-02 at 08:39 -0800, Dmitry Torokhov wrote:
> >> > > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >> > > >
> >> > > > 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.
> >> >
> >> > 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
:(.

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

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.

-- 
Dmitry

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

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

On Fri, Feb 3, 2017 at 1:16 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Fri, Feb 03, 2017 at 12:16:29AM +0100, Rafael J. Wysocki wrote:
>> On Thu, Feb 2, 2017 at 7:38 PM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>> > On Thu, Feb 02, 2017 at 07:52:58PM +0200, Andy Shevchenko wrote:
>> >> On Thu, 2017-02-02 at 09:07 -0800, Dmitry Torokhov wrote:
>> >> > On February 2, 2017 8:48:30 AM PST, Andy Shevchenko <andriy.shevchenko
>> >> > @linux.intel.com> wrote:
>> >> > > On Thu, 2017-02-02 at 08:39 -0800, Dmitry Torokhov wrote:
>> >> > > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> >> > > >
>> >> > > > 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.
>> >> >
>> >> > 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
> :(.

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.

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

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

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

On Fri, Feb 03, 2017 at 01:37:25AM +0100, Rafael J. Wysocki wrote:
> On Fri, Feb 3, 2017 at 1:16 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Fri, Feb 03, 2017 at 12:16:29AM +0100, Rafael J. Wysocki wrote:
> >> On Thu, Feb 2, 2017 at 7:38 PM, Dmitry Torokhov
> >> <dmitry.torokhov@gmail.com> wrote:
> >> > On Thu, Feb 02, 2017 at 07:52:58PM +0200, Andy Shevchenko wrote:
> >> >> On Thu, 2017-02-02 at 09:07 -0800, Dmitry Torokhov wrote:
> >> >> > On February 2, 2017 8:48:30 AM PST, Andy Shevchenko <andriy.shevchenko
> >> >> > @linux.intel.com> wrote:
> >> >> > > On Thu, 2017-02-02 at 08:39 -0800, Dmitry Torokhov wrote:
> >> >> > > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >> >> > > >
> >> >> > > > 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.
> >> >> >
> >> >> > 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
> > :(.
> 
> 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.

-- 
Dmitry

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

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

On Wed, Feb 01, 2017 at 09:31:24AM -0800, Dmitry Torokhov wrote:
> 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 <dmitry.torokhov@gmail.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

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

#1571826 — [PATCH v3 2/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.

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/property.c  | 35 ++++++++++++++++++-----------------
 include/linux/property.h |  2 +-
 2 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 934845fc72e5..edc09854520b 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -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;
 
diff --git a/include/linux/property.h b/include/linux/property.h
index 48f154ae7e99..5746e9927016 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -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);
-- 
2.11.0.483.g087da7b7c-goog

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

#1572035

On Wed, Feb 01, 2017 at 09:31:21AM -0800, 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 #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.

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

-- 
Dmitry

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web