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

Dmitry Baryshkov
2025-12-18T01:22:50Z

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