* Re: [PATCH v4 2/2] usb: typec: ucsi_glink: Increase buffer size to support UCSI v2
2025-09-24 23:26 ` [PATCH v4 2/2] usb: typec: ucsi_glink: Increase buffer size to support UCSI v2 Anjelique Melendez
@ 2025-09-25 12:07 ` Konrad Dybcio
2025-09-25 21:43 ` Dmitry Baryshkov
2025-09-26 0:50 ` kernel test robot
2 siblings, 0 replies; 9+ messages in thread
From: Konrad Dybcio @ 2025-09-25 12:07 UTC (permalink / raw)
To: Anjelique Melendez, heikki.krogerus, gregkh
Cc: lumag, neil.armstrong, johan+linaro, quic_bjorande, linux-usb,
linux-kernel, linux-arm-msm
On 9/25/25 1:26 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>
> ---
[...]
> 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));
= {} already zero-initializes the struct
> 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;
> + } else if (ucsi->ucsi->version) {
> + req_len = sizeof(struct ucsi_v1_write_buf_req_msg);
> + max_buf_len = UCSI_BUF_V1_SIZE;
> + } else {
> + return -EINVAL;
> + }
> +
> + if (offset + val_len > max_buf_len)
> + return -EINVAL;
> +
> memcpy(&req.buf[offset], val, val_len);
>
> reinit_completion(&ucsi->write_ack);
>
> - ret = pmic_glink_send(ucsi->client, &req, sizeof(req));
> + ret = pmic_glink_send(ucsi->client, &req, req_len);
This code keeps the 'reserved' field zeored out for v1, but it does so
in a fragile and implicit way :/
> if (ret < 0) {
> dev_err(ucsi->dev, "failed to send UCSI write request: %d\n", ret);
> return ret;
> @@ -216,12 +244,39 @@ 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;
> + u8 *buf = ((struct ucsi_v2_read_buf_resp_msg *)data)->buf;
> + u32 ret_code, max_len;
> + u32 buf_len = 0;
> +
> + if (ucsi->ucsi->version) {
> + if (ucsi->ucsi->version >= UCSI_VERSION_2_0)
> + buf_len = UCSI_BUF_V2_SIZE;
> + else
> + buf_len = UCSI_BUF_V1_SIZE;
> + } else if (!ucsi->ucsi_registered) {
> + /*
> + * If UCSI version is not known yet because device is not registered,
> + * choose buffer size which best fits incoming data
> + */
> + if (len > sizeof(struct pmic_glink_hdr) + UCSI_BUF_V2_SIZE)
> + buf_len = UCSI_BUF_V2_SIZE;
> + else
> + buf_len = UCSI_BUF_V1_SIZE;
> + }
>
> - if (resp->ret_code)
> + max_len = sizeof(struct pmic_glink_hdr) + buf_len + sizeof(u32);
> +
> + if (len > max_len)
> + return;
> +
> + if (buf_len > len - sizeof(struct pmic_glink_hdr) - sizeof(u32))
> + buf_len = len - sizeof(struct pmic_glink_hdr) - sizeof(u32);
Is this expected to happen?
Konrad
> +
> + memcpy(&ret_code, buf + buf_len, sizeof(u32));
> + if (ret_code)
> return;
>
> - memcpy(ucsi->read_buf, resp->buf, UCSI_BUF_SIZE);
> + memcpy(ucsi->read_buf, buf, buf_len);
> complete(&ucsi->read_ack);
> }
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v4 2/2] usb: typec: ucsi_glink: Increase buffer size to support UCSI v2
2025-09-24 23:26 ` [PATCH v4 2/2] usb: typec: ucsi_glink: Increase buffer size to support UCSI v2 Anjelique Melendez
2025-09-25 12:07 ` Konrad Dybcio
@ 2025-09-25 21:43 ` Dmitry Baryshkov
2025-09-26 18:19 ` Anjelique Melendez
2025-09-26 0:50 ` kernel test robot
2 siblings, 1 reply; 9+ messages in thread
From: Dmitry Baryshkov @ 2025-09-25 21:43 UTC (permalink / raw)
To: Anjelique Melendez
Cc: heikki.krogerus, gregkh, lumag, neil.armstrong, johan+linaro,
quic_bjorande, linux-usb, linux-kernel, linux-arm-msm
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.
> + } else if (ucsi->ucsi->version) {
> + req_len = sizeof(struct ucsi_v1_write_buf_req_msg);
> + max_buf_len = UCSI_BUF_V1_SIZE;
> + } else {
> + return -EINVAL;
> + }
> +
> + if (offset + val_len > max_buf_len)
> + return -EINVAL;
> +
> memcpy(&req.buf[offset], val, val_len);
>
> reinit_completion(&ucsi->write_ack);
>
> - ret = pmic_glink_send(ucsi->client, &req, sizeof(req));
> + ret = pmic_glink_send(ucsi->client, &req, req_len);
> if (ret < 0) {
> dev_err(ucsi->dev, "failed to send UCSI write request: %d\n", ret);
> return ret;
> @@ -216,12 +244,39 @@ 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;
> + u8 *buf = ((struct ucsi_v2_read_buf_resp_msg *)data)->buf;
> + u32 ret_code, max_len;
> + u32 buf_len = 0;
> +
> + if (ucsi->ucsi->version) {
> + if (ucsi->ucsi->version >= UCSI_VERSION_2_0)
> + buf_len = UCSI_BUF_V2_SIZE;
> + else
> + buf_len = UCSI_BUF_V1_SIZE;
> + } else if (!ucsi->ucsi_registered) {
> + /*
> + * If UCSI version is not known yet because device is not registered,
> + * choose buffer size which best fits incoming data
> + */
> + if (len > sizeof(struct pmic_glink_hdr) + UCSI_BUF_V2_SIZE)
> + buf_len = UCSI_BUF_V2_SIZE;
> + else
> + buf_len = UCSI_BUF_V1_SIZE;
> + }
>
> - if (resp->ret_code)
> + max_len = sizeof(struct pmic_glink_hdr) + buf_len + sizeof(u32);
> +
> + if (len > max_len)
> + return;
> +
> + if (buf_len > len - sizeof(struct pmic_glink_hdr) - sizeof(u32))
> + buf_len = len - sizeof(struct pmic_glink_hdr) - sizeof(u32);
Comment?
> +
> + memcpy(&ret_code, buf + buf_len, sizeof(u32));
> + if (ret_code)
> return;
>
> - memcpy(ucsi->read_buf, resp->buf, UCSI_BUF_SIZE);
> + memcpy(ucsi->read_buf, buf, buf_len);
> complete(&ucsi->read_ack);
> }
>
> --
> 2.34.1
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v4 2/2] usb: typec: ucsi_glink: Increase buffer size to support UCSI v2
2025-09-25 21:43 ` Dmitry Baryshkov
@ 2025-09-26 18:19 ` Anjelique Melendez
2025-09-26 22:07 ` Dmitry Baryshkov
0 siblings, 1 reply; 9+ messages in thread
From: Anjelique Melendez @ 2025-09-26 18:19 UTC (permalink / raw)
To: Dmitry Baryshkov, Konrad Dybcio
Cc: heikki.krogerus, gregkh, lumag, neil.armstrong, johan+linaro,
quic_bjorande, linux-usb, linux-kernel, linux-arm-msm
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v4 2/2] usb: typec: ucsi_glink: Increase buffer size to support UCSI v2
2025-09-26 18:19 ` Anjelique Melendez
@ 2025-09-26 22:07 ` Dmitry Baryshkov
2025-10-01 9:42 ` Konrad Dybcio
0 siblings, 1 reply; 9+ messages in thread
From: Dmitry Baryshkov @ 2025-09-26 22:07 UTC (permalink / raw)
To: Anjelique Melendez
Cc: Konrad Dybcio, heikki.krogerus, gregkh, lumag, neil.armstrong,
johan+linaro, quic_bjorande, linux-usb, linux-kernel,
linux-arm-msm
On Fri, Sep 26, 2025 at 11:19:13AM -0700, Anjelique Melendez wrote:
>
>
> 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;
> };
LGTM.
>
> 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
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v4 2/2] usb: typec: ucsi_glink: Increase buffer size to support UCSI v2
2025-09-26 22:07 ` Dmitry Baryshkov
@ 2025-10-01 9:42 ` Konrad Dybcio
0 siblings, 0 replies; 9+ messages in thread
From: Konrad Dybcio @ 2025-10-01 9:42 UTC (permalink / raw)
To: Dmitry Baryshkov, Anjelique Melendez
Cc: heikki.krogerus, gregkh, lumag, neil.armstrong, johan+linaro,
quic_bjorande, linux-usb, linux-kernel, linux-arm-msm
On 9/27/25 12:07 AM, Dmitry Baryshkov wrote:
> On Fri, Sep 26, 2025 at 11:19:13AM -0700, Anjelique Melendez wrote:
>>
>>
>> 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>
>>>> ---
[...]
>>> 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;
>> };
>
> LGTM.
+1
Konrad
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 2/2] usb: typec: ucsi_glink: Increase buffer size to support UCSI v2
2025-09-24 23:26 ` [PATCH v4 2/2] usb: typec: ucsi_glink: Increase buffer size to support UCSI v2 Anjelique Melendez
2025-09-25 12:07 ` Konrad Dybcio
2025-09-25 21:43 ` Dmitry Baryshkov
@ 2025-09-26 0:50 ` kernel test robot
2 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2025-09-26 0:50 UTC (permalink / raw)
To: Anjelique Melendez, heikki.krogerus, gregkh
Cc: oe-kbuild-all, lumag, neil.armstrong, johan+linaro,
quic_bjorande, linux-usb, linux-kernel, linux-arm-msm
Hi Anjelique,
kernel test robot noticed the following build warnings:
[auto build test WARNING on usb/usb-testing]
[also build test WARNING on usb/usb-next usb/usb-linus linus/master v6.17-rc7 next-20250925]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Anjelique-Melendez/usb-typec-ucsi_glink-Update-request-response-buffers-to-be-packed/20250925-074205
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link: https://lore.kernel.org/r/20250924232631.644234-3-anjelique.melendez%40oss.qualcomm.com
patch subject: [PATCH v4 2/2] usb: typec: ucsi_glink: Increase buffer size to support UCSI v2
config: i386-randconfig-061-20250926 (https://download.01.org/0day-ci/archive/20250926/202509260852.9BZHHRAh-lkp@intel.com/config)
compiler: gcc-14 (Debian 14.2.0-19) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250926/202509260852.9BZHHRAh-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202509260852.9BZHHRAh-lkp@intel.com/
sparse warnings: (new ones prefixed by >>)
drivers/usb/typec/ucsi/ucsi_glink.c:98:23: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted __le32 [usertype] owner @@ got int @@
drivers/usb/typec/ucsi/ucsi_glink.c:98:23: sparse: expected restricted __le32 [usertype] owner
drivers/usb/typec/ucsi/ucsi_glink.c:98:23: sparse: got int
drivers/usb/typec/ucsi/ucsi_glink.c:99:22: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted __le32 [usertype] type @@ got int @@
drivers/usb/typec/ucsi/ucsi_glink.c:99:22: sparse: expected restricted __le32 [usertype] type
drivers/usb/typec/ucsi/ucsi_glink.c:99:22: sparse: got int
drivers/usb/typec/ucsi/ucsi_glink.c:100:24: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted __le32 [usertype] opcode @@ got int @@
drivers/usb/typec/ucsi/ucsi_glink.c:100:24: sparse: expected restricted __le32 [usertype] opcode
drivers/usb/typec/ucsi/ucsi_glink.c:100:24: sparse: got int
>> drivers/usb/typec/ucsi/ucsi_glink.c:152:23: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted __le32 [addressable] [usertype] owner @@ got int @@
drivers/usb/typec/ucsi/ucsi_glink.c:152:23: sparse: expected restricted __le32 [addressable] [usertype] owner
drivers/usb/typec/ucsi/ucsi_glink.c:152:23: sparse: got int
>> drivers/usb/typec/ucsi/ucsi_glink.c:153:22: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted __le32 [addressable] [usertype] type @@ got int @@
drivers/usb/typec/ucsi/ucsi_glink.c:153:22: sparse: expected restricted __le32 [addressable] [usertype] type
drivers/usb/typec/ucsi/ucsi_glink.c:153:22: sparse: got int
>> drivers/usb/typec/ucsi/ucsi_glink.c:154:24: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted __le32 [addressable] [usertype] opcode @@ got int @@
drivers/usb/typec/ucsi/ucsi_glink.c:154:24: sparse: expected restricted __le32 [addressable] [usertype] opcode
drivers/usb/typec/ucsi/ucsi_glink.c:154:24: sparse: got int
vim +152 drivers/usb/typec/ucsi/ucsi_glink.c
467399d989d799 Dmitry Baryshkov 2024-06-27 142
62b5412b1f4afa Neil Armstrong 2023-03-21 143 static int pmic_glink_ucsi_locked_write(struct pmic_glink_ucsi *ucsi, unsigned int offset,
62b5412b1f4afa Neil Armstrong 2023-03-21 144 const void *val, size_t val_len)
62b5412b1f4afa Neil Armstrong 2023-03-21 145 {
87289544ef0b29 Anjelique Melendez 2025-09-24 146 struct ucsi_v2_write_buf_req_msg req = {};
87289544ef0b29 Anjelique Melendez 2025-09-24 147 unsigned long left, max_buf_len;
87289544ef0b29 Anjelique Melendez 2025-09-24 148 size_t req_len;
62b5412b1f4afa Neil Armstrong 2023-03-21 149 int ret;
62b5412b1f4afa Neil Armstrong 2023-03-21 150
87289544ef0b29 Anjelique Melendez 2025-09-24 151 memset(&req, 0, sizeof(req));
62b5412b1f4afa Neil Armstrong 2023-03-21 @152 req.hdr.owner = PMIC_GLINK_OWNER_USBC;
62b5412b1f4afa Neil Armstrong 2023-03-21 @153 req.hdr.type = MSG_TYPE_REQ_RESP;
62b5412b1f4afa Neil Armstrong 2023-03-21 @154 req.hdr.opcode = UC_UCSI_WRITE_BUF_REQ;
87289544ef0b29 Anjelique Melendez 2025-09-24 155
87289544ef0b29 Anjelique Melendez 2025-09-24 156 if (ucsi->ucsi->version >= UCSI_VERSION_2_0) {
87289544ef0b29 Anjelique Melendez 2025-09-24 157 req_len = sizeof(struct ucsi_v2_write_buf_req_msg);
87289544ef0b29 Anjelique Melendez 2025-09-24 158 max_buf_len = UCSI_BUF_V2_SIZE;
87289544ef0b29 Anjelique Melendez 2025-09-24 159 } else if (ucsi->ucsi->version) {
87289544ef0b29 Anjelique Melendez 2025-09-24 160 req_len = sizeof(struct ucsi_v1_write_buf_req_msg);
87289544ef0b29 Anjelique Melendez 2025-09-24 161 max_buf_len = UCSI_BUF_V1_SIZE;
87289544ef0b29 Anjelique Melendez 2025-09-24 162 } else {
87289544ef0b29 Anjelique Melendez 2025-09-24 163 return -EINVAL;
87289544ef0b29 Anjelique Melendez 2025-09-24 164 }
87289544ef0b29 Anjelique Melendez 2025-09-24 165
87289544ef0b29 Anjelique Melendez 2025-09-24 166 if (offset + val_len > max_buf_len)
87289544ef0b29 Anjelique Melendez 2025-09-24 167 return -EINVAL;
87289544ef0b29 Anjelique Melendez 2025-09-24 168
62b5412b1f4afa Neil Armstrong 2023-03-21 169 memcpy(&req.buf[offset], val, val_len);
62b5412b1f4afa Neil Armstrong 2023-03-21 170
62b5412b1f4afa Neil Armstrong 2023-03-21 171 reinit_completion(&ucsi->write_ack);
62b5412b1f4afa Neil Armstrong 2023-03-21 172
87289544ef0b29 Anjelique Melendez 2025-09-24 173 ret = pmic_glink_send(ucsi->client, &req, req_len);
62b5412b1f4afa Neil Armstrong 2023-03-21 174 if (ret < 0) {
62b5412b1f4afa Neil Armstrong 2023-03-21 175 dev_err(ucsi->dev, "failed to send UCSI write request: %d\n", ret);
62b5412b1f4afa Neil Armstrong 2023-03-21 176 return ret;
62b5412b1f4afa Neil Armstrong 2023-03-21 177 }
62b5412b1f4afa Neil Armstrong 2023-03-21 178
62b5412b1f4afa Neil Armstrong 2023-03-21 179 left = wait_for_completion_timeout(&ucsi->write_ack, 5 * HZ);
62b5412b1f4afa Neil Armstrong 2023-03-21 180 if (!left) {
62b5412b1f4afa Neil Armstrong 2023-03-21 181 dev_err(ucsi->dev, "timeout waiting for UCSI write response\n");
62b5412b1f4afa Neil Armstrong 2023-03-21 182 return -ETIMEDOUT;
62b5412b1f4afa Neil Armstrong 2023-03-21 183 }
62b5412b1f4afa Neil Armstrong 2023-03-21 184
62b5412b1f4afa Neil Armstrong 2023-03-21 185 return 0;
62b5412b1f4afa Neil Armstrong 2023-03-21 186 }
62b5412b1f4afa Neil Armstrong 2023-03-21 187
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 9+ messages in thread