LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 1/3][V3] lib: fix __sysfs_match_string() helper when n != -1
@ 2019-05-08 11:19 Alexandru Ardelean
  2019-05-08 11:19 ` [PATCH 2/3][V3] scsi: sd: remove sysfs_match_string() dense array comment Alexandru Ardelean
                   ` (4 more replies)
  0 siblings, 5 replies; 12+ messages in thread
From: Alexandru Ardelean @ 2019-05-08 11:19 UTC (permalink / raw)
  To: linux-kernel, linux-iio, linux-scsi
  Cc: gregkh, andriy.shevchenko, jic23, lars, 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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 2/3][V3] scsi: sd: remove sysfs_match_string() dense array comment
  2019-05-08 11:19 [PATCH 1/3][V3] lib: fix __sysfs_match_string() helper when n != -1 Alexandru Ardelean
@ 2019-05-08 11:19 ` Alexandru Ardelean
  2019-05-08 11:19 ` [PATCH 3/3][V3] iio: Handle enumerated properties with gaps Alexandru Ardelean
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 12+ messages in thread
From: Alexandru Ardelean @ 2019-05-08 11:19 UTC (permalink / raw)
  To: linux-kernel, linux-iio, linux-scsi
  Cc: gregkh, andriy.shevchenko, jic23, lars, 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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 3/3][V3] iio: Handle enumerated properties with gaps
  2019-05-08 11:19 [PATCH 1/3][V3] lib: fix __sysfs_match_string() helper when n != -1 Alexandru Ardelean
  2019-05-08 11:19 ` [PATCH 2/3][V3] scsi: sd: remove sysfs_match_string() dense array comment Alexandru Ardelean
@ 2019-05-08 11:19 ` Alexandru Ardelean
  2019-05-08 13:17   ` Andy Shevchenko
  2019-05-28  7:18 ` [PATCH 1/3][V3] lib: fix __sysfs_match_string() helper when n != -1 Ardelean, Alexandru
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: Alexandru Ardelean @ 2019-05-08 11:19 UTC (permalink / raw)
  To: linux-kernel, linux-iio, linux-scsi
  Cc: gregkh, andriy.shevchenko, jic23, lars, 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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/3][V3] iio: Handle enumerated properties with gaps
  2019-05-08 11:19 ` [PATCH 3/3][V3] iio: Handle enumerated properties with gaps Alexandru Ardelean
@ 2019-05-08 13:17   ` Andy Shevchenko
  2019-05-09  7:31     ` Ardelean, Alexandru
  0 siblings, 1 reply; 12+ messages in thread
From: Andy Shevchenko @ 2019-05-08 13:17 UTC (permalink / raw)
  To: Alexandru Ardelean
  Cc: linux-kernel, linux-iio, linux-scsi, gregkh, jic23, lars

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



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/3][V3] iio: Handle enumerated properties with gaps
  2019-05-08 13:17   ` Andy Shevchenko
@ 2019-05-09  7:31     ` Ardelean, Alexandru
  2019-07-05 13:35       ` Ardelean, Alexandru
  0 siblings, 1 reply; 12+ messages in thread
From: Ardelean, Alexandru @ 2019-05-09  7:31 UTC (permalink / raw)
  To: andriy.shevchenko
  Cc: linux-scsi, linux-kernel, lars, linux-iio, gregkh, jic23

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3][V3] lib: fix __sysfs_match_string() helper when n != -1
  2019-05-08 11:19 [PATCH 1/3][V3] lib: fix __sysfs_match_string() helper when n != -1 Alexandru Ardelean
  2019-05-08 11:19 ` [PATCH 2/3][V3] scsi: sd: remove sysfs_match_string() dense array comment Alexandru Ardelean
  2019-05-08 11:19 ` [PATCH 3/3][V3] iio: Handle enumerated properties with gaps Alexandru Ardelean
@ 2019-05-28  7:18 ` Ardelean, Alexandru
  2019-06-25 13:01 ` [PATCH][V4] " Alexandru Ardelean
  2019-07-14 10:12 ` [PATCH 1/3][V3] " Jonathan Cameron
  4 siblings, 0 replies; 12+ messages in thread
From: Ardelean, Alexandru @ 2019-05-28  7:18 UTC (permalink / raw)
  To: linux-scsi, linux-kernel, linux-iio
  Cc: andriy.shevchenko, gregkh, jic23, lars

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH][V4] lib: fix __sysfs_match_string() helper when n != -1
  2019-05-08 11:19 [PATCH 1/3][V3] lib: fix __sysfs_match_string() helper when n != -1 Alexandru Ardelean
                   ` (2 preceding siblings ...)
  2019-05-28  7:18 ` [PATCH 1/3][V3] lib: fix __sysfs_match_string() helper when n != -1 Ardelean, Alexandru
@ 2019-06-25 13:01 ` Alexandru Ardelean
  2019-06-25 13:28   ` Andy Shevchenko
  2019-07-14 10:12 ` [PATCH 1/3][V3] " Jonathan Cameron
  4 siblings, 1 reply; 12+ messages in thread
From: Alexandru Ardelean @ 2019-06-25 13:01 UTC (permalink / raw)
  To: linux-kernel
  Cc: andriy.shevchenko, gregkh, alexander.shishkin, akpm,
	ndesaulniers, 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 v3 -> v4:
* split this patch away from series; there are some unsolved discussions
  that will probably need resolving per sub-system

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH][V4] lib: fix __sysfs_match_string() helper when n != -1
  2019-06-25 13:01 ` [PATCH][V4] " Alexandru Ardelean
@ 2019-06-25 13:28   ` Andy Shevchenko
  2019-06-25 19:42     ` Andrew Morton
  0 siblings, 1 reply; 12+ messages in thread
From: Andy Shevchenko @ 2019-06-25 13:28 UTC (permalink / raw)
  To: Alexandru Ardelean
  Cc: linux-kernel, gregkh, alexander.shishkin, akpm, ndesaulniers

On Tue, Jun 25, 2019 at 04:01:04PM +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).

Since it does nothing for current users and comes without an example,
it's hard to justify the need.

The code itself looks good to me.

> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
> 
> Changelog v3 -> v4:
> * split this patch away from series; there are some unsolved discussions
>   that will probably need resolving per sub-system
> 
>  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.20.1
> 

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH][V4] lib: fix __sysfs_match_string() helper when n != -1
  2019-06-25 13:28   ` Andy Shevchenko
@ 2019-06-25 19:42     ` Andrew Morton
  2019-06-26  7:57       ` Ardelean, Alexandru
  0 siblings, 1 reply; 12+ messages in thread
From: Andrew Morton @ 2019-06-25 19:42 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Alexandru Ardelean, linux-kernel, gregkh, alexander.shishkin,
	ndesaulniers

On Tue, 25 Jun 2019 16:28:12 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Tue, Jun 25, 2019 at 04:01:04PM +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).
> 
> Since it does nothing for current users and comes without an example,
> it's hard to justify the need.

Presumably "split this patch away from series" means there's some code
which uses this.  A reference to this in the changelog would be good.

> The code itself looks good to me.

Sure.  But the kerneldoc description of __sysfs_match_string() could do
with an update.



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH][V4] lib: fix __sysfs_match_string() helper when n != -1
  2019-06-25 19:42     ` Andrew Morton
@ 2019-06-26  7:57       ` Ardelean, Alexandru
  0 siblings, 0 replies; 12+ messages in thread
From: Ardelean, Alexandru @ 2019-06-26  7:57 UTC (permalink / raw)
  To: andriy.shevchenko, akpm
  Cc: gregkh, ndesaulniers, linux-kernel, alexander.shishkin

On Tue, 2019-06-25 at 12:42 -0700, Andrew Morton wrote:
> [External]
> 
> 
> On Tue, 25 Jun 2019 16:28:12 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > On Tue, Jun 25, 2019 at 04:01:04PM +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).
> > 
> > Since it does nothing for current users and comes without an example,
> > it's hard to justify the need.
> 
> Presumably "split this patch away from series" means there's some code
> which uses this.  A reference to this in the changelog would be good.
> 
> > The code itself looks good to me.
> 
> Sure.  But the kerneldoc description of __sysfs_match_string() could do
> with an update.
> 

Apologies for any clumsiness here [from my side].

The series tried to handle a change for supporting gaps in IIO enums.
Particularly this patch:
https://patchwork.kernel.org/patch/10935003/

These enums use __sysfs_match_string() to match strings to enum/int values.

There were 3 versions:
https://patchwork.kernel.org/project/linux-iio/list/?series=108481  [V1]
https://patchwork.kernel.org/project/linux-iio/list/?series=108603  [V2]
https://patchwork.kernel.org/project/linux-iio/list/?series=115201  [V3]

The last series [V3] has an unresolved discussion about how to handle gaps in IIO enums.
This will be resolved at a later point in time.

In the meantime, I thought I'd suggest this change on it's own, mostly due to the kerneldoc not being accurate.

I'm fine with both fixing the kerneldoc or fixing the code.
We will still have to resolve the discussion in IIO about how to handle those gapped enums.

> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/3][V3] iio: Handle enumerated properties with gaps
  2019-05-09  7:31     ` Ardelean, Alexandru
@ 2019-07-05 13:35       ` Ardelean, Alexandru
  0 siblings, 0 replies; 12+ messages in thread
From: Ardelean, Alexandru @ 2019-07-05 13:35 UTC (permalink / raw)
  To: andriy.shevchenko
  Cc: linux-scsi, gregkh, jic23, linux-kernel, linux-iio, lars

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3][V3] lib: fix __sysfs_match_string() helper when n != -1
  2019-05-08 11:19 [PATCH 1/3][V3] lib: fix __sysfs_match_string() helper when n != -1 Alexandru Ardelean
                   ` (3 preceding siblings ...)
  2019-06-25 13:01 ` [PATCH][V4] " Alexandru Ardelean
@ 2019-07-14 10:12 ` Jonathan Cameron
  4 siblings, 0 replies; 12+ messages in thread
From: Jonathan Cameron @ 2019-07-14 10:12 UTC (permalink / raw)
  To: Alexandru Ardelean
  Cc: linux-kernel, linux-iio, linux-scsi, gregkh, andriy.shevchenko, lars

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2019-07-14 10:12 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-08 11:19 [PATCH 1/3][V3] lib: fix __sysfs_match_string() helper when n != -1 Alexandru Ardelean
2019-05-08 11:19 ` [PATCH 2/3][V3] scsi: sd: remove sysfs_match_string() dense array comment Alexandru Ardelean
2019-05-08 11:19 ` [PATCH 3/3][V3] iio: Handle enumerated properties with gaps Alexandru Ardelean
2019-05-08 13:17   ` Andy Shevchenko
2019-05-09  7:31     ` Ardelean, Alexandru
2019-07-05 13:35       ` Ardelean, Alexandru
2019-05-28  7:18 ` [PATCH 1/3][V3] lib: fix __sysfs_match_string() helper when n != -1 Ardelean, Alexandru
2019-06-25 13:01 ` [PATCH][V4] " Alexandru Ardelean
2019-06-25 13:28   ` Andy Shevchenko
2019-06-25 19:42     ` Andrew Morton
2019-06-26  7:57       ` Ardelean, Alexandru
2019-07-14 10:12 ` [PATCH 1/3][V3] " Jonathan Cameron

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).