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

From: Jonathan Cameron <jic23@kernel.org>
To: Alexandru Ardelean <alexandru.ardelean@analog.com>
Cc: <linux-iio@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 3/3] iio: dac: ad5592r: localize locks only where needed in ad5592r_read_raw()
Date: Thu, 17 Sep 2020 19:42:33 +0100	[thread overview]
Message-ID: <20200917194233.57b548be@archlinux> (raw)
In-Reply-To: <20200706110259.23947-3-alexandru.ardelean@analog.com>

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,