| 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: <pblyblite64vy5ghk77crga5y6f2a6bmngtxu6rlqmq3p7rzxt@srquqhwt43nr>
Date: Thu, 31 Jul 2025 11:52:10 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: Sean Anderson <sean.anderson@...ux.dev>
Cc: 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,
Andy Shevchenko <andy@...nel.org>, Nuno Sá <nuno.sa@...log.com>,
linux-kernel@...r.kernel.org, David Lechner <dlechner@...libre.com>
Subject: Re: [PATCH 7/7] hwmon: iio: Add alarm support
On Thu, Jul 17, 2025 at 12:00:13PM -0400, Sean Anderson wrote:
> On 7/16/25 02:37, Nuno Sá wrote:
> > On Tue, 2025-07-15 at 13:02 -0400, Sean Anderson wrote:
> >> On 7/15/25 07:28, Nuno Sá wrote:
> >> > On Mon, 2025-07-14 at 21:20 -0400, Sean Anderson wrote:
> >> > > Add alarm support based on IIO threshold events. The alarm is cleared on
> >> > > read, but will be set again if the condition is still present. This is
> >> > > detected by disabling and re-enabling the event. The same trick is done
> >> > > when creating the attribute to detect already-triggered events.
> >> > >
> >> > > The alarms are updated by an event listener. To keep the notifier call
> >> > > chain short, we create one listener per iio device, shared across all
> >> > > hwmon devices.
> >> > >
> >> > > To avoid dynamic creation of alarms, alarms for all possible events are
> >> > > allocated at creation. Lookup is done by a linear scan, as I expect
> >> > > events to occur rarely. If performance becomes an issue, a binary search
> >> > > could be done instead (or some kind of hash lookup).
> >> > >
> >> > > Signed-off-by: Sean Anderson <sean.anderson@...ux.dev>
> >> > > ---
> >> > >
> >> > > drivers/hwmon/iio_hwmon.c | 322 +++++++++++++++++++++++++++++++++++++-
> >> > > 1 file changed, 321 insertions(+), 1 deletion(-)
> >> > >
> >> > > diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
> >> > > index 3db4d4b30022..c963bc5452ba 100644
> >> > > --- a/drivers/hwmon/iio_hwmon.c
> >> > > +++ b/drivers/hwmon/iio_hwmon.c
> >> > > @@ -8,6 +8,7 @@
> >> > > #include <linux/slab.h>
> >> > > #include <linux/mod_devicetable.h>
> >> > > #include <linux/module.h>
> >> > > +#include <linux/notifier.h>
> >> > > #include <linux/err.h>
> >> > > #include <linux/platform_device.h>
> >> > > #include <linux/property.h>
> >> > > @@ -15,7 +16,192 @@
> >> > > #include <linux/hwmon.h>
> >> > > #include <linux/hwmon-sysfs.h>
> >> > > #include <linux/iio/consumer.h>
> >> > > +#include <linux/iio/events.h>
> >> > > +#include <linux/iio/iio.h>
> >> > > #include <linux/iio/types.h>
> >> > > +#include <uapi/linux/iio/events.h>
> >> > > +
> >> > > +/* Protects iio_hwmon_listeners and listeners' refcnt */
> >> > > +DEFINE_MUTEX(iio_hwmon_listener_lock);
> >> > > +LIST_HEAD(iio_hwmon_listeners);
> >> > > +
> >> > > +/**
> >> > > + * struct iio_hwmon_listener - Listener for IIO events
> >> > > + * @block: Notifier for events
> >> > > + * @ids: Array of IIO event ids, one per alarm
> >> > > + * @alarms: Bitmap of alarms
> >> > > + * @num_alarms: Length of @ids and @alarms
> >> > > + * @indio_dev: Device we are listening to
> >> > > + * @list: List of all listeners
> >> > > + * @refcnt: Reference count
> >> > > + */
> >> > > +struct iio_hwmon_listener {
> >> > > + struct notifier_block block;
> >> > > + u64 *ids;
> >> > > + unsigned long *alarms;
> >> > > + size_t num_alarms;
> >> > > +
> >> > > + struct iio_dev *indio_dev;
> >> > > + struct list_head list;
> >> > > + unsigned int refcnt;
> >> > > +};
> >> > > +
> >> > > +/**
> >> > > + * iio_hwmon_lookup_alarm() - Find an alarm by id
> >> > > + * @listener: Event listener
> >> > > + * @id: IIO event id
> >> > > + *
> >> > > + * Return: index of @id in @listener->ids, or -1 if not found
> >> > > + */
> >> > > +static ssize_t iio_hwmon_lookup_alarm(struct iio_hwmon_listener *listener,
> >> > > + u64 id)
> >> > > +{
> >> > > + ssize_t i;
> >> > > +
> >> > > + for (i = 0; i < listener->num_alarms; i++)
> >> > > + if (listener->ids[i] == id)
> >> > > + return i;
> >> > > +
> >> > > + return -1;
> >> > > +}
> >> > > +
> >> > > +static int iio_hwmon_listener_callback(struct notifier_block *block,
> >> > > + unsigned long action, void *data)
> >> > > +{
> >> > > + struct iio_hwmon_listener *listener =
> >> > > + container_of(block, struct iio_hwmon_listener, block);
> >> > > + struct iio_event_data *ev = data;
> >> > > + ssize_t i;
> >> > > +
> >> > > + if (action != IIO_NOTIFY_EVENT)
> >> > > + return NOTIFY_DONE;
> >> > > +
> >> > > + i = iio_hwmon_lookup_alarm(listener, ev->id);
> >> > > + if (i >= 0)
> >> > > + set_bit(i, listener->alarms);
> >> > > + else
> >> > > + dev_warn_once(&listener->indio_dev->dev,
> >> > > + "unknown event %016llx\n", ev->id);
> >> > > +
> >> > > + return NOTIFY_DONE;
> >> > > +}
> >> > > +
> >> > > +/**
> >> > > + * iio_event_id() - Calculate an IIO event id
> >> > > + * @channel: IIO channel for this event
> >> > > + * @type: Event type (theshold, rate-of-change, etc.)
> >> > > + * @dir: Event direction (rising, falling, etc.)
> >> > > + *
> >> > > + * Return: IIO event id corresponding to this event's IIO id
> >> > > + */
> >> > > +static u64 iio_event_id(struct iio_chan_spec const *chan,
> >> > > + enum iio_event_type type,
> >> > > + enum iio_event_direction dir)
> >> > > +{
> >> > > + if (chan->differential)
> >> > > + return IIO_DIFF_EVENT_CODE(chan->type, chan->channel,
> >> > > + chan->channel2, type, dir);
> >> > > + if (chan->modified)
> >> > > + return IIO_MOD_EVENT_CODE(chan->type, chan->channel,
> >> > > + chan->channel2, type, dir);
> >> > > + return IIO_UNMOD_EVENT_CODE(chan->type, chan->channel, type, dir);
> >> > > +}
> >> > > +
> >> > > +/**
> >> > > + * iio_hwmon_listener_get() - Get a listener for an IIO device
> >> > > + * @indio_dev: IIO device to listen to
> >> > > + *
> >> > > + * Look up or create a new listener for @indio_dev. The returned listener is
> >> > > + * registered with @indio_dev, but events still need to be manually enabled.
> >> > > + * You must call iio_hwmon_listener_put() when you are done.
> >> > > + *
> >> > > + * Return: Listener for @indio_dev, or an error pointer
> >> > > + */
> >> > > +static struct iio_hwmon_listener *iio_hwmon_listener_get(struct iio_dev
> >> > > *indio_dev)
> >> > > +{
> >> > > + struct iio_hwmon_listener *listener;
> >> > > + int err = -ENOMEM;
> >> > > + size_t i, j;
> >> > > +
> >> > > + guard(mutex)(&iio_hwmon_listener_lock);
> >> > > + list_for_each_entry(listener, &iio_hwmon_listeners, list) {
> >> > > + if (listener->indio_dev == indio_dev) {
> >> > > + if (likely(listener->refcnt != UINT_MAX))
> >> > > + listener->refcnt++;
> >> >
> >> > I dunno for the above to ever happen :).
> >>
> >> Well, I can remove it if you like.
> >>
> >> > And as Andy stated, let's just use proper refcount APIs.
> >>
> >> No point in using atomic ops if they are only accessed under a mutex.
> >
> > Not the point... If there are proper APIs for handling things like this, not sure why
> > not using and then coming up with things like the above? And the same goes to the
> > release path.
>
> The API is for doing reference counts *atomically*. If you do not need
> atomic reference counting, then it is the *wrong* API. I suggest reading
Well, It won't make your code wrong. It's just about re-using what we have already.
But my main complain was about having your own saturation checks in here.
I also dislike the release path where you do have to explicitly check for 0 to
call the cleanup API. That is all handled already. Not to mention that it is a fairly
common pattern to use these APIs even if you don't really __need__ it's atomicity.
> the block comment at the beginning of refcnt.h to see the sorts of
> contortions it has to go through because it is an atomic API. Since we
And? It's very well hidden in the API... This is also not a fastpath at
all so performance is also not a concern AFAICT.
Up to the maintainers anyways but I cannot say I agree with it. So, I
guess we can agree in disagreeing :)
- Nuno Sá
Powered by blists - more mailing lists