Re: [PATCH v4 3/4] iio: ssp_sensors: ssp_spi: use guard() to release mutexes

[Sanjay Chitroda]

On 30 March 2026 9:47:52 pm IST, Sanjay Chitroda <sanjayembeddedse@xxxxxxxxx> wrote:
>
>
>On 29 March 2026 5:14:38 pm IST, Andy Shevchenko <andriy.shevchenko@xxxxxxxxx> 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@xxxxxxxxx> 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.
>>