* [PATCH v2 0/2] Add "extended_name" to label @ 2021-06-16 15:57 Paul Cercueil 2021-06-16 15:57 ` [PATCH v2 1/2] iio: core: Forbid use of both labels and extended names Paul Cercueil 2021-06-16 15:57 ` [PATCH v2 2/2] iio: core: Support reading extended name as label Paul Cercueil 0 siblings, 2 replies; 6+ messages in thread From: Paul Cercueil @ 2021-06-16 15:57 UTC (permalink / raw) To: Jonathan Cameron, Lars-Peter Clausen Cc: linux-iio, linux-kernel, Paul Cercueil Hi Jonathan, Here's the v2 of my previous patchset which added an "extended_name" attribute. Following the discussion on the v1, here's a set of two patches that 1. make sure we will never have channels with both labels and extended names, 2. make the extended name available to userspace as the channel's label. Cheers, -Paul Paul Cercueil (2): iio: core: Forbid use of both labels and extended names iio: core: Support reading extended name as label drivers/iio/industrialio-core.c | 32 +++++++++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-) -- 2.30.2 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/2] iio: core: Forbid use of both labels and extended names 2021-06-16 15:57 [PATCH v2 0/2] Add "extended_name" to label Paul Cercueil @ 2021-06-16 15:57 ` Paul Cercueil 2021-06-17 7:07 ` Alexandru Ardelean 2021-06-16 15:57 ` [PATCH v2 2/2] iio: core: Support reading extended name as label Paul Cercueil 1 sibling, 1 reply; 6+ messages in thread From: Paul Cercueil @ 2021-06-16 15:57 UTC (permalink / raw) To: Jonathan Cameron, Lars-Peter Clausen Cc: linux-iio, linux-kernel, Paul Cercueil Extended names are a problem for user-space as they make the filenames in sysfs sometimes not parsable. They are now deprecated in favor of labels. This change makes sure that a device driver won't provide both labels and extended names for its channels. It has never been the case and we don't want it to happen. Signed-off-by: Paul Cercueil <paul@crapouillou.net> --- drivers/iio/industrialio-core.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c index 59efb36db2c7..81f40dab778a 100644 --- a/drivers/iio/industrialio-core.c +++ b/drivers/iio/industrialio-core.c @@ -1836,6 +1836,24 @@ static int iio_check_unique_scan_index(struct iio_dev *indio_dev) return 0; } +static int iio_check_extended_name(const struct iio_dev *indio_dev) +{ + unsigned int i; + + if (!indio_dev->info->read_label) + return 0; + + for (i = 0; i < indio_dev->num_channels; i++) { + if (indio_dev->channels[i].extend_name) { + dev_err(&indio_dev->dev, + "Cannot use labels and extend_name at the same time\n"); + return -EINVAL; + } + } + + return 0; +} + static const struct iio_buffer_setup_ops noop_ring_setup_ops; int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod) @@ -1860,6 +1878,10 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod) if (ret < 0) return ret; + ret = iio_check_extended_name(indio_dev); + if (ret < 0) + return ret; + iio_device_register_debugfs(indio_dev); ret = iio_buffers_alloc_sysfs_and_mask(indio_dev); -- 2.30.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] iio: core: Forbid use of both labels and extended names 2021-06-16 15:57 ` [PATCH v2 1/2] iio: core: Forbid use of both labels and extended names Paul Cercueil @ 2021-06-17 7:07 ` Alexandru Ardelean 0 siblings, 0 replies; 6+ messages in thread From: Alexandru Ardelean @ 2021-06-17 7:07 UTC (permalink / raw) To: Paul Cercueil; +Cc: Jonathan Cameron, Lars-Peter Clausen, linux-iio, LKML On Wed, Jun 16, 2021 at 7:01 PM Paul Cercueil <paul@crapouillou.net> wrote: > > Extended names are a problem for user-space as they make the filenames > in sysfs sometimes not parsable. They are now deprecated in favor of > labels. > > This change makes sure that a device driver won't provide both labels > and extended names for its channels. It has never been the case and we > don't want it to happen. > Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com> > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > drivers/iio/industrialio-core.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c > index 59efb36db2c7..81f40dab778a 100644 > --- a/drivers/iio/industrialio-core.c > +++ b/drivers/iio/industrialio-core.c > @@ -1836,6 +1836,24 @@ static int iio_check_unique_scan_index(struct iio_dev *indio_dev) > return 0; > } > > +static int iio_check_extended_name(const struct iio_dev *indio_dev) > +{ > + unsigned int i; > + > + if (!indio_dev->info->read_label) > + return 0; > + > + for (i = 0; i < indio_dev->num_channels; i++) { > + if (indio_dev->channels[i].extend_name) { > + dev_err(&indio_dev->dev, > + "Cannot use labels and extend_name at the same time\n"); > + return -EINVAL; > + } > + } > + > + return 0; > +} > + > static const struct iio_buffer_setup_ops noop_ring_setup_ops; > > int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod) > @@ -1860,6 +1878,10 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod) > if (ret < 0) > return ret; > > + ret = iio_check_extended_name(indio_dev); > + if (ret < 0) > + return ret; > + > iio_device_register_debugfs(indio_dev); > > ret = iio_buffers_alloc_sysfs_and_mask(indio_dev); > -- > 2.30.2 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] iio: core: Support reading extended name as label 2021-06-16 15:57 [PATCH v2 0/2] Add "extended_name" to label Paul Cercueil 2021-06-16 15:57 ` [PATCH v2 1/2] iio: core: Forbid use of both labels and extended names Paul Cercueil @ 2021-06-16 15:57 ` Paul Cercueil 2021-06-17 7:22 ` Alexandru Ardelean 1 sibling, 1 reply; 6+ messages in thread From: Paul Cercueil @ 2021-06-16 15:57 UTC (permalink / raw) To: Jonathan Cameron, Lars-Peter Clausen Cc: linux-iio, linux-kernel, Paul Cercueil The point of this new change is to make the IIO tree actually parsable. Before, given this attribute as a filename: in_voltage0_aux_sample_rate Userspace had no way to know if the attribute name was "aux_sample_rate" with no extended name, or "sample_rate" with "aux" as the extended name, or just "rate" with "aux_sample" as the extended name. This was somewhat possible to deduce when there was more than one attribute present for a given channel, e.g: in_voltage0_aux_sample_rate in_voltage0_aux_frequency There, it was possible to deduce that "aux" was the extended name. But even with more than one attribute, this wasn't very robust, as two attributes starting with the same prefix (e.g. "sample_rate" and "sample_size") would result in the first part of the prefix being interpreted as being part of the extended name. To address the issue, knowing that channels will never have both a label and an extended name, set the channel's label to the extended name. In this case, the label's attribute will also have the extended name in its filename, but we can live with that - userspace can open in_voltage0_<prefix>_label and verify that it returns <prefix> to obtain the extended name. Signed-off-by: Paul Cercueil <paul@crapouillou.net> --- 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 81f40dab778a..9b37e96538c2 100644 --- a/drivers/iio/industrialio-core.c +++ b/drivers/iio/industrialio-core.c @@ -717,8 +717,12 @@ static ssize_t iio_read_channel_label(struct device *dev, struct iio_dev *indio_dev = dev_to_iio_dev(dev); struct iio_dev_attr *this_attr = to_iio_dev_attr(attr); - if (!indio_dev->info->read_label) - return -EINVAL; + if (!indio_dev->info->read_label) { + if (this_attr->c->extend_name) + return sprintf(buf, "%s\n", this_attr->c->extend_name); + else + return -EINVAL; + } return indio_dev->info->read_label(indio_dev, this_attr->c, buf); } @@ -1160,7 +1164,7 @@ static int iio_device_add_channel_label(struct iio_dev *indio_dev, struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev); int ret; - if (!indio_dev->info->read_label) + if (!indio_dev->info->read_label && !chan->extend_name) return 0; ret = __iio_add_chan_devattr("label", -- 2.30.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] iio: core: Support reading extended name as label 2021-06-16 15:57 ` [PATCH v2 2/2] iio: core: Support reading extended name as label Paul Cercueil @ 2021-06-17 7:22 ` Alexandru Ardelean 2021-06-17 9:08 ` Paul Cercueil 0 siblings, 1 reply; 6+ messages in thread From: Alexandru Ardelean @ 2021-06-17 7:22 UTC (permalink / raw) To: Paul Cercueil; +Cc: Jonathan Cameron, Lars-Peter Clausen, linux-iio, LKML On Wed, Jun 16, 2021 at 7:00 PM Paul Cercueil <paul@crapouillou.net> wrote: > > The point of this new change is to make the IIO tree actually parsable. > > Before, given this attribute as a filename: > in_voltage0_aux_sample_rate > > Userspace had no way to know if the attribute name was > "aux_sample_rate" with no extended name, or "sample_rate" with "aux" as > the extended name, or just "rate" with "aux_sample" as the extended > name. > > This was somewhat possible to deduce when there was more than one > attribute present for a given channel, e.g: > in_voltage0_aux_sample_rate > in_voltage0_aux_frequency > > There, it was possible to deduce that "aux" was the extended name. But > even with more than one attribute, this wasn't very robust, as two > attributes starting with the same prefix (e.g. "sample_rate" and > "sample_size") would result in the first part of the prefix being > interpreted as being part of the extended name. > > To address the issue, knowing that channels will never have both a label > and an extended name, set the channel's label to the extended name. > In this case, the label's attribute will also have the extended name in > its filename, but we can live with that - userspace can open > in_voltage0_<prefix>_label and verify that it returns <prefix> to obtain > the extended name. > The best way would have been for all drivers [using extend_name] to implement their own read_label hook. But this can work fine as well [as the other method would add some duplication]. > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > 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 81f40dab778a..9b37e96538c2 100644 > --- a/drivers/iio/industrialio-core.c > +++ b/drivers/iio/industrialio-core.c > @@ -717,8 +717,12 @@ static ssize_t iio_read_channel_label(struct device *dev, > struct iio_dev *indio_dev = dev_to_iio_dev(dev); > struct iio_dev_attr *this_attr = to_iio_dev_attr(attr); > > - if (!indio_dev->info->read_label) > - return -EINVAL; > + if (!indio_dev->info->read_label) { > + if (this_attr->c->extend_name) > + return sprintf(buf, "%s\n", this_attr->c->extend_name); > + else nitpick: else statement has no effect well, this block could be reworked a bit as: ---------------------------------------------------- if (indio_dev->info->read_label) return indio_dev->info->read_label(indio_dev, this_attr->c, buf); if (this_attr->c->extend_name) return sprintf(buf, "%s\n", this_attr->c->extend_name); return -EINVAL; ---------------------------------------------------- > + return -EINVAL; > + } > > return indio_dev->info->read_label(indio_dev, this_attr->c, buf); > } > @@ -1160,7 +1164,7 @@ static int iio_device_add_channel_label(struct iio_dev *indio_dev, > struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev); > int ret; > > - if (!indio_dev->info->read_label) > + if (!indio_dev->info->read_label && !chan->extend_name) > return 0; > > ret = __iio_add_chan_devattr("label", > -- > 2.30.2 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] iio: core: Support reading extended name as label 2021-06-17 7:22 ` Alexandru Ardelean @ 2021-06-17 9:08 ` Paul Cercueil 0 siblings, 0 replies; 6+ messages in thread From: Paul Cercueil @ 2021-06-17 9:08 UTC (permalink / raw) To: Alexandru Ardelean; +Cc: Jonathan Cameron, Lars-Peter Clausen, linux-iio, LKML Hi Alexandru, Le jeu., juin 17 2021 at 10:22:35 +0300, Alexandru Ardelean <ardeleanalex@gmail.com> a écrit : > On Wed, Jun 16, 2021 at 7:00 PM Paul Cercueil <paul@crapouillou.net> > wrote: >> >> The point of this new change is to make the IIO tree actually >> parsable. >> >> Before, given this attribute as a filename: >> in_voltage0_aux_sample_rate >> >> Userspace had no way to know if the attribute name was >> "aux_sample_rate" with no extended name, or "sample_rate" with >> "aux" as >> the extended name, or just "rate" with "aux_sample" as the extended >> name. >> >> This was somewhat possible to deduce when there was more than one >> attribute present for a given channel, e.g: >> in_voltage0_aux_sample_rate >> in_voltage0_aux_frequency >> >> There, it was possible to deduce that "aux" was the extended name. >> But >> even with more than one attribute, this wasn't very robust, as two >> attributes starting with the same prefix (e.g. "sample_rate" and >> "sample_size") would result in the first part of the prefix being >> interpreted as being part of the extended name. >> >> To address the issue, knowing that channels will never have both a >> label >> and an extended name, set the channel's label to the extended name. >> In this case, the label's attribute will also have the extended >> name in >> its filename, but we can live with that - userspace can open >> in_voltage0_<prefix>_label and verify that it returns <prefix> to >> obtain >> the extended name. >> > > The best way would have been for all drivers [using extend_name] to > implement their own read_label hook. Let's agree to disagree :) > But this can work fine as well [as the other method would add some > duplication]. > >> Signed-off-by: Paul Cercueil <paul@crapouillou.net> >> --- >> 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 81f40dab778a..9b37e96538c2 100644 >> --- a/drivers/iio/industrialio-core.c >> +++ b/drivers/iio/industrialio-core.c >> @@ -717,8 +717,12 @@ static ssize_t iio_read_channel_label(struct >> device *dev, >> struct iio_dev *indio_dev = dev_to_iio_dev(dev); >> struct iio_dev_attr *this_attr = to_iio_dev_attr(attr); >> >> - if (!indio_dev->info->read_label) >> - return -EINVAL; >> + if (!indio_dev->info->read_label) { >> + if (this_attr->c->extend_name) >> + return sprintf(buf, "%s\n", >> this_attr->c->extend_name); >> + else > > nitpick: else statement has no effect > > well, this block could be reworked a bit as: > > ---------------------------------------------------- > if (indio_dev->info->read_label) > return indio_dev->info->read_label(indio_dev, this_attr->c, buf); > > if (this_attr->c->extend_name) > return sprintf(buf, "%s\n", this_attr->c->extend_name); > > return -EINVAL; > ---------------------------------------------------- I generally prefer to make the diff as small as possible so that the changes are more obvious. But this does look better. -Paul > >> + return -EINVAL; >> + } >> >> return indio_dev->info->read_label(indio_dev, this_attr->c, >> buf); >> } >> @@ -1160,7 +1164,7 @@ static int >> iio_device_add_channel_label(struct iio_dev *indio_dev, >> struct iio_dev_opaque *iio_dev_opaque = >> to_iio_dev_opaque(indio_dev); >> int ret; >> >> - if (!indio_dev->info->read_label) >> + if (!indio_dev->info->read_label && !chan->extend_name) >> return 0; >> >> ret = __iio_add_chan_devattr("label", >> -- >> 2.30.2 >> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-06-17 9:08 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-16 15:57 [PATCH v2 0/2] Add "extended_name" to label Paul Cercueil 2021-06-16 15:57 ` [PATCH v2 1/2] iio: core: Forbid use of both labels and extended names Paul Cercueil 2021-06-17 7:07 ` Alexandru Ardelean 2021-06-16 15:57 ` [PATCH v2 2/2] iio: core: Support reading extended name as label Paul Cercueil 2021-06-17 7:22 ` Alexandru Ardelean 2021-06-17 9:08 ` Paul Cercueil
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).