Replace manual mutex_lock/mutex_unlock pair with guard(mutex) in
sca3000_print_rev(), sca3000_ring_int_process(),
sca3000_read_event_config(), __sca3000_hw_ring_state_set(),
sca3000_hw_ring_preenable(), sca3000_hw_ring_postdisable(),
sca3000_clean_setup(), sca3000_stop_all_interrupts().
This ensures the mutex is released on all return paths and
allows returning directly without a goto label.
Signed-off-by: Rajveer Chaudhari <rajveer.chaudhari.linux@gmail.com>
---
v2: Dropped Header alignment change
---
drivers/iio/accel/sca3000.c | 97 ++++++++++++++++---------------------
1 file changed, 42 insertions(+), 55 deletions(-)
diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
index 4a827be439a2..8a06602ab41c 100644
--- a/drivers/iio/accel/sca3000.c
+++ b/drivers/iio/accel/sca3000.c
@@ -9,6 +9,7 @@
#include <linux/interrupt.h>
#include <linux/fs.h>
+#include <linux/cleanup.h>
#include <linux/device.h>
#include <linux/slab.h>
#include <linux/kernel.h>
@@ -424,16 +425,16 @@ 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 = sca3000_read_data_short(st, SCA3000_REG_REVID_ADDR, 1);
if (ret < 0)
- goto error_ret;
+ return ret;
+
dev_info(&indio_dev->dev,
"sca3000 revision major=%lu, minor=%lu\n",
st->rx[0] & SCA3000_REG_REVID_MAJOR_MASK,
st->rx[0] & SCA3000_REG_REVID_MINOR_MASK);
-error_ret:
- mutex_unlock(&st->lock);
return ret;
}
@@ -996,13 +997,14 @@ 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 = sca3000_read_data_short(st, SCA3000_REG_BUF_COUNT_ADDR,
1);
if (ret)
- goto error_ret;
+ return;
+
num_available = st->rx[0];
/*
* num_available is the total number of samples available
@@ -1011,7 +1013,8 @@ static void sca3000_ring_int_process(u8 val, struct iio_dev *indio_dev)
ret = sca3000_read_data(st, SCA3000_REG_RING_OUT_ADDR, st->rx,
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
@@ -1023,8 +1026,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);
}
/**
@@ -1110,11 +1111,11 @@ 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 = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
if (ret)
- goto error_ret;
+ return ret;
switch (chan->channel2) {
case IIO_MOD_X_AND_Y_AND_Z:
@@ -1134,7 +1135,7 @@ static int sca3000_read_event_config(struct iio_dev *indio_dev,
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]);
}
@@ -1143,9 +1144,6 @@ static int sca3000_read_event_config(struct iio_dev *indio_dev,
ret = -EINVAL;
}
-error_ret:
- mutex_unlock(&st->lock);
-
return ret;
}
@@ -1277,10 +1275,12 @@ 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);
+ guard(mutex)(&st->lock);
+
ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
if (ret)
- goto error_ret;
+ return ret;
+
if (state) {
dev_info(&indio_dev->dev, "supposedly enabling ring buffer\n");
ret = sca3000_write_reg(st,
@@ -1290,8 +1290,6 @@ int __sca3000_hw_ring_state_set(struct iio_dev *indio_dev, bool state)
ret = sca3000_write_reg(st,
SCA3000_REG_MODE_ADDR,
(st->rx[0] & ~SCA3000_REG_MODE_RING_BUF_ENABLE));
-error_ret:
- mutex_unlock(&st->lock);
return ret;
}
@@ -1310,26 +1308,20 @@ 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 = sca3000_read_data_short(st, SCA3000_REG_INT_MASK_ADDR, 1);
if (ret)
- goto error_unlock;
+ return ret;
+
ret = sca3000_write_reg(st,
SCA3000_REG_INT_MASK_ADDR,
st->rx[0] | 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)
@@ -1342,17 +1334,15 @@ static int sca3000_hw_ring_postdisable(struct iio_dev *indio_dev)
return ret;
/* Disable the 50% full interrupt */
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
ret = sca3000_read_data_short(st, SCA3000_REG_INT_MASK_ADDR, 1);
if (ret)
- goto unlock;
- ret = sca3000_write_reg(st,
+ return ret;
+
+ return sca3000_write_reg(st,
SCA3000_REG_INT_MASK_ADDR,
st->rx[0] & ~SCA3000_REG_INT_MASK_RING_HALF);
-unlock:
- mutex_unlock(&st->lock);
- return ret;
}
static const struct iio_buffer_setup_ops sca3000_ring_setup_ops = {
@@ -1372,25 +1362,26 @@ 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 = sca3000_read_data_short(st, SCA3000_REG_INT_STATUS_ADDR, 1);
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
@@ -1398,17 +1389,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 = sca3000_read_data_short(st, SCA3000_REG_INT_MASK_ADDR, 1);
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
@@ -1416,13 +1407,10 @@ static int sca3000_clean_setup(struct sca3000_state *st)
*/
ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
if (ret)
- goto error_ret;
- ret = sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
- (st->rx[0] & SCA3000_MODE_PROT_MASK));
+ return ret;
-error_ret:
- mutex_unlock(&st->lock);
- return ret;
+ return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
+ (st->rx[0] & SCA3000_MODE_PROT_MASK));
}
static const struct iio_info sca3000_info = {
@@ -1504,18 +1492,17 @@ static int sca3000_stop_all_interrupts(struct sca3000_state *st)
{
int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
+
ret = sca3000_read_data_short(st, SCA3000_REG_INT_MASK_ADDR, 1);
if (ret)
- goto error_ret;
- ret = sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
+ return ret;
+
+ return sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
(st->rx[0] &
~(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.53.0On Mon, 9 Mar 2026 21:04:08 +0530
Rajveer Chaudhari <rajveer.chaudhari.linux@gmail.com> wrote:
> Replace manual mutex_lock/mutex_unlock pair with guard(mutex) in
> sca3000_print_rev(), sca3000_ring_int_process(),
> sca3000_read_event_config(), __sca3000_hw_ring_state_set(),
> sca3000_hw_ring_preenable(), sca3000_hw_ring_postdisable(),
> sca3000_clean_setup(), sca3000_stop_all_interrupts().
> This ensures the mutex is released on all return paths and
> allows returning directly without a goto label.
>
> Signed-off-by: Rajveer Chaudhari <rajveer.chaudhari.linux@gmail.com>
Hi Rajveer,
This one isn't so simple. May need a couple of steps to get to the
code we want to end up with that takes full advantage of guard()
> ---
> v2: Dropped Header alignment change
> ---
> drivers/iio/accel/sca3000.c | 97 ++++++++++++++++---------------------
> 1 file changed, 42 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
> index 4a827be439a2..8a06602ab41c 100644
> --- a/drivers/iio/accel/sca3000.c
> +++ b/drivers/iio/accel/sca3000.c
> @@ -9,6 +9,7 @@
>
> #include <linux/interrupt.h>
> #include <linux/fs.h>
> +#include <linux/cleanup.h>
> #include <linux/device.h>
> #include <linux/slab.h>
> #include <linux/kernel.h>
> @@ -424,16 +425,16 @@ 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 = sca3000_read_data_short(st, SCA3000_REG_REVID_ADDR, 1);
> if (ret < 0)
> - goto error_ret;
> + return ret;
> +
> dev_info(&indio_dev->dev,
> "sca3000 revision major=%lu, minor=%lu\n",
> st->rx[0] & SCA3000_REG_REVID_MAJOR_MASK,
> st->rx[0] & SCA3000_REG_REVID_MINOR_MASK);
> -error_ret:
> - mutex_unlock(&st->lock);
>
> return ret;
return 0;
To make it clear that if we get here it is a good path.
> }
> @@ -996,13 +997,14 @@ 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) {
Hmm. This code is unnecessarily complex - Probably written by a younger more foolish me ;)
So I think we really want to head towards something like the following.
Probably break this up though. guard() patch first, then flip the logic in
a second patch.
Val is a parameter so the test can be outside the lock. Also, we can just
exit early if the bit isn't set, reducing the indent considerably.
if (!(val & SCA3000_REG_INT_STATUS_HALF))
return;
guard(mutex)(&st->lock);
ret = sca3000_read_data_short(st, SCA3000_REG_BUF_COUNT_ADDR, 1);
if (ret)
return;
num_available = st->rx[0];
/*
* num_available is the total number of samples available
* i.e. number of time points * number of channels.
*/
ret = sca3000_read_data(st, SCA3000_REG_RING_OUT_ADDR, st->rx,
num_available * 2);
if (ret)
return;
for (i = 0; i < num_available / 3; i++) {
/*
* Dirty hack to cover for 11 bit in fifo, 13 bit direct reading.
*
* In theory the bottom two bits are undefined. In reality they
* appear to always be 0.
*/
iio_push_to_buffers(indio_dev, st->rx + i * 3 * 2);
}
> ret = sca3000_read_data_short(st, SCA3000_REG_BUF_COUNT_ADDR,
> 1);
> if (ret)
> - goto error_ret;
> + return;
> +
> num_available = st->rx[0];
> /*
> * num_available is the total number of samples available
> @@ -1011,7 +1013,8 @@ static void sca3000_ring_int_process(u8 val, struct iio_dev *indio_dev)
> ret = sca3000_read_data(st, SCA3000_REG_RING_OUT_ADDR, st->rx,
> 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
> @@ -1023,8 +1026,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);
> }
>
> /**
> @@ -1110,11 +1111,11 @@ 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 = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
> if (ret)
> - goto error_ret;
> + return ret;
>
> switch (chan->channel2) {
> case IIO_MOD_X_AND_Y_AND_Z:
> @@ -1134,7 +1135,7 @@ static int sca3000_read_event_config(struct iio_dev *indio_dev,
With a return where there is currently a ret = 0; just above here you can drop
the else and reduce the indent of this code.
> 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]);
Can return here too. It's a 'good' path but that's fine.
There are a couple above here as well that aren't in the context.
> }
> @@ -1143,9 +1144,6 @@ static int sca3000_read_event_config(struct iio_dev *indio_dev,
> ret = -EINVAL;
return there.
> }
>
> -error_ret:
> - mutex_unlock(&st->lock);
> -
And ultimately there should be no way of getting here, so this can go.
> return ret;
> }
>
> @@ -1277,10 +1275,12 @@ 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);
> + guard(mutex)(&st->lock);
> +
> ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
> 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 in both these legs. No point in having the reader have to look down
to see if anything else happens if we just end up with return ret;
> @@ -1290,8 +1290,6 @@ int __sca3000_hw_ring_state_set(struct iio_dev *indio_dev, bool state)
> ret = sca3000_write_reg(st,
> SCA3000_REG_MODE_ADDR,
> (st->rx[0] & ~SCA3000_REG_MODE_RING_BUF_ENABLE));
> -error_ret:
> - mutex_unlock(&st->lock);
>
> return ret;
> }
> static int sca3000_hw_ring_postdisable(struct iio_dev *indio_dev)
> @@ -1342,17 +1334,15 @@ static int sca3000_hw_ring_postdisable(struct iio_dev *indio_dev)
> return ret;
>
> /* Disable the 50% full interrupt */
> - mutex_lock(&st->lock);
> + guard(mutex)(&st->lock);
>
> ret = sca3000_read_data_short(st, SCA3000_REG_INT_MASK_ADDR, 1);
> if (ret)
> - goto unlock;
> - ret = sca3000_write_reg(st,
> + return ret;
> +
> + return sca3000_write_reg(st,
> SCA3000_REG_INT_MASK_ADDR,
Check indentation. I suspect this is all 1 space too little now.
> st->rx[0] & ~SCA3000_REG_INT_MASK_RING_HALF);
> -unlock:
> - mutex_unlock(&st->lock);
> - return ret;
> }
>
> static const struct iio_info sca3000_info = {
> @@ -1504,18 +1492,17 @@ static int sca3000_stop_all_interrupts(struct sca3000_state *st)
> {
> int ret;
>
> - mutex_lock(&st->lock);
> + guard(mutex)(&st->lock);
> +
> ret = sca3000_read_data_short(st, SCA3000_REG_INT_MASK_ADDR, 1);
> if (ret)
> - goto error_ret;
> - ret = sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
> + return ret;
> +
> + return sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
> (st->rx[0] &
As above. Indentation needs an update.
> ~(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)
Hello Jonathan, Thanks for the review, I will add all of your suggested return path, indentation fixes in v3. But I have a question, should I submit the next version of patch right now as you reviewed the whole patch, or wait for 24 hrs as instructed by Andy Shevchenko to let others have a chance to comment, review. On Tue, Mar 10, 2026 at 1:00 AM Jonathan Cameron <jic23@kernel.org> wrote: > > So I think we really want to head towards something like the following. > Probably break this up though. guard() patch first, then flip the logic in > a second patch. Yes, I will prepare and submit a separate patch for this logic flip. Thankyou, Rajveer
On 3/9/26 2:58 PM, Rajveer Chaudhari wrote: > Hello Jonathan, > > Thanks for the review, I will add all of your suggested return path, > indentation fixes in v3. > > But I have a question, should I submit the next version of patch right > now as you reviewed > the whole patch, or wait for 24 hrs as instructed by Andy Shevchenko > to let others > have a chance to comment, review. Waiting even longer than 24 hours is better. Not everyone has time to review every day. Ideally, wait 1 week between revisions. If it is something urgent, it can be sooner. Converting things to guard() is certainly not urgent.
On Tue, Mar 10, 2026 at 1:33 AM David Lechner <dlechner@baylibre.com> wrote: > > Waiting even longer than 24 hours is better. Not everyone has time to > review every day. Ideally, wait 1 week between revisions. If it is something > urgent, it can be sooner. Converting things to guard() is certainly not > urgent. Yes, understood. Thank you
© 2016 - 2026 Red Hat, Inc.