Re: [PATCH v2] iio: hid-sensors: Use software trigger

["Hanne-Lotta Mäenpää" <hannelotta@gmail.com>]

On 2/21/26 12:45 AM, Srinivas Pandruvada wrote:
> Recent changes linux mainline resulted in warning:
> "genirq: Warn about using IRQF_ONESHOT without a threaded handler"
> when HID sensor hub is used.
> 
> When INDIO_BUFFER_TRIGGERED is used, the core attaches a poll function
> when enabling the buffer. This poll function uses request_threaded_irq()
> with both bottom half and top half handlers. But when using HID
> sensor hub, bottom half (thread handler) is not registered.
> 
> In HID sensors, once a sensor is powered on, the hub collects samples
> and pushes data to the host when programmed thresholds are met. When
> this data is received for a sensor, it is pushed using
> iio_push_to_buffers_with_ts().
> 
> The sensor is powered ON or OFF based on the trigger callback
> set_trigger_state() when the poll function is attached. During the call
> to iio_triggered_buffer_setup_ext(), the HID sensor specifies only a
> handler function but provides no thread handler, as there is no data
> to read from the hub in thread context. Internally, this results in
> calling request_threaded_irq(). Recent kernel changes now warn when
> request_threaded_irq() is called without a thread handler.
> 
> To address this issue, fundamental changes are required to avoid using
> iio_triggered_buffer_setup_ext(). HID sensors can use
> INDIO_BUFFER_SOFTWARE instead of INDIO_BUFFER_TRIGGERED, as this can
> work in trigger-less mode.
> 
> In this approach, when user space opens the buffer, the sensor is powered
> on, and when the buffer is closed, the sensor is powered off using
> iio_buffer_setup_ops callbacks.
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
> Changes:
> v2:
> changed
> indio_dev->modes |= INDIO_DIRECT_MODE | INDIO_HARDWARE_TRIGGERED;
> to
> indio_dev->modes = INDIO_DIRECT_MODE | INDIO_HARDWARE_TRIGGERED;
> 
>  From RFC
> - The trigger alloc and register is still kept as old code with
> additional
> indio_dev->modes |= INDIO_DIRECT_MODE | INDIO_HARDWARE_TRIGGERED;
> 
>   .../common/hid-sensors/hid-sensor-trigger.c   | 48 ++++++++++++-------
>   1 file changed, 30 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> index 5540e2d28f4a..417c4ab8c1b2 100644
> --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> @@ -14,6 +14,7 @@
>   #include <linux/iio/triggered_buffer.h>
>   #include <linux/iio/trigger_consumer.h>
>   #include <linux/iio/sysfs.h>
> +#include <linux/iio/kfifo_buf.h>
>   #include "hid-sensor-trigger.h"
>   
>   static ssize_t _hid_sensor_set_report_latency(struct device *dev,
> @@ -202,12 +203,21 @@ static void hid_sensor_set_power_work(struct work_struct *work)
>   		_hid_sensor_power_state(attrb, true);
>   }
>   
> -static int hid_sensor_data_rdy_trigger_set_state(struct iio_trigger *trig,
> -						bool state)
> +static int buffer_postenable(struct iio_dev *indio_dev)
>   {
> -	return hid_sensor_power_state(iio_trigger_get_drvdata(trig), state);
> +	return hid_sensor_power_state(iio_device_get_drvdata(indio_dev), 1);
>   }
>   
> +static int buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	return hid_sensor_power_state(iio_device_get_drvdata(indio_dev), 0);
> +}
> +
> +static const struct iio_buffer_setup_ops hid_sensor_buffer_ops = {
> +	.postenable = buffer_postenable,
> +	.predisable = buffer_predisable,
> +};
> +
>   void hid_sensor_remove_trigger(struct iio_dev *indio_dev,
>   			       struct hid_sensor_common *attrb)
>   {
> @@ -219,14 +229,9 @@ void hid_sensor_remove_trigger(struct iio_dev *indio_dev,
>   	cancel_work_sync(&attrb->work);
>   	iio_trigger_unregister(attrb->trigger);
>   	iio_trigger_free(attrb->trigger);
> -	iio_triggered_buffer_cleanup(indio_dev);
>   }
>   EXPORT_SYMBOL_NS(hid_sensor_remove_trigger, "IIO_HID");
>   
> -static const struct iio_trigger_ops hid_sensor_trigger_ops = {
> -	.set_trigger_state = &hid_sensor_data_rdy_trigger_set_state,
> -};
> -
>   int hid_sensor_setup_trigger(struct iio_dev *indio_dev, const char *name,
>   				struct hid_sensor_common *attrb)
>   {
> @@ -239,25 +244,34 @@ int hid_sensor_setup_trigger(struct iio_dev *indio_dev, const char *name,
>   	else
>   		fifo_attrs = NULL;
>   
> -	ret = iio_triggered_buffer_setup_ext(indio_dev,
> -					     &iio_pollfunc_store_time, NULL,
> -					     IIO_BUFFER_DIRECTION_IN,
> -					     NULL, fifo_attrs);
> +	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_HARDWARE_TRIGGERED;
> +
> +	ret = devm_iio_kfifo_buffer_setup_ext(&indio_dev->dev, indio_dev,
> +					      &hid_sensor_buffer_ops,
> +					      fifo_attrs);
>   	if (ret) {
> -		dev_err(&indio_dev->dev, "Triggered Buffer Setup Failed\n");
> +		dev_err(&indio_dev->dev, "Kfifo Buffer Setup Failed\n");
>   		return ret;
>   	}
>   
> +	/*
> +	 * The current user space in distro "iio-sensor-proxy" is not working in
> +	 * trigerless mode and it expects
> +	 * /sys/bus/iio/devices/iio:device0/trigger/current_trigger.
> +	 * The change replacing iio_triggered_buffer_setup_ext() with
> +	 * devm_iio_kfifo_buffer_setup_ext() will not create attribute without
> +	 * registering a trigger with INDIO_HARDWARE_TRIGGERED.
> +	 * So the below code fragment is still required.
> +	 */
> +
>   	trig = iio_trigger_alloc(indio_dev->dev.parent,
>   				 "%s-dev%d", name, iio_device_id(indio_dev));
>   	if (trig == NULL) {
>   		dev_err(&indio_dev->dev, "Trigger Allocate Failed\n");
> -		ret = -ENOMEM;
> -		goto error_triggered_buffer_cleanup;
> +		return -ENOMEM;
>   	}
>   
>   	iio_trigger_set_drvdata(trig, attrb);
> -	trig->ops = &hid_sensor_trigger_ops;
>   	ret = iio_trigger_register(trig);
>   
>   	if (ret) {
> @@ -284,8 +298,6 @@ int hid_sensor_setup_trigger(struct iio_dev *indio_dev, const char *name,
>   	iio_trigger_unregister(trig);
>   error_free_trig:
>   	iio_trigger_free(trig);
> -error_triggered_buffer_cleanup:
> -	iio_triggered_buffer_cleanup(indio_dev);
>   	return ret;
>   }
>   EXPORT_SYMBOL_NS(hid_sensor_setup_trigger, "IIO_HID");


Hi,

This patch clears the warning "WARNING: kernel/irq/manage.c:1502 at 
__setup_irq" on Debian forky.

Tested-by: Hanne-Lotta Mäenpää

The referenced commit ID could be included in the commit message, like this:

Commit aef30c8d569c0f317154 ("genirq: Warn about using IRQF_ONESHOT 
without a threaded handler")

With that included, this patch will be easier to find if someone bisects 
the issue.

Also, a Closes tag could be added:

Closes: https://bugzilla.kernel.org/show_bug.cgi?id=221256

Best regards,

Hanne-Lotta Mäenpää