Message-ID: <52340e28-0ad4-4e67-adee-3c08b1c98b10@linux.dev>
Date: Tue, 15 Jul 2025 16:08:13 -0400
From: Sean Anderson <sean.anderson@...ux.dev>
To: Guenter Roeck <linux@...ck-us.net>, Jonathan Cameron <jic23@...nel.org>,
 Jean Delvare <jdelvare@...e.com>, linux-iio@...r.kernel.org,
 linux-hwmon@...r.kernel.org
Cc: 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 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

Powered by blists - more mailing lists