| 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: <11735f77-25cc-4220-b7be-e6fda8f72161@linux.dev>
Date: Thu, 17 Jul 2025 12:00:13 -0400
From: Sean Anderson <sean.anderson@...ux.dev>
To: Nuno Sá <noname.nuno@...il.com>,
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 7/7] hwmon: iio: Add alarm support
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
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
hold a mutex, we can just increment/decrement. I will remove the
saturation check to avoid confusion.
--Sean
Powered by blists - more mailing lists