| Message ID | 20200706110259.23947-3-alexandru.ardelean@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] iio: dac: ad5592r: fix unbalanced mutex unlocks in ad5592r_read_raw() | expand |
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,
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,
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(-)