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++;
+			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));
+		strcpy(buf, "1\n");
+		return 2;
+	}
+
+	strcpy(buf, "0\n");
+	return 2;
+}
+
 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;
+	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;
+
+	ret = iio_hwmon_alarm_toggle(&st->channels[i], dir);
+	if (ret)
+		return ret;
+
+	a = devm_kzalloc(dev, sizeof(*a), GFP_KERNEL);
+	if (!a)
+		return -ENOMEM;
+
+	sysfs_attr_init(&a->dev_attr.attr);
+	va_start(ap, fmt);
+	a->dev_attr.attr.name = devm_kvasprintf(dev, GFP_KERNEL, fmt, ap);
+	va_end(ap);
+	if (!a->dev_attr.attr.name)
+		return -ENOMEM;
+
+	a->dev_attr.show = iio_hwmon_read_alarm;
+	a->dev_attr.attr.mode = 0400;
+	a->listener = listener;
+	a->index = i;
+	a->alarm = alarm;
+
+	st->attrs[st->num_attrs++] = &a->dev_attr.attr;
+	return 0;
+}
+
 static int iio_hwmon_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -238,7 +543,7 @@ static int iio_hwmon_probe(struct platform_device *pdev)
 		st->num_channels++;
 
 	st->attrs = devm_kcalloc(dev,
-				 4 * st->num_channels + 1, sizeof(*st->attrs),
+				 7 * st->num_channels + 1, sizeof(*st->attrs),
 				 GFP_KERNEL);
 	if (st->attrs == NULL)
 		return -ENOMEM;
@@ -298,6 +603,21 @@ static int iio_hwmon_probe(struct platform_device *pdev)
 				     "%s%d_max", prefix, n);
 		if (ret)
 			return ret;
+
+		ret = add_alarm_attr(dev, st, i, IIO_EV_DIR_RISING,
+				     "%s%d_max_alarm", prefix, n);
+		if (ret)
+			return ret;
+
+		ret = add_alarm_attr(dev, st, i, IIO_EV_DIR_FALLING,
+				     "%s%d_min_alarm", prefix, n);
+		if (ret)
+			return ret;
+
+		ret = add_alarm_attr(dev, st, i, IIO_EV_DIR_EITHER,
+				     "%s%d_alarm", prefix, n);
+		if (ret)
+			return ret;
 	}
 
 	devm_free_pages(dev, (unsigned long)buf);
-- 
2.35.1.1320.gc452695387.dirty

On Mon, 14 Jul 2025 21:20:23 -0400
Sean Anderson <sean.anderson@linux.dev> 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>

A few minor comments inline.

Thanks,

Jonathan

> + * 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);

Guard + unlock.  However, in general combining cleanup.h stuff and
gotos is a bad idea.  You might better off wrapping a function
with the lock in the outer function

Alternative, use local variables for ids and alarms.  Use __free()
on those + return_ptr(listener) and no_free_ptr() as you assign
the elements of listener after there is no possibility of error.


> +	list_for_each_entry(listener, &iio_hwmon_listeners, list) {
> +		if (listener->indio_dev == indio_dev) {
Deep nest.  I'd do
		if (listener->indio_dev != indio-dev)
			continue;

		if (likely(listener->refcnt != UINT_MAX)
//Needs a comment on why this make sense as opposed to returning an error.

			listener->refcnt++;
		return listener;
> +			if (likely(listener->refcnt != UINT_MAX))
> +				listener->refcnt++;
> +			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'd prefer this split into a pair of checks and separate labels.

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

Maybe use a kref and kref_put() with a suitable release to
do the cleanup.

> + * @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);
> +}
>

Hi Sean,

kernel test robot noticed the following build warnings:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on groeck-staging/hwmon-next akpm-mm/mm-nonmm-unstable linus/master v6.16-rc6 next-20250715]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Sean-Anderson/math64-Add-div64_s64_rem/20250715-092337
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20250715012023.2050178-8-sean.anderson%40linux.dev
patch subject: [PATCH 7/7] hwmon: iio: Add alarm support
config: i386-randconfig-063-20250716 (https://download.01.org/0day-ci/archive/20250716/202507161550.frzFNyCa-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250716/202507161550.frzFNyCa-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507161550.frzFNyCa-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/hwmon/iio_hwmon.c:25:1: sparse: sparse: symbol 'iio_hwmon_listener_lock' was not declared. Should it be static?
>> drivers/hwmon/iio_hwmon.c:26:1: sparse: sparse: symbol 'iio_hwmon_listeners' was not declared. Should it be static?

vim +/iio_hwmon_listener_lock +25 drivers/hwmon/iio_hwmon.c

    23	
    24	/* Protects iio_hwmon_listeners and listeners' refcnt */
  > 25	DEFINE_MUTEX(iio_hwmon_listener_lock);
  > 26	LIST_HEAD(iio_hwmon_listeners);
    27	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

On 7/14/25 18:20, 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).
> 

I am very concerned about this. The context suggests that the iio events
are just that - events without specific association to hardware or system
limits. Hardware monitoring limits are system specific limits, which are not
supposed to change at runtime. A high voltage or temperature warning is
just that - it is not supposed to trigger a change in the event limit.
If anything, it is supposed to trigger some action to bring the observed
value back to normal.

For this series to move forward, there needs to be some guarantee that
the limits are used and usable only as intended, and can not be used for
random thresholds. The idea of "if a temperature alarm is triggered, do
something and change the threshold temperature" is not an acceptable use
for hardware monitoring alarms.

Guenter

On 7/15/25 15:34, Guenter Roeck wrote:
> On 7/14/25 18:20, 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).
>>
> 
> I am very concerned about this. The context suggests that the iio events
> are just that - events without specific association to hardware or system
> limits. Hardware monitoring limits are system specific limits, which are not
> supposed to change at runtime. A high voltage or temperature warning is
> just that - it is not supposed to trigger a change in the event limit.
> If anything, it is supposed to trigger some action to bring the observed
> value back to normal.

If the system integrator has instantiated this driver, then the associated
IIO channels correspond to physical values related to the health of the
system. Other IIO channels should not be attached to the iio-hwmon driver.

For example, in my use case the Xilinx AMS was implemented as an IIO
device because it's a generic ADC, and several of the channels can
monitor arbitrary analog voltages. However, many channels are
permanently connected to the SoC's power rails and to internal
temperature probes. These channels are best exposed as an hwmon device
to take advantage of existing userspace tooling (e.g. lm-sensors).

The above paragraph in the commit message specifically refers to the
approach taken to handle IIO events for a given device. As we process
the hwmon's IIO channels, we create alarm attributes for the
corresponding events. Because we don't know which IIO events we are
interested in when we create the IIO listener, there are two general
approaches:

- We could allocate some memory for the alarm and then add it to a list
  or hash table in the listener. When the listener gets an event it
  would then search the list or hash table for the appropriate alarm.
- We can allocate memory for all possible events up front. When we want
  to create an alarm we look up the appropriate event.

I chose the latter approach because I believe that there are typically
not too many events on a given IIO device (i.e. dozens) and it makes the
lookup simpler, since we can just iterate through an array (or do a
binary search).

> For this series to move forward, there needs to be some guarantee that
> the limits are used and usable only as intended, and can not be used for
> random thresholds. The idea of "if a temperature alarm is triggered, do
> something and change the threshold temperature" is not an acceptable use
> for hardware monitoring alarms.

What userspace sets the limits to or does in response to an alarm is not
the kernel's concern. That said, I suspect the most-likely userspace response
is to log the alarm, possibly to some remote system.

--Sean

Hi Sean,

kernel test robot noticed the following build warnings:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on groeck-staging/hwmon-next akpm-mm/mm-nonmm-unstable linus/master v6.16-rc6 next-20250715]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Sean-Anderson/math64-Add-div64_s64_rem/20250715-092337
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20250715012023.2050178-8-sean.anderson%40linux.dev
patch subject: [PATCH 7/7] hwmon: iio: Add alarm support
config: i386-buildonly-randconfig-001-20250715 (https://download.01.org/0day-ci/archive/20250715/202507152309.wBE1wHwM-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14+deb12u1) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250715/202507152309.wBE1wHwM-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507152309.wBE1wHwM-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> Warning: drivers/hwmon/iio_hwmon.c:99 function parameter 'chan' not described in 'iio_event_id'
>> Warning: drivers/hwmon/iio_hwmon.c:99 Excess function parameter 'channel' description in 'iio_event_id'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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 :). And as Andy stated, let's just use
proper refcount APIs.
> +			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?

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

> +	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()

- Nuno Sá

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

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.

- Nuno Sá
 
> 
> > > +			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

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

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

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á

On Thu, 31 Jul 2025 11:52:10 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> 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@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  
> 
> 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 :)

I'd prefer using the standard refcount API.  Sure it's not a performant
choice in all cases, but it wins on basis of familiarity and reduced
burden to review and maintenance.

Jonathan

> 
> - Nuno Sá
> 

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

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

On 7/15/25 15:26, Guenter Roeck wrote:
> 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@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.
>>
> 
> 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.

If userspace is interested in the alarm it certainly is more useful to
say "yes, there was an alarm!" than "sorry, something went wrong, but we
can't tell you what and if you retry there will no longer be an alarm or
an error so it will look some spurious problem." 

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

This should be a BUG() but that sort of thing is no longer allowed.

It cannot occur except if something has gone catastrophically wrong (e.g. bit
flips due to radiation or something similar). Returning an error is just
us being nice; at this point the system unstable in some unknown way
that may not allow for recovery.

--Sean

On Mon, Jul 14, 2025 at 09:20:23PM -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).

...

>  #include <linux/hwmon-sysfs.h>

+ blank line here..

>  #include <linux/iio/consumer.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/iio.h>
>  #include <linux/iio/types.h>

...and here?

> +#include <uapi/linux/iio/events.h>

...

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

-ENOENT ?
This will allow to propagate an error code to the upper layer(s).

> +}

...

> +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);

Do you need an atomic set?

> +	else
> +		dev_warn_once(&listener->indio_dev->dev,
> +			      "unknown event %016llx\n", ev->id);
> +
> +	return NOTIFY_DONE;
> +}

...

> +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++;
> +			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);

With guard() ???

> +	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);

What about using __free()?

> +}

...

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

Can the refcount_t be used with the respective APIs? Or even kref?

> +		list_del(&listener->list);
> +		iio_event_unregister(listener->indio_dev, &listener->block);
> +	}
> +
> +	kfree(listener->alarms);
> +	kfree(listener->ids);
> +	kfree(listener);
> +}

...

> +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));

> +		strcpy(buf, "1\n");
> +		return 2;

> +	}
> +
> +	strcpy(buf, "0\n");
> +	return 2;

Better to assign the value and

	return sysfs_emit(...);

which will make even easier to recognize that this is supplied to user via
sysfs.

> +}

...

> +static int add_alarm_attr(struct device *dev, struct iio_hwmon_state *st,
> +			  int i, enum iio_event_direction dir,
> +			  const char *fmt, ...)

Same comments as per previous patches.

-- 
With Best Regards,
Andy Shevchenko

On 7/15/25 04:50, Andy Shevchenko wrote:
> On Mon, Jul 14, 2025 at 09:20:23PM -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).
> 
> ...
> 
>>  #include <linux/hwmon-sysfs.h>
> 
> + blank line here..

why?

>>  #include <linux/iio/consumer.h>
>> +#include <linux/iio/events.h>
>> +#include <linux/iio/iio.h>
>>  #include <linux/iio/types.h>
> 
> ...and here?

OK

>> +#include <uapi/linux/iio/events.h>
> 
> ...
> 
>> +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;
> 
> -ENOENT ?
> This will allow to propagate an error code to the upper layer(s).

I suppose. But I think

alarm = iio_hwmon_lookup_alarm(...);
if (alarm < 0)
	return -ENOENT;

is clearer than

alarm = iio_hwmon_lookup_alarm(...);
if (alarm < 0)
	return alarm;

because you don't have to read the definition of iio_hwmon_lookup_alarm
to determine what the return value is.

>> +}
> 
> ...
> 
>> +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);
> 
> Do you need an atomic set?

Yes. This protects against concurrent access by iio_hwmon_read_alarm.

>> +	else
>> +		dev_warn_once(&listener->indio_dev->dev,
>> +			      "unknown event %016llx\n", ev->id);
>> +
>> +	return NOTIFY_DONE;
>> +}
> 
> ...
> 
>> +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++;
>> +			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);
> 
> With guard() ???

Whoops. Missed that when refactoring.

>> +	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);
> 
> What about using __free()?

That works for listener, but not for alarms or ids.

>> +}
> 
> ...
> 
>> +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;
> 
> Can the refcount_t be used with the respective APIs? Or even kref?

Why? We do all the manipulation under a mutex, so there is no point in
atomic access. Instead of the games refcnt_t has to play to try and
prevent overflow we can just check for it directly.

>> +		list_del(&listener->list);
>> +		iio_event_unregister(listener->indio_dev, &listener->block);
>> +	}
>> +
>> +	kfree(listener->alarms);
>> +	kfree(listener->ids);
>> +	kfree(listener);
>> +}
> 
> ...
> 
>> +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));
> 
>> +		strcpy(buf, "1\n");
>> +		return 2;
> 
>> +	}
>> +
>> +	strcpy(buf, "0\n");
>> +	return 2;
> 
> Better to assign the value and
>
> 	return sysfs_emit(...);
> 
> which will make even easier to recognize that this is supplied to user via
> sysfs.

:l

the things we do to avoid memcpy...

--Sean

>> +}
> 
> ...
> 
>> +static int add_alarm_attr(struct device *dev, struct iio_hwmon_state *st,
>> +			  int i, enum iio_event_direction dir,
>> +			  const char *fmt, ...)
> 
> Same comments as per previous patches.
>

On Tue, Jul 15, 2025 at 12:20:24PM -0400, Sean Anderson wrote:
> On 7/15/25 04:50, Andy Shevchenko wrote:
> > On Mon, Jul 14, 2025 at 09:20:23PM -0400, Sean Anderson wrote:

...

> >>  #include <linux/hwmon-sysfs.h>
> > 
> > + blank line here..
> 
> why?

To group the subsystem related headers (which are more custom and less generic).
This allows to follow what the subsystems are in use and what APIs / types are
taken.

> >>  #include <linux/iio/consumer.h>
> >> +#include <linux/iio/events.h>
> >> +#include <linux/iio/iio.h>
> >>  #include <linux/iio/types.h>
> > 
> > ...and here?
> 
> OK
> 
> >> +#include <uapi/linux/iio/events.h>

As similar here, to visually split uAPI and the rest. This increases
readability and maintenance.

...

> >> +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;
> > 
> > -ENOENT ?
> > This will allow to propagate an error code to the upper layer(s).
> 
> I suppose. But I think
> 
> alarm = iio_hwmon_lookup_alarm(...);
> if (alarm < 0)
> 	return -ENOENT;
> 
> is clearer than

I disagree. This makes it worth as it shadows other possible code(s), if any,
and makes harder to follow as reader has to check the callee implementation.

The shadow error codes need a justification.

> alarm = iio_hwmon_lookup_alarm(...);
> if (alarm < 0)
> 	return alarm;
> 
> because you don't have to read the definition of iio_hwmon_lookup_alarm
> to determine what the return value is.

Exactly my point!

> >> +}

...

> >> +err_alarms:
> >> +	kfree(listener->alarms);
> >> +	kfree(listener->ids);
> >> +err_listener:
> >> +	kfree(listener);
> >> +err_unlock:
> >> +	mutex_unlock(&iio_hwmon_listener_lock);
> >> +	return ERR_PTR(err);
> > 
> > What about using __free()?
> 
> That works for listener, but not for alarms or ids.

Why not?

...

> >> +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;
> > 
> > Can the refcount_t be used with the respective APIs? Or even kref?
> 
> Why? We do all the manipulation under a mutex, so there is no point in
> atomic access. Instead of the games refcnt_t has to play to try and
> prevent overflow we can just check for it directly.

refcount_t provides a facility of overflow/underflow. Also it gives better
understanding from the data type to see which value and how does that.

> >> +		list_del(&listener->list);
> >> +		iio_event_unregister(listener->indio_dev, &listener->block);
> >> +	}
> >> +
> >> +	kfree(listener->alarms);
> >> +	kfree(listener->ids);
> >> +	kfree(listener);
> >> +}

...

> >> +	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));
> > 
> >> +		strcpy(buf, "1\n");
> >> +		return 2;
> > 
> >> +	}
> >> +
> >> +	strcpy(buf, "0\n");
> >> +	return 2;
> > 
> > Better to assign the value and
> >
> > 	return sysfs_emit(...);
> > 
> > which will make even easier to recognize that this is supplied to user via
> > sysfs.
> 
> :l
> 
> the things we do to avoid memcpy...

...for the cost of readability. Also this is a slow path.

-- 
With Best Regards,
Andy Shevchenko

On 7/16/25 06:08, Andy Shevchenko wrote:
> On Tue, Jul 15, 2025 at 12:20:24PM -0400, Sean Anderson wrote:
>> On 7/15/25 04:50, Andy Shevchenko wrote:
>> > On Mon, Jul 14, 2025 at 09:20:23PM -0400, Sean Anderson wrote:
> 
> ...
> 
>> >>  #include <linux/hwmon-sysfs.h>
>> > 
>> > + blank line here..
>> 
>> why?
> 
> To group the subsystem related headers (which are more custom and less generic).
> This allows to follow what the subsystems are in use and what APIs / types are
> taken.

Then you should send a patch for coding-style.rst.

>> >>  #include <linux/iio/consumer.h>
>> >> +#include <linux/iio/events.h>
>> >> +#include <linux/iio/iio.h>
>> >>  #include <linux/iio/types.h>
>> > 
>> > ...and here?
>> 
>> OK
>> 
>> >> +#include <uapi/linux/iio/events.h>
> 
> As similar here, to visually split uAPI and the rest. This increases
> readability and maintenance.
> 
> ...
> 
>> >> +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;
>> > 
>> > -ENOENT ?
>> > This will allow to propagate an error code to the upper layer(s).
>> 
>> I suppose. But I think
>> 
>> alarm = iio_hwmon_lookup_alarm(...);
>> if (alarm < 0)
>> 	return -ENOENT;
>> 
>> is clearer than
> 
> I disagree. This makes it worth as it shadows other possible code(s), if any,
> and makes harder to follow as reader has to check the callee implementation.
> 
> The shadow error codes need a justification.

OK, I will return a bool next time to avoid any misconceptions that the return
code means anything other than "found" or "not found"

>> alarm = iio_hwmon_lookup_alarm(...);
>> if (alarm < 0)
>> 	return alarm;
>> 
>> because you don't have to read the definition of iio_hwmon_lookup_alarm
>> to determine what the return value is.
> 
> Exactly my point!

your point is that you want readers to have to read the definition of
iio_hwmon_lookup_alarm in order to determine that ENOENT is a possible
error from add_alarm_attr? I don't follow.

>> >> +}
> 
> ...
> 
>> >> +err_alarms:
>> >> +	kfree(listener->alarms);
>> >> +	kfree(listener->ids);
>> >> +err_listener:
>> >> +	kfree(listener);
>> >> +err_unlock:
>> >> +	mutex_unlock(&iio_hwmon_listener_lock);
>> >> +	return ERR_PTR(err);
>> > 
>> > What about using __free()?
>> 
>> That works for listener, but not for alarms or ids.
> 
> Why not?
> 
> ...
> 
>> >> +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;
>> > 
>> > Can the refcount_t be used with the respective APIs? Or even kref?
>> 
>> Why? We do all the manipulation under a mutex, so there is no point in
>> atomic access. Instead of the games refcnt_t has to play to try and
>> prevent overflow we can just check for it directly.
> 
> refcount_t provides a facility of overflow/underflow.

refcount_t can't prevent underflow because it's atomic. All it can do is
warn after the fact. And of course overflow is handled properly here.
But it can't occur in practice unless you specifically load multiple
devicetrees at runtime. So we don't need it anyway.

> Also it gives better
> understanding from the data type to see which value and how does that.

That's why I named the variable "refcnt".

>> >> +		list_del(&listener->list);
>> >> +		iio_event_unregister(listener->indio_dev, &listener->block);
>> >> +	}
>> >> +
>> >> +	kfree(listener->alarms);
>> >> +	kfree(listener->ids);
>> >> +	kfree(listener);
>> >> +}
> 
> ...
> 
>> >> +	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));
>> > 
>> >> +		strcpy(buf, "1\n");
>> >> +		return 2;
>> > 
>> >> +	}
>> >> +
>> >> +	strcpy(buf, "0\n");
>> >> +	return 2;
>> > 
>> > Better to assign the value and
>> >
>> > 	return sysfs_emit(...);
>> > 
>> > which will make even easier to recognize that this is supplied to user via
>> > sysfs.
>> 
>> :l
>> 
>> the things we do to avoid memcpy...
> 
> ...for the cost of readability. Also this is a slow path.

On Thu, Jul 17, 2025 at 12:23:58PM -0400, Sean Anderson wrote:
> On 7/16/25 06:08, Andy Shevchenko wrote:
> > On Tue, Jul 15, 2025 at 12:20:24PM -0400, Sean Anderson wrote:
> >> On 7/15/25 04:50, Andy Shevchenko wrote:
> >> > On Mon, Jul 14, 2025 at 09:20:23PM -0400, Sean Anderson wrote:

...

> >> >>  #include <linux/hwmon-sysfs.h>
> >> > 
> >> > + blank line here..
> >> 
> >> why?
> > 
> > To group the subsystem related headers (which are more custom and less generic).
> > This allows to follow what the subsystems are in use and what APIs / types are
> > taken.
> 
> Then you should send a patch for coding-style.rst.

Does any of the common sense approach need to be written in the documentation?

> >> >>  #include <linux/iio/consumer.h>
> >> >> +#include <linux/iio/events.h>
> >> >> +#include <linux/iio/iio.h>
> >> >>  #include <linux/iio/types.h>
> >> > 
> >> > ...and here?
> >> 
> >> OK
> >> 
> >> >> +#include <uapi/linux/iio/events.h>
> > 
> > As similar here, to visually split uAPI and the rest. This increases
> > readability and maintenance.

...

> >> >> +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;
> >> > 
> >> > -ENOENT ?
> >> > This will allow to propagate an error code to the upper layer(s).
> >> 
> >> I suppose. But I think
> >> 
> >> alarm = iio_hwmon_lookup_alarm(...);
> >> if (alarm < 0)
> >> 	return -ENOENT;
> >> 
> >> is clearer than
> > 
> > I disagree. This makes it worth as it shadows other possible code(s), if any,
> > and makes harder to follow as reader has to check the callee implementation.
> > 
> > The shadow error codes need a justification.
> 
> OK, I will return a bool next time to avoid any misconceptions that the return
> code means anything other than "found" or "not found"

This makes sense. And IIRC it's even documented.

> >> alarm = iio_hwmon_lookup_alarm(...);
> >> if (alarm < 0)
> >> 	return alarm;
> >> 
> >> because you don't have to read the definition of iio_hwmon_lookup_alarm
> >> to determine what the return value is.
> > 
> > Exactly my point!
> 
> your point is that you want readers to have to read the definition of
> iio_hwmon_lookup_alarm in order to determine that ENOENT is a possible
> error from add_alarm_attr? I don't follow.

No, my point is that readers should not care about error code. If it's
propagated to the upper layer, the upper layer will decide on how to proceed.
And -ENOENT is de facto standard for "entity not found".

> >> >> +}

...

> >> >> +err_alarms:
> >> >> +	kfree(listener->alarms);
> >> >> +	kfree(listener->ids);
> >> >> +err_listener:
> >> >> +	kfree(listener);
> >> >> +err_unlock:
> >> >> +	mutex_unlock(&iio_hwmon_listener_lock);
> >> >> +	return ERR_PTR(err);
> >> > 
> >> > What about using __free()?
> >> 
> >> That works for listener, but not for alarms or ids.
> > 
> > Why not?

No answer? Have you checked how cleanup.h suggests to avoid cleaning the memory
when it's supposed to be used later on?

...

> >> >> +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;
> >> > 
> >> > Can the refcount_t be used with the respective APIs? Or even kref?
> >> 
> >> Why? We do all the manipulation under a mutex, so there is no point in
> >> atomic access. Instead of the games refcnt_t has to play to try and
> >> prevent overflow we can just check for it directly.
> > 
> > refcount_t provides a facility of overflow/underflow.
> 
> refcount_t can't prevent underflow because it's atomic. All it can do is
> warn after the fact. And of course overflow is handled properly here.
> But it can't occur in practice unless you specifically load multiple
> devicetrees at runtime. So we don't need it anyway.

It will warn the user in such cases. Your code won't do it, even if it's not a
big deal or never happens situation, it's still better to use in-kernel
standard ways of handling these things.

> > Also it gives better
> > understanding from the data type to see which value and how does that.
> 
> That's why I named the variable "refcnt".

Yes, and that's why I asked about existing interface / API / type to use.

> >> >> +		list_del(&listener->list);
> >> >> +		iio_event_unregister(listener->indio_dev, &listener->block);
> >> >> +	}
> >> >> +
> >> >> +	kfree(listener->alarms);
> >> >> +	kfree(listener->ids);
> >> >> +	kfree(listener);
> >> >> +}

-- 
With Best Regards,
Andy Shevchenko

On 7/21/25 03:42, Andy Shevchenko wrote:
> On Thu, Jul 17, 2025 at 12:23:58PM -0400, Sean Anderson wrote:
>> On 7/16/25 06:08, Andy Shevchenko wrote:
>> > On Tue, Jul 15, 2025 at 12:20:24PM -0400, Sean Anderson wrote:
>> >> On 7/15/25 04:50, Andy Shevchenko wrote:
>> >> > On Mon, Jul 14, 2025 at 09:20:23PM -0400, Sean Anderson wrote:
> 
> ...
> 
>> >> >>  #include <linux/hwmon-sysfs.h>
>> >> > 
>> >> > + blank line here..
>> >> 
>> >> why?
>> > 
>> > To group the subsystem related headers (which are more custom and less generic).
>> > This allows to follow what the subsystems are in use and what APIs / types are
>> > taken.
>> 
>> Then you should send a patch for coding-style.rst.
> 
> Does any of the common sense approach need to be written in the documentation?

Yes! My base assumption would be that includes should be alphabetized,
but that no other ordering or spacing is necessary. Your "common sense"
is not so common.

>> >> >>  #include <linux/iio/consumer.h>
>> >> >> +#include <linux/iio/events.h>
>> >> >> +#include <linux/iio/iio.h>
>> >> >>  #include <linux/iio/types.h>
>> >> > 
>> >> > ...and here?
>> >> 
>> >> OK
>> >> 
>> >> >> +#include <uapi/linux/iio/events.h>
>> > 
>> > As similar here, to visually split uAPI and the rest. This increases
>> > readability and maintenance.
> 
> ...
> 
>> >> >> +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;
>> >> > 
>> >> > -ENOENT ?
>> >> > This will allow to propagate an error code to the upper layer(s).
>> >> 
>> >> I suppose. But I think
>> >> 
>> >> alarm = iio_hwmon_lookup_alarm(...);
>> >> if (alarm < 0)
>> >> 	return -ENOENT;
>> >> 
>> >> is clearer than
>> > 
>> > I disagree. This makes it worth as it shadows other possible code(s), if any,
>> > and makes harder to follow as reader has to check the callee implementation.
>> > 
>> > The shadow error codes need a justification.
>> 
>> OK, I will return a bool next time to avoid any misconceptions that the return
>> code means anything other than "found" or "not found"
> 
> This makes sense. And IIRC it's even documented.
> 
>> >> alarm = iio_hwmon_lookup_alarm(...);
>> >> if (alarm < 0)
>> >> 	return alarm;
>> >> 
>> >> because you don't have to read the definition of iio_hwmon_lookup_alarm
>> >> to determine what the return value is.
>> > 
>> > Exactly my point!
>> 
>> your point is that you want readers to have to read the definition of
>> iio_hwmon_lookup_alarm in order to determine that ENOENT is a possible
>> error from add_alarm_attr? I don't follow.
> 
> No, my point is that readers should not care about error code. If it's
> propagated to the upper layer, the upper layer will decide on how to proceed.
> And -ENOENT is de facto standard for "entity not found".
> 
>> >> >> +}
> 
> ...
> 
>> >> >> +err_alarms:
>> >> >> +	kfree(listener->alarms);
>> >> >> +	kfree(listener->ids);
>> >> >> +err_listener:
>> >> >> +	kfree(listener);
>> >> >> +err_unlock:
>> >> >> +	mutex_unlock(&iio_hwmon_listener_lock);
>> >> >> +	return ERR_PTR(err);
>> >> > 
>> >> > What about using __free()?
>> >> 
>> >> That works for listener, but not for alarms or ids.
>> > 
>> > Why not?
> 
> No answer? Have you checked how cleanup.h suggests to avoid cleaning the memory
> when it's supposed to be used later on?

Sorry, I missed this the first time. Anyway the reason it doesn't work
for alarms/ids is that those are members of listener and not separate
variables. And I think it's more concise this way. Compare the existing

	listener->alarms = bitmap_zalloc(listener->num_alarms, GFP_KERNEL);
	if (!listener->alarms)
		goto err_listener;

	/* ... */
	return listener;

err_alarms:
	kfree(listener->alarms);
	/* ... */

vs

	unsigned long __free alarms = NULL;

	alarms = bitmap_zalloc(listener->num_alarms, GFP_KERNEL);
	if (!alarms)
		return -ENOMEM;
	listener->alarms = alarms;
	
	/* ... */
	no_free_ptr(alarms);
	return listener;

I don't really think there's an advantage.

>> >> >> +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;
>> >> > 
>> >> > Can the refcount_t be used with the respective APIs? Or even kref?
>> >> 
>> >> Why? We do all the manipulation under a mutex, so there is no point in
>> >> atomic access. Instead of the games refcnt_t has to play to try and
>> >> prevent overflow we can just check for it directly.
>> > 
>> > refcount_t provides a facility of overflow/underflow.
>> 
>> refcount_t can't prevent underflow because it's atomic. All it can do is
>> warn after the fact. And of course overflow is handled properly here.
>> But it can't occur in practice unless you specifically load multiple
>> devicetrees at runtime. So we don't need it anyway.
> 
> It will warn the user in such cases. Your code won't do it, even if it's not a
> big deal or never happens situation, it's still better to use in-kernel
> standard ways of handling these things.

It's not the standard for refcounts protected by a mutex. There are
literally hundreds of existing examples of this.

--Sean