On Mon, 24 Jun 2024 11:47:39 -0700
Guenter Roeck <linux@xxxxxxxxxxxx> 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@xxxxxxxxx>
> > ---
> >
> > 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@xxxxxxxxxxxx>
>
> Jonathan, please apply through your tree.
>
> Thanks,
> Guenter
>
