| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4cbcf312-7697-4725-8fd8-45f2b5b0584f@oss.qualcomm.com>
Date: Fri, 26 Sep 2025 11:19:13 -0700
From: Anjelique Melendez <anjelique.melendez@....qualcomm.com>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>,
Konrad Dybcio <konrad.dybcio@....qualcomm.com>
Cc: heikki.krogerus@...ux.intel.com, gregkh@...uxfoundation.org,
lumag@...nel.org, neil.armstrong@...aro.org, johan+linaro@...nel.org,
quic_bjorande@...cinc.com, linux-usb@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v4 2/2] usb: typec: ucsi_glink: Increase buffer size to
support UCSI v2
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@....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
Powered by blists - more mailing lists