| Message ID | 20190508111913.7276-3-alexandru.ardelean@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3,V3] lib: fix __sysfs_match_string() helper when n != -1 | expand |
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.
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 > >
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 > > > >
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]);