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

Katiyar, Pooja
2025-12-18T01:17:18Z

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