Re: [PATCH v3] iio: common: st_sensors: fix possible infinite loop in st_sensors_irq_thread

[Jonathan Cameron <jic23@kernel.org>]

On Tue,  8 Dec 2020 15:36:40 +0100
Lorenzo Bianconi <lorenzo@kernel.org> wrote:

> Return a boolean value in st_sensors_new_samples_available routine in
> order to avoid an infinite loop in st_sensors_irq_thread if
> stat_drdy.addr is not defined or stat_drdy read fails
> 
> Fixes: 90efe05562921 ("iio: st_sensors: harden interrupt handling")
> Reported-by: Jonathan Cameron <jic23@kernel.org>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>

One trivial inline. Note I'm looking for an Ack form Dennis on this one
as it may result in a some breakage if any devices are using
edge interrupts.

It's possible we would be better falling back to interrupt free support
in that case rather than failing to probe at all.
I think that would be best done by moving the check out to the
various per type drivers so we fail in the same fashion as no irq
provided + a warning.

thanks,

Jonathan


> ---
> Changes since v2:
> - return -EOPNOTSUPP if the drv requests edge IRQ and the sensor does not support
>   status register
> 
> Changes since v1:
> - return true if the sensor does not have stat_drdy register
> ---
>  .../common/st_sensors/st_sensors_trigger.c    | 31 ++++++++++---------
>  1 file changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> index 0507283bd4c1..2dbd2646e44e 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_trigger.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> @@ -23,35 +23,31 @@
>   * @sdata: Sensor data.
>   *
>   * returns:
> - * 0 - no new samples available
> - * 1 - new samples available
> - * negative - error or unknown
> + * false - no new samples available or read error
> + * true - new samples available
>   */
> -static int st_sensors_new_samples_available(struct iio_dev *indio_dev,
> -					    struct st_sensor_data *sdata)
> +static bool st_sensors_new_samples_available(struct iio_dev *indio_dev,
> +					     struct st_sensor_data *sdata)
>  {
>  	int ret, status;
>  
>  	/* How would I know if I can't check it? */
>  	if (!sdata->sensor_settings->drdy_irq.stat_drdy.addr)
> -		return -EINVAL;
> +		return true;
>  
>  	/* No scan mask, no interrupt */
>  	if (!indio_dev->active_scan_mask)
> -		return 0;
> +		return false;
>  
>  	ret = regmap_read(sdata->regmap,
>  			  sdata->sensor_settings->drdy_irq.stat_drdy.addr,
>  			  &status);
>  	if (ret < 0) {
>  		dev_err(sdata->dev, "error checking samples available\n");
> -		return ret;
> +		return false;
>  	}
>  
> -	if (status & sdata->sensor_settings->drdy_irq.stat_drdy.mask)
> -		return 1;
> -
> -	return 0;
> +	return !!(status & sdata->sensor_settings->drdy_irq.stat_drdy.mask);

No need for the !! as you can rely on type conversion to a boolean.

>  }
>  
>  /**
> @@ -180,9 +176,15 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
>  
>  	/* Tell the interrupt handler that we're dealing with edges */
>  	if (irq_trig == IRQF_TRIGGER_FALLING ||
> -	    irq_trig == IRQF_TRIGGER_RISING)
> +	    irq_trig == IRQF_TRIGGER_RISING) {
> +		if (!sdata->sensor_settings->drdy_irq.stat_drdy.addr) {
> +			dev_err(&indio_dev->dev,
> +				"edge IRQ not supported w/o stat register.\n");
> +			err = -EOPNOTSUPP;
> +			goto iio_trigger_free;
> +		}
>  		sdata->edge_irq = true;
> -	else
> +	} else {
>  		/*
>  		 * If we're not using edges (i.e. level interrupts) we
>  		 * just mask off the IRQ, handle one interrupt, then
> @@ -190,6 +192,7 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
>  		 * interrupt handler top half again and start over.
>  		 */
>  		irq_trig |= IRQF_ONESHOT;
> +	}
>  
>  	/*
>  	 * If the interrupt pin is Open Drain, by definition this