[PATCH 1/3] iio: dac: ad5592r: fix unbalanced mutex unlocks in ad5592r_read_raw()

[PATCH 1/3] iio: dac: ad5592r: fix unbalanced mutex unlocks in ad5592r_read_raw()

[Alexandru Ardelean]

There are 2 exit paths where the lock isn't held, but try to unlock the
mutex when exiting. In these places we should just return from the
function.

A neater approach would be to cleanup the ad5592r_read_raw(), but that
would make this patch more difficult to backport to stable versions.

Fixes 56ca9db862bf3: ("iio: dac: Add support for the AD5592R/AD5593R ADCs/DACs")
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/dac/ad5592r-base.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/dac/ad5592r-base.c b/drivers/iio/dac/ad5592r-base.c
index 5c4e5ff70380..cc4875660a69 100644
--- a/drivers/iio/dac/ad5592r-base.c
+++ b/drivers/iio/dac/ad5592r-base.c
@@ -413,7 +413,7 @@ static int ad5592r_read_raw(struct iio_dev *iio_dev,
 			s64 tmp = *val * (3767897513LL / 25LL);
 			*val = div_s64_rem(tmp, 1000000000LL, val2);
 
-			ret = IIO_VAL_INT_PLUS_MICRO;
+			return IIO_VAL_INT_PLUS_MICRO;
 		} else {
 			int mult;
 
@@ -444,7 +444,7 @@ static int ad5592r_read_raw(struct iio_dev *iio_dev,
 		ret =  IIO_VAL_INT;
 		break;
 	default:
-		ret = -EINVAL;
+		return -EINVAL;
 	}
 
 unlock:
-- 
2.25.1

[PATCH 2/3] iio: dac: ad5592r: un-indent code-block for scale read

[Alexandru Ardelean]

The next rework may require an unindentation of a code block in
ad5592r_read_raw(), which would make review a bit more difficult.

This change unindents the code block for reading the scale of the
non-temperature channels.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/dac/ad5592r-base.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/dac/ad5592r-base.c b/drivers/iio/dac/ad5592r-base.c
index cc4875660a69..7ea79e9bfa1d 100644
--- a/drivers/iio/dac/ad5592r-base.c
+++ b/drivers/iio/dac/ad5592r-base.c
@@ -376,7 +376,7 @@ static int ad5592r_read_raw(struct iio_dev *iio_dev,
 {
 	struct ad5592r_state *st = iio_priv(iio_dev);
 	u16 read_val;
-	int ret;
+	int ret, mult;
 
 	switch (m) {
 	case IIO_CHAN_INFO_RAW:
@@ -414,23 +414,21 @@ static int ad5592r_read_raw(struct iio_dev *iio_dev,
 			*val = div_s64_rem(tmp, 1000000000LL, val2);
 
 			return IIO_VAL_INT_PLUS_MICRO;
-		} else {
-			int mult;
+		}
 
-			mutex_lock(&st->lock);
+		mutex_lock(&st->lock);
 
-			if (chan->output)
-				mult = !!(st->cached_gp_ctrl &
-					AD5592R_REG_CTRL_DAC_RANGE);
-			else
-				mult = !!(st->cached_gp_ctrl &
-					AD5592R_REG_CTRL_ADC_RANGE);
+		if (chan->output)
+			mult = !!(st->cached_gp_ctrl &
+				AD5592R_REG_CTRL_DAC_RANGE);
+		else
+			mult = !!(st->cached_gp_ctrl &
+				AD5592R_REG_CTRL_ADC_RANGE);
 
-			*val *= ++mult;
+		*val *= ++mult;
 
-			*val2 = chan->scan_type.realbits;
-			ret = IIO_VAL_FRACTIONAL_LOG2;
-		}
+		*val2 = chan->scan_type.realbits;
+		ret = IIO_VAL_FRACTIONAL_LOG2;
 		break;
 	case IIO_CHAN_INFO_OFFSET:
 		ret = ad5592r_get_vref(st);
-- 
2.25.1

[PATCH 3/3] iio: dac: ad5592r: localize locks only where needed in ad5592r_read_raw()

[Alexandru Ardelean]

Since there was a recently discovered issue with these locks, it probably
makes sense to cleanup the code a bit, to prevent it from being used as an
example/reference.

This change moves the lock only where it is explicitly needed to protect
resources from potential concurrent accesses.

It also reworks the switch statements to do direct returns vs caching the
return value on a variable.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/dac/ad5592r-base.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/iio/dac/ad5592r-base.c b/drivers/iio/dac/ad5592r-base.c
index 7ea79e9bfa1d..f697b20efb6e 100644
--- a/drivers/iio/dac/ad5592r-base.c
+++ b/drivers/iio/dac/ad5592r-base.c
@@ -380,32 +380,32 @@ static int ad5592r_read_raw(struct iio_dev *iio_dev,
 
 	switch (m) {
 	case IIO_CHAN_INFO_RAW:
-		mutex_lock(&st->lock);
-
 		if (!chan->output) {
+			mutex_lock(&st->lock);
 			ret = st->ops->read_adc(st, chan->channel, &read_val);
+			mutex_unlock(&st->lock);
 			if (ret)
-				goto unlock;
+				return ret;
 
 			if ((read_val >> 12 & 0x7) != (chan->channel & 0x7)) {
 				dev_err(st->dev, "Error while reading channel %u\n",
 						chan->channel);
-				ret = -EIO;
-				goto unlock;
+				return -EIO;
 			}
 
 			read_val &= GENMASK(11, 0);
 
 		} else {
+			mutex_lock(&st->lock);
 			read_val = st->cached_dac[chan->channel];
+			mutex_unlock(&st->lock);
 		}
 
 		dev_dbg(st->dev, "Channel %u read: 0x%04hX\n",
 				chan->channel, read_val);
 
 		*val = (int) read_val;
-		ret = IIO_VAL_INT;
-		break;
+		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
 		*val = ad5592r_get_vref(st);
 
@@ -425,11 +425,13 @@ static int ad5592r_read_raw(struct iio_dev *iio_dev,
 			mult = !!(st->cached_gp_ctrl &
 				AD5592R_REG_CTRL_ADC_RANGE);
 
+		mutex_unlock(&st->lock);
+
 		*val *= ++mult;
 
 		*val2 = chan->scan_type.realbits;
-		ret = IIO_VAL_FRACTIONAL_LOG2;
-		break;
+
+		return IIO_VAL_FRACTIONAL_LOG2;
 	case IIO_CHAN_INFO_OFFSET:
 		ret = ad5592r_get_vref(st);
 
@@ -439,15 +441,13 @@ static int ad5592r_read_raw(struct iio_dev *iio_dev,
 			*val = (-34365 * 25) / ret;
 		else
 			*val = (-75365 * 25) / ret;
-		ret =  IIO_VAL_INT;
-		break;
+
+		mutex_unlock(&st->lock);
+
+		return IIO_VAL_INT;
 	default:
 		return -EINVAL;
 	}
-
-unlock:
-	mutex_unlock(&st->lock);
-	return ret;
 }
 
 static int ad5592r_write_raw_get_fmt(struct iio_dev *indio_dev,
-- 
2.25.1

Re: [PATCH 1/3] iio: dac: ad5592r: fix unbalanced mutex unlocks in ad5592r_read_raw()

[Ardelean, Alexandru]

On Mon, 2020-07-06 at 14:02 +0300, Alexandru Ardelean wrote:
> [External]
> 
> There are 2 exit paths where the lock isn't held, but try to unlock the
> mutex when exiting. In these places we should just return from the
> function.
> 
> A neater approach would be to cleanup the ad5592r_read_raw(), but that
> would make this patch more difficult to backport to stable versions.
> 

I was a bit too hasty with this.
Apologies.
I'd like to add a tag here.

Reported-by: Charles Stanhope <charles.stanhope@gmail.com>

> Fixes 56ca9db862bf3: ("iio: dac: Add support for the AD5592R/AD5593R
> ADCs/DACs")
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  drivers/iio/dac/ad5592r-base.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/dac/ad5592r-base.c b/drivers/iio/dac/ad5592r-
> base.c
> index 5c4e5ff70380..cc4875660a69 100644
> --- a/drivers/iio/dac/ad5592r-base.c
> +++ b/drivers/iio/dac/ad5592r-base.c
> @@ -413,7 +413,7 @@ static int ad5592r_read_raw(struct iio_dev *iio_dev,
>  			s64 tmp = *val * (3767897513LL / 25LL);
>  			*val = div_s64_rem(tmp, 1000000000LL, val2);
>  
> -			ret = IIO_VAL_INT_PLUS_MICRO;
> +			return IIO_VAL_INT_PLUS_MICRO;
>  		} else {
>  			int mult;
>  
> @@ -444,7 +444,7 @@ static int ad5592r_read_raw(struct iio_dev *iio_dev,
>  		ret =  IIO_VAL_INT;
>  		break;
>  	default:
> -		ret = -EINVAL;
> +		return -EINVAL;
>  	}
>  
>  unlock:

Re: [PATCH 1/3] iio: dac: ad5592r: fix unbalanced mutex unlocks in ad5592r_read_raw()

[Jonathan Cameron]

On Mon, 6 Jul 2020 16:36:09 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Mon, 2020-07-06 at 14:02 +0300, Alexandru Ardelean wrote:
> > [External]
> > 
> > There are 2 exit paths where the lock isn't held, but try to unlock the
> > mutex when exiting. In these places we should just return from the
> > function.
> > 
> > A neater approach would be to cleanup the ad5592r_read_raw(), but that
> > would make this patch more difficult to backport to stable versions.
> >   
> 
> I was a bit too hasty with this.
> Apologies.
> I'd like to add a tag here.
> 
> Reported-by: Charles Stanhope <charles.stanhope@gmail.com>
Applied to the fixes-togreg branch of iio.git.

I'll have to wait for this to trickle through to the togreg branch though
before I can do anything with the next two patches.  As ever, if I've
clearly lost them give me a poke!

Jonathan

> 
> > Fixes 56ca9db862bf3: ("iio: dac: Add support for the AD5592R/AD5593R
> > ADCs/DACs")
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> >  drivers/iio/dac/ad5592r-base.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/dac/ad5592r-base.c b/drivers/iio/dac/ad5592r-
> > base.c
> > index 5c4e5ff70380..cc4875660a69 100644
> > --- a/drivers/iio/dac/ad5592r-base.c
> > +++ b/drivers/iio/dac/ad5592r-base.c
> > @@ -413,7 +413,7 @@ static int ad5592r_read_raw(struct iio_dev *iio_dev,
> >  			s64 tmp = *val * (3767897513LL / 25LL);
> >  			*val = div_s64_rem(tmp, 1000000000LL, val2);
> >  
> > -			ret = IIO_VAL_INT_PLUS_MICRO;
> > +			return IIO_VAL_INT_PLUS_MICRO;
> >  		} else {
> >  			int mult;
> >  
> > @@ -444,7 +444,7 @@ static int ad5592r_read_raw(struct iio_dev *iio_dev,
> >  		ret =  IIO_VAL_INT;
> >  		break;
> >  	default:
> > -		ret = -EINVAL;
> > +		return -EINVAL;
> >  	}
> >  
> >  unlock:  

Re: [PATCH 3/3] iio: dac: ad5592r: localize locks only where needed in ad5592r_read_raw()

[Jonathan Cameron]

On Mon, 6 Jul 2020 14:02:59 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> Since there was a recently discovered issue with these locks, it probably
> makes sense to cleanup the code a bit, to prevent it from being used as an
> example/reference.
> 
> This change moves the lock only where it is explicitly needed to protect
> resources from potential concurrent accesses.
> 
> It also reworks the switch statements to do direct returns vs caching the
> return value on a variable.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Applied these two to the togreg branch of iio.git

Thanks,

Jonathan

> ---
>  drivers/iio/dac/ad5592r-base.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/iio/dac/ad5592r-base.c b/drivers/iio/dac/ad5592r-base.c
> index 7ea79e9bfa1d..f697b20efb6e 100644
> --- a/drivers/iio/dac/ad5592r-base.c
> +++ b/drivers/iio/dac/ad5592r-base.c
> @@ -380,32 +380,32 @@ static int ad5592r_read_raw(struct iio_dev *iio_dev,
>  
>  	switch (m) {
>  	case IIO_CHAN_INFO_RAW:
> -		mutex_lock(&st->lock);
> -
>  		if (!chan->output) {
> +			mutex_lock(&st->lock);
>  			ret = st->ops->read_adc(st, chan->channel, &read_val);
> +			mutex_unlock(&st->lock);
>  			if (ret)
> -				goto unlock;
> +				return ret;
>  
>  			if ((read_val >> 12 & 0x7) != (chan->channel & 0x7)) {
>  				dev_err(st->dev, "Error while reading channel %u\n",
>  						chan->channel);
> -				ret = -EIO;
> -				goto unlock;
> +				return -EIO;
>  			}
>  
>  			read_val &= GENMASK(11, 0);
>  
>  		} else {
> +			mutex_lock(&st->lock);
>  			read_val = st->cached_dac[chan->channel];
> +			mutex_unlock(&st->lock);
>  		}
>  
>  		dev_dbg(st->dev, "Channel %u read: 0x%04hX\n",
>  				chan->channel, read_val);
>  
>  		*val = (int) read_val;
> -		ret = IIO_VAL_INT;
> -		break;
> +		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_SCALE:
>  		*val = ad5592r_get_vref(st);
>  
> @@ -425,11 +425,13 @@ static int ad5592r_read_raw(struct iio_dev *iio_dev,
>  			mult = !!(st->cached_gp_ctrl &
>  				AD5592R_REG_CTRL_ADC_RANGE);
>  
> +		mutex_unlock(&st->lock);
> +
>  		*val *= ++mult;
>  
>  		*val2 = chan->scan_type.realbits;
> -		ret = IIO_VAL_FRACTIONAL_LOG2;
> -		break;
> +
> +		return IIO_VAL_FRACTIONAL_LOG2;
>  	case IIO_CHAN_INFO_OFFSET:
>  		ret = ad5592r_get_vref(st);
>  
> @@ -439,15 +441,13 @@ static int ad5592r_read_raw(struct iio_dev *iio_dev,
>  			*val = (-34365 * 25) / ret;
>  		else
>  			*val = (-75365 * 25) / ret;
> -		ret =  IIO_VAL_INT;
> -		break;
> +
> +		mutex_unlock(&st->lock);
> +
> +		return IIO_VAL_INT;
>  	default:
>  		return -EINVAL;
>  	}
> -
> -unlock:
> -	mutex_unlock(&st->lock);
> -	return ret;
>  }
>  
>  static int ad5592r_write_raw_get_fmt(struct iio_dev *indio_dev,