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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250727175054.54fe5629@jic23-huawei>
Date: Sun, 27 Jul 2025 17:50:54 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Sean Anderson <sean.anderson@...ux.dev>
Cc: Jean Delvare <jdelvare@...e.com>, Guenter Roeck <linux@...ck-us.net>,
 linux-iio@...r.kernel.org, linux-hwmon@...r.kernel.org, Andy Shevchenko
 <andy@...nel.org>, Nuno Sá <nuno.sa@...log.com>,
 linux-kernel@...r.kernel.org, David Lechner <dlechner@...libre.com>
Subject: Re: [PATCH 7/7] hwmon: iio: Add alarm support

On Mon, 14 Jul 2025 21:20:23 -0400
Sean Anderson <sean.anderson@...ux.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@...ux.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);
> +}
>