[PATCH v2] usb: typec: ucsi: ucsi_glink: Increase buffer size to support UCSI v2

[PATCH v2] usb: typec: ucsi: ucsi_glink: Increase buffer size to support UCSI v2

[Anjelique Melendez]

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>
---
Changes since v1:
 - Defined buf size in terms of other UCSI defines
 - Removed UCSI_BUF_SIZE and used the explicit v1 or v2 buffer size macros
 - Removed Qualcomm copyright
 - link: https://lore.kernel.org/all/20250624222922.2010820-1-anjelique.melendez@oss.qualcomm.com/
---
 drivers/usb/typec/ucsi/ucsi_glink.c | 58 ++++++++++++++++++++++++-----
 1 file changed, 48 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi_glink.c b/drivers/usb/typec/ucsi/ucsi_glink.c
index 8af79101a2fc..2918c88e54d2 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
@@ -32,13 +32,25 @@ struct ucsi_read_buf_req_msg {
 
 struct ucsi_read_buf_resp_msg {
 	struct pmic_glink_hdr   hdr;
-	u8                      buf[UCSI_BUF_SIZE];
+	u8                      buf[UCSI_BUF_V1_SIZE];
+	u32                     ret_code;
+};
+
+struct ucsi_v2_read_buf_resp_msg {
+	struct pmic_glink_hdr   hdr;
+	u8                      buf[UCSI_BUF_V2_SIZE];
 	u32                     ret_code;
 };
 
 struct ucsi_write_buf_req_msg {
 	struct pmic_glink_hdr   hdr;
-	u8                      buf[UCSI_BUF_SIZE];
+	u8                      buf[UCSI_BUF_V1_SIZE];
+	u32                     reserved;
+};
+
+struct 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,8 +143,9 @@ 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 = {};
+	struct ucsi_v2_write_buf_req_msg req = {};
 	unsigned long left;
+	size_t len;
 	int ret;
 
 	req.hdr.owner = PMIC_GLINK_OWNER_USBC;
@@ -142,7 +155,18 @@ static int pmic_glink_ucsi_locked_write(struct pmic_glink_ucsi *ucsi, unsigned i
 
 	reinit_completion(&ucsi->write_ack);
 
-	ret = pmic_glink_send(ucsi->client, &req, sizeof(req));
+	if (!ucsi->ucsi->version || ucsi->ucsi->version >= UCSI_VERSION_2_1) {
+		/* If UCSI version is unknown, use the maximum buffer size */
+		len = sizeof(req);
+	} else {
+		/*
+		 * If UCSI V1, buffer size should be UCSI_BUF_V1_SIZE so update
+		 * len accordingly
+		 */
+		len = sizeof(struct ucsi_write_buf_req_msg);
+	}
+
+	ret = pmic_glink_send(ucsi->client, &req, len);
 	if (ret < 0) {
 		dev_err(ucsi->dev, "failed to send UCSI write request: %d\n", ret);
 		return ret;
@@ -216,12 +240,26 @@ static const struct ucsi_operations pmic_glink_ucsi_ops = {
 
 static void pmic_glink_ucsi_read_ack(struct pmic_glink_ucsi *ucsi, const void *data, int len)
 {
-	const struct ucsi_read_buf_resp_msg *resp = data;
+	const struct ucsi_v2_read_buf_resp_msg *resp = data;
+	u32 ret_code, buffer_len;
+
+	if (!ucsi->ucsi->version || ucsi->ucsi->version >= UCSI_VERSION_2_1) {
+		/* If UCSI version is unknown, use the maximum buffer size */
+		ret_code = resp->ret_code;
+		buffer_len = UCSI_BUF_V2_SIZE;
+	} else {
+		/*
+		 * If UCSI V1, use UCSI_BUF_V1_SIZE buffer size and
+		 * update ret_code offset accordingly
+		 */
+		ret_code = ((struct ucsi_read_buf_resp_msg *)data)->ret_code;
+		buffer_len = UCSI_BUF_V1_SIZE;
+	}
 
-	if (resp->ret_code)
+	if (ret_code)
 		return;
 
-	memcpy(ucsi->read_buf, resp->buf, UCSI_BUF_SIZE);
+	memcpy(ucsi->read_buf, resp->buf, buffer_len);
 	complete(&ucsi->read_ack);
 }
 
-- 
2.34.1

Re: [PATCH v2] usb: typec: ucsi: ucsi_glink: Increase buffer size to support UCSI v2

[Greg KH]

On Tue, Jul 15, 2025 at 05:52:24PM -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>
> ---
> Changes since v1:
>  - Defined buf size in terms of other UCSI defines
>  - Removed UCSI_BUF_SIZE and used the explicit v1 or v2 buffer size macros
>  - Removed Qualcomm copyright
>  - link: https://lore.kernel.org/all/20250624222922.2010820-1-anjelique.melendez@oss.qualcomm.com/
> ---
>  drivers/usb/typec/ucsi/ucsi_glink.c | 58 ++++++++++++++++++++++++-----
>  1 file changed, 48 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi_glink.c b/drivers/usb/typec/ucsi/ucsi_glink.c
> index 8af79101a2fc..2918c88e54d2 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
> @@ -32,13 +32,25 @@ struct ucsi_read_buf_req_msg {
>  
>  struct ucsi_read_buf_resp_msg {
>  	struct pmic_glink_hdr   hdr;
> -	u8                      buf[UCSI_BUF_SIZE];
> +	u8                      buf[UCSI_BUF_V1_SIZE];
> +	u32                     ret_code;

What is the endian-ness of ret_code as it comes from the device?

> +};
> +
> +struct ucsi_v2_read_buf_resp_msg {
> +	struct pmic_glink_hdr   hdr;
> +	u8                      buf[UCSI_BUF_V2_SIZE];
>  	u32                     ret_code;
>  };
>  
>  struct ucsi_write_buf_req_msg {
>  	struct pmic_glink_hdr   hdr;
> -	u8                      buf[UCSI_BUF_SIZE];
> +	u8                      buf[UCSI_BUF_V1_SIZE];
> +	u32                     reserved;

What is "reserved" for?  What is it set to?  Where is it tested to
ensure it is set properly?

> +};
> +
> +struct ucsi_v2_write_buf_req_msg {
> +	struct pmic_glink_hdr   hdr;
> +	u8                      buf[UCSI_BUF_V2_SIZE];
>  	u32                     reserved;
>  };

And for all of these structures, are you sure there are not "holes" in
them?  As you are using sizeof(), don't they need to be packed?

>  
> @@ -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];

Why is this one just v2 and not also v1?

>  };
>  
>  static int pmic_glink_ucsi_read(struct ucsi *__ucsi, unsigned int offset,
> @@ -131,8 +143,9 @@ 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 = {};
> +	struct ucsi_v2_write_buf_req_msg req = {};
>  	unsigned long left;
> +	size_t len;
>  	int ret;
>  
>  	req.hdr.owner = PMIC_GLINK_OWNER_USBC;
> @@ -142,7 +155,18 @@ static int pmic_glink_ucsi_locked_write(struct pmic_glink_ucsi *ucsi, unsigned i
>  
>  	reinit_completion(&ucsi->write_ack);
>  
> -	ret = pmic_glink_send(ucsi->client, &req, sizeof(req));
> +	if (!ucsi->ucsi->version || ucsi->ucsi->version >= UCSI_VERSION_2_1) {
> +		/* If UCSI version is unknown, use the maximum buffer size */
> +		len = sizeof(req);
> +	} else {
> +		/*
> +		 * If UCSI V1, buffer size should be UCSI_BUF_V1_SIZE so update
> +		 * len accordingly
> +		 */
> +		len = sizeof(struct ucsi_write_buf_req_msg);
> +	}
> +
> +	ret = pmic_glink_send(ucsi->client, &req, len);
>  	if (ret < 0) {
>  		dev_err(ucsi->dev, "failed to send UCSI write request: %d\n", ret);
>  		return ret;
> @@ -216,12 +240,26 @@ static const struct ucsi_operations pmic_glink_ucsi_ops = {
>  
>  static void pmic_glink_ucsi_read_ack(struct pmic_glink_ucsi *ucsi, const void *data, int len)
>  {
> -	const struct ucsi_read_buf_resp_msg *resp = data;
> +	const struct ucsi_v2_read_buf_resp_msg *resp = data;
> +	u32 ret_code, buffer_len;
> +
> +	if (!ucsi->ucsi->version || ucsi->ucsi->version >= UCSI_VERSION_2_1) {
> +		/* If UCSI version is unknown, use the maximum buffer size */
> +		ret_code = resp->ret_code;

No endian change?

> +		buffer_len = UCSI_BUF_V2_SIZE;

Shouldn't you warn about this?  Doesn't the version have to be set?


> +	} else {
> +		/*
> +		 * If UCSI V1, use UCSI_BUF_V1_SIZE buffer size and
> +		 * update ret_code offset accordingly
> +		 */
> +		ret_code = ((struct ucsi_read_buf_resp_msg *)data)->ret_code;

Are you sure the buffer was that big?  Where was that checked?

> +		buffer_len = UCSI_BUF_V1_SIZE;
> +	}
>  
> -	if (resp->ret_code)
> +	if (ret_code)
>  		return;
>  
> -	memcpy(ucsi->read_buf, resp->buf, UCSI_BUF_SIZE);
> +	memcpy(ucsi->read_buf, resp->buf, buffer_len);

Where is the length checked against the "real" length of the buffer
passed in here?  What happens if the device lies?

thanks,

greg k-h

Re: [PATCH v2] usb: typec: ucsi: ucsi_glink: Increase buffer size to support UCSI v2

[Anjelique Melendez]

On 7/15/2025 10:06 PM, Greg KH wrote:
> On Tue, Jul 15, 2025 at 05:52:24PM -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>
>> ---
>> Changes since v1:
>>   - Defined buf size in terms of other UCSI defines
>>   - Removed UCSI_BUF_SIZE and used the explicit v1 or v2 buffer size macros
>>   - Removed Qualcomm copyright
>>   - link: https://lore.kernel.org/all/20250624222922.2010820-1-anjelique.melendez@oss.qualcomm.com/
>> ---
>>   drivers/usb/typec/ucsi/ucsi_glink.c | 58 ++++++++++++++++++++++++-----
>>   1 file changed, 48 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/usb/typec/ucsi/ucsi_glink.c b/drivers/usb/typec/ucsi/ucsi_glink.c
>> index 8af79101a2fc..2918c88e54d2 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
>> @@ -32,13 +32,25 @@ struct ucsi_read_buf_req_msg {
>>   
>>   struct ucsi_read_buf_resp_msg {
>>   	struct pmic_glink_hdr   hdr;
>> -	u8                      buf[UCSI_BUF_SIZE];
>> +	u8                      buf[UCSI_BUF_V1_SIZE];
>> +	u32                     ret_code;
> 
> What is the endian-ness of ret_code as it comes from the device?

ret_code is little endian but since ret_code = 0 on success we only ever 
check " if (ret_code)" and return early if there is any error.

>> +};
>> +
>> +struct ucsi_v2_read_buf_resp_msg {
>> +	struct pmic_glink_hdr   hdr;
>> +	u8                      buf[UCSI_BUF_V2_SIZE];
>>   	u32                     ret_code;
>>   };
>>   
>>   struct ucsi_write_buf_req_msg {
>>   	struct pmic_glink_hdr   hdr;
>> -	u8                      buf[UCSI_BUF_SIZE];
>> +	u8                      buf[UCSI_BUF_V1_SIZE];
>> +	u32                     reserved;
> 
> What is "reserved" for?  What is it set to?  Where is it tested to
> ensure it is set properly?
The reserved property is part of the message format defined by the 
charger FW(PPM). We do not set it explicitly but I will add length 
checks to our write requests to ensure we do not write to this property.
> 
>> +};
>> +
>> +struct ucsi_v2_write_buf_req_msg {
>> +	struct pmic_glink_hdr   hdr;
>> +	u8                      buf[UCSI_BUF_V2_SIZE];
>>   	u32                     reserved;
>>   };
> 
> And for all of these structures, are you sure there are not "holes" in
> them?  As you are using sizeof(), don't they need to be packed?
> 
Ack - will update to be packed

>>   
>> @@ -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];
> 
> Why is this one just v2 and not also v1?
  Read_buf[] is used to read back the read_requests. If we added a 
separate read buffer for v2 we would need to add more logic into 
pmic_glink_ucsi_read() and pmic_glink_ucsi_read_ack(). By setting 
read_buf[] to the maximum buffer size (v2) then we can reuse this buffer 
for v1 and v2 and code reuse will be cleaner.

> 
>>   };
>>   
>>   static int pmic_glink_ucsi_read(struct ucsi *__ucsi, unsigned int offset,
>> @@ -131,8 +143,9 @@ 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 = {};
>> +	struct ucsi_v2_write_buf_req_msg req = {};
>>   	unsigned long left;
>> +	size_t len;
>>   	int ret;
>>   
>>   	req.hdr.owner = PMIC_GLINK_OWNER_USBC;
>> @@ -142,7 +155,18 @@ static int pmic_glink_ucsi_locked_write(struct pmic_glink_ucsi *ucsi, unsigned i
>>   
>>   	reinit_completion(&ucsi->write_ack);
>>   
>> -	ret = pmic_glink_send(ucsi->client, &req, sizeof(req));
>> +	if (!ucsi->ucsi->version || ucsi->ucsi->version >= UCSI_VERSION_2_1) {
>> +		/* If UCSI version is unknown, use the maximum buffer size */
>> +		len = sizeof(req);
>> +	} else {
>> +		/*
>> +		 * If UCSI V1, buffer size should be UCSI_BUF_V1_SIZE so update
>> +		 * len accordingly
>> +		 */
>> +		len = sizeof(struct ucsi_write_buf_req_msg);
>> +	}
>> +
>> +	ret = pmic_glink_send(ucsi->client, &req, len);
>>   	if (ret < 0) {
>>   		dev_err(ucsi->dev, "failed to send UCSI write request: %d\n", ret);
>>   		return ret;
>> @@ -216,12 +240,26 @@ static const struct ucsi_operations pmic_glink_ucsi_ops = {
>>   
>>   static void pmic_glink_ucsi_read_ack(struct pmic_glink_ucsi *ucsi, const void *data, int len)
>>   {
>> -	const struct ucsi_read_buf_resp_msg *resp = data;
>> +	const struct ucsi_v2_read_buf_resp_msg *resp = data;
>> +	u32 ret_code, buffer_len;
>> +
>> +	if (!ucsi->ucsi->version || ucsi->ucsi->version >= UCSI_VERSION_2_1) {
>> +		/* If UCSI version is unknown, use the maximum buffer size */
>> +		ret_code = resp->ret_code;
> 
> No endian change?

We only check if ret_code == 0, so no need for endian change

> 
>> +		buffer_len = UCSI_BUF_V2_SIZE;
> 
> Shouldn't you warn about this?  Doesn't the version have to be set?
Version is set from a read_request during ucsi_register(). This would be 
the first read_request we receive and should be the only time we do not 
know the version. Will add the ucsi->ucsi_registered var to enforce that 
version is unknown ONLY before ucsi_register() otherwise warn/return error.
> 
> 
>> +	} else {
>> +		/*
>> +		 * If UCSI V1, use UCSI_BUF_V1_SIZE buffer size and
>> +		 * update ret_code offset accordingly
>> +		 */
>> +		ret_code = ((struct ucsi_read_buf_resp_msg *)data)->ret_code;
> 
> Are you sure the buffer was that big?  Where was that checked?
ACK - will add length checks

> 
>> +		buffer_len = UCSI_BUF_V1_SIZE;
>> +	}
>>   
>> -	if (resp->ret_code)
>> +	if (ret_code)
>>   		return;
>>   
>> -	memcpy(ucsi->read_buf, resp->buf, UCSI_BUF_SIZE);
>> +	memcpy(ucsi->read_buf, resp->buf, buffer_len);
> 
> Where is the length checked against the "real" length of the buffer
> passed in here?  What happens if the device lies?
> 
Ack - will add length checks
> thanks,
> 
> greg k-h

Re: [PATCH v2] usb: typec: ucsi: ucsi_glink: Increase buffer size to support UCSI v2

[Konrad Dybcio]

On 7/16/25 2:52 AM, 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>
> ---
> Changes since v1:
>  - Defined buf size in terms of other UCSI defines
>  - Removed UCSI_BUF_SIZE and used the explicit v1 or v2 buffer size macros
>  - Removed Qualcomm copyright
>  - link: https://lore.kernel.org/all/20250624222922.2010820-1-anjelique.melendez@oss.qualcomm.com/
> ---

[...]

> +	if (!ucsi->ucsi->version || ucsi->ucsi->version >= UCSI_VERSION_2_1) {

You mention V2 everywhere, but then check for V2_1 here - is that
intended?

Konrad

Re: [PATCH v2] usb: typec: ucsi: ucsi_glink: Increase buffer size to support UCSI v2

[Anjelique Melendez]

On 7/16/2025 3:44 AM, Konrad Dybcio wrote:
> On 7/16/25 2:52 AM, 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>
>> ---
>> Changes since v1:
>>   - Defined buf size in terms of other UCSI defines
>>   - Removed UCSI_BUF_SIZE and used the explicit v1 or v2 buffer size macros
>>   - Removed Qualcomm copyright
>>   - link: https://lore.kernel.org/all/20250624222922.2010820-1-anjelique.melendez@oss.qualcomm.com/
>> ---
> 
> [...]
> 
>> +	if (!ucsi->ucsi->version || ucsi->ucsi->version >= UCSI_VERSION_2_1) {
> 
> You mention V2 everywhere, but then check for V2_1 here - is that
> intended?
> Konrad
ACK - Will update to check for UCSI_VERSION_2_0

Thanks,
Anjelique