mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH v4 0/2] usb: typec: ucsi_glink: Add support UCSI v2
@ 2025-09-24 23:26 Anjelique Melendez
  2025-09-24 23:26 ` [PATCH v4 1/2] usb: typec: ucsi_glink: Update request/response buffers to be packed Anjelique Melendez
  2025-09-24 23:26 ` [PATCH v4 2/2] usb: typec: ucsi_glink: Increase buffer size to support UCSI v2 Anjelique Melendez
  0 siblings, 2 replies; 9+ messages in thread
From: Anjelique Melendez @ 2025-09-24 23:26 UTC (permalink / raw)
  To: heikki.krogerus, gregkh
  Cc: lumag, neil.armstrong, johan+linaro, quic_bjorande, linux-usb,
	linux-kernel, linux-arm-msm

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.

While at it also update the UCSI read/request buffers to be packed.

Changes since v3:
 - Added "Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>" for patch 1/2
 - Updated 
 - Fixed if-else if conditional in pmic_glink_ucsi_read_ack() in patch 2/2
 - link: https://lore.kernel.org/all/20250827201241.3111857-1-anjelique.melendez@oss.qualcomm.com/
Changes since v2:
 - Added "usb: typec: ucsi_glink: Update request/response buffers
   to be packed" patch
 - Added length checks
 - Updated version checks to use UCSI_VERSION_2_0 instead of UCSI_VERSION_2_1
 - link: https://lore.kernel.org/all/20250716005224.312155-1-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/

Anjelique Melendez (2):
  usb: typec: ucsi_glink: Update request/response buffers to be packed
  usb: typec: ucsi_glink: Increase buffer size to support UCSI v2

 drivers/usb/typec/ucsi/ucsi_glink.c | 85 ++++++++++++++++++++++++-----
 1 file changed, 70 insertions(+), 15 deletions(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v4 1/2] usb: typec: ucsi_glink: Update request/response buffers to be packed
  2025-09-24 23:26 [PATCH v4 0/2] usb: typec: ucsi_glink: Add support UCSI v2 Anjelique Melendez
@ 2025-09-24 23:26 ` Anjelique Melendez
  2025-09-24 23:26 ` [PATCH v4 2/2] usb: typec: ucsi_glink: Increase buffer size to support UCSI v2 Anjelique Melendez
  1 sibling, 0 replies; 9+ messages in thread
From: Anjelique Melendez @ 2025-09-24 23:26 UTC (permalink / raw)
  To: heikki.krogerus, gregkh
  Cc: lumag, neil.armstrong, johan+linaro, quic_bjorande, linux-usb,
	linux-kernel, linux-arm-msm

Update the ucsi request/response buffers to be packed to ensure there
are no "holes" in memory while we read/write these structs.

Signed-off-by: Anjelique Melendez <anjelique.melendez@oss.qualcomm.com>
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/ucsi/ucsi_glink.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi_glink.c b/drivers/usb/typec/ucsi/ucsi_glink.c
index 8af79101a2fc..1f9f0d942c1a 100644
--- a/drivers/usb/typec/ucsi/ucsi_glink.c
+++ b/drivers/usb/typec/ucsi/ucsi_glink.c
@@ -30,24 +30,24 @@ struct ucsi_read_buf_req_msg {
 	struct pmic_glink_hdr   hdr;
 };
 
-struct ucsi_read_buf_resp_msg {
+struct __packed ucsi_read_buf_resp_msg {
 	struct pmic_glink_hdr   hdr;
 	u8                      buf[UCSI_BUF_SIZE];
 	u32                     ret_code;
 };
 
-struct ucsi_write_buf_req_msg {
+struct __packed ucsi_write_buf_req_msg {
 	struct pmic_glink_hdr   hdr;
 	u8                      buf[UCSI_BUF_SIZE];
 	u32                     reserved;
 };
 
-struct ucsi_write_buf_resp_msg {
+struct __packed ucsi_write_buf_resp_msg {
 	struct pmic_glink_hdr   hdr;
 	u32                     ret_code;
 };
 
-struct ucsi_notify_ind_msg {
+struct __packed ucsi_notify_ind_msg {
 	struct pmic_glink_hdr   hdr;
 	u32                     notification;
 	u32                     receiver;
-- 
2.34.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v4 2/2] usb: typec: ucsi_glink: Increase buffer size to support UCSI v2
  2025-09-24 23:26 [PATCH v4 0/2] usb: typec: ucsi_glink: Add support UCSI v2 Anjelique Melendez
  2025-09-24 23:26 ` [PATCH v4 1/2] usb: typec: ucsi_glink: Update request/response buffers to be packed Anjelique Melendez
@ 2025-09-24 23:26 ` Anjelique Melendez
  2025-09-25 12:07   ` Konrad Dybcio
                     ` (2 more replies)
  1 sibling, 3 replies; 9+ messages in thread
From: Anjelique Melendez @ 2025-09-24 23:26 UTC (permalink / raw)
  To: heikki.krogerus, gregkh
  Cc: lumag, neil.armstrong, johan+linaro, quic_bjorande, linux-usb,
	linux-kernel, linux-arm-msm

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;
+	} 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);
+
+	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


^ 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: 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-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

* 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

end of thread, other threads:[~2025-10-01  9:42 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-09-24 23:26 [PATCH v4 0/2] usb: typec: ucsi_glink: Add support UCSI v2 Anjelique Melendez
2025-09-24 23:26 ` [PATCH v4 1/2] usb: typec: ucsi_glink: Update request/response buffers to be packed Anjelique Melendez
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 22:07       ` Dmitry Baryshkov
2025-10-01  9:42         ` Konrad Dybcio
2025-09-26  0:50   ` kernel test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®