Re: [PATCH v6 06/10] iio: adc: ad_sigma_delta: Fix a race condition

[Jonathan Cameron <jic23@kernel.org>]

On Fri,  6 Dec 2024 18:28:38 +0100
Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> The ad_sigma_delta driver helper uses irq_disable_nosync(). With that
> one it is possible that the irq handler still runs after the
> irq_disable_nosync() function call returns. Also to properly synchronize
> irq disabling in the different threads proper locking is needed and
> because it's unclear if the irq handler's irq_disable_nosync() call
> comes first or the one in the enabler's error path, all code locations
> that disable the irq must check for .irq_dis first to ensure there is
> exactly one disable call per enable call.
> 
> So add a spinlock to the struct ad_sigma_delta and use it to synchronize
> irq enabling and disabling. Also only act in the irq handler if the irq
> is still enabled.
> 
> Fixes: af3008485ea0 ("iio:adc: Add common code for ADI Sigma Delta devices")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

From sparse.

drivers/iio/adc/ad_sigma_delta.c:205:13: warning: context imbalance in 'ad_sd_disable_irq' - wrong count at exit
drivers/iio/adc/ad_sigma_delta.c:218:13: warning: context imbalance in 'ad_sd_enable_irq' - wrong count at exit

I saw your discussion with Linus on this...

https://lore.kernel.org/all/CAHk-=wiVDZejo_1BhOaR33qb=pny7sWnYtP4JUbRTXkXCkW6jA@mail.gmail.com/

So I guess we just treat that as a false positive and move on.

> ---
>  drivers/iio/adc/ad_sigma_delta.c       | 56 ++++++++++++++++----------
>  include/linux/iio/adc/ad_sigma_delta.h |  1 +
>  2 files changed, 36 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
> index ff20fa61c293..a4c31baa9c9e 100644
> --- a/drivers/iio/adc/ad_sigma_delta.c
> +++ b/drivers/iio/adc/ad_sigma_delta.c
> @@ -202,6 +202,27 @@ int ad_sd_reset(struct ad_sigma_delta *sigma_delta,
>  }
>  EXPORT_SYMBOL_NS_GPL(ad_sd_reset, IIO_AD_SIGMA_DELTA);
>  
> +static bool ad_sd_disable_irq(struct ad_sigma_delta *sigma_delta)
> +{
> +	guard(spinlock_irqsave)(&sigma_delta->irq_lock);
> +
> +	/* It's already off, return false to indicate nothing was changed */
> +	if (sigma_delta->irq_dis)
> +		return false;
> +
> +	sigma_delta->irq_dis = true;
> +	disable_irq_nosync(sigma_delta->irq_line);
> +	return true;
> +}
> +
> +static void ad_sd_enable_irq(struct ad_sigma_delta *sigma_delta)
> +{
> +	guard(spinlock_irqsave)(&sigma_delta->irq_lock);
> +
> +	sigma_delta->irq_dis = false;
> +	enable_irq(sigma_delta->irq_line);
> +}
> +
>  int ad_sd_calibrate(struct ad_sigma_delta *sigma_delta,
>  	unsigned int mode, unsigned int channel)
>  {
> @@ -221,12 +242,10 @@ int ad_sd_calibrate(struct ad_sigma_delta *sigma_delta,
>  	if (ret < 0)
>  		goto out;
>  
> -	sigma_delta->irq_dis = false;
> -	enable_irq(sigma_delta->irq_line);
> +	ad_sd_enable_irq(sigma_delta);
>  	time_left = wait_for_completion_timeout(&sigma_delta->completion, 2 * HZ);
>  	if (time_left == 0) {
> -		sigma_delta->irq_dis = true;
> -		disable_irq_nosync(sigma_delta->irq_line);
> +		ad_sd_disable_irq(sigma_delta);
>  		ret = -EIO;
>  	} else {
>  		ret = 0;
> @@ -294,8 +313,7 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev,
>  
>  	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_SINGLE);
>  
> -	sigma_delta->irq_dis = false;
> -	enable_irq(sigma_delta->irq_line);
> +	ad_sd_enable_irq(sigma_delta);
>  	ret = wait_for_completion_interruptible_timeout(
>  			&sigma_delta->completion, HZ);
>  
> @@ -314,10 +332,7 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev,
>  		&raw_sample);
>  
>  out:
> -	if (!sigma_delta->irq_dis) {
> -		disable_irq_nosync(sigma_delta->irq_line);
> -		sigma_delta->irq_dis = true;
> -	}
> +	ad_sd_disable_irq(sigma_delta);
>  
>  	sigma_delta->keep_cs_asserted = false;
>  	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE);
> @@ -396,8 +411,7 @@ static int ad_sd_buffer_postenable(struct iio_dev *indio_dev)
>  	if (ret)
>  		goto err_unlock;
>  
> -	sigma_delta->irq_dis = false;
> -	enable_irq(sigma_delta->irq_line);
> +	ad_sd_enable_irq(sigma_delta);
>  
>  	return 0;
>  
> @@ -414,10 +428,7 @@ static int ad_sd_buffer_postdisable(struct iio_dev *indio_dev)
>  	reinit_completion(&sigma_delta->completion);
>  	wait_for_completion_timeout(&sigma_delta->completion, HZ);
>  
> -	if (!sigma_delta->irq_dis) {
> -		disable_irq_nosync(sigma_delta->irq_line);
> -		sigma_delta->irq_dis = true;
> -	}
> +	ad_sd_disable_irq(sigma_delta);
>  
>  	sigma_delta->keep_cs_asserted = false;
>  	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE);
> @@ -516,8 +527,7 @@ static irqreturn_t ad_sd_trigger_handler(int irq, void *p)
>  
>  irq_handled:
>  	iio_trigger_notify_done(indio_dev->trig);
> -	sigma_delta->irq_dis = false;
> -	enable_irq(sigma_delta->irq_line);
> +	ad_sd_enable_irq(sigma_delta);
>  
>  	return IRQ_HANDLED;
>  }
> @@ -551,11 +561,13 @@ static irqreturn_t ad_sd_data_rdy_trig_poll(int irq, void *private)
>  	 * So read the MOSI line as GPIO (if available) and only trigger the irq
>  	 * if the line is active. Without such a GPIO assume this is a valid
>  	 * interrupt.
> +	 *
> +	 * Also as disable_irq_nosync() is used to disable the irq, only act if
> +	 * the irq wasn't disabled before.
>  	 */
> -	if (!sigma_delta->rdy_gpiod || gpiod_get_value(sigma_delta->rdy_gpiod)) {
> +	if ((!sigma_delta->rdy_gpiod || gpiod_get_value(sigma_delta->rdy_gpiod)) &&
> +	    ad_sd_disable_irq(sigma_delta)) {
>  		complete(&sigma_delta->completion);
> -		disable_irq_nosync(irq);
> -		sigma_delta->irq_dis = true;
>  		iio_trigger_poll(sigma_delta->trig);
>  
>  		return IRQ_HANDLED;
> @@ -691,6 +703,8 @@ int ad_sd_init(struct ad_sigma_delta *sigma_delta, struct iio_dev *indio_dev,
>  		}
>  	}
>  
> +	spin_lock_init(&sigma_delta->irq_lock);
> +
>  	if (info->irq_line)
>  		sigma_delta->irq_line = info->irq_line;
>  	else
> diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
> index 126b187d70e9..f86eca6126b4 100644
> --- a/include/linux/iio/adc/ad_sigma_delta.h
> +++ b/include/linux/iio/adc/ad_sigma_delta.h
> @@ -86,6 +86,7 @@ struct ad_sigma_delta {
>  
>  /* private: */
>  	struct completion	completion;
> +	spinlock_t		irq_lock; /* protects .irq_dis and irq en/disable state */
>  	bool			irq_dis;
>  
>  	bool			bus_locked;