[PATCH 1/3][V3] lib: fix __sysfs_match_string() helper when n != -1

[PATCH 1/3][V3] lib: fix __sysfs_match_string() helper when n != -1

[Alexandru Ardelean]

The documentation the `__sysfs_match_string()` helper mentions that `n`
(the size of the given array) should be:
 * @n: number of strings in the array or -1 for NULL terminated arrays

The behavior of the function is different, in the sense that it exits on
the first NULL element in the array.

This patch changes the behavior, to exit the loop when a NULL element is
found, and the size of the array is provided as -1.

All current users of __sysfs_match_string() & sysfs_match_string() provide
contiguous arrays of strings, so this behavior change doesn't influence
anything (at this point in time).

This behavior change allows for an array of strings to have NULL elements
within the array, which will be ignored. This is particularly useful when
creating mapping of strings and integers (as bitfields or other HW
description).

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---

Changelog v2 -> v3:
* fix __sysfs_match_string() vs adding a new
  __sysfs_match_string_with_gaps() helper

 lib/string.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/string.c b/lib/string.c
index 3ab861c1a857..5bea3f98478a 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -674,8 +674,11 @@ int __sysfs_match_string(const char * const *array, size_t n, const char *str)
 
 	for (index = 0; index < n; index++) {
 		item = array[index];
-		if (!item)
+		if (!item) {
+			if (n != (size_t)-1)
+				continue;
 			break;
+		}
 		if (sysfs_streq(item, str))
 			return index;
 	}
-- 
2.17.1

[PATCH 2/3][V3] scsi: sd: remove sysfs_match_string() dense array comment

[Alexandru Ardelean]

The comment is no longer valid, since it supports arrays with gaps now.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---

Changelog v2 -> v3:
* after fixing __sysfs_match_string() this comment is no longer valid

 drivers/scsi/sd.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 2b2bc4b49d78..73ce390956c1 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -376,7 +376,6 @@ thin_provisioning_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(thin_provisioning);
 
-/* sysfs_match_string() requires dense arrays */
 static const char *lbp_mode[] = {
 	[SD_LBP_FULL]		= "full",
 	[SD_LBP_UNMAP]		= "unmap",
@@ -424,7 +423,6 @@ provisioning_mode_store(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RW(provisioning_mode);
 
-/* sysfs_match_string() requires dense arrays */
 static const char *zeroing_mode[] = {
 	[SD_ZERO_WRITE]		= "write",
 	[SD_ZERO_WS]		= "writesame",
-- 
2.17.1

[PATCH 3/3][V3] iio: Handle enumerated properties with gaps

[Alexandru Ardelean]

From: Lars-Peter Clausen <lars@metafoo.de>

Some enums might have gaps or reserved values in the middle of their value
range. E.g. consider a 2-bit enum where the values 0, 1 and 3 have a
meaning, but 2 is a reserved value and can not be used.

Add support for such enums to the IIO enum helper functions. A reserved
values is marked by setting its entry in the items array to NULL rather
than the normal descriptive string value.

Also, `__sysfs_match_string()` now supports NULL gaps, so that doesn't
require any changes.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---

Changelog v2 -> v3:
* after fixing __sysfs_match_string(), this change only requires that NULL
  be handled in the iio_enum_{available_}read functions
  __sysfs_match_string() handles the NULL gaps

 drivers/iio/industrialio-core.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 9c4d92115504..8b4ff3c8f547 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -446,8 +446,11 @@ ssize_t iio_enum_available_read(struct iio_dev *indio_dev,
 	if (!e->num_items)
 		return 0;
 
-	for (i = 0; i < e->num_items; ++i)
+	for (i = 0; i < e->num_items; ++i) {
+		if (!e->items[i])
+			continue;
 		len += scnprintf(buf + len, PAGE_SIZE - len, "%s ", e->items[i]);
+	}
 
 	/* replace last space with a newline */
 	buf[len - 1] = '\n';
@@ -468,7 +471,7 @@ ssize_t iio_enum_read(struct iio_dev *indio_dev,
 	i = e->get(indio_dev, chan);
 	if (i < 0)
 		return i;
-	else if (i >= e->num_items)
+	else if (i >= e->num_items || !e->items[i])
 		return -EINVAL;
 
 	return snprintf(buf, PAGE_SIZE, "%s\n", e->items[i]);
-- 
2.17.1

Re: [PATCH 3/3][V3] iio: Handle enumerated properties with gaps

[Andy Shevchenko]

On Wed, May 08, 2019 at 02:19:13PM +0300, Alexandru Ardelean wrote:
> From: Lars-Peter Clausen <lars@metafoo.de>
> 
> Some enums might have gaps or reserved values in the middle of their value
> range. E.g. consider a 2-bit enum where the values 0, 1 and 3 have a
> meaning, but 2 is a reserved value and can not be used.
> 
> Add support for such enums to the IIO enum helper functions. A reserved
> values is marked by setting its entry in the items array to NULL rather
> than the normal descriptive string value.
> 
> Also, `__sysfs_match_string()` now supports NULL gaps, so that doesn't
> require any changes.

> -	for (i = 0; i < e->num_items; ++i)
> +	for (i = 0; i < e->num_items; ++i) {
> +		if (!e->items[i])
> +			continue;
>  		len += scnprintf(buf + len, PAGE_SIZE - len, "%s ", e->items[i]);
> +	}

The problem here that the user will have no clue where the gap is happened, to
solve this we need either bitmap of array, where set bits shows defined items,
or use comma-separated list of values. The latter would need another node since
we don't break user space.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3][V3] iio: Handle enumerated properties with gaps

[Ardelean, Alexandru]

On Wed, 2019-05-08 at 16:17 +0300, Andy Shevchenko wrote:
> [External]
> 
> 
> On Wed, May 08, 2019 at 02:19:13PM +0300, Alexandru Ardelean wrote:
> > From: Lars-Peter Clausen <lars@metafoo.de>
> > 
> > Some enums might have gaps or reserved values in the middle of their
> > value
> > range. E.g. consider a 2-bit enum where the values 0, 1 and 3 have a
> > meaning, but 2 is a reserved value and can not be used.
> > 
> > Add support for such enums to the IIO enum helper functions. A reserved
> > values is marked by setting its entry in the items array to NULL rather
> > than the normal descriptive string value.
> > 
> > Also, `__sysfs_match_string()` now supports NULL gaps, so that doesn't
> > require any changes.
> > -     for (i = 0; i < e->num_items; ++i)
> > +     for (i = 0; i < e->num_items; ++i) {
> > +             if (!e->items[i])
> > +                     continue;
> >               len += scnprintf(buf + len, PAGE_SIZE - len, "%s ", e-
> > >items[i]);
> > +     }
> 
> The problem here that the user will have no clue where the gap is
> happened, to
> solve this we need either bitmap of array, where set bits shows defined
> items,
> or use comma-separated list of values. The latter would need another node
> since
> we don't break user space.

Hmmm.
I am wondering if there are cases where userspace would care about reserved
values and/or positions of reserved bit-fields.
Maybe you could offer examples/use-cases where this is needed.

To some extent the kernel [drivers & frameworks] should probably not need
to expose that "string-enum-X"  == `bitfield_2` matching; otherwise it
doesn't really add much value ; the whole point of frameworks [in general]
is to offer some level of abstraction to HW.

The only example I can think of [atm], is when a reserved bit-field will be
used in the future. But then, the driver should care about this, and not
the framework. The driver should decide that "bitfield_2" will
enable/disable something [in the future], and should be considered in a
such a way (when being written). If the driver can't make this prediction [
about "bitfield_2"] then a new driver must be written anyway.

But I will agree that I may not have all arguments in mind to be 100% sure
of all this.

Thanks
Alex

> 
> --
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH 3/3][V3] iio: Handle enumerated properties with gaps

[Ardelean, Alexandru]

On Thu, 2019-05-09 at 10:31 +0300, Alexandru Ardelean wrote:
> On Wed, 2019-05-08 at 16:17 +0300, Andy Shevchenko wrote:
> > [External]
> > 
> > 
> > On Wed, May 08, 2019 at 02:19:13PM +0300, Alexandru Ardelean wrote:
> > > From: Lars-Peter Clausen <lars@metafoo.de>
> > > 
> > > Some enums might have gaps or reserved values in the middle of their
> > > value
> > > range. E.g. consider a 2-bit enum where the values 0, 1 and 3 have a
> > > meaning, but 2 is a reserved value and can not be used.
> > > 
> > > Add support for such enums to the IIO enum helper functions. A reserved
> > > values is marked by setting its entry in the items array to NULL rather
> > > than the normal descriptive string value.
> > > 
> > > Also, `__sysfs_match_string()` now supports NULL gaps, so that doesn't
> > > require any changes.
> > > -     for (i = 0; i < e->num_items; ++i)
> > > +     for (i = 0; i < e->num_items; ++i) {
> > > +             if (!e->items[i])
> > > +                     continue;
> > >               len += scnprintf(buf + len, PAGE_SIZE - len, "%s ", e-
> > > > items[i]);
> > > +     }
> > 
> > The problem here that the user will have no clue where the gap is
> > happened, to
> > solve this we need either bitmap of array, where set bits shows defined
> > items,
> > or use comma-separated list of values. The latter would need another node
> > since
> > we don't break user space.
> 
> Hmmm.
> I am wondering if there are cases where userspace would care about reserved
> values and/or positions of reserved bit-fields.
> Maybe you could offer examples/use-cases where this is needed.
> 
> To some extent the kernel [drivers & frameworks] should probably not need
> to expose that "string-enum-X"  == `bitfield_2` matching; otherwise it
> doesn't really add much value ; the whole point of frameworks [in general]
> is to offer some level of abstraction to HW.
> 
> The only example I can think of [atm], is when a reserved bit-field will be
> used in the future. But then, the driver should care about this, and not
> the framework. The driver should decide that "bitfield_2" will
> enable/disable something [in the future], and should be considered in a
> such a way (when being written). If the driver can't make this prediction [
> about "bitfield_2"] then a new driver must be written anyway.
> 
> But I will agree that I may not have all arguments in mind to be 100% sure
> of all this.
> 

Hey,

Is there any feedback/counter-arguments for this?

Thanks
Alex


> Thanks
> Alex
> 
> > --
> > With Best Regards,
> > Andy Shevchenko
> > 
> > 

Re: [PATCH 1/3][V3] lib: fix __sysfs_match_string() helper when n != -1

[Ardelean, Alexandru]

On Wed, 2019-05-08 at 14:19 +0300, Alexandru Ardelean wrote:
> The documentation the `__sysfs_match_string()` helper mentions that `n`
> (the size of the given array) should be:
>  * @n: number of strings in the array or -1 for NULL terminated arrays
> 
> The behavior of the function is different, in the sense that it exits on
> the first NULL element in the array.
> 
> This patch changes the behavior, to exit the loop when a NULL element is
> found, and the size of the array is provided as -1.
> 
> All current users of __sysfs_match_string() & sysfs_match_string() provide
> contiguous arrays of strings, so this behavior change doesn't influence
> anything (at this point in time).
> 
> This behavior change allows for an array of strings to have NULL elements
> within the array, which will be ignored. This is particularly useful when
> creating mapping of strings and integers (as bitfields or other HW
> description).
> 

Hey,

I did not see any reaction on this.
Do I need to do something on this (re-spin, add other people, re-send as standalone patch, etc) ?

Thanks
Alex


> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
> 
> Changelog v2 -> v3:
> * fix __sysfs_match_string() vs adding a new
>   __sysfs_match_string_with_gaps() helper
> 
>  lib/string.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/string.c b/lib/string.c
> index 3ab861c1a857..5bea3f98478a 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -674,8 +674,11 @@ int __sysfs_match_string(const char * const *array, size_t n, const char *str)
>  
>  	for (index = 0; index < n; index++) {
>  		item = array[index];
> -		if (!item)
> +		if (!item) {
> +			if (n != (size_t)-1)
> +				continue;
>  			break;
> +		}
>  		if (sysfs_streq(item, str))
>  			return index;
>  	}

Re: [PATCH 1/3][V3] lib: fix __sysfs_match_string() helper when n != -1

[Jonathan Cameron]

On Wed, 8 May 2019 14:19:11 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> The documentation the `__sysfs_match_string()` helper mentions that `n`
> (the size of the given array) should be:
>  * @n: number of strings in the array or -1 for NULL terminated arrays
> 
> The behavior of the function is different, in the sense that it exits on
> the first NULL element in the array.
> 
> This patch changes the behavior, to exit the loop when a NULL element is
> found, and the size of the array is provided as -1.
> 
> All current users of __sysfs_match_string() & sysfs_match_string() provide
> contiguous arrays of strings, so this behavior change doesn't influence
> anything (at this point in time).
> 
> This behavior change allows for an array of strings to have NULL elements
> within the array, which will be ignored. This is particularly useful when
> creating mapping of strings and integers (as bitfields or other HW
> description).
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Looks good to me.  I can take it through IIO given patch 3, but
fine with it taking another route if people would prefer as I don't
think the two 'need' to go together.

Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
> 
> Changelog v2 -> v3:
> * fix __sysfs_match_string() vs adding a new
>   __sysfs_match_string_with_gaps() helper
> 
>  lib/string.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/string.c b/lib/string.c
> index 3ab861c1a857..5bea3f98478a 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -674,8 +674,11 @@ int __sysfs_match_string(const char * const *array, size_t n, const char *str)
>  
>  	for (index = 0; index < n; index++) {
>  		item = array[index];
> -		if (!item)
> +		if (!item) {
> +			if (n != (size_t)-1)
> +				continue;
>  			break;
> +		}
>  		if (sysfs_streq(item, str))
>  			return index;
>  	}