On Mon, 6 Jul 2020 14:02:59 +0300
Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx> 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@xxxxxxxxxx>
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,
