| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <bc45e2de04d6decfcbfa60502e75e8b7bef1450e.camel@gmail.com>
Date: Wed, 16 Jul 2025 07:23:05 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: Sean Anderson <sean.anderson@...ux.dev>, Jonathan Cameron
<jic23@...nel.org>, Jean Delvare <jdelvare@...e.com>, Guenter Roeck
<linux@...ck-us.net>, linux-iio@...r.kernel.org,
linux-hwmon@...r.kernel.org
Cc: Andy Shevchenko <andy@...nel.org>, Nuno Sá
<nuno.sa@...log.com>, linux-kernel@...r.kernel.org, David Lechner
<dlechner@...libre.com>
Subject: Re: [PATCH 2/7] iio: inkern: Add API for reading/writing events
On Tue, 2025-07-15 at 11:43 -0400, Sean Anderson wrote:
> On 7/15/25 06:35, Nuno Sá wrote:
> > On Mon, 2025-07-14 at 21:20 -0400, Sean Anderson wrote:
> > > Add an in-kernel API for reading/writing event properties. Like the
> > > raw-to-processed conversion, with processed-to-raw we only convert the
> > > integer part, introducing some round-off error.
> > >
> > > A common case is for other drivers to re-expose IIO events as sysfs
> > > properties with a different API. To help out with this, iio_event_mode
> > > returns the appropriate mode. It can also be used to test for existence
> > > if the consumer doesn't care about read/write capability.
> > >
> > > Signed-off-by: Sean Anderson <sean.anderson@...ux.dev>
> > > ---
> >
> > Just one comment on top of Andy's review
> >
> > >
> > > drivers/iio/inkern.c | 198 +++++++++++++++++++++++++++++++++++
> > > include/linux/iio/consumer.h | 56 ++++++++++
> > > 2 files changed, 254 insertions(+)
> > >
> > > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> > > index c174ebb7d5e6..d3bbd2444fb5 100644
> > > --- a/drivers/iio/inkern.c
> > > +++ b/drivers/iio/inkern.c
> > > @@ -1028,3 +1028,201 @@ ssize_t iio_read_channel_label(struct iio_channel
> > > *chan, char *buf)
> > > return do_iio_read_channel_label(chan->indio_dev, chan->channel,
> > > buf);
> > > }
> > > EXPORT_SYMBOL_GPL(iio_read_channel_label);
> > > +
> > > +static bool iio_event_exists(struct iio_channel *channel,
> > > + enum iio_event_type type,
> > > + enum iio_event_direction dir,
> > > + enum iio_event_info info)
> > > +{
> > > + struct iio_chan_spec const *chan = channel->channel;
> > > + int i;
> > > +
> >
> > Can we iio_event_exists() -> iio_event_exists_locked()? Or likely the best way
>
> wouldn't _unlocked be the convention for this file?
Oh, indeed!
>
> > would be to annotate it with lockdep (though that would mean some dance to get
> > the opaque object.
>
> I will add a lockdep annotation.
>
> --Sean
>
> > Anyways, bottom line is it should clear that iio_event_exists() is to be called
> > with the lock held.
> >
> > - Nuno Sá
> >
> > > + if (!channel->indio_dev->info)
> > > + return false;
> > > +
> > > + for (i = 0; i < chan->num_event_specs; i++) {
> > > + if (chan->event_spec[i].type != type)
> > > + continue;
> > > + if (chan->event_spec[i].dir != dir)
> > > + continue;
> > > + if (chan->event_spec[i].mask_separate & BIT(info))
> > > + return true;
> > > + }
> > > +
> > > + return false;
> > > +}
> > > +
> > > +umode_t iio_event_mode(struct iio_channel *chan, enum iio_event_type type,
> > > + enum iio_event_direction dir, enum iio_event_info
> > > info)
> > > +{
> > > + struct iio_dev *indio_dev = chan->indio_dev;
> > > + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> > > + umode_t mode = 0;
> > > +
> > > + guard(mutex)(&iio_dev_opaque->info_exist_lock);
> > > + if (!iio_event_exists(chan, type, dir, info))
> > > + return 0;
> > > +
> > > + if (info == IIO_EV_INFO_ENABLE) {
> > > + if (indio_dev->info->read_event_config)
> > > + mode |= 0444;
> > > +
> > > + if (indio_dev->info->write_event_config)
> > > + mode |= 0200;
> > > + } else {
> > > + if (indio_dev->info->read_event_value)
> > > + mode |= 0444;
> > > +
> > > + if (indio_dev->info->write_event_value)
> > > + mode |= 0200;
> > > + }
> > > +
> > > + return mode;
> > > +}
> > > +EXPORT_SYMBOL_GPL(iio_event_mode);
> > > +
> > > +int iio_read_event_processed_scale(struct iio_channel *chan,
> > > + enum iio_event_type type,
> > > + enum iio_event_direction dir,
> > > + enum iio_event_info info, int *val,
> > > + unsigned int scale)
> > > +{
> > > + struct iio_dev *indio_dev = chan->indio_dev;
> > > + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> > > + int ret, raw;
> > > +
> > > + guard(mutex)(&iio_dev_opaque->info_exist_lock);
> > > + if (!iio_event_exists(chan, type, dir, info))
> > > + return -ENODEV;
> > > +
> > > + if (info == IIO_EV_INFO_ENABLE) {
> > > + if (!indio_dev->info->read_event_config)
> > > + return -EINVAL;
> > > +
> > > + raw = indio_dev->info->read_event_config(indio_dev,
> > > + chan->channel, type,
> > > + dir);
> > > + if (raw < 0)
> > > + return raw;
> > > +
> > > + *val = raw;
> > > + return 0;
> > > + }
> > > +
> > > + if (!indio_dev->info->read_event_value)
> > > + return -EINVAL;
> > > +
> > > + ret = indio_dev->info->read_event_value(indio_dev, chan->channel,
> > > type,
> > > + dir, info, &raw, NULL);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + return iio_convert_raw_to_processed_unlocked(chan, raw, val, scale);
> > > +}
> > > +EXPORT_SYMBOL_GPL(iio_read_event_processed_scale);
> > > +
> > > +static int iio_convert_processed_to_raw_unlocked(struct iio_channel *chan,
> > > + int processed, int *raw,
> > > + unsigned int scale)
> > > +{
> > > + int scale_type, scale_val, scale_val2;
> > > + int offset_type, offset_val, offset_val2;
> > > + s64 r, scale64, raw64;
> > > +
> > > + scale_type = iio_channel_read(chan, &scale_val, &scale_val2,
> > > + IIO_CHAN_INFO_SCALE);
> > > + if (scale_type < 0) {
> > > + raw64 = processed / scale;
> > > + } else {
> > > + switch (scale_type) {
> > > + case IIO_VAL_INT:
> > > + scale64 = (s64)scale_val * scale;
> > > + if (scale64 <= INT_MAX && scale64 >= INT_MIN)
> > > + raw64 = processed / (int)scale64;
> > > + else
> > > + raw64 = 0;
> > > + break;
> > > + case IIO_VAL_INT_PLUS_MICRO:
> > > + scale64 = scale_val * scale * 1000000LL + scale_val2;
> > > + raw64 = div64_s64_rem(processed, scale64, &r);
> > > + raw64 = raw64 * 1000000 +
> > > + div64_s64(r * 1000000, scale64);
> > > + break;
> > > + case IIO_VAL_INT_PLUS_NANO:
> > > + scale64 = scale_val * scale * 1000000000LL +
> > > scale_val2;
> > > + raw64 = div64_s64_rem(processed, scale64, &r);
> > > + raw64 = raw64 * 1000000000 +
> > > + div64_s64(r * 1000000000, scale64);
> > > + break;
> > > + case IIO_VAL_FRACTIONAL:
> > > + raw64 = div64_s64((s64)processed * scale_val2,
> > > + (s64)scale_val * scale);
> > > + break;
> > > + case IIO_VAL_FRACTIONAL_LOG2:
> > > + raw64 = div64_s64((s64)processed << scale_val2,
> > > + (s64)scale_val * scale);
> > > + break;
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > + }
> > > +
> > > + offset_type = iio_channel_read(chan, &offset_val, &offset_val2,
> > > + IIO_CHAN_INFO_OFFSET);
> > > + if (offset_type >= 0) {
> > > + switch (offset_type) {
> > > + case IIO_VAL_INT:
> > > + case IIO_VAL_INT_PLUS_MICRO:
> > > + case IIO_VAL_INT_PLUS_NANO:
> > > + raw64 -= offset_val;
> > > + break;
> > > + case IIO_VAL_FRACTIONAL:
> > > + raw64 -= offset_val / offset_val2;
> > > + break;
> > > + case IIO_VAL_FRACTIONAL_LOG2:
> > > + raw64 -= offset_val >> offset_val2;
> > > + break;
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > + }
> > > +
> > > + *raw = clamp(raw64, (s64)INT_MIN, (s64)INT_MAX);
> > > + return 0;
> > > +}
> > > +
> > > +int iio_write_event_processed_scale(struct iio_channel *chan,
> > > + enum iio_event_type type,
> > > + enum iio_event_direction dir,
> > > + enum iio_event_info info, int processed,
> > > + unsigned int scale)
> > > +{
> > > + struct iio_dev *indio_dev = chan->indio_dev;
> > > + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(chan-
> > > > indio_dev);
> > > + int ret, raw;
> > > +
> > > + guard(mutex)(&iio_dev_opaque->info_exist_lock);
> > > + if (!iio_event_exists(chan, type, dir, info))
> > > + return -ENODEV;
> > > +
> > > + if (info == IIO_EV_INFO_ENABLE) {
> > > + if (!indio_dev->info->write_event_config)
> > > + return -EINVAL;
> > > +
> > > + return indio_dev->info->write_event_config(indio_dev,
> > > + chan->channel,
> > > type,
> > > + dir, processed);
> > > + }
> > > +
> > > + if (!indio_dev->info->write_event_value)
> > > + return -EINVAL;
> > > +
> > > + ret = iio_convert_processed_to_raw_unlocked(chan, processed, &raw,
> > > + scale);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + return indio_dev->info->write_event_value(indio_dev, chan->channel,
> > > + type, dir, info, raw, 0);
> > > +}
> > > +EXPORT_SYMBOL_GPL(iio_write_event_processed_scale);
> > > diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
> > > index 6a4479616479..16e7682474f3 100644
> > > --- a/include/linux/iio/consumer.h
> > > +++ b/include/linux/iio/consumer.h
> > > @@ -451,4 +451,60 @@ ssize_t iio_write_channel_ext_info(struct iio_channel
> > > *chan, const char *attr,
> > > */
> > > ssize_t iio_read_channel_label(struct iio_channel *chan, char *buf);
> > >
> > > +/**
> > > + * iio_event_mode() - get file mode for an event property
> > > + * @chan: Channel being queried
> > > + * @type: Event type (theshold, rate-of-change, etc.)
> > > + * @dir: Event direction (rising, falling, etc.)
> > > + * @info: Event property (enable, value, etc.)
> > > + *
> > > + * Determine an appropriate mode for sysfs files derived from this event.
> > > + *
> > > + * Return:
> > > + * - `0000` if the event is unsupported or otherwise unavailable
> > > + * - `0444` if the event is read-only
> > > + * - `0200` if the event is write-only
> > > + * - `0644` if the event is read-write
> > > + */
> > > +umode_t iio_event_mode(struct iio_channel *chan, enum iio_event_type type,
> > > + enum iio_event_direction dir, enum iio_event_info
> > > info);
> > > +
> > > +/**
> > > + * iio_read_event_processed_scale() - Read an event property
> > > + * @chan: Channel being queried
> > > + * @type: Event type (theshold, rate-of-change, etc.)
> > > + * @dir: Event direction (rising, falling, etc.)
> > > + * @info: Event property (enable, value, etc.)
> > > + * @val: Processed property value
> > > + * @scale: Factor to scale @val by
> > > + *
> > > + * Read a processed (scaled and offset) event property of a given channel.
> > > + *
> > > + * Return: 0 on success, or negative error on failure
> > > + */
> > > +int iio_read_event_processed_scale(struct iio_channel *chan,
> > > + enum iio_event_type type,
> > > + enum iio_event_direction dir,
> > > + enum iio_event_info info, int *val,
> > > + unsigned int scale);
> > > +
> > > +/**
> > > + * iio_write_event_processed_scale() - Read an event property
> > > + * @chan: Channel being queried
> > > + * @type: Event type (theshold, rate-of-change, etc.)
> > > + * @dir: Event direction (rising, falling, etc.)
> > > + * @info: Event property (enable, value, etc.)
> > > + * @processed: Processed property value
> > > + * @scale: Factor to scale @processed by
> > > + *
> > > + * Write a processed (scaled and offset) event property of a given channel.
> > > + *
> > > + * Return: 0 on success, or negative error on failure
> > > + */
> > > +int iio_write_event_processed_scale(struct iio_channel *chan,
> > > + enum iio_event_type type,
> > > + enum iio_event_direction dir,
> > > + enum iio_event_info info, int processed,
> > > + unsigned int scale);
> > > +
> > > #endif
>
Powered by blists - more mailing lists