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

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

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.

>> +			return listener;
>> +		}
>> +	}
>> +
>> +	listener = kzalloc(sizeof(*listener), GFP_KERNEL);
>> +	if (!listener)
>> +		goto err_unlock;
>> +
>> +	listener->refcnt = 1;
>> +	listener->indio_dev = indio_dev;
>> +	listener->block.notifier_call = iio_hwmon_listener_callback;
>> +	for (i = 0; i < indio_dev->num_channels; i++)
>> +		listener->num_alarms += indio_dev-
>> >channels[i].num_event_specs;
>> +
>> +	listener->ids = kcalloc(listener->num_alarms, sizeof(*listener->ids),
>> +				GFP_KERNEL);
>> +	listener->alarms = bitmap_zalloc(listener->num_alarms, GFP_KERNEL);
>> +	if (!listener->ids || !listener->alarms)
>> +		goto err_listener;
>> +
>> +	i = 0;
>> +	for (j = 0; j < indio_dev->num_channels; j++) {
>> +		struct iio_chan_spec const *chan = &indio_dev->channels[j];
>> +		size_t k;
>> +
>> +		for (k = 0; k < chan->num_event_specs; k++)
>> +			listener->ids[i++] =
>> +				iio_event_id(chan, chan->event_spec[k].type,
>> +					     chan->event_spec[k].dir);
>> +	}
>> +
>> +	err = iio_event_register(indio_dev, &listener->block);
>> +	if (err)
>> +		goto err_alarms;
>> +
>> +	list_add(&listener->list, &iio_hwmon_listeners);
>> +	mutex_unlock(&iio_hwmon_listener_lock);
>> +	return listener;
>> +
>> +err_alarms:
>> +	kfree(listener->alarms);
>> +	kfree(listener->ids);
>> +err_listener:
>> +	kfree(listener);
>> +err_unlock:
>> +	mutex_unlock(&iio_hwmon_listener_lock);
>> +	return ERR_PTR(err);
>> +}
>> +
>> +/**
>> + * iio_hwmon_listener_put() - Release a listener
>> + * @data: &struct iio_hwmon_listener to release
>> + *
>> + * For convenience, @data is void.
>> + */
>> +static void iio_hwmon_listener_put(void *data)
>> +{
>> +	struct iio_hwmon_listener *listener = data;
>> +
>> +	scoped_guard(mutex, &iio_hwmon_listener_lock) {
>> +		if (unlikely(listener->refcnt == UINT_MAX))
>> +			return;
>> +
>> +		if (--listener->refcnt)
>> +			return;
>> +
>> +		list_del(&listener->list);
>> +		iio_event_unregister(listener->indio_dev, &listener->block);
>> +	}
>> +
>> +	kfree(listener->alarms);
>> +	kfree(listener->ids);
>> +	kfree(listener);
>> +}
>>  
>>  /**
>>   * struct iio_hwmon_state - device instance state
>> @@ -143,6 +329,68 @@ static ssize_t iio_hwmon_write_event(struct device *dev,
>>  	return count;
>>  }
>>  
>> +/**
>> + * struct iio_hwmon_alarm_attribute - IIO HWMON alarm attribute
>> + * @dev_attr: Base device attribute
>> + * @listener: Listener for this alarm
>> + * @index: Index of the channel in the IIO HWMON
>> + * @alarm: Index of the alarm within @listener
>> + */
>> +struct iio_hwmon_alarm_attribute {
>> +	struct device_attribute dev_attr;
>> +	struct iio_hwmon_listener *listener;
>> +	size_t index;
>> +	size_t alarm;
>> +};
>> +#define to_alarm_attr(_dev_attr) \
>> +	container_of(_dev_attr, struct iio_hwmon_alarm_attribute, dev_attr)
>> +
>> +/**
>> + * iio_hwmon_alarm_toggle() - Turn an event off and back on again
>> + * @chan: Channel of the event
>> + * @dir: Event direction (rising, falling, etc.)
>> + *
>> + * Toggle an event's enable so we get notified if the alarm is already
>> + * triggered. We use this to convert IIO's event-triggered events into
>> + * level-triggered alarms.
>> + *
>> + * Return: 0 on success or negative error on failure
>> + */
>> +static int iio_hwmon_alarm_toggle(struct iio_channel *chan,
>> +				  enum iio_event_direction dir)
>> +{
>> +	int ret;
>> +
>> +	ret = iio_write_event_processed_scale(chan, IIO_EV_TYPE_THRESH, dir,
>> +					      IIO_EV_INFO_ENABLE, 0, 1);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return iio_write_event_processed_scale(chan, IIO_EV_TYPE_THRESH, dir,
>> +					       IIO_EV_INFO_ENABLE, 1, 1);
>> +}
>> +
>> +static ssize_t iio_hwmon_read_alarm(struct device *dev,
>> +				    struct device_attribute *attr,
>> +				    char *buf)
>> +{
>> +	struct iio_hwmon_alarm_attribute *sattr = to_alarm_attr(attr);
>> +	struct iio_hwmon_state *state = dev_get_drvdata(dev);
>> +	struct iio_channel *chan = &state->channels[sattr->index];
>> +
>> +	if (test_and_clear_bit(sattr->alarm, sattr->listener->alarms)) {
>> +		u64 id = sattr->listener->ids[sattr->alarm];
>> +		enum iio_event_direction dir =
>> IIO_EVENT_CODE_EXTRACT_DIR(id);
>> +
>> +		WARN_ON(iio_hwmon_alarm_toggle(chan, dir));
> 
> WARN_ON() is highly discouraged... Also do we really need a "scary" splat in
> this case?

OK, maybe dev_warn then. I don't want to propagate the error because I think
it's more important to tell userspace that the alarm went off than if there
was a problem determining if the alarm is still active.

>> +		strcpy(buf, "1\n");
>> +		return 2;
>> +	}
>> +
>> +	strcpy(buf, "0\n");
>> +	return 2;
> 
> As stated, sysfs_emit()
> 
>> +}
>> +
>>  static int add_device_attr(struct device *dev, struct iio_hwmon_state *st,
>>  			   ssize_t (*show)(struct device *dev,
>>  					   struct device_attribute *attr,
>> @@ -205,6 +453,63 @@ static int add_event_attr(struct device *dev, struct
>> iio_hwmon_state *st,
>>  	return 0;
>>  }
>>  
>> +static int add_alarm_attr(struct device *dev, struct iio_hwmon_state *st,
>> +			  int i, enum iio_event_direction dir,
>> +			  const char *fmt, ...)
>> +{
>> +	struct iio_hwmon_alarm_attribute *a;
>> +	struct iio_hwmon_listener *listener;
>> +	ssize_t alarm;
>> +	umode_t mode;
>> +	va_list ap;
>> +	int ret;
>> +
>> +	mode = iio_event_mode(&st->channels[i], IIO_EV_TYPE_THRESH, dir,
>> +			      IIO_EV_INFO_ENABLE);
>> +	if (!(mode & 0200))
>> +		return 0;
>> +
>> +	listener = iio_hwmon_listener_get(st->channels[i].indio_dev);
>> +	if (listener == ERR_PTR(-EBUSY))
>> +		return 0;
> 
> Maybe I missed something, where can we get -EBUSY? And should we ignore it?

Oh, this was from before I refactored the notification API to allow kernel
consumers to co-exist with userspace ones. So this can't occur.

>> +	if (IS_ERR(listener))
>> +		return PTR_ERR(listener);
>> +
>> +	ret = devm_add_action_or_reset(dev, iio_hwmon_listener_put,
>> listener);
>> +	if (ret)
>> +		return ret;
>> +
>> +	alarm = iio_hwmon_lookup_alarm(listener,
>> +				       iio_event_id(st->channels[i].channel,
>> +						    IIO_EV_TYPE_THRESH,
>> dir));
>> +	if (WARN_ON_ONCE(alarm < 0))
>> +		return -ENOENT;
>> +
> 
> Again, I would drop WARN_ON_ONCE()

This can only occur if there is a bug in the kernel. We should have returned
0 from iio_event_mode() before we get to this point.

--Sean