Re: [PATCH] iio: trigger: Add validation to reject devices requiring top half

[Jonathan Cameron]

On Sun,  4 May 2025 04:00:43 +0900
Gyeyoung Baek <gye976@xxxxxxxxx> wrote:

> Some device drivers implement top-half handler,
> which is not compatible with threaded handler trigger.
> This patch adds a validation function to reject such devices,
> allowing only iio_pollfunc_store_time().

This needs more reasoning.  What makes it not work?
+ what do we mean by not compatible?
I'd expect at least a reference to it using iio_trigger_poll_nested()
directly.

It's unfortunately hard to tell whether a top half handler is
actually needed or not.  As a follow up question, what cases do we have
of top half / interrupt context handlers other than iio_pollfunc_store_time()?

Maybe we don't need this code to be this complex any more at all
(i.e. could it become a flag to say whether the timestamp is useful or not)
rather than registering the callback.

Jonathan


> 
> Signed-off-by: Gyeyoung Baek <gye976@xxxxxxxxx>
> ---
>  drivers/iio/trigger/iio-trig-loop.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/trigger/iio-trig-loop.c b/drivers/iio/trigger/iio-trig-loop.c
> index 7aaed0611899..a37615567a6c 100644
> --- a/drivers/iio/trigger/iio-trig-loop.c
> +++ b/drivers/iio/trigger/iio-trig-loop.c
> @@ -7,18 +7,12 @@
>   *
>   * Note this is still rather experimental and may eat babies.
>   *
> - * Todo
> - * * Protect against connection of devices that 'need' the top half
> - *   handler.
> - * * Work out how to run top half handlers in this context if it is
> - *   safe to do so (timestamp grabbing for example)
> - *
>   * Tested against a max1363. Used about 33% cpu for the thread and 20%
>   * for generic_buffer piping to /dev/null. Watermark set at 64 on a 128
>   * element kfifo buffer.
>   */
>  
> -#include <linux/kernel.h>
> +#include <linux/errno.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
> @@ -27,8 +21,10 @@
>  #include <linux/freezer.h>
>  
>  #include <linux/iio/iio.h>
> -#include <linux/iio/trigger.h>
>  #include <linux/iio/sw_trigger.h>
> +#include <linux/iio/trigger.h>
> +

Improving header ordering / which headers are here is good to do, but
it needs to be in a separate patch.

> +#include "linux/iio/trigger_consumer.h"
>  
>  struct iio_loop_info {
>  	struct iio_sw_trigger swt;
> @@ -71,8 +67,25 @@ static int iio_loop_trigger_set_state(struct iio_trigger *trig, bool state)
>  	return 0;
>  }
>  
> +/*
> + * Protect against connection of devices that 'need' the top half
> + * handler.
> + */
> +static int iio_loop_trigger_validate_device(struct iio_trigger *trig,
> +						struct iio_dev *indio_dev)
> +{
> +	struct iio_poll_func *pf = indio_dev->pollfunc;
> +
> +	/* Only iio timestamp grabbing is allowed. */
> +	if (pf->h && pf->h != iio_pollfunc_store_time)

Why is iio_pollfunc_store_time useable here?  It's not going to store the
time if we don't call it...  We could special case it probably but we'd
need to ensure the call is actually made.


> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  static const struct iio_trigger_ops iio_loop_trigger_ops = {
>  	.set_trigger_state = iio_loop_trigger_set_state,
> +	.validate_device = iio_loop_trigger_validate_device,
>  };
>  
>  static struct iio_sw_trigger *iio_trig_loop_probe(const char *name)