Message-ID: <9faeea8d-98b7-476e-9b4c-ed5be0a48913@roeck-us.net>
Date: Tue, 15 Jul 2025 12:26:21 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Sean Anderson <sean.anderson@...ux.dev>, Nuno Sá
 <noname.nuno@...il.com>, Jonathan Cameron <jic23@...nel.org>,
 Jean Delvare <jdelvare@...e.com>, 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/15/25 10:02, 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.
> 
>>> +			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.
> 

Sorry, I will neither accept backtraces not warning messages in hwmon code.
That risks polluting the kernel log. Propagate the error. I fail to see
the problem with that.

>>> +		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.
> 

Just return the error to the caller (without replacing the error).

Guenter

Powered by blists - more mailing lists