[PATCH v2 0/7] usb: typec: ucsi: rework glue driver interface

[PATCH v2 0/7] usb: typec: ucsi: rework glue driver interface

[Dmitry Baryshkov]

The interface between UCSI and the glue driver is very low-level. It
allows reading the UCSI data from any offset (but in reality the UCSI
driver reads only VERSION, CCI an MESSAGE_IN data). All event handling
is to be done by the glue driver (which already resulted in several
similar-but-slightly different implementations). It leaves no place to
optimize the write-read-read sequence for the command execution (which
might be beneficial for some of the drivers), etc.

The patchseries attempts to restructure the UCSI glue driver interface
in order to provide sensible operations instead of a low-level read /
write calls.

If this approach is found to be acceptable, I plan to further rework the
command interface, moving reading CCI and MESSAGE_IN to the common
control code, which should simplify driver's implementation and remove
necessity to split quirks between sync_control and read_message_in e.g.
as implemented in the ucsi_ccg.c.

Note, the series was tested only on the ucsi_glink platforms. Further
testing is appreciated.

Depends: [1], [2]

[1] https://lore.kernel.org/linux-usb/20240612124656.2305603-1-fabrice.gasnier@foss.st.com/

[2] https://lore.kernel.org/linux-usb/20240621-ucsi-yoga-ec-driver-v8-1-e03f3536b8c6@linaro.org/

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
Changes in v2:
- Dropped the RFC prefix
- Rebased on top of the fixed STM32 patch
- Included the pending Yoga C630 driver into the cleanup.
- Link to v1: https://lore.kernel.org/r/20240603-ucsi-rework-interface-v1-0-99a6d544cec8@linaro.org

---
Dmitry Baryshkov (7):
      usb: typec: ucsi: move ucsi_acknowledge() from ucsi_read_error()
      usb: typec: ucsi: simplify command sending API
      usb: typec: ucsi: split read operation
      usb: typec: ucsi: rework command execution functions
      usb: typec: ucsi: inline ucsi_read_message_in
      usb: typec: ucsi: extract common code for command handling
      usb: typec: ucsi: reorder operations in ucsi_run_command()

 drivers/usb/typec/ucsi/ucsi.c           | 215 +++++++++++++++++---------------
 drivers/usb/typec/ucsi/ucsi.h           |  26 ++--
 drivers/usb/typec/ucsi/ucsi_acpi.c      | 100 +++++++--------
 drivers/usb/typec/ucsi/ucsi_ccg.c       | 103 +++++++--------
 drivers/usb/typec/ucsi/ucsi_glink.c     |  74 ++++-------
 drivers/usb/typec/ucsi/ucsi_stm32g0.c   |  79 ++++--------
 drivers/usb/typec/ucsi/ucsi_yoga_c630.c | 104 +++++----------
 7 files changed, 309 insertions(+), 392 deletions(-)
---
base-commit: f0dbf09a40c8100a895f675d619db5ed1f58f7ac
change-id: 20240525-ucsi-rework-interface-5ff2264f6aec

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

[PATCH v2 1/7] usb: typec: ucsi: move ucsi_acknowledge() from ucsi_read_error()

[Dmitry Baryshkov]

As a preparation for reworking UCSI command handling, move
ucsi_acknowledge() for the failed command from ucsi_read_error() to
ucsi_exec_command().

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/usb/typec/ucsi/ucsi.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 134ef4e17d85..8e4d92cbd6e2 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -70,11 +70,6 @@ static int ucsi_read_error(struct ucsi *ucsi)
 	u16 error;
 	int ret;
 
-	/* Acknowledge the command that failed */
-	ret = ucsi_acknowledge(ucsi, false);
-	if (ret)
-		return ret;
-
 	ret = ucsi_exec_command(ucsi, UCSI_GET_ERROR_STATUS);
 	if (ret < 0)
 		return ret;
@@ -153,13 +148,14 @@ static int ucsi_exec_command(struct ucsi *ucsi, u64 cmd)
 	}
 
 	if (cci & UCSI_CCI_ERROR) {
-		if (cmd == UCSI_GET_ERROR_STATUS) {
-			ret = ucsi_acknowledge(ucsi, false);
-			if (ret)
-				return ret;
+		/* Acknowledge the command that failed */
+		ret = ucsi_acknowledge(ucsi, false);
+		if (ret)
+			return ret;
 
+		if (cmd == UCSI_GET_ERROR_STATUS)
 			return -EIO;
-		}
+
 		return ucsi_read_error(ucsi);
 	}
 

-- 
2.39.2

[PATCH v2 2/7] usb: typec: ucsi: simplify command sending API

[Dmitry Baryshkov]

The sync_write and async_write are used only for writing UCSI commands
to the UCSI_CONTROL offsets. Rename sync_write and async_write
operations to sync_control and async_control accordingly. Drop the
offset and length fields and pass u64 command instead.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/usb/typec/ucsi/ucsi.c           | 18 +++++++----------
 drivers/usb/typec/ucsi/ucsi.h           | 10 ++++------
 drivers/usb/typec/ucsi/ucsi_acpi.c      | 22 ++++++++++-----------
 drivers/usb/typec/ucsi/ucsi_ccg.c       | 34 +++++++++++++++------------------
 drivers/usb/typec/ucsi/ucsi_glink.c     | 14 ++++++--------
 drivers/usb/typec/ucsi/ucsi_stm32g0.c   | 24 +++++++++++------------
 drivers/usb/typec/ucsi/ucsi_yoga_c630.c | 20 +++++++------------
 7 files changed, 60 insertions(+), 82 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 8e4d92cbd6e2..2441b077f457 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -60,7 +60,7 @@ static int ucsi_acknowledge(struct ucsi *ucsi, bool conn_ack)
 		ctrl |= UCSI_ACK_CONNECTOR_CHANGE;
 	}
 
-	return ucsi->ops->sync_write(ucsi, UCSI_CONTROL, &ctrl, sizeof(ctrl));
+	return ucsi->ops->sync_control(ucsi, ctrl);
 }
 
 static int ucsi_exec_command(struct ucsi *ucsi, u64 command);
@@ -126,7 +126,7 @@ static int ucsi_exec_command(struct ucsi *ucsi, u64 cmd)
 	u32 cci;
 	int ret;
 
-	ret = ucsi->ops->sync_write(ucsi, UCSI_CONTROL, &cmd, sizeof(cmd));
+	ret = ucsi->ops->sync_control(ucsi, cmd);
 	if (ret)
 		return ret;
 
@@ -1314,8 +1314,7 @@ static int ucsi_reset_ppm(struct ucsi *ucsi)
 	 */
 	if (cci & UCSI_CCI_RESET_COMPLETE) {
 		command = UCSI_SET_NOTIFICATION_ENABLE;
-		ret = ucsi->ops->async_write(ucsi, UCSI_CONTROL, &command,
-					     sizeof(command));
+		ret = ucsi->ops->async_control(ucsi, command);
 		if (ret < 0)
 			goto out;
 
@@ -1336,8 +1335,7 @@ static int ucsi_reset_ppm(struct ucsi *ucsi)
 	}
 
 	command = UCSI_PPM_RESET;
-	ret = ucsi->ops->async_write(ucsi, UCSI_CONTROL, &command,
-				     sizeof(command));
+	ret = ucsi->ops->async_control(ucsi, command);
 	if (ret < 0)
 		goto out;
 
@@ -1358,9 +1356,7 @@ static int ucsi_reset_ppm(struct ucsi *ucsi)
 
 		/* If the PPM is still doing something else, reset it again. */
 		if (cci & ~UCSI_CCI_RESET_COMPLETE) {
-			ret = ucsi->ops->async_write(ucsi, UCSI_CONTROL,
-						     &command,
-						     sizeof(command));
+			ret = ucsi->ops->async_control(ucsi, command);
 			if (ret < 0)
 				goto out;
 		}
@@ -1888,7 +1884,7 @@ struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops)
 {
 	struct ucsi *ucsi;
 
-	if (!ops || !ops->read || !ops->sync_write || !ops->async_write)
+	if (!ops || !ops->read || !ops->sync_control || !ops->async_control)
 		return ERR_PTR(-EINVAL);
 
 	ucsi = kzalloc(sizeof(*ucsi), GFP_KERNEL);
@@ -1964,7 +1960,7 @@ void ucsi_unregister(struct ucsi *ucsi)
 	cancel_work_sync(&ucsi->resume_work);
 
 	/* Disable notifications */
-	ucsi->ops->async_write(ucsi, UCSI_CONTROL, &cmd, sizeof(cmd));
+	ucsi->ops->async_control(ucsi, cmd);
 
 	if (!ucsi->connector)
 		return;
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index 97eda8cd63df..713a6d3fc4e9 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -57,8 +57,8 @@ struct dentry;
 /**
  * struct ucsi_operations - UCSI I/O operations
  * @read: Read operation
- * @sync_write: Blocking write operation
- * @async_write: Non-blocking write operation
+ * @sync_control: Blocking control operation
+ * @async_control: Non-blocking control operation
  * @update_altmodes: Squashes duplicate DP altmodes
  * @update_connector: Update connector capabilities before registering
  * @connector_status: Updates connector status, called holding connector lock
@@ -70,10 +70,8 @@ struct dentry;
 struct ucsi_operations {
 	int (*read)(struct ucsi *ucsi, unsigned int offset,
 		    void *val, size_t val_len);
-	int (*sync_write)(struct ucsi *ucsi, unsigned int offset,
-			  const void *val, size_t val_len);
-	int (*async_write)(struct ucsi *ucsi, unsigned int offset,
-			   const void *val, size_t val_len);
+	int (*sync_control)(struct ucsi *ucsi, u64 command);
+	int (*async_control)(struct ucsi *ucsi, u64 command);
 	bool (*update_altmodes)(struct ucsi *ucsi, struct ucsi_altmode *orig,
 				struct ucsi_altmode *updated);
 	void (*update_connector)(struct ucsi_connector *con);
diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
index 8d112c3edae5..feccbfc8acbe 100644
--- a/drivers/usb/typec/ucsi/ucsi_acpi.c
+++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
@@ -60,22 +60,20 @@ static int ucsi_acpi_read(struct ucsi *ucsi, unsigned int offset,
 	return 0;
 }
 
-static int ucsi_acpi_async_write(struct ucsi *ucsi, unsigned int offset,
-				 const void *val, size_t val_len)
+static int ucsi_acpi_async_control(struct ucsi *ucsi, u64 command)
 {
 	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
 
-	memcpy(ua->base + offset, val, val_len);
-	ua->cmd = *(u64 *)val;
+	memcpy(ua->base + UCSI_CONTROL, &command, sizeof(command));
+	ua->cmd = command;
 
 	return ucsi_acpi_dsm(ua, UCSI_DSM_FUNC_WRITE);
 }
 
-static int ucsi_acpi_sync_write(struct ucsi *ucsi, unsigned int offset,
-				const void *val, size_t val_len)
+static int ucsi_acpi_sync_control(struct ucsi *ucsi, u64 command)
 {
 	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
-	bool ack = UCSI_COMMAND(*(u64 *)val) == UCSI_ACK_CC_CI;
+	bool ack = UCSI_COMMAND(command) == UCSI_ACK_CC_CI;
 	int ret;
 
 	if (ack)
@@ -83,7 +81,7 @@ static int ucsi_acpi_sync_write(struct ucsi *ucsi, unsigned int offset,
 	else
 		set_bit(UCSI_ACPI_COMMAND_PENDING, &ua->flags);
 
-	ret = ucsi_acpi_async_write(ucsi, offset, val, val_len);
+	ret = ucsi_acpi_async_control(ucsi, command);
 	if (ret)
 		goto out_clear_bit;
 
@@ -101,8 +99,8 @@ static int ucsi_acpi_sync_write(struct ucsi *ucsi, unsigned int offset,
 
 static const struct ucsi_operations ucsi_acpi_ops = {
 	.read = ucsi_acpi_read,
-	.sync_write = ucsi_acpi_sync_write,
-	.async_write = ucsi_acpi_async_write
+	.sync_control = ucsi_acpi_sync_control,
+	.async_control = ucsi_acpi_async_control
 };
 
 static int
@@ -124,8 +122,8 @@ ucsi_zenbook_read(struct ucsi *ucsi, unsigned int offset, void *val, size_t val_
 
 static const struct ucsi_operations ucsi_zenbook_ops = {
 	.read = ucsi_zenbook_read,
-	.sync_write = ucsi_acpi_sync_write,
-	.async_write = ucsi_acpi_async_write
+	.sync_control = ucsi_acpi_sync_control,
+	.async_control = ucsi_acpi_async_control
 };
 
 static const struct dmi_system_id ucsi_acpi_quirks[] = {
diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
index dda7c7c94e08..76b39bb9762d 100644
--- a/drivers/usb/typec/ucsi/ucsi_ccg.c
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -610,25 +610,23 @@ static int ucsi_ccg_read(struct ucsi *ucsi, unsigned int offset,
 	return ret;
 }
 
-static int ucsi_ccg_async_write(struct ucsi *ucsi, unsigned int offset,
-				const void *val, size_t val_len)
+static int ucsi_ccg_async_control(struct ucsi *ucsi, u64 command)
 {
 	struct ucsi_ccg *uc = ucsi_get_drvdata(ucsi);
-	u16 reg = CCGX_RAB_UCSI_DATA_BLOCK(offset);
+	u16 reg = CCGX_RAB_UCSI_DATA_BLOCK(UCSI_CONTROL);
 
 	/*
-	 * UCSI may read CCI instantly after async_write,
+	 * UCSI may read CCI instantly after async_control,
 	 * clear CCI to avoid caller getting wrong data before we get CCI from ISR
 	 */
 	spin_lock(&uc->op_lock);
 	uc->op_data.cci = 0;
 	spin_unlock(&uc->op_lock);
 
-	return ccg_write(uc, reg, val, val_len);
+	return ccg_write(uc, reg, (u8 *)&command, sizeof(command));
 }
 
-static int ucsi_ccg_sync_write(struct ucsi *ucsi, unsigned int offset,
-			       const void *val, size_t val_len)
+static int ucsi_ccg_sync_control(struct ucsi *ucsi, u64 command)
 {
 	struct ucsi_ccg *uc = ucsi_get_drvdata(ucsi);
 	struct ucsi_connector *con;
@@ -639,19 +637,17 @@ static int ucsi_ccg_sync_write(struct ucsi *ucsi, unsigned int offset,
 	pm_runtime_get_sync(uc->dev);
 	set_bit(DEV_CMD_PENDING, &uc->flags);
 
-	if (offset == UCSI_CONTROL && val_len == sizeof(uc->last_cmd_sent)) {
-		uc->last_cmd_sent = *(u64 *)val;
+	uc->last_cmd_sent = command;
 
-		if (UCSI_COMMAND(uc->last_cmd_sent) == UCSI_SET_NEW_CAM &&
-		    uc->has_multiple_dp) {
-			con_index = (uc->last_cmd_sent >> 16) &
-				    UCSI_CMD_CONNECTOR_MASK;
-			con = &uc->ucsi->connector[con_index - 1];
-			ucsi_ccg_update_set_new_cam_cmd(uc, con, (u64 *)val);
-		}
+	if (UCSI_COMMAND(uc->last_cmd_sent) == UCSI_SET_NEW_CAM &&
+	    uc->has_multiple_dp) {
+		con_index = (uc->last_cmd_sent >> 16) &
+			UCSI_CMD_CONNECTOR_MASK;
+		con = &uc->ucsi->connector[con_index - 1];
+		ucsi_ccg_update_set_new_cam_cmd(uc, con, &command);
 	}
 
-	ret = ucsi_ccg_async_write(ucsi, offset, val, val_len);
+	ret = ucsi_ccg_async_control(ucsi, command);
 	if (ret)
 		goto err_clear_bit;
 
@@ -668,8 +664,8 @@ static int ucsi_ccg_sync_write(struct ucsi *ucsi, unsigned int offset,
 
 static const struct ucsi_operations ucsi_ccg_ops = {
 	.read = ucsi_ccg_read,
-	.sync_write = ucsi_ccg_sync_write,
-	.async_write = ucsi_ccg_async_write,
+	.sync_control = ucsi_ccg_sync_control,
+	.async_control = ucsi_ccg_async_control,
 	.update_altmodes = ucsi_ccg_update_altmodes
 };
 
diff --git a/drivers/usb/typec/ucsi/ucsi_glink.c b/drivers/usb/typec/ucsi/ucsi_glink.c
index 985a880e86da..5986c4a824a6 100644
--- a/drivers/usb/typec/ucsi/ucsi_glink.c
+++ b/drivers/usb/typec/ucsi/ucsi_glink.c
@@ -143,21 +143,19 @@ static int pmic_glink_ucsi_locked_write(struct pmic_glink_ucsi *ucsi, unsigned i
 	return 0;
 }
 
-static int pmic_glink_ucsi_async_write(struct ucsi *__ucsi, unsigned int offset,
-				       const void *val, size_t val_len)
+static int pmic_glink_ucsi_async_control(struct ucsi *__ucsi, u64 command)
 {
 	struct pmic_glink_ucsi *ucsi = ucsi_get_drvdata(__ucsi);
 	int ret;
 
 	mutex_lock(&ucsi->lock);
-	ret = pmic_glink_ucsi_locked_write(ucsi, offset, val, val_len);
+	ret = pmic_glink_ucsi_locked_write(ucsi, UCSI_CONTROL, &command, sizeof(command));
 	mutex_unlock(&ucsi->lock);
 
 	return ret;
 }
 
-static int pmic_glink_ucsi_sync_write(struct ucsi *__ucsi, unsigned int offset,
-				      const void *val, size_t val_len)
+static int pmic_glink_ucsi_sync_control(struct ucsi *__ucsi, u64 command)
 {
 	struct pmic_glink_ucsi *ucsi = ucsi_get_drvdata(__ucsi);
 	unsigned long left;
@@ -169,7 +167,7 @@ static int pmic_glink_ucsi_sync_write(struct ucsi *__ucsi, unsigned int offset,
 	ucsi->sync_val = 0;
 	reinit_completion(&ucsi->sync_ack);
 	ucsi->sync_pending = true;
-	ret = pmic_glink_ucsi_locked_write(ucsi, offset, val, val_len);
+	ret = pmic_glink_ucsi_locked_write(ucsi, UCSI_CONTROL, &command, sizeof(command));
 	mutex_unlock(&ucsi->lock);
 
 	left = wait_for_completion_timeout(&ucsi->sync_ack, 5 * HZ);
@@ -217,8 +215,8 @@ static void pmic_glink_ucsi_connector_status(struct ucsi_connector *con)
 
 static const struct ucsi_operations pmic_glink_ucsi_ops = {
 	.read = pmic_glink_ucsi_read,
-	.sync_write = pmic_glink_ucsi_sync_write,
-	.async_write = pmic_glink_ucsi_async_write,
+	.sync_control = pmic_glink_ucsi_sync_control,
+	.async_control = pmic_glink_ucsi_async_control,
 	.update_connector = pmic_glink_ucsi_update_connector,
 	.connector_status = pmic_glink_ucsi_connector_status,
 };
diff --git a/drivers/usb/typec/ucsi/ucsi_stm32g0.c b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
index ac69288e8bb0..396e2090e7c3 100644
--- a/drivers/usb/typec/ucsi/ucsi_stm32g0.c
+++ b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
@@ -359,8 +359,7 @@ static int ucsi_stm32g0_read(struct ucsi *ucsi, unsigned int offset, void *val,
 	return 0;
 }
 
-static int ucsi_stm32g0_async_write(struct ucsi *ucsi, unsigned int offset, const void *val,
-				    size_t len)
+static int ucsi_stm32g0_async_control(struct ucsi *ucsi, u64 command)
 {
 	struct ucsi_stm32g0 *g0 = ucsi_get_drvdata(ucsi);
 	struct i2c_client *client = g0->client;
@@ -373,19 +372,19 @@ static int ucsi_stm32g0_async_write(struct ucsi *ucsi, unsigned int offset, cons
 	unsigned char *buf;
 	int ret;
 
-	buf = kmalloc(len + 1, GFP_KERNEL);
+	buf = kmalloc(sizeof(command) + 1, GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
 
-	buf[0] = offset;
-	memcpy(&buf[1], val, len);
-	msg[0].len = len + 1;
+	buf[0] = UCSI_CONTROL;
+	memcpy(&buf[1], &command, sizeof(command));
+	msg[0].len = sizeof(command) + 1;
 	msg[0].buf = buf;
 
 	ret = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
 	kfree(buf);
 	if (ret != ARRAY_SIZE(msg)) {
-		dev_err(g0->dev, "i2c write %02x, %02x error: %d\n", client->addr, offset, ret);
+		dev_err(g0->dev, "i2c write %02x, %02x error: %d\n", client->addr, UCSI_CONTROL, ret);
 
 		return ret < 0 ? ret : -EIO;
 	}
@@ -393,11 +392,10 @@ static int ucsi_stm32g0_async_write(struct ucsi *ucsi, unsigned int offset, cons
 	return 0;
 }
 
-static int ucsi_stm32g0_sync_write(struct ucsi *ucsi, unsigned int offset, const void *val,
-				   size_t len)
+static int ucsi_stm32g0_sync_control(struct ucsi *ucsi, u64 command)
 {
 	struct ucsi_stm32g0 *g0 = ucsi_get_drvdata(ucsi);
-	bool ack = UCSI_COMMAND(*(u64 *)val) == UCSI_ACK_CC_CI;
+	bool ack = UCSI_COMMAND(command) == UCSI_ACK_CC_CI;
 	int ret;
 
 	if (ack)
@@ -405,7 +403,7 @@ static int ucsi_stm32g0_sync_write(struct ucsi *ucsi, unsigned int offset, const
 	else
 		set_bit(COMMAND_PENDING, &g0->flags);
 
-	ret = ucsi_stm32g0_async_write(ucsi, offset, val, len);
+	ret = ucsi_stm32g0_async_control(ucsi, command);
 	if (ret)
 		goto out_clear_bit;
 
@@ -449,8 +447,8 @@ static irqreturn_t ucsi_stm32g0_irq_handler(int irq, void *data)
 
 static const struct ucsi_operations ucsi_stm32g0_ops = {
 	.read = ucsi_stm32g0_read,
-	.sync_write = ucsi_stm32g0_sync_write,
-	.async_write = ucsi_stm32g0_async_write,
+	.sync_control = ucsi_stm32g0_sync_control,
+	.async_control = ucsi_stm32g0_async_control,
 };
 
 static int ucsi_stm32g0_register(struct ucsi *ucsi)
diff --git a/drivers/usb/typec/ucsi/ucsi_yoga_c630.c b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
index 8bee0b469041..e5e8ba0c0eaa 100644
--- a/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
+++ b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
@@ -56,23 +56,17 @@ static int yoga_c630_ucsi_read(struct ucsi *ucsi, unsigned int offset,
 	}
 }
 
-static int yoga_c630_ucsi_async_write(struct ucsi *ucsi, unsigned int offset,
-				      const void *val, size_t val_len)
+static int yoga_c630_ucsi_async_control(struct ucsi *ucsi, u64 command)
 {
 	struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi);
 
-	if (offset != UCSI_CONTROL ||
-	    val_len != YOGA_C630_UCSI_WRITE_SIZE)
-		return -EINVAL;
-
-	return yoga_c630_ec_ucsi_write(uec->ec, val);
+	return yoga_c630_ec_ucsi_write(uec->ec, (u8*)&command);
 }
 
-static int yoga_c630_ucsi_sync_write(struct ucsi *ucsi, unsigned int offset,
-				     const void *val, size_t val_len)
+static int yoga_c630_ucsi_sync_control(struct ucsi *ucsi, u64 command)
 {
 	struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi);
-	bool ack = UCSI_COMMAND(*(u64 *)val) == UCSI_ACK_CC_CI;
+	bool ack = UCSI_COMMAND(command) == UCSI_ACK_CC_CI;
 	int ret;
 
 	if (ack)
@@ -82,7 +76,7 @@ static int yoga_c630_ucsi_sync_write(struct ucsi *ucsi, unsigned int offset,
 
 	reinit_completion(&uec->complete);
 
-	ret = yoga_c630_ucsi_async_write(ucsi, offset, val, val_len);
+	ret = yoga_c630_ucsi_async_control(ucsi, command);
 	if (ret)
 		goto out_clear_bit;
 
@@ -100,8 +94,8 @@ static int yoga_c630_ucsi_sync_write(struct ucsi *ucsi, unsigned int offset,
 
 const struct ucsi_operations yoga_c630_ucsi_ops = {
 	.read = yoga_c630_ucsi_read,
-	.sync_write = yoga_c630_ucsi_sync_write,
-	.async_write = yoga_c630_ucsi_async_write,
+	.sync_control = yoga_c630_ucsi_sync_control,
+	.async_control = yoga_c630_ucsi_async_control,
 };
 
 static void yoga_c630_ucsi_notify_ucsi(struct yoga_c630_ucsi *uec, u32 cci)

-- 
2.39.2

[PATCH v2 3/7] usb: typec: ucsi: split read operation

[Dmitry Baryshkov]

The read operation is only used to read fixed data at fixed offsets
(UCSI_VERSION, UCSI_CCI, UCSI_MESSAGE_IN). In some cases drivers apply
offset-specific overrides. Split the read() operation into three
operations, read_version(), read_cci(), read_message_in().

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/usb/typec/ucsi/ucsi.c           | 20 +++++------
 drivers/usb/typec/ucsi/ucsi.h           |  9 +++--
 drivers/usb/typec/ucsi/ucsi_acpi.c      | 60 ++++++++++++++++++++++++++++-----
 drivers/usb/typec/ucsi/ucsi_ccg.c       | 50 ++++++++++++++-------------
 drivers/usb/typec/ucsi/ucsi_glink.c     | 19 ++++++++++-
 drivers/usb/typec/ucsi/ucsi_stm32g0.c   | 19 ++++++++++-
 drivers/usb/typec/ucsi/ucsi_yoga_c630.c | 52 +++++++++++++++++-----------
 7 files changed, 163 insertions(+), 66 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 2441b077f457..81b459b26b74 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -46,7 +46,7 @@ static int ucsi_read_message_in(struct ucsi *ucsi, void *buf,
 	if (ucsi->version <= UCSI_VERSION_1_2)
 		buf_size = clamp(buf_size, 0, 16);
 
-	return ucsi->ops->read(ucsi, UCSI_MESSAGE_IN, buf, buf_size);
+	return ucsi->ops->read_message_in(ucsi, buf, buf_size);
 }
 
 static int ucsi_acknowledge(struct ucsi *ucsi, bool conn_ack)
@@ -130,7 +130,7 @@ static int ucsi_exec_command(struct ucsi *ucsi, u64 cmd)
 	if (ret)
 		return ret;
 
-	ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci));
+	ret = ucsi->ops->read_cci(ucsi, &cci);
 	if (ret)
 		return ret;
 
@@ -1302,7 +1302,7 @@ static int ucsi_reset_ppm(struct ucsi *ucsi)
 
 	mutex_lock(&ucsi->ppm_lock);
 
-	ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci));
+	ret = ucsi->ops->read_cci(ucsi, &cci);
 	if (ret < 0)
 		goto out;
 
@@ -1320,8 +1320,7 @@ static int ucsi_reset_ppm(struct ucsi *ucsi)
 
 		tmo = jiffies + msecs_to_jiffies(UCSI_TIMEOUT_MS);
 		do {
-			ret = ucsi->ops->read(ucsi, UCSI_CCI,
-					      &cci, sizeof(cci));
+			ret = ucsi->ops->read_cci(ucsi, &cci);
 			if (ret < 0)
 				goto out;
 			if (cci & UCSI_CCI_COMMAND_COMPLETE)
@@ -1350,7 +1349,7 @@ static int ucsi_reset_ppm(struct ucsi *ucsi)
 		/* Give the PPM time to process a reset before reading CCI */
 		msleep(20);
 
-		ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci));
+		ret = ucsi->ops->read_cci(ucsi, &cci);
 		if (ret)
 			goto out;
 
@@ -1770,7 +1769,7 @@ static int ucsi_init(struct ucsi *ucsi)
 	ucsi->ntfy = ntfy;
 
 	mutex_lock(&ucsi->ppm_lock);
-	ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci));
+	ret = ucsi->ops->read_cci(ucsi, &cci);
 	mutex_unlock(&ucsi->ppm_lock);
 	if (ret)
 		return ret;
@@ -1884,7 +1883,9 @@ struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops)
 {
 	struct ucsi *ucsi;
 
-	if (!ops || !ops->read || !ops->sync_control || !ops->async_control)
+	if (!ops ||
+	    !ops->read_version || !ops->read_cci || !ops->read_message_in ||
+	    !ops->sync_control || !ops->async_control)
 		return ERR_PTR(-EINVAL);
 
 	ucsi = kzalloc(sizeof(*ucsi), GFP_KERNEL);
@@ -1920,8 +1921,7 @@ int ucsi_register(struct ucsi *ucsi)
 {
 	int ret;
 
-	ret = ucsi->ops->read(ucsi, UCSI_VERSION, &ucsi->version,
-			      sizeof(ucsi->version));
+	ret = ucsi->ops->read_version(ucsi, &ucsi->version);
 	if (ret)
 		return ret;
 
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index 713a6d3fc4e9..0cf550cc9b5d 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -56,7 +56,9 @@ struct dentry;
 
 /**
  * struct ucsi_operations - UCSI I/O operations
- * @read: Read operation
+ * @read_version: Read implemented UCSI version
+ * @read_cci: Read CCI register
+ * @read_message_in: Read message data from UCSI
  * @sync_control: Blocking control operation
  * @async_control: Non-blocking control operation
  * @update_altmodes: Squashes duplicate DP altmodes
@@ -68,8 +70,9 @@ struct dentry;
  * return immediately after sending the data to the PPM.
  */
 struct ucsi_operations {
-	int (*read)(struct ucsi *ucsi, unsigned int offset,
-		    void *val, size_t val_len);
+	int (*read_version)(struct ucsi *ucsi, u16 *version);
+	int (*read_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);
 	int (*async_control)(struct ucsi *ucsi, u64 command);
 	bool (*update_altmodes)(struct ucsi *ucsi, struct ucsi_altmode *orig,
diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
index feccbfc8acbe..61dd28dae3a4 100644
--- a/drivers/usb/typec/ucsi/ucsi_acpi.c
+++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
@@ -45,8 +45,7 @@ static int ucsi_acpi_dsm(struct ucsi_acpi *ua, int func)
 	return 0;
 }
 
-static int ucsi_acpi_read(struct ucsi *ucsi, unsigned int offset,
-			  void *val, size_t val_len)
+static int ucsi_acpi_read_version(struct ucsi *ucsi, u16 *version)
 {
 	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
 	int ret;
@@ -55,7 +54,35 @@ static int ucsi_acpi_read(struct ucsi *ucsi, unsigned int offset,
 	if (ret)
 		return ret;
 
-	memcpy(val, ua->base + offset, val_len);
+	memcpy(version, ua->base + UCSI_VERSION, sizeof(*version));
+
+	return 0;
+}
+
+static int ucsi_acpi_read_cci(struct ucsi *ucsi, u32 *cci)
+{
+	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
+	int ret;
+
+	ret = ucsi_acpi_dsm(ua, UCSI_DSM_FUNC_READ);
+	if (ret)
+		return ret;
+
+	memcpy(cci, ua->base + UCSI_CCI, sizeof(*cci));
+
+	return 0;
+}
+
+static int ucsi_acpi_read_message_in(struct ucsi *ucsi, void *val, size_t val_len)
+{
+	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
+	int ret;
+
+	ret = ucsi_acpi_dsm(ua, UCSI_DSM_FUNC_READ);
+	if (ret)
+		return ret;
+
+	memcpy(val, ua->base + UCSI_MESSAGE_IN, val_len);
 
 	return 0;
 }
@@ -98,30 +125,45 @@ static int ucsi_acpi_sync_control(struct ucsi *ucsi, u64 command)
 }
 
 static const struct ucsi_operations ucsi_acpi_ops = {
-	.read = ucsi_acpi_read,
+	.read_version = ucsi_acpi_read_version,
+	.read_cci = ucsi_acpi_read_cci,
+	.read_message_in = ucsi_acpi_read_message_in,
 	.sync_control = ucsi_acpi_sync_control,
 	.async_control = ucsi_acpi_async_control
 };
 
 static int
-ucsi_zenbook_read(struct ucsi *ucsi, unsigned int offset, void *val, size_t val_len)
+ucsi_zenbook_read_cci(struct ucsi *ucsi, u32 *cci)
 {
 	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
 	int ret;
 
-	if (offset == UCSI_VERSION || UCSI_COMMAND(ua->cmd) == UCSI_PPM_RESET) {
+	if (UCSI_COMMAND(ua->cmd) == UCSI_PPM_RESET) {
 		ret = ucsi_acpi_dsm(ua, UCSI_DSM_FUNC_READ);
 		if (ret)
 			return ret;
 	}
 
-	memcpy(val, ua->base + offset, val_len);
+	memcpy(cci, ua->base + UCSI_CCI, sizeof(*cci));
+
+	return 0;
+}
+
+static int
+ucsi_zenbook_read_message_in(struct ucsi *ucsi, void *val, size_t val_len)
+{
+	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
+
+	/* UCSI_MESSAGE_IN is never read for PPM_RESET, return stored data */
+	memcpy(val, ua->base + UCSI_MESSAGE_IN, val_len);
 
 	return 0;
 }
 
 static const struct ucsi_operations ucsi_zenbook_ops = {
-	.read = ucsi_zenbook_read,
+	.read_version = ucsi_acpi_read_version,
+	.read_cci = ucsi_zenbook_read_cci,
+	.read_message_in = ucsi_zenbook_read_message_in,
 	.sync_control = ucsi_acpi_sync_control,
 	.async_control = ucsi_acpi_async_control
 };
@@ -143,7 +185,7 @@ static void ucsi_acpi_notify(acpi_handle handle, u32 event, void *data)
 	u32 cci;
 	int ret;
 
-	ret = ua->ucsi->ops->read(ua->ucsi, UCSI_CCI, &cci, sizeof(cci));
+	ret = ua->ucsi->ops->read_cci(ua->ucsi, &cci);
 	if (ret)
 		return;
 
diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
index 76b39bb9762d..6ccc394f268e 100644
--- a/drivers/usb/typec/ucsi/ucsi_ccg.c
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -556,32 +556,34 @@ static void ucsi_ccg_nvidia_altmode(struct ucsi_ccg *uc,
 	}
 }
 
-static int ucsi_ccg_read(struct ucsi *ucsi, unsigned int offset,
-			 void *val, size_t val_len)
+static int ucsi_ccg_read_version(struct ucsi *ucsi, u16 *version)
 {
 	struct ucsi_ccg *uc = ucsi_get_drvdata(ucsi);
-	u16 reg = CCGX_RAB_UCSI_DATA_BLOCK(offset);
-	struct ucsi_capability *cap;
-	struct ucsi_altmode *alt;
-	int ret = 0;
+	u16 reg = CCGX_RAB_UCSI_DATA_BLOCK(UCSI_VERSION);
 
-	if (offset == UCSI_CCI) {
-		spin_lock(&uc->op_lock);
-		memcpy(val, &(uc->op_data).cci, val_len);
-		spin_unlock(&uc->op_lock);
-	} else if (offset == UCSI_MESSAGE_IN) {
-		spin_lock(&uc->op_lock);
-		memcpy(val, &(uc->op_data).message_in, val_len);
-		spin_unlock(&uc->op_lock);
-	} else {
-		ret = ccg_read(uc, reg, val, val_len);
-	}
+	return ccg_read(uc, reg, (u8 *)version, sizeof(*version));
+}
 
-	if (ret)
-		return ret;
+static int ucsi_ccg_read_cci(struct ucsi *ucsi, u32 *cci)
+{
+	struct ucsi_ccg *uc = ucsi_get_drvdata(ucsi);
 
-	if (offset != UCSI_MESSAGE_IN)
-		return ret;
+	spin_lock(&uc->op_lock);
+	*cci = uc->op_data.cci;
+	spin_unlock(&uc->op_lock);
+
+	return 0;
+}
+
+static int ucsi_ccg_read_message_in(struct ucsi *ucsi, void *val, size_t val_len)
+{
+	struct ucsi_ccg *uc = ucsi_get_drvdata(ucsi);
+	struct ucsi_capability *cap;
+	struct ucsi_altmode *alt;
+
+	spin_lock(&uc->op_lock);
+	memcpy(val, uc->op_data.message_in, val_len);
+	spin_unlock(&uc->op_lock);
 
 	switch (UCSI_COMMAND(uc->last_cmd_sent)) {
 	case UCSI_GET_CURRENT_CAM:
@@ -607,7 +609,7 @@ static int ucsi_ccg_read(struct ucsi *ucsi, unsigned int offset,
 	}
 	uc->last_cmd_sent = 0;
 
-	return ret;
+	return 0;
 }
 
 static int ucsi_ccg_async_control(struct ucsi *ucsi, u64 command)
@@ -663,7 +665,9 @@ static int ucsi_ccg_sync_control(struct ucsi *ucsi, u64 command)
 }
 
 static const struct ucsi_operations ucsi_ccg_ops = {
-	.read = ucsi_ccg_read,
+	.read_version = ucsi_ccg_read_version,
+	.read_cci = ucsi_ccg_read_cci,
+	.read_message_in = ucsi_ccg_read_message_in,
 	.sync_control = ucsi_ccg_sync_control,
 	.async_control = ucsi_ccg_async_control,
 	.update_altmodes = ucsi_ccg_update_altmodes
diff --git a/drivers/usb/typec/ucsi/ucsi_glink.c b/drivers/usb/typec/ucsi/ucsi_glink.c
index 5986c4a824a6..62a3a192e076 100644
--- a/drivers/usb/typec/ucsi/ucsi_glink.c
+++ b/drivers/usb/typec/ucsi/ucsi_glink.c
@@ -114,6 +114,21 @@ static int pmic_glink_ucsi_read(struct ucsi *__ucsi, unsigned int offset,
 	return ret;
 }
 
+static int pmic_glink_ucsi_read_version(struct ucsi *ucsi, u16 *version)
+{
+	return pmic_glink_ucsi_read(ucsi, UCSI_VERSION, version, sizeof(*version));
+}
+
+static int pmic_glink_ucsi_read_cci(struct ucsi *ucsi, u32 *cci)
+{
+	return pmic_glink_ucsi_read(ucsi, UCSI_CCI, cci, sizeof(*cci));
+}
+
+static int pmic_glink_ucsi_read_message_in(struct ucsi *ucsi, void *val, size_t val_len)
+{
+	return pmic_glink_ucsi_read(ucsi, UCSI_MESSAGE_IN, val, val_len);
+}
+
 static int pmic_glink_ucsi_locked_write(struct pmic_glink_ucsi *ucsi, unsigned int offset,
 					const void *val, size_t val_len)
 {
@@ -214,7 +229,9 @@ static void pmic_glink_ucsi_connector_status(struct ucsi_connector *con)
 }
 
 static const struct ucsi_operations pmic_glink_ucsi_ops = {
-	.read = pmic_glink_ucsi_read,
+	.read_version = pmic_glink_ucsi_read_version,
+	.read_cci = pmic_glink_ucsi_read_cci,
+	.read_message_in = pmic_glink_ucsi_read_message_in,
 	.sync_control = pmic_glink_ucsi_sync_control,
 	.async_control = pmic_glink_ucsi_async_control,
 	.update_connector = pmic_glink_ucsi_update_connector,
diff --git a/drivers/usb/typec/ucsi/ucsi_stm32g0.c b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
index 396e2090e7c3..14737ca3724c 100644
--- a/drivers/usb/typec/ucsi/ucsi_stm32g0.c
+++ b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
@@ -359,6 +359,21 @@ static int ucsi_stm32g0_read(struct ucsi *ucsi, unsigned int offset, void *val,
 	return 0;
 }
 
+static int ucsi_stm32g0_read_version(struct ucsi *ucsi, u16 *version)
+{
+	return ucsi_stm32g0_read(ucsi, UCSI_VERSION, version, sizeof(*version));
+}
+
+static int ucsi_stm32g0_read_cci(struct ucsi *ucsi, u32 *cci)
+{
+	return ucsi_stm32g0_read(ucsi, UCSI_CCI, cci, sizeof(*cci));
+}
+
+static int ucsi_stm32g0_read_message_in(struct ucsi *ucsi, void *val, size_t len)
+{
+	return ucsi_stm32g0_read(ucsi, UCSI_MESSAGE_IN, val, len);
+}
+
 static int ucsi_stm32g0_async_control(struct ucsi *ucsi, u64 command)
 {
 	struct ucsi_stm32g0 *g0 = ucsi_get_drvdata(ucsi);
@@ -446,7 +461,9 @@ static irqreturn_t ucsi_stm32g0_irq_handler(int irq, void *data)
 }
 
 static const struct ucsi_operations ucsi_stm32g0_ops = {
-	.read = ucsi_stm32g0_read,
+	.read_version = ucsi_stm32g0_read_version,
+	.read_cci = ucsi_stm32g0_read_cci,
+	.read_message_in = ucsi_stm32g0_read_message_in,
 	.sync_control = ucsi_stm32g0_sync_control,
 	.async_control = ucsi_stm32g0_async_control,
 };
diff --git a/drivers/usb/typec/ucsi/ucsi_yoga_c630.c b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
index e5e8ba0c0eaa..95a333ad5496 100644
--- a/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
+++ b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
@@ -27,8 +27,16 @@ struct yoga_c630_ucsi {
 	u16 version;
 };
 
-static int yoga_c630_ucsi_read(struct ucsi *ucsi, unsigned int offset,
-			       void *val, size_t val_len)
+static int yoga_c630_ucsi_read_version(struct ucsi *ucsi, u16 *version)
+{
+	struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi);
+
+	*version = uec->version;
+
+	return 0;
+}
+
+static int yoga_c630_ucsi_read_cci(struct ucsi *ucsi, u32 *cci)
 {
 	struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi);
 	u8 buf[YOGA_C630_UCSI_READ_SIZE];
@@ -38,22 +46,26 @@ static int yoga_c630_ucsi_read(struct ucsi *ucsi, unsigned int offset,
 	if (ret)
 		return ret;
 
-	if (offset == UCSI_VERSION) {
-		memcpy(val, &uec->version, min(val_len, sizeof(uec->version)));
-		return 0;
-	}
+	memcpy(cci, buf, sizeof(*cci));
 
-	switch (offset) {
-	case UCSI_CCI:
-		memcpy(val, buf, min(val_len, YOGA_C630_UCSI_CCI_SIZE));
-		return 0;
-	case UCSI_MESSAGE_IN:
-		memcpy(val, buf + YOGA_C630_UCSI_CCI_SIZE,
-		       min(val_len, YOGA_C630_UCSI_DATA_SIZE));
-		return 0;
-	default:
-		return -EINVAL;
-	}
+	return 0;
+}
+
+static int yoga_c630_ucsi_read_message_in(struct ucsi *ucsi,
+					  void *val, size_t val_len)
+{
+	struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi);
+	u8 buf[YOGA_C630_UCSI_READ_SIZE];
+	int ret;
+
+	ret = yoga_c630_ec_ucsi_read(uec->ec, buf);
+	if (ret)
+		return ret;
+
+	memcpy(val, buf + YOGA_C630_UCSI_CCI_SIZE,
+	       min(val_len, YOGA_C630_UCSI_DATA_SIZE));
+
+	return 0;
 }
 
 static int yoga_c630_ucsi_async_control(struct ucsi *ucsi, u64 command)
@@ -93,7 +105,9 @@ static int yoga_c630_ucsi_sync_control(struct ucsi *ucsi, u64 command)
 }
 
 const struct ucsi_operations yoga_c630_ucsi_ops = {
-	.read = yoga_c630_ucsi_read,
+	.read_version = yoga_c630_ucsi_read_version,
+	.read_cci = yoga_c630_ucsi_read_cci,
+	.read_message_in = yoga_c630_ucsi_read_message_in,
 	.sync_control = yoga_c630_ucsi_sync_control,
 	.async_control = yoga_c630_ucsi_async_control,
 };
@@ -126,7 +140,7 @@ static int yoga_c630_ucsi_notify(struct notifier_block *nb,
 		return NOTIFY_OK;
 
 	case LENOVO_EC_EVENT_UCSI:
-		ret = uec->ucsi->ops->read(uec->ucsi, UCSI_CCI, &cci, sizeof(cci));
+		ret = uec->ucsi->ops->read_cci(uec->ucsi, &cci);
 		if (ret)
 			return NOTIFY_DONE;
 

-- 
2.39.2

[PATCH v2 4/7] usb: typec: ucsi: rework command execution functions

[Dmitry Baryshkov]

Rework command execution code to remove recursive calls of
ucsi_exec_command. This also streamlines the sync_control / read(CCI)
read (MESSAGE_IN) sequence, allowing further rework of the command code.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/usb/typec/ucsi/ucsi.c | 134 ++++++++++++++++++++----------------------
 1 file changed, 64 insertions(+), 70 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 81b459b26b74..58d9602bd752 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -63,25 +63,74 @@ static int ucsi_acknowledge(struct ucsi *ucsi, bool conn_ack)
 	return ucsi->ops->sync_control(ucsi, ctrl);
 }
 
-static int ucsi_exec_command(struct ucsi *ucsi, u64 command);
-
-static int ucsi_read_error(struct ucsi *ucsi)
+static int ucsi_run_command(struct ucsi *ucsi, u64 command, u32 *cci,
+			    void *data, size_t size, bool conn_ack)
 {
-	u16 error;
 	int ret;
 
-	ret = ucsi_exec_command(ucsi, UCSI_GET_ERROR_STATUS);
-	if (ret < 0)
+	*cci = 0;
+
+	ret = ucsi->ops->sync_control(ucsi, command);
+	if (ret)
 		return ret;
 
-	ret = ucsi_read_message_in(ucsi, &error, sizeof(error));
+	ret = ucsi->ops->read_cci(ucsi, cci);
 	if (ret)
 		return ret;
 
-	ret = ucsi_acknowledge(ucsi, false);
+	if (*cci & UCSI_CCI_BUSY)
+		return -EBUSY;
+
+	if (!(*cci & UCSI_CCI_COMMAND_COMPLETE))
+		return -EIO;
+
+	if (*cci & UCSI_CCI_NOT_SUPPORTED) {
+		if (ucsi_acknowledge(ucsi, false) < 0)
+			dev_err(ucsi->dev,
+				"ACK of unsupported command failed\n");
+		return -EOPNOTSUPP;
+	}
+
+	if (*cci & UCSI_CCI_ERROR) {
+		/* Acknowledge the command that failed */
+		ret = ucsi_acknowledge(ucsi, false);
+		return ret ? ret : -EIO;
+	}
+
+	if (data) {
+		ret = ucsi_read_message_in(ucsi, data, size);
+		if (ret)
+			return ret;
+	}
+
+	ret = ucsi_acknowledge(ucsi, conn_ack);
 	if (ret)
 		return ret;
 
+	return 0;
+}
+
+static int ucsi_read_error(struct ucsi *ucsi)
+{
+	u16 error;
+	u32 cci;
+	int ret;
+
+	ret = ucsi_run_command(ucsi, UCSI_GET_ERROR_STATUS, &cci,
+			       &error, sizeof(error), false);
+
+	if (cci & UCSI_CCI_BUSY) {
+		ret = ucsi_run_command(ucsi, UCSI_CANCEL, &cci, NULL, 0, false);
+
+		return ret ? ret : -EBUSY;
+	}
+
+	if (ret < 0)
+		return ret;
+
+	if (cci & UCSI_CCI_ERROR)
+		return -EIO;
+
 	switch (error) {
 	case UCSI_ERROR_INCOMPATIBLE_PARTNER:
 		return -EOPNOTSUPP;
@@ -121,78 +170,23 @@ static int ucsi_read_error(struct ucsi *ucsi)
 	return -EIO;
 }
 
-static int ucsi_exec_command(struct ucsi *ucsi, u64 cmd)
-{
-	u32 cci;
-	int ret;
-
-	ret = ucsi->ops->sync_control(ucsi, cmd);
-	if (ret)
-		return ret;
-
-	ret = ucsi->ops->read_cci(ucsi, &cci);
-	if (ret)
-		return ret;
-
-	if (cmd != UCSI_CANCEL && cci & UCSI_CCI_BUSY)
-		return ucsi_exec_command(ucsi, UCSI_CANCEL);
-
-	if (!(cci & UCSI_CCI_COMMAND_COMPLETE))
-		return -EIO;
-
-	if (cci & UCSI_CCI_NOT_SUPPORTED) {
-		if (ucsi_acknowledge(ucsi, false) < 0)
-			dev_err(ucsi->dev,
-				"ACK of unsupported command failed\n");
-		return -EOPNOTSUPP;
-	}
-
-	if (cci & UCSI_CCI_ERROR) {
-		/* Acknowledge the command that failed */
-		ret = ucsi_acknowledge(ucsi, false);
-		if (ret)
-			return ret;
-
-		if (cmd == UCSI_GET_ERROR_STATUS)
-			return -EIO;
-
-		return ucsi_read_error(ucsi);
-	}
-
-	if (cmd == UCSI_CANCEL && cci & UCSI_CCI_CANCEL_COMPLETE) {
-		ret = ucsi_acknowledge(ucsi, false);
-		return ret ? ret : -EBUSY;
-	}
-
-	return UCSI_CCI_LENGTH(cci);
-}
-
 static int ucsi_send_command_common(struct ucsi *ucsi, u64 command,
 				    void *data, size_t size, bool conn_ack)
 {
-	u8 length;
+	u32 cci;
 	int ret;
 
 	mutex_lock(&ucsi->ppm_lock);
 
-	ret = ucsi_exec_command(ucsi, command);
-	if (ret < 0)
-		goto out;
-
-	length = ret;
-
-	if (data) {
-		ret = ucsi_read_message_in(ucsi, data, size);
-		if (ret)
-			goto out;
+	ret = ucsi_run_command(ucsi, command, &cci, data, size, conn_ack);
+	if (cci & UCSI_CCI_BUSY) {
+		ret = ucsi_run_command(ucsi, UCSI_CANCEL, &cci, NULL, 0, false);
+		return ret ? ret : -EBUSY;
 	}
 
-	ret = ucsi_acknowledge(ucsi, conn_ack);
-	if (ret)
-		goto out;
+	if (cci & UCSI_CCI_ERROR)
+		return ucsi_read_error(ucsi);
 
-	ret = length;
-out:
 	mutex_unlock(&ucsi->ppm_lock);
 	return ret;
 }

-- 
2.39.2

[PATCH v2 5/7] usb: typec: ucsi: inline ucsi_read_message_in

[Dmitry Baryshkov]

There is no need to have a separate wrapper for reading MESSAGE_IN data,
inline it to ucsi_run_command().

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/usb/typec/ucsi/ucsi.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 58d9602bd752..4ba22323dbf9 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -36,19 +36,6 @@
  */
 #define UCSI_SWAP_TIMEOUT_MS	5000
 
-static int ucsi_read_message_in(struct ucsi *ucsi, void *buf,
-					  size_t buf_size)
-{
-	/*
-	 * Below UCSI 2.0, MESSAGE_IN was limited to 16 bytes. Truncate the
-	 * reads here.
-	 */
-	if (ucsi->version <= UCSI_VERSION_1_2)
-		buf_size = clamp(buf_size, 0, 16);
-
-	return ucsi->ops->read_message_in(ucsi, buf, buf_size);
-}
-
 static int ucsi_acknowledge(struct ucsi *ucsi, bool conn_ack)
 {
 	u64 ctrl;
@@ -70,6 +57,13 @@ static int ucsi_run_command(struct ucsi *ucsi, u64 command, u32 *cci,
 
 	*cci = 0;
 
+	/*
+	 * Below UCSI 2.0, MESSAGE_IN was limited to 16 bytes. Truncate the
+	 * reads here.
+	 */
+	if (ucsi->version <= UCSI_VERSION_1_2)
+		size = clamp(size, 0, 16);
+
 	ret = ucsi->ops->sync_control(ucsi, command);
 	if (ret)
 		return ret;
@@ -98,7 +92,7 @@ static int ucsi_run_command(struct ucsi *ucsi, u64 command, u32 *cci,
 	}
 
 	if (data) {
-		ret = ucsi_read_message_in(ucsi, data, size);
+		ret = ucsi->ops->read_message_in(ucsi, data, size);
 		if (ret)
 			return ret;
 	}

-- 
2.39.2

[PATCH v2 6/7] usb: typec: ucsi: extract common code for command handling

[Dmitry Baryshkov]

Extract common functions to handle command sending and to handle events
from UCSI. This ensures that all UCSI glue drivers handle the ACKs in
the same way.

The CCG driver used DEV_CMD_PENDING both for internal
firmware-related commands and for UCSI control handling. Leave the
former use case intact.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/usb/typec/ucsi/ucsi.c           | 43 +++++++++++++++++++++++++++
 drivers/usb/typec/ucsi/ucsi.h           |  7 +++++
 drivers/usb/typec/ucsi/ucsi_acpi.c      | 46 ++---------------------------
 drivers/usb/typec/ucsi/ucsi_ccg.c       | 21 ++-----------
 drivers/usb/typec/ucsi/ucsi_glink.c     | 47 ++---------------------------
 drivers/usb/typec/ucsi/ucsi_stm32g0.c   | 44 ++--------------------------
 drivers/usb/typec/ucsi/ucsi_yoga_c630.c | 52 ++-------------------------------
 7 files changed, 62 insertions(+), 198 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 4ba22323dbf9..691ee0c4ef87 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -36,6 +36,48 @@
  */
 #define UCSI_SWAP_TIMEOUT_MS	5000
 
+void ucsi_notify_common(struct ucsi *ucsi, u32 cci)
+{
+	if (UCSI_CCI_CONNECTOR(cci))
+		ucsi_connector_change(ucsi, UCSI_CCI_CONNECTOR(cci));
+
+	if (cci & UCSI_CCI_ACK_COMPLETE &&
+	    test_bit(ACK_PENDING, &ucsi->flags))
+		complete(&ucsi->complete);
+
+	if (cci & UCSI_CCI_COMMAND_COMPLETE &&
+	    test_bit(COMMAND_PENDING, &ucsi->flags))
+		complete(&ucsi->complete);
+}
+EXPORT_SYMBOL_GPL(ucsi_notify_common);
+
+int ucsi_sync_control_common(struct ucsi *ucsi, u64 command)
+{
+	bool ack = UCSI_COMMAND(command) == UCSI_ACK_CC_CI;
+	int ret;
+
+	if (ack)
+		set_bit(ACK_PENDING, &ucsi->flags);
+	else
+		set_bit(COMMAND_PENDING, &ucsi->flags);
+
+	ret = ucsi->ops->async_control(ucsi, command);
+	if (ret)
+		goto out_clear_bit;
+
+	if (!wait_for_completion_timeout(&ucsi->complete, 5 * HZ))
+		ret = -ETIMEDOUT;
+
+out_clear_bit:
+	if (ack)
+		clear_bit(ACK_PENDING, &ucsi->flags);
+	else
+		clear_bit(COMMAND_PENDING, &ucsi->flags);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(ucsi_sync_control_common);
+
 static int ucsi_acknowledge(struct ucsi *ucsi, bool conn_ack)
 {
 	u64 ctrl;
@@ -1883,6 +1925,7 @@ struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops)
 	INIT_WORK(&ucsi->resume_work, ucsi_resume_work);
 	INIT_DELAYED_WORK(&ucsi->work, ucsi_init_work);
 	mutex_init(&ucsi->ppm_lock);
+	init_completion(&ucsi->complete);
 	ucsi->dev = dev;
 	ucsi->ops = ops;
 
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index 0cf550cc9b5d..676fa9db9ba8 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -4,6 +4,7 @@
 #define __DRIVER_USB_TYPEC_UCSI_H
 
 #include <linux/bitops.h>
+#include <linux/completion.h>
 #include <linux/device.h>
 #include <linux/power_supply.h>
 #include <linux/types.h>
@@ -420,6 +421,9 @@ struct ucsi {
 	/* PPM communication flags */
 	unsigned long flags;
 #define EVENT_PENDING	0
+#define COMMAND_PENDING	1
+#define ACK_PENDING	2
+	struct completion complete;
 
 	unsigned long quirks;
 #define UCSI_NO_PARTNER_PDOS	BIT(0)	/* Don't read partner's PDOs */
@@ -484,6 +488,9 @@ 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);
+
 #if IS_ENABLED(CONFIG_POWER_SUPPLY)
 int ucsi_register_port_psy(struct ucsi_connector *con);
 void ucsi_unregister_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 61dd28dae3a4..e8402fac70c8 100644
--- a/drivers/usb/typec/ucsi/ucsi_acpi.c
+++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
@@ -21,10 +21,6 @@ struct ucsi_acpi {
 	struct device *dev;
 	struct ucsi *ucsi;
 	void *base;
-	struct completion complete;
-	unsigned long flags;
-#define UCSI_ACPI_COMMAND_PENDING	1
-#define UCSI_ACPI_ACK_PENDING		2
 	guid_t guid;
 	u64 cmd;
 };
@@ -97,38 +93,11 @@ static int ucsi_acpi_async_control(struct ucsi *ucsi, u64 command)
 	return ucsi_acpi_dsm(ua, UCSI_DSM_FUNC_WRITE);
 }
 
-static int ucsi_acpi_sync_control(struct ucsi *ucsi, u64 command)
-{
-	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
-	bool ack = UCSI_COMMAND(command) == UCSI_ACK_CC_CI;
-	int ret;
-
-	if (ack)
-		set_bit(UCSI_ACPI_ACK_PENDING, &ua->flags);
-	else
-		set_bit(UCSI_ACPI_COMMAND_PENDING, &ua->flags);
-
-	ret = ucsi_acpi_async_control(ucsi, command);
-	if (ret)
-		goto out_clear_bit;
-
-	if (!wait_for_completion_timeout(&ua->complete, 5 * HZ))
-		ret = -ETIMEDOUT;
-
-out_clear_bit:
-	if (ack)
-		clear_bit(UCSI_ACPI_ACK_PENDING, &ua->flags);
-	else
-		clear_bit(UCSI_ACPI_COMMAND_PENDING, &ua->flags);
-
-	return ret;
-}
-
 static const struct ucsi_operations ucsi_acpi_ops = {
 	.read_version = ucsi_acpi_read_version,
 	.read_cci = ucsi_acpi_read_cci,
 	.read_message_in = ucsi_acpi_read_message_in,
-	.sync_control = ucsi_acpi_sync_control,
+	.sync_control = ucsi_sync_control_common,
 	.async_control = ucsi_acpi_async_control
 };
 
@@ -164,7 +133,7 @@ static const struct ucsi_operations ucsi_zenbook_ops = {
 	.read_version = ucsi_acpi_read_version,
 	.read_cci = ucsi_zenbook_read_cci,
 	.read_message_in = ucsi_zenbook_read_message_in,
-	.sync_control = ucsi_acpi_sync_control,
+	.sync_control = ucsi_sync_control_common,
 	.async_control = ucsi_acpi_async_control
 };
 
@@ -189,15 +158,7 @@ static void ucsi_acpi_notify(acpi_handle handle, u32 event, void *data)
 	if (ret)
 		return;
 
-	if (UCSI_CCI_CONNECTOR(cci))
-		ucsi_connector_change(ua->ucsi, UCSI_CCI_CONNECTOR(cci));
-
-	if (cci & UCSI_CCI_ACK_COMPLETE &&
-	    test_bit(UCSI_ACPI_ACK_PENDING, &ua->flags))
-		complete(&ua->complete);
-	if (cci & UCSI_CCI_COMMAND_COMPLETE &&
-	    test_bit(UCSI_ACPI_COMMAND_PENDING, &ua->flags))
-		complete(&ua->complete);
+	ucsi_notify_common(ua->ucsi, cci);
 }
 
 static int ucsi_acpi_probe(struct platform_device *pdev)
@@ -231,7 +192,6 @@ static int ucsi_acpi_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	init_completion(&ua->complete);
 	ua->dev = &pdev->dev;
 
 	id = dmi_first_match(ucsi_acpi_quirks);
diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
index 6ccc394f268e..ba4db2310a05 100644
--- a/drivers/usb/typec/ucsi/ucsi_ccg.c
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -222,8 +222,6 @@ struct ucsi_ccg {
 	u16 fw_build;
 	struct work_struct pm_work;
 
-	struct completion complete;
-
 	u64 last_cmd_sent;
 	bool has_multiple_dp;
 	struct ucsi_ccg_altmode orig[UCSI_MAX_ALTMODES];
@@ -637,7 +635,6 @@ static int ucsi_ccg_sync_control(struct ucsi *ucsi, u64 command)
 
 	mutex_lock(&uc->lock);
 	pm_runtime_get_sync(uc->dev);
-	set_bit(DEV_CMD_PENDING, &uc->flags);
 
 	uc->last_cmd_sent = command;
 
@@ -649,15 +646,8 @@ static int ucsi_ccg_sync_control(struct ucsi *ucsi, u64 command)
 		ucsi_ccg_update_set_new_cam_cmd(uc, con, &command);
 	}
 
-	ret = ucsi_ccg_async_control(ucsi, command);
-	if (ret)
-		goto err_clear_bit;
-
-	if (!wait_for_completion_timeout(&uc->complete, msecs_to_jiffies(5000)))
-		ret = -ETIMEDOUT;
+	ret = ucsi_sync_control_common(ucsi, command);
 
-err_clear_bit:
-	clear_bit(DEV_CMD_PENDING, &uc->flags);
 	pm_runtime_put_sync(uc->dev);
 	mutex_unlock(&uc->lock);
 
@@ -694,9 +684,6 @@ static irqreturn_t ccg_irq_handler(int irq, void *data)
 	if (ret)
 		goto err_clear_irq;
 
-	if (UCSI_CCI_CONNECTOR(cci))
-		ucsi_connector_change(uc->ucsi, UCSI_CCI_CONNECTOR(cci));
-
 	/*
 	 * As per CCGx UCSI interface guide, copy CCI and MESSAGE_IN
 	 * to the OpRegion before clear the UCSI interrupt
@@ -708,9 +695,8 @@ static irqreturn_t ccg_irq_handler(int irq, void *data)
 err_clear_irq:
 	ccg_write(uc, CCGX_RAB_INTR_REG, &intr_reg, sizeof(intr_reg));
 
-	if (!ret && test_bit(DEV_CMD_PENDING, &uc->flags) &&
-	    cci & (UCSI_CCI_ACK_COMPLETE | UCSI_CCI_COMMAND_COMPLETE))
-		complete(&uc->complete);
+	if (!ret)
+		ucsi_notify_common(uc->ucsi, cci);
 
 	return IRQ_HANDLED;
 }
@@ -1429,7 +1415,6 @@ static int ucsi_ccg_probe(struct i2c_client *client)
 	uc->client = client;
 	uc->irq = client->irq;
 	mutex_init(&uc->lock);
-	init_completion(&uc->complete);
 	INIT_WORK(&uc->work, ccg_update_firmware);
 	INIT_WORK(&uc->pm_work, ccg_pm_workaround_work);
 
diff --git a/drivers/usb/typec/ucsi/ucsi_glink.c b/drivers/usb/typec/ucsi/ucsi_glink.c
index 62a3a192e076..57bc9540fc36 100644
--- a/drivers/usb/typec/ucsi/ucsi_glink.c
+++ b/drivers/usb/typec/ucsi/ucsi_glink.c
@@ -64,12 +64,8 @@ struct pmic_glink_ucsi {
 	struct ucsi *ucsi;
 	struct completion read_ack;
 	struct completion write_ack;
-	struct completion sync_ack;
-	bool sync_pending;
 	struct mutex lock;	/* protects concurrent access to PMIC Glink interface */
 
-	int sync_val;
-
 	struct work_struct notify_work;
 	struct work_struct register_work;
 
@@ -170,35 +166,6 @@ static int pmic_glink_ucsi_async_control(struct ucsi *__ucsi, u64 command)
 	return ret;
 }
 
-static int pmic_glink_ucsi_sync_control(struct ucsi *__ucsi, u64 command)
-{
-	struct pmic_glink_ucsi *ucsi = ucsi_get_drvdata(__ucsi);
-	unsigned long left;
-	int ret;
-
-	/* TOFIX: Downstream forces recipient to CON when UCSI_GET_ALTERNATE_MODES command */
-
-	mutex_lock(&ucsi->lock);
-	ucsi->sync_val = 0;
-	reinit_completion(&ucsi->sync_ack);
-	ucsi->sync_pending = true;
-	ret = pmic_glink_ucsi_locked_write(ucsi, UCSI_CONTROL, &command, sizeof(command));
-	mutex_unlock(&ucsi->lock);
-
-	left = wait_for_completion_timeout(&ucsi->sync_ack, 5 * HZ);
-	if (!left) {
-		dev_err(ucsi->dev, "timeout waiting for UCSI sync write response\n");
-		/* return 0 here and let core UCSI code handle the CCI_BUSY */
-		ret = 0;
-	} else if (ucsi->sync_val) {
-		dev_err(ucsi->dev, "sync write returned: %d\n", ucsi->sync_val);
-	}
-
-	ucsi->sync_pending = false;
-
-	return ret;
-}
-
 static void pmic_glink_ucsi_update_connector(struct ucsi_connector *con)
 {
 	struct pmic_glink_ucsi *ucsi = ucsi_get_drvdata(con->ucsi);
@@ -232,7 +199,7 @@ static const struct ucsi_operations pmic_glink_ucsi_ops = {
 	.read_version = pmic_glink_ucsi_read_version,
 	.read_cci = pmic_glink_ucsi_read_cci,
 	.read_message_in = pmic_glink_ucsi_read_message_in,
-	.sync_control = pmic_glink_ucsi_sync_control,
+	.sync_control = ucsi_sync_control_common,
 	.async_control = pmic_glink_ucsi_async_control,
 	.update_connector = pmic_glink_ucsi_update_connector,
 	.connector_status = pmic_glink_ucsi_connector_status,
@@ -256,14 +223,12 @@ static void pmic_glink_ucsi_write_ack(struct pmic_glink_ucsi *ucsi, const void *
 	if (resp->ret_code)
 		return;
 
-	ucsi->sync_val = resp->ret_code;
 	complete(&ucsi->write_ack);
 }
 
 static void pmic_glink_ucsi_notify(struct work_struct *work)
 {
 	struct pmic_glink_ucsi *ucsi = container_of(work, struct pmic_glink_ucsi, notify_work);
-	unsigned int con_num;
 	u32 cci;
 	int ret;
 
@@ -273,14 +238,7 @@ static void pmic_glink_ucsi_notify(struct work_struct *work)
 		return;
 	}
 
-	con_num = UCSI_CCI_CONNECTOR(cci);
-	if (con_num)
-		ucsi_connector_change(ucsi->ucsi, con_num);
-
-	if (ucsi->sync_pending &&
-		   (cci & (UCSI_CCI_ACK_COMPLETE | UCSI_CCI_COMMAND_COMPLETE))) {
-		complete(&ucsi->sync_ack);
-	}
+	ucsi_notify_common(ucsi->ucsi, cci);
 }
 
 static void pmic_glink_ucsi_register(struct work_struct *work)
@@ -362,7 +320,6 @@ static int pmic_glink_ucsi_probe(struct auxiliary_device *adev,
 	INIT_WORK(&ucsi->register_work, pmic_glink_ucsi_register);
 	init_completion(&ucsi->read_ack);
 	init_completion(&ucsi->write_ack);
-	init_completion(&ucsi->sync_ack);
 	mutex_init(&ucsi->lock);
 
 	ucsi->ucsi = ucsi_create(dev, &pmic_glink_ucsi_ops);
diff --git a/drivers/usb/typec/ucsi/ucsi_stm32g0.c b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
index 14737ca3724c..d948c3f579e1 100644
--- a/drivers/usb/typec/ucsi/ucsi_stm32g0.c
+++ b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
@@ -61,11 +61,7 @@ struct ucsi_stm32g0 {
 	struct i2c_client *i2c_bl;
 	bool in_bootloader;
 	u8 bl_version;
-	struct completion complete;
 	struct device *dev;
-	unsigned long flags;
-#define COMMAND_PENDING	1
-#define ACK_PENDING	2
 	const char *fw_name;
 	struct ucsi *ucsi;
 	bool suspended;
@@ -407,35 +403,6 @@ static int ucsi_stm32g0_async_control(struct ucsi *ucsi, u64 command)
 	return 0;
 }
 
-static int ucsi_stm32g0_sync_control(struct ucsi *ucsi, u64 command)
-{
-	struct ucsi_stm32g0 *g0 = ucsi_get_drvdata(ucsi);
-	bool ack = UCSI_COMMAND(command) == UCSI_ACK_CC_CI;
-	int ret;
-
-	if (ack)
-		set_bit(ACK_PENDING, &g0->flags);
-	else
-		set_bit(COMMAND_PENDING, &g0->flags);
-
-	ret = ucsi_stm32g0_async_control(ucsi, command);
-	if (ret)
-		goto out_clear_bit;
-
-	if (!wait_for_completion_timeout(&g0->complete, msecs_to_jiffies(5000)))
-		ret = -ETIMEDOUT;
-	else
-		return 0;
-
-out_clear_bit:
-	if (ack)
-		clear_bit(ACK_PENDING, &g0->flags);
-	else
-		clear_bit(COMMAND_PENDING, &g0->flags);
-
-	return ret;
-}
-
 static irqreturn_t ucsi_stm32g0_irq_handler(int irq, void *data)
 {
 	struct ucsi_stm32g0 *g0 = data;
@@ -449,13 +416,7 @@ static irqreturn_t ucsi_stm32g0_irq_handler(int irq, void *data)
 	if (ret)
 		return IRQ_NONE;
 
-	if (UCSI_CCI_CONNECTOR(cci))
-		ucsi_connector_change(g0->ucsi, UCSI_CCI_CONNECTOR(cci));
-
-	if (cci & UCSI_CCI_ACK_COMPLETE && test_and_clear_bit(ACK_PENDING, &g0->flags))
-		complete(&g0->complete);
-	if (cci & UCSI_CCI_COMMAND_COMPLETE && test_and_clear_bit(COMMAND_PENDING, &g0->flags))
-		complete(&g0->complete);
+	ucsi_notify_common(g0->ucsi, cci);
 
 	return IRQ_HANDLED;
 }
@@ -464,7 +425,7 @@ static const struct ucsi_operations ucsi_stm32g0_ops = {
 	.read_version = ucsi_stm32g0_read_version,
 	.read_cci = ucsi_stm32g0_read_cci,
 	.read_message_in = ucsi_stm32g0_read_message_in,
-	.sync_control = ucsi_stm32g0_sync_control,
+	.sync_control = ucsi_sync_control_common,
 	.async_control = ucsi_stm32g0_async_control,
 };
 
@@ -665,7 +626,6 @@ static int ucsi_stm32g0_probe(struct i2c_client *client)
 
 	g0->dev = dev;
 	g0->client = client;
-	init_completion(&g0->complete);
 	i2c_set_clientdata(client, g0);
 
 	g0->ucsi = ucsi_create(dev, &ucsi_stm32g0_ops);
diff --git a/drivers/usb/typec/ucsi/ucsi_yoga_c630.c b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
index 95a333ad5496..f3a5e24ea84d 100644
--- a/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
+++ b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
@@ -20,10 +20,6 @@ struct yoga_c630_ucsi {
 	struct yoga_c630_ec *ec;
 	struct ucsi *ucsi;
 	struct notifier_block nb;
-	struct completion complete;
-	unsigned long flags;
-#define UCSI_C630_COMMAND_PENDING	0
-#define UCSI_C630_ACK_PENDING		1
 	u16 version;
 };
 
@@ -75,57 +71,14 @@ static int yoga_c630_ucsi_async_control(struct ucsi *ucsi, u64 command)
 	return yoga_c630_ec_ucsi_write(uec->ec, (u8*)&command);
 }
 
-static int yoga_c630_ucsi_sync_control(struct ucsi *ucsi, u64 command)
-{
-	struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi);
-	bool ack = UCSI_COMMAND(command) == UCSI_ACK_CC_CI;
-	int ret;
-
-	if (ack)
-		set_bit(UCSI_C630_ACK_PENDING, &uec->flags);
-	else
-		set_bit(UCSI_C630_COMMAND_PENDING, &uec->flags);
-
-	reinit_completion(&uec->complete);
-
-	ret = yoga_c630_ucsi_async_control(ucsi, command);
-	if (ret)
-		goto out_clear_bit;
-
-	if (!wait_for_completion_timeout(&uec->complete, 5 * HZ))
-		ret = -ETIMEDOUT;
-
-out_clear_bit:
-	if (ack)
-		clear_bit(UCSI_C630_ACK_PENDING, &uec->flags);
-	else
-		clear_bit(UCSI_C630_COMMAND_PENDING, &uec->flags);
-
-	return ret;
-}
-
 const struct ucsi_operations yoga_c630_ucsi_ops = {
 	.read_version = yoga_c630_ucsi_read_version,
 	.read_cci = yoga_c630_ucsi_read_cci,
 	.read_message_in = yoga_c630_ucsi_read_message_in,
-	.sync_control = yoga_c630_ucsi_sync_control,
+	.sync_control = ucsi_sync_control_common,
 	.async_control = yoga_c630_ucsi_async_control,
 };
 
-static void yoga_c630_ucsi_notify_ucsi(struct yoga_c630_ucsi *uec, u32 cci)
-{
-	if (UCSI_CCI_CONNECTOR(cci))
-		ucsi_connector_change(uec->ucsi, UCSI_CCI_CONNECTOR(cci));
-
-	if (cci & UCSI_CCI_ACK_COMPLETE &&
-	    test_bit(UCSI_C630_ACK_PENDING, &uec->flags))
-		complete(&uec->complete);
-
-	if (cci & UCSI_CCI_COMMAND_COMPLETE &&
-	    test_bit(UCSI_C630_COMMAND_PENDING, &uec->flags))
-		complete(&uec->complete);
-}
-
 static int yoga_c630_ucsi_notify(struct notifier_block *nb,
 				 unsigned long action, void *data)
 {
@@ -144,7 +97,7 @@ static int yoga_c630_ucsi_notify(struct notifier_block *nb,
 		if (ret)
 			return NOTIFY_DONE;
 
-		yoga_c630_ucsi_notify_ucsi(uec, cci);
+		ucsi_notify_common(uec->ucsi, cci);
 
 		return NOTIFY_OK;
 
@@ -165,7 +118,6 @@ static int yoga_c630_ucsi_probe(struct auxiliary_device *adev,
 		return -ENOMEM;
 
 	uec->ec = ec;
-	init_completion(&uec->complete);
 	uec->nb.notifier_call = yoga_c630_ucsi_notify;
 
 	uec->ucsi = ucsi_create(&adev->dev, &yoga_c630_ucsi_ops);

-- 
2.39.2

[PATCH v2 7/7] usb: typec: ucsi: reorder operations in ucsi_run_command()

[Dmitry Baryshkov]

Streamline control stream of ucsi_run_command(). Reorder operations so
that there is only one call to ucsi_acknowledge(), making sure that all
complete commands are acknowledged. This also makes sure that the
command is acknowledged even if reading MESSAGE_IN data returns an
error.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/usb/typec/ucsi/ucsi.c | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 691ee0c4ef87..02d7f745acad 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -95,7 +95,7 @@ static int ucsi_acknowledge(struct ucsi *ucsi, bool conn_ack)
 static int ucsi_run_command(struct ucsi *ucsi, u64 command, u32 *cci,
 			    void *data, size_t size, bool conn_ack)
 {
-	int ret;
+	int ret, err;
 
 	*cci = 0;
 
@@ -120,30 +120,24 @@ static int ucsi_run_command(struct ucsi *ucsi, u64 command, u32 *cci,
 	if (!(*cci & UCSI_CCI_COMMAND_COMPLETE))
 		return -EIO;
 
-	if (*cci & UCSI_CCI_NOT_SUPPORTED) {
-		if (ucsi_acknowledge(ucsi, false) < 0)
-			dev_err(ucsi->dev,
-				"ACK of unsupported command failed\n");
-		return -EOPNOTSUPP;
-	}
-
-	if (*cci & UCSI_CCI_ERROR) {
-		/* Acknowledge the command that failed */
-		ret = ucsi_acknowledge(ucsi, false);
-		return ret ? ret : -EIO;
-	}
+	if (*cci & UCSI_CCI_NOT_SUPPORTED)
+		err = -EOPNOTSUPP;
+	else if (*cci & UCSI_CCI_ERROR)
+		err = -EIO;
+	else
+		err = 0;
 
-	if (data) {
-		ret = ucsi->ops->read_message_in(ucsi, data, size);
-		if (ret)
-			return ret;
-	}
+	if (!err && data && UCSI_CCI_LENGTH(*cci))
+		err = ucsi->ops->read_message_in(ucsi, data, size);
 
-	ret = ucsi_acknowledge(ucsi, conn_ack);
+	/*
+	 * Don't ACK connection change if there was an error.
+	 */
+	ret = ucsi_acknowledge(ucsi, err ? false : conn_ack);
 	if (ret)
 		return ret;
 
-	return 0;
+	return err;
 }
 
 static int ucsi_read_error(struct ucsi *ucsi)

-- 
2.39.2

Re: [PATCH v2 0/7] usb: typec: ucsi: rework glue driver interface

[Heikki Krogerus]

On Fri, Jun 21, 2024 at 01:55:19AM +0300, Dmitry Baryshkov wrote:
> The interface between UCSI and the glue driver is very low-level. It
> allows reading the UCSI data from any offset (but in reality the UCSI
> driver reads only VERSION, CCI an MESSAGE_IN data). All event handling
> is to be done by the glue driver (which already resulted in several
> similar-but-slightly different implementations). It leaves no place to
> optimize the write-read-read sequence for the command execution (which
> might be beneficial for some of the drivers), etc.
> 
> The patchseries attempts to restructure the UCSI glue driver interface
> in order to provide sensible operations instead of a low-level read /
> write calls.
> 
> If this approach is found to be acceptable, I plan to further rework the
> command interface, moving reading CCI and MESSAGE_IN to the common
> control code, which should simplify driver's implementation and remove
> necessity to split quirks between sync_control and read_message_in e.g.
> as implemented in the ucsi_ccg.c.
> 
> Note, the series was tested only on the ucsi_glink platforms. Further
> testing is appreciated.

I can run a few tests against these tomorrow.

I don't have have any objections with this approach, but you'll need
to do another rebase. Now these don't apply cleanly because of
9e3caa9dd51b ("usb: typec: ucsi_acpi: Add LG Gram quirk").

thanks,

> Depends: [1], [2]
> 
> [1] https://lore.kernel.org/linux-usb/20240612124656.2305603-1-fabrice.gasnier@foss.st.com/
> 
> [2] https://lore.kernel.org/linux-usb/20240621-ucsi-yoga-ec-driver-v8-1-e03f3536b8c6@linaro.org/
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> Changes in v2:
> - Dropped the RFC prefix
> - Rebased on top of the fixed STM32 patch
> - Included the pending Yoga C630 driver into the cleanup.
> - Link to v1: https://lore.kernel.org/r/20240603-ucsi-rework-interface-v1-0-99a6d544cec8@linaro.org
> 
> ---
> Dmitry Baryshkov (7):
>       usb: typec: ucsi: move ucsi_acknowledge() from ucsi_read_error()
>       usb: typec: ucsi: simplify command sending API
>       usb: typec: ucsi: split read operation
>       usb: typec: ucsi: rework command execution functions
>       usb: typec: ucsi: inline ucsi_read_message_in
>       usb: typec: ucsi: extract common code for command handling
>       usb: typec: ucsi: reorder operations in ucsi_run_command()
> 
>  drivers/usb/typec/ucsi/ucsi.c           | 215 +++++++++++++++++---------------
>  drivers/usb/typec/ucsi/ucsi.h           |  26 ++--
>  drivers/usb/typec/ucsi/ucsi_acpi.c      | 100 +++++++--------
>  drivers/usb/typec/ucsi/ucsi_ccg.c       | 103 +++++++--------
>  drivers/usb/typec/ucsi/ucsi_glink.c     |  74 ++++-------
>  drivers/usb/typec/ucsi/ucsi_stm32g0.c   |  79 ++++--------
>  drivers/usb/typec/ucsi/ucsi_yoga_c630.c | 104 +++++----------
>  7 files changed, 309 insertions(+), 392 deletions(-)
> ---
> base-commit: f0dbf09a40c8100a895f675d619db5ed1f58f7ac
> change-id: 20240525-ucsi-rework-interface-5ff2264f6aec
> 
> Best regards,
> -- 
> Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
heikki

Re: [Linux-stm32] [PATCH v2 6/7] usb: typec: ucsi: extract common code for command handling

[Fabrice Gasnier]

On 6/21/24 00:55, Dmitry Baryshkov wrote:
> Extract common functions to handle command sending and to handle events
> from UCSI. This ensures that all UCSI glue drivers handle the ACKs in
> the same way.
> 
> The CCG driver used DEV_CMD_PENDING both for internal
> firmware-related commands and for UCSI control handling. Leave the
> former use case intact.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/usb/typec/ucsi/ucsi.c           | 43 +++++++++++++++++++++++++++
>  drivers/usb/typec/ucsi/ucsi.h           |  7 +++++
>  drivers/usb/typec/ucsi/ucsi_acpi.c      | 46 ++---------------------------
>  drivers/usb/typec/ucsi/ucsi_ccg.c       | 21 ++-----------
>  drivers/usb/typec/ucsi/ucsi_glink.c     | 47 ++---------------------------
>  drivers/usb/typec/ucsi/ucsi_stm32g0.c   | 44 ++--------------------------
>  drivers/usb/typec/ucsi/ucsi_yoga_c630.c | 52 ++-------------------------------
>  7 files changed, 62 insertions(+), 198 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 4ba22323dbf9..691ee0c4ef87 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -36,6 +36,48 @@
>   */
>  #define UCSI_SWAP_TIMEOUT_MS	5000
>  
> +void ucsi_notify_common(struct ucsi *ucsi, u32 cci)
> +{
> +	if (UCSI_CCI_CONNECTOR(cci))
> +		ucsi_connector_change(ucsi, UCSI_CCI_CONNECTOR(cci));
> +
> +	if (cci & UCSI_CCI_ACK_COMPLETE &&
> +	    test_bit(ACK_PENDING, &ucsi->flags))
> +		complete(&ucsi->complete);
> +
> +	if (cci & UCSI_CCI_COMMAND_COMPLETE &&
> +	    test_bit(COMMAND_PENDING, &ucsi->flags))
> +		complete(&ucsi->complete);

Hi Dmitry,

I've recently faced some race with ucsi_stm32g0 driver, and have sent a
fix for it [1], as you've noticed in the cover letter.

To fix that, I've used test_and_clear_bit() in above two cases, instead
of test_bit().

https://lore.kernel.org/linux-usb/20240612124656.2305603-1-fabrice.gasnier@foss.st.com/

> +}
> +EXPORT_SYMBOL_GPL(ucsi_notify_common);
> +
> +int ucsi_sync_control_common(struct ucsi *ucsi, u64 command)
> +{
> +	bool ack = UCSI_COMMAND(command) == UCSI_ACK_CC_CI;
> +	int ret;
> +
> +	if (ack)
> +		set_bit(ACK_PENDING, &ucsi->flags);
> +	else
> +		set_bit(COMMAND_PENDING, &ucsi->flags);
> +
> +	ret = ucsi->ops->async_control(ucsi, command);
> +	if (ret)
> +		goto out_clear_bit;
> +
> +	if (!wait_for_completion_timeout(&ucsi->complete, 5 * HZ))
> +		ret = -ETIMEDOUT;

With test_and_clear_bit(), could return 0, in case of success here.

I'd suggest to use similar approach here, unless you see some drawback?

Best Regards,
Fabrice

> +
> +out_clear_bit:
> +	if (ack)
> +		clear_bit(ACK_PENDING, &ucsi->flags);
> +	else
> +		clear_bit(COMMAND_PENDING, &ucsi->flags);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(ucsi_sync_control_common);
> +
>  static int ucsi_acknowledge(struct ucsi *ucsi, bool conn_ack)
>  {
>  	u64 ctrl;
> @@ -1883,6 +1925,7 @@ struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops)
>  	INIT_WORK(&ucsi->resume_work, ucsi_resume_work);
>  	INIT_DELAYED_WORK(&ucsi->work, ucsi_init_work);
>  	mutex_init(&ucsi->ppm_lock);
> +	init_completion(&ucsi->complete);
>  	ucsi->dev = dev;
>  	ucsi->ops = ops;

[snip]

>  	ucsi->ucsi = ucsi_create(dev, &pmic_glink_ucsi_ops);
> diff --git a/drivers/usb/typec/ucsi/ucsi_stm32g0.c b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
> index 14737ca3724c..d948c3f579e1 100644
> --- a/drivers/usb/typec/ucsi/ucsi_stm32g0.c
> +++ b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
> @@ -61,11 +61,7 @@ struct ucsi_stm32g0 {

[snip]

> -
> -	ret = ucsi_stm32g0_async_control(ucsi, command);
> -	if (ret)
> -		goto out_clear_bit;
> -
> -	if (!wait_for_completion_timeout(&g0->complete, msecs_to_jiffies(5000)))
> -		ret = -ETIMEDOUT;
> -	else
> -		return 0;
> -
> -out_clear_bit:
> -	if (ack)
> -		clear_bit(ACK_PENDING, &g0->flags);
> -	else
> -		clear_bit(COMMAND_PENDING, &g0->flags);
> -
> -	return ret;
> -}
> -
>  static irqreturn_t ucsi_stm32g0_irq_handler(int irq, void *data)
>  {
>  	struct ucsi_stm32g0 *g0 = data;
> @@ -449,13 +416,7 @@ static irqreturn_t ucsi_stm32g0_irq_handler(int irq, void *data)
>  	if (ret)
>  		return IRQ_NONE;
>  
> -	if (UCSI_CCI_CONNECTOR(cci))
> -		ucsi_connector_change(g0->ucsi, UCSI_CCI_CONNECTOR(cci));
> -
> -	if (cci & UCSI_CCI_ACK_COMPLETE && test_and_clear_bit(ACK_PENDING, &g0->flags))
> -		complete(&g0->complete);
> -	if (cci & UCSI_CCI_COMMAND_COMPLETE && test_and_clear_bit(COMMAND_PENDING, &g0->flags))
> -		complete(&g0->complete);
> +	ucsi_notify_common(g0->ucsi, cci);

I can see the fix "test_and_clear_bit()" sent earlier is removed from here.

I'd suggest to use similar approach as here, unless you see some drawback?

Please advise,
Best Regards,
Fabrice

Re: [Linux-stm32] [PATCH v2 6/7] usb: typec: ucsi: extract common code for command handling

[Dmitry Baryshkov]

On Tue, Jun 25, 2024 at 05:24:54PM GMT, Fabrice Gasnier wrote:
> On 6/21/24 00:55, Dmitry Baryshkov wrote:
> > Extract common functions to handle command sending and to handle events
> > from UCSI. This ensures that all UCSI glue drivers handle the ACKs in
> > the same way.
> > 
> > The CCG driver used DEV_CMD_PENDING both for internal
> > firmware-related commands and for UCSI control handling. Leave the
> > former use case intact.
> > 
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  drivers/usb/typec/ucsi/ucsi.c           | 43 +++++++++++++++++++++++++++
> >  drivers/usb/typec/ucsi/ucsi.h           |  7 +++++
> >  drivers/usb/typec/ucsi/ucsi_acpi.c      | 46 ++---------------------------
> >  drivers/usb/typec/ucsi/ucsi_ccg.c       | 21 ++-----------
> >  drivers/usb/typec/ucsi/ucsi_glink.c     | 47 ++---------------------------
> >  drivers/usb/typec/ucsi/ucsi_stm32g0.c   | 44 ++--------------------------
> >  drivers/usb/typec/ucsi/ucsi_yoga_c630.c | 52 ++-------------------------------
> >  7 files changed, 62 insertions(+), 198 deletions(-)
> > 
> > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> > index 4ba22323dbf9..691ee0c4ef87 100644
> > --- a/drivers/usb/typec/ucsi/ucsi.c
> > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > @@ -36,6 +36,48 @@
> >   */
> >  #define UCSI_SWAP_TIMEOUT_MS	5000
> >  
> > +void ucsi_notify_common(struct ucsi *ucsi, u32 cci)
> > +{
> > +	if (UCSI_CCI_CONNECTOR(cci))
> > +		ucsi_connector_change(ucsi, UCSI_CCI_CONNECTOR(cci));
> > +
> > +	if (cci & UCSI_CCI_ACK_COMPLETE &&
> > +	    test_bit(ACK_PENDING, &ucsi->flags))
> > +		complete(&ucsi->complete);
> > +
> > +	if (cci & UCSI_CCI_COMMAND_COMPLETE &&
> > +	    test_bit(COMMAND_PENDING, &ucsi->flags))
> > +		complete(&ucsi->complete);
> 
> Hi Dmitry,
> 
> I've recently faced some race with ucsi_stm32g0 driver, and have sent a
> fix for it [1], as you've noticed in the cover letter.
> 
> To fix that, I've used test_and_clear_bit() in above two cases, instead
> of test_bit().

Could you possible describe, why do you need test_and_clear_bit()
instead of just test_bit()? The bits are cleared at the end of the
.sync_write(), also there can be no other command (or ACK_CC) submission
before this one is fully processed.

> 
> https://lore.kernel.org/linux-usb/20240612124656.2305603-1-fabrice.gasnier@foss.st.com/
> 
> > +}
> > +EXPORT_SYMBOL_GPL(ucsi_notify_common);
> > +
> > +int ucsi_sync_control_common(struct ucsi *ucsi, u64 command)
> > +{
> > +	bool ack = UCSI_COMMAND(command) == UCSI_ACK_CC_CI;
> > +	int ret;
> > +
> > +	if (ack)
> > +		set_bit(ACK_PENDING, &ucsi->flags);
> > +	else
> > +		set_bit(COMMAND_PENDING, &ucsi->flags);
> > +
> > +	ret = ucsi->ops->async_control(ucsi, command);
> > +	if (ret)
> > +		goto out_clear_bit;
> > +
> > +	if (!wait_for_completion_timeout(&ucsi->complete, 5 * HZ))
> > +		ret = -ETIMEDOUT;
> 
> With test_and_clear_bit(), could return 0, in case of success here.

Oh, I see. So your code returns earlier. I have a feeling that this
approach is less logical and slightly harder to follow.

Maybe it's easier if it is implemented as:

if (wait_for_completion_timeout(...))
	return 0;

if (ack)
	clear_bit(ACK_PENDING)
else
	clear_bit(COMMAND_PENDING)

return -ETIMEDOUT;


OR

if (!wait_for_completion_timeout(...)) {
	if (ack)
		clear_bit(ACK_PENDING)
	else
		clear_bit(COMMAND_PENDING)

	return -ETIMEDOUT;
}

return 0;

But really, unless there is an actual issue with the current code, I'd
prefer to keep it. It makes it clear that the bits are set and then are
cleared properly.

> I'd suggest to use similar approach here, unless you see some drawback?
> 
> Best Regards,
> Fabrice
> 
> > +
> > +out_clear_bit:
> > +	if (ack)
> > +		clear_bit(ACK_PENDING, &ucsi->flags);
> > +	else
> > +		clear_bit(COMMAND_PENDING, &ucsi->flags);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(ucsi_sync_control_common);
> > +
> >  static int ucsi_acknowledge(struct ucsi *ucsi, bool conn_ack)
> >  {
> >  	u64 ctrl;
> > @@ -1883,6 +1925,7 @@ struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops)
> >  	INIT_WORK(&ucsi->resume_work, ucsi_resume_work);
> >  	INIT_DELAYED_WORK(&ucsi->work, ucsi_init_work);
> >  	mutex_init(&ucsi->ppm_lock);
> > +	init_completion(&ucsi->complete);
> >  	ucsi->dev = dev;
> >  	ucsi->ops = ops;
> 
> [snip]
> 
> >  	ucsi->ucsi = ucsi_create(dev, &pmic_glink_ucsi_ops);
> > diff --git a/drivers/usb/typec/ucsi/ucsi_stm32g0.c b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
> > index 14737ca3724c..d948c3f579e1 100644
> > --- a/drivers/usb/typec/ucsi/ucsi_stm32g0.c
> > +++ b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
> > @@ -61,11 +61,7 @@ struct ucsi_stm32g0 {
> 
> [snip]
> 
> > -
> > -	ret = ucsi_stm32g0_async_control(ucsi, command);
> > -	if (ret)
> > -		goto out_clear_bit;
> > -
> > -	if (!wait_for_completion_timeout(&g0->complete, msecs_to_jiffies(5000)))
> > -		ret = -ETIMEDOUT;
> > -	else
> > -		return 0;
> > -
> > -out_clear_bit:
> > -	if (ack)
> > -		clear_bit(ACK_PENDING, &g0->flags);
> > -	else
> > -		clear_bit(COMMAND_PENDING, &g0->flags);
> > -
> > -	return ret;
> > -}
> > -
> >  static irqreturn_t ucsi_stm32g0_irq_handler(int irq, void *data)
> >  {
> >  	struct ucsi_stm32g0 *g0 = data;
> > @@ -449,13 +416,7 @@ static irqreturn_t ucsi_stm32g0_irq_handler(int irq, void *data)
> >  	if (ret)
> >  		return IRQ_NONE;
> >  
> > -	if (UCSI_CCI_CONNECTOR(cci))
> > -		ucsi_connector_change(g0->ucsi, UCSI_CCI_CONNECTOR(cci));
> > -
> > -	if (cci & UCSI_CCI_ACK_COMPLETE && test_and_clear_bit(ACK_PENDING, &g0->flags))
> > -		complete(&g0->complete);
> > -	if (cci & UCSI_CCI_COMMAND_COMPLETE && test_and_clear_bit(COMMAND_PENDING, &g0->flags))
> > -		complete(&g0->complete);
> > +	ucsi_notify_common(g0->ucsi, cci);
> 
> I can see the fix "test_and_clear_bit()" sent earlier is removed from here.
> 
> I'd suggest to use similar approach as here, unless you see some drawback?
> 
> Please advise,
> Best Regards,
> Fabrice

-- 
With best wishes
Dmitry

Re: [Linux-stm32] [PATCH v2 6/7] usb: typec: ucsi: extract common code for command handling

[Fabrice Gasnier]

On 6/25/24 18:49, Dmitry Baryshkov wrote:
> On Tue, Jun 25, 2024 at 05:24:54PM GMT, Fabrice Gasnier wrote:
>> On 6/21/24 00:55, Dmitry Baryshkov wrote:
>>> Extract common functions to handle command sending and to handle events
>>> from UCSI. This ensures that all UCSI glue drivers handle the ACKs in
>>> the same way.
>>>
>>> The CCG driver used DEV_CMD_PENDING both for internal
>>> firmware-related commands and for UCSI control handling. Leave the
>>> former use case intact.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>  drivers/usb/typec/ucsi/ucsi.c           | 43 +++++++++++++++++++++++++++
>>>  drivers/usb/typec/ucsi/ucsi.h           |  7 +++++
>>>  drivers/usb/typec/ucsi/ucsi_acpi.c      | 46 ++---------------------------
>>>  drivers/usb/typec/ucsi/ucsi_ccg.c       | 21 ++-----------
>>>  drivers/usb/typec/ucsi/ucsi_glink.c     | 47 ++---------------------------
>>>  drivers/usb/typec/ucsi/ucsi_stm32g0.c   | 44 ++--------------------------
>>>  drivers/usb/typec/ucsi/ucsi_yoga_c630.c | 52 ++-------------------------------
>>>  7 files changed, 62 insertions(+), 198 deletions(-)
>>>
>>> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
>>> index 4ba22323dbf9..691ee0c4ef87 100644
>>> --- a/drivers/usb/typec/ucsi/ucsi.c
>>> +++ b/drivers/usb/typec/ucsi/ucsi.c
>>> @@ -36,6 +36,48 @@
>>>   */
>>>  #define UCSI_SWAP_TIMEOUT_MS	5000
>>>  
>>> +void ucsi_notify_common(struct ucsi *ucsi, u32 cci)
>>> +{
>>> +	if (UCSI_CCI_CONNECTOR(cci))
>>> +		ucsi_connector_change(ucsi, UCSI_CCI_CONNECTOR(cci));
>>> +
>>> +	if (cci & UCSI_CCI_ACK_COMPLETE &&
>>> +	    test_bit(ACK_PENDING, &ucsi->flags))
>>> +		complete(&ucsi->complete);
>>> +
>>> +	if (cci & UCSI_CCI_COMMAND_COMPLETE &&
>>> +	    test_bit(COMMAND_PENDING, &ucsi->flags))
>>> +		complete(&ucsi->complete);
>>
>> Hi Dmitry,
>>
>> I've recently faced some race with ucsi_stm32g0 driver, and have sent a
>> fix for it [1], as you've noticed in the cover letter.
>>
>> To fix that, I've used test_and_clear_bit() in above two cases, instead
>> of test_bit().
> 
> Could you possible describe, why do you need test_and_clear_bit()
> instead of just test_bit()? The bits are cleared at the end of the
> .sync_write(), also there can be no other command (or ACK_CC) submission
> before this one is fully processed.

Hi Dmitry,

It took me some time to reproduce this race I observed earlier.
(I observe this during DR swap.)

Once the ->async_control(UCSI_ACK_CC_CI) call bellow gets completed, and
before the ACK_PENDING bit gets cleared, e.g. clear_bit(ACK_PENDING), I
get an asynchronous interrupt.

Basically, Then the above complete() gets called (due to
UCSI_CCI_ACK_COMPLETE & ACK_PENDING).

Subsequent UCSI_GET_CONNECTOR_STATUS command (from
ucsi_handle_connector_change) will be unblocked immediately due to
complete() call has already happen, without UCSI_CCI_COMMAND_COMPLETE
cci flag, hence returning -EIO.

This is where the test_and_clear_bit() atomic operation helps, to avoid
non atomic operation:

-> async_control(UCSI_ACK_CC_CI)
new interrupt may occur here
-> clear_bit(ACK_PENDING)

> 
>>
>> https://lore.kernel.org/linux-usb/20240612124656.2305603-1-fabrice.gasnier@foss.st.com/
>>
>>> +}
>>> +EXPORT_SYMBOL_GPL(ucsi_notify_common);
>>> +
>>> +int ucsi_sync_control_common(struct ucsi *ucsi, u64 command)
>>> +{
>>> +	bool ack = UCSI_COMMAND(command) == UCSI_ACK_CC_CI;
>>> +	int ret;
>>> +
>>> +	if (ack)
>>> +		set_bit(ACK_PENDING, &ucsi->flags);
>>> +	else
>>> +		set_bit(COMMAND_PENDING, &ucsi->flags);
>>> +
>>> +	ret = ucsi->ops->async_control(ucsi, command);
>>> +	if (ret)
>>> +		goto out_clear_bit;
>>> +
>>> +	if (!wait_for_completion_timeout(&ucsi->complete, 5 * HZ))
>>> +		ret = -ETIMEDOUT;
>>
>> With test_and_clear_bit(), could return 0, in case of success here.
> 
> Oh, I see. So your code returns earlier. I have a feeling that this
> approach is less logical and slightly harder to follow.

By reading your proposal bellow, I'd agree with you.
> 
> Maybe it's easier if it is implemented as:
> 
> if (wait_for_completion_timeout(...))
> 	return 0;

Yes, sounds good to me.

> 
> if (ack)
> 	clear_bit(ACK_PENDING)
> else
> 	clear_bit(COMMAND_PENDING)
> 
> return -ETIMEDOUT;
> 
> 
> OR
> 
> if (!wait_for_completion_timeout(...)) {
> 	if (ack)
> 		clear_bit(ACK_PENDING)
> 	else
> 		clear_bit(COMMAND_PENDING)
> 
> 	return -ETIMEDOUT;
> }

Both seems fine.

Please advise,
BR,
Fabrice

> 
> return 0;
> 
> But really, unless there is an actual issue with the current code, I'd
> prefer to keep it. It makes it clear that the bits are set and then are
> cleared properly.
> 
>> I'd suggest to use similar approach here, unless you see some drawback?
>>
>> Best Regards,
>> Fabrice
>>
>>> +
>>> +out_clear_bit:
>>> +	if (ack)
>>> +		clear_bit(ACK_PENDING, &ucsi->flags);
>>> +	else
>>> +		clear_bit(COMMAND_PENDING, &ucsi->flags);
>>> +
>>> +	return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(ucsi_sync_control_common);
>>> +
>>>  static int ucsi_acknowledge(struct ucsi *ucsi, bool conn_ack)
>>>  {
>>>  	u64 ctrl;
>>> @@ -1883,6 +1925,7 @@ struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops)
>>>  	INIT_WORK(&ucsi->resume_work, ucsi_resume_work);
>>>  	INIT_DELAYED_WORK(&ucsi->work, ucsi_init_work);
>>>  	mutex_init(&ucsi->ppm_lock);
>>> +	init_completion(&ucsi->complete);
>>>  	ucsi->dev = dev;
>>>  	ucsi->ops = ops;
>>
>> [snip]
>>
>>>  	ucsi->ucsi = ucsi_create(dev, &pmic_glink_ucsi_ops);
>>> diff --git a/drivers/usb/typec/ucsi/ucsi_stm32g0.c b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
>>> index 14737ca3724c..d948c3f579e1 100644
>>> --- a/drivers/usb/typec/ucsi/ucsi_stm32g0.c
>>> +++ b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
>>> @@ -61,11 +61,7 @@ struct ucsi_stm32g0 {
>>
>> [snip]
>>
>>> -
>>> -	ret = ucsi_stm32g0_async_control(ucsi, command);
>>> -	if (ret)
>>> -		goto out_clear_bit;
>>> -
>>> -	if (!wait_for_completion_timeout(&g0->complete, msecs_to_jiffies(5000)))
>>> -		ret = -ETIMEDOUT;
>>> -	else
>>> -		return 0;
>>> -
>>> -out_clear_bit:
>>> -	if (ack)
>>> -		clear_bit(ACK_PENDING, &g0->flags);
>>> -	else
>>> -		clear_bit(COMMAND_PENDING, &g0->flags);
>>> -
>>> -	return ret;
>>> -}
>>> -
>>>  static irqreturn_t ucsi_stm32g0_irq_handler(int irq, void *data)
>>>  {
>>>  	struct ucsi_stm32g0 *g0 = data;
>>> @@ -449,13 +416,7 @@ static irqreturn_t ucsi_stm32g0_irq_handler(int irq, void *data)
>>>  	if (ret)
>>>  		return IRQ_NONE;
>>>  
>>> -	if (UCSI_CCI_CONNECTOR(cci))
>>> -		ucsi_connector_change(g0->ucsi, UCSI_CCI_CONNECTOR(cci));
>>> -
>>> -	if (cci & UCSI_CCI_ACK_COMPLETE && test_and_clear_bit(ACK_PENDING, &g0->flags))
>>> -		complete(&g0->complete);
>>> -	if (cci & UCSI_CCI_COMMAND_COMPLETE && test_and_clear_bit(COMMAND_PENDING, &g0->flags))
>>> -		complete(&g0->complete);
>>> +	ucsi_notify_common(g0->ucsi, cci);
>>
>> I can see the fix "test_and_clear_bit()" sent earlier is removed from here.
>>
>> I'd suggest to use similar approach as here, unless you see some drawback?
>>
>> Please advise,
>> Best Regards,
>> Fabrice
> 

Re: [Linux-stm32] [PATCH v2 6/7] usb: typec: ucsi: extract common code for command handling

[Dmitry Baryshkov]

On Thu, 27 Jun 2024 at 18:51, Fabrice Gasnier
<fabrice.gasnier@foss.st.com> wrote:
>
> On 6/25/24 18:49, Dmitry Baryshkov wrote:
> > On Tue, Jun 25, 2024 at 05:24:54PM GMT, Fabrice Gasnier wrote:
> >> On 6/21/24 00:55, Dmitry Baryshkov wrote:
> >>> Extract common functions to handle command sending and to handle events
> >>> from UCSI. This ensures that all UCSI glue drivers handle the ACKs in
> >>> the same way.
> >>>
> >>> The CCG driver used DEV_CMD_PENDING both for internal
> >>> firmware-related commands and for UCSI control handling. Leave the
> >>> former use case intact.
> >>>
> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>> ---
> >>>  drivers/usb/typec/ucsi/ucsi.c           | 43 +++++++++++++++++++++++++++
> >>>  drivers/usb/typec/ucsi/ucsi.h           |  7 +++++
> >>>  drivers/usb/typec/ucsi/ucsi_acpi.c      | 46 ++---------------------------
> >>>  drivers/usb/typec/ucsi/ucsi_ccg.c       | 21 ++-----------
> >>>  drivers/usb/typec/ucsi/ucsi_glink.c     | 47 ++---------------------------
> >>>  drivers/usb/typec/ucsi/ucsi_stm32g0.c   | 44 ++--------------------------
> >>>  drivers/usb/typec/ucsi/ucsi_yoga_c630.c | 52 ++-------------------------------
> >>>  7 files changed, 62 insertions(+), 198 deletions(-)
> >>>
> >>> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> >>> index 4ba22323dbf9..691ee0c4ef87 100644
> >>> --- a/drivers/usb/typec/ucsi/ucsi.c
> >>> +++ b/drivers/usb/typec/ucsi/ucsi.c
> >>> @@ -36,6 +36,48 @@
> >>>   */
> >>>  #define UCSI_SWAP_TIMEOUT_MS       5000
> >>>
> >>> +void ucsi_notify_common(struct ucsi *ucsi, u32 cci)
> >>> +{
> >>> +   if (UCSI_CCI_CONNECTOR(cci))
> >>> +           ucsi_connector_change(ucsi, UCSI_CCI_CONNECTOR(cci));
> >>> +
> >>> +   if (cci & UCSI_CCI_ACK_COMPLETE &&
> >>> +       test_bit(ACK_PENDING, &ucsi->flags))
> >>> +           complete(&ucsi->complete);
> >>> +
> >>> +   if (cci & UCSI_CCI_COMMAND_COMPLETE &&
> >>> +       test_bit(COMMAND_PENDING, &ucsi->flags))
> >>> +           complete(&ucsi->complete);
> >>
> >> Hi Dmitry,
> >>
> >> I've recently faced some race with ucsi_stm32g0 driver, and have sent a
> >> fix for it [1], as you've noticed in the cover letter.
> >>
> >> To fix that, I've used test_and_clear_bit() in above two cases, instead
> >> of test_bit().
> >
> > Could you possible describe, why do you need test_and_clear_bit()
> > instead of just test_bit()? The bits are cleared at the end of the
> > .sync_write(), also there can be no other command (or ACK_CC) submission
> > before this one is fully processed.
>
> Hi Dmitry,
>
> It took me some time to reproduce this race I observed earlier.
> (I observe this during DR swap.)
>
> Once the ->async_control(UCSI_ACK_CC_CI) call bellow gets completed, and
> before the ACK_PENDING bit gets cleared, e.g. clear_bit(ACK_PENDING), I
> get an asynchronous interrupt.
>
> Basically, Then the above complete() gets called (due to
> UCSI_CCI_ACK_COMPLETE & ACK_PENDING).
>
> Subsequent UCSI_GET_CONNECTOR_STATUS command (from
> ucsi_handle_connector_change) will be unblocked immediately due to
> complete() call has already happen, without UCSI_CCI_COMMAND_COMPLETE
> cci flag, hence returning -EIO.

But the ACK_CI is being sent as a response to a command. This means
that the ppm_lock should be locked. The UCSI_GET_CONNECTOR_STATUS
command should wait for ppm_lock to be freed and only then it can
proceed with sending the command. Maybe I'm misunderstanding the case
or maybe there is a loophole somewhere.

> This is where the test_and_clear_bit() atomic operation helps, to avoid
> non atomic operation:
>
> -> async_control(UCSI_ACK_CC_CI)
> new interrupt may occur here
> -> clear_bit(ACK_PENDING)
>
> >
> >>
> >> https://lore.kernel.org/linux-usb/20240612124656.2305603-1-fabrice.gasnier@foss.st.com/
> >>
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(ucsi_notify_common);
> >>> +
> >>> +int ucsi_sync_control_common(struct ucsi *ucsi, u64 command)
> >>> +{
> >>> +   bool ack = UCSI_COMMAND(command) == UCSI_ACK_CC_CI;
> >>> +   int ret;
> >>> +
> >>> +   if (ack)
> >>> +           set_bit(ACK_PENDING, &ucsi->flags);
> >>> +   else
> >>> +           set_bit(COMMAND_PENDING, &ucsi->flags);
> >>> +
> >>> +   ret = ucsi->ops->async_control(ucsi, command);
> >>> +   if (ret)
> >>> +           goto out_clear_bit;
> >>> +
> >>> +   if (!wait_for_completion_timeout(&ucsi->complete, 5 * HZ))
> >>> +           ret = -ETIMEDOUT;
> >>
> >> With test_and_clear_bit(), could return 0, in case of success here.
> >
> > Oh, I see. So your code returns earlier. I have a feeling that this
> > approach is less logical and slightly harder to follow.
>
> By reading your proposal bellow, I'd agree with you.
> >
> > Maybe it's easier if it is implemented as:
> >
> > if (wait_for_completion_timeout(...))
> >       return 0;
>
> Yes, sounds good to me.
>
> >
> > if (ack)
> >       clear_bit(ACK_PENDING)
> > else
> >       clear_bit(COMMAND_PENDING)
> >
> > return -ETIMEDOUT;
> >
> >
> > OR
> >
> > if (!wait_for_completion_timeout(...)) {
> >       if (ack)
> >               clear_bit(ACK_PENDING)
> >       else
> >               clear_bit(COMMAND_PENDING)
> >
> >       return -ETIMEDOUT;
> > }
>
> Both seems fine.
>
> Please advise,
> BR,
> Fabrice
>
> >
> > return 0;
> >
> > But really, unless there is an actual issue with the current code, I'd
> > prefer to keep it. It makes it clear that the bits are set and then are
> > cleared properly.
> >
> >> I'd suggest to use similar approach here, unless you see some drawback?
> >>
> >> Best Regards,
> >> Fabrice
> >>
> >>> +
> >>> +out_clear_bit:
> >>> +   if (ack)
> >>> +           clear_bit(ACK_PENDING, &ucsi->flags);
> >>> +   else
> >>> +           clear_bit(COMMAND_PENDING, &ucsi->flags);
> >>> +
> >>> +   return ret;
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(ucsi_sync_control_common);
> >>> +
> >>>  static int ucsi_acknowledge(struct ucsi *ucsi, bool conn_ack)
> >>>  {
> >>>     u64 ctrl;
> >>> @@ -1883,6 +1925,7 @@ struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops)
> >>>     INIT_WORK(&ucsi->resume_work, ucsi_resume_work);
> >>>     INIT_DELAYED_WORK(&ucsi->work, ucsi_init_work);
> >>>     mutex_init(&ucsi->ppm_lock);
> >>> +   init_completion(&ucsi->complete);
> >>>     ucsi->dev = dev;
> >>>     ucsi->ops = ops;
> >>
> >> [snip]
> >>
> >>>     ucsi->ucsi = ucsi_create(dev, &pmic_glink_ucsi_ops);
> >>> diff --git a/drivers/usb/typec/ucsi/ucsi_stm32g0.c b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
> >>> index 14737ca3724c..d948c3f579e1 100644
> >>> --- a/drivers/usb/typec/ucsi/ucsi_stm32g0.c
> >>> +++ b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
> >>> @@ -61,11 +61,7 @@ struct ucsi_stm32g0 {
> >>
> >> [snip]
> >>
> >>> -
> >>> -   ret = ucsi_stm32g0_async_control(ucsi, command);
> >>> -   if (ret)
> >>> -           goto out_clear_bit;
> >>> -
> >>> -   if (!wait_for_completion_timeout(&g0->complete, msecs_to_jiffies(5000)))
> >>> -           ret = -ETIMEDOUT;
> >>> -   else
> >>> -           return 0;
> >>> -
> >>> -out_clear_bit:
> >>> -   if (ack)
> >>> -           clear_bit(ACK_PENDING, &g0->flags);
> >>> -   else
> >>> -           clear_bit(COMMAND_PENDING, &g0->flags);
> >>> -
> >>> -   return ret;
> >>> -}
> >>> -
> >>>  static irqreturn_t ucsi_stm32g0_irq_handler(int irq, void *data)
> >>>  {
> >>>     struct ucsi_stm32g0 *g0 = data;
> >>> @@ -449,13 +416,7 @@ static irqreturn_t ucsi_stm32g0_irq_handler(int irq, void *data)
> >>>     if (ret)
> >>>             return IRQ_NONE;
> >>>
> >>> -   if (UCSI_CCI_CONNECTOR(cci))
> >>> -           ucsi_connector_change(g0->ucsi, UCSI_CCI_CONNECTOR(cci));
> >>> -
> >>> -   if (cci & UCSI_CCI_ACK_COMPLETE && test_and_clear_bit(ACK_PENDING, &g0->flags))
> >>> -           complete(&g0->complete);
> >>> -   if (cci & UCSI_CCI_COMMAND_COMPLETE && test_and_clear_bit(COMMAND_PENDING, &g0->flags))
> >>> -           complete(&g0->complete);
> >>> +   ucsi_notify_common(g0->ucsi, cci);
> >>
> >> I can see the fix "test_and_clear_bit()" sent earlier is removed from here.
> >>
> >> I'd suggest to use similar approach as here, unless you see some drawback?
> >>
> >> Please advise,
> >> Best Regards,
> >> Fabrice
> >



-- 
With best wishes
Dmitry

Re: [Linux-stm32] [PATCH v2 6/7] usb: typec: ucsi: extract common code for command handling

[Fabrice Gasnier]

On 6/27/24 17:58, Dmitry Baryshkov wrote:
> On Thu, 27 Jun 2024 at 18:51, Fabrice Gasnier
> <fabrice.gasnier@foss.st.com> wrote:
>>
>> On 6/25/24 18:49, Dmitry Baryshkov wrote:
>>> On Tue, Jun 25, 2024 at 05:24:54PM GMT, Fabrice Gasnier wrote:
>>>> On 6/21/24 00:55, Dmitry Baryshkov wrote:
>>>>> Extract common functions to handle command sending and to handle events
>>>>> from UCSI. This ensures that all UCSI glue drivers handle the ACKs in
>>>>> the same way.
>>>>>
>>>>> The CCG driver used DEV_CMD_PENDING both for internal
>>>>> firmware-related commands and for UCSI control handling. Leave the
>>>>> former use case intact.
>>>>>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>> ---
>>>>>  drivers/usb/typec/ucsi/ucsi.c           | 43 +++++++++++++++++++++++++++
>>>>>  drivers/usb/typec/ucsi/ucsi.h           |  7 +++++
>>>>>  drivers/usb/typec/ucsi/ucsi_acpi.c      | 46 ++---------------------------
>>>>>  drivers/usb/typec/ucsi/ucsi_ccg.c       | 21 ++-----------
>>>>>  drivers/usb/typec/ucsi/ucsi_glink.c     | 47 ++---------------------------
>>>>>  drivers/usb/typec/ucsi/ucsi_stm32g0.c   | 44 ++--------------------------
>>>>>  drivers/usb/typec/ucsi/ucsi_yoga_c630.c | 52 ++-------------------------------
>>>>>  7 files changed, 62 insertions(+), 198 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
>>>>> index 4ba22323dbf9..691ee0c4ef87 100644
>>>>> --- a/drivers/usb/typec/ucsi/ucsi.c
>>>>> +++ b/drivers/usb/typec/ucsi/ucsi.c
>>>>> @@ -36,6 +36,48 @@
>>>>>   */
>>>>>  #define UCSI_SWAP_TIMEOUT_MS       5000
>>>>>
>>>>> +void ucsi_notify_common(struct ucsi *ucsi, u32 cci)
>>>>> +{
>>>>> +   if (UCSI_CCI_CONNECTOR(cci))
>>>>> +           ucsi_connector_change(ucsi, UCSI_CCI_CONNECTOR(cci));
>>>>> +
>>>>> +   if (cci & UCSI_CCI_ACK_COMPLETE &&
>>>>> +       test_bit(ACK_PENDING, &ucsi->flags))
>>>>> +           complete(&ucsi->complete);
>>>>> +
>>>>> +   if (cci & UCSI_CCI_COMMAND_COMPLETE &&
>>>>> +       test_bit(COMMAND_PENDING, &ucsi->flags))
>>>>> +           complete(&ucsi->complete);
>>>>
>>>> Hi Dmitry,
>>>>
>>>> I've recently faced some race with ucsi_stm32g0 driver, and have sent a
>>>> fix for it [1], as you've noticed in the cover letter.
>>>>
>>>> To fix that, I've used test_and_clear_bit() in above two cases, instead
>>>> of test_bit().
>>>
>>> Could you possible describe, why do you need test_and_clear_bit()
>>> instead of just test_bit()? The bits are cleared at the end of the
>>> .sync_write(), also there can be no other command (or ACK_CC) submission
>>> before this one is fully processed.
>>
>> Hi Dmitry,
>>
>> It took me some time to reproduce this race I observed earlier.
>> (I observe this during DR swap.)
>>
>> Once the ->async_control(UCSI_ACK_CC_CI) call bellow gets completed, and
>> before the ACK_PENDING bit gets cleared, e.g. clear_bit(ACK_PENDING), I
>> get an asynchronous interrupt.
>>
>> Basically, Then the above complete() gets called (due to
>> UCSI_CCI_ACK_COMPLETE & ACK_PENDING).
>>
>> Subsequent UCSI_GET_CONNECTOR_STATUS command (from
>> ucsi_handle_connector_change) will be unblocked immediately due to
>> complete() call has already happen, without UCSI_CCI_COMMAND_COMPLETE
>> cci flag, hence returning -EIO.
> 
> But the ACK_CI is being sent as a response to a command. This means
> that the ppm_lock should be locked. The UCSI_GET_CONNECTOR_STATUS
> command should wait for ppm_lock to be freed and only then it can
> proceed with sending the command. Maybe I'm misunderstanding the case
> or maybe there is a loophole somewhere.

There's probably also some miss-understanding at my end, I don't get how
the ppm_lock would protext from non atomic async_control()/clear_bit().

Let me share some trace here, I hope it can better show the difference
at my end when I get the race.
I've added some debug prints around these lines, to trace the set/clear
of the COMMAND/ACK_PENDING, main commands and cci flags.

normal case is:
---
ucsi_send_command_common: UCSI_SET_UOR
set:COMMAND_PENDING
ucsi_stm32g0_irq_handler
ucsi_notify_common: UCSI_CCI_COMMAND_COMPLETE
clr:COMMAND_PENDING
ucsi_acknowledge: UCSI_ACK_COMMAND_COMPLETE UCSI_ACK_CONNECTOR_CHANGE
set:ACK_PENDING
ucsi_stm32g0_irq_handler
ucsi_notify_common: UCSI_CCI_ACK_COMPLETE
clr:ACK_PENDING
ucsi_stm32g0_irq_handler
ucsi_notify_common: ucsi_connector_change
ucsi_send_command_common: UCSI_GET_CONNECTOR_STATUS
set:COMMAND_PENDING
ucsi_stm32g0_irq_handler
ucsi_notify_common: UCSI_CCI_COMMAND_COMPLETE
clr:COMMAND_PENDING
ucsi_acknowledge: UCSI_ACK_COMMAND_COMPLETE
set:ACK_PENDING
ucsi_stm32g0_irq_handler
ucsi_notify_common: UCSI_CCI_ACK_COMPLETE
clr:ACK_PENDING

racy case:
---
ucsi_send_command_common: UCSI_SET_UOR
set:COMMAND_PENDING
ucsi_stm32g0_irq_handler
ucsi_notify_common: UCSI_CCI_COMMAND_COMPLETE
clr:COMMAND_PENDING
ucsi_acknowledge: UCSI_ACK_COMMAND_COMPLETE UCSI_ACK_CONNECTOR_CHANGE
set:ACK_PENDING
ucsi_stm32g0_irq_handler              <-- up to here nothing supicious
ucsi_notify_common: ucsi_connector_change
ucsi_notify_common: UCSI_CCI_ACK_COMPLETE
ucsi_stm32g0_irq_handler              <-- 2nd IRQ before clr:ACK_PENDING
ucsi_notify_common: ucsi_connector_change
ucsi_notify_common: UCSI_CCI_ACK_COMPLETE
clr:ACK_PENDING
ucsi_send_command_common: UCSI_GET_CONNECTOR_STATUS
set:COMMAND_PENDING                   <-- completion already done :-(
clr:COMMAND_PENDING
ucsi_handle_connector_change: GET_CONNECTOR_STATUS failed (-5)

I notice the two conditions are met above: both ucsi_connector_change()
and cci ack completed.

This happens before clear_bit(ACK_PENDING), which lead to anticipate
completion of the subsequent UCSI_GET_CONNECTOR_STATUS command.

So the test_and_clear_bit() would address a robustness case?

Please advise,
Best Regards,
Fabrice

> 
>> This is where the test_and_clear_bit() atomic operation helps, to avoid
>> non atomic operation:
>>
>> -> async_control(UCSI_ACK_CC_CI)
>> new interrupt may occur here
>> -> clear_bit(ACK_PENDING)
>>
>>>
>>>>
>>>> https://lore.kernel.org/linux-usb/20240612124656.2305603-1-fabrice.gasnier@foss.st.com/
>>>>
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(ucsi_notify_common);
>>>>> +
>>>>> +int ucsi_sync_control_common(struct ucsi *ucsi, u64 command)
>>>>> +{
>>>>> +   bool ack = UCSI_COMMAND(command) == UCSI_ACK_CC_CI;
>>>>> +   int ret;
>>>>> +
>>>>> +   if (ack)
>>>>> +           set_bit(ACK_PENDING, &ucsi->flags);
>>>>> +   else
>>>>> +           set_bit(COMMAND_PENDING, &ucsi->flags);
>>>>> +
>>>>> +   ret = ucsi->ops->async_control(ucsi, command);
>>>>> +   if (ret)
>>>>> +           goto out_clear_bit;
>>>>> +
>>>>> +   if (!wait_for_completion_timeout(&ucsi->complete, 5 * HZ))
>>>>> +           ret = -ETIMEDOUT;
>>>>
>>>> With test_and_clear_bit(), could return 0, in case of success here.
>>>
>>> Oh, I see. So your code returns earlier. I have a feeling that this
>>> approach is less logical and slightly harder to follow.
>>
>> By reading your proposal bellow, I'd agree with you.
>>>
>>> Maybe it's easier if it is implemented as:
>>>
>>> if (wait_for_completion_timeout(...))
>>>       return 0;
>>
>> Yes, sounds good to me.
>>
>>>
>>> if (ack)
>>>       clear_bit(ACK_PENDING)
>>> else
>>>       clear_bit(COMMAND_PENDING)
>>>
>>> return -ETIMEDOUT;
>>>
>>>
>>> OR
>>>
>>> if (!wait_for_completion_timeout(...)) {
>>>       if (ack)
>>>               clear_bit(ACK_PENDING)
>>>       else
>>>               clear_bit(COMMAND_PENDING)
>>>
>>>       return -ETIMEDOUT;
>>> }
>>
>> Both seems fine.
>>
>> Please advise,
>> BR,
>> Fabrice
>>
>>>
>>> return 0;
>>>
>>> But really, unless there is an actual issue with the current code, I'd
>>> prefer to keep it. It makes it clear that the bits are set and then are
>>> cleared properly.
>>>
>>>> I'd suggest to use similar approach here, unless you see some drawback?
>>>>
>>>> Best Regards,
>>>> Fabrice
>>>>
>>>>> +
>>>>> +out_clear_bit:
>>>>> +   if (ack)
>>>>> +           clear_bit(ACK_PENDING, &ucsi->flags);
>>>>> +   else
>>>>> +           clear_bit(COMMAND_PENDING, &ucsi->flags);
>>>>> +
>>>>> +   return ret;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(ucsi_sync_control_common);
>>>>> +
>>>>>  static int ucsi_acknowledge(struct ucsi *ucsi, bool conn_ack)
>>>>>  {
>>>>>     u64 ctrl;
>>>>> @@ -1883,6 +1925,7 @@ struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops)
>>>>>     INIT_WORK(&ucsi->resume_work, ucsi_resume_work);
>>>>>     INIT_DELAYED_WORK(&ucsi->work, ucsi_init_work);
>>>>>     mutex_init(&ucsi->ppm_lock);
>>>>> +   init_completion(&ucsi->complete);
>>>>>     ucsi->dev = dev;
>>>>>     ucsi->ops = ops;
>>>>
>>>> [snip]
>>>>
>>>>>     ucsi->ucsi = ucsi_create(dev, &pmic_glink_ucsi_ops);
>>>>> diff --git a/drivers/usb/typec/ucsi/ucsi_stm32g0.c b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
>>>>> index 14737ca3724c..d948c3f579e1 100644
>>>>> --- a/drivers/usb/typec/ucsi/ucsi_stm32g0.c
>>>>> +++ b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
>>>>> @@ -61,11 +61,7 @@ struct ucsi_stm32g0 {
>>>>
>>>> [snip]
>>>>
>>>>> -
>>>>> -   ret = ucsi_stm32g0_async_control(ucsi, command);
>>>>> -   if (ret)
>>>>> -           goto out_clear_bit;
>>>>> -
>>>>> -   if (!wait_for_completion_timeout(&g0->complete, msecs_to_jiffies(5000)))
>>>>> -           ret = -ETIMEDOUT;
>>>>> -   else
>>>>> -           return 0;
>>>>> -
>>>>> -out_clear_bit:
>>>>> -   if (ack)
>>>>> -           clear_bit(ACK_PENDING, &g0->flags);
>>>>> -   else
>>>>> -           clear_bit(COMMAND_PENDING, &g0->flags);
>>>>> -
>>>>> -   return ret;
>>>>> -}
>>>>> -
>>>>>  static irqreturn_t ucsi_stm32g0_irq_handler(int irq, void *data)
>>>>>  {
>>>>>     struct ucsi_stm32g0 *g0 = data;
>>>>> @@ -449,13 +416,7 @@ static irqreturn_t ucsi_stm32g0_irq_handler(int irq, void *data)
>>>>>     if (ret)
>>>>>             return IRQ_NONE;
>>>>>
>>>>> -   if (UCSI_CCI_CONNECTOR(cci))
>>>>> -           ucsi_connector_change(g0->ucsi, UCSI_CCI_CONNECTOR(cci));
>>>>> -
>>>>> -   if (cci & UCSI_CCI_ACK_COMPLETE && test_and_clear_bit(ACK_PENDING, &g0->flags))
>>>>> -           complete(&g0->complete);
>>>>> -   if (cci & UCSI_CCI_COMMAND_COMPLETE && test_and_clear_bit(COMMAND_PENDING, &g0->flags))
>>>>> -           complete(&g0->complete);
>>>>> +   ucsi_notify_common(g0->ucsi, cci);
>>>>
>>>> I can see the fix "test_and_clear_bit()" sent earlier is removed from here.
>>>>
>>>> I'd suggest to use similar approach as here, unless you see some drawback?
>>>>
>>>> Please advise,
>>>> Best Regards,
>>>> Fabrice
>>>
> 
> 
> 

Re: [Linux-stm32] [PATCH v2 6/7] usb: typec: ucsi: extract common code for command handling

[Dmitry Baryshkov]

On Thu, Jun 27, 2024 at 06:49:02PM GMT, Fabrice Gasnier wrote:
> On 6/27/24 17:58, Dmitry Baryshkov wrote:
> > On Thu, 27 Jun 2024 at 18:51, Fabrice Gasnier
> > <fabrice.gasnier@foss.st.com> wrote:
> >>
> >> On 6/25/24 18:49, Dmitry Baryshkov wrote:
> >>> On Tue, Jun 25, 2024 at 05:24:54PM GMT, Fabrice Gasnier wrote:
> >>>> On 6/21/24 00:55, Dmitry Baryshkov wrote:
> >>>>> Extract common functions to handle command sending and to handle events
> >>>>> from UCSI. This ensures that all UCSI glue drivers handle the ACKs in
> >>>>> the same way.
> >>>>>
> >>>>> The CCG driver used DEV_CMD_PENDING both for internal
> >>>>> firmware-related commands and for UCSI control handling. Leave the
> >>>>> former use case intact.
> >>>>>
> >>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>>>> ---
> >>>>>  drivers/usb/typec/ucsi/ucsi.c           | 43 +++++++++++++++++++++++++++
> >>>>>  drivers/usb/typec/ucsi/ucsi.h           |  7 +++++
> >>>>>  drivers/usb/typec/ucsi/ucsi_acpi.c      | 46 ++---------------------------
> >>>>>  drivers/usb/typec/ucsi/ucsi_ccg.c       | 21 ++-----------
> >>>>>  drivers/usb/typec/ucsi/ucsi_glink.c     | 47 ++---------------------------
> >>>>>  drivers/usb/typec/ucsi/ucsi_stm32g0.c   | 44 ++--------------------------
> >>>>>  drivers/usb/typec/ucsi/ucsi_yoga_c630.c | 52 ++-------------------------------
> >>>>>  7 files changed, 62 insertions(+), 198 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> >>>>> index 4ba22323dbf9..691ee0c4ef87 100644
> >>>>> --- a/drivers/usb/typec/ucsi/ucsi.c
> >>>>> +++ b/drivers/usb/typec/ucsi/ucsi.c
> >>>>> @@ -36,6 +36,48 @@
> >>>>>   */
> >>>>>  #define UCSI_SWAP_TIMEOUT_MS       5000
> >>>>>
> >>>>> +void ucsi_notify_common(struct ucsi *ucsi, u32 cci)
> >>>>> +{
> >>>>> +   if (UCSI_CCI_CONNECTOR(cci))
> >>>>> +           ucsi_connector_change(ucsi, UCSI_CCI_CONNECTOR(cci));
> >>>>> +
> >>>>> +   if (cci & UCSI_CCI_ACK_COMPLETE &&
> >>>>> +       test_bit(ACK_PENDING, &ucsi->flags))
> >>>>> +           complete(&ucsi->complete);
> >>>>> +
> >>>>> +   if (cci & UCSI_CCI_COMMAND_COMPLETE &&
> >>>>> +       test_bit(COMMAND_PENDING, &ucsi->flags))
> >>>>> +           complete(&ucsi->complete);
> >>>>
> >>>> Hi Dmitry,
> >>>>
> >>>> I've recently faced some race with ucsi_stm32g0 driver, and have sent a
> >>>> fix for it [1], as you've noticed in the cover letter.
> >>>>
> >>>> To fix that, I've used test_and_clear_bit() in above two cases, instead
> >>>> of test_bit().
> >>>
> >>> Could you possible describe, why do you need test_and_clear_bit()
> >>> instead of just test_bit()? The bits are cleared at the end of the
> >>> .sync_write(), also there can be no other command (or ACK_CC) submission
> >>> before this one is fully processed.
> >>
> >> Hi Dmitry,
> >>
> >> It took me some time to reproduce this race I observed earlier.
> >> (I observe this during DR swap.)
> >>
> >> Once the ->async_control(UCSI_ACK_CC_CI) call bellow gets completed, and
> >> before the ACK_PENDING bit gets cleared, e.g. clear_bit(ACK_PENDING), I
> >> get an asynchronous interrupt.
> >>
> >> Basically, Then the above complete() gets called (due to
> >> UCSI_CCI_ACK_COMPLETE & ACK_PENDING).
> >>
> >> Subsequent UCSI_GET_CONNECTOR_STATUS command (from
> >> ucsi_handle_connector_change) will be unblocked immediately due to
> >> complete() call has already happen, without UCSI_CCI_COMMAND_COMPLETE
> >> cci flag, hence returning -EIO.
> > 
> > But the ACK_CI is being sent as a response to a command. This means
> > that the ppm_lock should be locked. The UCSI_GET_CONNECTOR_STATUS
> > command should wait for ppm_lock to be freed and only then it can
> > proceed with sending the command. Maybe I'm misunderstanding the case
> > or maybe there is a loophole somewhere.
> 
> There's probably also some miss-understanding at my end, I don't get how
> the ppm_lock would protext from non atomic async_control()/clear_bit().
> 
> Let me share some trace here, I hope it can better show the difference
> at my end when I get the race.
> I've added some debug prints around these lines, to trace the set/clear
> of the COMMAND/ACK_PENDING, main commands and cci flags.
> 
> normal case is:
> ---
> ucsi_send_command_common: UCSI_SET_UOR
> set:COMMAND_PENDING
> ucsi_stm32g0_irq_handler
> ucsi_notify_common: UCSI_CCI_COMMAND_COMPLETE
> clr:COMMAND_PENDING
> ucsi_acknowledge: UCSI_ACK_COMMAND_COMPLETE UCSI_ACK_CONNECTOR_CHANGE
> set:ACK_PENDING
> ucsi_stm32g0_irq_handler
> ucsi_notify_common: UCSI_CCI_ACK_COMPLETE
> clr:ACK_PENDING
> ucsi_stm32g0_irq_handler
> ucsi_notify_common: ucsi_connector_change
> ucsi_send_command_common: UCSI_GET_CONNECTOR_STATUS
> set:COMMAND_PENDING
> ucsi_stm32g0_irq_handler
> ucsi_notify_common: UCSI_CCI_COMMAND_COMPLETE
> clr:COMMAND_PENDING
> ucsi_acknowledge: UCSI_ACK_COMMAND_COMPLETE
> set:ACK_PENDING
> ucsi_stm32g0_irq_handler
> ucsi_notify_common: UCSI_CCI_ACK_COMPLETE
> clr:ACK_PENDING
> 
> racy case:
> ---
> ucsi_send_command_common: UCSI_SET_UOR
> set:COMMAND_PENDING
> ucsi_stm32g0_irq_handler
> ucsi_notify_common: UCSI_CCI_COMMAND_COMPLETE
> clr:COMMAND_PENDING
> ucsi_acknowledge: UCSI_ACK_COMMAND_COMPLETE UCSI_ACK_CONNECTOR_CHANGE
> set:ACK_PENDING
> ucsi_stm32g0_irq_handler              <-- up to here nothing supicious
> ucsi_notify_common: ucsi_connector_change
> ucsi_notify_common: UCSI_CCI_ACK_COMPLETE
> ucsi_stm32g0_irq_handler              <-- 2nd IRQ before clr:ACK_PENDING
> ucsi_notify_common: ucsi_connector_change
> ucsi_notify_common: UCSI_CCI_ACK_COMPLETE
> clr:ACK_PENDING
> ucsi_send_command_common: UCSI_GET_CONNECTOR_STATUS
> set:COMMAND_PENDING                   <-- completion already done :-(
> clr:COMMAND_PENDING
> ucsi_handle_connector_change: GET_CONNECTOR_STATUS failed (-5)
> 
> I notice the two conditions are met above: both ucsi_connector_change()
> and cci ack completed.
> 
> This happens before clear_bit(ACK_PENDING), which lead to anticipate
> completion of the subsequent UCSI_GET_CONNECTOR_STATUS command.
> 
> So the test_and_clear_bit() would address a robustness case?

Ah, I see. So the race is for the completion. I fear that your solution
also doesn't fully solve the problem. The event can arrive after setting
the bit and before sending the command to the hardware. I thought about
doing various tricks, like reinit_completion or something close, but the
issue is that test_and_clear_bit() just makes the race window smaller,
but doesn't eliminate it completely.

It seems the only practical way to handle that is by making sure that
ucsi_notify_common() can sleep and locks the ppm_lock while
processing the CCI.

> 
> Please advise,
> Best Regards,
> Fabrice
> 
> > 
> >> This is where the test_and_clear_bit() atomic operation helps, to avoid
> >> non atomic operation:
> >>
> >> -> async_control(UCSI_ACK_CC_CI)
> >> new interrupt may occur here
> >> -> clear_bit(ACK_PENDING)

-- 
With best wishes
Dmitry

Re: [Linux-stm32] [PATCH v2 6/7] usb: typec: ucsi: extract common code for command handling

[Fabrice Gasnier]

On 6/27/24 22:14, Dmitry Baryshkov wrote:
> On Thu, Jun 27, 2024 at 06:49:02PM GMT, Fabrice Gasnier wrote:
>> On 6/27/24 17:58, Dmitry Baryshkov wrote:
>>> On Thu, 27 Jun 2024 at 18:51, Fabrice Gasnier
>>> <fabrice.gasnier@foss.st.com> wrote:
>>>>
>>>> On 6/25/24 18:49, Dmitry Baryshkov wrote:
>>>>> On Tue, Jun 25, 2024 at 05:24:54PM GMT, Fabrice Gasnier wrote:
>>>>>> On 6/21/24 00:55, Dmitry Baryshkov wrote:
>>>>>>> Extract common functions to handle command sending and to handle events
>>>>>>> from UCSI. This ensures that all UCSI glue drivers handle the ACKs in
>>>>>>> the same way.
>>>>>>>
>>>>>>> The CCG driver used DEV_CMD_PENDING both for internal
>>>>>>> firmware-related commands and for UCSI control handling. Leave the
>>>>>>> former use case intact.
>>>>>>>
>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>> ---
>>>>>>>  drivers/usb/typec/ucsi/ucsi.c           | 43 +++++++++++++++++++++++++++
>>>>>>>  drivers/usb/typec/ucsi/ucsi.h           |  7 +++++
>>>>>>>  drivers/usb/typec/ucsi/ucsi_acpi.c      | 46 ++---------------------------
>>>>>>>  drivers/usb/typec/ucsi/ucsi_ccg.c       | 21 ++-----------
>>>>>>>  drivers/usb/typec/ucsi/ucsi_glink.c     | 47 ++---------------------------
>>>>>>>  drivers/usb/typec/ucsi/ucsi_stm32g0.c   | 44 ++--------------------------
>>>>>>>  drivers/usb/typec/ucsi/ucsi_yoga_c630.c | 52 ++-------------------------------
>>>>>>>  7 files changed, 62 insertions(+), 198 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
>>>>>>> index 4ba22323dbf9..691ee0c4ef87 100644
>>>>>>> --- a/drivers/usb/typec/ucsi/ucsi.c
>>>>>>> +++ b/drivers/usb/typec/ucsi/ucsi.c
>>>>>>> @@ -36,6 +36,48 @@
>>>>>>>   */
>>>>>>>  #define UCSI_SWAP_TIMEOUT_MS       5000
>>>>>>>
>>>>>>> +void ucsi_notify_common(struct ucsi *ucsi, u32 cci)
>>>>>>> +{
>>>>>>> +   if (UCSI_CCI_CONNECTOR(cci))
>>>>>>> +           ucsi_connector_change(ucsi, UCSI_CCI_CONNECTOR(cci));
>>>>>>> +
>>>>>>> +   if (cci & UCSI_CCI_ACK_COMPLETE &&
>>>>>>> +       test_bit(ACK_PENDING, &ucsi->flags))
>>>>>>> +           complete(&ucsi->complete);
>>>>>>> +
>>>>>>> +   if (cci & UCSI_CCI_COMMAND_COMPLETE &&
>>>>>>> +       test_bit(COMMAND_PENDING, &ucsi->flags))
>>>>>>> +           complete(&ucsi->complete);
>>>>>>
>>>>>> Hi Dmitry,
>>>>>>
>>>>>> I've recently faced some race with ucsi_stm32g0 driver, and have sent a
>>>>>> fix for it [1], as you've noticed in the cover letter.
>>>>>>
>>>>>> To fix that, I've used test_and_clear_bit() in above two cases, instead
>>>>>> of test_bit().
>>>>>
>>>>> Could you possible describe, why do you need test_and_clear_bit()
>>>>> instead of just test_bit()? The bits are cleared at the end of the
>>>>> .sync_write(), also there can be no other command (or ACK_CC) submission
>>>>> before this one is fully processed.
>>>>
>>>> Hi Dmitry,
>>>>
>>>> It took me some time to reproduce this race I observed earlier.
>>>> (I observe this during DR swap.)
>>>>
>>>> Once the ->async_control(UCSI_ACK_CC_CI) call bellow gets completed, and
>>>> before the ACK_PENDING bit gets cleared, e.g. clear_bit(ACK_PENDING), I
>>>> get an asynchronous interrupt.
>>>>
>>>> Basically, Then the above complete() gets called (due to
>>>> UCSI_CCI_ACK_COMPLETE & ACK_PENDING).
>>>>
>>>> Subsequent UCSI_GET_CONNECTOR_STATUS command (from
>>>> ucsi_handle_connector_change) will be unblocked immediately due to
>>>> complete() call has already happen, without UCSI_CCI_COMMAND_COMPLETE
>>>> cci flag, hence returning -EIO.
>>>
>>> But the ACK_CI is being sent as a response to a command. This means
>>> that the ppm_lock should be locked. The UCSI_GET_CONNECTOR_STATUS
>>> command should wait for ppm_lock to be freed and only then it can
>>> proceed with sending the command. Maybe I'm misunderstanding the case
>>> or maybe there is a loophole somewhere.
>>
>> There's probably also some miss-understanding at my end, I don't get how
>> the ppm_lock would protext from non atomic async_control()/clear_bit().
>>
>> Let me share some trace here, I hope it can better show the difference
>> at my end when I get the race.
>> I've added some debug prints around these lines, to trace the set/clear
>> of the COMMAND/ACK_PENDING, main commands and cci flags.
>>
>> normal case is:
>> ---
>> ucsi_send_command_common: UCSI_SET_UOR
>> set:COMMAND_PENDING
>> ucsi_stm32g0_irq_handler
>> ucsi_notify_common: UCSI_CCI_COMMAND_COMPLETE
>> clr:COMMAND_PENDING
>> ucsi_acknowledge: UCSI_ACK_COMMAND_COMPLETE UCSI_ACK_CONNECTOR_CHANGE
>> set:ACK_PENDING
>> ucsi_stm32g0_irq_handler
>> ucsi_notify_common: UCSI_CCI_ACK_COMPLETE
>> clr:ACK_PENDING
>> ucsi_stm32g0_irq_handler
>> ucsi_notify_common: ucsi_connector_change
>> ucsi_send_command_common: UCSI_GET_CONNECTOR_STATUS
>> set:COMMAND_PENDING
>> ucsi_stm32g0_irq_handler
>> ucsi_notify_common: UCSI_CCI_COMMAND_COMPLETE
>> clr:COMMAND_PENDING
>> ucsi_acknowledge: UCSI_ACK_COMMAND_COMPLETE
>> set:ACK_PENDING
>> ucsi_stm32g0_irq_handler
>> ucsi_notify_common: UCSI_CCI_ACK_COMPLETE
>> clr:ACK_PENDING
>>
>> racy case:
>> ---
>> ucsi_send_command_common: UCSI_SET_UOR
>> set:COMMAND_PENDING
>> ucsi_stm32g0_irq_handler
>> ucsi_notify_common: UCSI_CCI_COMMAND_COMPLETE
>> clr:COMMAND_PENDING
>> ucsi_acknowledge: UCSI_ACK_COMMAND_COMPLETE UCSI_ACK_CONNECTOR_CHANGE
>> set:ACK_PENDING
>> ucsi_stm32g0_irq_handler              <-- up to here nothing supicious
>> ucsi_notify_common: ucsi_connector_change
>> ucsi_notify_common: UCSI_CCI_ACK_COMPLETE
>> ucsi_stm32g0_irq_handler              <-- 2nd IRQ before clr:ACK_PENDING
>> ucsi_notify_common: ucsi_connector_change
>> ucsi_notify_common: UCSI_CCI_ACK_COMPLETE
>> clr:ACK_PENDING
>> ucsi_send_command_common: UCSI_GET_CONNECTOR_STATUS
>> set:COMMAND_PENDING                   <-- completion already done :-(
>> clr:COMMAND_PENDING
>> ucsi_handle_connector_change: GET_CONNECTOR_STATUS failed (-5)
>>
>> I notice the two conditions are met above: both ucsi_connector_change()
>> and cci ack completed.
>>
>> This happens before clear_bit(ACK_PENDING), which lead to anticipate
>> completion of the subsequent UCSI_GET_CONNECTOR_STATUS command.
>>
>> So the test_and_clear_bit() would address a robustness case?
> 
> Ah, I see. So the race is for the completion. I fear that your solution
> also doesn't fully solve the problem. The event can arrive after setting
> the bit and before sending the command to the hardware. I thought about
> doing various tricks, like reinit_completion or something close, but the
> issue is that test_and_clear_bit() just makes the race window smaller,
> but doesn't eliminate it completely.

Hi Dmitry,

I've done some tests with reinit_completion(), when entering the
ucsi_sync_control_common() routine:

+	reinit_completion(&ucsi->complete);

	if (ack)
		set_bit(ACK_PENDING, &ucsi->flags);
	else
		set_bit(COMMAND_PENDING, &ucsi->flags);


This indeed avoids the race on the completion at my end.
With that, I longer get the error on the subsequent
UCSI_GET_CONNECTOR_STATUS command.

It looks like a better compromise to me, than test_and_clear_bit() as
you pointed out.

Best Regards,
Fabrice

> 
> It seems the only practical way to handle that is by making sure that
> ucsi_notify_common() can sleep and locks the ppm_lock while
> processing the CCI.
> 
>>
>> Please advise,
>> Best Regards,
>> Fabrice
>>
>>>
>>>> This is where the test_and_clear_bit() atomic operation helps, to avoid
>>>> non atomic operation:
>>>>
>>>> -> async_control(UCSI_ACK_CC_CI)
>>>> new interrupt may occur here
>>>> -> clear_bit(ACK_PENDING)
>