diff mbox series

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

Message ID 20220609083213.1795019-12-claudiu.beznea@microchip.com (mailing list archive)
State New, archived
Headers show
Series iio: adc: at91-sama5d2_adc: add support for temperature sensor | expand

Commit Message

Claudiu Beznea June 9, 2022, 8:32 a.m. UTC
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.

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

Comments

Jonathan Cameron June 11, 2022, 5:58 p.m. UTC | #1
On Thu, 9 Jun 2022 11:32:08 +0300
Claudiu Beznea <claudiu.beznea@microchip.com> wrote:

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

Jonathan


> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
>  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)
Claudiu Beznea June 14, 2022, 8:50 a.m. UTC | #2
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 <claudiu.beznea@microchip.com> wrote:
> 
>> 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
> 
> 
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>> ---
>>  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)
>
Jonathan Cameron June 14, 2022, 12:02 p.m. UTC | #3
On Tue, 14 Jun 2022 08:50:14 +0000
<Claudiu.Beznea@microchip.com> wrote:

> 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 <claudiu.beznea@microchip.com> wrote:
> >   
> >> 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.

Hi Claudia,

Minor kernel mailing list etiquette thing is that there is no need
to reply to say you'll check something or that you agree with review
feedback.  Just generates more emails to read.  Reviewers assume
anything you don't comment on their feedback will be addressed in
next version of the code!

Thanks,

Jonathan

> 
> > 
> > Jonathan
> > 
> >   
> >>
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> >> ---
> >>  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)  
> >   
>
diff mbox series

Patch

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)