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

[Sanjay Chitroda]

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.
>
>...
>
>>  	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.