[PATCH 0/7] usb: typec: ucsi: Driver improvements

[PATCH 0/7] usb: typec: ucsi: Driver improvements

[Heikki Krogerus]

Hi,

The goal of this series was to improve the alt mode handling in the
driver, but now it seems that we can use the "poll worker" that was
introduced for that to handle other tasks better as well.

Ulrich reported some problems that are caused by the second
GET_CONNECTOR_STATUS right after the first one that was introduced in
217504a05532 ("usb: typec: ucsi: Work around PPM losing change
information"). In the last patch I try to improve that workaround by
extracting it out of the generic event handler into its own task and
executing it only when it's really needed. That seems to improve the
situation.

These patches definitely improve the quality of the driver by making
it a bit more readable, but they also appear to make the behaviour a
bit more predictably and uniform on different platforms.

Benjamin, can you test these?

thanks,

Heikki Krogerus (7):
  usb: typec: ucsi: Always cancel the command if PPM reports BUSY
    condition
  usb: typec: ucsi: Don't stop alt mode registration on busy condition
  usb: typec: ucsi: Add polling mechanism for partner tasks like alt
    mode checking
  usb: typec: ucsi: acpi: Reduce the command completion timeout
  usb: typec: ucsi: Check the partner alt modes always if there is PD
    contract
  usb: typec: ucsi: Read the PDOs in separate work
  usb: typec: ucsi: Better fix for missing unplug events issue

 drivers/usb/typec/ucsi/ucsi.c      | 337 ++++++++++++++---------------
 drivers/usb/typec/ucsi/ucsi.h      |   3 +-
 drivers/usb/typec/ucsi/ucsi_acpi.c |   2 +-
 3 files changed, 167 insertions(+), 175 deletions(-)

-- 
2.33.0

[PATCH 1/7] usb: typec: ucsi: Always cancel the command if PPM reports BUSY condition

[Heikki Krogerus]

This makes it possible to execute next command immediately
after the busy condition.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/ucsi/ucsi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 5ef5bd0e87cf2..ffb5be51daf85 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -128,8 +128,10 @@ static int ucsi_exec_command(struct ucsi *ucsi, u64 cmd)
 	if (ret)
 		return ret;
 
-	if (cci & UCSI_CCI_BUSY)
+	if (cci & UCSI_CCI_BUSY) {
+		ucsi->ops->async_write(ucsi, UCSI_CANCEL, NULL, 0);
 		return -EBUSY;
+	}
 
 	if (!(cci & UCSI_CCI_COMMAND_COMPLETE))
 		return -EIO;
-- 
2.33.0

[PATCH 2/7] usb: typec: ucsi: Don't stop alt mode registration on busy condition

[Heikki Krogerus]

If the PPM says it's busy, we can now simply try again.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/ucsi/ucsi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index ffb5be51daf85..a35efd8174bd4 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -437,6 +437,8 @@ static int ucsi_register_altmodes(struct ucsi_connector *con, u8 recipient)
 		command |= UCSI_GET_ALTMODE_CONNECTOR_NUMBER(con->num);
 		command |= UCSI_GET_ALTMODE_OFFSET(i);
 		len = ucsi_send_command(con->ucsi, command, alt, sizeof(alt));
+		if (len == -EBUSY)
+			continue;
 		if (len <= 0)
 			return len;
 
-- 
2.33.0

[PATCH 3/7] usb: typec: ucsi: Add polling mechanism for partner tasks like alt mode checking

[Heikki Krogerus]

The "poll worker" that is introduced here is first used for
checking partner alternate modes, but it can later be used
for any partner task that requires a separate job to be
scheduled to the connector specific workqueues.

The mechanism allows the partner device specific tasks to be
polling tasks and also delayed tasks if necessary.

By polling the partner alternate modes with this mechanism
the long command completion timeout value can be reduced
back to normal. The long command completion timeout was only
used to work around a problem on some platforms where the EC
firmware (PPM) didn't return BUSY even when it should with
the alt mode commands.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/ucsi/ucsi.c | 119 ++++++++++++++++++++++++++++------
 drivers/usb/typec/ucsi/ucsi.h |   1 +
 2 files changed, 101 insertions(+), 19 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index a35efd8174bd4..8af292141fe7b 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -191,6 +191,64 @@ int ucsi_resume(struct ucsi *ucsi)
 EXPORT_SYMBOL_GPL(ucsi_resume);
 /* -------------------------------------------------------------------------- */
 
+struct ucsi_work {
+	struct delayed_work work;
+	unsigned long delay;
+	unsigned int count;
+	struct ucsi_connector *con;
+	int (*cb)(struct ucsi_connector *);
+};
+
+static void ucsi_poll_worker(struct work_struct *work)
+{
+	struct ucsi_work *uwork = container_of(work, struct ucsi_work, work.work);
+	struct ucsi_connector *con = uwork->con;
+	int ret;
+
+	mutex_lock(&con->lock);
+
+	if (!con->partner) {
+		mutex_unlock(&con->lock);
+		kfree(uwork);
+		return;
+	}
+
+	ret = uwork->cb(con);
+
+	if (uwork->count-- && (ret == -EBUSY || ret == -ETIMEDOUT))
+		queue_delayed_work(con->wq, &uwork->work, uwork->delay);
+	else
+		kfree(uwork);
+
+	mutex_unlock(&con->lock);
+}
+
+static int ucsi_partner_task(struct ucsi_connector *con,
+			     int (*cb)(struct ucsi_connector *),
+			     int retries, unsigned long delay)
+{
+	struct ucsi_work *uwork;
+
+	if (!con->partner)
+		return 0;
+
+	uwork = kzalloc(sizeof(*uwork), GFP_KERNEL);
+	if (!uwork)
+		return -ENOMEM;
+
+	INIT_DELAYED_WORK(&uwork->work, ucsi_poll_worker);
+	uwork->count = retries;
+	uwork->delay = delay;
+	uwork->con = con;
+	uwork->cb = cb;
+
+	queue_delayed_work(con->wq, &uwork->work, delay);
+
+	return 0;
+}
+
+/* -------------------------------------------------------------------------- */
+
 void ucsi_altmode_update_active(struct ucsi_connector *con)
 {
 	const struct typec_altmode *altmode = NULL;
@@ -543,6 +601,25 @@ static void ucsi_get_src_pdos(struct ucsi_connector *con, int is_partner)
 	con->num_pdos += ret / sizeof(u32);
 }
 
+static int ucsi_check_altmodes(struct ucsi_connector *con)
+{
+	int ret;
+
+	ret = ucsi_register_altmodes(con, UCSI_RECIPIENT_SOP);
+	if (ret && ret != -ETIMEDOUT)
+		dev_err(con->ucsi->dev,
+			"con%d: failed to register partner alt modes (%d)\n",
+			con->num, ret);
+
+	/* Ignoring the errors in this case. */
+	if (con->partner_altmode[0]) {
+		ucsi_altmode_update_active(con);
+		return 0;
+	}
+
+	return ret;
+}
+
 static void ucsi_pwr_opmode_change(struct ucsi_connector *con)
 {
 	switch (UCSI_CONSTAT_PWR_OPMODE(con->status.flags)) {
@@ -650,14 +727,7 @@ static void ucsi_partner_change(struct ucsi_connector *con)
 		dev_err(con->ucsi->dev, "con:%d: failed to set usb role:%d\n",
 			con->num, u_role);
 
-	/* Can't rely on Partner Flags field. Always checking the alt modes. */
-	ret = ucsi_register_altmodes(con, UCSI_RECIPIENT_SOP);
-	if (ret)
-		dev_err(con->ucsi->dev,
-			"con%d: failed to register partner alternate modes\n",
-			con->num);
-	else
-		ucsi_altmode_update_active(con);
+	ucsi_partner_task(con, ucsi_check_altmodes, 30, 0);
 }
 
 static void ucsi_handle_connector_change(struct work_struct *work)
@@ -1045,8 +1115,18 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
 	enum typec_accessory *accessory = cap->accessory;
 	enum usb_role u_role = USB_ROLE_NONE;
 	u64 command;
+	char *name;
 	int ret;
 
+	name = kasprintf(GFP_KERNEL, "%s-con%d", dev_name(ucsi->dev), con->num);
+	if (!name)
+		return -ENOMEM;
+
+	con->wq = create_singlethread_workqueue(name);
+	kfree(name);
+	if (!con->wq)
+		return -ENOMEM;
+
 	INIT_WORK(&con->work, ucsi_handle_connector_change);
 	init_completion(&con->complete);
 	mutex_init(&con->lock);
@@ -1164,17 +1244,8 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
 		ret = 0;
 	}
 
-	if (con->partner) {
-		ret = ucsi_register_altmodes(con, UCSI_RECIPIENT_SOP);
-		if (ret) {
-			dev_err(ucsi->dev,
-				"con%d: failed to register alternate modes\n",
-				con->num);
-			ret = 0;
-		} else {
-			ucsi_altmode_update_active(con);
-		}
-	}
+	if (con->partner)
+		ucsi_check_altmodes(con);
 
 	trace_ucsi_register_port(con->num, &con->status);
 
@@ -1182,6 +1253,12 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
 	fwnode_handle_put(cap->fwnode);
 out_unlock:
 	mutex_unlock(&con->lock);
+
+	if (ret && con->wq) {
+		destroy_workqueue(con->wq);
+		con->wq = NULL;
+	}
+
 	return ret;
 }
 
@@ -1252,6 +1329,8 @@ static int ucsi_init(struct ucsi *ucsi)
 		ucsi_unregister_partner(con);
 		ucsi_unregister_altmodes(con, UCSI_RECIPIENT_CON);
 		ucsi_unregister_port_psy(con);
+		if (con->wq)
+			destroy_workqueue(con->wq);
 		typec_unregister_port(con->port);
 		con->port = NULL;
 	}
@@ -1374,6 +1453,8 @@ void ucsi_unregister(struct ucsi *ucsi)
 		ucsi_unregister_altmodes(&ucsi->connector[i],
 					 UCSI_RECIPIENT_CON);
 		ucsi_unregister_port_psy(&ucsi->connector[i]);
+		if (ucsi->connector[i].wq)
+			destroy_workqueue(ucsi->connector[i].wq);
 		typec_unregister_port(ucsi->connector[i].port);
 	}
 
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index cee666790907e..d10b8c24435af 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -317,6 +317,7 @@ struct ucsi_connector {
 	struct mutex lock; /* port lock */
 	struct work_struct work;
 	struct completion complete;
+	struct workqueue_struct *wq;
 
 	struct typec_port *port;
 	struct typec_partner *partner;
-- 
2.33.0

[PATCH 4/7] usb: typec: ucsi: acpi: Reduce the command completion timeout

[Heikki Krogerus]

The huge delay was there to workaround a problem where the
firmware did not report that it was busy with the alternate
mode commands. Now that the alternate modes are polled, the
delay can be dropped.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/ucsi/ucsi_acpi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
index 04976435ad736..6771f05e32c29 100644
--- a/drivers/usb/typec/ucsi/ucsi_acpi.c
+++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
@@ -78,7 +78,7 @@ static int ucsi_acpi_sync_write(struct ucsi *ucsi, unsigned int offset,
 	if (ret)
 		goto out_clear_bit;
 
-	if (!wait_for_completion_timeout(&ua->complete, 60 * HZ))
+	if (!wait_for_completion_timeout(&ua->complete, HZ))
 		ret = -ETIMEDOUT;
 
 out_clear_bit:
-- 
2.33.0

[PATCH 5/7] usb: typec: ucsi: Check the partner alt modes always if there is PD contract

[Heikki Krogerus]

UCSI does not tell the driver explicitly when the firmware
(PPM in UCSI lingo) has actually detected the partner
alternate modes, there is no specific change event for that.
That's why they have to be checked with any notification
that informs that PD contract with that partner has been
achieved.

Previously the alternate modes were checked always when the
firmware (PPM) informed that something with the partner had
changed, but on some platforms the EC firmware does not
generate separate events for generic partner changes at all.
On those platforms the EC firmware notifies the driver only
about connections, or separately about the PD contract if it
was not achieved soon enough after the initial connection
event.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/ucsi/ucsi.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 8af292141fe7b..188181737acf0 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -627,6 +627,7 @@ static void ucsi_pwr_opmode_change(struct ucsi_connector *con)
 		con->rdo = con->status.request_data_obj;
 		typec_set_pwr_opmode(con->port, TYPEC_PWR_MODE_PD);
 		ucsi_get_src_pdos(con, 1);
+		ucsi_partner_task(con, ucsi_check_altmodes, 30, 0);
 		break;
 	case UCSI_CONSTAT_PWR_OPMODE_TYPEC1_5:
 		con->rdo = 0;
@@ -726,8 +727,6 @@ static void ucsi_partner_change(struct ucsi_connector *con)
 	if (ret)
 		dev_err(con->ucsi->dev, "con:%d: failed to set usb role:%d\n",
 			con->num, u_role);
-
-	ucsi_partner_task(con, ucsi_check_altmodes, 30, 0);
 }
 
 static void ucsi_handle_connector_change(struct work_struct *work)
@@ -878,10 +877,15 @@ static void ucsi_handle_connector_change(struct work_struct *work)
 			break;
 		}
 
-		if (con->status.flags & UCSI_CONSTAT_CONNECTED)
+		if (con->status.flags & UCSI_CONSTAT_CONNECTED) {
 			ucsi_register_partner(con);
-		else
+
+			if (UCSI_CONSTAT_PWR_OPMODE(con->status.flags) ==
+			    UCSI_CONSTAT_PWR_OPMODE_PD)
+				ucsi_partner_task(con, ucsi_check_altmodes, 30, 0);
+		} else {
 			ucsi_unregister_partner(con);
+		}
 
 		ucsi_port_psy_changed(con);
 
@@ -1244,7 +1248,7 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
 		ret = 0;
 	}
 
-	if (con->partner)
+	if (UCSI_CONSTAT_PWR_OPMODE(con->status.flags) == UCSI_CONSTAT_PWR_OPMODE_PD)
 		ucsi_check_altmodes(con);
 
 	trace_ucsi_register_port(con->num, &con->status);
-- 
2.33.0

[PATCH 6/7] usb: typec: ucsi: Read the PDOs in separate work

[Heikki Krogerus]

Polling also the PDOs, just like the alt modes.

After this ucsi_handle_connector_change() doesn't execute
any commands.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/ucsi/ucsi.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 188181737acf0..e7267e47c3e4d 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -571,7 +571,7 @@ static int ucsi_get_pdos(struct ucsi_connector *con, int is_partner,
 	command |= UCSI_GET_PDOS_SRC_PDOS;
 	ret = ucsi_send_command(ucsi, command, pdos + offset,
 				num_pdos * sizeof(u32));
-	if (ret < 0)
+	if (ret < 0 && ret != -ETIMEDOUT)
 		dev_err(ucsi->dev, "UCSI_GET_PDOS failed (%d)\n", ret);
 	if (ret == 0 && offset == 0)
 		dev_warn(ucsi->dev, "UCSI_GET_PDOS returned 0 bytes\n");
@@ -579,26 +579,30 @@ static int ucsi_get_pdos(struct ucsi_connector *con, int is_partner,
 	return ret;
 }
 
-static void ucsi_get_src_pdos(struct ucsi_connector *con, int is_partner)
+static int ucsi_get_src_pdos(struct ucsi_connector *con)
 {
 	int ret;
 
 	/* UCSI max payload means only getting at most 4 PDOs at a time */
 	ret = ucsi_get_pdos(con, 1, con->src_pdos, 0, UCSI_MAX_PDOS);
 	if (ret < 0)
-		return;
+		return ret;
 
 	con->num_pdos = ret / sizeof(u32); /* number of bytes to 32-bit PDOs */
 	if (con->num_pdos < UCSI_MAX_PDOS)
-		return;
+		return 0;
 
 	/* get the remaining PDOs, if any */
 	ret = ucsi_get_pdos(con, 1, con->src_pdos, UCSI_MAX_PDOS,
 			    PDO_MAX_OBJECTS - UCSI_MAX_PDOS);
 	if (ret < 0)
-		return;
+		return ret;
 
 	con->num_pdos += ret / sizeof(u32);
+
+	ucsi_port_psy_changed(con);
+
+	return 0;
 }
 
 static int ucsi_check_altmodes(struct ucsi_connector *con)
@@ -626,7 +630,7 @@ static void ucsi_pwr_opmode_change(struct ucsi_connector *con)
 	case UCSI_CONSTAT_PWR_OPMODE_PD:
 		con->rdo = con->status.request_data_obj;
 		typec_set_pwr_opmode(con->port, TYPEC_PWR_MODE_PD);
-		ucsi_get_src_pdos(con, 1);
+		ucsi_partner_task(con, ucsi_get_src_pdos, 30, 0);
 		ucsi_partner_task(con, ucsi_check_altmodes, 30, 0);
 		break;
 	case UCSI_CONSTAT_PWR_OPMODE_TYPEC1_5:
@@ -844,12 +848,6 @@ static void ucsi_handle_connector_change(struct work_struct *work)
 
 	role = !!(con->status.flags & UCSI_CONSTAT_PWR_DIR);
 
-	if (con->status.change & UCSI_CONSTAT_POWER_OPMODE_CHANGE ||
-	    con->status.change & UCSI_CONSTAT_POWER_LEVEL_CHANGE) {
-		ucsi_pwr_opmode_change(con);
-		ucsi_port_psy_changed(con);
-	}
-
 	if (con->status.change & UCSI_CONSTAT_POWER_DIR_CHANGE) {
 		typec_set_pwr_role(con->port, role);
 
@@ -900,6 +898,10 @@ static void ucsi_handle_connector_change(struct work_struct *work)
 				con->num, u_role);
 	}
 
+	if (con->status.change & UCSI_CONSTAT_POWER_OPMODE_CHANGE ||
+	    con->status.change & UCSI_CONSTAT_POWER_LEVEL_CHANGE)
+		ucsi_pwr_opmode_change(con);
+
 	if (con->status.change & UCSI_CONSTAT_PARTNER_CHANGE)
 		ucsi_partner_change(con);
 
@@ -1248,8 +1250,10 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
 		ret = 0;
 	}
 
-	if (UCSI_CONSTAT_PWR_OPMODE(con->status.flags) == UCSI_CONSTAT_PWR_OPMODE_PD)
+	if (UCSI_CONSTAT_PWR_OPMODE(con->status.flags) == UCSI_CONSTAT_PWR_OPMODE_PD) {
+		ucsi_get_src_pdos(con);
 		ucsi_check_altmodes(con);
+	}
 
 	trace_ucsi_register_port(con->num, &con->status);
 
-- 
2.33.0

[PATCH 7/7] usb: typec: ucsi: Better fix for missing unplug events issue

[Heikki Krogerus]

The commit 217504a05532 ("usb: typec: ucsi: Work around PPM
losing change information") had solved this issue
previously, but in a really complex manner. The core issue
is that on some platforms the EC firmware does not interrupt
the driver on unplug event in some cases, mainly when the
cable is unplugged immediately after the plug-in.

From now on handling that problem by simply re-checking new
connections.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/ucsi/ucsi.c | 190 ++++++++--------------------------
 drivers/usb/typec/ucsi/ucsi.h |   2 -
 2 files changed, 45 insertions(+), 147 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index e7267e47c3e4d..6aa28384f77f1 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -700,9 +700,6 @@ static void ucsi_partner_change(struct ucsi_connector *con)
 	enum usb_role u_role = USB_ROLE_NONE;
 	int ret;
 
-	if (!con->partner)
-		return;
-
 	switch (UCSI_CONSTAT_PARTNER_TYPE(con->status.flags)) {
 	case UCSI_CONSTAT_PARTNER_TYPE_UFP:
 	case UCSI_CONSTAT_PARTNER_TYPE_CABLE_AND_UFP:
@@ -719,10 +716,6 @@ static void ucsi_partner_change(struct ucsi_connector *con)
 		break;
 	}
 
-	/* Complete pending data role swap */
-	if (!completion_done(&con->complete))
-		complete(&con->complete);
-
 	/* Only notify USB controller if partner supports USB data */
 	if (!(UCSI_CONSTAT_PARTNER_FLAGS(con->status.flags) & UCSI_CONSTAT_PARTNER_FLAG_USB))
 		u_role = USB_ROLE_NONE;
@@ -733,118 +726,51 @@ static void ucsi_partner_change(struct ucsi_connector *con)
 			con->num, u_role);
 }
 
+static int ucsi_check_connection(struct ucsi_connector *con)
+{
+	u64 command;
+	int ret;
+
+	command = UCSI_GET_CONNECTOR_STATUS | UCSI_CONNECTOR_NUMBER(con->num);
+	ret = ucsi_send_command(con->ucsi, command, &con->status, sizeof(con->status));
+	if (ret < 0) {
+		dev_err(con->ucsi->dev, "GET_CONNECTOR_STATUS failed (%d)\n", ret);
+		return ret;
+	}
+
+	if (con->status.flags & UCSI_CONSTAT_CONNECTED) {
+		if (UCSI_CONSTAT_PWR_OPMODE(con->status.flags) ==
+		    UCSI_CONSTAT_PWR_OPMODE_PD)
+			ucsi_partner_task(con, ucsi_check_altmodes, 30, 0);
+	} else {
+		ucsi_partner_change(con);
+		ucsi_port_psy_changed(con);
+		ucsi_unregister_partner(con);
+	}
+
+	return 0;
+}
+
 static void ucsi_handle_connector_change(struct work_struct *work)
 {
 	struct ucsi_connector *con = container_of(work, struct ucsi_connector,
 						  work);
 	struct ucsi *ucsi = con->ucsi;
-	struct ucsi_connector_status pre_ack_status;
-	struct ucsi_connector_status post_ack_status;
 	enum typec_role role;
-	enum usb_role u_role = USB_ROLE_NONE;
-	u16 inferred_changes;
-	u16 changed_flags;
 	u64 command;
 	int ret;
 
 	mutex_lock(&con->lock);
 
-	/*
-	 * Some/many PPMs have an issue where all fields in the change bitfield
-	 * are cleared when an ACK is send. This will causes any change
-	 * between GET_CONNECTOR_STATUS and ACK to be lost.
-	 *
-	 * We work around this by re-fetching the connector status afterwards.
-	 * We then infer any changes that we see have happened but that may not
-	 * be represented in the change bitfield.
-	 *
-	 * Also, even though we don't need to know the currently supported alt
-	 * modes, we run the GET_CAM_SUPPORTED command to ensure the PPM does
-	 * not get stuck in case it assumes we do.
-	 * Always do this, rather than relying on UCSI_CONSTAT_CAM_CHANGE to be
-	 * set in the change bitfield.
-	 *
-	 * We end up with the following actions:
-	 *  1. UCSI_GET_CONNECTOR_STATUS, store result, update unprocessed_changes
-	 *  2. UCSI_GET_CAM_SUPPORTED, discard result
-	 *  3. ACK connector change
-	 *  4. UCSI_GET_CONNECTOR_STATUS, store result
-	 *  5. Infere lost changes by comparing UCSI_GET_CONNECTOR_STATUS results
-	 *  6. If PPM reported a new change, then restart in order to ACK
-	 *  7. Process everything as usual.
-	 *
-	 * We may end up seeing a change twice, but we can only miss extremely
-	 * short transitional changes.
-	 */
-
-	/* 1. First UCSI_GET_CONNECTOR_STATUS */
-	command = UCSI_GET_CONNECTOR_STATUS | UCSI_CONNECTOR_NUMBER(con->num);
-	ret = ucsi_send_command(ucsi, command, &pre_ack_status,
-				sizeof(pre_ack_status));
-	if (ret < 0) {
-		dev_err(ucsi->dev, "%s: GET_CONNECTOR_STATUS failed (%d)\n",
-			__func__, ret);
-		goto out_unlock;
-	}
-	con->unprocessed_changes |= pre_ack_status.change;
-
-	/* 2. Run UCSI_GET_CAM_SUPPORTED and discard the result. */
-	command = UCSI_GET_CAM_SUPPORTED;
-	command |= UCSI_CONNECTOR_NUMBER(con->num);
-	ucsi_send_command(con->ucsi, command, NULL, 0);
-
-	/* 3. ACK connector change */
-	ret = ucsi_acknowledge_connector_change(ucsi);
-	clear_bit(EVENT_PENDING, &ucsi->flags);
-	if (ret) {
-		dev_err(ucsi->dev, "%s: ACK failed (%d)", __func__, ret);
-		goto out_unlock;
-	}
-
-	/* 4. Second UCSI_GET_CONNECTOR_STATUS */
 	command = UCSI_GET_CONNECTOR_STATUS | UCSI_CONNECTOR_NUMBER(con->num);
-	ret = ucsi_send_command(ucsi, command, &post_ack_status,
-				sizeof(post_ack_status));
+	ret = ucsi_send_command(ucsi, command, &con->status, sizeof(con->status));
 	if (ret < 0) {
 		dev_err(ucsi->dev, "%s: GET_CONNECTOR_STATUS failed (%d)\n",
 			__func__, ret);
 		goto out_unlock;
 	}
 
-	/* 5. Inferre any missing changes */
-	changed_flags = pre_ack_status.flags ^ post_ack_status.flags;
-	inferred_changes = 0;
-	if (UCSI_CONSTAT_PWR_OPMODE(changed_flags) != 0)
-		inferred_changes |= UCSI_CONSTAT_POWER_OPMODE_CHANGE;
-
-	if (changed_flags & UCSI_CONSTAT_CONNECTED)
-		inferred_changes |= UCSI_CONSTAT_CONNECT_CHANGE;
-
-	if (changed_flags & UCSI_CONSTAT_PWR_DIR)
-		inferred_changes |= UCSI_CONSTAT_POWER_DIR_CHANGE;
-
-	if (UCSI_CONSTAT_PARTNER_FLAGS(changed_flags) != 0)
-		inferred_changes |= UCSI_CONSTAT_PARTNER_CHANGE;
-
-	if (UCSI_CONSTAT_PARTNER_TYPE(changed_flags) != 0)
-		inferred_changes |= UCSI_CONSTAT_PARTNER_CHANGE;
-
-	/* Mask out anything that was correctly notified in the later call. */
-	inferred_changes &= ~post_ack_status.change;
-	if (inferred_changes)
-		dev_dbg(ucsi->dev, "%s: Inferred changes that would have been lost: 0x%04x\n",
-			__func__, inferred_changes);
-
-	con->unprocessed_changes |= inferred_changes;
-
-	/* 6. If PPM reported a new change, then restart in order to ACK */
-	if (post_ack_status.change)
-		goto out_unlock;
-
-	/* 7. Continue as if nothing happened */
-	con->status = post_ack_status;
-	con->status.change = con->unprocessed_changes;
-	con->unprocessed_changes = 0;
+	trace_ucsi_connector_change(con->num, &con->status);
 
 	role = !!(con->status.flags & UCSI_CONSTAT_PWR_DIR);
 
@@ -858,63 +784,39 @@ static void ucsi_handle_connector_change(struct work_struct *work)
 
 	if (con->status.change & UCSI_CONSTAT_CONNECT_CHANGE) {
 		typec_set_pwr_role(con->port, role);
-
-		switch (UCSI_CONSTAT_PARTNER_TYPE(con->status.flags)) {
-		case UCSI_CONSTAT_PARTNER_TYPE_UFP:
-		case UCSI_CONSTAT_PARTNER_TYPE_CABLE_AND_UFP:
-			u_role = USB_ROLE_HOST;
-			fallthrough;
-		case UCSI_CONSTAT_PARTNER_TYPE_CABLE:
-			typec_set_data_role(con->port, TYPEC_HOST);
-			break;
-		case UCSI_CONSTAT_PARTNER_TYPE_DFP:
-			u_role = USB_ROLE_DEVICE;
-			typec_set_data_role(con->port, TYPEC_DEVICE);
-			break;
-		default:
-			break;
-		}
+		ucsi_port_psy_changed(con);
+		ucsi_partner_change(con);
 
 		if (con->status.flags & UCSI_CONSTAT_CONNECTED) {
 			ucsi_register_partner(con);
-
-			if (UCSI_CONSTAT_PWR_OPMODE(con->status.flags) ==
-			    UCSI_CONSTAT_PWR_OPMODE_PD)
-				ucsi_partner_task(con, ucsi_check_altmodes, 30, 0);
+			ucsi_partner_task(con, ucsi_check_connection, 1, HZ);
 		} else {
 			ucsi_unregister_partner(con);
 		}
-
-		ucsi_port_psy_changed(con);
-
-		/* Only notify USB controller if partner supports USB data */
-		if (!(UCSI_CONSTAT_PARTNER_FLAGS(con->status.flags) &
-				UCSI_CONSTAT_PARTNER_FLAG_USB))
-			u_role = USB_ROLE_NONE;
-
-		ret = usb_role_switch_set_role(con->usb_role_sw, u_role);
-		if (ret)
-			dev_err(ucsi->dev, "con:%d: failed to set usb role:%d\n",
-				con->num, u_role);
 	}
 
 	if (con->status.change & UCSI_CONSTAT_POWER_OPMODE_CHANGE ||
 	    con->status.change & UCSI_CONSTAT_POWER_LEVEL_CHANGE)
 		ucsi_pwr_opmode_change(con);
 
-	if (con->status.change & UCSI_CONSTAT_PARTNER_CHANGE)
+	if (con->partner && con->status.change & UCSI_CONSTAT_PARTNER_CHANGE) {
 		ucsi_partner_change(con);
 
-	trace_ucsi_connector_change(con->num, &con->status);
-
-out_unlock:
-	if (test_and_clear_bit(EVENT_PENDING, &ucsi->flags)) {
-		schedule_work(&con->work);
-		mutex_unlock(&con->lock);
-		return;
+		/* Complete pending data role swap */
+		if (!completion_done(&con->complete))
+			complete(&con->complete);
 	}
 
-	clear_bit(EVENT_PROCESSING, &ucsi->flags);
+	if (con->status.change & UCSI_CONSTAT_CAM_CHANGE)
+		ucsi_partner_task(con, ucsi_check_altmodes, 1, 0);
+
+	clear_bit(EVENT_PENDING, &con->ucsi->flags);
+
+	ret = ucsi_acknowledge_connector_change(ucsi);
+	if (ret)
+		dev_err(ucsi->dev, "%s: ACK failed (%d)", __func__, ret);
+
+out_unlock:
 	mutex_unlock(&con->lock);
 }
 
@@ -932,9 +834,7 @@ void ucsi_connector_change(struct ucsi *ucsi, u8 num)
 		return;
 	}
 
-	set_bit(EVENT_PENDING, &ucsi->flags);
-
-	if (!test_and_set_bit(EVENT_PROCESSING, &ucsi->flags))
+	if (!test_and_set_bit(EVENT_PENDING, &ucsi->flags))
 		schedule_work(&con->work);
 }
 EXPORT_SYMBOL_GPL(ucsi_connector_change);
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index d10b8c24435af..280f1e1bda2c9 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -300,7 +300,6 @@ struct ucsi {
 #define EVENT_PENDING	0
 #define COMMAND_PENDING	1
 #define ACK_PENDING	2
-#define EVENT_PROCESSING	3
 };
 
 #define UCSI_MAX_SVID		5
@@ -327,7 +326,6 @@ struct ucsi_connector {
 
 	struct typec_capability typec_cap;
 
-	u16 unprocessed_changes;
 	struct ucsi_connector_status status;
 	struct ucsi_connector_capability cap;
 	struct power_supply *psy;
-- 
2.33.0

Re: [PATCH 0/7] usb: typec: ucsi: Driver improvements

[Benjamin Berg]

[-- Attachment #1: Type: text/plain, Size: 2279 bytes --]

Hi,

On Mon, 2021-09-20 at 17:24 +0300, Heikki Krogerus wrote:
> The goal of this series was to improve the alt mode handling in the
> driver, but now it seems that we can use the "poll worker" that was
> introduced for that to handle other tasks better as well.
> 
> Ulrich reported some problems that are caused by the second
> GET_CONNECTOR_STATUS right after the first one that was introduced in
> 217504a05532 ("usb: typec: ucsi: Work around PPM losing change
> information"). In the last patch I try to improve that workaround by
> extracting it out of the generic event handler into its own task and
> executing it only when it's really needed. That seems to improve the
> situation.
> 
> These patches definitely improve the quality of the driver by making
> it a bit more readable, but they also appear to make the behaviour a
> bit more predictably and uniform on different platforms.
> 
> Benjamin, can you test these?

I just gave this a spin on a X1 Carbon Gen 8 with a Lenovo TB 3 Dock.
Unfortunately, I can still reproduce the issue occasionally. My take is
that the rate is much lower than it was before my patch was introduced.
However, unfortunately the patchset does appear to cause a regression
on the machine I tested.

As before. The "online" status of the UCSI power supply is reported as
"1" occasionally even after the cable was unplugged. And the issue
seems to only happens with a dock, not if I use a USB-C charger.

Benjamin

> Heikki Krogerus (7):
>   usb: typec: ucsi: Always cancel the command if PPM reports BUSY
>     condition
>   usb: typec: ucsi: Don't stop alt mode registration on busy condition
>   usb: typec: ucsi: Add polling mechanism for partner tasks like alt
>     mode checking
>   usb: typec: ucsi: acpi: Reduce the command completion timeout
>   usb: typec: ucsi: Check the partner alt modes always if there is PD
>     contract
>   usb: typec: ucsi: Read the PDOs in separate work
>   usb: typec: ucsi: Better fix for missing unplug events issue
> 
>  drivers/usb/typec/ucsi/ucsi.c      | 337 ++++++++++++++---------------
>  drivers/usb/typec/ucsi/ucsi.h      |   3 +-
>  drivers/usb/typec/ucsi/ucsi_acpi.c |   2 +-
>  3 files changed, 167 insertions(+), 175 deletions(-)
> 


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/7] usb: typec: ucsi: Driver improvements

[Ulrich Huber]

Hi,

Am 23.09.21 um 16:38 schrieb Benjamin Berg:
> Hi,
>
> On Mon, 2021-09-20 at 17:24 +0300, Heikki Krogerus wrote:
>> The goal of this series was to improve the alt mode handling in the
>> driver, but now it seems that we can use the "poll worker" that was
>> introduced for that to handle other tasks better as well.
>>
>> Ulrich reported some problems that are caused by the second
>> GET_CONNECTOR_STATUS right after the first one that was introduced in
>> 217504a05532 ("usb: typec: ucsi: Work around PPM losing change
>> information"). In the last patch I try to improve that workaround by
>> extracting it out of the generic event handler into its own task and
>> executing it only when it's really needed. That seems to improve the
>> situation.
>>
>> These patches definitely improve the quality of the driver by making
>> it a bit more readable, but they also appear to make the behaviour a
>> bit more predictably and uniform on different platforms.
>>
>> Benjamin, can you test these?
> I just gave this a spin on a X1 Carbon Gen 8 with a Lenovo TB 3 Dock.
> Unfortunately, I can still reproduce the issue occasionally. My take is
> that the rate is much lower than it was before my patch was introduced.
> However, unfortunately the patchset does appear to cause a regression
> on the machine I tested.
>
> As before. The "online" status of the UCSI power supply is reported as
> "1" occasionally even after the cable was unplugged. And the issue
> seems to only happens with a dock, not if I use a USB-C charger.
>
> Benjamin

 From my point of view the patch set is still a huge improvement to the 
current state of the driver. Before it, the status of the UCSI power 
supply was unpredictable when using an USB-C charger with my Lenovo Yoga 
9i.

I do still get error messages in the kernel log right after waking from 
suspend occasionally, but I have not yet found reproducible steps. Most 
likely it has something to do with the controller being in an invalid 
state after waking from suspension. Though even then the status of the 
UCSI power supply is correct when this happens.


Ulrich


>
>> Heikki Krogerus (7):
>>    usb: typec: ucsi: Always cancel the command if PPM reports BUSY
>>      condition
>>    usb: typec: ucsi: Don't stop alt mode registration on busy condition
>>    usb: typec: ucsi: Add polling mechanism for partner tasks like alt
>>      mode checking
>>    usb: typec: ucsi: acpi: Reduce the command completion timeout
>>    usb: typec: ucsi: Check the partner alt modes always if there is PD
>>      contract
>>    usb: typec: ucsi: Read the PDOs in separate work
>>    usb: typec: ucsi: Better fix for missing unplug events issue
>>
>>   drivers/usb/typec/ucsi/ucsi.c      | 337 ++++++++++++++---------------
>>   drivers/usb/typec/ucsi/ucsi.h      |   3 +-
>>   drivers/usb/typec/ucsi/ucsi_acpi.c |   2 +-
>>   3 files changed, 167 insertions(+), 175 deletions(-)
>>

Re: [PATCH 0/7] usb: typec: ucsi: Driver improvements

[Heikki Krogerus]

On Thu, Sep 23, 2021 at 06:06:21PM +0200, Ulrich Huber wrote:
> Hi,
> 
> Am 23.09.21 um 16:38 schrieb Benjamin Berg:
> > Hi,
> > 
> > On Mon, 2021-09-20 at 17:24 +0300, Heikki Krogerus wrote:
> > > The goal of this series was to improve the alt mode handling in the
> > > driver, but now it seems that we can use the "poll worker" that was
> > > introduced for that to handle other tasks better as well.
> > > 
> > > Ulrich reported some problems that are caused by the second
> > > GET_CONNECTOR_STATUS right after the first one that was introduced in
> > > 217504a05532 ("usb: typec: ucsi: Work around PPM losing change
> > > information"). In the last patch I try to improve that workaround by
> > > extracting it out of the generic event handler into its own task and
> > > executing it only when it's really needed. That seems to improve the
> > > situation.
> > > 
> > > These patches definitely improve the quality of the driver by making
> > > it a bit more readable, but they also appear to make the behaviour a
> > > bit more predictably and uniform on different platforms.
> > > 
> > > Benjamin, can you test these?
> > I just gave this a spin on a X1 Carbon Gen 8 with a Lenovo TB 3 Dock.
> > Unfortunately, I can still reproduce the issue occasionally. My take is
> > that the rate is much lower than it was before my patch was introduced.
> > However, unfortunately the patchset does appear to cause a regression
> > on the machine I tested.
> > 
> > As before. The "online" status of the UCSI power supply is reported as
> > "1" occasionally even after the cable was unplugged. And the issue
> > seems to only happens with a dock, not if I use a USB-C charger.
> > 
> > Benjamin
> 
> From my point of view the patch set is still a huge improvement to the
> current state of the driver. Before it, the status of the UCSI power supply
> was unpredictable when using an USB-C charger with my Lenovo Yoga 9i.

This is the problem with these workarounds that attempt to fix
firmware issues. It's difficult to find a solution that works on every
board. That's why it's important to attempt to isolate them, and use
them only when needed.

Right now the driver does behave quite unpredictable on several boards
because of commit 217504a05532. The way that it solves a single issue
is not isolated enough like it should be, which means every single
connector change event is affected by it even when there is no need
for that, but the solution itself - duplicated command execution - is
also simply too heavy for many EC firmwares.

I do admit that my series still leaves problems, it does not solve
everything, and I'm not claiming that it's actually fixing anything
(it's not tagged as a fix), but it does improve the behaviour of the
driver so much that I still think that we should use it as the new
"baseline" for future improvements.

> I do still get error messages in the kernel log right after waking from
> suspend occasionally, but I have not yet found reproducible steps. Most
> likely it has something to do with the controller being in an invalid state
> after waking from suspension. Though even then the status of the UCSI power
> supply is correct when this happens.

This is most likely separate issue that needs its own fix.


thanks,

-- 
heikki