Re: [PATCH 7/7] hwmon: iio: Add alarm support

[Sean Anderson <sean.anderson@linux.dev>]

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@linux.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