Add an API to notify consumers about events. Events still need to be
enabled using the iio_read_event/iio_write_event functions. Of course,
userspace can also manipulate the enabled events. I don't think this is
too much of an issue, since userspace can also manipulate the event
thresholds. But enabling events may cause existing programs to be
surprised when they get something unexpected. Maybe we should set the
interface as busy when there are any in-kernel listeners?
Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---
drivers/iio/industrialio-event.c | 34 +++++++++++++++++++++++++++-----
include/linux/iio/consumer.h | 30 ++++++++++++++++++++++++++++
2 files changed, 59 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
index 06295cfc2da8..b9e3ff1cd5c9 100644
--- a/drivers/iio/industrialio-event.c
+++ b/drivers/iio/industrialio-event.c
@@ -12,11 +12,13 @@
#include <linux/kernel.h>
#include <linux/kfifo.h>
#include <linux/module.h>
+#include <linux/notifier.h>
#include <linux/poll.h>
#include <linux/sched.h>
#include <linux/slab.h>
#include <linux/uaccess.h>
#include <linux/wait.h>
+#include <linux/iio/consumer.h>
#include <linux/iio/iio.h>
#include <linux/iio/iio-opaque.h>
#include "iio_core.h"
@@ -26,6 +28,7 @@
/**
* struct iio_event_interface - chrdev interface for an event line
* @wait: wait queue to allow blocking reads of events
+ * @notifier: notifier head for in-kernel event listeners
* @det_events: list of detected events
* @dev_attr_list: list of event interface sysfs attribute
* @flags: file operations related flags including busy flag.
@@ -35,6 +38,7 @@
*/
struct iio_event_interface {
wait_queue_head_t wait;
+ struct atomic_notifier_head notifier;
DECLARE_KFIFO(det_events, struct iio_event_data, 16);
struct list_head dev_attr_list;
@@ -67,18 +71,19 @@ int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64 timestamp)
{
struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
struct iio_event_interface *ev_int = iio_dev_opaque->event_interface;
- struct iio_event_data ev;
+ struct iio_event_data ev = {
+ .id = ev_code,
+ .timestamp = timestamp,
+ };
int copied;
if (!ev_int)
return 0;
+ atomic_notifier_call_chain(&ev_int->notifier, IIO_NOTIFY_EVENT, &ev);
+
/* Does anyone care? */
if (iio_event_enabled(ev_int)) {
-
- ev.id = ev_code;
- ev.timestamp = timestamp;
-
copied = kfifo_put(&ev_int->det_events, ev);
if (copied != 0)
wake_up_poll(&ev_int->wait, EPOLLIN);
@@ -223,6 +228,25 @@ static int iio_event_getfd(struct iio_dev *indio_dev)
return fd;
}
+int iio_event_register(struct iio_dev *indio_dev, struct notifier_block *block)
+{
+ struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+ struct iio_event_interface *ev_int = iio_dev_opaque->event_interface;
+
+ return atomic_notifier_chain_register(&ev_int->notifier, block);
+}
+EXPORT_SYMBOL_GPL(iio_event_register);
+
+void iio_event_unregister(struct iio_dev *indio_dev,
+ struct notifier_block *block)
+{
+ struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+ struct iio_event_interface *ev_int = iio_dev_opaque->event_interface;
+
+ WARN_ON(atomic_notifier_chain_unregister(&ev_int->notifier, block));
+}
+EXPORT_SYMBOL_GPL(iio_event_unregister);
+
static const char * const iio_ev_type_text[] = {
[IIO_EV_TYPE_THRESH] = "thresh",
[IIO_EV_TYPE_MAG] = "mag",
diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
index 16e7682474f3..9918e3f7af3d 100644
--- a/include/linux/iio/consumer.h
+++ b/include/linux/iio/consumer.h
@@ -507,4 +507,34 @@ int iio_write_event_processed_scale(struct iio_channel *chan,
enum iio_event_info info, int processed,
unsigned int scale);
+struct notifier_block;
+enum iio_notifier_val {
+ /** IIO_NOTIFY_EVENT: v is a pointer to &struct iio_event_data */
+ IIO_NOTIFY_EVENT,
+};
+
+/**
+ * iio_event_register() - Register a notifier for events
+ * @indio_dev: Device to be notified of events on
+ * @block: Notifier block to register
+ *
+ * Register a notifier for events on @indio_dev. @v will be a member of &enum
+ * iio_notifier_val. Notifiers will be called in atomic context. @indio_dev
+ * must stay valid until you call iio_event_unregister().
+ *
+ * Return: 0 on success, or -EEXIST if @block has already been registered
+ */
+int iio_event_register(struct iio_dev *indio_dev,
+ struct notifier_block *block);
+
+/**
+ * iio_event_unregister() - Remove a previously-added notifier
+ * @indio_dev: Device to be notified of events on
+ * @block: Notifier previously-registered with iio_event_register()
+ *
+ * Remove a previously-added notifier.
+ */
+void iio_event_unregister(struct iio_dev *indio_dev,
+ struct notifier_block *block);
+
#endif
--
2.35.1.1320.gc452695387.dirtyOn Mon, 14 Jul 2025 21:20:19 -0400 Sean Anderson <sean.anderson@linux.dev> wrote: > Add an API to notify consumers about events. Events still need to be > enabled using the iio_read_event/iio_write_event functions. Of course, > userspace can also manipulate the enabled events. I don't think this is > too much of an issue, since userspace can also manipulate the event > thresholds. But enabling events may cause existing programs to be > surprised when they get something unexpected. Maybe we should set the > interface as busy when there are any in-kernel listeners? I think we definitely want that to be an option. > > Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
On Mon, 2025-07-14 at 21:20 -0400, Sean Anderson wrote:
> Add an API to notify consumers about events. Events still need to be
> enabled using the iio_read_event/iio_write_event functions. Of course,
> userspace can also manipulate the enabled events. I don't think this is
> too much of an issue, since userspace can also manipulate the event
> thresholds. But enabling events may cause existing programs to be
> surprised when they get something unexpected. Maybe we should set the
> interface as busy when there are any in-kernel listeners?
>
Sensible question. I'm not that familiar with events but I suspect is not
trivial (if doable) to do a similar approach as with buffers? With buffers, an
inkernal consumer get's it's own buffer object (that goes into a list of active
buffers in the iio device) with all channels enabled and then we demux the
appropriate channels for each consumer.
Independent of the above, we can argue that having both inkernel and userspace
changing thresholds is ok (I mean, there's nothing stopping two userspace apps
doing that) but we should likely be careful with enabling/disabling. If multiple
consumers enable the same event, one of them disabling it should not disable it
for all the consumers, right?
- Nuno Sá
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
>
> drivers/iio/industrialio-event.c | 34 +++++++++++++++++++++++++++-----
> include/linux/iio/consumer.h | 30 ++++++++++++++++++++++++++++
> 2 files changed, 59 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-
> event.c
> index 06295cfc2da8..b9e3ff1cd5c9 100644
> --- a/drivers/iio/industrialio-event.c
> +++ b/drivers/iio/industrialio-event.c
> @@ -12,11 +12,13 @@
> #include <linux/kernel.h>
> #include <linux/kfifo.h>
> #include <linux/module.h>
> +#include <linux/notifier.h>
> #include <linux/poll.h>
> #include <linux/sched.h>
> #include <linux/slab.h>
> #include <linux/uaccess.h>
> #include <linux/wait.h>
> +#include <linux/iio/consumer.h>
> #include <linux/iio/iio.h>
> #include <linux/iio/iio-opaque.h>
> #include "iio_core.h"
> @@ -26,6 +28,7 @@
> /**
> * struct iio_event_interface - chrdev interface for an event line
> * @wait: wait queue to allow blocking reads of events
> + * @notifier: notifier head for in-kernel event listeners
> * @det_events: list of detected events
> * @dev_attr_list: list of event interface sysfs attribute
> * @flags: file operations related flags including busy flag.
> @@ -35,6 +38,7 @@
> */
> struct iio_event_interface {
> wait_queue_head_t wait;
> + struct atomic_notifier_head notifier;
> DECLARE_KFIFO(det_events, struct iio_event_data, 16);
>
> struct list_head dev_attr_list;
> @@ -67,18 +71,19 @@ int iio_push_event(struct iio_dev *indio_dev, u64 ev_code,
> s64 timestamp)
> {
> struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> struct iio_event_interface *ev_int = iio_dev_opaque->event_interface;
> - struct iio_event_data ev;
> + struct iio_event_data ev = {
> + .id = ev_code,
> + .timestamp = timestamp,
> + };
> int copied;
>
> if (!ev_int)
> return 0;
>
> + atomic_notifier_call_chain(&ev_int->notifier, IIO_NOTIFY_EVENT, &ev);
> +
> /* Does anyone care? */
> if (iio_event_enabled(ev_int)) {
> -
> - ev.id = ev_code;
> - ev.timestamp = timestamp;
> -
> copied = kfifo_put(&ev_int->det_events, ev);
> if (copied != 0)
> wake_up_poll(&ev_int->wait, EPOLLIN);
> @@ -223,6 +228,25 @@ static int iio_event_getfd(struct iio_dev *indio_dev)
> return fd;
> }
>
> +int iio_event_register(struct iio_dev *indio_dev, struct notifier_block
> *block)
> +{
> + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> + struct iio_event_interface *ev_int = iio_dev_opaque->event_interface;
> +
> + return atomic_notifier_chain_register(&ev_int->notifier, block);
> +}
> +EXPORT_SYMBOL_GPL(iio_event_register);
> +
> +void iio_event_unregister(struct iio_dev *indio_dev,
> + struct notifier_block *block)
> +{
> + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> + struct iio_event_interface *ev_int = iio_dev_opaque->event_interface;
> +
> + WARN_ON(atomic_notifier_chain_unregister(&ev_int->notifier, block));
> +}
> +EXPORT_SYMBOL_GPL(iio_event_unregister);
> +
> static const char * const iio_ev_type_text[] = {
> [IIO_EV_TYPE_THRESH] = "thresh",
> [IIO_EV_TYPE_MAG] = "mag",
> diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
> index 16e7682474f3..9918e3f7af3d 100644
> --- a/include/linux/iio/consumer.h
> +++ b/include/linux/iio/consumer.h
> @@ -507,4 +507,34 @@ int iio_write_event_processed_scale(struct iio_channel
> *chan,
> enum iio_event_info info, int processed,
> unsigned int scale);
>
> +struct notifier_block;
> +enum iio_notifier_val {
> + /** IIO_NOTIFY_EVENT: v is a pointer to &struct iio_event_data */
> + IIO_NOTIFY_EVENT,
> +};
> +
> +/**
> + * iio_event_register() - Register a notifier for events
> + * @indio_dev: Device to be notified of events on
> + * @block: Notifier block to register
> + *
> + * Register a notifier for events on @indio_dev. @v will be a member of &enum
> + * iio_notifier_val. Notifiers will be called in atomic context. @indio_dev
> + * must stay valid until you call iio_event_unregister().
> + *
> + * Return: 0 on success, or -EEXIST if @block has already been registered
> + */
> +int iio_event_register(struct iio_dev *indio_dev,
> + struct notifier_block *block);
> +
> +/**
> + * iio_event_unregister() - Remove a previously-added notifier
> + * @indio_dev: Device to be notified of events on
> + * @block: Notifier previously-registered with iio_event_register()
> + *
> + * Remove a previously-added notifier.
> + */
> +void iio_event_unregister(struct iio_dev *indio_dev,
> + struct notifier_block *block);
> +
> #endif
On 7/15/25 07:09, Nuno Sá wrote:
> On Mon, 2025-07-14 at 21:20 -0400, Sean Anderson wrote:
>> Add an API to notify consumers about events. Events still need to be
>> enabled using the iio_read_event/iio_write_event functions. Of course,
>> userspace can also manipulate the enabled events. I don't think this is
>> too much of an issue, since userspace can also manipulate the event
>> thresholds. But enabling events may cause existing programs to be
>> surprised when they get something unexpected. Maybe we should set the
>> interface as busy when there are any in-kernel listeners?
>>
>
> Sensible question. I'm not that familiar with events but I suspect is not
> trivial (if doable) to do a similar approach as with buffers? With buffers, an
> inkernal consumer get's it's own buffer object (that goes into a list of active
> buffers in the iio device) with all channels enabled and then we demux the
> appropriate channels for each consumer.
For in-kernel consumers I think it's reasonable to expect them to handle
events they didn't explicitly enable. I'm not sure about userspace
consumers.
> Independent of the above, we can argue that having both inkernel and userspace
> changing thresholds is ok (I mean, there's nothing stopping two userspace apps
> doing that) but we should likely be careful with enabling/disabling. If multiple
> consumers enable the same event, one of them disabling it should not disable it
> for all the consumers, right?
Right now the HWMON consumer never permanently disable events to avoid this
issue. It does toggle the enable to determine if an alarm should stay
enabled:
________
condition __/ \________
_____ ____ ___
enable \__/ \__/
event | |
__ ____
alarm __/ \__/ \_____
read 1 1 0
I suppose this could also be done by comparing the raw threshold to the
channel.
--Sean
>
>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> ---
>>
>> drivers/iio/industrialio-event.c | 34 +++++++++++++++++++++++++++-----
>> include/linux/iio/consumer.h | 30 ++++++++++++++++++++++++++++
>> 2 files changed, 59 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-
>> event.c
>> index 06295cfc2da8..b9e3ff1cd5c9 100644
>> --- a/drivers/iio/industrialio-event.c
>> +++ b/drivers/iio/industrialio-event.c
>> @@ -12,11 +12,13 @@
>> #include <linux/kernel.h>
>> #include <linux/kfifo.h>
>> #include <linux/module.h>
>> +#include <linux/notifier.h>
>> #include <linux/poll.h>
>> #include <linux/sched.h>
>> #include <linux/slab.h>
>> #include <linux/uaccess.h>
>> #include <linux/wait.h>
>> +#include <linux/iio/consumer.h>
>> #include <linux/iio/iio.h>
>> #include <linux/iio/iio-opaque.h>
>> #include "iio_core.h"
>> @@ -26,6 +28,7 @@
>> /**
>> * struct iio_event_interface - chrdev interface for an event line
>> * @wait: wait queue to allow blocking reads of events
>> + * @notifier: notifier head for in-kernel event listeners
>> * @det_events: list of detected events
>> * @dev_attr_list: list of event interface sysfs attribute
>> * @flags: file operations related flags including busy flag.
>> @@ -35,6 +38,7 @@
>> */
>> struct iio_event_interface {
>> wait_queue_head_t wait;
>> + struct atomic_notifier_head notifier;
>> DECLARE_KFIFO(det_events, struct iio_event_data, 16);
>>
>> struct list_head dev_attr_list;
>> @@ -67,18 +71,19 @@ int iio_push_event(struct iio_dev *indio_dev, u64 ev_code,
>> s64 timestamp)
>> {
>> struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
>> struct iio_event_interface *ev_int = iio_dev_opaque->event_interface;
>> - struct iio_event_data ev;
>> + struct iio_event_data ev = {
>> + .id = ev_code,
>> + .timestamp = timestamp,
>> + };
>> int copied;
>>
>> if (!ev_int)
>> return 0;
>>
>> + atomic_notifier_call_chain(&ev_int->notifier, IIO_NOTIFY_EVENT, &ev);
>> +
>> /* Does anyone care? */
>> if (iio_event_enabled(ev_int)) {
>> -
>> - ev.id = ev_code;
>> - ev.timestamp = timestamp;
>> -
>> copied = kfifo_put(&ev_int->det_events, ev);
>> if (copied != 0)
>> wake_up_poll(&ev_int->wait, EPOLLIN);
>> @@ -223,6 +228,25 @@ static int iio_event_getfd(struct iio_dev *indio_dev)
>> return fd;
>> }
>>
>> +int iio_event_register(struct iio_dev *indio_dev, struct notifier_block
>> *block)
>> +{
>> + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
>> + struct iio_event_interface *ev_int = iio_dev_opaque->event_interface;
>> +
>> + return atomic_notifier_chain_register(&ev_int->notifier, block);
>> +}
>> +EXPORT_SYMBOL_GPL(iio_event_register);
>> +
>> +void iio_event_unregister(struct iio_dev *indio_dev,
>> + struct notifier_block *block)
>> +{
>> + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
>> + struct iio_event_interface *ev_int = iio_dev_opaque->event_interface;
>> +
>> + WARN_ON(atomic_notifier_chain_unregister(&ev_int->notifier, block));
>> +}
>> +EXPORT_SYMBOL_GPL(iio_event_unregister);
>> +
>> static const char * const iio_ev_type_text[] = {
>> [IIO_EV_TYPE_THRESH] = "thresh",
>> [IIO_EV_TYPE_MAG] = "mag",
>> diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
>> index 16e7682474f3..9918e3f7af3d 100644
>> --- a/include/linux/iio/consumer.h
>> +++ b/include/linux/iio/consumer.h
>> @@ -507,4 +507,34 @@ int iio_write_event_processed_scale(struct iio_channel
>> *chan,
>> enum iio_event_info info, int processed,
>> unsigned int scale);
>>
>> +struct notifier_block;
>> +enum iio_notifier_val {
>> + /** IIO_NOTIFY_EVENT: v is a pointer to &struct iio_event_data */
>> + IIO_NOTIFY_EVENT,
>> +};
>> +
>> +/**
>> + * iio_event_register() - Register a notifier for events
>> + * @indio_dev: Device to be notified of events on
>> + * @block: Notifier block to register
>> + *
>> + * Register a notifier for events on @indio_dev. @v will be a member of &enum
>> + * iio_notifier_val. Notifiers will be called in atomic context. @indio_dev
>> + * must stay valid until you call iio_event_unregister().
>> + *
>> + * Return: 0 on success, or -EEXIST if @block has already been registered
>> + */
>> +int iio_event_register(struct iio_dev *indio_dev,
>> + struct notifier_block *block);
>> +
>> +/**
>> + * iio_event_unregister() - Remove a previously-added notifier
>> + * @indio_dev: Device to be notified of events on
>> + * @block: Notifier previously-registered with iio_event_register()
>> + *
>> + * Remove a previously-added notifier.
>> + */
>> +void iio_event_unregister(struct iio_dev *indio_dev,
>> + struct notifier_block *block);
>> +
>> #endif
On Tue, 15 Jul 2025 12:52:19 -0400 Sean Anderson <sean.anderson@linux.dev> wrote: > On 7/15/25 07:09, Nuno Sá wrote: > > On Mon, 2025-07-14 at 21:20 -0400, Sean Anderson wrote: > >> Add an API to notify consumers about events. Events still need to be > >> enabled using the iio_read_event/iio_write_event functions. Of course, > >> userspace can also manipulate the enabled events. I don't think this is > >> too much of an issue, since userspace can also manipulate the event > >> thresholds. But enabling events may cause existing programs to be > >> surprised when they get something unexpected. Maybe we should set the > >> interface as busy when there are any in-kernel listeners? > >> > > > > Sensible question. I'm not that familiar with events but I suspect is not > > trivial (if doable) to do a similar approach as with buffers? With buffers, an > > inkernal consumer get's it's own buffer object (that goes into a list of active > > buffers in the iio device) with all channels enabled and then we demux the > > appropriate channels for each consumer. > > For in-kernel consumers I think it's reasonable to expect them to handle > events they didn't explicitly enable. I'm not sure about userspace > consumers. This already happens because we don't have a demux equivalent (what we do for buffered data flow) so if a device only has a single enable bit that covers multiple events (annoyingly common for accelerometers for example) then userspace will get events it didn't ask for. We 'could' fix that, but it's never really been worth the effort. Events tend to be low data rate so an occasionally extra is rather different to having to have much larger data buffers to handle a range of channels you never asked for. Lets be careful to document this behaviour as 'may enable extra events' as then if we decide later to do demux type stuff we won't be breaking ABI. No one will mind getting fewer spurious events due to a core improvement. > > > Independent of the above, we can argue that having both inkernel and userspace > > changing thresholds is ok (I mean, there's nothing stopping two userspace apps > > doing that) but we should likely be careful with enabling/disabling. If multiple > > consumers enable the same event, one of them disabling it should not disable it > > for all the consumers, right? > > Right now the HWMON consumer never permanently disable events to avoid this > issue. It does toggle the enable to determine if an alarm should stay > enabled: > ________ > condition __/ \________ > _____ ____ ___ > enable \__/ \__/ > > event | | > __ ____ > alarm __/ \__/ \_____ > > read 1 1 0 > > I suppose this could also be done by comparing the raw threshold to the > channel. I wonder if we should add the option to do a 'get_exclusive' or similar to block the IIO user interfaces if something critical is using the device. If we were for instance to use this to block the IOCTL to get the events fd then any built in driver etc will almost certainly load before anyone can call the ioctl so it will fairly cleanly block things. Jonathan
On 7/27/25 12:21, Jonathan Cameron wrote: > On Tue, 15 Jul 2025 12:52:19 -0400 > Sean Anderson <sean.anderson@linux.dev> wrote: > >> On 7/15/25 07:09, Nuno Sá wrote: >> > On Mon, 2025-07-14 at 21:20 -0400, Sean Anderson wrote: >> >> Add an API to notify consumers about events. Events still need to be >> >> enabled using the iio_read_event/iio_write_event functions. Of course, >> >> userspace can also manipulate the enabled events. I don't think this is >> >> too much of an issue, since userspace can also manipulate the event >> >> thresholds. But enabling events may cause existing programs to be >> >> surprised when they get something unexpected. Maybe we should set the >> >> interface as busy when there are any in-kernel listeners? >> >> >> > >> > Sensible question. I'm not that familiar with events but I suspect is not >> > trivial (if doable) to do a similar approach as with buffers? With buffers, an >> > inkernal consumer get's it's own buffer object (that goes into a list of active >> > buffers in the iio device) with all channels enabled and then we demux the >> > appropriate channels for each consumer. >> >> For in-kernel consumers I think it's reasonable to expect them to handle >> events they didn't explicitly enable. I'm not sure about userspace >> consumers. > > This already happens because we don't have a demux equivalent (what we do > for buffered data flow) so if a device only has a single enable bit that covers > multiple events (annoyingly common for accelerometers for example) then > userspace will get events it didn't ask for. We 'could' fix that, > but it's never really been worth the effort. > > Events tend to be low data rate so an occasionally extra is rather different > to having to have much larger data buffers to handle a range of channels you > never asked for. > > Lets be careful to document this behaviour as 'may enable extra events' > as then if we decide later to do demux type stuff we won't be breaking ABI. > No one will mind getting fewer spurious events due to a core improvement. Where would this get documented? >> >> > Independent of the above, we can argue that having both inkernel and userspace >> > changing thresholds is ok (I mean, there's nothing stopping two userspace apps >> > doing that) but we should likely be careful with enabling/disabling. If multiple >> > consumers enable the same event, one of them disabling it should not disable it >> > for all the consumers, right? >> >> Right now the HWMON consumer never permanently disable events to avoid this >> issue. It does toggle the enable to determine if an alarm should stay >> enabled: >> ________ >> condition __/ \________ >> _____ ____ ___ >> enable \__/ \__/ >> >> event | | >> __ ____ >> alarm __/ \__/ \_____ >> >> read 1 1 0 >> >> I suppose this could also be done by comparing the raw threshold to the >> channel. > > I wonder if we should add the option to do a 'get_exclusive' or similar > to block the IIO user interfaces if something critical is using the device. > > If we were for instance to use this to block the IOCTL to get the events > fd then any built in driver etc will almost certainly load before anyone > can call the ioctl so it will fairly cleanly block things. This is how it currently works for userspace. Only one process can create the event fd, and everyone else gets -EBUSY. Of course, it would be pretty surprising to have an IIO device where some channels were used by userspace and others were used by hwmon and then have your daemon stop working after you update your kernel because now the hwmon driver takes exclusive event access. I originally had kernel users read from the kfifo just like userspace, but I was concerned about the above scenario. --Sean
On Mon, 28 Jul 2025 18:44:30 -0400 Sean Anderson <sean.anderson@linux.dev> wrote: > On 7/27/25 12:21, Jonathan Cameron wrote: > > On Tue, 15 Jul 2025 12:52:19 -0400 > > Sean Anderson <sean.anderson@linux.dev> wrote: > > > >> On 7/15/25 07:09, Nuno Sá wrote: > >> > On Mon, 2025-07-14 at 21:20 -0400, Sean Anderson wrote: > >> >> Add an API to notify consumers about events. Events still need to be > >> >> enabled using the iio_read_event/iio_write_event functions. Of course, > >> >> userspace can also manipulate the enabled events. I don't think this is > >> >> too much of an issue, since userspace can also manipulate the event > >> >> thresholds. But enabling events may cause existing programs to be > >> >> surprised when they get something unexpected. Maybe we should set the > >> >> interface as busy when there are any in-kernel listeners? > >> >> > >> > > >> > Sensible question. I'm not that familiar with events but I suspect is not > >> > trivial (if doable) to do a similar approach as with buffers? With buffers, an > >> > inkernal consumer get's it's own buffer object (that goes into a list of active > >> > buffers in the iio device) with all channels enabled and then we demux the > >> > appropriate channels for each consumer. > >> > >> For in-kernel consumers I think it's reasonable to expect them to handle > >> events they didn't explicitly enable. I'm not sure about userspace > >> consumers. > > > > This already happens because we don't have a demux equivalent (what we do > > for buffered data flow) so if a device only has a single enable bit that covers > > multiple events (annoyingly common for accelerometers for example) then > > userspace will get events it didn't ask for. We 'could' fix that, > > but it's never really been worth the effort. > > > > Events tend to be low data rate so an occasionally extra is rather different > > to having to have much larger data buffers to handle a range of channels you > > never asked for. > > > > Lets be careful to document this behaviour as 'may enable extra events' > > as then if we decide later to do demux type stuff we won't be breaking ABI. > > No one will mind getting fewer spurious events due to a core improvement. > > Where would this get documented? Starting point will be in the docs for the ABI that asks for any events at all. Also useful to add some thing to Documentation/IIO though there are lots of other things those docs don't yet cover :( > > >> > >> > Independent of the above, we can argue that having both inkernel and userspace > >> > changing thresholds is ok (I mean, there's nothing stopping two userspace apps > >> > doing that) but we should likely be careful with enabling/disabling. If multiple > >> > consumers enable the same event, one of them disabling it should not disable it > >> > for all the consumers, right? > >> > >> Right now the HWMON consumer never permanently disable events to avoid this > >> issue. It does toggle the enable to determine if an alarm should stay > >> enabled: > >> ________ > >> condition __/ \________ > >> _____ ____ ___ > >> enable \__/ \__/ > >> > >> event | | > >> __ ____ > >> alarm __/ \__/ \_____ > >> > >> read 1 1 0 > >> > >> I suppose this could also be done by comparing the raw threshold to the > >> channel. > > > > I wonder if we should add the option to do a 'get_exclusive' or similar > > to block the IIO user interfaces if something critical is using the device. > > > > If we were for instance to use this to block the IOCTL to get the events > > fd then any built in driver etc will almost certainly load before anyone > > can call the ioctl so it will fairly cleanly block things. > > This is how it currently works for userspace. Only one process can create > the event fd, and everyone else gets -EBUSY. > > Of course, it would be pretty surprising to have an IIO device where > some channels were used by userspace and others were used by hwmon and > then have your daemon stop working after you update your kernel because > now the hwmon driver takes exclusive event access. True. I wonder how many boards we don't know about are using the iio-hwmon bridge. We can check the ones in kernel for whether they grab all the channels (which would rule this out). Another things we could do is have an opt in from the IIO driver. That way only 'new' drivers would have this behaviour. Not nice though. > > I originally had kernel users read from the kfifo just like userspace, > but I was concerned about the above scenario. > yeah, always a problem to retrofit policy. > --Sean >
On 7/29/25 14:33, Jonathan Cameron wrote:
> On Mon, 28 Jul 2025 18:44:30 -0400
> Sean Anderson <sean.anderson@linux.dev> wrote:
>
>> On 7/27/25 12:21, Jonathan Cameron wrote:
>> > On Tue, 15 Jul 2025 12:52:19 -0400
>> > Sean Anderson <sean.anderson@linux.dev> wrote:
>> >
>> >> On 7/15/25 07:09, Nuno Sá wrote:
>> >> > On Mon, 2025-07-14 at 21:20 -0400, Sean Anderson wrote:
>> >> >> Add an API to notify consumers about events. Events still need to be
>> >> >> enabled using the iio_read_event/iio_write_event functions. Of course,
>> >> >> userspace can also manipulate the enabled events. I don't think this is
>> >> >> too much of an issue, since userspace can also manipulate the event
>> >> >> thresholds. But enabling events may cause existing programs to be
>> >> >> surprised when they get something unexpected. Maybe we should set the
>> >> >> interface as busy when there are any in-kernel listeners?
>> >> >>
>> >> >
>> >> > Sensible question. I'm not that familiar with events but I suspect is not
>> >> > trivial (if doable) to do a similar approach as with buffers? With buffers, an
>> >> > inkernal consumer get's it's own buffer object (that goes into a list of active
>> >> > buffers in the iio device) with all channels enabled and then we demux the
>> >> > appropriate channels for each consumer.
>> >>
>> >> For in-kernel consumers I think it's reasonable to expect them to handle
>> >> events they didn't explicitly enable. I'm not sure about userspace
>> >> consumers.
>> >
>> > This already happens because we don't have a demux equivalent (what we do
>> > for buffered data flow) so if a device only has a single enable bit that covers
>> > multiple events (annoyingly common for accelerometers for example) then
>> > userspace will get events it didn't ask for. We 'could' fix that,
>> > but it's never really been worth the effort.
>> >
>> > Events tend to be low data rate so an occasionally extra is rather different
>> > to having to have much larger data buffers to handle a range of channels you
>> > never asked for.
>> >
>> > Lets be careful to document this behaviour as 'may enable extra events'
>> > as then if we decide later to do demux type stuff we won't be breaking ABI.
>> > No one will mind getting fewer spurious events due to a core improvement.
>>
>> Where would this get documented?
>
> Starting point will be in the docs for the ABI that asks for any events at all.
>
> Also useful to add some thing to Documentation/IIO though there are lots of
> other things those docs don't yet cover :(
Notably the whole events API :l
>>
>> >>
>> >> > Independent of the above, we can argue that having both inkernel and userspace
>> >> > changing thresholds is ok (I mean, there's nothing stopping two userspace apps
>> >> > doing that) but we should likely be careful with enabling/disabling. If multiple
>> >> > consumers enable the same event, one of them disabling it should not disable it
>> >> > for all the consumers, right?
>> >>
>> >> Right now the HWMON consumer never permanently disable events to avoid this
>> >> issue. It does toggle the enable to determine if an alarm should stay
>> >> enabled:
>> >> ________
>> >> condition __/ \________
>> >> _____ ____ ___
>> >> enable \__/ \__/
>> >>
>> >> event | |
>> >> __ ____
>> >> alarm __/ \__/ \_____
>> >>
>> >> read 1 1 0
>> >>
>> >> I suppose this could also be done by comparing the raw threshold to the
>> >> channel.
>> >
>> > I wonder if we should add the option to do a 'get_exclusive' or similar
>> > to block the IIO user interfaces if something critical is using the device.
>> >
>> > If we were for instance to use this to block the IOCTL to get the events
>> > fd then any built in driver etc will almost certainly load before anyone
>> > can call the ioctl so it will fairly cleanly block things.
>>
>> This is how it currently works for userspace. Only one process can create
>> the event fd, and everyone else gets -EBUSY.
>>
>> Of course, it would be pretty surprising to have an IIO device where
>> some channels were used by userspace and others were used by hwmon and
>> then have your daemon stop working after you update your kernel because
>> now the hwmon driver takes exclusive event access.
>
> True. I wonder how many boards we don't know about are using the iio-hwmon
> bridge. We can check the ones in kernel for whether they grab all the
> channels (which would rule this out).
>
> Another things we could do is have an opt in from the IIO driver.
> That way only 'new' drivers would have this behaviour. Not nice though.
I would really like for this to "just work" if at all possible, so an
opt-out would be preferable. Maybe a hwmon module parameter.
But I think we can do better:
- Both kernel/userspace can/should handle unexpected events
- This includes extra (synthetic) events.
- Both kernel/userspace mostly just want to enable events
- Disabling events is not as important because of the previous bullet.
- But losing events is probably bad so we want to ensure we trigger
events at the same places they would have been triggered before.
So maybe we have an implementation where
- Enabling an event disables the backing event before re-enabling it if
there are any existing users
- Disabling an event only disables the backing event if all users are
gone
It could look something like
iio_sysfs_event_set(event, val):
if val:
if !event.user_enable
disable(event)
enable(event)
else if !event.kernel_enables
disable(event)
event.user_enable = val
iio_inkern_event_set(event, val):
if val:
if event.kernel_enables++ || event.user_enable
disable(event)
enable(event)
else if !--event.kernel_enables && !event.user_enable:
disable(event)
--Sean
>>
>> I originally had kernel users read from the kfifo just like userspace,
>> but I was concerned about the above scenario.
>>
>
> yeah, always a problem to retrofit policy.
>
>> --Sean
>>
>
On Tue, 29 Jul 2025 16:09:20 -0400 Sean Anderson <sean.anderson@linux.dev> wrote: > On 7/29/25 14:33, Jonathan Cameron wrote: > > On Mon, 28 Jul 2025 18:44:30 -0400 > > Sean Anderson <sean.anderson@linux.dev> wrote: > > > >> On 7/27/25 12:21, Jonathan Cameron wrote: > >> > On Tue, 15 Jul 2025 12:52:19 -0400 > >> > Sean Anderson <sean.anderson@linux.dev> wrote: > >> > > >> >> On 7/15/25 07:09, Nuno Sá wrote: > >> >> > On Mon, 2025-07-14 at 21:20 -0400, Sean Anderson wrote: > >> >> >> Add an API to notify consumers about events. Events still need to be > >> >> >> enabled using the iio_read_event/iio_write_event functions. Of course, > >> >> >> userspace can also manipulate the enabled events. I don't think this is > >> >> >> too much of an issue, since userspace can also manipulate the event > >> >> >> thresholds. But enabling events may cause existing programs to be > >> >> >> surprised when they get something unexpected. Maybe we should set the > >> >> >> interface as busy when there are any in-kernel listeners? > >> >> >> > >> >> > > >> >> > Sensible question. I'm not that familiar with events but I suspect is not > >> >> > trivial (if doable) to do a similar approach as with buffers? With buffers, an > >> >> > inkernal consumer get's it's own buffer object (that goes into a list of active > >> >> > buffers in the iio device) with all channels enabled and then we demux the > >> >> > appropriate channels for each consumer. > >> >> > >> >> For in-kernel consumers I think it's reasonable to expect them to handle > >> >> events they didn't explicitly enable. I'm not sure about userspace > >> >> consumers. > >> > > >> > This already happens because we don't have a demux equivalent (what we do > >> > for buffered data flow) so if a device only has a single enable bit that covers > >> > multiple events (annoyingly common for accelerometers for example) then > >> > userspace will get events it didn't ask for. We 'could' fix that, > >> > but it's never really been worth the effort. > >> > > >> > Events tend to be low data rate so an occasionally extra is rather different > >> > to having to have much larger data buffers to handle a range of channels you > >> > never asked for. > >> > > >> > Lets be careful to document this behaviour as 'may enable extra events' > >> > as then if we decide later to do demux type stuff we won't be breaking ABI. > >> > No one will mind getting fewer spurious events due to a core improvement. > >> > >> Where would this get documented? > > > > Starting point will be in the docs for the ABI that asks for any events at all. > > > > Also useful to add some thing to Documentation/IIO though there are lots of > > other things those docs don't yet cover :( > > Notably the whole events API :l > > >> > >> >> > >> >> > Independent of the above, we can argue that having both inkernel and userspace > >> >> > changing thresholds is ok (I mean, there's nothing stopping two userspace apps > >> >> > doing that) but we should likely be careful with enabling/disabling. If multiple > >> >> > consumers enable the same event, one of them disabling it should not disable it > >> >> > for all the consumers, right? > >> >> > >> >> Right now the HWMON consumer never permanently disable events to avoid this > >> >> issue. It does toggle the enable to determine if an alarm should stay > >> >> enabled: > >> >> ________ > >> >> condition __/ \________ > >> >> _____ ____ ___ > >> >> enable \__/ \__/ > >> >> > >> >> event | | > >> >> __ ____ > >> >> alarm __/ \__/ \_____ > >> >> > >> >> read 1 1 0 > >> >> > >> >> I suppose this could also be done by comparing the raw threshold to the > >> >> channel. > >> > > >> > I wonder if we should add the option to do a 'get_exclusive' or similar > >> > to block the IIO user interfaces if something critical is using the device. > >> > > >> > If we were for instance to use this to block the IOCTL to get the events > >> > fd then any built in driver etc will almost certainly load before anyone > >> > can call the ioctl so it will fairly cleanly block things. > >> > >> This is how it currently works for userspace. Only one process can create > >> the event fd, and everyone else gets -EBUSY. > >> > >> Of course, it would be pretty surprising to have an IIO device where > >> some channels were used by userspace and others were used by hwmon and > >> then have your daemon stop working after you update your kernel because > >> now the hwmon driver takes exclusive event access. > > > > True. I wonder how many boards we don't know about are using the iio-hwmon > > bridge. We can check the ones in kernel for whether they grab all the > > channels (which would rule this out). > > > > Another things we could do is have an opt in from the IIO driver. > > That way only 'new' drivers would have this behaviour. Not nice though. > > I would really like for this to "just work" if at all possible, so an > opt-out would be preferable. Maybe a hwmon module parameter. > > But I think we can do better: > > - Both kernel/userspace can/should handle unexpected events > - This includes extra (synthetic) events. > - Both kernel/userspace mostly just want to enable events > - Disabling events is not as important because of the previous bullet. > - But losing events is probably bad so we want to ensure we trigger > events at the same places they would have been triggered before. > > So maybe we have an implementation where > > - Enabling an event disables the backing event before re-enabling it if > there are any existing users > - Disabling an event only disables the backing event if all users are > gone > > It could look something like > > iio_sysfs_event_set(event, val): > if val: > if !event.user_enable > disable(event) > enable(event) > else if !event.kernel_enables > disable(event) > event.user_enable = val > > iio_inkern_event_set(event, val): > if val: > if event.kernel_enables++ || event.user_enable > disable(event) > enable(event) > else if !--event.kernel_enables && !event.user_enable: > disable(event) Something like that should work. We'll need to be careful to gate any push towards userspace on it waiting for something. Given we only send them when IIO_BUSY_BIT_POS is set on the event interface (which happens on requesting the fd) I think we may be fine already. Jonathan > > --Sean > > >> > >> I originally had kernel users read from the kfifo just like userspace, > >> but I was concerned about the above scenario. > >> > > > > yeah, always a problem to retrofit policy. > > > >> --Sean > >> > > > >
On Mon, Jul 14, 2025 at 09:20:19PM -0400, Sean Anderson wrote:
> Add an API to notify consumers about events. Events still need to be
> enabled using the iio_read_event/iio_write_event functions. Of course,
> userspace can also manipulate the enabled events. I don't think this is
> too much of an issue, since userspace can also manipulate the event
> thresholds. But enabling events may cause existing programs to be
> surprised when they get something unexpected. Maybe we should set the
> interface as busy when there are any in-kernel listeners?
...
> #include <linux/wait.h>
While at it...
+ blank line here...
> +#include <linux/iio/consumer.h>
> #include <linux/iio/iio.h>
> #include <linux/iio/iio-opaque.h>
...and here?
> #include "iio_core.h"
...
> + struct iio_event_data ev = {
> + .id = ev_code,
> + .timestamp = timestamp,
> + };
...
> /* Does anyone care? */
> if (iio_event_enabled(ev_int)) {
> -
> - ev.id = ev_code;
> - ev.timestamp = timestamp;
> -
> copied = kfifo_put(&ev_int->det_events, ev);
> if (copied != 0)
> wake_up_poll(&ev_int->wait, EPOLLIN);
Looks like this refactoring can be done before main change.
...
> + WARN_ON(atomic_notifier_chain_unregister(&ev_int->notifier, block));
Is bug.h already included?
--
With Best Regards,
Andy Shevchenko
On 7/15/25 04:20, Andy Shevchenko wrote:
> On Mon, Jul 14, 2025 at 09:20:19PM -0400, Sean Anderson wrote:
>> Add an API to notify consumers about events. Events still need to be
>> enabled using the iio_read_event/iio_write_event functions. Of course,
>> userspace can also manipulate the enabled events. I don't think this is
>> too much of an issue, since userspace can also manipulate the event
>> thresholds. But enabling events may cause existing programs to be
>> surprised when they get something unexpected. Maybe we should set the
>> interface as busy when there are any in-kernel listeners?
>
> ...
>
>> #include <linux/wait.h>
>
> While at it...
>
> + blank line here...
>
>> +#include <linux/iio/consumer.h>
>> #include <linux/iio/iio.h>
>> #include <linux/iio/iio-opaque.h>
>
> ...and here?
>
>> #include "iio_core.h"
>
> ...
>
>> + struct iio_event_data ev = {
>> + .id = ev_code,
>> + .timestamp = timestamp,
>> + };
>
> ...
>
>> /* Does anyone care? */
>> if (iio_event_enabled(ev_int)) {
>> -
>> - ev.id = ev_code;
>> - ev.timestamp = timestamp;
>> -
>> copied = kfifo_put(&ev_int->det_events, ev);
>> if (copied != 0)
>> wake_up_poll(&ev_int->wait, EPOLLIN);
>
> Looks like this refactoring can be done before main change.
I think it is clearer to keep this in the same patch as the
functionality that uses it.
> ...
>
>> + WARN_ON(atomic_notifier_chain_unregister(&ev_int->notifier, block));
>
> Is bug.h already included?
I assume so. No build errors.
--Sean
On Tue, Jul 15, 2025 at 11:47:07AM -0400, Sean Anderson wrote: > On 7/15/25 04:20, Andy Shevchenko wrote: > > On Mon, Jul 14, 2025 at 09:20:19PM -0400, Sean Anderson wrote: ... > >> + WARN_ON(atomic_notifier_chain_unregister(&ev_int->notifier, block)); > > > > Is bug.h already included? > > I assume so. No build errors. Explicitly? Otherwise it's against IWYU principle. -- With Best Regards, Andy Shevchenko
© 2016 - 2025 Red Hat, Inc.