* [PATCH 2/3] iio: dac: ad5592r: un-indent code-block for scale read
2020-07-06 11:02 [PATCH 1/3] iio: dac: ad5592r: fix unbalanced mutex unlocks in ad5592r_read_raw() Alexandru Ardelean
@ 2020-07-06 11:02 ` Alexandru Ardelean
2020-07-06 11:02 ` [PATCH 3/3] iio: dac: ad5592r: localize locks only where needed in ad5592r_read_raw() Alexandru Ardelean
2020-07-06 16:36 ` [PATCH 1/3] iio: dac: ad5592r: fix unbalanced mutex unlocks " Ardelean, Alexandru
2 siblings, 0 replies; 6+ messages in thread
From: Alexandru Ardelean @ 2020-07-06 11:02 UTC (permalink / raw)
To: linux-iio, linux-kernel; +Cc: jic23, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 3/3] iio: dac: ad5592r: localize locks only where needed in ad5592r_read_raw()
2020-07-06 11:02 [PATCH 1/3] iio: dac: ad5592r: fix unbalanced mutex unlocks in ad5592r_read_raw() Alexandru Ardelean
2020-07-06 11:02 ` [PATCH 2/3] iio: dac: ad5592r: un-indent code-block for scale read Alexandru Ardelean
@ 2020-07-06 11:02 ` Alexandru Ardelean
2020-09-17 18:42 ` Jonathan Cameron
2020-07-06 16:36 ` [PATCH 1/3] iio: dac: ad5592r: fix unbalanced mutex unlocks " Ardelean, Alexandru
2 siblings, 1 reply; 6+ messages in thread
From: Alexandru Ardelean @ 2020-07-06 11:02 UTC (permalink / raw)
To: linux-iio, linux-kernel; +Cc: jic23, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 3/3] iio: dac: ad5592r: localize locks only where needed in ad5592r_read_raw()
2020-07-06 11:02 ` [PATCH 3/3] iio: dac: ad5592r: localize locks only where needed in ad5592r_read_raw() Alexandru Ardelean
@ 2020-09-17 18:42 ` Jonathan Cameron
0 siblings, 0 replies; 6+ messages in thread
From: Jonathan Cameron @ 2020-09-17 18:42 UTC (permalink / raw)
To: Alexandru Ardelean; +Cc: linux-iio, linux-kernel
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,
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] iio: dac: ad5592r: fix unbalanced mutex unlocks in ad5592r_read_raw()
2020-07-06 11:02 [PATCH 1/3] iio: dac: ad5592r: fix unbalanced mutex unlocks in ad5592r_read_raw() Alexandru Ardelean
2020-07-06 11:02 ` [PATCH 2/3] iio: dac: ad5592r: un-indent code-block for scale read Alexandru Ardelean
2020-07-06 11:02 ` [PATCH 3/3] iio: dac: ad5592r: localize locks only where needed in ad5592r_read_raw() Alexandru Ardelean
@ 2020-07-06 16:36 ` Ardelean, Alexandru
2020-07-12 11:16 ` Jonathan Cameron
2 siblings, 1 reply; 6+ messages in thread
From: Ardelean, Alexandru @ 2020-07-06 16:36 UTC (permalink / raw)
To: linux-kernel, linux-iio; +Cc: jic23, charles.stanhope
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:
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/3] iio: dac: ad5592r: fix unbalanced mutex unlocks in ad5592r_read_raw()
2020-07-06 16:36 ` [PATCH 1/3] iio: dac: ad5592r: fix unbalanced mutex unlocks " Ardelean, Alexandru
@ 2020-07-12 11:16 ` Jonathan Cameron
0 siblings, 0 replies; 6+ messages in thread
From: Jonathan Cameron @ 2020-07-12 11:16 UTC (permalink / raw)
To: Ardelean, Alexandru; +Cc: linux-kernel, linux-iio, charles.stanhope
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:
^ permalink raw reply [flat|nested] 6+ messages in thread