Message-ID: <3b35b3a7-3f1a-4401-9b60-ba4afda5636e@linux.dev>
Date: Tue, 15 Jul 2025 12:20:24 -0400
From: Sean Anderson <sean.anderson@...ux.dev>
To: Andy Shevchenko <andriy.shevchenko@...el.com>
Cc: Jonathan Cameron <jic23@...nel.org>, 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 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.
> 

Powered by blists - more mailing lists