From: Sanjay Chitroda <sanjayembeddedse@gmail.com>
Replace explicit mutex_lock() and mutex_unlock() with the guard() macro
for cleaner and safer mutex handling.
Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
---
drivers/iio/common/ssp_sensors/ssp_spi.c | 54 ++++++++++--------------
1 file changed, 22 insertions(+), 32 deletions(-)
diff --git a/drivers/iio/common/ssp_sensors/ssp_spi.c b/drivers/iio/common/ssp_sensors/ssp_spi.c
index 6c81c0385fb5..87a38002b218 100644
--- a/drivers/iio/common/ssp_sensors/ssp_spi.c
+++ b/drivers/iio/common/ssp_sensors/ssp_spi.c
@@ -2,6 +2,7 @@
/*
* Copyright (C) 2014, Samsung Electronics Co. Ltd. All Rights Reserved.
*/
+#include <linux/cleanup.h>
#include "ssp.h"
@@ -189,55 +190,49 @@ static int ssp_do_transfer(struct ssp_data *data, struct ssp_msg *msg,
msg->done = done;
- mutex_lock(&data->comm_lock);
+ guard(mutex)(&data->comm_lock);
status = ssp_check_lines(data, false);
- if (status < 0)
- goto _error_locked;
+ if (status < 0) {
+ data->timeout_cnt++;
+ return status;
+ }
status = spi_write(data->spi, msg->buffer, SSP_HEADER_SIZE);
if (status < 0) {
gpiod_set_value_cansleep(data->ap_mcu_gpiod, 1);
dev_err(SSP_DEV, "%s spi_write fail\n", __func__);
- goto _error_locked;
+ data->timeout_cnt++;
+ return status;
}
if (!use_no_irq) {
- mutex_lock(&data->pending_lock);
- list_add_tail(&msg->list, &data->pending_list);
- mutex_unlock(&data->pending_lock);
+ scoped_guard(mutex, &data->pending_lock)
+ list_add_tail(&msg->list, &data->pending_list);
}
status = ssp_check_lines(data, true);
if (status < 0) {
if (!use_no_irq) {
- mutex_lock(&data->pending_lock);
- list_del(&msg->list);
- mutex_unlock(&data->pending_lock);
+ scoped_guard(mutex, &data->pending_lock)
+ list_add_tail(&msg->list, &data->pending_list);
}
- goto _error_locked;
+ data->timeout_cnt++;
+ return status;
}
- mutex_unlock(&data->comm_lock);
-
if (!use_no_irq && done)
if (wait_for_completion_timeout(done,
msecs_to_jiffies(timeout)) ==
0) {
- mutex_lock(&data->pending_lock);
- list_del(&msg->list);
- mutex_unlock(&data->pending_lock);
+ scoped_guard(mutex, &data->pending_lock)
+ list_add_tail(&msg->list, &data->pending_list);
data->timeout_cnt++;
return -ETIMEDOUT;
}
return 0;
-
-_error_locked:
- mutex_unlock(&data->comm_lock);
- data->timeout_cnt++;
- return status;
}
static inline int ssp_spi_sync_command(struct ssp_data *data,
@@ -360,7 +355,7 @@ int ssp_irq_msg(struct ssp_data *data)
* this is a small list, a few elements - the packets can be
* received with no order
*/
- mutex_lock(&data->pending_lock);
+ guard(mutex)(&data->pending_lock);
list_for_each_entry_safe(iter, n, &data->pending_list, list) {
if (iter->options == msg_options) {
list_del(&iter->list);
@@ -376,10 +371,8 @@ int ssp_irq_msg(struct ssp_data *data)
* check but let's handle this
*/
buffer = kmalloc(length, GFP_KERNEL | GFP_DMA);
- if (!buffer) {
- ret = -ENOMEM;
- goto _unlock;
- }
+ if (!buffer)
+ return -ENOMEM;
/* got dead packet so it is always an error */
ret = spi_read(data->spi, buffer, length);
@@ -391,7 +384,7 @@ int ssp_irq_msg(struct ssp_data *data)
dev_err(SSP_DEV, "No match error %x\n",
msg_options);
- goto _unlock;
+ return ret;
}
if (msg_type == SSP_AP2HUB_READ)
@@ -409,15 +402,13 @@ int ssp_irq_msg(struct ssp_data *data)
msg->length = 1;
list_add_tail(&msg->list, &data->pending_list);
- goto _unlock;
+ return ret;
}
}
if (msg->done)
if (!completion_done(msg->done))
complete(msg->done);
-_unlock:
- mutex_unlock(&data->pending_lock);
break;
case SSP_HUB2AP_WRITE:
buffer = kzalloc(length, GFP_KERNEL | GFP_DMA);
@@ -448,7 +439,7 @@ void ssp_clean_pending_list(struct ssp_data *data)
{
struct ssp_msg *msg, *n;
- mutex_lock(&data->pending_lock);
+ guard(mutex)(&data->pending_lock);
list_for_each_entry_safe(msg, n, &data->pending_list, list) {
list_del(&msg->list);
@@ -456,7 +447,6 @@ void ssp_clean_pending_list(struct ssp_data *data)
if (!completion_done(msg->done))
complete(msg->done);
}
- mutex_unlock(&data->pending_lock);
}
int ssp_command(struct ssp_data *data, char command, int arg)
--
2.34.1On Sun, Mar 15, 2026 at 06:25:07PM +0530, Sanjay Chitroda wrote:
> Replace explicit mutex_lock() and mutex_unlock() with the guard() macro
> for cleaner and safer mutex handling.
TBH I don't see much benefit in this form. What I am thinking of is to refactor
to have the guard and timeout_cnt++ in the top level, and
static void ...(flag)
{
if (flag)
return;
guard(mutex)(&data->pending_lock);
list_add_tail(&msg->list, &data->pending_list);
}
helper for three (*yes, 3) repetitive code snippets.
--
With Best Regards,
Andy Shevchenko
On 16 March 2026 8:02:09 pm IST, Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
>On Sun, Mar 15, 2026 at 06:25:07PM +0530, Sanjay Chitroda wrote:
>
>> Replace explicit mutex_lock() and mutex_unlock() with the guard() macro
>> for cleaner and safer mutex handling.
>
>TBH I don't see much benefit in this form. What I am thinking of is to refactor
>to have the guard and timeout_cnt++ in the top level, and
>
>static void ...(flag)
>{
> if (flag)
> return;
>
> guard(mutex)(&data->pending_lock);
> list_add_tail(&msg->list, &data->pending_list);
>}
>
>helper for three (*yes, 3) repetitive code snippets.
>
Thank you Andy for the review.
I understand your point about refactoring if pattern is repeated to simply things.
Also, while revisiting the changes, I noticed that unintended mistake where list_del() was replaced with list_add_tail() in couple of place during guard() conversion. I'll fix that in v4 along with suggestions and input from you and Jonathan.
I’ll send an updated version shortly.
On Sun, 15 Mar 2026 18:25:07 +0530
Sanjay Chitroda <sanjayembeddedse@gmail.com> wrote:
> From: Sanjay Chitroda <sanjayembeddedse@gmail.com>
>
> Replace explicit mutex_lock() and mutex_unlock() with the guard() macro
> for cleaner and safer mutex handling.
>
> Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
> ---
> drivers/iio/common/ssp_sensors/ssp_spi.c | 54 ++++++++++--------------
> 1 file changed, 22 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/iio/common/ssp_sensors/ssp_spi.c b/drivers/iio/common/ssp_sensors/ssp_spi.c
> index 6c81c0385fb5..87a38002b218 100644
> --- a/drivers/iio/common/ssp_sensors/ssp_spi.c
> +++ b/drivers/iio/common/ssp_sensors/ssp_spi.c
> @@ -2,6 +2,7 @@
> /*
> * Copyright (C) 2014, Samsung Electronics Co. Ltd. All Rights Reserved.
> */
> +#include <linux/cleanup.h>
>
> #include "ssp.h"
>
> @@ -189,55 +190,49 @@ static int ssp_do_transfer(struct ssp_data *data, struct ssp_msg *msg,
>
> msg->done = done;
>
> - mutex_lock(&data->comm_lock);
> + guard(mutex)(&data->comm_lock);
>
> status = ssp_check_lines(data, false);
> - if (status < 0)
> - goto _error_locked;
> + if (status < 0) {
> + data->timeout_cnt++;
> + return status;
> + }
>
> status = spi_write(data->spi, msg->buffer, SSP_HEADER_SIZE);
> if (status < 0) {
> gpiod_set_value_cansleep(data->ap_mcu_gpiod, 1);
> dev_err(SSP_DEV, "%s spi_write fail\n", __func__);
> - goto _error_locked;
> + data->timeout_cnt++;
guard() isn't always the right answer. The counter needing updating
here in every error path makes me thing maybe it isn't here.
Pity there is one error path at the top where that's not updated,
otherwise I'd suggest updating in a wrapper function.
> + return status;
> }
>
> if (!use_no_irq) {
> - mutex_lock(&data->pending_lock);
> - list_add_tail(&msg->list, &data->pending_list);
> - mutex_unlock(&data->pending_lock);
> + scoped_guard(mutex, &data->pending_lock)
The guard brings little here, but if you want to do it.
guard(mutex)(&data->pending_lock);
is fine - no need for scoped_guard() and increased indent.
Make sure you understand why that is effectively the same in this cases.
> + list_add_tail(&msg->list, &data->pending_list);
> }
>
> status = ssp_check_lines(data, true);
> if (status < 0) {
> if (!use_no_irq) {
> - mutex_lock(&data->pending_lock);
> - list_del(&msg->list);
> - mutex_unlock(&data->pending_lock);
> + scoped_guard(mutex, &data->pending_lock)
As above.
> + list_add_tail(&msg->list, &data->pending_list);
> }
> - goto _error_locked;
> + data->timeout_cnt++;
> + return status;
> }
>
> - mutex_unlock(&data->comm_lock);
> -
> if (!use_no_irq && done)
> if (wait_for_completion_timeout(done,
> msecs_to_jiffies(timeout)) ==
> 0) {
> - mutex_lock(&data->pending_lock);
> - list_del(&msg->list);
> - mutex_unlock(&data->pending_lock);
> + scoped_guard(mutex, &data->pending_lock)
> + list_add_tail(&msg->list, &data->pending_list);
Slightly trickier but holding the mutex over the timeout_cnt is fine, so
I'd use guard() here as well.
>
> data->timeout_cnt++;
> return -ETIMEDOUT;
> }
>
> return 0;
> -
> -_error_locked:
> - mutex_unlock(&data->comm_lock);
> - data->timeout_cnt++;
> - return status;
> }
>
> static inline int ssp_spi_sync_command(struct ssp_data *data,
> @@ -360,7 +355,7 @@ int ssp_irq_msg(struct ssp_data *data)
> * this is a small list, a few elements - the packets can be
> * received with no order
> */
> - mutex_lock(&data->pending_lock);
> + guard(mutex)(&data->pending_lock);
Check the scope definition. Given where you remove the unlock below
I'm not sure the scope is what you think it is.
Switch statements are tricky wrt to scope!
> list_for_each_entry_safe(iter, n, &data->pending_list, list) {
> if (iter->options == msg_options) {
> list_del(&iter->list);
> @@ -376,10 +371,8 @@ int ssp_irq_msg(struct ssp_data *data)
> * check but let's handle this
> */
> buffer = kmalloc(length, GFP_KERNEL | GFP_DMA);
> - if (!buffer) {
> - ret = -ENOMEM;
> - goto _unlock;
> - }
> + if (!buffer)
> + return -ENOMEM;
>
> /* got dead packet so it is always an error */
> ret = spi_read(data->spi, buffer, length);
> @@ -391,7 +384,7 @@ int ssp_irq_msg(struct ssp_data *data)
> dev_err(SSP_DEV, "No match error %x\n",
> msg_options);
>
> - goto _unlock;
> + return ret;
> }
>
> if (msg_type == SSP_AP2HUB_READ)
> @@ -409,15 +402,13 @@ int ssp_irq_msg(struct ssp_data *data)
> msg->length = 1;
>
> list_add_tail(&msg->list, &data->pending_list);
> - goto _unlock;
> + return ret;
> }
> }
>
> if (msg->done)
> if (!completion_done(msg->done))
> complete(msg->done);
> -_unlock:
> - mutex_unlock(&data->pending_lock);
> break;
> case SSP_HUB2AP_WRITE:
> buffer = kzalloc(length, GFP_KERNEL | GFP_DMA);
> @@ -448,7 +439,7 @@ void ssp_clean_pending_list(struct ssp_data *data)
> {
> struct ssp_msg *msg, *n;
>
> - mutex_lock(&data->pending_lock);
> + guard(mutex)(&data->pending_lock);
> list_for_each_entry_safe(msg, n, &data->pending_list, list) {
> list_del(&msg->list);
>
> @@ -456,7 +447,6 @@ void ssp_clean_pending_list(struct ssp_data *data)
> if (!completion_done(msg->done))
> complete(msg->done);
> }
> - mutex_unlock(&data->pending_lock);
> }
>
> int ssp_command(struct ssp_data *data, char command, int arg)
© 2016 - 2026 Red Hat, Inc.