diff mbox series

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

Message ID 9e6def47e2e773e0e15b7a2c29d22629b53d91b1.1733504533.git.u.kleine-koenig@baylibre.com (mailing list archive)
State Accepted
Headers show
Series iio: adc: ad7124: Various fixes | expand

Commit Message

Uwe Kleine-König Dec. 6, 2024, 5:28 p.m. UTC 
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>
---
 drivers/iio/adc/ad_sigma_delta.c       | 56 ++++++++++++++++----------
 include/linux/iio/adc/ad_sigma_delta.h |  1 +
 2 files changed, 36 insertions(+), 21 deletions(-)

Comments

Jonathan Cameron Dec. 8, 2024, 12:42 p.m. UTC | #1 
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;
Andy Shevchenko Dec. 8, 2024, 1:05 p.m. UTC | #2 
On Sun, Dec 8, 2024 at 2:42 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Fri,  6 Dec 2024 18:28:38 +0100
> Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

...

> 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.

I'm wondering if sparse annotation __acquire and __release may help here...
Jonathan Cameron Dec. 8, 2024, 6:23 p.m. UTC | #3 
On Sun, 8 Dec 2024 15:05:38 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sun, Dec 8, 2024 at 2:42 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > On Fri,  6 Dec 2024 18:28:38 +0100
> > Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:  
> 
> ...
> 
> > 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.  
> 
> I'm wondering if sparse annotation __acquire and __release may help here...

The complaint is (I think) about guard(spinlock_irqsave)
so I'm not immediately sure how.
Andy Shevchenko Dec. 9, 2024, 12:52 a.m. UTC | #4 
On Sun, Dec 8, 2024 at 8:23 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Sun, 8 Dec 2024 15:05:38 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Sun, Dec 8, 2024 at 2:42 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > > On Fri,  6 Dec 2024 18:28:38 +0100

...

> > > 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.
> >
> > I'm wondering if sparse annotation __acquire and __release may help here...
>
> The complaint is (I think) about guard(spinlock_irqsave)
> so I'm not immediately sure how.

In cases like

 if (x)
   lock();
 ...do smth...
if (x)
  unlock()

Those annotations give sparse hints that locking is balanced. That is
why I was thinking it may help guard as well.
Uwe Kleine-König Dec. 9, 2024, 9:37 a.m. UTC | #5 
Hello Jonathan,

On Sun, Dec 08, 2024 at 12:42:05PM +0000, Jonathan Cameron wrote:
> 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.

Yes, my discussion was about a different driver, but it's the same here.
sparse is happy when applying the following patch:

diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index d32102b25530..dea4816793fa 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -206,23 +206,33 @@ 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);
+	unsigned long flags;
+
+	spin_lock_irqsave(&sigma_delta->irq_lock, flags);
 
 	/* It's already off, return false to indicate nothing was changed */
-	if (sigma_delta->irq_dis)
+	if (sigma_delta->irq_dis) {
+		spin_unlock_irqrestore(&sigma_delta->irq_lock, flags);
 		return false;
+	}
 
 	sigma_delta->irq_dis = true;
 	disable_irq_nosync(sigma_delta->irq_line);
+	spin_unlock_irqrestore(&sigma_delta->irq_lock, flags);
+
 	return true;
 }
 
 static void ad_sd_enable_irq(struct ad_sigma_delta *sigma_delta)
 {
-	guard(spinlock_irqsave)(&sigma_delta->irq_lock);
+	unsigned long flags;
+
+	spin_lock_irqsave(&sigma_delta->irq_lock, flags);
 
 	sigma_delta->irq_dis = false;
 	enable_irq(sigma_delta->irq_line);
+
+	spin_unlock_irqrestore(&sigma_delta->irq_lock, flags);
 }
 
 #define AD_SD_CLEAR_DATA_BUFLEN 9

which results in equivalent code. Also TTBOMK this can only be fixed by
teaching sparse about the cleanup stuff and an annotation doesn't help.

So yes, please move on (or fix sparse :-)

Best regards
Uwe
diff mbox series

Patch

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;