| 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: <6kbkkiwsp3hzu2u6uezelwufooeeh7efrcc2buydpbziuhr5px@xdp24rhfjqyf>
Date: Sat, 27 Sep 2025 01:07:14 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
To: Anjelique Melendez <anjelique.melendez@....qualcomm.com>
Cc: Konrad Dybcio <konrad.dybcio@....qualcomm.com>,
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 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@....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
Powered by blists - more mailing lists