Re: [PATCH 11/16] iio: adc: at91-sama5d2_adc: add locking parameter to at91_adc_read_info_raw()

On 11.06.2022 20:58, Jonathan Cameron wrote:

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

On Thu, 9 Jun 2022 11:32:08 +0300
Claudiu Beznea [off-list ref] wrote:

quoted

Add a parameter to at91_adc_read_info_raw() to specify if st->lock mutex
need to be acquired. This prepares for the addition of temperature sensor
code which will re-use at91_adc_read_info_raw() function to read 2 voltages
for determining the real temperature.

This looks like a potential lock dependency issue.
iio_device_claim_direct_mode() takes an internal iio lock, and
you then take st->lock.

If you are going to invert that locking order in another path
you have a deadlock.

So rethink this. If you want to reuse the code you'll need to factor
it out to a separate function that takes none of the locks then
take all locks needed in each call path (in the same order).

OK, I'll check it.

Jonathan

quoted

Signed-off-by: Claudiu Beznea <redacted>
---
 drivers/iio/adc/at91-sama5d2_adc.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 1283bcf4e682..8f8fef42de84 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c

@@ -1583,7 +1583,8 @@ static irqreturn_t at91_adc_interrupt(int irq, void *private)
 }

 static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
-                               struct iio_chan_spec const *chan, int *val)
+                               struct iio_chan_spec const *chan, int *val,
+                               bool lock)
 {
      struct at91_adc_state *st = iio_priv(indio_dev);
      int (*fn)(struct at91_adc_state *, int, u16 *) = NULL;

@@ -1602,13 +1603,15 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
      ret = iio_device_claim_direct_mode(indio_dev);
      if (ret)
              return ret;
-     mutex_lock(&st->lock);
+     if (lock)
+             mutex_lock(&st->lock);

      if (fn) {
              ret = fn(st, chan->channel, &tmp_val);
              *val = tmp_val;
              ret = at91_adc_adjust_val_osr(st, val);
-             mutex_unlock(&st->lock);
+             if (lock)
+                     mutex_unlock(&st->lock);
              iio_device_release_direct_mode(indio_dev);

              return ret;

@@ -1644,7 +1647,8 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
      /* Needed to ACK the DRDY interruption */
      at91_adc_readl(st, LCDR);

-     mutex_unlock(&st->lock);
+     if (lock)
+             mutex_unlock(&st->lock);

      iio_device_release_direct_mode(indio_dev);
      return ret;

@@ -1658,7 +1662,8 @@ static int at91_adc_read_raw(struct iio_dev *indio_dev,

      switch (mask) {
      case IIO_CHAN_INFO_RAW:
-             return at91_adc_read_info_raw(indio_dev, chan, val);
+             return at91_adc_read_info_raw(indio_dev, chan, val, true);
+
      case IIO_CHAN_INFO_SCALE:
              *val = st->vref_uv / 1000;
              if (chan->differential)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel