Re: [PATCH v5 1/4] usb: typec: ucsi: Update UCSI structure to have message in and message out fields

[Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>]

On 18/12/2025 03:17, Katiyar, Pooja wrote:
> Hello,
> 
> On Fri, Dec 12, 2025 at 06:58:06PM -0800, Dmitry Baryshkov wrote:
>> On Thu, Oct 30, 2025 at 07:48:55AM -0700, Pooja Katiyar wrote:
>>> Update UCSI structure by adding fields for incoming and outgoing
>>> messages. Update .sync_control function and other related functions
>>> to use these new fields within the UCSI structure, instead of handling
>>> them as separate parameters.
>>>
>>> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>> Signed-off-by: Pooja Katiyar <pooja.katiyar@intel.com>
>>> ---
>>> Changelog v3:
>>> - Added message fields to UCSI structure and updated sync_control handling.
>>
>> Colleagues, after looking at this patch I have a question. What prevents
>> message_{in,out}{,_size} to be modified concurrently? While we have PPM
>> lock around command submission, size fields and buffers are modified /
>> accessed outside of the lock. Granted all the notifications and async
>> nature of the UCSI and USB-C protocols, what prevents two commands from
>> being executed at the same time with one of the execution threads
>> accessing the results returned by the other command?
>>
>> In other words:
>>
>> - thread A sets message_in_size, calls ucsi_send_command(CMD_A), which
>>    takes PPM lock
>>
>>     - meanwhile thread B writes another value to message_in_size and
>>       calls ucsi_send_command(CMD_B), which now waits on PPM lock
>>
>> - thread A finishes execution of the CMD_A, copies data (with the
>>    wrong size!) to the message_in_buf, then it releases PPM lock.
>>
>> - thread A gets preempted
>>
>>      - thread B gets scheduled, it takes PPM lock, executes CMD_B, gets
>>        data into the message_in_buf and finally it releases PPM lock
>>
>> - finally at some later point thread A gets scheduled, it accesses
>>    message_in_buf, reading data that has been overwritten by CMD_B
>>    execution.
>>
>> WDYT?
> 
> Thank you for identifying this race condition. You are correct about the
> concurrent access issue with the message buffer fields.
> 
> Here are two potential approaches I see to resolve this:
> 
> Option 1: Separate mutex locks for message variables
> Add a dedicated message_lock mutex to protect message_{in,out}{,_size}.
> message_{in,out}{,_size} will only be modified within the message_lock
> protection.
> 
> 	mutex_lock(&ucsi->message_lock);
> 	ucsi->message_in_size = size;
> 	ret = ucsi_send_command(ucsi, cmd);
> 	memcpy(buffer, ucsi->message_in, size);
> 	mutex_unlock(&ucsi->message_lock);
> 
> Option 2: Pass message variables as function arguments
> Pass message buffers and sizes as parameters down to where ppm_lock is
> acquired, ensuring protection throughout command execution.
> 
> 	int ucsi_send_command(ucsi, cmd, msg_in_buf, msg_in_size, msg_out_buf,
> 			      msg_out_size) {
>      		mutex_lock(&ucsi->ppm_lock);
>      		ucsi->message_in_size = msg_in_size;
>      		// execute command and copy results to msg_in_buf
> 		memcpy(msg_in_buf, ucsi->message_in, msg_in_size);
>      		mutex_unlock(&ucsi->ppm_lock);
> 	}
> 
> I'm leaning toward Option 1 as it resolves the concern of passing too many args.
> What are your thoughts on the suggested options or do you have alternative
> suggestions?

I'm slightly biased towards  the second option (it doesn't required 
extra locks and it also is a bit more idiomatic), but I'm fine either way.

Looking forward to seeing the fixing patch!

> 
> Best regards,
> Pooja


-- 
With best wishes
Dmitry