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>
---
Changes in v4:
- Use guard() instead of scoped_guard() to avoid additional indentation
as scope is same for both APIs and add scope in switch case
- Link to v3: https://lore.kernel.org/all/20260315125509.857195-2-sanjayembedded@gmail.com/
---
drivers/iio/common/ssp_sensors/ssp_spi.c | 50 ++++++++++--------------
1 file changed, 20 insertions(+), 30 deletions(-)
diff --git a/drivers/iio/common/ssp_sensors/ssp_spi.c b/drivers/iio/common/ssp_sensors/ssp_spi.c
index 08ed92859be0..61a4e978d9b2 100644
--- a/drivers/iio/common/ssp_sensors/ssp_spi.c
+++ b/drivers/iio/common/ssp_sensors/ssp_spi.c
@@ -189,55 +189,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);
+ guard(mutex)(&data->pending_lock);
list_add_tail(&msg->list, &data->pending_list);
- mutex_unlock(&data->pending_lock);
}
status = ssp_check_lines(data, true);
if (status < 0) {
if (!use_no_irq) {
- mutex_lock(&data->pending_lock);
+ guard(mutex)(&data->pending_lock);
list_del(&msg->list);
- mutex_unlock(&data->pending_lock);
}
- 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);
+ guard(mutex)(&data->pending_lock);
list_del(&msg->list);
- mutex_unlock(&data->pending_lock);
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,
@@ -355,12 +349,12 @@ int ssp_irq_msg(struct ssp_data *data)
switch (msg_type) {
case SSP_AP2HUB_READ:
- case SSP_AP2HUB_WRITE:
+ case SSP_AP2HUB_WRITE: {
/*
* 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 +370,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 +383,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,16 +401,15 @@ 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);
if (!buffer)
@@ -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 Thu, Mar 26, 2026 at 01:48:14PM +0530, Sanjay Chitroda wrote:
> Replace explicit mutex_lock() and mutex_unlock() with the guard() macro
> for cleaner and safer mutex handling.
NAK. Please, be very careful when do such changes.
...
> 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);
> + guard(mutex)(&data->pending_lock);
> list_add_tail(&msg->list, &data->pending_list);
> - mutex_unlock(&data->pending_lock);
> }
>
> status = ssp_check_lines(data, true);
> if (status < 0) {
> if (!use_no_irq) {
> - mutex_lock(&data->pending_lock);
> + guard(mutex)(&data->pending_lock);
> list_del(&msg->list);
> - mutex_unlock(&data->pending_lock);
> }
> - goto _error_locked;
> + data->timeout_cnt++;
> + return status;
> }
> - mutex_unlock(&data->comm_lock);
> -
Pzzz! See what you are doing here...
> if (!use_no_irq && done)
> if (wait_for_completion_timeout(done,
> msecs_to_jiffies(timeout)) ==
> 0) {
> - mutex_lock(&data->pending_lock);
> + guard(mutex)(&data->pending_lock);
> list_del(&msg->list);
> - mutex_unlock(&data->pending_lock);
>
> data->timeout_cnt++;
> return -ETIMEDOUT;
> }
>
> return 0;
> -
> -_error_locked:
> - mutex_unlock(&data->comm_lock);
> - data->timeout_cnt++;
> - return status;
> }
--
With Best Regards,
Andy Shevchenko
On 26 March 2026 2:52:06 pm IST, Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
>On Thu, Mar 26, 2026 at 01:48:14PM +0530, Sanjay Chitroda wrote:
>
>> Replace explicit mutex_lock() and mutex_unlock() with the guard() macro
>> for cleaner and safer mutex handling.
>
>NAK. Please, be very careful when do such changes.
>
>...
>
>> 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);
>> + guard(mutex)(&data->pending_lock);
>> list_add_tail(&msg->list, &data->pending_list);
>> - mutex_unlock(&data->pending_lock);
>> }
>>
>> status = ssp_check_lines(data, true);
>> if (status < 0) {
>> if (!use_no_irq) {
>> - mutex_lock(&data->pending_lock);
>> + guard(mutex)(&data->pending_lock);
>> list_del(&msg->list);
>> - mutex_unlock(&data->pending_lock);
>> }
>> - goto _error_locked;
>> + data->timeout_cnt++;
>> + return status;
>> }
>
>> - mutex_unlock(&data->comm_lock);
>> -
>
>Pzzz! See what you are doing here...
>
>> if (!use_no_irq && done)
>> if (wait_for_completion_timeout(done,
>> msecs_to_jiffies(timeout)) ==
>> 0) {
>> - mutex_lock(&data->pending_lock);
>> + guard(mutex)(&data->pending_lock);
>> list_del(&msg->list);
>> - mutex_unlock(&data->pending_lock);
>>
>> data->timeout_cnt++;
>> return -ETIMEDOUT;
>> }
>>
>> return 0;
>> -
>> -_error_locked:
>> - mutex_unlock(&data->comm_lock);
>> - data->timeout_cnt++;
>> - return status;
>> }
>
Thank Andy for pointing this out — you’re right, using "guard(mutex)" here unintentionally extends the lifetime of "comm_lock" and can hold it across the completion wait, which is not safe.
I’ve reworked the change to keep the original locking semantics intact:
- "comm_lock" is still explicitly unlocked before "wait_for_completion_timeout()"
- No sleeping paths are executed while holding "comm_lock"
- Introduced small helpers to simplify the flow:
- "ssp_send_and_enqueue()" for SPI write + pending list add
- "ssp_dequeue_msg()" for safe removal from the pending list
Updated flow looks like this:
mutex_lock(&data->comm_lock);
status = ssp_check_lines(data, false);
if (status < 0)
goto err;
status = ssp_send_and_enqueue(data, msg, use_no_irq);
if (status < 0)
goto err;
status = ssp_check_lines(data, true);
if (status < 0) {
if (!use_no_irq)
ssp_dequeue_msg(data, msg);
goto err;
}
mutex_unlock(&data->comm_lock);
/* wait outside lock */
if (!use_no_irq && done) {
if (wait_for_completion_timeout(done,
msecs_to_jiffies(timeout)) == 0) {
ssp_dequeue_msg(data, msg);
data->timeout_cnt++;
return -ETIMEDOUT;
}
}
return 0;
err:
mutex_unlock(&data->comm_lock);
data->timeout_cnt++;
return status;
This keeps the synchronization boundary unchanged while reducing duplication around pending list handling. I’ve also limited "guard()" usage to short, local critical sections only.
Please let me know if you’d prefer keeping the list operations inline instead of helpers.
On Sat, Mar 28, 2026 at 12:24:11PM +0530, Sanjay Chitroda wrote: > On 26 March 2026 2:52:06 pm IST, Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > >On Thu, Mar 26, 2026 at 01:48:14PM +0530, Sanjay Chitroda wrote: > >> Replace explicit mutex_lock() and mutex_unlock() with the guard() macro > >> for cleaner and safer mutex handling. > > > >NAK. Please, be very careful when do such changes. ... > >> - mutex_unlock(&data->comm_lock); > >> - > > > >Pzzz! See what you are doing here... > > Thank Andy for pointing this out — you’re right, using "guard(mutex)" here > unintentionally extends the lifetime of "comm_lock" and can hold it across > the completion wait, which is not safe. > > I’ve reworked the change to keep the original locking semantics intact: > > - "comm_lock" is still explicitly unlocked before "wait_for_completion_timeout()" > - No sleeping paths are executed while holding "comm_lock" > - Introduced small helpers to simplify the flow: > - "ssp_send_and_enqueue()" for SPI write + pending list add > - "ssp_dequeue_msg()" for safe removal from the pending list > > Updated flow looks like this: If you are going to mix mutex_lock() with guard()(), it's even more NAKish solution (id est worse than no change). > This keeps the synchronization boundary unchanged while reducing duplication > around pending list handling. I’ve also limited "guard()" usage to short, > local critical sections only. > > Please let me know if you’d prefer keeping the list operations inline instead > of helpers. -- With Best Regards, Andy Shevchenko
On 29 March 2026 5:14:38 pm IST, Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
>On Sat, Mar 28, 2026 at 12:24:11PM +0530, Sanjay Chitroda wrote:
>> On 26 March 2026 2:52:06 pm IST, Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
>> >On Thu, Mar 26, 2026 at 01:48:14PM +0530, Sanjay Chitroda wrote:
>
>> >> Replace explicit mutex_lock() and mutex_unlock() with the guard() macro
>> >> for cleaner and safer mutex handling.
>> >
>> >NAK. Please, be very careful when do such changes.
>
>...
>
>> >> - mutex_unlock(&data->comm_lock);
>> >> -
>> >
>> >Pzzz! See what you are doing here...
>>
>> Thank Andy for pointing this out — you’re right, using "guard(mutex)" here
>> unintentionally extends the lifetime of "comm_lock" and can hold it across
>> the completion wait, which is not safe.
>>
>> I’ve reworked the change to keep the original locking semantics intact:
>>
>> - "comm_lock" is still explicitly unlocked before "wait_for_completion_timeout()"
>> - No sleeping paths are executed while holding "comm_lock"
>> - Introduced small helpers to simplify the flow:
>> - "ssp_send_and_enqueue()" for SPI write + pending list add
>> - "ssp_dequeue_msg()" for safe removal from the pending list
>>
>> Updated flow looks like this:
>
>If you are going to mix mutex_lock() with guard()(), it's even more NAKish
>solution (id est worse than no change).
>
Thanks — agreed on not mixing "mutex_lock()"/"mutex_unlock()" with "guard()".
This patch is intended as a cleanup to switch to "guard()", not to change locking semantics.
I’ll rework this so that it’s a consistent conversion:
- convert it fully to "guard()" without changing the lock scope (e.g., by restructuring the critical section appropriately or limiting scope with {} ).
Will update the patch accordingly.
>> This keeps the synchronization boundary unchanged while reducing duplication
>> around pending list handling. I’ve also limited "guard()" usage to short,
>> local critical sections only.
>>
>> Please let me know if you’d prefer keeping the list operations inline instead
>> of helpers.
>
On 30 March 2026 9:47:52 pm IST, Sanjay Chitroda <sanjayembeddedse@gmail.com> wrote:
>
>
>On 29 March 2026 5:14:38 pm IST, Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
>>On Sat, Mar 28, 2026 at 12:24:11PM +0530, Sanjay Chitroda wrote:
>>> On 26 March 2026 2:52:06 pm IST, Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
>>> >On Thu, Mar 26, 2026 at 01:48:14PM +0530, Sanjay Chitroda wrote:
>>
>>> >> Replace explicit mutex_lock() and mutex_unlock() with the guard() macro
>>> >> for cleaner and safer mutex handling.
>>> >
>>> >NAK. Please, be very careful when do such changes.
>>
>>...
>>
>>> >> - mutex_unlock(&data->comm_lock);
>>> >> -
>>> >
>>> >Pzzz! See what you are doing here...
>>>
>>> Thank Andy for pointing this out — you’re right, using "guard(mutex)" here
>>> unintentionally extends the lifetime of "comm_lock" and can hold it across
>>> the completion wait, which is not safe.
>>>
>>> I’ve reworked the change to keep the original locking semantics intact:
>>>
>>> - "comm_lock" is still explicitly unlocked before "wait_for_completion_timeout()"
>>> - No sleeping paths are executed while holding "comm_lock"
>>> - Introduced small helpers to simplify the flow:
>>> - "ssp_send_and_enqueue()" for SPI write + pending list add
>>> - "ssp_dequeue_msg()" for safe removal from the pending list
>>>
>>> Updated flow looks like this:
>>
>>If you are going to mix mutex_lock() with guard()(), it's even more NAKish
>>solution (id est worse than no change).
>>
>
>Thanks — agreed on not mixing "mutex_lock()"/"mutex_unlock()" with "guard()".
>
>This patch is intended as a cleanup to switch to "guard()", not to change locking semantics.
>
>I’ll rework this so that it’s a consistent conversion:
>
>- convert it fully to "guard()" without changing the lock scope (e.g., by restructuring the critical section appropriately or limiting scope with {} ).
>
>Will update the patch accordingly.
>
Following up on my previous reply — after reconsidering, I’ll drop the guard-based conversion to keep the locking style consistent with the existing code.
Instead, I’ll keep the explicit mutex usage and, if needed, simplify the repeated pending_list handling via small helper functions without changing any locking semantics.
I’ll send a revised version accordingly.
>
>>> This keeps the synchronization boundary unchanged while reducing duplication
>>> around pending list handling. I’ve also limited "guard()" usage to short,
>>> local critical sections only.
>>>
>>> Please let me know if you’d prefer keeping the list operations inline instead
>>> of helpers.
>>
© 2016 - 2026 Red Hat, Inc.