iio: adc: ad_sigma_delta: Fix a race condition

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>
Link: https://patch.msgid.link/9e6def47e2e773e0e15b7a2c29d22629b53d91b1.1733504533.git.u.kleine-koenig@baylibre.com
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index 65608dc2bfecae1083e8bd11394b89b51d811c1a..950baf4160da4c4d1cfe3632c2dd042c7e2a279c 100644 (file)
--- 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 895b7ebf4be5ef3779de13440c3b9ee58c25e46a..200130e4244df388a7a33491353f02126e57ef4c 100644 (file)
--- 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;