Re: [PATCH v4 2/2] usb: typec: ucsi_glink: Increase buffer size to support UCSI v2
From: Anjelique Melendez
Date: Fri Sep 26 2025 - 14:19:17 EST
On 9/25/2025 2:43 PM, Dmitry Baryshkov wrote:
On Wed, Sep 24, 2025 at 04:26:31PM -0700, Anjelique Melendez wrote: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@xxxxxxxxxxxxxxxx/#t)
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@xxxxxxxxxxxxxxxx>
---
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.
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