| Message ID | 20240624174601.1527244-3-sean.anderson@linux.dev (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | hwmon: iio: Add labels | expand |
On 6/24/24 10:46, Sean Anderson wrote: > Add labels from IIO channels to our channels. This allows userspace to > display more meaningful names instead of "in0" or "temp5". > > Although lm-sensors gracefully handles errors when reading channel > labels, the ABI says the label attribute > >> Should only be created if the driver has hints about what this voltage >> channel is being used for, and user-space doesn't. > > Therefore, we test to see if the channel has a label before > creating the attribute. > FWIW, complaining about an ABI really does not belong into a commit message. Maybe you and lm-sensors don't care about error returns when reading a label, but there are other userspace applications which may expect drivers to follow the ABI. Last time I checked, the basic rule was still "Don't break userspace", and that doesn't mean "it's ok to violate / break an ABI as long as no one notices". > Signed-off-by: Sean Anderson <sean.anderson@linux.dev> > --- > > Changes in v2: > - Check if the label exists before creating the attribute > > drivers/hwmon/iio_hwmon.c | 45 ++++++++++++++++++++++++++++++++++----- > 1 file changed, 40 insertions(+), 5 deletions(-) > > diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c > index 4c8a80847891..5722cb9d81f9 100644 > --- a/drivers/hwmon/iio_hwmon.c > +++ b/drivers/hwmon/iio_hwmon.c > @@ -33,6 +33,17 @@ struct iio_hwmon_state { > struct attribute **attrs; > }; > > +static ssize_t iio_hwmon_read_label(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr); > + struct iio_hwmon_state *state = dev_get_drvdata(dev); > + struct iio_channel *chan = &state->channels[sattr->index]; > + > + return iio_read_channel_label(chan, buf); > +} > + I personally find it a bit kludgy that an in-kernel API would do a sysfs write like this and expect a page-aligned buffer as parameter, but since Jonathan is fine with it: Acked-by: Guenter Roeck <linux@roeck-us.net> Jonathan, please apply through your tree. Thanks, Guenter
On Mon, 24 Jun 2024 11:47:39 -0700 Guenter Roeck <linux@roeck-us.net> wrote: > On 6/24/24 10:46, Sean Anderson wrote: > > Add labels from IIO channels to our channels. This allows userspace to > > display more meaningful names instead of "in0" or "temp5". > > > > Although lm-sensors gracefully handles errors when reading channel > > labels, the ABI says the label attribute > > > >> Should only be created if the driver has hints about what this voltage > >> channel is being used for, and user-space doesn't. > > > > Therefore, we test to see if the channel has a label before > > creating the attribute. > > > > FWIW, complaining about an ABI really does not belong into a commit > message. Maybe you and lm-sensors don't care about error returns when > reading a label, but there are other userspace applications which may > expect drivers to follow the ABI. Last time I checked, the basic rule > was still "Don't break userspace", and that doesn't mean "it's ok to > violate / break an ABI as long as no one notices". > > > Signed-off-by: Sean Anderson <sean.anderson@linux.dev> > > --- > > > > Changes in v2: > > - Check if the label exists before creating the attribute > > > > drivers/hwmon/iio_hwmon.c | 45 ++++++++++++++++++++++++++++++++++----- > > 1 file changed, 40 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c > > index 4c8a80847891..5722cb9d81f9 100644 > > --- a/drivers/hwmon/iio_hwmon.c > > +++ b/drivers/hwmon/iio_hwmon.c > > @@ -33,6 +33,17 @@ struct iio_hwmon_state { > > struct attribute **attrs; > > }; > > > > +static ssize_t iio_hwmon_read_label(struct device *dev, > > + struct device_attribute *attr, > > + char *buf) > > +{ > > + struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr); > > + struct iio_hwmon_state *state = dev_get_drvdata(dev); > > + struct iio_channel *chan = &state->channels[sattr->index]; > > + > > + return iio_read_channel_label(chan, buf); > > +} > > + > > I personally find it a bit kludgy that an in-kernel API would do a > sysfs write like this and expect a page-aligned buffer as parameter, > but since Jonathan is fine with it: That's a good point that I'd not picked up on and it probably makes sense to address that before it bites us on some other subsystem. It was more reasonable when the only path was to a light wrapper that went directly around the sysfs callback. Now we are wrapping these up for more general use we should avoid that restriction. Two approaches to that occur to me. 1) Fix up read_label() everywhere to not use sysfs_emit and take a size of the buffer to print into. There are only 11 implementations so far so this should be straight forward. 2) Add a bounce buffer so we emit into a suitable size for sysfs_emit() then reprint from there into a buffer provided via this interface with the appropriate size provided. This one is clunky and given the relatively few call sits I think fixing it via option 1 is the better route forwards. Jonathan > > Acked-by: Guenter Roeck <linux@roeck-us.net> > > Jonathan, please apply through your tree. > > Thanks, > Guenter >
On 6/24/24 14:47, Guenter Roeck wrote: > On 6/24/24 10:46, Sean Anderson wrote: >> Add labels from IIO channels to our channels. This allows userspace to >> display more meaningful names instead of "in0" or "temp5". >> >> Although lm-sensors gracefully handles errors when reading channel >> labels, the ABI says the label attribute >> >>> Should only be created if the driver has hints about what this voltage >>> channel is being used for, and user-space doesn't. >> >> Therefore, we test to see if the channel has a label before >> creating the attribute. >> > > FWIW, complaining about an ABI really does not belong into a commit > message. Maybe you and lm-sensors don't care about error returns when > reading a label, but there are other userspace applications which may > expect drivers to follow the ABI. Last time I checked, the basic rule > was still "Don't break userspace", and that doesn't mean "it's ok to > violate / break an ABI as long as no one notices". This isn't complaining about the ABI, just documenting the reason it was done this way... >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev> >> --- >> >> Changes in v2: >> - Check if the label exists before creating the attribute >> >> drivers/hwmon/iio_hwmon.c | 45 ++++++++++++++++++++++++++++++++++----- >> 1 file changed, 40 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c >> index 4c8a80847891..5722cb9d81f9 100644 >> --- a/drivers/hwmon/iio_hwmon.c >> +++ b/drivers/hwmon/iio_hwmon.c >> @@ -33,6 +33,17 @@ struct iio_hwmon_state { >> struct attribute **attrs; >> }; >> +static ssize_t iio_hwmon_read_label(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr); >> + struct iio_hwmon_state *state = dev_get_drvdata(dev); >> + struct iio_channel *chan = &state->channels[sattr->index]; >> + >> + return iio_read_channel_label(chan, buf); >> +} >> + > > I personally find it a bit kludgy that an in-kernel API would do a > sysfs write like this and expect a page-aligned buffer as parameter, > but since Jonathan is fine with it: This is also how the in-kernel iio_read_channel_ext_info API works. I agree that it is a strange API, but I do not want to rewrite all the implementing drivers. --Sean > Acked-by: Guenter Roeck <linux@roeck-us.net> > > Jonathan, please apply through your tree. > > Thanks, > Guenter >
On 6/24/24 12:34, Sean Anderson wrote: > On 6/24/24 14:47, Guenter Roeck wrote: >> On 6/24/24 10:46, Sean Anderson wrote: >>> Add labels from IIO channels to our channels. This allows userspace to >>> display more meaningful names instead of "in0" or "temp5". >>> >>> Although lm-sensors gracefully handles errors when reading channel >>> labels, the ABI says the label attribute >>> >>>> Should only be created if the driver has hints about what this voltage >>>> channel is being used for, and user-space doesn't. >>> >>> Therefore, we test to see if the channel has a label before >>> creating the attribute. >>> >> >> FWIW, complaining about an ABI really does not belong into a commit >> message. Maybe you and lm-sensors don't care about error returns when >> reading a label, but there are other userspace applications which may >> expect drivers to follow the ABI. Last time I checked, the basic rule >> was still "Don't break userspace", and that doesn't mean "it's ok to >> violate / break an ABI as long as no one notices". > > This isn't complaining about the ABI, just documenting the reason it was > done this way... > That a patch is implemented to follow its ABI is not worth mentioning in the commit message. You _do_ mention it, and added "Although lm-sensors gracefully ... ". So, from my perspective it is complaining about the ABI, unless you think that pretty much all patches should include "this is done to comply with the ABI, even though <some userspace application> is fine with violating it". Never mind though, I gave it my Acked-by:, and consider the issue closed. Guenter
On 6/24/24 15:24, Jonathan Cameron wrote: > On Mon, 24 Jun 2024 11:47:39 -0700 > Guenter Roeck <linux@roeck-us.net> wrote: > >> On 6/24/24 10:46, Sean Anderson wrote: >> > Add labels from IIO channels to our channels. This allows userspace to >> > display more meaningful names instead of "in0" or "temp5". >> > >> > Although lm-sensors gracefully handles errors when reading channel >> > labels, the ABI says the label attribute >> > >> >> Should only be created if the driver has hints about what this voltage >> >> channel is being used for, and user-space doesn't. >> > >> > Therefore, we test to see if the channel has a label before >> > creating the attribute. >> > >> >> FWIW, complaining about an ABI really does not belong into a commit >> message. Maybe you and lm-sensors don't care about error returns when >> reading a label, but there are other userspace applications which may >> expect drivers to follow the ABI. Last time I checked, the basic rule >> was still "Don't break userspace", and that doesn't mean "it's ok to >> violate / break an ABI as long as no one notices". >> >> > Signed-off-by: Sean Anderson <sean.anderson@linux.dev> >> > --- >> > >> > Changes in v2: >> > - Check if the label exists before creating the attribute >> > >> > drivers/hwmon/iio_hwmon.c | 45 ++++++++++++++++++++++++++++++++++----- >> > 1 file changed, 40 insertions(+), 5 deletions(-) >> > >> > diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c >> > index 4c8a80847891..5722cb9d81f9 100644 >> > --- a/drivers/hwmon/iio_hwmon.c >> > +++ b/drivers/hwmon/iio_hwmon.c >> > @@ -33,6 +33,17 @@ struct iio_hwmon_state { >> > struct attribute **attrs; >> > }; >> > >> > +static ssize_t iio_hwmon_read_label(struct device *dev, >> > + struct device_attribute *attr, >> > + char *buf) >> > +{ >> > + struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr); >> > + struct iio_hwmon_state *state = dev_get_drvdata(dev); >> > + struct iio_channel *chan = &state->channels[sattr->index]; >> > + >> > + return iio_read_channel_label(chan, buf); >> > +} >> > + >> >> I personally find it a bit kludgy that an in-kernel API would do a >> sysfs write like this and expect a page-aligned buffer as parameter, >> but since Jonathan is fine with it: > > That's a good point that I'd not picked up on and it probably makes sense > to address that before it bites us on some other subsystem. > > It was more reasonable when the only path was to a light wrapper that went > directly around the sysfs callback. Now we are wrapping these up for more > general use we should avoid that restriction. > > Two approaches to that occur to me. > 1) Fix up read_label() everywhere to not use sysfs_emit and take a size > of the buffer to print into. There are only 11 implementations so > far so this should be straight forward. This API is the same as the existing iio_read_channel_ext_info. It is used for the same purpose: forwarding sysfs reads/writes from one device to another (see e.g. iio-mux and iio-rescale). ext_info is used by around 85 drivers, so it is not so trivial to change the API. While I agree that the current API is unusual, it's not too bad given that we get the same guarantees from device_attribute.show. --Sean > 2) Add a bounce buffer so we emit into a suitable size for sysfs_emit() > then reprint from there into a buffer provided via this interface with > the appropriate size provided. This one is clunky and given the relatively > few call sits I think fixing it via option 1 is the better route forwards.
On Thu, 27 Jun 2024 14:37:01 -0400 Sean Anderson <sean.anderson@linux.dev> wrote: > On 6/24/24 15:24, Jonathan Cameron wrote: > > On Mon, 24 Jun 2024 11:47:39 -0700 > > Guenter Roeck <linux@roeck-us.net> wrote: > > > >> On 6/24/24 10:46, Sean Anderson wrote: > >> > Add labels from IIO channels to our channels. This allows userspace to > >> > display more meaningful names instead of "in0" or "temp5". > >> > > >> > Although lm-sensors gracefully handles errors when reading channel > >> > labels, the ABI says the label attribute > >> > > >> >> Should only be created if the driver has hints about what this voltage > >> >> channel is being used for, and user-space doesn't. > >> > > >> > Therefore, we test to see if the channel has a label before > >> > creating the attribute. > >> > > >> > >> FWIW, complaining about an ABI really does not belong into a commit > >> message. Maybe you and lm-sensors don't care about error returns when > >> reading a label, but there are other userspace applications which may > >> expect drivers to follow the ABI. Last time I checked, the basic rule > >> was still "Don't break userspace", and that doesn't mean "it's ok to > >> violate / break an ABI as long as no one notices". > >> > >> > Signed-off-by: Sean Anderson <sean.anderson@linux.dev> > >> > --- > >> > > >> > Changes in v2: > >> > - Check if the label exists before creating the attribute > >> > > >> > drivers/hwmon/iio_hwmon.c | 45 ++++++++++++++++++++++++++++++++++----- > >> > 1 file changed, 40 insertions(+), 5 deletions(-) > >> > > >> > diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c > >> > index 4c8a80847891..5722cb9d81f9 100644 > >> > --- a/drivers/hwmon/iio_hwmon.c > >> > +++ b/drivers/hwmon/iio_hwmon.c > >> > @@ -33,6 +33,17 @@ struct iio_hwmon_state { > >> > struct attribute **attrs; > >> > }; > >> > > >> > +static ssize_t iio_hwmon_read_label(struct device *dev, > >> > + struct device_attribute *attr, > >> > + char *buf) > >> > +{ > >> > + struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr); > >> > + struct iio_hwmon_state *state = dev_get_drvdata(dev); > >> > + struct iio_channel *chan = &state->channels[sattr->index]; > >> > + > >> > + return iio_read_channel_label(chan, buf); > >> > +} > >> > + > >> > >> I personally find it a bit kludgy that an in-kernel API would do a > >> sysfs write like this and expect a page-aligned buffer as parameter, > >> but since Jonathan is fine with it: > > > > That's a good point that I'd not picked up on and it probably makes sense > > to address that before it bites us on some other subsystem. > > > > It was more reasonable when the only path was to a light wrapper that went > > directly around the sysfs callback. Now we are wrapping these up for more > > general use we should avoid that restriction. > > > > Two approaches to that occur to me. > > 1) Fix up read_label() everywhere to not use sysfs_emit and take a size > > of the buffer to print into. There are only 11 implementations so > > far so this should be straight forward. > > This API is the same as the existing iio_read_channel_ext_info. It is > used for the same purpose: forwarding sysfs reads/writes from one > device to another (see e.g. iio-mux and iio-rescale). ext_info is used > by around 85 drivers, so it is not so trivial to change the API. While I > agree that the current API is unusual, it's not too bad given that we > get the same guarantees from device_attribute.show. Fair enough. Maybe we can clean this up at somepoint but let's not do it today. Applied to the togreg branch of iio.git and pushed out as testing for 0-day to poke at it and maybe find something we missed. Jonathan > > --Sean > > > 2) Add a bounce buffer so we emit into a suitable size for sysfs_emit() > > then reprint from there into a buffer provided via this interface with > > the appropriate size provided. This one is clunky and given the relatively > > few call sits I think fixing it via option 1 is the better route forwards. > >
diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c index 4c8a80847891..5722cb9d81f9 100644 --- a/drivers/hwmon/iio_hwmon.c +++ b/drivers/hwmon/iio_hwmon.c @@ -33,6 +33,17 @@ struct iio_hwmon_state { struct attribute **attrs; }; +static ssize_t iio_hwmon_read_label(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr); + struct iio_hwmon_state *state = dev_get_drvdata(dev); + struct iio_channel *chan = &state->channels[sattr->index]; + + return iio_read_channel_label(chan, buf); +} + /* * Assumes that IIO and hwmon operate in the same base units. * This is supposed to be true, but needs verification for @@ -68,12 +79,13 @@ static int iio_hwmon_probe(struct platform_device *pdev) struct device *dev = &pdev->dev; struct iio_hwmon_state *st; struct sensor_device_attribute *a; - int ret, i; + int ret, i, attr = 0; int in_i = 1, temp_i = 1, curr_i = 1, humidity_i = 1, power_i = 1; enum iio_chan_type type; struct iio_channel *channels; struct device *hwmon_dev; char *sname; + void *buf; channels = devm_iio_channel_get_all(dev); if (IS_ERR(channels)) { @@ -85,17 +97,18 @@ static int iio_hwmon_probe(struct platform_device *pdev) } st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL); - if (st == NULL) + buf = (void *)devm_get_free_pages(dev, GFP_KERNEL, 0); + if (!st || !buf) return -ENOMEM; st->channels = channels; - /* count how many attributes we have */ + /* count how many channels we have */ while (st->channels[st->num_channels].indio_dev) st->num_channels++; st->attrs = devm_kcalloc(dev, - st->num_channels + 1, sizeof(*st->attrs), + 2 * st->num_channels + 1, sizeof(*st->attrs), GFP_KERNEL); if (st->attrs == NULL) return -ENOMEM; @@ -147,9 +160,31 @@ static int iio_hwmon_probe(struct platform_device *pdev) a->dev_attr.show = iio_hwmon_read_val; a->dev_attr.attr.mode = 0444; a->index = i; - st->attrs[i] = &a->dev_attr.attr; + st->attrs[attr++] = &a->dev_attr.attr; + + /* Let's see if we have a label... */ + if (iio_read_channel_label(&st->channels[i], buf) < 0) + continue; + + a = devm_kzalloc(dev, sizeof(*a), GFP_KERNEL); + if (a == NULL) + return -ENOMEM; + + sysfs_attr_init(&a->dev_attr.attr); + a->dev_attr.attr.name = devm_kasprintf(dev, GFP_KERNEL, + "%s%d_label", + prefix, n); + if (!a->dev_attr.attr.name) + return -ENOMEM; + + a->dev_attr.show = iio_hwmon_read_label; + a->dev_attr.attr.mode = 0444; + a->index = i; + st->attrs[attr++] = &a->dev_attr.attr; } + devm_free_pages(dev, (unsigned long)buf); + st->attr_group.attrs = st->attrs; st->groups[0] = &st->attr_group;