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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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