This helper is similar to __sysfs_match_string() with the exception that it
ignores NULL elements within the array.
It takes an extra parameter (called `gaps`) which when true will ignore
the NULL elements. When false, this function behaves exactly like
`__sysfs_match_string()`.
Signed-off-by: Alexandru Ardelean <[email protected]>
---
include/linux/string.h | 9 ++++++++-
lib/string.c | 14 ++++++++++----
2 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/include/linux/string.h b/include/linux/string.h
index 7927b875f80c..30595ed483dc 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -189,7 +189,14 @@ static inline int strtobool(const char *s, bool *res)
}
int match_string(const char * const *array, size_t n, const char *string);
-int __sysfs_match_string(const char * const *array, size_t n, const char *s);
+int __sysfs_match_string_with_gaps(const char * const *array, size_t n,
+ const char *str, bool gaps);
+
+static inline int __sysfs_match_string(const char * const *array, size_t n,
+ const char *str)
+{
+ return __sysfs_match_string_with_gaps(array, n, str, false);
+}
/**
* sysfs_match_string - matches given string in an array
diff --git a/lib/string.c b/lib/string.c
index 38e4ca08e757..8ddac3cd292a 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -659,30 +659,36 @@ int match_string(const char * const *array, size_t n, const char *string)
EXPORT_SYMBOL(match_string);
/**
- * __sysfs_match_string - matches given string in an array
+ * __sysfs_match_string_with_gaps - matches string in array ignoring NULLs
* @array: array of strings
* @n: number of strings in the array or -1 for NULL terminated arrays
* @str: string to match with
+ * @gaps: boolean to ignore NULL elements within the array
*
* Returns index of @str in the @array or -EINVAL, just like match_string().
* Uses sysfs_streq instead of strcmp for matching.
+ * Ignores NULL pointers within the @array if @gaps is true.
*/
-int __sysfs_match_string(const char * const *array, size_t n, const char *str)
+int __sysfs_match_string_with_gaps(const char * const *array, size_t n,
+ const char *str, bool gaps)
{
const char *item;
int index;
for (index = 0; index < n; index++) {
item = array[index];
- if (!item)
+ if (!item) {
+ if (gaps)
+ continue;
break;
+ }
if (sysfs_streq(item, str))
return index;
}
return -EINVAL;
}
-EXPORT_SYMBOL(__sysfs_match_string);
+EXPORT_SYMBOL(__sysfs_match_string_with_gaps);
#ifndef __HAVE_ARCH_MEMSET
/**
--
2.17.1
From: Lars-Peter Clausen <[email protected]>
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.
Signed-off-by: Lars-Peter Clausen <[email protected]>
Signed-off-by: Alexandru Ardelean <[email protected]>
---
drivers/iio/industrialio-core.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index f2ebca65f964..c141a29bf446 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -447,8 +447,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';
@@ -469,7 +472,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]);
@@ -486,7 +489,8 @@ ssize_t iio_enum_write(struct iio_dev *indio_dev,
if (!e->set)
return -EINVAL;
- ret = __sysfs_match_string(e->items, e->num_items, buf);
+ ret = __sysfs_match_string_with_gaps(e->items, e->num_items,
+ buf, true);
if (ret < 0)
return ret;
--
2.17.1
On Mon, 22 Apr 2019 11:32:56 +0300
Alexandru Ardelean <[email protected]> wrote:
> This helper is similar to __sysfs_match_string() with the exception that it
> ignores NULL elements within the array.
> It takes an extra parameter (called `gaps`) which when true will ignore
> the NULL elements. When false, this function behaves exactly like
> `__sysfs_match_string()`.
>
> Signed-off-by: Alexandru Ardelean <[email protected]>
Hi Alex,
The existence of that gaps parameter does seem a little odd.
Why not just have the exposed _gaps form wrap a version that
takes that parameter and always set it to true?
Otherwise looks good to me.
Jonathan
> ---
> include/linux/string.h | 9 ++++++++-
> lib/string.c | 14 ++++++++++----
> 2 files changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 7927b875f80c..30595ed483dc 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -189,7 +189,14 @@ static inline int strtobool(const char *s, bool *res)
> }
>
> int match_string(const char * const *array, size_t n, const char *string);
> -int __sysfs_match_string(const char * const *array, size_t n, const char *s);
> +int __sysfs_match_string_with_gaps(const char * const *array, size_t n,
> + const char *str, bool gaps);
> +
> +static inline int __sysfs_match_string(const char * const *array, size_t n,
> + const char *str)
> +{
> + return __sysfs_match_string_with_gaps(array, n, str, false);
> +}
>
> /**
> * sysfs_match_string - matches given string in an array
> diff --git a/lib/string.c b/lib/string.c
> index 38e4ca08e757..8ddac3cd292a 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -659,30 +659,36 @@ int match_string(const char * const *array, size_t n, const char *string)
> EXPORT_SYMBOL(match_string);
>
> /**
> - * __sysfs_match_string - matches given string in an array
> + * __sysfs_match_string_with_gaps - matches string in array ignoring NULLs
> * @array: array of strings
> * @n: number of strings in the array or -1 for NULL terminated arrays
> * @str: string to match with
> + * @gaps: boolean to ignore NULL elements within the array
> *
> * Returns index of @str in the @array or -EINVAL, just like match_string().
> * Uses sysfs_streq instead of strcmp for matching.
> + * Ignores NULL pointers within the @array if @gaps is true.
> */
> -int __sysfs_match_string(const char * const *array, size_t n, const char *str)
> +int __sysfs_match_string_with_gaps(const char * const *array, size_t n,
> + const char *str, bool gaps)
> {
> const char *item;
> int index;
>
> for (index = 0; index < n; index++) {
> item = array[index];
> - if (!item)
> + if (!item) {
> + if (gaps)
> + continue;
> break;
> + }
> if (sysfs_streq(item, str))
> return index;
> }
>
> return -EINVAL;
> }
> -EXPORT_SYMBOL(__sysfs_match_string);
> +EXPORT_SYMBOL(__sysfs_match_string_with_gaps);
>
> #ifndef __HAVE_ARCH_MEMSET
> /**
On Mon, 22 Apr 2019 11:32:57 +0300
Alexandru Ardelean <[email protected]> wrote:
> From: Lars-Peter Clausen <[email protected]>
>
> 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.
>
> Signed-off-by: Lars-Peter Clausen <[email protected]>
> Signed-off-by: Alexandru Ardelean <[email protected]>
Looks good, subject to the whole gaps parameter discussion on patch
1.
Thanks,
Jonathan
> ---
> drivers/iio/industrialio-core.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index f2ebca65f964..c141a29bf446 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -447,8 +447,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';
> @@ -469,7 +472,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]);
> @@ -486,7 +489,8 @@ ssize_t iio_enum_write(struct iio_dev *indio_dev,
> if (!e->set)
> return -EINVAL;
>
> - ret = __sysfs_match_string(e->items, e->num_items, buf);
> + ret = __sysfs_match_string_with_gaps(e->items, e->num_items,
> + buf, true);
> if (ret < 0)
> return ret;
>
On Mon, 2019-04-22 at 11:37 +0100, Jonathan Cameron wrote:
> [External]
>
>
> On Mon, 22 Apr 2019 11:32:56 +0300
> Alexandru Ardelean <[email protected]> wrote:
>
> > This helper is similar to __sysfs_match_string() with the exception
> > that it
> > ignores NULL elements within the array.
> > It takes an extra parameter (called `gaps`) which when true will ignore
> > the NULL elements. When false, this function behaves exactly like
> > `__sysfs_match_string()`.
> >
> > Signed-off-by: Alexandru Ardelean <[email protected]>
>
> Hi Alex,
>
> The existence of that gaps parameter does seem a little odd.
> Why not just have the exposed _gaps form wrap a version that
> takes that parameter and always set it to true?
>
That was one way I thought about doing it.
I was thinking about maybe not exporting too many symbols, since that can
add some size-overhead to the kernel during linking.
I'll send a V2.
Thanks
Alex
> Otherwise looks good to me.
>
> Jonathan
>
> > ---
> > include/linux/string.h | 9 ++++++++-
> > lib/string.c | 14 ++++++++++----
> > 2 files changed, 18 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/linux/string.h b/include/linux/string.h
> > index 7927b875f80c..30595ed483dc 100644
> > --- a/include/linux/string.h
> > +++ b/include/linux/string.h
> > @@ -189,7 +189,14 @@ static inline int strtobool(const char *s, bool
> > *res)
> > }
> >
> > int match_string(const char * const *array, size_t n, const char
> > *string);
> > -int __sysfs_match_string(const char * const *array, size_t n, const
> > char *s);
> > +int __sysfs_match_string_with_gaps(const char * const *array, size_t
> > n,
> > + const char *str, bool gaps);
> > +
> > +static inline int __sysfs_match_string(const char * const *array,
> > size_t n,
> > + const char *str)
> > +{
> > + return __sysfs_match_string_with_gaps(array, n, str, false);
> > +}
> >
> > /**
> > * sysfs_match_string - matches given string in an array
> > diff --git a/lib/string.c b/lib/string.c
> > index 38e4ca08e757..8ddac3cd292a 100644
> > --- a/lib/string.c
> > +++ b/lib/string.c
> > @@ -659,30 +659,36 @@ int match_string(const char * const *array,
> > size_t n, const char *string)
> > EXPORT_SYMBOL(match_string);
> >
> > /**
> > - * __sysfs_match_string - matches given string in an array
> > + * __sysfs_match_string_with_gaps - matches string in array ignoring
> > NULLs
> > * @array: array of strings
> > * @n: number of strings in the array or -1 for NULL terminated arrays
> > * @str: string to match with
> > + * @gaps: boolean to ignore NULL elements within the array
> > *
> > * Returns index of @str in the @array or -EINVAL, just like
> > match_string().
> > * Uses sysfs_streq instead of strcmp for matching.
> > + * Ignores NULL pointers within the @array if @gaps is true.
> > */
> > -int __sysfs_match_string(const char * const *array, size_t n, const
> > char *str)
> > +int __sysfs_match_string_with_gaps(const char * const *array, size_t
> > n,
> > + const char *str, bool gaps)
> > {
> > const char *item;
> > int index;
> >
> > for (index = 0; index < n; index++) {
> > item = array[index];
> > - if (!item)
> > + if (!item) {
> > + if (gaps)
> > + continue;
> > break;
> > + }
> > if (sysfs_streq(item, str))
> > return index;
> > }
> >
> > return -EINVAL;
> > }
> > -EXPORT_SYMBOL(__sysfs_match_string);
> > +EXPORT_SYMBOL(__sysfs_match_string_with_gaps);
> >
> > #ifndef __HAVE_ARCH_MEMSET
> > /**
>
>
This helper is similar to __sysfs_match_string() with the exception that it
ignores NULL elements within the array.
It takes an extra parameter (called `gaps`) which when true will ignore
the NULL elements. When false, this function behaves exactly like
`__sysfs_match_string()`.
Signed-off-by: Alexandru Ardelean <[email protected]>
---
include/linux/string.h | 2 ++
lib/string.c | 48 ++++++++++++++++++++++++++++++++----------
2 files changed, 39 insertions(+), 11 deletions(-)
diff --git a/include/linux/string.h b/include/linux/string.h
index 7927b875f80c..08e4a33b5f29 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -190,6 +190,8 @@ static inline int strtobool(const char *s, bool *res)
int match_string(const char * const *array, size_t n, const char *string);
int __sysfs_match_string(const char * const *array, size_t n, const char *s);
+int __sysfs_match_string_with_gaps(const char * const *array, size_t n,
+ const char *str);
/**
* sysfs_match_string - matches given string in an array
diff --git a/lib/string.c b/lib/string.c
index 38e4ca08e757..2ae3b7e5ff3d 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -658,32 +658,58 @@ int match_string(const char * const *array, size_t n, const char *string)
}
EXPORT_SYMBOL(match_string);
-/**
- * __sysfs_match_string - matches given string in an array
- * @array: array of strings
- * @n: number of strings in the array or -1 for NULL terminated arrays
- * @str: string to match with
- *
- * Returns index of @str in the @array or -EINVAL, just like match_string().
- * Uses sysfs_streq instead of strcmp for matching.
- */
-int __sysfs_match_string(const char * const *array, size_t n, const char *str)
+static int __sysfs_match_string_common(const char * const *array, size_t n,
+ const char *str, bool gaps)
{
const char *item;
int index;
for (index = 0; index < n; index++) {
item = array[index];
- if (!item)
+ if (!item) {
+ if (gaps)
+ continue;
break;
+ }
if (sysfs_streq(item, str))
return index;
}
return -EINVAL;
}
+
+/**
+ * __sysfs_match_string - matches given string in an array
+ * @array: array of strings
+ * @n: number of strings in the array or -1 for NULL terminated arrays
+ * @str: string to match with
+ *
+ * Returns index of @str in the @array or -EINVAL, just like match_string().
+ * Uses sysfs_streq instead of strcmp for matching.
+ */
+int __sysfs_match_string(const char * const *array, size_t n, const char *str)
+{
+ return __sysfs_match_string_common(array, n, str, false);
+}
EXPORT_SYMBOL(__sysfs_match_string);
+/**
+ * __sysfs_match_string_with_gaps - matches string in array ignoring NULLs
+ * @array: array of strings
+ * @n: number of strings in the array or -1 for NULL terminated arrays
+ * @str: string to match with
+ *
+ * Returns index of @str in the @array or -EINVAL, just like match_string().
+ * Uses sysfs_streq instead of strcmp for matching.
+ * Ignores NULL entries within the @array.
+ */
+int __sysfs_match_string_with_gaps(const char * const *array, size_t n,
+ const char *str)
+{
+ return __sysfs_match_string_common(array, n, str, true);
+}
+EXPORT_SYMBOL(__sysfs_match_string_with_gaps);
+
#ifndef __HAVE_ARCH_MEMSET
/**
* memset - Fill a region of memory with the given value
--
2.17.1
From: Lars-Peter Clausen <[email protected]>
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.
Signed-off-by: Lars-Peter Clausen <[email protected]>
Signed-off-by: Alexandru Ardelean <[email protected]>
---
drivers/iio/industrialio-core.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index f2ebca65f964..e848b35bd9c0 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -447,8 +447,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';
@@ -469,7 +472,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]);
@@ -486,7 +489,7 @@ ssize_t iio_enum_write(struct iio_dev *indio_dev,
if (!e->set)
return -EINVAL;
- ret = __sysfs_match_string(e->items, e->num_items, buf);
+ ret = __sysfs_match_string_with_gaps(e->items, e->num_items, buf);
if (ret < 0)
return ret;
--
2.17.1
On Mon, Apr 22, 2019 at 11:32:56AM +0300, Alexandru Ardelean wrote:
> This helper is similar to __sysfs_match_string() with the exception that it
> ignores NULL elements within the array.
sysfs is "one value per file", why are you trying to write multiple
things on a single line to a single sysfs file?
Is IIO really that messy? :)
thanks,
greg k-h
On Mon, 2019-04-22 at 23:06 +0200, Greg KH wrote:
>
>
> On Mon, Apr 22, 2019 at 11:32:56AM +0300, Alexandru Ardelean wrote:
> > This helper is similar to __sysfs_match_string() with the exception
> > that it
> > ignores NULL elements within the array.
>
> sysfs is "one value per file", why are you trying to write multiple
> things on a single line to a single sysfs file?
>
> Is IIO really that messy? :)
>
Hmm, I don't think I understood the comment/question, or maybe I did not
formulate the comment properly.
Maybe Jonathan can pitch-in here if I'm saying something wrong.
So, in IIO there is `struct iio_enum` which is essentially a sysfs wrapper
for exposing an "enum" type to userspace via sysfs (which takes only one
value). This iio_enum type is basically a string-to-int mapping.
Some example in C:
enum {
ENUM0,
ENUM1,
ENUM5 = 5,
ENUM6,
ENUM7
};
/* Notice the gaps in the elements */
static const char * const item_strings[] = {
[ENUM0] = "mode0",
[ENUM1] = "mode1",
[ENUM5] = "mode5",
[ENUM6] = "mode6",
[ENUM7] = "mode7",
};
static const struct iio_enum iio_enum1 = {
.items = item_strings,
.num_items = ARRAY_SIZE(item_strings),
.set = iio_enum1_set,
.get = iio_enum1_get,
};
The signature of the iio_enum1_set / iio_enum1_get is below:
static int iio_enum1_set(struct iio_dev *indio_dev,
const struct iio_chan_spec *chan, unsigned int val);
static int iio_enum1_get(struct iio_dev *indio_dev,
const struct iio_chan_spec *chan)
IIO core resolves the string-to-int mapping.
It uses __sysfs_match_string() to do that, but it requires that the list of
strings (and C enums) be contiguous.
This change [and V2 of this patch] introduces a
__sysfs_match_string_with_gaps() helper that ignores gaps (represented as
NULLs).
For reference, __sysfs_match_string() returns -EINVAL on the first NULL in
the array of strings (regardless of the given array size).
__sysfs_match_string_with_gaps() is typically helpful when C enums refer to
bitfields, or have some equivalence in HW.
Thanks
Alex
> thanks,
>
> greg k-h
On Tue, 23 Apr 2019 06:38:44 +0000
"Ardelean, Alexandru" <[email protected]> wrote:
> On Mon, 2019-04-22 at 23:06 +0200, Greg KH wrote:
> >
> >
> > On Mon, Apr 22, 2019 at 11:32:56AM +0300, Alexandru Ardelean wrote:
> > > This helper is similar to __sysfs_match_string() with the exception
> > > that it
> > > ignores NULL elements within the array.
> >
> > sysfs is "one value per file", why are you trying to write multiple
> > things on a single line to a single sysfs file?
> >
> > Is IIO really that messy? :)
> >
>
> Hmm, I don't think I understood the comment/question, or maybe I did not
> formulate the comment properly.
>
> Maybe Jonathan can pitch-in here if I'm saying something wrong.
>
> So, in IIO there is `struct iio_enum` which is essentially a sysfs wrapper
> for exposing an "enum" type to userspace via sysfs (which takes only one
> value). This iio_enum type is basically a string-to-int mapping.
>
> Some example in C:
>
> enum {
> ENUM0,
> ENUM1,
> ENUM5 = 5,
> ENUM6,
> ENUM7
> };
>
>
> /* Notice the gaps in the elements */
> static const char * const item_strings[] = {
> [ENUM0] = "mode0",
> [ENUM1] = "mode1",
> [ENUM5] = "mode5",
> [ENUM6] = "mode6",
> [ENUM7] = "mode7",
> };
>
> static const struct iio_enum iio_enum1 = {
> .items = item_strings,
> .num_items = ARRAY_SIZE(item_strings),
> .set = iio_enum1_set,
> .get = iio_enum1_get,
> };
>
>
> The signature of the iio_enum1_set / iio_enum1_get is below:
>
> static int iio_enum1_set(struct iio_dev *indio_dev,
> const struct iio_chan_spec *chan, unsigned int val);
>
> static int iio_enum1_get(struct iio_dev *indio_dev,
> const struct iio_chan_spec *chan)
>
>
> IIO core resolves the string-to-int mapping.
> It uses __sysfs_match_string() to do that, but it requires that the list of
> strings (and C enums) be contiguous.
> This change [and V2 of this patch] introduces a
> __sysfs_match_string_with_gaps() helper that ignores gaps (represented as
> NULLs).
>
> For reference, __sysfs_match_string() returns -EINVAL on the first NULL in
> the array of strings (regardless of the given array size).
>
> __sysfs_match_string_with_gaps() is typically helpful when C enums refer to
> bitfields, or have some equivalence in HW.
>
You have described it well.
Perhaps the issue is in the naming? Or more description is needed for the original
patch.
It's worth highlighting that the current help text for
__sysfs_match_string has a description that says:
/**
* __sysfs_match_string - matches given string in an array
* @array: array of strings
* @n: number of strings in the array or -1 for NULL terminated arrays
* @str: string to match with
*
* Returns index of @str in the @array or -EINVAL, just like match_string().
* Uses sysfs_streq instead of strcmp for matching.
*/
so one could argue that if you pass a value of n which is not -1 the function
should not assume that any NULL terminates the array...
So basically this new function is implementing what I would have assumed
__sysfs_match_string would do, but doesn't.
Jonathan
> Thanks
> Alex
>
> > thanks,
> >
> > greg k-h
On Wed, Apr 24, 2019 at 01:34:55PM +0100, Jonathan Cameron wrote:
> On Tue, 23 Apr 2019 06:38:44 +0000
> "Ardelean, Alexandru" <[email protected]> wrote:
>
> > On Mon, 2019-04-22 at 23:06 +0200, Greg KH wrote:
> > >
> > >
> > > On Mon, Apr 22, 2019 at 11:32:56AM +0300, Alexandru Ardelean wrote:
> > > > This helper is similar to __sysfs_match_string() with the exception
> > > > that it
> > > > ignores NULL elements within the array.
> > >
> > > sysfs is "one value per file", why are you trying to write multiple
> > > things on a single line to a single sysfs file?
> > >
> > > Is IIO really that messy? :)
> > >
> >
> > Hmm, I don't think I understood the comment/question, or maybe I did not
> > formulate the comment properly.
> >
> > Maybe Jonathan can pitch-in here if I'm saying something wrong.
> >
> > So, in IIO there is `struct iio_enum` which is essentially a sysfs wrapper
> > for exposing an "enum" type to userspace via sysfs (which takes only one
> > value). This iio_enum type is basically a string-to-int mapping.
>
> >
> > Some example in C:
> >
> > enum {
> > ENUM0,
> > ENUM1,
> > ENUM5 = 5,
> > ENUM6,
> > ENUM7
> > };
> >
> >
> > /* Notice the gaps in the elements */
> > static const char * const item_strings[] = {
> > [ENUM0] = "mode0",
> > [ENUM1] = "mode1",
> > [ENUM5] = "mode5",
> > [ENUM6] = "mode6",
> > [ENUM7] = "mode7",
> > };
> >
> > static const struct iio_enum iio_enum1 = {
> > .items = item_strings,
> > .num_items = ARRAY_SIZE(item_strings),
> > .set = iio_enum1_set,
> > .get = iio_enum1_get,
> > };
> >
> >
> > The signature of the iio_enum1_set / iio_enum1_get is below:
> >
> > static int iio_enum1_set(struct iio_dev *indio_dev,
> > const struct iio_chan_spec *chan, unsigned int val);
> >
> > static int iio_enum1_get(struct iio_dev *indio_dev,
> > const struct iio_chan_spec *chan)
> >
> >
> > IIO core resolves the string-to-int mapping.
> > It uses __sysfs_match_string() to do that, but it requires that the list of
> > strings (and C enums) be contiguous.
> > This change [and V2 of this patch] introduces a
> > __sysfs_match_string_with_gaps() helper that ignores gaps (represented as
> > NULLs).
> >
> > For reference, __sysfs_match_string() returns -EINVAL on the first NULL in
> > the array of strings (regardless of the given array size).
> >
> > __sysfs_match_string_with_gaps() is typically helpful when C enums refer to
> > bitfields, or have some equivalence in HW.
> >
>
> You have described it well.
> Perhaps the issue is in the naming? Or more description is needed for the original
> patch.
>
> It's worth highlighting that the current help text for
> __sysfs_match_string has a description that says:
>
> /**
> * __sysfs_match_string - matches given string in an array
> * @array: array of strings
> * @n: number of strings in the array or -1 for NULL terminated arrays
> * @str: string to match with
> *
> * Returns index of @str in the @array or -EINVAL, just like match_string().
> * Uses sysfs_streq instead of strcmp for matching.
> */
>
> so one could argue that if you pass a value of n which is not -1 the function
> should not assume that any NULL terminates the array...
>
> So basically this new function is implementing what I would have assumed
> __sysfs_match_string would do, but doesn't.
Ok, yeah, I'm confused, I also thought this is what the original
function did.
Nevermind, no objection from me on this:
Acked-by: Greg Kroah-Hartman <[email protected]>
On Thu, Apr 25, 2019 at 10:38 PM [email protected]
<[email protected]> wrote:
>
> On Wed, Apr 24, 2019 at 01:34:55PM +0100, Jonathan Cameron wrote:
> > On Tue, 23 Apr 2019 06:38:44 +0000
> > "Ardelean, Alexandru" <[email protected]> wrote:
> >
> > > On Mon, 2019-04-22 at 23:06 +0200, Greg KH wrote:
> > > >
> > > >
> > > > On Mon, Apr 22, 2019 at 11:32:56AM +0300, Alexandru Ardelean wrote:
> > > > > This helper is similar to __sysfs_match_string() with the exception
> > > > > that it
> > > > > ignores NULL elements within the array.
> > > >
> > > > sysfs is "one value per file", why are you trying to write multiple
> > > > things on a single line to a single sysfs file?
> > > >
> > > > Is IIO really that messy? :)
> > > >
> > >
> > > Hmm, I don't think I understood the comment/question, or maybe I did not
> > > formulate the comment properly.
> > >
> > > Maybe Jonathan can pitch-in here if I'm saying something wrong.
> > >
> > > So, in IIO there is `struct iio_enum` which is essentially a sysfs wrapper
> > > for exposing an "enum" type to userspace via sysfs (which takes only one
> > > value). This iio_enum type is basically a string-to-int mapping.
> >
> > >
> > > Some example in C:
> > >
> > > enum {
> > > ENUM0,
> > > ENUM1,
> > > ENUM5 = 5,
> > > ENUM6,
> > > ENUM7
> > > };
> > >
> > >
> > > /* Notice the gaps in the elements */
> > > static const char * const item_strings[] = {
> > > [ENUM0] = "mode0",
> > > [ENUM1] = "mode1",
> > > [ENUM5] = "mode5",
> > > [ENUM6] = "mode6",
> > > [ENUM7] = "mode7",
> > > };
> > >
> > > static const struct iio_enum iio_enum1 = {
> > > .items = item_strings,
> > > .num_items = ARRAY_SIZE(item_strings),
> > > .set = iio_enum1_set,
> > > .get = iio_enum1_get,
> > > };
> > >
> > >
> > > The signature of the iio_enum1_set / iio_enum1_get is below:
> > >
> > > static int iio_enum1_set(struct iio_dev *indio_dev,
> > > const struct iio_chan_spec *chan, unsigned int val);
> > >
> > > static int iio_enum1_get(struct iio_dev *indio_dev,
> > > const struct iio_chan_spec *chan)
> > >
> > >
> > > IIO core resolves the string-to-int mapping.
> > > It uses __sysfs_match_string() to do that, but it requires that the list of
> > > strings (and C enums) be contiguous.
> > > This change [and V2 of this patch] introduces a
> > > __sysfs_match_string_with_gaps() helper that ignores gaps (represented as
> > > NULLs).
> > >
> > > For reference, __sysfs_match_string() returns -EINVAL on the first NULL in
> > > the array of strings (regardless of the given array size).
> > >
> > > __sysfs_match_string_with_gaps() is typically helpful when C enums refer to
> > > bitfields, or have some equivalence in HW.
> > >
> >
> > You have described it well.
> > Perhaps the issue is in the naming? Or more description is needed for the original
> > patch.
> >
> > It's worth highlighting that the current help text for
> > __sysfs_match_string has a description that says:
> >
> > /**
> > * __sysfs_match_string - matches given string in an array
> > * @array: array of strings
> > * @n: number of strings in the array or -1 for NULL terminated arrays
> > * @str: string to match with
> > *
> > * Returns index of @str in the @array or -EINVAL, just like match_string().
> > * Uses sysfs_streq instead of strcmp for matching.
> > */
> >
> > so one could argue that if you pass a value of n which is not -1 the function
> > should not assume that any NULL terminates the array...
> >
> > So basically this new function is implementing what I would have assumed
> > __sysfs_match_string would do, but doesn't.
>
> Ok, yeah, I'm confused, I also thought this is what the original
> function did.
>
> Nevermind, no objection from me on this:
>
> Acked-by: Greg Kroah-Hartman <[email protected]>
Hmm, I actually did not give much thought to that -1.
I'll check into this and see about a V3.
It may make more sense to just fix the original
`__sysfs_match_string()`, but I'll need to go through the users of
this function and see.
Thanks
Alex
On Fri, Apr 26, 2019 at 12:29:11PM +0300, Alexandru Ardelean wrote:
> Hmm, I actually did not give much thought to that -1.
> I'll check into this and see about a V3.
> It may make more sense to just fix the original
> `__sysfs_match_string()`, but I'll need to go through the users of
> this function and see.
I was thinking about existing users of such (with "gaps") cases.
Not all of them have NULL there and would like to avoid some members.
Though, I think that we may ignore NULL items if -1 is supplied.
Think as well about ARRAY_SIZE() as given to that.
And consider to fix match_string() accordingly.
--
With Best Regards,
Andy Shevchenko
On Fri, Apr 26, 2019 at 5:27 PM [email protected]
<[email protected]> wrote:
>
> On Fri, Apr 26, 2019 at 12:29:11PM +0300, Alexandru Ardelean wrote:
>
> > Hmm, I actually did not give much thought to that -1.
> > I'll check into this and see about a V3.
> > It may make more sense to just fix the original
> > `__sysfs_match_string()`, but I'll need to go through the users of
> > this function and see.
>
> I was thinking about existing users of such (with "gaps") cases.
> Not all of them have NULL there and would like to avoid some members.
> Though, I think that we may ignore NULL items if -1 is supplied.
>
> Think as well about ARRAY_SIZE() as given to that.
>
I am a bit vague on what you are proposing.
Is it:
a) Leave __sysfs_match_string() as-is and introduce a new
`__sysfs_match_string_with_gaps()` helper/variant ?
b) Fix __sysfs_match_string() to break/exit on the first NULL, only if
-1 is provided ?
Either is fine, but I wanted to clarify.
Thanks
Alex
> And consider to fix match_string() accordingly.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Mon, May 06, 2019 at 04:45:43PM +0300, Alexandru Ardelean wrote:
> On Fri, Apr 26, 2019 at 5:27 PM [email protected]
> <[email protected]> wrote:
> >
> > On Fri, Apr 26, 2019 at 12:29:11PM +0300, Alexandru Ardelean wrote:
> >
> > > Hmm, I actually did not give much thought to that -1.
> > > I'll check into this and see about a V3.
> > > It may make more sense to just fix the original
> > > `__sysfs_match_string()`, but I'll need to go through the users of
> > > this function and see.
> >
> > I was thinking about existing users of such (with "gaps") cases.
> > Not all of them have NULL there and would like to avoid some members.
> > Though, I think that we may ignore NULL items if -1 is supplied.
> >
> > Think as well about ARRAY_SIZE() as given to that.
> >
>
> I am a bit vague on what you are proposing.
> Is it:
>
> a) Leave __sysfs_match_string() as-is and introduce a new
> `__sysfs_match_string_with_gaps()` helper/variant ?
> b) Fix __sysfs_match_string() to break/exit on the first NULL, only if
> -1 is provided ?
>
> Either is fine, but I wanted to clarify.
The current logic something like "-1 to go till first NULL" and ARRAY_SIZE() in
*some* cases is basically the synonym to above.
What I meant is to check if there is *any* case where ARRAY_SIZE() behaves in
the same way as -1. Those cases should be fixed accordingly.
Otherwise, the b) is what would be preferred according to the discussion.
> > And consider to fix match_string() accordingly.
--
With Best Regards,
Andy Shevchenko
On Mon, 2019-05-06 at 17:46 +0300, [email protected] wrote:
> [External]
>
>
> On Mon, May 06, 2019 at 04:45:43PM +0300, Alexandru Ardelean wrote:
> > On Fri, Apr 26, 2019 at 5:27 PM [email protected]
> > <[email protected]> wrote:
> > >
> > > On Fri, Apr 26, 2019 at 12:29:11PM +0300, Alexandru Ardelean wrote:
> > >
> > > > Hmm, I actually did not give much thought to that -1.
> > > > I'll check into this and see about a V3.
> > > > It may make more sense to just fix the original
> > > > `__sysfs_match_string()`, but I'll need to go through the users of
> > > > this function and see.
> > >
> > > I was thinking about existing users of such (with "gaps") cases.
> > > Not all of them have NULL there and would like to avoid some members.
> > > Though, I think that we may ignore NULL items if -1 is supplied.
> > >
> > > Think as well about ARRAY_SIZE() as given to that.
> > >
> >
> > I am a bit vague on what you are proposing.
> > Is it:
> >
> > a) Leave __sysfs_match_string() as-is and introduce a new
> > `__sysfs_match_string_with_gaps()` helper/variant ?
> > b) Fix __sysfs_match_string() to break/exit on the first NULL, only if
> > -1 is provided ?
> >
> > Either is fine, but I wanted to clarify.
>
> The current logic something like "-1 to go till first NULL" and
> ARRAY_SIZE() in
> *some* cases is basically the synonym to above.
>
> What I meant is to check if there is *any* case where ARRAY_SIZE()
> behaves in
> the same way as -1. Those cases should be fixed accordingly.
>
> Otherwise, the b) is what would be preferred according to the discussion.
>
Ack.
I sent a series.
I guess this is the noisiest I've ever been.
And I feel a bit bad/guilty [for generating that much noise], but I'll
probably grab a beer later to treat it.
I'll probably learn something from this.
Guilt-tripped experiences are pretty learnful.
Thanks
Alex
> > > And consider to fix match_string() accordingly.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>