| 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: <20250727171347.61a97f83@jic23-huawei>
Date: Sun, 27 Jul 2025 17:13:47 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Sean Anderson <sean.anderson@...ux.dev>
Cc: Jean Delvare <jdelvare@...e.com>, Guenter Roeck <linux@...ck-us.net>,
linux-iio@...r.kernel.org, linux-hwmon@...r.kernel.org, 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 Mon, 14 Jul 2025 21:20:18 -0400
Sean Anderson <sean.anderson@...ux.dev> 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>
Hi Sean,
A few minor comments inline.
> ---
>
> 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);
> +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,
Maybe rename info to ev_info or similar to avoid confusion with
indio_dev->info
> + 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;
> +
Perhaps a local variable
struct iio_info info;
info = indio_dev->info;
> + if (info == IIO_EV_INFO_ENABLE) {
> + if (!indio_dev->info->read_event_config)
> + return -EINVAL;
> +
> + raw = indio_dev->info->read_event_config(indio_dev,
raw = info->read_event_config(indio_dev, chan->channel,
type, dir);
> + 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);
> +
> +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,
Similar to above, feels like a local variable to shorten these would be good,
> + 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
Can we name this something more specific. Sounds like it might be
the mode of the events, not the mode of the file.
> + * @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.
This isn't precise unfortunately as if we have a mix of read only and rw
for different events (maybe some thresholds fixed and others not).
We should maybe say that it may indicate more control than actually possible.
In most cases it'll be right though.
> + *
> + * 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
> + */
So here is one of those bits of "don't do it the way we currently do it".
Move the docs next to the implementation in the c file rather than the header.
We are horribly inconsistent in IIO mostly because of younger me making a mess
of it. General thinking today is that we are much less likely to forget
to update docs if they are next to the code.
> +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