Use guard() and scoped_guard() for handling mutex lock instead of
manually locking and unlocking the mutex. This prevents forgotten
locks due to early exits and remove the need of gotos.
Signed-off-by: Andrew Ijano <andrew.lopes@alumni.usp.br>
Co-developed-by: Gustavo Bastos <gustavobastos@usp.br>
Signed-off-by: Gustavo Bastos <gustavobastos@usp.br>
Suggested-by: Jonathan Cameron <jic23@kernel.org>
---
drivers/iio/accel/sca3000.c | 205 +++++++++++++-----------------------
1 file changed, 73 insertions(+), 132 deletions(-)
diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
index 7145b4541264..058a2d67c91c 100644
--- a/drivers/iio/accel/sca3000.c
+++ b/drivers/iio/accel/sca3000.c
@@ -405,17 +405,14 @@ static int sca3000_print_rev(struct iio_dev *indio_dev)
int ret;
struct sca3000_state *st = iio_priv(indio_dev);
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_REVID_ADDR));
if (ret < 0)
- goto error_ret;
+ return ret;
dev_info(&indio_dev->dev,
"sca3000 revision major=%lu, minor=%lu\n",
ret & SCA3000_REG_REVID_MAJOR_MASK,
ret & SCA3000_REG_REVID_MINOR_MASK);
-error_ret:
- mutex_unlock(&st->lock);
-
return ret;
}
@@ -698,33 +695,27 @@ static int sca3000_read_raw(struct iio_dev *indio_dev,
u8 address;
switch (mask) {
- case IIO_CHAN_INFO_RAW:
- mutex_lock(&st->lock);
+ case IIO_CHAN_INFO_RAW: {
+ guard(mutex)(&st->lock);
if (chan->type == IIO_ACCEL) {
- if (st->mo_det_use_count) {
- mutex_unlock(&st->lock);
+ if (st->mo_det_use_count)
return -EBUSY;
- }
address = sca3000_addresses[chan->address][0];
ret = spi_w8r16be(st->us, SCA3000_READ_REG(address));
- if (ret < 0) {
- mutex_unlock(&st->lock);
+ if (ret < 0)
return ret;
- }
*val = sign_extend32(ret >> chan->scan_type.shift,
chan->scan_type.realbits - 1);
} else {
/* get the temperature when available */
ret = spi_w8r16be(st->us, SCA3000_READ_REG(SCA3000_REG_TEMP_MSB_ADDR));
- if (ret < 0) {
- mutex_unlock(&st->lock);
+ if (ret < 0)
return ret;
- }
*val = (ret >> chan->scan_type.shift) &
GENMASK(chan->scan_type.realbits - 1, 0);
}
- mutex_unlock(&st->lock);
return IIO_VAL_INT;
+ }
case IIO_CHAN_INFO_SCALE:
*val = 0;
if (chan->type == IIO_ACCEL)
@@ -736,16 +727,16 @@ static int sca3000_read_raw(struct iio_dev *indio_dev,
*val = -214;
*val2 = 600000;
return IIO_VAL_INT_PLUS_MICRO;
- case IIO_CHAN_INFO_SAMP_FREQ:
- mutex_lock(&st->lock);
+ case IIO_CHAN_INFO_SAMP_FREQ: {
+ guard(mutex)(&st->lock);
ret = sca3000_read_raw_samp_freq(st, val);
- mutex_unlock(&st->lock);
return ret ? ret : IIO_VAL_INT;
- case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
- mutex_lock(&st->lock);
+ }
+ case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY: {
+ guard(mutex)(&st->lock);
ret = sca3000_read_3db_freq(st, val);
- mutex_unlock(&st->lock);
return ret;
+ }
default:
return -EINVAL;
}
@@ -756,28 +747,23 @@ static int sca3000_write_raw(struct iio_dev *indio_dev,
int val, int val2, long mask)
{
struct sca3000_state *st = iio_priv(indio_dev);
- int ret;
switch (mask) {
- case IIO_CHAN_INFO_SAMP_FREQ:
+ case IIO_CHAN_INFO_SAMP_FREQ: {
+ guard(mutex)(&st->lock);
if (val2)
return -EINVAL;
- mutex_lock(&st->lock);
- ret = sca3000_write_raw_samp_freq(st, val);
- mutex_unlock(&st->lock);
- return ret;
- case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+ return sca3000_write_raw_samp_freq(st, val);
+ }
+ case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY: {
+ guard(mutex)(&st->lock);
if (val2)
return -EINVAL;
- mutex_lock(&st->lock);
- ret = sca3000_write_3db_freq(st, val);
- mutex_unlock(&st->lock);
- return ret;
+ return sca3000_write_3db_freq(st, val);
+ }
default:
return -EINVAL;
}
-
- return ret;
}
/**
@@ -800,9 +786,9 @@ static ssize_t sca3000_read_av_freq(struct device *dev,
unsigned int len = 0;
int ret;
- mutex_lock(&st->lock);
- ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
- mutex_unlock(&st->lock);
+ scoped_guard(mutex, &st->lock) {
+ ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
+ }
if (ret)
return ret;
@@ -850,11 +836,10 @@ static int sca3000_read_event_value(struct iio_dev *indio_dev,
int i;
switch (info) {
- case IIO_EV_INFO_VALUE:
- mutex_lock(&st->lock);
- ret = sca3000_read_ctrl_reg(st,
- sca3000_addresses[chan->address][1]);
- mutex_unlock(&st->lock);
+ case IIO_EV_INFO_VALUE: {
+ scoped_guard(mutex, &st->lock) {
+ ret = sca3000_read_ctrl_reg(st, sca3000_addresses[chan->address][1]);
+ }
if (ret < 0)
return ret;
*val = 0;
@@ -868,6 +853,7 @@ static int sca3000_read_event_value(struct iio_dev *indio_dev,
*val += st->info->mot_det_mult_xz[i];
return IIO_VAL_INT;
+ }
case IIO_EV_INFO_PERIOD:
*val = 0;
*val2 = 226000;
@@ -898,7 +884,6 @@ static int sca3000_write_event_value(struct iio_dev *indio_dev,
int val, int val2)
{
struct sca3000_state *st = iio_priv(indio_dev);
- int ret;
int i;
u8 nonlinear = 0;
@@ -918,13 +903,10 @@ static int sca3000_write_event_value(struct iio_dev *indio_dev,
}
}
- mutex_lock(&st->lock);
- ret = sca3000_write_ctrl_reg(st,
+ guard(mutex)(&st->lock);
+ return sca3000_write_ctrl_reg(st,
sca3000_addresses[chan->address][1],
nonlinear);
- mutex_unlock(&st->lock);
-
- return ret;
}
static struct attribute *sca3000_attributes[] = {
@@ -969,12 +951,12 @@ static void sca3000_ring_int_process(u8 val, struct iio_dev *indio_dev)
struct sca3000_state *st = iio_priv(indio_dev);
int ret, i, num_available;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
if (val & SCA3000_REG_INT_STATUS_HALF) {
ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_BUF_COUNT_ADDR));
if (ret)
- goto error_ret;
+ return;
num_available = ret;
/*
* num_available is the total number of samples available
@@ -982,7 +964,7 @@ static void sca3000_ring_int_process(u8 val, struct iio_dev *indio_dev)
*/
ret = sca3000_read_data(st, SCA3000_REG_RING_OUT_ADDR, num_available * 2);
if (ret)
- goto error_ret;
+ return;
for (i = 0; i < num_available / 3; i++) {
/*
* Dirty hack to cover for 11 bit in fifo, 13 bit
@@ -994,8 +976,6 @@ static void sca3000_ring_int_process(u8 val, struct iio_dev *indio_dev)
iio_push_to_buffers(indio_dev, st->rx + i * 3 * 2);
}
}
-error_ret:
- mutex_unlock(&st->lock);
}
/**
@@ -1021,9 +1001,8 @@ static irqreturn_t sca3000_event_handler(int irq, void *private)
* Could lead if badly timed to an extra read of status reg,
* but ensures no interrupt is missed.
*/
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_STATUS_ADDR));
- mutex_unlock(&st->lock);
if (ret)
goto done;
@@ -1080,16 +1059,15 @@ static int sca3000_read_event_config(struct iio_dev *indio_dev,
struct sca3000_state *st = iio_priv(indio_dev);
int ret;
/* read current value of mode register */
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
if (ret)
- goto error_ret;
+ return ret;
switch (chan->channel2) {
case IIO_MOD_X_AND_Y_AND_Z:
- ret = !!(ret & SCA3000_REG_MODE_FREE_FALL_DETECT);
- break;
+ return !!(ret & SCA3000_REG_MODE_FREE_FALL_DETECT);
case IIO_MOD_X:
case IIO_MOD_Y:
case IIO_MOD_Z:
@@ -1099,24 +1077,18 @@ static int sca3000_read_event_config(struct iio_dev *indio_dev,
*/
if ((ret & SCA3000_REG_MODE_MODE_MASK)
!= SCA3000_REG_MODE_MEAS_MODE_MOT_DET) {
- ret = 0;
+ return 0;
} else {
ret = sca3000_read_ctrl_reg(st,
SCA3000_REG_CTRL_SEL_MD_CTRL);
if (ret < 0)
- goto error_ret;
+ return ret;
/* only supporting logical or's for now */
- ret = !!(ret & sca3000_addresses[chan->address][2]);
+ return !!(ret & sca3000_addresses[chan->address][2]);
}
- break;
default:
- ret = -EINVAL;
+ return -EINVAL;
}
-
-error_ret:
- mutex_unlock(&st->lock);
-
- return ret;
}
static int sca3000_freefall_set_state(struct iio_dev *indio_dev, bool state)
@@ -1217,28 +1189,20 @@ static int sca3000_write_event_config(struct iio_dev *indio_dev,
bool state)
{
struct sca3000_state *st = iio_priv(indio_dev);
- int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
switch (chan->channel2) {
case IIO_MOD_X_AND_Y_AND_Z:
- ret = sca3000_freefall_set_state(indio_dev, state);
- break;
-
+ return sca3000_freefall_set_state(indio_dev, state);
case IIO_MOD_X:
case IIO_MOD_Y:
case IIO_MOD_Z:
- ret = sca3000_motion_detect_set_state(indio_dev,
+ return sca3000_motion_detect_set_state(indio_dev,
chan->address,
state);
- break;
default:
- ret = -EINVAL;
- break;
+ return -EINVAL;
}
- mutex_unlock(&st->lock);
-
- return ret;
}
static inline
@@ -1247,23 +1211,19 @@ int __sca3000_hw_ring_state_set(struct iio_dev *indio_dev, bool state)
struct sca3000_state *st = iio_priv(indio_dev);
int ret;
- mutex_lock(&st->lock);
ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
if (ret)
- goto error_ret;
+ return ret;
+
if (state) {
dev_info(&indio_dev->dev, "supposedly enabling ring buffer\n");
- ret = sca3000_write_reg(st,
+ return sca3000_write_reg(st,
SCA3000_REG_MODE_ADDR,
ret | SCA3000_REG_MODE_RING_BUF_ENABLE);
- } else
- ret = sca3000_write_reg(st,
- SCA3000_REG_MODE_ADDR,
- ret & ~SCA3000_REG_MODE_RING_BUF_ENABLE);
-error_ret:
- mutex_unlock(&st->lock);
-
- return ret;
+ }
+ return sca3000_write_reg(st,
+ SCA3000_REG_MODE_ADDR,
+ ret & ~SCA3000_REG_MODE_RING_BUF_ENABLE);
}
/**
@@ -1280,26 +1240,18 @@ static int sca3000_hw_ring_preenable(struct iio_dev *indio_dev)
int ret;
struct sca3000_state *st = iio_priv(indio_dev);
- mutex_lock(&st->lock);
-
+ guard(mutex)(&st->lock);
/* Enable the 50% full interrupt */
ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
if (ret)
- goto error_unlock;
+ return ret;
ret = sca3000_write_reg(st,
SCA3000_REG_INT_MASK_ADDR,
ret | SCA3000_REG_INT_MASK_RING_HALF);
if (ret)
- goto error_unlock;
-
- mutex_unlock(&st->lock);
+ return ret;
return __sca3000_hw_ring_state_set(indio_dev, 1);
-
-error_unlock:
- mutex_unlock(&st->lock);
-
- return ret;
}
static int sca3000_hw_ring_postdisable(struct iio_dev *indio_dev)
@@ -1307,22 +1259,18 @@ static int sca3000_hw_ring_postdisable(struct iio_dev *indio_dev)
int ret;
struct sca3000_state *st = iio_priv(indio_dev);
+ guard(mutex)(&st->lock);
ret = __sca3000_hw_ring_state_set(indio_dev, 0);
if (ret)
return ret;
/* Disable the 50% full interrupt */
- mutex_lock(&st->lock);
-
ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
if (ret)
- goto unlock;
- ret = sca3000_write_reg(st,
+ return ret;
+ return sca3000_write_reg(st,
SCA3000_REG_INT_MASK_ADDR,
ret & ~SCA3000_REG_INT_MASK_RING_HALF);
-unlock:
- mutex_unlock(&st->lock);
- return ret;
}
static const struct iio_buffer_setup_ops sca3000_ring_setup_ops = {
@@ -1342,25 +1290,25 @@ static int sca3000_clean_setup(struct sca3000_state *st)
{
int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
/* Ensure all interrupts have been acknowledged */
ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
if (ret)
- goto error_ret;
+ return ret;
/* Turn off all motion detection channels */
ret = sca3000_read_ctrl_reg(st, SCA3000_REG_CTRL_SEL_MD_CTRL);
if (ret < 0)
- goto error_ret;
+ return ret;
ret = sca3000_write_ctrl_reg(st, SCA3000_REG_CTRL_SEL_MD_CTRL,
ret & SCA3000_MD_CTRL_PROT_MASK);
if (ret)
- goto error_ret;
+ return ret;
/* Disable ring buffer */
ret = sca3000_read_ctrl_reg(st, SCA3000_REG_CTRL_SEL_OUT_CTRL);
if (ret < 0)
- goto error_ret;
+ return ret;
ret = sca3000_write_ctrl_reg(st, SCA3000_REG_CTRL_SEL_OUT_CTRL,
(ret & SCA3000_REG_OUT_CTRL_PROT_MASK)
| SCA3000_REG_OUT_CTRL_BUF_X_EN
@@ -1368,17 +1316,17 @@ static int sca3000_clean_setup(struct sca3000_state *st)
| SCA3000_REG_OUT_CTRL_BUF_Z_EN
| SCA3000_REG_OUT_CTRL_BUF_DIV_4);
if (ret)
- goto error_ret;
+ return ret;
/* Enable interrupts, relevant to mode and set up as active low */
ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
if (ret)
- goto error_ret;
+ return ret;
ret = sca3000_write_reg(st,
SCA3000_REG_INT_MASK_ADDR,
(ret & SCA3000_REG_INT_MASK_PROT_MASK)
| SCA3000_REG_INT_MASK_ACTIVE_LOW);
if (ret)
- goto error_ret;
+ return ret;
/*
* Select normal measurement mode, free fall off, ring off
* Ring in 12 bit mode - it is fine to overwrite reserved bits 3,5
@@ -1386,13 +1334,9 @@ static int sca3000_clean_setup(struct sca3000_state *st)
*/
ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
if (ret)
- goto error_ret;
- ret = sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
+ return ret;
+ return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
ret & SCA3000_MODE_PROT_MASK);
-
-error_ret:
- mutex_unlock(&st->lock);
- return ret;
}
static const struct iio_info sca3000_info = {
@@ -1470,19 +1414,16 @@ static int sca3000_stop_all_interrupts(struct sca3000_state *st)
{
int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
if (ret)
- goto error_ret;
+ return ret;
- ret = sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
+ return sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
ret &
~(SCA3000_REG_INT_MASK_RING_THREE_QUARTER |
SCA3000_REG_INT_MASK_RING_HALF |
SCA3000_REG_INT_MASK_ALL_INTS));
-error_ret:
- mutex_unlock(&st->lock);
- return ret;
}
static void sca3000_remove(struct spi_device *spi)
--
2.49.0On Wed, 18 Jun 2025 00:12:18 -0300
Andrew Ijano <andrew.ijano@gmail.com> wrote:
> Use guard() and scoped_guard() for handling mutex lock instead of
> manually locking and unlocking the mutex. This prevents forgotten
> locks due to early exits and remove the need of gotos.
Please add extra description for where you have pushed locks up
a level in the call tree and why that is fine to do.
>
> Signed-off-by: Andrew Ijano <andrew.lopes@alumni.usp.br>
> Co-developed-by: Gustavo Bastos <gustavobastos@usp.br>
> Signed-off-by: Gustavo Bastos <gustavobastos@usp.br>
> Suggested-by: Jonathan Cameron <jic23@kernel.org>
A few comments inline.
Thanks
Jonathan
> @@ -756,28 +747,23 @@ static int sca3000_write_raw(struct iio_dev *indio_dev,
> int val, int val2, long mask)
> {
> struct sca3000_state *st = iio_priv(indio_dev);
> - int ret;
>
> switch (mask) {
> - case IIO_CHAN_INFO_SAMP_FREQ:
> + case IIO_CHAN_INFO_SAMP_FREQ: {
> + guard(mutex)(&st->lock);
As below
> if (val2)
> return -EINVAL;
> - mutex_lock(&st->lock);
> - ret = sca3000_write_raw_samp_freq(st, val);
> - mutex_unlock(&st->lock);
> - return ret;
> - case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> + return sca3000_write_raw_samp_freq(st, val);
> + }
> + case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY: {
> + guard(mutex)(&st->lock);
> if (val2)
> return -EINVAL;
> - mutex_lock(&st->lock);
You can keep the old ordering here. It doesn't matter much but
easier to be sure about a patch that makes no functional change.
> - ret = sca3000_write_3db_freq(st, val);
> - mutex_unlock(&st->lock);
> - return ret;
> + return sca3000_write_3db_freq(st, val);
> + }
>
> /**
> @@ -1021,9 +1001,8 @@ static irqreturn_t sca3000_event_handler(int irq, void *private)
> * Could lead if badly timed to an extra read of status reg,
> * but ensures no interrupt is missed.
> */
> - mutex_lock(&st->lock);
> + guard(mutex)(&st->lock);
This is a very large increase in scope. Use scoped_guard() here instead to avoid
holding the lock over a whole load of code that doesn't need it.
> ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_STATUS_ADDR));
> - mutex_unlock(&st->lock);
> if (ret)
> goto done;
>
> }
>
> static inline
> @@ -1247,23 +1211,19 @@ int __sca3000_hw_ring_state_set(struct iio_dev *indio_dev, bool state)
> struct sca3000_state *st = iio_priv(indio_dev);
> int ret;
>
> - mutex_lock(&st->lock);
> ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
> if (ret)
> - goto error_ret;
> + return ret;
> +
> if (state) {
> dev_info(&indio_dev->dev, "supposedly enabling ring buffer\n");
> - ret = sca3000_write_reg(st,
> + return sca3000_write_reg(st,
> SCA3000_REG_MODE_ADDR,
> ret | SCA3000_REG_MODE_RING_BUF_ENABLE);
This indent was already nasty so as we are touching the code good to clean it up.
For cases like this we can be more relaxed and if it helps readability go a little
over 80 chars (I think this will be 80 ish)
return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
ret | SCA3000_REG_MODE_RING_BUF_ENABLE);
> - } else
> - ret = sca3000_write_reg(st,
> - SCA3000_REG_MODE_ADDR,
> - ret & ~SCA3000_REG_MODE_RING_BUF_ENABLE);
> -error_ret:
> - mutex_unlock(&st->lock);
> -
> - return ret;
> + }
> + return sca3000_write_reg(st,
> + SCA3000_REG_MODE_ADDR,
> + ret & ~SCA3000_REG_MODE_RING_BUF_ENABLE);
> }
>
>
> static int sca3000_hw_ring_postdisable(struct iio_dev *indio_dev)
> @@ -1307,22 +1259,18 @@ static int sca3000_hw_ring_postdisable(struct iio_dev *indio_dev)
> int ret;
> struct sca3000_state *st = iio_priv(indio_dev);
>
> + guard(mutex)(&st->lock);
See comment at the top - Making this change is fine but call it out in the patch
description as it isn't simple change to using guards, but instead to holding
the lock for longer. Change is fine but point it out as it needs
more review than the mechanical changes.
> ret = __sca3000_hw_ring_state_set(indio_dev, 0);
> if (ret)
> return ret;
>
> /* Disable the 50% full interrupt */
> - mutex_lock(&st->lock);
> -
> ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
> if (ret)
> - goto unlock;
> - ret = sca3000_write_reg(st,
> + return ret;
> + return sca3000_write_reg(st,
> SCA3000_REG_INT_MASK_ADDR,
> ret & ~SCA3000_REG_INT_MASK_RING_HALF);
As below.
> -unlock:
> - mutex_unlock(&st->lock);
> - return ret;
> }
> @@ -1386,13 +1334,9 @@ static int sca3000_clean_setup(struct sca3000_state *st)
> */
> ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
> if (ret)
> - goto error_ret;
> - ret = sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
> + return ret;
> + return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
> ret & SCA3000_MODE_PROT_MASK);
As below.
> -
> -error_ret:
> - mutex_unlock(&st->lock);
> - return ret;
> }
>
> static const struct iio_info sca3000_info = {
> @@ -1470,19 +1414,16 @@ static int sca3000_stop_all_interrupts(struct sca3000_state *st)
> {
> int ret;
>
> - mutex_lock(&st->lock);
> + guard(mutex)(&st->lock);
> ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
> if (ret)
> - goto error_ret;
> + return ret;
>
> - ret = sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
> + return sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
This adds a character to this line which means that either the indent of
the following lines was previously wrong, or it is now.
I think you need to add a space to the following lines.
Check for other similar cases.
> ret &
> ~(SCA3000_REG_INT_MASK_RING_THREE_QUARTER |
> SCA3000_REG_INT_MASK_RING_HALF |
> SCA3000_REG_INT_MASK_ALL_INTS));
On Sat, Jun 21, 2025 at 2:55 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> Please add extra description for where you have pushed locks up
> a level in the call tree and why that is fine to do.
>
Ok! I'll do that.
> > switch (mask) {
> > - case IIO_CHAN_INFO_SAMP_FREQ:
> > + case IIO_CHAN_INFO_SAMP_FREQ: {
> > + guard(mutex)(&st->lock);
> As below
>
> > if (val2)
> > return -EINVAL;
> > - mutex_lock(&st->lock);
> > - ret = sca3000_write_raw_samp_freq(st, val);
> > - mutex_unlock(&st->lock);
> > - return ret;
> > - case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> > + return sca3000_write_raw_samp_freq(st, val);
> > + }
> > + case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY: {
> > + guard(mutex)(&st->lock);
> > if (val2)
> > return -EINVAL;
> > - mutex_lock(&st->lock);
>
> You can keep the old ordering here. It doesn't matter much but
> easier to be sure about a patch that makes no functional change.
Ok!
> > - mutex_lock(&st->lock);
> > + guard(mutex)(&st->lock);
>
> This is a very large increase in scope. Use scoped_guard() here instead to avoid
> holding the lock over a whole load of code that doesn't need it.
>
> > ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_STATUS_ADDR));
> > - mutex_unlock(&st->lock);
That makes sense! I don't know why I didn't use scoped_guard() in this
case before.
> > if (state) {
> > dev_info(&indio_dev->dev, "supposedly enabling ring buffer\n");
> > - ret = sca3000_write_reg(st,
> > + return sca3000_write_reg(st,
> > SCA3000_REG_MODE_ADDR,
> > ret | SCA3000_REG_MODE_RING_BUF_ENABLE);
>
> This indent was already nasty so as we are touching the code good to clean it up.
> For cases like this we can be more relaxed and if it helps readability go a little
> over 80 chars (I think this will be 80 ish)
Great! I'll pay attention to that.
> >
> >
> > static int sca3000_hw_ring_postdisable(struct iio_dev *indio_dev)
> > @@ -1307,22 +1259,18 @@ static int sca3000_hw_ring_postdisable(struct iio_dev *indio_dev)
> > int ret;
> > struct sca3000_state *st = iio_priv(indio_dev);
> >
> > + guard(mutex)(&st->lock);
>
> See comment at the top - Making this change is fine but call it out in the patch
> description as it isn't simple change to using guards, but instead to holding
> the lock for longer. Change is fine but point it out as it needs
> more review than the mechanical changes.
Ok!
>
> > ret = __sca3000_hw_ring_state_set(indio_dev, 0);
> > if (ret)
> > return ret;
> >
> > /* Disable the 50% full interrupt */
> > - mutex_lock(&st->lock);
> > -
> > ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
> > if (ret)
> > - goto unlock;
> > - ret = sca3000_write_reg(st,
> > + return ret;
> > + return sca3000_write_reg(st,
> > SCA3000_REG_INT_MASK_ADDR,
> > ret & ~SCA3000_REG_INT_MASK_RING_HALF);
>
> As below.
>
> > -unlock:
> > - mutex_unlock(&st->lock);
> > - return ret;
> > }
>
> > @@ -1386,13 +1334,9 @@ static int sca3000_clean_setup(struct sca3000_state *st)
> > */
> > ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
> > if (ret)
> > - goto error_ret;
> > - ret = sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
> > + return ret;
> > + return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
> > ret & SCA3000_MODE_PROT_MASK);
>
> As below.
>
> > -
> > -error_ret:
> > - mutex_unlock(&st->lock);
> > - return ret;
> > }
> >
> > static const struct iio_info sca3000_info = {
> > @@ -1470,19 +1414,16 @@ static int sca3000_stop_all_interrupts(struct sca3000_state *st)
> > {
> > int ret;
> >
> > - mutex_lock(&st->lock);
> > + guard(mutex)(&st->lock);
> > ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
> > if (ret)
> > - goto error_ret;
> > + return ret;
> >
> > - ret = sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
> > + return sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
> This adds a character to this line which means that either the indent of
> the following lines was previously wrong, or it is now.
> I think you need to add a space to the following lines.
>
> Check for other similar cases.
>
> > ret &
> > ~(SCA3000_REG_INT_MASK_RING_THREE_QUARTER |
> > SCA3000_REG_INT_MASK_RING_HALF |
> > SCA3000_REG_INT_MASK_ALL_INTS));
Hm, correct me if I'm mistaken but I couldn't find this extra
character, I used the same number of tabs in both cases. Even in this
email it shows as having the same number of white spaces.
Thanks,
Andrew
> > > - ret = sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR, > > > + return sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR, > > This adds a character to this line which means that either the indent of > > the following lines was previously wrong, or it is now. > > I think you need to add a space to the following lines. > > > > Check for other similar cases. > > > > > ret & > > > ~(SCA3000_REG_INT_MASK_RING_THREE_QUARTER | > > > SCA3000_REG_INT_MASK_RING_HALF | > > > SCA3000_REG_INT_MASK_ALL_INTS)); > > Hm, correct me if I'm mistaken but I couldn't find this extra > character, I used the same number of tabs in both cases. Even in this > email it shows as having the same number of white spaces. return sca3000_write_reg( is one character longer than ret = sca3000_write_reg( and seeing as parameters on later lines should align just after this they all need one additional space. > > Thanks, > Andrew
© 2016 - 2025 Red Hat, Inc.