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

[Jonathan Cameron <jic23@kernel.org>]

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