Re: [PATCH 1/2] lib: add __sysfs_match_string_with_gaps() helper

[Alexandru Ardelean]

On Thu, Apr 25, 2019 at 10:38 PM gregkh@xxxxxxxxxxxxxxxxxxx
<gregkh@xxxxxxxxxxxxxxxxxxx> 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" <alexandru.Ardelean@xxxxxxxxxx> 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 <gregkh@xxxxxxxxxxxxxxxxxxx>

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