[PATCH v5 0/4] usb: typec: ucsi: Add support for SET_PDOS command

[PATCH v5 0/4] usb: typec: ucsi: Add support for SET_PDOS command

[Pooja Katiyar]

This series implements support for UCSI SET_PDOS command. It provides
interface to send message out data structure and update source or sink
capabilities PDOs on a connector over debugfs interface.

It also updates UCSI structure to have message in and message out fields
instead of handling them as separate parameters.

Changelog v5:
- Rebased patches to v6.18-rc3.

Changelog v4:
- Fixed build errors reported by kernel test robot.
- Added changelogs.

Pooja Katiyar (4):
  usb: typec: ucsi: Update UCSI structure to have message in and message
    out fields
  usb: typec: ucsi: Add support for message out data structure
  usb: typec: ucsi: Enable debugfs for message_out data structure
  usb: typec: ucsi: Add support for SET_PDOS command

 drivers/usb/typec/ucsi/cros_ec_ucsi.c   |   5 +-
 drivers/usb/typec/ucsi/debugfs.c        |  36 +++++++-
 drivers/usb/typec/ucsi/displayport.c    |  11 ++-
 drivers/usb/typec/ucsi/ucsi.c           | 118 ++++++++++++++++--------
 drivers/usb/typec/ucsi/ucsi.h           |  22 +++--
 drivers/usb/typec/ucsi/ucsi_acpi.c      |  25 ++++-
 drivers/usb/typec/ucsi/ucsi_ccg.c       |  11 +--
 drivers/usb/typec/ucsi/ucsi_yoga_c630.c |  15 ++-
 8 files changed, 172 insertions(+), 71 deletions(-)

-- 
2.43.0

[PATCH v5 1/4] usb: typec: ucsi: Update UCSI structure to have message in and message out fields

[Pooja Katiyar]

Update UCSI structure by adding fields for incoming and outgoing
messages. Update .sync_control function and other related functions
to use these new fields within the UCSI structure, instead of handling
them as separate parameters.

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Pooja Katiyar <pooja.katiyar@intel.com>
---
Changelog v3:
- Added message fields to UCSI structure and updated sync_control handling.

 drivers/usb/typec/ucsi/cros_ec_ucsi.c   |   5 +-
 drivers/usb/typec/ucsi/debugfs.c        |   9 +-
 drivers/usb/typec/ucsi/displayport.c    |  11 ++-
 drivers/usb/typec/ucsi/ucsi.c           | 104 ++++++++++++++++--------
 drivers/usb/typec/ucsi/ucsi.h           |  19 +++--
 drivers/usb/typec/ucsi/ucsi_acpi.c      |   9 +-
 drivers/usb/typec/ucsi/ucsi_ccg.c       |  11 ++-
 drivers/usb/typec/ucsi/ucsi_yoga_c630.c |  15 ++--
 8 files changed, 112 insertions(+), 71 deletions(-)

diff --git a/drivers/usb/typec/ucsi/cros_ec_ucsi.c b/drivers/usb/typec/ucsi/cros_ec_ucsi.c
index eed2a7d0ebc6..d753f2188e25 100644
--- a/drivers/usb/typec/ucsi/cros_ec_ucsi.c
+++ b/drivers/usb/typec/ucsi/cros_ec_ucsi.c
@@ -105,13 +105,12 @@ static int cros_ucsi_async_control(struct ucsi *ucsi, u64 cmd)
 	return 0;
 }
 
-static int cros_ucsi_sync_control(struct ucsi *ucsi, u64 cmd, u32 *cci,
-				  void *data, size_t size)
+static int cros_ucsi_sync_control(struct ucsi *ucsi, u64 cmd, u32 *cci)
 {
 	struct cros_ucsi_data *udata = ucsi_get_drvdata(ucsi);
 	int ret;
 
-	ret = ucsi_sync_control_common(ucsi, cmd, cci, data, size);
+	ret = ucsi_sync_control_common(ucsi, cmd, cci);
 	switch (ret) {
 	case -EBUSY:
 		/* EC may return -EBUSY if CCI.busy is set.
diff --git a/drivers/usb/typec/ucsi/debugfs.c b/drivers/usb/typec/ucsi/debugfs.c
index f73f2b54554e..4bf3a8f65a4e 100644
--- a/drivers/usb/typec/ucsi/debugfs.c
+++ b/drivers/usb/typec/ucsi/debugfs.c
@@ -36,7 +36,8 @@ static int ucsi_cmd(void *data, u64 val)
 	case UCSI_SET_NEW_CAM:
 	case UCSI_SET_USB:
 	case UCSI_READ_POWER_LEVEL:
-		ret = ucsi_send_command(ucsi, val, NULL, 0);
+		ucsi->message_in_size = 0;
+		ret = ucsi_send_command(ucsi, val);
 		break;
 	case UCSI_GET_CAPABILITY:
 	case UCSI_GET_CONNECTOR_CAPABILITY:
@@ -51,9 +52,9 @@ static int ucsi_cmd(void *data, u64 val)
 	case UCSI_GET_ATTENTION_VDO:
 	case UCSI_GET_CAM_CS:
 	case UCSI_GET_LPM_PPM_INFO:
-		ret = ucsi_send_command(ucsi, val,
-					&ucsi->debugfs->response,
-					sizeof(ucsi->debugfs->response));
+		ucsi->message_in_size = sizeof(ucsi->debugfs->response);
+		ret = ucsi_send_command(ucsi, val);
+		memcpy(&ucsi->debugfs->response, ucsi->message_in, sizeof(ucsi->debugfs->response));
 		break;
 	default:
 		ret = -EOPNOTSUPP;
diff --git a/drivers/usb/typec/ucsi/displayport.c b/drivers/usb/typec/ucsi/displayport.c
index 8aae80b457d7..a09b4900ec76 100644
--- a/drivers/usb/typec/ucsi/displayport.c
+++ b/drivers/usb/typec/ucsi/displayport.c
@@ -67,11 +67,14 @@ static int ucsi_displayport_enter(struct typec_altmode *alt, u32 *vdo)
 	}
 
 	command = UCSI_GET_CURRENT_CAM | UCSI_CONNECTOR_NUMBER(dp->con->num);
-	ret = ucsi_send_command(ucsi, command, &cur, sizeof(cur));
+	ucsi->message_in_size = sizeof(cur);
+	ret = ucsi_send_command(ucsi, command);
 	if (ret < 0) {
 		if (ucsi->version > 0x0100)
 			goto err_unlock;
 		cur = 0xff;
+	} else {
+		memcpy(&cur, ucsi->message_in, ucsi->message_in_size);
 	}
 
 	if (cur != 0xff) {
@@ -126,7 +129,8 @@ static int ucsi_displayport_exit(struct typec_altmode *alt)
 	}
 
 	command = UCSI_CMD_SET_NEW_CAM(dp->con->num, 0, dp->offset, 0);
-	ret = ucsi_send_command(dp->con->ucsi, command, NULL, 0);
+	dp->con->ucsi->message_in_size = 0;
+	ret = ucsi_send_command(dp->con->ucsi, command);
 	if (ret < 0)
 		goto out_unlock;
 
@@ -193,7 +197,8 @@ static int ucsi_displayport_configure(struct ucsi_dp *dp)
 
 	command = UCSI_CMD_SET_NEW_CAM(dp->con->num, 1, dp->offset, pins);
 
-	return ucsi_send_command(dp->con->ucsi, command, NULL, 0);
+	dp->con->ucsi->message_in_size = 0;
+	return ucsi_send_command(dp->con->ucsi, command);
 }
 
 static int ucsi_displayport_vdm(struct typec_altmode *alt,
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index ed23edab7763..ee0c0d77e67d 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -55,8 +55,7 @@ void ucsi_notify_common(struct ucsi *ucsi, u32 cci)
 }
 EXPORT_SYMBOL_GPL(ucsi_notify_common);
 
-int ucsi_sync_control_common(struct ucsi *ucsi, u64 command, u32 *cci,
-			     void *data, size_t size)
+int ucsi_sync_control_common(struct ucsi *ucsi, u64 command, u32 *cci)
 {
 	bool ack = UCSI_COMMAND(command) == UCSI_ACK_CC_CI;
 	int ret;
@@ -84,9 +83,10 @@ int ucsi_sync_control_common(struct ucsi *ucsi, u64 command, u32 *cci,
 	if (!ret && cci)
 		ret = ucsi->ops->read_cci(ucsi, cci);
 
-	if (!ret && data &&
+	if (!ret && ucsi->message_in_size > 0 &&
 	    (*cci & UCSI_CCI_COMMAND_COMPLETE))
-		ret = ucsi->ops->read_message_in(ucsi, data, size);
+		ret = ucsi->ops->read_message_in(ucsi, ucsi->message_in,
+						 ucsi->message_in_size);
 
 	return ret;
 }
@@ -103,23 +103,25 @@ static int ucsi_acknowledge(struct ucsi *ucsi, bool conn_ack)
 		ctrl |= UCSI_ACK_CONNECTOR_CHANGE;
 	}
 
-	return ucsi->ops->sync_control(ucsi, ctrl, NULL, NULL, 0);
+	ucsi->message_in_size = 0;
+	return ucsi->ops->sync_control(ucsi, ctrl, NULL);
 }
 
-static int ucsi_run_command(struct ucsi *ucsi, u64 command, u32 *cci,
-			    void *data, size_t size, bool conn_ack)
+static int ucsi_run_command(struct ucsi *ucsi, u64 command, u32 *cci, bool conn_ack)
 {
 	int ret, err;
 
 	*cci = 0;
 
-	if (size > UCSI_MAX_DATA_LENGTH(ucsi))
+	if (ucsi->message_in_size > UCSI_MAX_DATA_LENGTH(ucsi))
 		return -EINVAL;
 
-	ret = ucsi->ops->sync_control(ucsi, command, cci, data, size);
+	ret = ucsi->ops->sync_control(ucsi, command, cci);
 
-	if (*cci & UCSI_CCI_BUSY)
-		return ucsi_run_command(ucsi, UCSI_CANCEL, cci, NULL, 0, false) ?: -EBUSY;
+	if (*cci & UCSI_CCI_BUSY) {
+		ucsi->message_in_size = 0;
+		return ucsi_run_command(ucsi, UCSI_CANCEL, cci, false) ?: -EBUSY;
+	}
 	if (ret)
 		return ret;
 
@@ -151,10 +153,13 @@ static int ucsi_read_error(struct ucsi *ucsi, u8 connector_num)
 	int ret;
 
 	command = UCSI_GET_ERROR_STATUS | UCSI_CONNECTOR_NUMBER(connector_num);
-	ret = ucsi_run_command(ucsi, command, &cci, &error, sizeof(error), false);
+	ucsi->message_in_size = sizeof(error);
+	ret = ucsi_run_command(ucsi, command, &cci, false);
 	if (ret < 0)
 		return ret;
 
+	memcpy(&error, ucsi->message_in, sizeof(error));
+
 	switch (error) {
 	case UCSI_ERROR_INCOMPATIBLE_PARTNER:
 		return -EOPNOTSUPP;
@@ -200,8 +205,7 @@ static int ucsi_read_error(struct ucsi *ucsi, u8 connector_num)
 	return -EIO;
 }
 
-static int ucsi_send_command_common(struct ucsi *ucsi, u64 cmd,
-				    void *data, size_t size, bool conn_ack)
+static int ucsi_send_command_common(struct ucsi *ucsi, u64 cmd, bool conn_ack)
 {
 	u8 connector_num;
 	u32 cci;
@@ -229,7 +233,7 @@ static int ucsi_send_command_common(struct ucsi *ucsi, u64 cmd,
 
 	mutex_lock(&ucsi->ppm_lock);
 
-	ret = ucsi_run_command(ucsi, cmd, &cci, data, size, conn_ack);
+	ret = ucsi_run_command(ucsi, cmd, &cci, conn_ack);
 
 	if (cci & UCSI_CCI_ERROR)
 		ret = ucsi_read_error(ucsi, connector_num);
@@ -238,10 +242,9 @@ static int ucsi_send_command_common(struct ucsi *ucsi, u64 cmd,
 	return ret;
 }
 
-int ucsi_send_command(struct ucsi *ucsi, u64 command,
-		      void *data, size_t size)
+int ucsi_send_command(struct ucsi *ucsi, u64 command)
 {
-	return ucsi_send_command_common(ucsi, command, data, size, false);
+	return ucsi_send_command_common(ucsi, command, false);
 }
 EXPORT_SYMBOL_GPL(ucsi_send_command);
 
@@ -319,7 +322,8 @@ void ucsi_altmode_update_active(struct ucsi_connector *con)
 	int i;
 
 	command = UCSI_GET_CURRENT_CAM | UCSI_CONNECTOR_NUMBER(con->num);
-	ret = ucsi_send_command(con->ucsi, command, &cur, sizeof(cur));
+	con->ucsi->message_in_size = sizeof(cur);
+	ret = ucsi_send_command(con->ucsi, command);
 	if (ret < 0) {
 		if (con->ucsi->version > 0x0100) {
 			dev_err(con->ucsi->dev,
@@ -327,6 +331,8 @@ void ucsi_altmode_update_active(struct ucsi_connector *con)
 			return;
 		}
 		cur = 0xff;
+	} else {
+		memcpy(&cur, con->ucsi->message_in, sizeof(cur));
 	}
 
 	if (cur < UCSI_MAX_ALTMODES)
@@ -510,7 +516,8 @@ ucsi_register_altmodes_nvidia(struct ucsi_connector *con, u8 recipient)
 		command |= UCSI_GET_ALTMODE_RECIPIENT(recipient);
 		command |= UCSI_GET_ALTMODE_CONNECTOR_NUMBER(con->num);
 		command |= UCSI_GET_ALTMODE_OFFSET(i);
-		len = ucsi_send_command(con->ucsi, command, &alt, sizeof(alt));
+		ucsi->message_in_size = sizeof(alt);
+		len = ucsi_send_command(con->ucsi, command);
 		/*
 		 * We are collecting all altmodes first and then registering.
 		 * Some type-C device will return zero length data beyond last
@@ -519,6 +526,8 @@ ucsi_register_altmodes_nvidia(struct ucsi_connector *con, u8 recipient)
 		if (len < 0)
 			return len;
 
+		memcpy(&alt, ucsi->message_in, sizeof(alt));
+
 		/* We got all altmodes, now break out and register them */
 		if (!len || !alt.svid)
 			break;
@@ -586,12 +595,15 @@ static int ucsi_register_altmodes(struct ucsi_connector *con, u8 recipient)
 		command |= UCSI_GET_ALTMODE_RECIPIENT(recipient);
 		command |= UCSI_GET_ALTMODE_CONNECTOR_NUMBER(con->num);
 		command |= UCSI_GET_ALTMODE_OFFSET(i);
-		len = ucsi_send_command(con->ucsi, command, alt, sizeof(alt));
+		con->ucsi->message_in_size = sizeof(alt);
+		len = ucsi_send_command(con->ucsi, command);
 		if (len == -EBUSY)
 			continue;
 		if (len <= 0)
 			return len;
 
+		memcpy(&alt, con->ucsi->message_in, sizeof(alt));
+
 		/*
 		 * This code is requesting one alt mode at a time, but some PPMs
 		 * may still return two. If that happens both alt modes need be
@@ -659,7 +671,9 @@ static int ucsi_get_connector_status(struct ucsi_connector *con, bool conn_ack)
 			  UCSI_MAX_DATA_LENGTH(con->ucsi));
 	int ret;
 
-	ret = ucsi_send_command_common(con->ucsi, command, &con->status, size, conn_ack);
+	con->ucsi->message_in_size = size;
+	ret = ucsi_send_command_common(con->ucsi, command, conn_ack);
+	memcpy(&con->status, con->ucsi->message_in, size);
 
 	return ret < 0 ? ret : 0;
 }
@@ -682,8 +696,9 @@ static int ucsi_read_pdos(struct ucsi_connector *con,
 	command |= UCSI_GET_PDOS_PDO_OFFSET(offset);
 	command |= UCSI_GET_PDOS_NUM_PDOS(num_pdos - 1);
 	command |= is_source(role) ? UCSI_GET_PDOS_SRC_PDOS : 0;
-	ret = ucsi_send_command(ucsi, command, pdos + offset,
-				num_pdos * sizeof(u32));
+	ucsi->message_in_size = num_pdos * sizeof(u32);
+	ret = ucsi_send_command(ucsi, command);
+	memcpy(pdos + offset, ucsi->message_in, num_pdos * sizeof(u32));
 	if (ret < 0 && ret != -ETIMEDOUT)
 		dev_err(ucsi->dev, "UCSI_GET_PDOS failed (%d)\n", ret);
 
@@ -770,7 +785,9 @@ static int ucsi_get_pd_message(struct ucsi_connector *con, u8 recipient,
 		command |= UCSI_GET_PD_MESSAGE_BYTES(len);
 		command |= UCSI_GET_PD_MESSAGE_TYPE(type);
 
-		ret = ucsi_send_command(con->ucsi, command, data + offset, len);
+		con->ucsi->message_in_size = len;
+		ret = ucsi_send_command(con->ucsi, command);
+		memcpy(data + offset, con->ucsi->message_in, len);
 		if (ret < 0)
 			return ret;
 	}
@@ -935,7 +952,9 @@ static int ucsi_register_cable(struct ucsi_connector *con)
 	int ret;
 
 	command = UCSI_GET_CABLE_PROPERTY | UCSI_CONNECTOR_NUMBER(con->num);
-	ret = ucsi_send_command(con->ucsi, command, &cable_prop, sizeof(cable_prop));
+	con->ucsi->message_in_size = sizeof(cable_prop);
+	ret = ucsi_send_command(con->ucsi, command);
+	memcpy(&cable_prop, con->ucsi->message_in, sizeof(cable_prop));
 	if (ret < 0) {
 		dev_err(con->ucsi->dev, "GET_CABLE_PROPERTY failed (%d)\n", ret);
 		return ret;
@@ -996,7 +1015,9 @@ static int ucsi_check_connector_capability(struct ucsi_connector *con)
 		return 0;
 
 	command = UCSI_GET_CONNECTOR_CAPABILITY | UCSI_CONNECTOR_NUMBER(con->num);
-	ret = ucsi_send_command(con->ucsi, command, &con->cap, sizeof(con->cap));
+	con->ucsi->message_in_size = sizeof(con->cap);
+	ret = ucsi_send_command(con->ucsi, command);
+	memcpy(&con->cap, con->ucsi->message_in, sizeof(con->cap));
 	if (ret < 0) {
 		dev_err(con->ucsi->dev, "GET_CONNECTOR_CAPABILITY failed (%d)\n", ret);
 		return ret;
@@ -1357,7 +1378,8 @@ static int ucsi_reset_connector(struct ucsi_connector *con, bool hard)
 	else if (con->ucsi->version >= UCSI_VERSION_2_0)
 		command |= hard ? 0 : UCSI_CONNECTOR_RESET_DATA_VER_2_0;
 
-	return ucsi_send_command(con->ucsi, command, NULL, 0);
+	con->ucsi->message_in_size = 0;
+	return ucsi_send_command(con->ucsi, command);
 }
 
 static int ucsi_reset_ppm(struct ucsi *ucsi)
@@ -1438,7 +1460,8 @@ static int ucsi_role_cmd(struct ucsi_connector *con, u64 command)
 {
 	int ret;
 
-	ret = ucsi_send_command(con->ucsi, command, NULL, 0);
+	con->ucsi->message_in_size = 0;
+	ret = ucsi_send_command(con->ucsi, command);
 	if (ret == -ETIMEDOUT) {
 		u64 c;
 
@@ -1446,7 +1469,8 @@ static int ucsi_role_cmd(struct ucsi_connector *con, u64 command)
 		ucsi_reset_ppm(con->ucsi);
 
 		c = UCSI_SET_NOTIFICATION_ENABLE | con->ucsi->ntfy;
-		ucsi_send_command(con->ucsi, c, NULL, 0);
+		con->ucsi->message_in_size = 0;
+		ucsi_send_command(con->ucsi, c);
 
 		ucsi_reset_connector(con, true);
 	}
@@ -1599,10 +1623,13 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con)
 	/* Get connector capability */
 	command = UCSI_GET_CONNECTOR_CAPABILITY;
 	command |= UCSI_CONNECTOR_NUMBER(con->num);
-	ret = ucsi_send_command(ucsi, command, &con->cap, sizeof(con->cap));
+	ucsi->message_in_size = sizeof(con->cap);
+	ret = ucsi_send_command(ucsi, command);
 	if (ret < 0)
 		goto out_unlock;
 
+	memcpy(&con->cap, ucsi->message_in, sizeof(con->cap));
+
 	if (UCSI_CONCAP(con, OPMODE_DRP))
 		cap->data = TYPEC_PORT_DRD;
 	else if (UCSI_CONCAP(con, OPMODE_DFP))
@@ -1795,17 +1822,20 @@ static int ucsi_init(struct ucsi *ucsi)
 	/* Enable basic notifications */
 	ntfy = UCSI_ENABLE_NTFY_CMD_COMPLETE | UCSI_ENABLE_NTFY_ERROR;
 	command = UCSI_SET_NOTIFICATION_ENABLE | ntfy;
-	ret = ucsi_send_command(ucsi, command, NULL, 0);
+	ucsi->message_in_size = 0;
+	ret = ucsi_send_command(ucsi, command);
 	if (ret < 0)
 		goto err_reset;
 
 	/* Get PPM capabilities */
 	command = UCSI_GET_CAPABILITY;
-	ret = ucsi_send_command(ucsi, command, &ucsi->cap,
-				BITS_TO_BYTES(UCSI_GET_CAPABILITY_SIZE));
+	ucsi->message_in_size = BITS_TO_BYTES(UCSI_GET_CAPABILITY_SIZE);
+	ret = ucsi_send_command(ucsi, command);
 	if (ret < 0)
 		goto err_reset;
 
+	memcpy(&ucsi->cap, ucsi->message_in, BITS_TO_BYTES(UCSI_GET_CAPABILITY_SIZE));
+
 	if (!ucsi->cap.num_connectors) {
 		ret = -ENODEV;
 		goto err_reset;
@@ -1835,7 +1865,8 @@ static int ucsi_init(struct ucsi *ucsi)
 	/* Enable all supported notifications */
 	ntfy = ucsi_get_supported_notifications(ucsi);
 	command = UCSI_SET_NOTIFICATION_ENABLE | ntfy;
-	ret = ucsi_send_command(ucsi, command, NULL, 0);
+	ucsi->message_in_size = 0;
+	ret = ucsi_send_command(ucsi, command);
 	if (ret < 0)
 		goto err_unregister;
 
@@ -1886,7 +1917,8 @@ static void ucsi_resume_work(struct work_struct *work)
 
 	/* Restore UCSI notification enable mask after system resume */
 	command = UCSI_SET_NOTIFICATION_ENABLE | ucsi->ntfy;
-	ret = ucsi_send_command(ucsi, command, NULL, 0);
+	ucsi->message_in_size = 0;
+	ret = ucsi_send_command(ucsi, command);
 	if (ret < 0) {
 		dev_err(ucsi->dev, "failed to re-enable notifications (%d)\n", ret);
 		return;
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index 35993bc34d4d..76343285be59 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -29,6 +29,10 @@ struct dentry;
 #define UCSI_MESSAGE_OUT		32
 #define UCSIv2_MESSAGE_OUT		272
 
+/* Define maximum lengths for message buffers */
+#define UCSI_MAX_MESSAGE_IN_LENGTH	256
+#define UCSI_MAX_MESSAGE_OUT_LENGTH	256
+
 /* UCSI versions */
 #define UCSI_VERSION_1_0	0x0100
 #define UCSI_VERSION_1_1	0x0110
@@ -80,8 +84,7 @@ struct ucsi_operations {
 	int (*read_cci)(struct ucsi *ucsi, u32 *cci);
 	int (*poll_cci)(struct ucsi *ucsi, u32 *cci);
 	int (*read_message_in)(struct ucsi *ucsi, void *val, size_t val_len);
-	int (*sync_control)(struct ucsi *ucsi, u64 command, u32 *cci,
-			    void *data, size_t size);
+	int (*sync_control)(struct ucsi *ucsi, u64 command, u32 *cci);
 	int (*async_control)(struct ucsi *ucsi, u64 command);
 	bool (*update_altmodes)(struct ucsi *ucsi, u8 recipient,
 				struct ucsi_altmode *orig,
@@ -489,6 +492,12 @@ struct ucsi {
 	unsigned long quirks;
 #define UCSI_NO_PARTNER_PDOS	BIT(0)	/* Don't read partner's PDOs */
 #define UCSI_DELAY_DEVICE_PDOS	BIT(1)	/* Reading PDOs fails until the parter is in PD mode */
+
+	/* Fixed-size buffers for incoming and outgoing messages */
+	u8 message_in[UCSI_MAX_MESSAGE_IN_LENGTH];
+	size_t message_in_size;
+	u8 message_out[UCSI_MAX_MESSAGE_OUT_LENGTH];
+	size_t message_out_size;
 };
 
 #define UCSI_MAX_DATA_LENGTH(u) (((u)->version < UCSI_VERSION_2_0) ? 0x10 : 0xff)
@@ -551,15 +560,13 @@ struct ucsi_connector {
 	struct usb_pd_identity cable_identity;
 };
 
-int ucsi_send_command(struct ucsi *ucsi, u64 command,
-		      void *retval, size_t size);
+int ucsi_send_command(struct ucsi *ucsi, u64 command);
 
 void ucsi_altmode_update_active(struct ucsi_connector *con);
 int ucsi_resume(struct ucsi *ucsi);
 
 void ucsi_notify_common(struct ucsi *ucsi, u32 cci);
-int ucsi_sync_control_common(struct ucsi *ucsi, u64 command, u32 *cci,
-			     void *data, size_t size);
+int ucsi_sync_control_common(struct ucsi *ucsi, u64 command, u32 *cci);
 
 #if IS_ENABLED(CONFIG_POWER_SUPPLY)
 int ucsi_register_port_psy(struct ucsi_connector *con);
diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
index 6b92f296e985..f1d1f6917b09 100644
--- a/drivers/usb/typec/ucsi/ucsi_acpi.c
+++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
@@ -105,15 +105,14 @@ static const struct ucsi_operations ucsi_acpi_ops = {
 	.async_control = ucsi_acpi_async_control
 };
 
-static int ucsi_gram_sync_control(struct ucsi *ucsi, u64 command, u32 *cci,
-				  void *val, size_t len)
+static int ucsi_gram_sync_control(struct ucsi *ucsi, u64 command, u32 *cci)
 {
 	u16 bogus_change = UCSI_CONSTAT_POWER_LEVEL_CHANGE |
 			   UCSI_CONSTAT_PDOS_CHANGE;
 	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
 	int ret;
 
-	ret = ucsi_sync_control_common(ucsi, command, cci, val, len);
+	ret = ucsi_sync_control_common(ucsi, command, cci);
 	if (ret < 0)
 		return ret;
 
@@ -125,8 +124,8 @@ static int ucsi_gram_sync_control(struct ucsi *ucsi, u64 command, u32 *cci,
 	if (UCSI_COMMAND(ua->cmd) == UCSI_GET_CONNECTOR_STATUS &&
 	    ua->check_bogus_event) {
 		/* Clear the bogus change */
-		if (*(u16 *)val == bogus_change)
-			*(u16 *)val = 0;
+		if (*(u16 *)ucsi->message_in == bogus_change)
+			*(u16 *)ucsi->message_in = 0;
 
 		ua->check_bogus_event = false;
 	}
diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
index d83a0051c737..ead1b2a25c79 100644
--- a/drivers/usb/typec/ucsi/ucsi_ccg.c
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -606,8 +606,7 @@ static int ucsi_ccg_async_control(struct ucsi *ucsi, u64 command)
 	return ccg_write(uc, reg, (u8 *)&command, sizeof(command));
 }
 
-static int ucsi_ccg_sync_control(struct ucsi *ucsi, u64 command, u32 *cci,
-				 void *data, size_t size)
+static int ucsi_ccg_sync_control(struct ucsi *ucsi, u64 command, u32 *cci)
 {
 	struct ucsi_ccg *uc = ucsi_get_drvdata(ucsi);
 	struct ucsi_connector *con;
@@ -629,16 +628,16 @@ static int ucsi_ccg_sync_control(struct ucsi *ucsi, u64 command, u32 *cci,
 		ucsi_ccg_update_set_new_cam_cmd(uc, con, &command);
 	}
 
-	ret = ucsi_sync_control_common(ucsi, command, cci, data, size);
+	ret = ucsi_sync_control_common(ucsi, command, cci);
 
 	switch (UCSI_COMMAND(command)) {
 	case UCSI_GET_CURRENT_CAM:
 		if (uc->has_multiple_dp)
-			ucsi_ccg_update_get_current_cam_cmd(uc, (u8 *)data);
+			ucsi_ccg_update_get_current_cam_cmd(uc, (u8 *)ucsi->message_in);
 		break;
 	case UCSI_GET_ALTERNATE_MODES:
 		if (UCSI_ALTMODE_RECIPIENT(command) == UCSI_RECIPIENT_SOP) {
-			struct ucsi_altmode *alt = data;
+			struct ucsi_altmode *alt = (struct ucsi_altmode *)ucsi->message_in;
 
 			if (alt[0].svid == USB_TYPEC_NVIDIA_VLINK_SID)
 				ucsi_ccg_nvidia_altmode(uc, alt, command);
@@ -646,7 +645,7 @@ static int ucsi_ccg_sync_control(struct ucsi *ucsi, u64 command, u32 *cci,
 		break;
 	case UCSI_GET_CAPABILITY:
 		if (uc->fw_build == CCG_FW_BUILD_NVIDIA_TEGRA) {
-			struct ucsi_capability *cap = data;
+			struct ucsi_capability *cap = (struct ucsi_capability *)ucsi->message_in;
 
 			cap->features &= ~UCSI_CAP_ALT_MODE_DETAILS;
 		}
diff --git a/drivers/usb/typec/ucsi/ucsi_yoga_c630.c b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
index 0187c1c4b21a..299081444caa 100644
--- a/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
+++ b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
@@ -88,8 +88,7 @@ static int yoga_c630_ucsi_async_control(struct ucsi *ucsi, u64 command)
 
 static int yoga_c630_ucsi_sync_control(struct ucsi *ucsi,
 				       u64 command,
-				       u32 *cci,
-				       void *data, size_t size)
+				       u32 *cci)
 {
 	int ret;
 
@@ -107,8 +106,8 @@ static int yoga_c630_ucsi_sync_control(struct ucsi *ucsi,
 		};
 
 		dev_dbg(ucsi->dev, "faking DP altmode for con1\n");
-		memset(data, 0, size);
-		memcpy(data, &alt, min(sizeof(alt), size));
+		memset(ucsi->message_in, 0, ucsi->message_in_size);
+		memcpy(ucsi->message_in, &alt, min(sizeof(alt), ucsi->message_in_size));
 		*cci = UCSI_CCI_COMMAND_COMPLETE | UCSI_SET_CCI_LENGTH(sizeof(alt));
 		return 0;
 	}
@@ -121,18 +120,18 @@ static int yoga_c630_ucsi_sync_control(struct ucsi *ucsi,
 	if (UCSI_COMMAND(command) == UCSI_GET_ALTERNATE_MODES &&
 	    UCSI_GET_ALTMODE_GET_CONNECTOR_NUMBER(command) == 2) {
 		dev_dbg(ucsi->dev, "ignoring altmodes for con2\n");
-		memset(data, 0, size);
+		memset(ucsi->message_in, 0, ucsi->message_in_size);
 		*cci = UCSI_CCI_COMMAND_COMPLETE;
 		return 0;
 	}
 
-	ret = ucsi_sync_control_common(ucsi, command, cci, data, size);
+	ret = ucsi_sync_control_common(ucsi, command, cci);
 	if (ret < 0)
 		return ret;
 
 	/* UCSI_GET_CURRENT_CAM is off-by-one on all ports */
-	if (UCSI_COMMAND(command) == UCSI_GET_CURRENT_CAM && data)
-		((u8 *)data)[0]--;
+	if (UCSI_COMMAND(command) == UCSI_GET_CURRENT_CAM && ucsi->message_in_size > 0)
+		ucsi->message_in[0]--;
 
 	return ret;
 }
-- 
2.43.0

Re: [PATCH v5 1/4] usb: typec: ucsi: Update UCSI structure to have message in and message out fields

[Dmitry Baryshkov]

On Thu, Oct 30, 2025 at 07:48:55AM -0700, Pooja Katiyar wrote:
> Update UCSI structure by adding fields for incoming and outgoing
> messages. Update .sync_control function and other related functions
> to use these new fields within the UCSI structure, instead of handling
> them as separate parameters.
> 
> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Signed-off-by: Pooja Katiyar <pooja.katiyar@intel.com>
> ---
> Changelog v3:
> - Added message fields to UCSI structure and updated sync_control handling.

Colleagues, after looking at this patch I have a question. What prevents
message_{in,out}{,_size} to be modified concurrently? While we have PPM
lock around command submission, size fields and buffers are modified /
accessed outside of the lock. Granted all the notifications and async
nature of the UCSI and USB-C protocols, what prevents two commands from
being executed at the same time with one of the execution threads
accessing the results returned by the other command?

In other words:

- thread A sets message_in_size, calls ucsi_send_command(CMD_A), which
  takes PPM lock

   - meanwhile thread B writes another value to message_in_size and
     calls ucsi_send_command(CMD_B), which now waits on PPM lock

- thread A finishes execution of the CMD_A, copies data (with the
  wrong size!) to the message_in_buf, then it releases PPM lock.

- thread A gets preempted

    - thread B gets scheduled, it takes PPM lock, executes CMD_B, gets
      data into the message_in_buf and finally it releases PPM lock

- finally at some later point thread A gets scheduled, it accesses
  message_in_buf, reading data that has been overwritten by CMD_B
  execution.

WDYT?

> 
>  drivers/usb/typec/ucsi/cros_ec_ucsi.c   |   5 +-
>  drivers/usb/typec/ucsi/debugfs.c        |   9 +-
>  drivers/usb/typec/ucsi/displayport.c    |  11 ++-
>  drivers/usb/typec/ucsi/ucsi.c           | 104 ++++++++++++++++--------
>  drivers/usb/typec/ucsi/ucsi.h           |  19 +++--
>  drivers/usb/typec/ucsi/ucsi_acpi.c      |   9 +-
>  drivers/usb/typec/ucsi/ucsi_ccg.c       |  11 ++-
>  drivers/usb/typec/ucsi/ucsi_yoga_c630.c |  15 ++--
>  8 files changed, 112 insertions(+), 71 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/cros_ec_ucsi.c b/drivers/usb/typec/ucsi/cros_ec_ucsi.c
> index eed2a7d0ebc6..d753f2188e25 100644
> --- a/drivers/usb/typec/ucsi/cros_ec_ucsi.c
> +++ b/drivers/usb/typec/ucsi/cros_ec_ucsi.c
> @@ -105,13 +105,12 @@ static int cros_ucsi_async_control(struct ucsi *ucsi, u64 cmd)
>  	return 0;
>  }
>  
> -static int cros_ucsi_sync_control(struct ucsi *ucsi, u64 cmd, u32 *cci,
> -				  void *data, size_t size)
> +static int cros_ucsi_sync_control(struct ucsi *ucsi, u64 cmd, u32 *cci)
>  {
>  	struct cros_ucsi_data *udata = ucsi_get_drvdata(ucsi);
>  	int ret;
>  
> -	ret = ucsi_sync_control_common(ucsi, cmd, cci, data, size);
> +	ret = ucsi_sync_control_common(ucsi, cmd, cci);
>  	switch (ret) {
>  	case -EBUSY:
>  		/* EC may return -EBUSY if CCI.busy is set.
> diff --git a/drivers/usb/typec/ucsi/debugfs.c b/drivers/usb/typec/ucsi/debugfs.c
> index f73f2b54554e..4bf3a8f65a4e 100644
> --- a/drivers/usb/typec/ucsi/debugfs.c
> +++ b/drivers/usb/typec/ucsi/debugfs.c
> @@ -36,7 +36,8 @@ static int ucsi_cmd(void *data, u64 val)
>  	case UCSI_SET_NEW_CAM:
>  	case UCSI_SET_USB:
>  	case UCSI_READ_POWER_LEVEL:
> -		ret = ucsi_send_command(ucsi, val, NULL, 0);
> +		ucsi->message_in_size = 0;
> +		ret = ucsi_send_command(ucsi, val);
>  		break;
>  	case UCSI_GET_CAPABILITY:
>  	case UCSI_GET_CONNECTOR_CAPABILITY:
> @@ -51,9 +52,9 @@ static int ucsi_cmd(void *data, u64 val)
>  	case UCSI_GET_ATTENTION_VDO:
>  	case UCSI_GET_CAM_CS:
>  	case UCSI_GET_LPM_PPM_INFO:
> -		ret = ucsi_send_command(ucsi, val,
> -					&ucsi->debugfs->response,
> -					sizeof(ucsi->debugfs->response));
> +		ucsi->message_in_size = sizeof(ucsi->debugfs->response);
> +		ret = ucsi_send_command(ucsi, val);
> +		memcpy(&ucsi->debugfs->response, ucsi->message_in, sizeof(ucsi->debugfs->response));
>  		break;
>  	default:
>  		ret = -EOPNOTSUPP;
> diff --git a/drivers/usb/typec/ucsi/displayport.c b/drivers/usb/typec/ucsi/displayport.c
> index 8aae80b457d7..a09b4900ec76 100644
> --- a/drivers/usb/typec/ucsi/displayport.c
> +++ b/drivers/usb/typec/ucsi/displayport.c
> @@ -67,11 +67,14 @@ static int ucsi_displayport_enter(struct typec_altmode *alt, u32 *vdo)
>  	}
>  
>  	command = UCSI_GET_CURRENT_CAM | UCSI_CONNECTOR_NUMBER(dp->con->num);
> -	ret = ucsi_send_command(ucsi, command, &cur, sizeof(cur));
> +	ucsi->message_in_size = sizeof(cur);
> +	ret = ucsi_send_command(ucsi, command);
>  	if (ret < 0) {
>  		if (ucsi->version > 0x0100)
>  			goto err_unlock;
>  		cur = 0xff;
> +	} else {
> +		memcpy(&cur, ucsi->message_in, ucsi->message_in_size);
>  	}
>  
>  	if (cur != 0xff) {
> @@ -126,7 +129,8 @@ static int ucsi_displayport_exit(struct typec_altmode *alt)
>  	}
>  
>  	command = UCSI_CMD_SET_NEW_CAM(dp->con->num, 0, dp->offset, 0);
> -	ret = ucsi_send_command(dp->con->ucsi, command, NULL, 0);
> +	dp->con->ucsi->message_in_size = 0;
> +	ret = ucsi_send_command(dp->con->ucsi, command);
>  	if (ret < 0)
>  		goto out_unlock;
>  
> @@ -193,7 +197,8 @@ static int ucsi_displayport_configure(struct ucsi_dp *dp)
>  
>  	command = UCSI_CMD_SET_NEW_CAM(dp->con->num, 1, dp->offset, pins);
>  
> -	return ucsi_send_command(dp->con->ucsi, command, NULL, 0);
> +	dp->con->ucsi->message_in_size = 0;
> +	return ucsi_send_command(dp->con->ucsi, command);
>  }
>  
>  static int ucsi_displayport_vdm(struct typec_altmode *alt,
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index ed23edab7763..ee0c0d77e67d 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -55,8 +55,7 @@ void ucsi_notify_common(struct ucsi *ucsi, u32 cci)
>  }
>  EXPORT_SYMBOL_GPL(ucsi_notify_common);
>  
> -int ucsi_sync_control_common(struct ucsi *ucsi, u64 command, u32 *cci,
> -			     void *data, size_t size)
> +int ucsi_sync_control_common(struct ucsi *ucsi, u64 command, u32 *cci)
>  {
>  	bool ack = UCSI_COMMAND(command) == UCSI_ACK_CC_CI;
>  	int ret;
> @@ -84,9 +83,10 @@ int ucsi_sync_control_common(struct ucsi *ucsi, u64 command, u32 *cci,
>  	if (!ret && cci)
>  		ret = ucsi->ops->read_cci(ucsi, cci);
>  
> -	if (!ret && data &&
> +	if (!ret && ucsi->message_in_size > 0 &&
>  	    (*cci & UCSI_CCI_COMMAND_COMPLETE))
> -		ret = ucsi->ops->read_message_in(ucsi, data, size);
> +		ret = ucsi->ops->read_message_in(ucsi, ucsi->message_in,
> +						 ucsi->message_in_size);
>  
>  	return ret;
>  }
> @@ -103,23 +103,25 @@ static int ucsi_acknowledge(struct ucsi *ucsi, bool conn_ack)
>  		ctrl |= UCSI_ACK_CONNECTOR_CHANGE;
>  	}
>  
> -	return ucsi->ops->sync_control(ucsi, ctrl, NULL, NULL, 0);
> +	ucsi->message_in_size = 0;
> +	return ucsi->ops->sync_control(ucsi, ctrl, NULL);
>  }
>  
> -static int ucsi_run_command(struct ucsi *ucsi, u64 command, u32 *cci,
> -			    void *data, size_t size, bool conn_ack)
> +static int ucsi_run_command(struct ucsi *ucsi, u64 command, u32 *cci, bool conn_ack)
>  {
>  	int ret, err;
>  
>  	*cci = 0;
>  
> -	if (size > UCSI_MAX_DATA_LENGTH(ucsi))
> +	if (ucsi->message_in_size > UCSI_MAX_DATA_LENGTH(ucsi))
>  		return -EINVAL;
>  
> -	ret = ucsi->ops->sync_control(ucsi, command, cci, data, size);
> +	ret = ucsi->ops->sync_control(ucsi, command, cci);
>  
> -	if (*cci & UCSI_CCI_BUSY)
> -		return ucsi_run_command(ucsi, UCSI_CANCEL, cci, NULL, 0, false) ?: -EBUSY;
> +	if (*cci & UCSI_CCI_BUSY) {
> +		ucsi->message_in_size = 0;
> +		return ucsi_run_command(ucsi, UCSI_CANCEL, cci, false) ?: -EBUSY;
> +	}
>  	if (ret)
>  		return ret;
>  
> @@ -151,10 +153,13 @@ static int ucsi_read_error(struct ucsi *ucsi, u8 connector_num)
>  	int ret;
>  
>  	command = UCSI_GET_ERROR_STATUS | UCSI_CONNECTOR_NUMBER(connector_num);
> -	ret = ucsi_run_command(ucsi, command, &cci, &error, sizeof(error), false);
> +	ucsi->message_in_size = sizeof(error);
> +	ret = ucsi_run_command(ucsi, command, &cci, false);
>  	if (ret < 0)
>  		return ret;
>  
> +	memcpy(&error, ucsi->message_in, sizeof(error));
> +
>  	switch (error) {
>  	case UCSI_ERROR_INCOMPATIBLE_PARTNER:
>  		return -EOPNOTSUPP;
> @@ -200,8 +205,7 @@ static int ucsi_read_error(struct ucsi *ucsi, u8 connector_num)
>  	return -EIO;
>  }
>  
> -static int ucsi_send_command_common(struct ucsi *ucsi, u64 cmd,
> -				    void *data, size_t size, bool conn_ack)
> +static int ucsi_send_command_common(struct ucsi *ucsi, u64 cmd, bool conn_ack)
>  {
>  	u8 connector_num;
>  	u32 cci;
> @@ -229,7 +233,7 @@ static int ucsi_send_command_common(struct ucsi *ucsi, u64 cmd,
>  
>  	mutex_lock(&ucsi->ppm_lock);
>  
> -	ret = ucsi_run_command(ucsi, cmd, &cci, data, size, conn_ack);
> +	ret = ucsi_run_command(ucsi, cmd, &cci, conn_ack);
>  
>  	if (cci & UCSI_CCI_ERROR)
>  		ret = ucsi_read_error(ucsi, connector_num);
> @@ -238,10 +242,9 @@ static int ucsi_send_command_common(struct ucsi *ucsi, u64 cmd,
>  	return ret;
>  }
>  
> -int ucsi_send_command(struct ucsi *ucsi, u64 command,
> -		      void *data, size_t size)
> +int ucsi_send_command(struct ucsi *ucsi, u64 command)
>  {
> -	return ucsi_send_command_common(ucsi, command, data, size, false);
> +	return ucsi_send_command_common(ucsi, command, false);
>  }
>  EXPORT_SYMBOL_GPL(ucsi_send_command);
>  
> @@ -319,7 +322,8 @@ void ucsi_altmode_update_active(struct ucsi_connector *con)
>  	int i;
>  
>  	command = UCSI_GET_CURRENT_CAM | UCSI_CONNECTOR_NUMBER(con->num);
> -	ret = ucsi_send_command(con->ucsi, command, &cur, sizeof(cur));
> +	con->ucsi->message_in_size = sizeof(cur);
> +	ret = ucsi_send_command(con->ucsi, command);
>  	if (ret < 0) {
>  		if (con->ucsi->version > 0x0100) {
>  			dev_err(con->ucsi->dev,
> @@ -327,6 +331,8 @@ void ucsi_altmode_update_active(struct ucsi_connector *con)
>  			return;
>  		}
>  		cur = 0xff;
> +	} else {
> +		memcpy(&cur, con->ucsi->message_in, sizeof(cur));
>  	}
>  
>  	if (cur < UCSI_MAX_ALTMODES)
> @@ -510,7 +516,8 @@ ucsi_register_altmodes_nvidia(struct ucsi_connector *con, u8 recipient)
>  		command |= UCSI_GET_ALTMODE_RECIPIENT(recipient);
>  		command |= UCSI_GET_ALTMODE_CONNECTOR_NUMBER(con->num);
>  		command |= UCSI_GET_ALTMODE_OFFSET(i);
> -		len = ucsi_send_command(con->ucsi, command, &alt, sizeof(alt));
> +		ucsi->message_in_size = sizeof(alt);
> +		len = ucsi_send_command(con->ucsi, command);
>  		/*
>  		 * We are collecting all altmodes first and then registering.
>  		 * Some type-C device will return zero length data beyond last
> @@ -519,6 +526,8 @@ ucsi_register_altmodes_nvidia(struct ucsi_connector *con, u8 recipient)
>  		if (len < 0)
>  			return len;
>  
> +		memcpy(&alt, ucsi->message_in, sizeof(alt));
> +
>  		/* We got all altmodes, now break out and register them */
>  		if (!len || !alt.svid)
>  			break;
> @@ -586,12 +595,15 @@ static int ucsi_register_altmodes(struct ucsi_connector *con, u8 recipient)
>  		command |= UCSI_GET_ALTMODE_RECIPIENT(recipient);
>  		command |= UCSI_GET_ALTMODE_CONNECTOR_NUMBER(con->num);
>  		command |= UCSI_GET_ALTMODE_OFFSET(i);
> -		len = ucsi_send_command(con->ucsi, command, alt, sizeof(alt));
> +		con->ucsi->message_in_size = sizeof(alt);
> +		len = ucsi_send_command(con->ucsi, command);
>  		if (len == -EBUSY)
>  			continue;
>  		if (len <= 0)
>  			return len;
>  
> +		memcpy(&alt, con->ucsi->message_in, sizeof(alt));
> +
>  		/*
>  		 * This code is requesting one alt mode at a time, but some PPMs
>  		 * may still return two. If that happens both alt modes need be
> @@ -659,7 +671,9 @@ static int ucsi_get_connector_status(struct ucsi_connector *con, bool conn_ack)
>  			  UCSI_MAX_DATA_LENGTH(con->ucsi));
>  	int ret;
>  
> -	ret = ucsi_send_command_common(con->ucsi, command, &con->status, size, conn_ack);
> +	con->ucsi->message_in_size = size;
> +	ret = ucsi_send_command_common(con->ucsi, command, conn_ack);
> +	memcpy(&con->status, con->ucsi->message_in, size);
>  
>  	return ret < 0 ? ret : 0;
>  }
> @@ -682,8 +696,9 @@ static int ucsi_read_pdos(struct ucsi_connector *con,
>  	command |= UCSI_GET_PDOS_PDO_OFFSET(offset);
>  	command |= UCSI_GET_PDOS_NUM_PDOS(num_pdos - 1);
>  	command |= is_source(role) ? UCSI_GET_PDOS_SRC_PDOS : 0;
> -	ret = ucsi_send_command(ucsi, command, pdos + offset,
> -				num_pdos * sizeof(u32));
> +	ucsi->message_in_size = num_pdos * sizeof(u32);
> +	ret = ucsi_send_command(ucsi, command);
> +	memcpy(pdos + offset, ucsi->message_in, num_pdos * sizeof(u32));
>  	if (ret < 0 && ret != -ETIMEDOUT)
>  		dev_err(ucsi->dev, "UCSI_GET_PDOS failed (%d)\n", ret);
>  
> @@ -770,7 +785,9 @@ static int ucsi_get_pd_message(struct ucsi_connector *con, u8 recipient,
>  		command |= UCSI_GET_PD_MESSAGE_BYTES(len);
>  		command |= UCSI_GET_PD_MESSAGE_TYPE(type);
>  
> -		ret = ucsi_send_command(con->ucsi, command, data + offset, len);
> +		con->ucsi->message_in_size = len;
> +		ret = ucsi_send_command(con->ucsi, command);
> +		memcpy(data + offset, con->ucsi->message_in, len);
>  		if (ret < 0)
>  			return ret;
>  	}
> @@ -935,7 +952,9 @@ static int ucsi_register_cable(struct ucsi_connector *con)
>  	int ret;
>  
>  	command = UCSI_GET_CABLE_PROPERTY | UCSI_CONNECTOR_NUMBER(con->num);
> -	ret = ucsi_send_command(con->ucsi, command, &cable_prop, sizeof(cable_prop));
> +	con->ucsi->message_in_size = sizeof(cable_prop);
> +	ret = ucsi_send_command(con->ucsi, command);
> +	memcpy(&cable_prop, con->ucsi->message_in, sizeof(cable_prop));
>  	if (ret < 0) {
>  		dev_err(con->ucsi->dev, "GET_CABLE_PROPERTY failed (%d)\n", ret);
>  		return ret;
> @@ -996,7 +1015,9 @@ static int ucsi_check_connector_capability(struct ucsi_connector *con)
>  		return 0;
>  
>  	command = UCSI_GET_CONNECTOR_CAPABILITY | UCSI_CONNECTOR_NUMBER(con->num);
> -	ret = ucsi_send_command(con->ucsi, command, &con->cap, sizeof(con->cap));
> +	con->ucsi->message_in_size = sizeof(con->cap);
> +	ret = ucsi_send_command(con->ucsi, command);
> +	memcpy(&con->cap, con->ucsi->message_in, sizeof(con->cap));
>  	if (ret < 0) {
>  		dev_err(con->ucsi->dev, "GET_CONNECTOR_CAPABILITY failed (%d)\n", ret);
>  		return ret;
> @@ -1357,7 +1378,8 @@ static int ucsi_reset_connector(struct ucsi_connector *con, bool hard)
>  	else if (con->ucsi->version >= UCSI_VERSION_2_0)
>  		command |= hard ? 0 : UCSI_CONNECTOR_RESET_DATA_VER_2_0;
>  
> -	return ucsi_send_command(con->ucsi, command, NULL, 0);
> +	con->ucsi->message_in_size = 0;
> +	return ucsi_send_command(con->ucsi, command);
>  }
>  
>  static int ucsi_reset_ppm(struct ucsi *ucsi)
> @@ -1438,7 +1460,8 @@ static int ucsi_role_cmd(struct ucsi_connector *con, u64 command)
>  {
>  	int ret;
>  
> -	ret = ucsi_send_command(con->ucsi, command, NULL, 0);
> +	con->ucsi->message_in_size = 0;
> +	ret = ucsi_send_command(con->ucsi, command);
>  	if (ret == -ETIMEDOUT) {
>  		u64 c;
>  
> @@ -1446,7 +1469,8 @@ static int ucsi_role_cmd(struct ucsi_connector *con, u64 command)
>  		ucsi_reset_ppm(con->ucsi);
>  
>  		c = UCSI_SET_NOTIFICATION_ENABLE | con->ucsi->ntfy;
> -		ucsi_send_command(con->ucsi, c, NULL, 0);
> +		con->ucsi->message_in_size = 0;
> +		ucsi_send_command(con->ucsi, c);
>  
>  		ucsi_reset_connector(con, true);
>  	}
> @@ -1599,10 +1623,13 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con)
>  	/* Get connector capability */
>  	command = UCSI_GET_CONNECTOR_CAPABILITY;
>  	command |= UCSI_CONNECTOR_NUMBER(con->num);
> -	ret = ucsi_send_command(ucsi, command, &con->cap, sizeof(con->cap));
> +	ucsi->message_in_size = sizeof(con->cap);
> +	ret = ucsi_send_command(ucsi, command);
>  	if (ret < 0)
>  		goto out_unlock;
>  
> +	memcpy(&con->cap, ucsi->message_in, sizeof(con->cap));
> +
>  	if (UCSI_CONCAP(con, OPMODE_DRP))
>  		cap->data = TYPEC_PORT_DRD;
>  	else if (UCSI_CONCAP(con, OPMODE_DFP))
> @@ -1795,17 +1822,20 @@ static int ucsi_init(struct ucsi *ucsi)
>  	/* Enable basic notifications */
>  	ntfy = UCSI_ENABLE_NTFY_CMD_COMPLETE | UCSI_ENABLE_NTFY_ERROR;
>  	command = UCSI_SET_NOTIFICATION_ENABLE | ntfy;
> -	ret = ucsi_send_command(ucsi, command, NULL, 0);
> +	ucsi->message_in_size = 0;
> +	ret = ucsi_send_command(ucsi, command);
>  	if (ret < 0)
>  		goto err_reset;
>  
>  	/* Get PPM capabilities */
>  	command = UCSI_GET_CAPABILITY;
> -	ret = ucsi_send_command(ucsi, command, &ucsi->cap,
> -				BITS_TO_BYTES(UCSI_GET_CAPABILITY_SIZE));
> +	ucsi->message_in_size = BITS_TO_BYTES(UCSI_GET_CAPABILITY_SIZE);
> +	ret = ucsi_send_command(ucsi, command);
>  	if (ret < 0)
>  		goto err_reset;
>  
> +	memcpy(&ucsi->cap, ucsi->message_in, BITS_TO_BYTES(UCSI_GET_CAPABILITY_SIZE));
> +
>  	if (!ucsi->cap.num_connectors) {
>  		ret = -ENODEV;
>  		goto err_reset;
> @@ -1835,7 +1865,8 @@ static int ucsi_init(struct ucsi *ucsi)
>  	/* Enable all supported notifications */
>  	ntfy = ucsi_get_supported_notifications(ucsi);
>  	command = UCSI_SET_NOTIFICATION_ENABLE | ntfy;
> -	ret = ucsi_send_command(ucsi, command, NULL, 0);
> +	ucsi->message_in_size = 0;
> +	ret = ucsi_send_command(ucsi, command);
>  	if (ret < 0)
>  		goto err_unregister;
>  
> @@ -1886,7 +1917,8 @@ static void ucsi_resume_work(struct work_struct *work)
>  
>  	/* Restore UCSI notification enable mask after system resume */
>  	command = UCSI_SET_NOTIFICATION_ENABLE | ucsi->ntfy;
> -	ret = ucsi_send_command(ucsi, command, NULL, 0);
> +	ucsi->message_in_size = 0;
> +	ret = ucsi_send_command(ucsi, command);
>  	if (ret < 0) {
>  		dev_err(ucsi->dev, "failed to re-enable notifications (%d)\n", ret);
>  		return;
> diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> index 35993bc34d4d..76343285be59 100644
> --- a/drivers/usb/typec/ucsi/ucsi.h
> +++ b/drivers/usb/typec/ucsi/ucsi.h
> @@ -29,6 +29,10 @@ struct dentry;
>  #define UCSI_MESSAGE_OUT		32
>  #define UCSIv2_MESSAGE_OUT		272
>  
> +/* Define maximum lengths for message buffers */
> +#define UCSI_MAX_MESSAGE_IN_LENGTH	256
> +#define UCSI_MAX_MESSAGE_OUT_LENGTH	256
> +
>  /* UCSI versions */
>  #define UCSI_VERSION_1_0	0x0100
>  #define UCSI_VERSION_1_1	0x0110
> @@ -80,8 +84,7 @@ struct ucsi_operations {
>  	int (*read_cci)(struct ucsi *ucsi, u32 *cci);
>  	int (*poll_cci)(struct ucsi *ucsi, u32 *cci);
>  	int (*read_message_in)(struct ucsi *ucsi, void *val, size_t val_len);
> -	int (*sync_control)(struct ucsi *ucsi, u64 command, u32 *cci,
> -			    void *data, size_t size);
> +	int (*sync_control)(struct ucsi *ucsi, u64 command, u32 *cci);
>  	int (*async_control)(struct ucsi *ucsi, u64 command);
>  	bool (*update_altmodes)(struct ucsi *ucsi, u8 recipient,
>  				struct ucsi_altmode *orig,
> @@ -489,6 +492,12 @@ struct ucsi {
>  	unsigned long quirks;
>  #define UCSI_NO_PARTNER_PDOS	BIT(0)	/* Don't read partner's PDOs */
>  #define UCSI_DELAY_DEVICE_PDOS	BIT(1)	/* Reading PDOs fails until the parter is in PD mode */
> +
> +	/* Fixed-size buffers for incoming and outgoing messages */
> +	u8 message_in[UCSI_MAX_MESSAGE_IN_LENGTH];
> +	size_t message_in_size;
> +	u8 message_out[UCSI_MAX_MESSAGE_OUT_LENGTH];
> +	size_t message_out_size;
>  };
>  
>  #define UCSI_MAX_DATA_LENGTH(u) (((u)->version < UCSI_VERSION_2_0) ? 0x10 : 0xff)
> @@ -551,15 +560,13 @@ struct ucsi_connector {
>  	struct usb_pd_identity cable_identity;
>  };
>  
> -int ucsi_send_command(struct ucsi *ucsi, u64 command,
> -		      void *retval, size_t size);
> +int ucsi_send_command(struct ucsi *ucsi, u64 command);
>  
>  void ucsi_altmode_update_active(struct ucsi_connector *con);
>  int ucsi_resume(struct ucsi *ucsi);
>  
>  void ucsi_notify_common(struct ucsi *ucsi, u32 cci);
> -int ucsi_sync_control_common(struct ucsi *ucsi, u64 command, u32 *cci,
> -			     void *data, size_t size);
> +int ucsi_sync_control_common(struct ucsi *ucsi, u64 command, u32 *cci);
>  
>  #if IS_ENABLED(CONFIG_POWER_SUPPLY)
>  int ucsi_register_port_psy(struct ucsi_connector *con);
> diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
> index 6b92f296e985..f1d1f6917b09 100644
> --- a/drivers/usb/typec/ucsi/ucsi_acpi.c
> +++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
> @@ -105,15 +105,14 @@ static const struct ucsi_operations ucsi_acpi_ops = {
>  	.async_control = ucsi_acpi_async_control
>  };
>  
> -static int ucsi_gram_sync_control(struct ucsi *ucsi, u64 command, u32 *cci,
> -				  void *val, size_t len)
> +static int ucsi_gram_sync_control(struct ucsi *ucsi, u64 command, u32 *cci)
>  {
>  	u16 bogus_change = UCSI_CONSTAT_POWER_LEVEL_CHANGE |
>  			   UCSI_CONSTAT_PDOS_CHANGE;
>  	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
>  	int ret;
>  
> -	ret = ucsi_sync_control_common(ucsi, command, cci, val, len);
> +	ret = ucsi_sync_control_common(ucsi, command, cci);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -125,8 +124,8 @@ static int ucsi_gram_sync_control(struct ucsi *ucsi, u64 command, u32 *cci,
>  	if (UCSI_COMMAND(ua->cmd) == UCSI_GET_CONNECTOR_STATUS &&
>  	    ua->check_bogus_event) {
>  		/* Clear the bogus change */
> -		if (*(u16 *)val == bogus_change)
> -			*(u16 *)val = 0;
> +		if (*(u16 *)ucsi->message_in == bogus_change)
> +			*(u16 *)ucsi->message_in = 0;
>  
>  		ua->check_bogus_event = false;
>  	}
> diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
> index d83a0051c737..ead1b2a25c79 100644
> --- a/drivers/usb/typec/ucsi/ucsi_ccg.c
> +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> @@ -606,8 +606,7 @@ static int ucsi_ccg_async_control(struct ucsi *ucsi, u64 command)
>  	return ccg_write(uc, reg, (u8 *)&command, sizeof(command));
>  }
>  
> -static int ucsi_ccg_sync_control(struct ucsi *ucsi, u64 command, u32 *cci,
> -				 void *data, size_t size)
> +static int ucsi_ccg_sync_control(struct ucsi *ucsi, u64 command, u32 *cci)
>  {
>  	struct ucsi_ccg *uc = ucsi_get_drvdata(ucsi);
>  	struct ucsi_connector *con;
> @@ -629,16 +628,16 @@ static int ucsi_ccg_sync_control(struct ucsi *ucsi, u64 command, u32 *cci,
>  		ucsi_ccg_update_set_new_cam_cmd(uc, con, &command);
>  	}
>  
> -	ret = ucsi_sync_control_common(ucsi, command, cci, data, size);
> +	ret = ucsi_sync_control_common(ucsi, command, cci);
>  
>  	switch (UCSI_COMMAND(command)) {
>  	case UCSI_GET_CURRENT_CAM:
>  		if (uc->has_multiple_dp)
> -			ucsi_ccg_update_get_current_cam_cmd(uc, (u8 *)data);
> +			ucsi_ccg_update_get_current_cam_cmd(uc, (u8 *)ucsi->message_in);
>  		break;
>  	case UCSI_GET_ALTERNATE_MODES:
>  		if (UCSI_ALTMODE_RECIPIENT(command) == UCSI_RECIPIENT_SOP) {
> -			struct ucsi_altmode *alt = data;
> +			struct ucsi_altmode *alt = (struct ucsi_altmode *)ucsi->message_in;
>  
>  			if (alt[0].svid == USB_TYPEC_NVIDIA_VLINK_SID)
>  				ucsi_ccg_nvidia_altmode(uc, alt, command);
> @@ -646,7 +645,7 @@ static int ucsi_ccg_sync_control(struct ucsi *ucsi, u64 command, u32 *cci,
>  		break;
>  	case UCSI_GET_CAPABILITY:
>  		if (uc->fw_build == CCG_FW_BUILD_NVIDIA_TEGRA) {
> -			struct ucsi_capability *cap = data;
> +			struct ucsi_capability *cap = (struct ucsi_capability *)ucsi->message_in;
>  
>  			cap->features &= ~UCSI_CAP_ALT_MODE_DETAILS;
>  		}
> diff --git a/drivers/usb/typec/ucsi/ucsi_yoga_c630.c b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
> index 0187c1c4b21a..299081444caa 100644
> --- a/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
> +++ b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
> @@ -88,8 +88,7 @@ static int yoga_c630_ucsi_async_control(struct ucsi *ucsi, u64 command)
>  
>  static int yoga_c630_ucsi_sync_control(struct ucsi *ucsi,
>  				       u64 command,
> -				       u32 *cci,
> -				       void *data, size_t size)
> +				       u32 *cci)
>  {
>  	int ret;
>  
> @@ -107,8 +106,8 @@ static int yoga_c630_ucsi_sync_control(struct ucsi *ucsi,
>  		};
>  
>  		dev_dbg(ucsi->dev, "faking DP altmode for con1\n");
> -		memset(data, 0, size);
> -		memcpy(data, &alt, min(sizeof(alt), size));
> +		memset(ucsi->message_in, 0, ucsi->message_in_size);
> +		memcpy(ucsi->message_in, &alt, min(sizeof(alt), ucsi->message_in_size));
>  		*cci = UCSI_CCI_COMMAND_COMPLETE | UCSI_SET_CCI_LENGTH(sizeof(alt));
>  		return 0;
>  	}
> @@ -121,18 +120,18 @@ static int yoga_c630_ucsi_sync_control(struct ucsi *ucsi,
>  	if (UCSI_COMMAND(command) == UCSI_GET_ALTERNATE_MODES &&
>  	    UCSI_GET_ALTMODE_GET_CONNECTOR_NUMBER(command) == 2) {
>  		dev_dbg(ucsi->dev, "ignoring altmodes for con2\n");
> -		memset(data, 0, size);
> +		memset(ucsi->message_in, 0, ucsi->message_in_size);
>  		*cci = UCSI_CCI_COMMAND_COMPLETE;
>  		return 0;
>  	}
>  
> -	ret = ucsi_sync_control_common(ucsi, command, cci, data, size);
> +	ret = ucsi_sync_control_common(ucsi, command, cci);
>  	if (ret < 0)
>  		return ret;
>  
>  	/* UCSI_GET_CURRENT_CAM is off-by-one on all ports */
> -	if (UCSI_COMMAND(command) == UCSI_GET_CURRENT_CAM && data)
> -		((u8 *)data)[0]--;
> +	if (UCSI_COMMAND(command) == UCSI_GET_CURRENT_CAM && ucsi->message_in_size > 0)
> +		ucsi->message_in[0]--;
>  
>  	return ret;
>  }
> -- 
> 2.43.0
> 

-- 
With best wishes
Dmitry

Re: [PATCH v5 1/4] usb: typec: ucsi: Update UCSI structure to have message in and message out fields

[Katiyar, Pooja]

Hello,

On Fri, Dec 12, 2025 at 06:58:06PM -0800, Dmitry Baryshkov wrote:
> On Thu, Oct 30, 2025 at 07:48:55AM -0700, Pooja Katiyar wrote:
>> Update UCSI structure by adding fields for incoming and outgoing
>> messages. Update .sync_control function and other related functions
>> to use these new fields within the UCSI structure, instead of handling
>> them as separate parameters.
>>
>> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>> Signed-off-by: Pooja Katiyar <pooja.katiyar@intel.com>
>> ---
>> Changelog v3:
>> - Added message fields to UCSI structure and updated sync_control handling.
> 
> Colleagues, after looking at this patch I have a question. What prevents
> message_{in,out}{,_size} to be modified concurrently? While we have PPM
> lock around command submission, size fields and buffers are modified /
> accessed outside of the lock. Granted all the notifications and async
> nature of the UCSI and USB-C protocols, what prevents two commands from
> being executed at the same time with one of the execution threads
> accessing the results returned by the other command?
> 
> In other words:
> 
> - thread A sets message_in_size, calls ucsi_send_command(CMD_A), which
>   takes PPM lock
> 
>    - meanwhile thread B writes another value to message_in_size and
>      calls ucsi_send_command(CMD_B), which now waits on PPM lock
> 
> - thread A finishes execution of the CMD_A, copies data (with the
>   wrong size!) to the message_in_buf, then it releases PPM lock.
> 
> - thread A gets preempted
> 
>     - thread B gets scheduled, it takes PPM lock, executes CMD_B, gets
>       data into the message_in_buf and finally it releases PPM lock
> 
> - finally at some later point thread A gets scheduled, it accesses
>   message_in_buf, reading data that has been overwritten by CMD_B
>   execution.
> 
> WDYT?

Thank you for identifying this race condition. You are correct about the
concurrent access issue with the message buffer fields.

Here are two potential approaches I see to resolve this:

Option 1: Separate mutex locks for message variables
Add a dedicated message_lock mutex to protect message_{in,out}{,_size}.
message_{in,out}{,_size} will only be modified within the message_lock
protection.

	mutex_lock(&ucsi->message_lock);
	ucsi->message_in_size = size;
	ret = ucsi_send_command(ucsi, cmd);
	memcpy(buffer, ucsi->message_in, size);
	mutex_unlock(&ucsi->message_lock);

Option 2: Pass message variables as function arguments
Pass message buffers and sizes as parameters down to where ppm_lock is 
acquired, ensuring protection throughout command execution.

	int ucsi_send_command(ucsi, cmd, msg_in_buf, msg_in_size, msg_out_buf,
			      msg_out_size) {
    		mutex_lock(&ucsi->ppm_lock);
    		ucsi->message_in_size = msg_in_size;
    		// execute command and copy results to msg_in_buf
		memcpy(msg_in_buf, ucsi->message_in, msg_in_size);
    		mutex_unlock(&ucsi->ppm_lock);
	}

I'm leaning toward Option 1 as it resolves the concern of passing too many args.
What are your thoughts on the suggested options or do you have alternative
suggestions? 

Best regards,
Pooja

Re: [PATCH v5 1/4] usb: typec: ucsi: Update UCSI structure to have message in and message out fields

[Dmitry Baryshkov]

On 18/12/2025 03:17, Katiyar, Pooja wrote:
> Hello,
> 
> On Fri, Dec 12, 2025 at 06:58:06PM -0800, Dmitry Baryshkov wrote:
>> On Thu, Oct 30, 2025 at 07:48:55AM -0700, Pooja Katiyar wrote:
>>> Update UCSI structure by adding fields for incoming and outgoing
>>> messages. Update .sync_control function and other related functions
>>> to use these new fields within the UCSI structure, instead of handling
>>> them as separate parameters.
>>>
>>> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>> Signed-off-by: Pooja Katiyar <pooja.katiyar@intel.com>
>>> ---
>>> Changelog v3:
>>> - Added message fields to UCSI structure and updated sync_control handling.
>>
>> Colleagues, after looking at this patch I have a question. What prevents
>> message_{in,out}{,_size} to be modified concurrently? While we have PPM
>> lock around command submission, size fields and buffers are modified /
>> accessed outside of the lock. Granted all the notifications and async
>> nature of the UCSI and USB-C protocols, what prevents two commands from
>> being executed at the same time with one of the execution threads
>> accessing the results returned by the other command?
>>
>> In other words:
>>
>> - thread A sets message_in_size, calls ucsi_send_command(CMD_A), which
>>    takes PPM lock
>>
>>     - meanwhile thread B writes another value to message_in_size and
>>       calls ucsi_send_command(CMD_B), which now waits on PPM lock
>>
>> - thread A finishes execution of the CMD_A, copies data (with the
>>    wrong size!) to the message_in_buf, then it releases PPM lock.
>>
>> - thread A gets preempted
>>
>>      - thread B gets scheduled, it takes PPM lock, executes CMD_B, gets
>>        data into the message_in_buf and finally it releases PPM lock
>>
>> - finally at some later point thread A gets scheduled, it accesses
>>    message_in_buf, reading data that has been overwritten by CMD_B
>>    execution.
>>
>> WDYT?
> 
> Thank you for identifying this race condition. You are correct about the
> concurrent access issue with the message buffer fields.
> 
> Here are two potential approaches I see to resolve this:
> 
> Option 1: Separate mutex locks for message variables
> Add a dedicated message_lock mutex to protect message_{in,out}{,_size}.
> message_{in,out}{,_size} will only be modified within the message_lock
> protection.
> 
> 	mutex_lock(&ucsi->message_lock);
> 	ucsi->message_in_size = size;
> 	ret = ucsi_send_command(ucsi, cmd);
> 	memcpy(buffer, ucsi->message_in, size);
> 	mutex_unlock(&ucsi->message_lock);
> 
> Option 2: Pass message variables as function arguments
> Pass message buffers and sizes as parameters down to where ppm_lock is
> acquired, ensuring protection throughout command execution.
> 
> 	int ucsi_send_command(ucsi, cmd, msg_in_buf, msg_in_size, msg_out_buf,
> 			      msg_out_size) {
>      		mutex_lock(&ucsi->ppm_lock);
>      		ucsi->message_in_size = msg_in_size;
>      		// execute command and copy results to msg_in_buf
> 		memcpy(msg_in_buf, ucsi->message_in, msg_in_size);
>      		mutex_unlock(&ucsi->ppm_lock);
> 	}
> 
> I'm leaning toward Option 1 as it resolves the concern of passing too many args.
> What are your thoughts on the suggested options or do you have alternative
> suggestions?

I'm slightly biased towards  the second option (it doesn't required 
extra locks and it also is a bit more idiomatic), but I'm fine either way.

Looking forward to seeing the fixing patch!

> 
> Best regards,
> Pooja


-- 
With best wishes
Dmitry

Re: [PATCH v5 1/4] usb: typec: ucsi: Update UCSI structure to have message in and message out fields

[Heikki Krogerus]

Hi,

Thu, Dec 18, 2025 at 03:22:43AM +0200, Dmitry Baryshkov kirjoitti:
> On 18/12/2025 03:17, Katiyar, Pooja wrote:
> > Hello,
> > 
> > On Fri, Dec 12, 2025 at 06:58:06PM -0800, Dmitry Baryshkov wrote:
> > > On Thu, Oct 30, 2025 at 07:48:55AM -0700, Pooja Katiyar wrote:
> > > > Update UCSI structure by adding fields for incoming and outgoing
> > > > messages. Update .sync_control function and other related functions
> > > > to use these new fields within the UCSI structure, instead of handling
> > > > them as separate parameters.
> > > > 
> > > > Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > Signed-off-by: Pooja Katiyar <pooja.katiyar@intel.com>
> > > > ---
> > > > Changelog v3:
> > > > - Added message fields to UCSI structure and updated sync_control handling.
> > > 
> > > Colleagues, after looking at this patch I have a question. What prevents
> > > message_{in,out}{,_size} to be modified concurrently? While we have PPM
> > > lock around command submission, size fields and buffers are modified /
> > > accessed outside of the lock. Granted all the notifications and async
> > > nature of the UCSI and USB-C protocols, what prevents two commands from
> > > being executed at the same time with one of the execution threads
> > > accessing the results returned by the other command?
> > > 
> > > In other words:
> > > 
> > > - thread A sets message_in_size, calls ucsi_send_command(CMD_A), which
> > >    takes PPM lock
> > > 
> > >     - meanwhile thread B writes another value to message_in_size and
> > >       calls ucsi_send_command(CMD_B), which now waits on PPM lock
> > > 
> > > - thread A finishes execution of the CMD_A, copies data (with the
> > >    wrong size!) to the message_in_buf, then it releases PPM lock.
> > > 
> > > - thread A gets preempted
> > > 
> > >      - thread B gets scheduled, it takes PPM lock, executes CMD_B, gets
> > >        data into the message_in_buf and finally it releases PPM lock
> > > 
> > > - finally at some later point thread A gets scheduled, it accesses
> > >    message_in_buf, reading data that has been overwritten by CMD_B
> > >    execution.
> > > 
> > > WDYT?
> > 
> > Thank you for identifying this race condition. You are correct about the
> > concurrent access issue with the message buffer fields.
> > 
> > Here are two potential approaches I see to resolve this:
> > 
> > Option 1: Separate mutex locks for message variables
> > Add a dedicated message_lock mutex to protect message_{in,out}{,_size}.
> > message_{in,out}{,_size} will only be modified within the message_lock
> > protection.
> > 
> > 	mutex_lock(&ucsi->message_lock);
> > 	ucsi->message_in_size = size;
> > 	ret = ucsi_send_command(ucsi, cmd);
> > 	memcpy(buffer, ucsi->message_in, size);
> > 	mutex_unlock(&ucsi->message_lock);
> > 
> > Option 2: Pass message variables as function arguments
> > Pass message buffers and sizes as parameters down to where ppm_lock is
> > acquired, ensuring protection throughout command execution.
> > 
> > 	int ucsi_send_command(ucsi, cmd, msg_in_buf, msg_in_size, msg_out_buf,
> > 			      msg_out_size) {
> >      		mutex_lock(&ucsi->ppm_lock);
> >      		ucsi->message_in_size = msg_in_size;
> >      		// execute command and copy results to msg_in_buf
> > 		memcpy(msg_in_buf, ucsi->message_in, msg_in_size);
> >      		mutex_unlock(&ucsi->ppm_lock);
> > 	}
> > 
> > I'm leaning toward Option 1 as it resolves the concern of passing too many args.
> > What are your thoughts on the suggested options or do you have alternative
> > suggestions?
> 
> I'm slightly biased towards  the second option (it doesn't required extra
> locks and it also is a bit more idiomatic), but I'm fine either way.
> 
> Looking forward to seeing the fixing patch!

I don't think you are fully solving the problem.

Pooja, you know my opinion. This whole approach of using separate
files is wrong. IMO we should only have a single file for the entire
UCSI mailbox, and the mailbox should be allocated searately for
everyone that opens the file. I don't think there is any way to
guarantee that the data in these separate files isn't for somebody
else.

thanks,

-- 
heikki

Re: [PATCH v5 1/4] usb: typec: ucsi: Update UCSI structure to have message in and message out fields

[Katiyar, Pooja]

Hello,

On Tue, Dec 23, 2025 at 04:06:44AM -0800, Heikki Krogerus wrote:
> Hi,
> 
> Thu, Dec 18, 2025 at 03:22:43AM +0200, Dmitry Baryshkov kirjoitti:
>> On 18/12/2025 03:17, Katiyar, Pooja wrote:
>>> Hello,
>>>
>>> On Fri, Dec 12, 2025 at 06:58:06PM -0800, Dmitry Baryshkov wrote:
>>>> On Thu, Oct 30, 2025 at 07:48:55AM -0700, Pooja Katiyar wrote:
>>>>> Update UCSI structure by adding fields for incoming and outgoing
>>>>> messages. Update .sync_control function and other related functions
>>>>> to use these new fields within the UCSI structure, instead of handling
>>>>> them as separate parameters.
>>>>>
>>>>> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>>>> Signed-off-by: Pooja Katiyar <pooja.katiyar@intel.com>
>>>>> ---
>>>>> Changelog v3:
>>>>> - Added message fields to UCSI structure and updated sync_control handling.
>>>>
>>>> Colleagues, after looking at this patch I have a question. What prevents
>>>> message_{in,out}{,_size} to be modified concurrently? While we have PPM
>>>> lock around command submission, size fields and buffers are modified /
>>>> accessed outside of the lock. Granted all the notifications and async
>>>> nature of the UCSI and USB-C protocols, what prevents two commands from
>>>> being executed at the same time with one of the execution threads
>>>> accessing the results returned by the other command?
>>>>
>>>> In other words:
>>>>
>>>> - thread A sets message_in_size, calls ucsi_send_command(CMD_A), which
>>>>    takes PPM lock
>>>>
>>>>     - meanwhile thread B writes another value to message_in_size and
>>>>       calls ucsi_send_command(CMD_B), which now waits on PPM lock
>>>>
>>>> - thread A finishes execution of the CMD_A, copies data (with the
>>>>    wrong size!) to the message_in_buf, then it releases PPM lock.
>>>>
>>>> - thread A gets preempted
>>>>
>>>>      - thread B gets scheduled, it takes PPM lock, executes CMD_B, gets
>>>>        data into the message_in_buf and finally it releases PPM lock
>>>>
>>>> - finally at some later point thread A gets scheduled, it accesses
>>>>    message_in_buf, reading data that has been overwritten by CMD_B
>>>>    execution.
>>>>
>>>> WDYT?
>>>
>>> Thank you for identifying this race condition. You are correct about the
>>> concurrent access issue with the message buffer fields.
>>>
>>> Here are two potential approaches I see to resolve this:
>>>
>>> Option 1: Separate mutex locks for message variables
>>> Add a dedicated message_lock mutex to protect message_{in,out}{,_size}.
>>> message_{in,out}{,_size} will only be modified within the message_lock
>>> protection.
>>>
>>> 	mutex_lock(&ucsi->message_lock);
>>> 	ucsi->message_in_size = size;
>>> 	ret = ucsi_send_command(ucsi, cmd);
>>> 	memcpy(buffer, ucsi->message_in, size);
>>> 	mutex_unlock(&ucsi->message_lock);
>>>
>>> Option 2: Pass message variables as function arguments
>>> Pass message buffers and sizes as parameters down to where ppm_lock is
>>> acquired, ensuring protection throughout command execution.
>>>
>>> 	int ucsi_send_command(ucsi, cmd, msg_in_buf, msg_in_size, msg_out_buf,
>>> 			      msg_out_size) {
>>>      		mutex_lock(&ucsi->ppm_lock);
>>>      		ucsi->message_in_size = msg_in_size;
>>>      		// execute command and copy results to msg_in_buf
>>> 		memcpy(msg_in_buf, ucsi->message_in, msg_in_size);
>>>      		mutex_unlock(&ucsi->ppm_lock);
>>> 	}
>>>
>>> I'm leaning toward Option 1 as it resolves the concern of passing too many args.
>>> What are your thoughts on the suggested options or do you have alternative
>>> suggestions?
>>
>> I'm slightly biased towards  the second option (it doesn't required extra
>> locks and it also is a bit more idiomatic), but I'm fine either way.
>>
>> Looking forward to seeing the fixing patch!
> 
> I don't think you are fully solving the problem.
> 
> Pooja, you know my opinion. This whole approach of using separate
> files is wrong. IMO we should only have a single file for the entire
> UCSI mailbox, and the mailbox should be allocated searately for
> everyone that opens the file. I don't think there is any way to
> guarantee that the data in these separate files isn't for somebody
> else.
> 
> thanks,
> 

Thank you for your feedback! We will work on a redesign for clearer implementation.

Regards,
Pooja

[PATCH v5 2/4] usb: typec: ucsi: Add support for message out data structure

[Pooja Katiyar]

Add support for updating message out data structure for UCSI
ACPI interface for UCSI 2.1 and UCSI 3.0 commands such as
Set PDOs and LPM Firmware Update.

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Pooja Katiyar <pooja.katiyar@intel.com>
---
Changelog v3:
- Added message_out fields to UCSI structure and updated sync_control
  to perform write operation.
- Removed extra DSM Write call.

Changelog v2:
- Moved write_message_out operation to .sync_control.
- Updated ucsi_send_command to accept message_out data.

 drivers/usb/typec/ucsi/ucsi.c      | 14 ++++++++++++++
 drivers/usb/typec/ucsi/ucsi.h      |  2 ++
 drivers/usb/typec/ucsi/ucsi_acpi.c | 16 ++++++++++++++++
 3 files changed, 32 insertions(+)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index ee0c0d77e67d..ae199e5683cb 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -67,6 +67,20 @@ int ucsi_sync_control_common(struct ucsi *ucsi, u64 command, u32 *cci)
 
 	reinit_completion(&ucsi->complete);
 
+	if (ucsi->message_out_size > 0) {
+		if (!ucsi->ops->write_message_out) {
+			ucsi->message_out_size = 0;
+			ret = -EOPNOTSUPP;
+			goto out_clear_bit;
+		}
+
+		ret = ucsi->ops->write_message_out(ucsi, ucsi->message_out,
+						   ucsi->message_out_size);
+		ucsi->message_out_size = 0;
+		if (ret)
+			goto out_clear_bit;
+	}
+
 	ret = ucsi->ops->async_control(ucsi, command);
 	if (ret)
 		goto out_clear_bit;
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index 76343285be59..8c79e6699020 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -69,6 +69,7 @@ struct dentry;
  * @read_cci: Read CCI register
  * @poll_cci: Read CCI register while polling with notifications disabled
  * @read_message_in: Read message data from UCSI
+ * @write_message_out: Write message data to UCSI
  * @sync_control: Blocking control operation
  * @async_control: Non-blocking control operation
  * @update_altmodes: Squashes duplicate DP altmodes
@@ -84,6 +85,7 @@ struct ucsi_operations {
 	int (*read_cci)(struct ucsi *ucsi, u32 *cci);
 	int (*poll_cci)(struct ucsi *ucsi, u32 *cci);
 	int (*read_message_in)(struct ucsi *ucsi, void *val, size_t val_len);
+	int (*write_message_out)(struct ucsi *ucsi, void *data, size_t data_len);
 	int (*sync_control)(struct ucsi *ucsi, u64 command, u32 *cci);
 	int (*async_control)(struct ucsi *ucsi, u64 command);
 	bool (*update_altmodes)(struct ucsi *ucsi, u8 recipient,
diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
index f1d1f6917b09..f9beeb835238 100644
--- a/drivers/usb/typec/ucsi/ucsi_acpi.c
+++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
@@ -86,6 +86,21 @@ static int ucsi_acpi_read_message_in(struct ucsi *ucsi, void *val, size_t val_le
 	return 0;
 }
 
+static int ucsi_acpi_write_message_out(struct ucsi *ucsi, void *data, size_t data_len)
+{
+	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
+
+	if (!data || !data_len)
+		return -EINVAL;
+
+	if (ucsi->version <= UCSI_VERSION_1_2)
+		memcpy(ua->base + UCSI_MESSAGE_OUT, data, data_len);
+	else
+		memcpy(ua->base + UCSIv2_MESSAGE_OUT, data, data_len);
+
+	return 0;
+}
+
 static int ucsi_acpi_async_control(struct ucsi *ucsi, u64 command)
 {
 	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
@@ -101,6 +116,7 @@ static const struct ucsi_operations ucsi_acpi_ops = {
 	.read_cci = ucsi_acpi_read_cci,
 	.poll_cci = ucsi_acpi_poll_cci,
 	.read_message_in = ucsi_acpi_read_message_in,
+	.write_message_out = ucsi_acpi_write_message_out,
 	.sync_control = ucsi_sync_control_common,
 	.async_control = ucsi_acpi_async_control
 };
-- 
2.43.0

[PATCH v5 3/4] usb: typec: ucsi: Enable debugfs for message_out data structure

[Pooja Katiyar]

Add debugfs entry for writing message_out data structure to handle
UCSI 2.1 and 3.0 commands through debugfs interface.

Users writing to the message_out debugfs file should ensure the input
data adheres to the following format:
1. Input must be a non-empty valid hexadecimal string.
2. Input length of hexadecimal string must not exceed 256 bytes of
   length to be in alignment with the message out data structure size
   as per the UCSI specification v2.1.
3. If the input string length is odd, then user needs to prepend a
   '0' to the first character for proper hex conversion.

Below are examples of valid hex strings. Note that these values are
just examples. The exact values depend on specific command use case.

        #echo 1A2B3C4D > message_out
        #echo 01234567 > message_out

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Pooja Katiyar <pooja.katiyar@intel.com>
---
Changelog v3:
- Updated debugfs message_out write handler to use message_out fields in
  UCSI structure.

Changelog v2:
- No changes.

 drivers/usb/typec/ucsi/debugfs.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/usb/typec/ucsi/debugfs.c b/drivers/usb/typec/ucsi/debugfs.c
index 4bf3a8f65a4e..0693acf04c37 100644
--- a/drivers/usb/typec/ucsi/debugfs.c
+++ b/drivers/usb/typec/ucsi/debugfs.c
@@ -109,6 +109,30 @@ static int ucsi_vbus_volt_show(struct seq_file *m, void *v)
 }
 DEFINE_SHOW_ATTRIBUTE(ucsi_vbus_volt);
 
+static ssize_t ucsi_message_out_write(struct file *file,
+				      const char __user *data, size_t count, loff_t *ppos)
+{
+	struct ucsi *ucsi = file->private_data;
+	int ret;
+
+	char *buf __free(kfree) = memdup_user_nul(data, count);
+	if (IS_ERR(buf))
+		return PTR_ERR(buf);
+
+	ucsi->message_out_size = min(count / 2, UCSI_MAX_MESSAGE_OUT_LENGTH);
+	ret = hex2bin(ucsi->message_out, buf, ucsi->message_out_size);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+static const struct file_operations ucsi_message_out_fops = {
+	.open = simple_open,
+	.write = ucsi_message_out_write,
+	.llseek = generic_file_llseek,
+};
+
 void ucsi_debugfs_register(struct ucsi *ucsi)
 {
 	ucsi->debugfs = kzalloc(sizeof(*ucsi->debugfs), GFP_KERNEL);
@@ -121,6 +145,8 @@ void ucsi_debugfs_register(struct ucsi *ucsi)
 	debugfs_create_file("peak_current", 0400, ucsi->debugfs->dentry, ucsi, &ucsi_peak_curr_fops);
 	debugfs_create_file("avg_current", 0400, ucsi->debugfs->dentry, ucsi, &ucsi_avg_curr_fops);
 	debugfs_create_file("vbus_voltage", 0400, ucsi->debugfs->dentry, ucsi, &ucsi_vbus_volt_fops);
+	debugfs_create_file("message_out", 0200, ucsi->debugfs->dentry, ucsi,
+			    &ucsi_message_out_fops);
 }
 
 void ucsi_debugfs_unregister(struct ucsi *ucsi)
-- 
2.43.0

[PATCH v5 4/4] usb: typec: ucsi: Add support for SET_PDOS command

[Pooja Katiyar]

Add support for UCSI SET_PDOS command as per UCSI specification
v2.1 and above to debugfs.

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Pooja Katiyar <pooja.katiyar@intel.com>
---
Changelog v3:
- No changes.

Changelog v2:
- Send message_out data as part of ucsi_send_command.

 drivers/usb/typec/ucsi/debugfs.c | 1 +
 drivers/usb/typec/ucsi/ucsi.h    | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/usb/typec/ucsi/debugfs.c b/drivers/usb/typec/ucsi/debugfs.c
index 0693acf04c37..63a5fe791a82 100644
--- a/drivers/usb/typec/ucsi/debugfs.c
+++ b/drivers/usb/typec/ucsi/debugfs.c
@@ -36,6 +36,7 @@ static int ucsi_cmd(void *data, u64 val)
 	case UCSI_SET_NEW_CAM:
 	case UCSI_SET_USB:
 	case UCSI_READ_POWER_LEVEL:
+	case UCSI_SET_PDOS:
 		ucsi->message_in_size = 0;
 		ret = ucsi_send_command(ucsi, val);
 		break;
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index 8c79e6699020..f538a997155a 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -136,6 +136,7 @@ void ucsi_connector_change(struct ucsi *ucsi, u8 num);
 #define UCSI_GET_PD_MESSAGE			0x15
 #define UCSI_GET_CAM_CS			0x18
 #define UCSI_SET_SINK_PATH			0x1c
+#define UCSI_SET_PDOS				0x1d
 #define UCSI_READ_POWER_LEVEL			0x1e
 #define UCSI_SET_USB				0x21
 #define UCSI_GET_LPM_PPM_INFO			0x22
-- 
2.43.0

RE: [PATCH v5 0/4] usb: typec: ucsi: Add support for SET_PDOS command

[Pathak, Asutosh]

Hi Greg,

Could you please take these series of patches into your next upcoming release?
These patches are rebased on v6.18-rc3

https://patchwork.kernel.org/project/linux-usb/list/?series=1017784

-
Thank you!
Asutosh

-----Original Message-----
From: Pooja Katiyar <pooja.katiyar@intel.com> 
Sent: Thursday, October 30, 2025 7:49 AM
To: linux-usb@vger.kernel.org; gregkh@linuxfoundation.org
Cc: dmitry.baryshkov@oss.qualcomm.com; heikki.krogerus@linux.intel.com; Katiyar, Pooja <pooja.katiyar@intel.com>
Subject: [PATCH v5 0/4] usb: typec: ucsi: Add support for SET_PDOS command

This series implements support for UCSI SET_PDOS command. It provides interface to send message out data structure and update source or sink capabilities PDOs on a connector over debugfs interface.

It also updates UCSI structure to have message in and message out fields instead of handling them as separate parameters.

Changelog v5:
- Rebased patches to v6.18-rc3.

Changelog v4:
- Fixed build errors reported by kernel test robot.
- Added changelogs.

Pooja Katiyar (4):
  usb: typec: ucsi: Update UCSI structure to have message in and message
    out fields
  usb: typec: ucsi: Add support for message out data structure
  usb: typec: ucsi: Enable debugfs for message_out data structure
  usb: typec: ucsi: Add support for SET_PDOS command

 drivers/usb/typec/ucsi/cros_ec_ucsi.c   |   5 +-
 drivers/usb/typec/ucsi/debugfs.c        |  36 +++++++-
 drivers/usb/typec/ucsi/displayport.c    |  11 ++-
 drivers/usb/typec/ucsi/ucsi.c           | 118 ++++++++++++++++--------
 drivers/usb/typec/ucsi/ucsi.h           |  22 +++--
 drivers/usb/typec/ucsi/ucsi_acpi.c      |  25 ++++-
 drivers/usb/typec/ucsi/ucsi_ccg.c       |  11 +--
 drivers/usb/typec/ucsi/ucsi_yoga_c630.c |  15 ++-
 8 files changed, 172 insertions(+), 71 deletions(-)

--
2.43.0

Re: [PATCH v5 0/4] usb: typec: ucsi: Add support for SET_PDOS command

[gregkh]

On Tue, Nov 11, 2025 at 12:21:58AM +0000, Pathak, Asutosh wrote:
> Hi Greg,

Please do not top-post :(

> Could you please take these series of patches into your next upcoming release?
> These patches are rebased on v6.18-rc3
> 
> https://patchwork.kernel.org/project/linux-usb/list/?series=1017784

Please relax, it's been less than 2 weeks, and I've been traveling
non-stop (still have 2 more continents to go).  Patches will be handled
when I get to them.  To help out, please review other patches on the
list to cause yours to bubble to the top.

thanks,

greg k-h