Re: [PATCH v4 2/2] usb: typec: ucsi_glink: Increase buffer size to support UCSI v2

[Anjelique Melendez <anjelique.melendez@oss.qualcomm.com>]

On 9/25/2025 2:43 PM, Dmitry Baryshkov wrote:
> On Wed, Sep 24, 2025 at 04:26:31PM -0700, Anjelique Melendez wrote:
>> UCSI v2 specification has increased the MSG_IN and MSG_OUT size from
>> 16 bytes to 256 bytes each for the message exchange between OPM and PPM
>> This makes the total buffer size increase from 48 bytes to 528 bytes.
>> Update the buffer size to support this increase.
>>
>> Signed-off-by: Anjelique Melendez <anjelique.melendez@oss.qualcomm.com>
>> ---
>>   drivers/usb/typec/ucsi/ucsi_glink.c | 81 ++++++++++++++++++++++++-----
>>   1 file changed, 68 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/usb/typec/ucsi/ucsi_glink.c b/drivers/usb/typec/ucsi/ucsi_glink.c
>> index 1f9f0d942c1a..7f19b4d23fed 100644
>> --- a/drivers/usb/typec/ucsi/ucsi_glink.c
>> +++ b/drivers/usb/typec/ucsi/ucsi_glink.c
>> @@ -16,10 +16,10 @@
>>   
>>   #define PMIC_GLINK_MAX_PORTS		3
>>   
>> -#define UCSI_BUF_SIZE                   48
>> +#define UCSI_BUF_V1_SIZE		(UCSI_MESSAGE_OUT + (UCSI_MESSAGE_OUT - UCSI_MESSAGE_IN))
>> +#define UCSI_BUF_V2_SIZE		(UCSIv2_MESSAGE_OUT + (UCSIv2_MESSAGE_OUT - UCSI_MESSAGE_IN))
>>   
>>   #define MSG_TYPE_REQ_RESP               1
>> -#define UCSI_BUF_SIZE                   48
>>   
>>   #define UC_NOTIFY_RECEIVER_UCSI         0x0
>>   #define UC_UCSI_READ_BUF_REQ            0x11
>> @@ -30,15 +30,27 @@ struct ucsi_read_buf_req_msg {
>>   	struct pmic_glink_hdr   hdr;
>>   };
>>   
>> -struct __packed ucsi_read_buf_resp_msg {
>> +struct __packed ucsi_v1_read_buf_resp_msg {
>>   	struct pmic_glink_hdr   hdr;
>> -	u8                      buf[UCSI_BUF_SIZE];
>> +	u8                      buf[UCSI_BUF_V1_SIZE];
>>   	u32                     ret_code;
>>   };
>>   
>> -struct __packed ucsi_write_buf_req_msg {
>> +struct __packed ucsi_v2_read_buf_resp_msg {
>>   	struct pmic_glink_hdr   hdr;
>> -	u8                      buf[UCSI_BUF_SIZE];
>> +	u8                      buf[UCSI_BUF_V2_SIZE];
>> +	u32                     ret_code;
>> +};
>> +
>> +struct __packed ucsi_v1_write_buf_req_msg {
>> +	struct pmic_glink_hdr   hdr;
>> +	u8                      buf[UCSI_BUF_V1_SIZE];
>> +	u32                     reserved;
>> +};
>> +
>> +struct __packed ucsi_v2_write_buf_req_msg {
>> +	struct pmic_glink_hdr   hdr;
>> +	u8                      buf[UCSI_BUF_V2_SIZE];
>>   	u32                     reserved;
>>   };
>>   
>> @@ -72,7 +84,7 @@ struct pmic_glink_ucsi {
>>   	bool ucsi_registered;
>>   	bool pd_running;
>>   
>> -	u8 read_buf[UCSI_BUF_SIZE];
>> +	u8 read_buf[UCSI_BUF_V2_SIZE];
>>   };
>>   
>>   static int pmic_glink_ucsi_read(struct ucsi *__ucsi, unsigned int offset,
>> @@ -131,18 +143,34 @@ static int pmic_glink_ucsi_read_message_in(struct ucsi *ucsi, void *val, size_t
>>   static int pmic_glink_ucsi_locked_write(struct pmic_glink_ucsi *ucsi, unsigned int offset,
>>   					const void *val, size_t val_len)
>>   {
>> -	struct ucsi_write_buf_req_msg req = {};
>> -	unsigned long left;
>> +	struct ucsi_v2_write_buf_req_msg req = {};
>> +	unsigned long left, max_buf_len;
>> +	size_t req_len;
>>   	int ret;
>>   
>> +	memset(&req, 0, sizeof(req));
>>   	req.hdr.owner = PMIC_GLINK_OWNER_USBC;
>>   	req.hdr.type = MSG_TYPE_REQ_RESP;
>>   	req.hdr.opcode = UC_UCSI_WRITE_BUF_REQ;
>> +
>> +	if (ucsi->ucsi->version >= UCSI_VERSION_2_0) {
>> +		req_len = sizeof(struct ucsi_v2_write_buf_req_msg);
>> +		max_buf_len = UCSI_BUF_V2_SIZE;
> 
> I'd prefer it to be more explicit. Define an union of v1 and v2, fill
> common parts and version-specific parts separately.
Konrad also left a similar comment in this function "This code keeps the 
'reserved' field zeored out for v1, but it does so in a fragile and 
implicit way :/" 
(https://lore.kernel.org/all/df671650-a5af-4453-a11d-e8e2a32bd1ab@oss.qualcomm.com/#t)

So I figured I would try to get thoughts from the both of you :)

We could have a union defined like so:
struct __packed ucsi_write_buf_req_msg {
	struct pmic_glink_hdr   hdr;
	union {
		u8 v2_buf[UCSI_BUF_V2_SIZE];
		u8 v1_buf[UCSI_BUF_V1_SIZE];
	} buf;
	u32                     reserved;
};

and then ucsi_locked_write() pseudo would be something like this:

pmic_glink_ucsi_locked_write()
{
	struct ucsi_write_buf_req_msg req = {};
	u8 *buf;

	req.hdr.owner = PMIC_GLINK_OWNER_USBC;
	req.hdr.type = MSG_TYPE_REQ_RESP;
	req.hdr.opcode = UC_UCSI_WRITE_BUF_REQ;

	if (version >= UCSI_VERSION_2_0)
		buf_len = UCSI_BUF_V2_SIZE;
		buf = req.buf.v2_buf;
	else if (version)
		buf_len = UCSI_BUF_V1_SIZE;
		buf = req.buf.v1_buf;
	else
		return -EINVAL;
	req_len = sizeof(struct pmic_glink_hdr) + buf_len + sizeof(u32);

	memcpy(&buf[offset], val, val_len);

	ret = pmic_glink_send(ucsi->client, &req, req_len);
	if (ret < 0)
		return ret;

	left = wait_for_completion_timeout(&ucsi->write_ack, 5 * HZ);
	if (!left)
		return -ETIMEDOUT;

	return 0;
}

Since we are adding the union we still end up initializing space for the 
larger UCSI v2 buffer and when we have UCSI v1 we are only expected to 
send a request with buffer size = UCSI v1. With this we would still be 
keeping a reserved field zeroed for v1 but it still is not the 
req.reserved field being explicitly sent.

The only other solution I can think of that would be fully explicit is 
if we created pmic_glink_ucsi_v2_locked_write() which basically just did 
the exact same thing as the original pmic_glink_ucsi_locked_write() but 
instead used ucsi_v2_write_buf_req_msg struct. 
pmic_glink_ucsi_async_control() would then decide which locked_write 
function to call based on version. However that would include a lot of 
code copying.

Let me know what your thoughts are - I'm more than happy to go the way 
of the union but just want to make sure that we are all on same page and 
can agree on best steps forward :)

Thanks,
Anjelique