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

["Katiyar, Pooja" <pooja.katiyar@linux.intel.com>]

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? 

Best regards,
Pooja