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 Tue, Dec 23, 2025 at 04:06:44AM -0800, Heikki Krogerus wrote:
> Hi,
> 
> Thu, Dec 18, 2025 at 03:22:43AM +0200, Dmitry Baryshkov kirjoitti:
>> 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!
> 
> I don't think you are fully solving the problem.
> 
> Pooja, you know my opinion. This whole approach of using separate
> files is wrong. IMO we should only have a single file for the entire
> UCSI mailbox, and the mailbox should be allocated searately for
> everyone that opens the file. I don't think there is any way to
> guarantee that the data in these separate files isn't for somebody
> else.
> 
> thanks,
> 

Thank you for your feedback! We will work on a redesign for clearer implementation.

Regards,
Pooja