[PATCH 0/5] Fix various races in UCSI

[PATCH 0/5] Fix various races in UCSI

[Christian A. Ehrhardt]

Fix various races in UCSI code:
- The EVENT_PENDING bit should be cleared under the PPM lock to
  avoid spurious re-checking of the connector status.
- The initial connector change notification during init may be
  lost which can cause a stuck UCSI controller. Observed by me
  and others during resume or after module reload.
- Unsupported commands must be ACKed. This was uncovered by the
  recent change from Jameson Thies that did sent unsupported commands.
- The DELL quirk still isn't quite complete and I've found a more
  elegant way to handle this. A connector change ack _is_ accepted
  on affected systems if it is bundled with a command ack.
- If we do two consecutive resets or the controller is already
  reset at boog the second reset might complete early because the
  reset complete bit is already set. ucsi_ccg.c has a work around
  for this but it looks like an more general issue to me.

NOTE:
As a result of these individual fixes we could think about the
question if there are additional cases where we send some type
of command to the PPM while the bit that indicates its completion
is already set in CCI. And in fact there is one more case where
this can happen: The ack command that clears the connector change
is sent directly after the ack command for the previous command.
It might be possible to simply ack the connector change along with
the first command ucsi_handle_connector_change() and not at the
end. AFAICS the connector lock should protect us from races that
might arise out of this.

Christian A. Ehrhardt (5):
  usb: typec: ucsi: Clear EVENT_PENDING under PPM lock
  usb: typec: ucsi: Check for notifications after init
  usb: typec: ucsi: Ack unsupported commands
  usb: typec: ucsi_acpi: Refactor and fix DELL quirk
  usb: typec: ucsi: Clear UCSI_CCI_RESET_COMPLETE before reset

 drivers/usb/typec/ucsi/ucsi.c      | 56 ++++++++++++++++++++--
 drivers/usb/typec/ucsi/ucsi_acpi.c | 75 +++++++++++++-----------------
 2 files changed, 84 insertions(+), 47 deletions(-)

-- 
2.40.1

[PATCH 1/5] usb: typec: ucsi: Clear EVENT_PENDING under PPM lock

[Christian A. Ehrhardt]

Suppose we sleep on the PPM lock after clearing the EVENT_PENDING
bit because the thread for another connector is executing a command.
In this case the command completion of the other command will still
report the connector change for our connector.

Clear the EVENT_PENDING bit under the PPM lock to avoid another
useless call to ucsi_handle_connector_change() in this case.

Fixes: c9aed03a0a68 ("usb: ucsi: Add missing ppm_lock")
Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
---
 drivers/usb/typec/ucsi/ucsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index cf52cb34d285..8a6645ffd938 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -1215,11 +1215,11 @@ static void ucsi_handle_connector_change(struct work_struct *work)
 	if (con->status.change & UCSI_CONSTAT_CAM_CHANGE)
 		ucsi_partner_task(con, ucsi_check_altmodes, 1, 0);
 
-	clear_bit(EVENT_PENDING, &con->ucsi->flags);
-
 	mutex_lock(&ucsi->ppm_lock);
+	clear_bit(EVENT_PENDING, &con->ucsi->flags);
 	ret = ucsi_acknowledge_connector_change(ucsi);
 	mutex_unlock(&ucsi->ppm_lock);
+
 	if (ret)
 		dev_err(ucsi->dev, "%s: ACK failed (%d)", __func__, ret);
 
-- 
2.40.1

[PATCH 2/5] usb: typec: ucsi: Check for notifications after init

[Christian A. Ehrhardt]

The completion notification for the final SET_NOTIFICATION_ENABLE
command during initialization can include a connector change
notification.  However, at the time this completion notification is
processed, the ucsi struct is not ready to handle this notification.
As a result the notification is ignored and the controller
never sends an interrupt again.

Re-check CCI for a pending connector state change after
initialization is complete. Adjust the corresponding debug
message accordingly.

Fixes: 71a1fa0df2a3 ("usb: typec: ucsi: Store the notification mask")
Cc: stable@vger.kernel.org
Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
---
 drivers/usb/typec/ucsi/ucsi.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 8a6645ffd938..dceeed207569 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -1237,7 +1237,7 @@ void ucsi_connector_change(struct ucsi *ucsi, u8 num)
 	struct ucsi_connector *con = &ucsi->connector[num - 1];
 
 	if (!(ucsi->ntfy & UCSI_ENABLE_NTFY_CONNECTOR_CHANGE)) {
-		dev_dbg(ucsi->dev, "Bogus connector change event\n");
+		dev_dbg(ucsi->dev, "Early connector change event\n");
 		return;
 	}
 
@@ -1636,6 +1636,7 @@ static int ucsi_init(struct ucsi *ucsi)
 {
 	struct ucsi_connector *con, *connector;
 	u64 command, ntfy;
+	u32 cci;
 	int ret;
 	int i;
 
@@ -1688,6 +1689,13 @@ static int ucsi_init(struct ucsi *ucsi)
 
 	ucsi->connector = connector;
 	ucsi->ntfy = ntfy;
+
+	ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci));
+	if (ret)
+		return ret;
+	if (UCSI_CCI_CONNECTOR(READ_ONCE(cci)))
+		ucsi_connector_change(ucsi, cci);
+
 	return 0;
 
 err_unregister:
-- 
2.40.1

[PATCH 3/5] usb: typec: ucsi: Ack unsupported commands

[Christian A. Ehrhardt]

If a command completes the OPM must send an ack. This applies
to unsupported commands, too.

Send the required ACK for unsupported commands.

Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
---
 drivers/usb/typec/ucsi/ucsi.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index dceeed207569..63f340dbd867 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -151,8 +151,12 @@ static int ucsi_exec_command(struct ucsi *ucsi, u64 cmd)
 	if (!(cci & UCSI_CCI_COMMAND_COMPLETE))
 		return -EIO;
 
-	if (cci & UCSI_CCI_NOT_SUPPORTED)
+	if (cci & UCSI_CCI_NOT_SUPPORTED) {
+		if (ucsi_acknowledge_command(ucsi) < 0)
+			dev_err(ucsi->dev,
+				"ACK of unsupported command failed\n");
 		return -EOPNOTSUPP;
+	}
 
 	if (cci & UCSI_CCI_ERROR) {
 		if (cmd == UCSI_GET_ERROR_STATUS)
-- 
2.40.1

[PATCH 4/5] usb: typec: ucsi_acpi: Refactor and fix DELL quirk

[Christian A. Ehrhardt]

Some DELL systems don't like UCSI_ACK_CC_CI commands with the
UCSI_ACK_CONNECTOR_CHANGE but not the UCSI_ACK_COMMAND_COMPLETE
bit set. The current quirk still leaves room for races because
it requires two consecutive ACK commands to be sent.

Refactor and significantly simplify the quirk to fix this:
Send a dummy command and bundle the connector change ack with the
command completion ack in a single UCSI_ACK_CC_CI command.
This removes the need to probe for the quirk.

While there define flag bits for struct ucsi_acpi->flags in ucsi_acpi.c
and don't re-use definitions from ucsi.h for struct ucsi->flags.

Fixes: f3be347ea42d ("usb: ucsi_acpi: Quirk to ack a connector change ack cmd")
Cc: stable@vger.kernel.org
Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
---
 drivers/usb/typec/ucsi/ucsi_acpi.c | 75 +++++++++++++-----------------
 1 file changed, 33 insertions(+), 42 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
index 928eacbeb21a..7b3ac133ef86 100644
--- a/drivers/usb/typec/ucsi/ucsi_acpi.c
+++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
@@ -23,10 +23,11 @@ struct ucsi_acpi {
 	void *base;
 	struct completion complete;
 	unsigned long flags;
+#define UCSI_ACPI_SUPPRESS_EVENT	0
+#define UCSI_ACPI_COMMAND_PENDING	1
+#define UCSI_ACPI_ACK_PENDING		2
 	guid_t guid;
 	u64 cmd;
-	bool dell_quirk_probed;
-	bool dell_quirk_active;
 };
 
 static int ucsi_acpi_dsm(struct ucsi_acpi *ua, int func)
@@ -79,9 +80,9 @@ static int ucsi_acpi_sync_write(struct ucsi *ucsi, unsigned int offset,
 	int ret;
 
 	if (ack)
-		set_bit(ACK_PENDING, &ua->flags);
+		set_bit(UCSI_ACPI_ACK_PENDING, &ua->flags);
 	else
-		set_bit(COMMAND_PENDING, &ua->flags);
+		set_bit(UCSI_ACPI_COMMAND_PENDING, &ua->flags);
 
 	ret = ucsi_acpi_async_write(ucsi, offset, val, val_len);
 	if (ret)
@@ -92,9 +93,9 @@ static int ucsi_acpi_sync_write(struct ucsi *ucsi, unsigned int offset,
 
 out_clear_bit:
 	if (ack)
-		clear_bit(ACK_PENDING, &ua->flags);
+		clear_bit(UCSI_ACPI_ACK_PENDING, &ua->flags);
 	else
-		clear_bit(COMMAND_PENDING, &ua->flags);
+		clear_bit(UCSI_ACPI_COMMAND_PENDING, &ua->flags);
 
 	return ret;
 }
@@ -129,51 +130,40 @@ static const struct ucsi_operations ucsi_zenbook_ops = {
 };
 
 /*
- * Some Dell laptops expect that an ACK command with the
- * UCSI_ACK_CONNECTOR_CHANGE bit set is followed by a (separate)
- * ACK command that only has the UCSI_ACK_COMMAND_COMPLETE bit set.
- * If this is not done events are not delivered to OSPM and
- * subsequent commands will timeout.
+ * Some Dell laptops don't like ACK commands with the
+ * UCSI_ACK_CONNECTOR_CHANGE but not the UCSI_ACK_COMMAND_COMPLETE
+ * bit set. To work around this send a dummy command and bundle the
+ * UCSI_ACK_CONNECTOR_CHANGE with the UCSI_ACK_COMMAND_COMPLETE
+ * for the dummy command.
  */
 static int
 ucsi_dell_sync_write(struct ucsi *ucsi, unsigned int offset,
 		     const void *val, size_t val_len)
 {
 	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
-	u64 cmd = *(u64 *)val, ack = 0;
+	u64 cmd = *(u64 *)val;
+	u64 dummycmd = UCSI_GET_CAPABILITY;
 	int ret;
 
-	if (UCSI_COMMAND(cmd) == UCSI_ACK_CC_CI &&
-	    cmd & UCSI_ACK_CONNECTOR_CHANGE)
-		ack = UCSI_ACK_CC_CI | UCSI_ACK_COMMAND_COMPLETE;
-
-	ret = ucsi_acpi_sync_write(ucsi, offset, val, val_len);
-	if (ret != 0)
-		return ret;
-	if (ack == 0)
-		return ret;
-
-	if (!ua->dell_quirk_probed) {
-		ua->dell_quirk_probed = true;
-
-		cmd = UCSI_GET_CAPABILITY;
-		ret = ucsi_acpi_sync_write(ucsi, UCSI_CONTROL, &cmd,
-					   sizeof(cmd));
-		if (ret == 0)
-			return ucsi_acpi_sync_write(ucsi, UCSI_CONTROL,
-						    &ack, sizeof(ack));
-		if (ret != -ETIMEDOUT)
+	if (cmd == (UCSI_ACK_CC_CI | UCSI_ACK_CONNECTOR_CHANGE)) {
+		cmd |= UCSI_ACK_COMMAND_COMPLETE;
+
+		/*
+		 * The UCSI core thinks it is sending a connector change ack
+		 * and will accept new connector change events. We don't want
+		 * this to happen for the dummy command as its response will
+		 * still report the very event that the core is trying to clear.
+		 */
+		set_bit(UCSI_ACPI_SUPPRESS_EVENT, &ua->flags);
+		ret = ucsi_acpi_sync_write(ucsi, UCSI_CONTROL, &dummycmd,
+					   sizeof(dummycmd));
+		clear_bit(UCSI_ACPI_SUPPRESS_EVENT, &ua->flags);
+
+		if (ret < 0)
 			return ret;
-
-		ua->dell_quirk_active = true;
-		dev_err(ua->dev, "Firmware bug: Additional ACK required after ACKing a connector change.\n");
-		dev_err(ua->dev, "Firmware bug: Enabling workaround\n");
 	}
 
-	if (!ua->dell_quirk_active)
-		return ret;
-
-	return ucsi_acpi_sync_write(ucsi, UCSI_CONTROL, &ack, sizeof(ack));
+	return ucsi_acpi_sync_write(ucsi, UCSI_CONTROL, &cmd, sizeof(cmd));
 }
 
 static const struct ucsi_operations ucsi_dell_ops = {
@@ -209,13 +199,14 @@ static void ucsi_acpi_notify(acpi_handle handle, u32 event, void *data)
 	if (ret)
 		return;
 
-	if (UCSI_CCI_CONNECTOR(cci))
+	if (UCSI_CCI_CONNECTOR(cci) &&
+	    !test_bit(UCSI_ACPI_SUPPRESS_EVENT, &ua->flags))
 		ucsi_connector_change(ua->ucsi, UCSI_CCI_CONNECTOR(cci));
 
 	if (cci & UCSI_CCI_ACK_COMPLETE && test_bit(ACK_PENDING, &ua->flags))
 		complete(&ua->complete);
 	if (cci & UCSI_CCI_COMMAND_COMPLETE &&
-	    test_bit(COMMAND_PENDING, &ua->flags))
+	    test_bit(UCSI_ACPI_COMMAND_PENDING, &ua->flags))
 		complete(&ua->complete);
 }
 
-- 
2.40.1

[PATCH 5/5] usb: typec: ucsi: Clear UCSI_CCI_RESET_COMPLETE before reset

[Christian A. Ehrhardt]

Check the UCSI_CCI_RESET_COMPLETE complete flag before starting
another reset. Use a UCSI_SET_NOTIFICATION_ENABLE command to clear
the flag if it is set.

Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
---
 drivers/usb/typec/ucsi/ucsi.c | 36 ++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 63f340dbd867..85e507df7fa8 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -1264,13 +1264,47 @@ static int ucsi_reset_connector(struct ucsi_connector *con, bool hard)
 
 static int ucsi_reset_ppm(struct ucsi *ucsi)
 {
-	u64 command = UCSI_PPM_RESET;
+	u64 command;
 	unsigned long tmo;
 	u32 cci;
 	int ret;
 
 	mutex_lock(&ucsi->ppm_lock);
 
+	ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci));
+	if (ret < 0)
+		goto out;
+
+	/*
+	 * If UCSI_CCI_RESET_COMPLETE is already set we must clear
+	 * the flag before we start another reset. Send a
+	 * UCSI_SET_NOTIFICATION_ENABLE command to achieve this.
+	 * Ignore a timeout and try the reset anyway if this fails.
+	 */
+	if (cci & UCSI_CCI_RESET_COMPLETE) {
+		command = UCSI_SET_NOTIFICATION_ENABLE;
+		ret = ucsi->ops->async_write(ucsi, UCSI_CONTROL, &command,
+					     sizeof(command));
+		if (ret < 0)
+			goto out;
+
+		tmo = jiffies + msecs_to_jiffies(UCSI_TIMEOUT_MS);
+		do {
+			ret = ucsi->ops->read(ucsi, UCSI_CCI,
+					      &cci, sizeof(cci));
+			if (ret < 0)
+				goto out;
+			if (cci & UCSI_CCI_COMMAND_COMPLETE)
+				break;
+			if (time_is_before_jiffies(tmo))
+				break;
+			msleep(20);
+		} while (1);
+
+		WARN_ON(cci & UCSI_CCI_RESET_COMPLETE);
+	}
+
+	command = UCSI_PPM_RESET;
 	ret = ucsi->ops->async_write(ucsi, UCSI_CONTROL, &command,
 				     sizeof(command));
 	if (ret < 0)
-- 
2.40.1

Re: [PATCH 0/5] Fix various races in UCSI

[Kenneth Crudup]

Applied (cleanly) onto 6.8.1; I'll be testing over the next few days, 
but a few connects/disconnects mixed in with suspend/resume cycles shows 
no obvious issues (and everything seems to work).

Dell XPS 9320, BIOS 2.10.0

-K

On 3/20/24 00:39, Christian A. Ehrhardt wrote:
> Fix various races in UCSI code:
> - The EVENT_PENDING bit should be cleared under the PPM lock to
>    avoid spurious re-checking of the connector status.
> - The initial connector change notification during init may be
>    lost which can cause a stuck UCSI controller. Observed by me
>    and others during resume or after module reload.
> - Unsupported commands must be ACKed. This was uncovered by the
>    recent change from Jameson Thies that did sent unsupported commands.
> - The DELL quirk still isn't quite complete and I've found a more
>    elegant way to handle this. A connector change ack _is_ accepted
>    on affected systems if it is bundled with a command ack.
> - If we do two consecutive resets or the controller is already
>    reset at boog the second reset might complete early because the
>    reset complete bit is already set. ucsi_ccg.c has a work around
>    for this but it looks like an more general issue to me.
> 
> NOTE:
> As a result of these individual fixes we could think about the
> question if there are additional cases where we send some type
> of command to the PPM while the bit that indicates its completion
> is already set in CCI. And in fact there is one more case where
> this can happen: The ack command that clears the connector change
> is sent directly after the ack command for the previous command.
> It might be possible to simply ack the connector change along with
> the first command ucsi_handle_connector_change() and not at the
> end. AFAICS the connector lock should protect us from races that
> might arise out of this.
> 
> Christian A. Ehrhardt (5):
>    usb: typec: ucsi: Clear EVENT_PENDING under PPM lock
>    usb: typec: ucsi: Check for notifications after init
>    usb: typec: ucsi: Ack unsupported commands
>    usb: typec: ucsi_acpi: Refactor and fix DELL quirk
>    usb: typec: ucsi: Clear UCSI_CCI_RESET_COMPLETE before reset
> 
>   drivers/usb/typec/ucsi/ucsi.c      | 56 ++++++++++++++++++++--
>   drivers/usb/typec/ucsi/ucsi_acpi.c | 75 +++++++++++++-----------------
>   2 files changed, 84 insertions(+), 47 deletions(-)
> 

-- 
Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange 
County CA

Re: [PATCH 1/5] usb: typec: ucsi: Clear EVENT_PENDING under PPM lock

[Heikki Krogerus]

On Wed, Mar 20, 2024 at 08:39:22AM +0100, Christian A. Ehrhardt wrote:
> Suppose we sleep on the PPM lock after clearing the EVENT_PENDING
> bit because the thread for another connector is executing a command.
> In this case the command completion of the other command will still
> report the connector change for our connector.
> 
> Clear the EVENT_PENDING bit under the PPM lock to avoid another
> useless call to ucsi_handle_connector_change() in this case.
> 
> Fixes: c9aed03a0a68 ("usb: ucsi: Add missing ppm_lock")
> Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/ucsi/ucsi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index cf52cb34d285..8a6645ffd938 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -1215,11 +1215,11 @@ static void ucsi_handle_connector_change(struct work_struct *work)
>  	if (con->status.change & UCSI_CONSTAT_CAM_CHANGE)
>  		ucsi_partner_task(con, ucsi_check_altmodes, 1, 0);
>  
> -	clear_bit(EVENT_PENDING, &con->ucsi->flags);
> -
>  	mutex_lock(&ucsi->ppm_lock);
> +	clear_bit(EVENT_PENDING, &con->ucsi->flags);
>  	ret = ucsi_acknowledge_connector_change(ucsi);
>  	mutex_unlock(&ucsi->ppm_lock);
> +
>  	if (ret)
>  		dev_err(ucsi->dev, "%s: ACK failed (%d)", __func__, ret);
>  
> -- 
> 2.40.1

-- 
heikki

Re: [PATCH 2/5] usb: typec: ucsi: Check for notifications after init

[Heikki Krogerus]

On Wed, Mar 20, 2024 at 08:39:23AM +0100, Christian A. Ehrhardt wrote:
> The completion notification for the final SET_NOTIFICATION_ENABLE
> command during initialization can include a connector change
> notification.  However, at the time this completion notification is
> processed, the ucsi struct is not ready to handle this notification.
> As a result the notification is ignored and the controller
> never sends an interrupt again.
> 
> Re-check CCI for a pending connector state change after
> initialization is complete. Adjust the corresponding debug
> message accordingly.
> 
> Fixes: 71a1fa0df2a3 ("usb: typec: ucsi: Store the notification mask")
> Cc: stable@vger.kernel.org
> Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/ucsi/ucsi.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 8a6645ffd938..dceeed207569 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -1237,7 +1237,7 @@ void ucsi_connector_change(struct ucsi *ucsi, u8 num)
>  	struct ucsi_connector *con = &ucsi->connector[num - 1];
>  
>  	if (!(ucsi->ntfy & UCSI_ENABLE_NTFY_CONNECTOR_CHANGE)) {
> -		dev_dbg(ucsi->dev, "Bogus connector change event\n");
> +		dev_dbg(ucsi->dev, "Early connector change event\n");
>  		return;
>  	}
>  
> @@ -1636,6 +1636,7 @@ static int ucsi_init(struct ucsi *ucsi)
>  {
>  	struct ucsi_connector *con, *connector;
>  	u64 command, ntfy;
> +	u32 cci;
>  	int ret;
>  	int i;
>  
> @@ -1688,6 +1689,13 @@ static int ucsi_init(struct ucsi *ucsi)
>  
>  	ucsi->connector = connector;
>  	ucsi->ntfy = ntfy;
> +
> +	ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci));
> +	if (ret)
> +		return ret;
> +	if (UCSI_CCI_CONNECTOR(READ_ONCE(cci)))
> +		ucsi_connector_change(ucsi, cci);
> +
>  	return 0;
>  
>  err_unregister:
> -- 
> 2.40.1

-- 
heikki

Re: [PATCH 4/5] usb: typec: ucsi_acpi: Refactor and fix DELL quirk

[Heikki Krogerus]

On Wed, Mar 20, 2024 at 08:39:25AM +0100, Christian A. Ehrhardt wrote:
> Some DELL systems don't like UCSI_ACK_CC_CI commands with the
> UCSI_ACK_CONNECTOR_CHANGE but not the UCSI_ACK_COMMAND_COMPLETE
> bit set. The current quirk still leaves room for races because
> it requires two consecutive ACK commands to be sent.
> 
> Refactor and significantly simplify the quirk to fix this:
> Send a dummy command and bundle the connector change ack with the
> command completion ack in a single UCSI_ACK_CC_CI command.
> This removes the need to probe for the quirk.
> 
> While there define flag bits for struct ucsi_acpi->flags in ucsi_acpi.c
> and don't re-use definitions from ucsi.h for struct ucsi->flags.
> 
> Fixes: f3be347ea42d ("usb: ucsi_acpi: Quirk to ack a connector change ack cmd")
> Cc: stable@vger.kernel.org
> Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/ucsi/ucsi_acpi.c | 75 +++++++++++++-----------------
>  1 file changed, 33 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
> index 928eacbeb21a..7b3ac133ef86 100644
> --- a/drivers/usb/typec/ucsi/ucsi_acpi.c
> +++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
> @@ -23,10 +23,11 @@ struct ucsi_acpi {
>  	void *base;
>  	struct completion complete;
>  	unsigned long flags;
> +#define UCSI_ACPI_SUPPRESS_EVENT	0
> +#define UCSI_ACPI_COMMAND_PENDING	1
> +#define UCSI_ACPI_ACK_PENDING		2
>  	guid_t guid;
>  	u64 cmd;
> -	bool dell_quirk_probed;
> -	bool dell_quirk_active;
>  };
>  
>  static int ucsi_acpi_dsm(struct ucsi_acpi *ua, int func)
> @@ -79,9 +80,9 @@ static int ucsi_acpi_sync_write(struct ucsi *ucsi, unsigned int offset,
>  	int ret;
>  
>  	if (ack)
> -		set_bit(ACK_PENDING, &ua->flags);
> +		set_bit(UCSI_ACPI_ACK_PENDING, &ua->flags);
>  	else
> -		set_bit(COMMAND_PENDING, &ua->flags);
> +		set_bit(UCSI_ACPI_COMMAND_PENDING, &ua->flags);
>  
>  	ret = ucsi_acpi_async_write(ucsi, offset, val, val_len);
>  	if (ret)
> @@ -92,9 +93,9 @@ static int ucsi_acpi_sync_write(struct ucsi *ucsi, unsigned int offset,
>  
>  out_clear_bit:
>  	if (ack)
> -		clear_bit(ACK_PENDING, &ua->flags);
> +		clear_bit(UCSI_ACPI_ACK_PENDING, &ua->flags);
>  	else
> -		clear_bit(COMMAND_PENDING, &ua->flags);
> +		clear_bit(UCSI_ACPI_COMMAND_PENDING, &ua->flags);
>  
>  	return ret;
>  }
> @@ -129,51 +130,40 @@ static const struct ucsi_operations ucsi_zenbook_ops = {
>  };
>  
>  /*
> - * Some Dell laptops expect that an ACK command with the
> - * UCSI_ACK_CONNECTOR_CHANGE bit set is followed by a (separate)
> - * ACK command that only has the UCSI_ACK_COMMAND_COMPLETE bit set.
> - * If this is not done events are not delivered to OSPM and
> - * subsequent commands will timeout.
> + * Some Dell laptops don't like ACK commands with the
> + * UCSI_ACK_CONNECTOR_CHANGE but not the UCSI_ACK_COMMAND_COMPLETE
> + * bit set. To work around this send a dummy command and bundle the
> + * UCSI_ACK_CONNECTOR_CHANGE with the UCSI_ACK_COMMAND_COMPLETE
> + * for the dummy command.
>   */
>  static int
>  ucsi_dell_sync_write(struct ucsi *ucsi, unsigned int offset,
>  		     const void *val, size_t val_len)
>  {
>  	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
> -	u64 cmd = *(u64 *)val, ack = 0;
> +	u64 cmd = *(u64 *)val;
> +	u64 dummycmd = UCSI_GET_CAPABILITY;
>  	int ret;
>  
> -	if (UCSI_COMMAND(cmd) == UCSI_ACK_CC_CI &&
> -	    cmd & UCSI_ACK_CONNECTOR_CHANGE)
> -		ack = UCSI_ACK_CC_CI | UCSI_ACK_COMMAND_COMPLETE;
> -
> -	ret = ucsi_acpi_sync_write(ucsi, offset, val, val_len);
> -	if (ret != 0)
> -		return ret;
> -	if (ack == 0)
> -		return ret;
> -
> -	if (!ua->dell_quirk_probed) {
> -		ua->dell_quirk_probed = true;
> -
> -		cmd = UCSI_GET_CAPABILITY;
> -		ret = ucsi_acpi_sync_write(ucsi, UCSI_CONTROL, &cmd,
> -					   sizeof(cmd));
> -		if (ret == 0)
> -			return ucsi_acpi_sync_write(ucsi, UCSI_CONTROL,
> -						    &ack, sizeof(ack));
> -		if (ret != -ETIMEDOUT)
> +	if (cmd == (UCSI_ACK_CC_CI | UCSI_ACK_CONNECTOR_CHANGE)) {
> +		cmd |= UCSI_ACK_COMMAND_COMPLETE;
> +
> +		/*
> +		 * The UCSI core thinks it is sending a connector change ack
> +		 * and will accept new connector change events. We don't want
> +		 * this to happen for the dummy command as its response will
> +		 * still report the very event that the core is trying to clear.
> +		 */
> +		set_bit(UCSI_ACPI_SUPPRESS_EVENT, &ua->flags);
> +		ret = ucsi_acpi_sync_write(ucsi, UCSI_CONTROL, &dummycmd,
> +					   sizeof(dummycmd));
> +		clear_bit(UCSI_ACPI_SUPPRESS_EVENT, &ua->flags);
> +
> +		if (ret < 0)
>  			return ret;
> -
> -		ua->dell_quirk_active = true;
> -		dev_err(ua->dev, "Firmware bug: Additional ACK required after ACKing a connector change.\n");
> -		dev_err(ua->dev, "Firmware bug: Enabling workaround\n");
>  	}
>  
> -	if (!ua->dell_quirk_active)
> -		return ret;
> -
> -	return ucsi_acpi_sync_write(ucsi, UCSI_CONTROL, &ack, sizeof(ack));
> +	return ucsi_acpi_sync_write(ucsi, UCSI_CONTROL, &cmd, sizeof(cmd));
>  }
>  
>  static const struct ucsi_operations ucsi_dell_ops = {
> @@ -209,13 +199,14 @@ static void ucsi_acpi_notify(acpi_handle handle, u32 event, void *data)
>  	if (ret)
>  		return;
>  
> -	if (UCSI_CCI_CONNECTOR(cci))
> +	if (UCSI_CCI_CONNECTOR(cci) &&
> +	    !test_bit(UCSI_ACPI_SUPPRESS_EVENT, &ua->flags))
>  		ucsi_connector_change(ua->ucsi, UCSI_CCI_CONNECTOR(cci));
>  
>  	if (cci & UCSI_CCI_ACK_COMPLETE && test_bit(ACK_PENDING, &ua->flags))
>  		complete(&ua->complete);
>  	if (cci & UCSI_CCI_COMMAND_COMPLETE &&
> -	    test_bit(COMMAND_PENDING, &ua->flags))
> +	    test_bit(UCSI_ACPI_COMMAND_PENDING, &ua->flags))
>  		complete(&ua->complete);
>  }
>  
> -- 
> 2.40.1

-- 
heikki

Re: [PATCH 0/5] Fix various races in UCSI

[Heikki Krogerus]

On Wed, Mar 20, 2024 at 08:39:21AM +0100, Christian A. Ehrhardt wrote:
> Fix various races in UCSI code:
> - The EVENT_PENDING bit should be cleared under the PPM lock to
>   avoid spurious re-checking of the connector status.
> - The initial connector change notification during init may be
>   lost which can cause a stuck UCSI controller. Observed by me
>   and others during resume or after module reload.
> - Unsupported commands must be ACKed. This was uncovered by the
>   recent change from Jameson Thies that did sent unsupported commands.
> - The DELL quirk still isn't quite complete and I've found a more
>   elegant way to handle this. A connector change ack _is_ accepted
>   on affected systems if it is bundled with a command ack.
> - If we do two consecutive resets or the controller is already
>   reset at boog the second reset might complete early because the
>   reset complete bit is already set. ucsi_ccg.c has a work around
>   for this but it looks like an more general issue to me.
> 
> NOTE:
> As a result of these individual fixes we could think about the
> question if there are additional cases where we send some type
> of command to the PPM while the bit that indicates its completion
> is already set in CCI. And in fact there is one more case where
> this can happen: The ack command that clears the connector change
> is sent directly after the ack command for the previous command.
> It might be possible to simply ack the connector change along with
> the first command ucsi_handle_connector_change() and not at the
> end. AFAICS the connector lock should protect us from races that
> might arise out of this.

That sounds good to me.

thanks,

-- 
heikki

Re: [PATCH 3/5] usb: typec: ucsi: Ack unsupported commands

[Heikki Krogerus]

On Wed, Mar 20, 2024 at 08:39:24AM +0100, Christian A. Ehrhardt wrote:
> If a command completes the OPM must send an ack. This applies
> to unsupported commands, too.
> 
> Send the required ACK for unsupported commands.
> 
> Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/ucsi/ucsi.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index dceeed207569..63f340dbd867 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -151,8 +151,12 @@ static int ucsi_exec_command(struct ucsi *ucsi, u64 cmd)
>  	if (!(cci & UCSI_CCI_COMMAND_COMPLETE))
>  		return -EIO;
>  
> -	if (cci & UCSI_CCI_NOT_SUPPORTED)
> +	if (cci & UCSI_CCI_NOT_SUPPORTED) {
> +		if (ucsi_acknowledge_command(ucsi) < 0)
> +			dev_err(ucsi->dev,
> +				"ACK of unsupported command failed\n");
>  		return -EOPNOTSUPP;
> +	}
>  
>  	if (cci & UCSI_CCI_ERROR) {
>  		if (cmd == UCSI_GET_ERROR_STATUS)
> -- 
> 2.40.1

-- 
heikki

Re: [PATCH 5/5] usb: typec: ucsi: Clear UCSI_CCI_RESET_COMPLETE before reset

[Heikki Krogerus]

On Wed, Mar 20, 2024 at 08:39:26AM +0100, Christian A. Ehrhardt wrote:
> Check the UCSI_CCI_RESET_COMPLETE complete flag before starting
> another reset. Use a UCSI_SET_NOTIFICATION_ENABLE command to clear
> the flag if it is set.
> 
> Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/ucsi/ucsi.c | 36 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 63f340dbd867..85e507df7fa8 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -1264,13 +1264,47 @@ static int ucsi_reset_connector(struct ucsi_connector *con, bool hard)
>  
>  static int ucsi_reset_ppm(struct ucsi *ucsi)
>  {
> -	u64 command = UCSI_PPM_RESET;
> +	u64 command;
>  	unsigned long tmo;
>  	u32 cci;
>  	int ret;
>  
>  	mutex_lock(&ucsi->ppm_lock);
>  
> +	ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci));
> +	if (ret < 0)
> +		goto out;
> +
> +	/*
> +	 * If UCSI_CCI_RESET_COMPLETE is already set we must clear
> +	 * the flag before we start another reset. Send a
> +	 * UCSI_SET_NOTIFICATION_ENABLE command to achieve this.
> +	 * Ignore a timeout and try the reset anyway if this fails.
> +	 */
> +	if (cci & UCSI_CCI_RESET_COMPLETE) {
> +		command = UCSI_SET_NOTIFICATION_ENABLE;
> +		ret = ucsi->ops->async_write(ucsi, UCSI_CONTROL, &command,
> +					     sizeof(command));
> +		if (ret < 0)
> +			goto out;
> +
> +		tmo = jiffies + msecs_to_jiffies(UCSI_TIMEOUT_MS);
> +		do {
> +			ret = ucsi->ops->read(ucsi, UCSI_CCI,
> +					      &cci, sizeof(cci));
> +			if (ret < 0)
> +				goto out;
> +			if (cci & UCSI_CCI_COMMAND_COMPLETE)
> +				break;
> +			if (time_is_before_jiffies(tmo))
> +				break;
> +			msleep(20);
> +		} while (1);
> +
> +		WARN_ON(cci & UCSI_CCI_RESET_COMPLETE);
> +	}
> +
> +	command = UCSI_PPM_RESET;
>  	ret = ucsi->ops->async_write(ucsi, UCSI_CONTROL, &command,
>  				     sizeof(command));
>  	if (ret < 0)
> -- 
> 2.40.1

-- 
heikki

Re: [PATCH 0/5] Fix various races in UCSI

[Neil Armstrong]

On 20/03/2024 11:53, Kenneth Crudup wrote:
> 
> Applied (cleanly) onto 6.8.1; I'll be testing over the next few days, but a few connects/disconnects mixed in with suspend/resume cycles shows no obvious issues (and everything seems to work).
> 
> Dell XPS 9320, BIOS 2.10.0
> 
> -K
> 
> On 3/20/24 00:39, Christian A. Ehrhardt wrote:
>> Fix various races in UCSI code:
>> - The EVENT_PENDING bit should be cleared under the PPM lock to
>>    avoid spurious re-checking of the connector status.
>> - The initial connector change notification during init may be
>>    lost which can cause a stuck UCSI controller. Observed by me
>>    and others during resume or after module reload.
>> - Unsupported commands must be ACKed. This was uncovered by the
>>    recent change from Jameson Thies that did sent unsupported commands.
>> - The DELL quirk still isn't quite complete and I've found a more
>>    elegant way to handle this. A connector change ack _is_ accepted
>>    on affected systems if it is bundled with a command ack.
>> - If we do two consecutive resets or the controller is already
>>    reset at boog the second reset might complete early because the
>>    reset complete bit is already set. ucsi_ccg.c has a work around
>>    for this but it looks like an more general issue to me.
>>
>> NOTE:
>> As a result of these individual fixes we could think about the
>> question if there are additional cases where we send some type
>> of command to the PPM while the bit that indicates its completion
>> is already set in CCI. And in fact there is one more case where
>> this can happen: The ack command that clears the connector change
>> is sent directly after the ack command for the previous command.
>> It might be possible to simply ack the connector change along with
>> the first command ucsi_handle_connector_change() and not at the
>> end. AFAICS the connector lock should protect us from races that
>> might arise out of this.
>>
>> Christian A. Ehrhardt (5):
>>    usb: typec: ucsi: Clear EVENT_PENDING under PPM lock
>>    usb: typec: ucsi: Check for notifications after init
>>    usb: typec: ucsi: Ack unsupported commands
>>    usb: typec: ucsi_acpi: Refactor and fix DELL quirk
>>    usb: typec: ucsi: Clear UCSI_CCI_RESET_COMPLETE before reset
>>
>>   drivers/usb/typec/ucsi/ucsi.c      | 56 ++++++++++++++++++++--
>>   drivers/usb/typec/ucsi/ucsi_acpi.c | 75 +++++++++++++-----------------
>>   2 files changed, 84 insertions(+), 47 deletions(-)
>>
> 

with [1] applied:

Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8550-QRD
Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-QRD
Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8550-HDK

[1] https://lore.kernel.org/all/20240315171836.343830-1-jthies@google.com/

Thanks,
Neil

Re: [PATCH 2/5] usb: typec: ucsi: Check for notifications after init

[Dmitry Baryshkov]

On Wed, Mar 20, 2024 at 08:39:23AM +0100, Christian A. Ehrhardt wrote:
> The completion notification for the final SET_NOTIFICATION_ENABLE
> command during initialization can include a connector change
> notification.  However, at the time this completion notification is
> processed, the ucsi struct is not ready to handle this notification.
> As a result the notification is ignored and the controller
> never sends an interrupt again.
> 
> Re-check CCI for a pending connector state change after
> initialization is complete. Adjust the corresponding debug
> message accordingly.
> 
> Fixes: 71a1fa0df2a3 ("usb: typec: ucsi: Store the notification mask")
> Cc: stable@vger.kernel.org
> Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
> ---
>  drivers/usb/typec/ucsi/ucsi.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 8a6645ffd938..dceeed207569 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -1237,7 +1237,7 @@ void ucsi_connector_change(struct ucsi *ucsi, u8 num)
>  	struct ucsi_connector *con = &ucsi->connector[num - 1];
>  
>  	if (!(ucsi->ntfy & UCSI_ENABLE_NTFY_CONNECTOR_CHANGE)) {
> -		dev_dbg(ucsi->dev, "Bogus connector change event\n");
> +		dev_dbg(ucsi->dev, "Early connector change event\n");
>  		return;
>  	}
>  
> @@ -1636,6 +1636,7 @@ static int ucsi_init(struct ucsi *ucsi)
>  {
>  	struct ucsi_connector *con, *connector;
>  	u64 command, ntfy;
> +	u32 cci;
>  	int ret;
>  	int i;
>  
> @@ -1688,6 +1689,13 @@ static int ucsi_init(struct ucsi *ucsi)
>  
>  	ucsi->connector = connector;
>  	ucsi->ntfy = ntfy;
> +
> +	ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci));
> +	if (ret)
> +		return ret;
> +	if (UCSI_CCI_CONNECTOR(READ_ONCE(cci)))
> +		ucsi_connector_change(ucsi, cci);
> +

I think this leaves place for the race. With this patchset + "Ack
connector change early" in place Neil triggered the following backtrace
on sm8550 HDK while testing my UCSI-qcom-fixes patchset:
What happens:

[   10.421640] write: 00000000: 05 00 e7 db 00 00 00 00

SET_NOTIFICATION_ENABLE

[   10.432359] read: 00000000: 10 01 00 00 00 00 00 80 00 00 00 00 00 00 00 00
[   10.469553] read: 00000010: 04 58 29 20 00 00 00 00 00 00 00 00 00 03 30 01
[   10.476783] read: 00000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[   10.489552] notify: 00000000: 00 00 00 80

COMMAND_COMPLETE

[   10.494194] read: 00000000: 10 01 00 00 00 00 00 80 00 00 00 00 00 00 00 00
[   10.501370] read: 00000010: 04 58 29 20 00 00 00 00 00 00 00 00 00 03 30 01
[   10.508578] read: 00000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[   10.515757] write: 00000000: 04 00 02 00 00 00 00 00

ACK_CC_CI(command completed)

[   10.521100] read: 00000000: 10 01 00 00 00 00 00 20 00 00 00 00 00 00 00 00
[   10.528363] read: 00000010: 04 58 29 20 00 00 00 00 00 00 00 00 00 03 30 01
[   10.535603] read: 00000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[   10.549549] notify: 00000000: 00 00 00 20

ACK_COMPLETE

[Here ucsi->connector and ucsi->ntfy are being set by ucsi_init()

[   10.566654] read: 00000010: 04 58 29 20 00 00 00 00 00 00 00 00 00 03 30 01
[   10.593553] notify: 00000000: 02 00 00 20

Event with CONNECTION_CHANGE. It also schedules connector_change work,
because ucsi->ntfy is already set

[   10.595796] read: 00000000: 10 01 00 00 02 00 00 20 00 00 00 00 00 00 00 00
[   10.595798] read: 00000010: 04 58 29 20 00 00 00 00 00 00 00 00 00 03 30 01
[   10.595799] read: 00000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

The CCI read coming from ucsi_init()

[   10.595807] ------------[ cut here ]------------
[   10.595808] WARNING: CPU: 6 PID: 101 at kernel/workqueue.c:2384 __queue_work+0x374/0x474

[skipped the register dump]

[   10.595953]  __queue_work+0x374/0x474
[   10.595956]  queue_work_on+0x68/0x84
[   10.595959]  ucsi_connector_change+0x54/0x88 [typec_ucsi]
[   10.595963]  ucsi_init_work+0x834/0x85c [typec_ucsi]
[   10.595968]  process_one_work+0x148/0x29c
[   10.595971]  worker_thread+0x2fc/0x40c
[   10.595974]  kthread+0x110/0x114
[   10.595978]  ret_from_fork+0x10/0x20
[   10.595985] ---[ end trace 0000000000000000 ]---

Warning, because the work is already scheduled.


>  	return 0;
>  
>  err_unregister:
> -- 
> 2.40.1
> 

Re: [PATCH 2/5] usb: typec: ucsi: Check for notifications after init

[Christian A. Ehrhardt]

Hi,

On Fri, Mar 29, 2024 at 06:21:08PM +0200, Dmitry Baryshkov wrote:
> On Wed, Mar 20, 2024 at 08:39:23AM +0100, Christian A. Ehrhardt wrote:
> > The completion notification for the final SET_NOTIFICATION_ENABLE
> > command during initialization can include a connector change
> > notification.  However, at the time this completion notification is
> > processed, the ucsi struct is not ready to handle this notification.
> > As a result the notification is ignored and the controller
> > never sends an interrupt again.
> > 
> > Re-check CCI for a pending connector state change after
> > initialization is complete. Adjust the corresponding debug
> > message accordingly.
> > 
> > Fixes: 71a1fa0df2a3 ("usb: typec: ucsi: Store the notification mask")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
> > ---
> >  drivers/usb/typec/ucsi/ucsi.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> > index 8a6645ffd938..dceeed207569 100644
> > --- a/drivers/usb/typec/ucsi/ucsi.c
> > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > @@ -1237,7 +1237,7 @@ void ucsi_connector_change(struct ucsi *ucsi, u8 num)
> >  	struct ucsi_connector *con = &ucsi->connector[num - 1];
> >  
> >  	if (!(ucsi->ntfy & UCSI_ENABLE_NTFY_CONNECTOR_CHANGE)) {
> > -		dev_dbg(ucsi->dev, "Bogus connector change event\n");
> > +		dev_dbg(ucsi->dev, "Early connector change event\n");
> >  		return;
> >  	}
> >  
> > @@ -1636,6 +1636,7 @@ static int ucsi_init(struct ucsi *ucsi)
> >  {
> >  	struct ucsi_connector *con, *connector;
> >  	u64 command, ntfy;
> > +	u32 cci;
> >  	int ret;
> >  	int i;
> >  
> > @@ -1688,6 +1689,13 @@ static int ucsi_init(struct ucsi *ucsi)
> >  
> >  	ucsi->connector = connector;
> >  	ucsi->ntfy = ntfy;
> > +
> > +	ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci));
> > +	if (ret)
> > +		return ret;
> > +	if (UCSI_CCI_CONNECTOR(READ_ONCE(cci)))
> > +		ucsi_connector_change(ucsi, cci);
> > +
> 
> I think this leaves place for the race. With this patchset + "Ack
> connector change early" in place Neil triggered the following backtrace
> on sm8550 HDK while testing my UCSI-qcom-fixes patchset:

Sorry, but this seems to be a brown paper bag change.
- The READ_ONCE is bogus and a remnant of a prevoius verion of the
  change.
- Calling ->read should probably be done with the PPM lock held.
- The argument to ucsi_connector_change() must be
  UCSI_CCI_CONNECTOR(cci) instead of the plain cci.
I'll send a fix.

> What happens:
[ ... ]
> 
> [   10.595807] ------------[ cut here ]------------
> [   10.595808] WARNING: CPU: 6 PID: 101 at kernel/workqueue.c:2384 __queue_work+0x374/0x474
> 
> [skipped the register dump]
> 
> [   10.595953]  __queue_work+0x374/0x474
> [   10.595956]  queue_work_on+0x68/0x84
> [   10.595959]  ucsi_connector_change+0x54/0x88 [typec_ucsi]
> [   10.595963]  ucsi_init_work+0x834/0x85c [typec_ucsi]
> [   10.595968]  process_one_work+0x148/0x29c
> [   10.595971]  worker_thread+0x2fc/0x40c
> [   10.595974]  kthread+0x110/0x114
> [   10.595978]  ret_from_fork+0x10/0x20
> [   10.595985] ---[ end trace 0000000000000000 ]---
> 
> Warning, because the work is already scheduled.

No, the reason is the wrong connector number. Scheduling a work that
is already scheduled is fine.

Best regards
Christian

Re: [PATCH 5/5] usb: typec: ucsi: Clear UCSI_CCI_RESET_COMPLETE before reset

[Sasha Levin]

On Wed, Mar 20, 2024 at 08:39:26AM +0100, Christian A. Ehrhardt wrote:
>Check the UCSI_CCI_RESET_COMPLETE complete flag before starting
>another reset. Use a UCSI_SET_NOTIFICATION_ENABLE command to clear
>the flag if it is set.

Hi Christian,

It looks like kernelci is able to trigger the warning added by this
commit:

[   15.988528] WARNING: CPU: 0 PID: 8 at drivers/usb/typec/ucsi/ucsi.c:1377 ucsi_reset_ppm+0x1af/0x1c0 [typec_ucsi]
[   15.999924] Modules linked in: snd_sof_pci_intel_cnl snd_sof_intel_hda_generic snd_sof_intel_hda_common snd_soc_hdac_hda snd_sof_intel_hda_mlink snd_sof_intel_hda snd_sof_pci snd_sof_xtensa_dsp snd_sof snd_sof_utils snd_soc_acpi_intel_match snd_soc_acpi snd_soc_acpi_intel_sdca_quirks snd_soc_sdca snd_soc_avs snd_soc_hda_codec snd_hda_ext_core btusb btrtl snd_soc_core intel_pmc_core_pltdrv iTCO_wdt intel_ish_ipc snd_compress btintel btbcm intel_pmc_bxt intel_pmc_core btmtk ucsi_acpi bluetooth gsmi wilco_charger wilco_ec_telem typec_ucsi intel_vsec pmt_telemetry roles pmt_class i2c_hid_acpi iwlmvm x86_pkg_temp_thermal snd_pcm_dmaengine iwlwifi watchdog i2c_hid intel_ishtp idma64 rtc_wilco_ec wilco_ec_debugfs uvcvideo processor_thermal_device_pci_legacy videobuf2_vmalloc elan_i2c intel_soc_dts_iosf videobuf2_memops uvc videobuf2_v4l2 videobuf2_common processor_thermal_device processor_thermal_wt_hint processor_thermal_rfim processor_thermal_wt_req processor_thermal_power_floor processor_thermal_mbox int3403_thermal
[   16.000010]  int340x_thermal_zone typec memconsole_coreboot memconsole vpd_sysfs wilco_ec pinctrl_cannonlake intel_vbtn int3400_thermal wilco_ec_events chromeos_pstore coreboot_table acpi_thermal_rel
[   16.120093] CPU: 0 UID: 0 PID: 8 Comm: kworker/0:0 Not tainted 6.13.0-rc2 #1
[   16.127972] Hardware name: Dell Inc. Arcada/Arcada, BIOS Google_Arcada.12200.103.0 07/29/2020
Linux debian-bookworm-amd64 6.13.0-rc2 #1 SMP PREEMPT_DYNAMIC Su[   16.137499] Workqueue: events_long ucsi_init_work [typec_ucsi]
[   16.150229] RIP: 0010:ucsi_reset_ppm+0x1af/0x1c0 [typec_ucsi]
[   16.156654] Code: 44 24 04 a9 00 00 00 08 0f 85 36 ff ff ff 4c 89 74 24 10 48 8b 05 b1 fe f1 e1 49 39 c4 79 8d bb 92 ff ff ff e9 1b ff ff ff 90 <0f> 0b 90 e9 4c ff ff ff e8 74 72 f9 df 0f 1f 40 00 90 90 90 90 90
[   16.177649] RSP: 0018:ffffa62bc0053dd0 EFLAGS: 00010206
[   16.189709] RAX: 0000000008000000 RBX: 0000000000000000 RCX: 0000000000000002
[   16.203899] RDX: 00000000fffba9c0 RSI: ffffa62bc0053dd4 RDI: ffff91f601610200
[   16.211860] RBP: ffff91f601610200 R08: 0000000000000400 R09: 0000000000000001
[   16.219831] R10: 0000000000000001 R11: 00000000000ee6b2 R12: 00000000fffba9be
[   16.227801] R13: ffff91f6016102c0 R14: ffff91f60006f605 R15: ffff91f6001b8000
[   16.235770] FS:  0000000000000000(0000) GS:ffff91f764400000(0000) knlGS:0000000000000000
[   16.244810] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   16.251226] CR2: 00007f5bcc93b030 CR3: 000000022542e001 CR4: 00000000003706f0
[   16.259195] Call Trace:
[   16.261923]  <TASK>
[   16.264261]  ? __warn+0x84/0x130
[   16.267862]  ? ucsi_reset_ppm+0x1af/0x1c0 [typec_ucsi]
[   16.273600]  ? report_bug+0x164/0x190
[   16.277679]  ? handle_bug+0x54/0x90
[   16.281572]  ? exc_invalid_op+0x17/0x70
[   16.285853]  ? asm_exc_invalid_op+0x1a/0x20
[   16.290523]  ? ucsi_reset_ppm+0x1af/0x1c0 [typec_ucsi]
[   16.296261]  ucsi_init_work+0xaa/0xb20 [typec_ucsi]
[   16.301709]  process_one_work+0x160/0x390
[   16.306185]  worker_thread+0x2a0/0x3b0
[   16.310359]  ? __pfx_worker_thread+0x10/0x10
[   16.315126]  kthread+0xc8/0xf0
[   16.318533]  ? __pfx_kthread+0x10/0x10
[   16.322706]  ret_from_fork+0x2c/0x50
[   16.326687]  ? __pfx_kthread+0x10/0x10
[   16.330869]  ret_from_fork_asm+0x1a/0x30
[   16.335238]  </TASK>

-- 
Thanks,
Sasha

Re: [PATCH 5/5] usb: typec: ucsi: Clear UCSI_CCI_RESET_COMPLETE before reset

[Christian A. Ehrhardt]

Hi,

[ CC: Saranya Gopal <saranya.gopal@intel.com> ]

On Sun, Dec 15, 2024 at 01:34:12PM -0500, Sasha Levin wrote:
> On Wed, Mar 20, 2024 at 08:39:26AM +0100, Christian A. Ehrhardt wrote:
> > Check the UCSI_CCI_RESET_COMPLETE complete flag before starting
> > another reset. Use a UCSI_SET_NOTIFICATION_ENABLE command to clear
> > the flag if it is set.
> 
> Hi Christian,
> 
> It looks like kernelci is able to trigger the warning added by this
> commit:
> 
> [   15.988528] WARNING: CPU: 0 PID: 8 at drivers/usb/typec/ucsi/ucsi.c:1377 ucsi_reset_ppm+0x1af/0x1c0 [typec_ucsi]
> [ ... ]

I think I can see what's going on.

First of all: The warning is harmless and UCSI will still work as
expected (but there is an additional delay during init).

The trigger for the warning is likely this commit (reviewed by me, so ...):

| commit fa48d7e81624efdf398b990a9049e9cd75a5aead
| Author: Saranya Gopal <saranya.gopal@intel.com>
| Date:   Fri Aug 30 14:13:42 2024 +0530
| 
|     usb: typec: ucsi: Do not call ACPI _DSM method for UCSI read
|     operations

The (compile tested) diff below should fix it and I can turn this
into a proper patch but I lost access to test hardware with UCSI,
thus this would need a "Tested-by:" from someone else before it can
be included. Maybe Saranya can do this?

     Best regards   Christian


commit b44ba223cd840e6dbab6c7f69da6203c7a8ba570
Author: Christian A. Ehrhardt <lk@c--e.de>
Date:   Mon Dec 16 21:52:46 2024 +0100

    acpi: typec: ucsi: Introduce a ->poll_cci method
    
    For the ACPI backend of UCSI the UCSI "registers" are just
    a memory copy of the register values in an opregion. The ACPI
    implementation in the BIOS ensures that the opregion contents
    are synced to the embedded controller and it ensures that the
    registers (in particular CCI) are synced back to the opregion
    on notifications. While there is an ACPI call that syncs the
    actual registers to the opregion there is rarely a need to do
    this and on some ACPI implementations it actually breaks in
    various interesting ways.
    
    The only reason to force a sync from the embedded controller
    is to poll CCI while notifications are disabled. Only the
    ucsi core knows if this is the case and guessing based on
    the current command is suboptimal.
    
    Thus introduce a ->poll_cci() method that works like
    ->read_cci() with an additional forced sync and document that
    this should be used when polling with notifications disabled.
    For all other backends that presumably don't have this issue
    use the same implementation for both methods.
    
    Fixes: fa48d7e81624 ("usb: typec: ucsi: Do not call ACPI _DSM method for UCSI read operations")
    Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index fcf499cc9458..0fe1476f4c29 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -1346,7 +1346,7 @@ static int ucsi_reset_ppm(struct ucsi *ucsi)
 
 	mutex_lock(&ucsi->ppm_lock);
 
-	ret = ucsi->ops->read_cci(ucsi, &cci);
+	ret = ucsi->ops->poll_cci(ucsi, &cci);
 	if (ret < 0)
 		goto out;
 
@@ -1364,7 +1364,7 @@ static int ucsi_reset_ppm(struct ucsi *ucsi)
 
 		tmo = jiffies + msecs_to_jiffies(UCSI_TIMEOUT_MS);
 		do {
-			ret = ucsi->ops->read_cci(ucsi, &cci);
+			ret = ucsi->ops->poll_cci(ucsi, &cci);
 			if (ret < 0)
 				goto out;
 			if (cci & UCSI_CCI_COMMAND_COMPLETE)
@@ -1393,7 +1393,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_cci(ucsi, &cci);
+		ret = ucsi->ops->poll_cci(ucsi, &cci);
 		if (ret)
 			goto out;
 
@@ -1929,8 +1929,8 @@ struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops)
 	struct ucsi *ucsi;
 
 	if (!ops ||
-	    !ops->read_version || !ops->read_cci || !ops->read_message_in ||
-	    !ops->sync_control || !ops->async_control)
+	    !ops->read_version || !ops->read_cci || !ops->poll_cci ||
+	    !ops->read_message_in || !ops->sync_control || !ops->async_control)
 		return ERR_PTR(-EINVAL);
 
 	ucsi = kzalloc(sizeof(*ucsi), GFP_KERNEL);
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index 5ff369c24a2f..e4c563da715f 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -61,6 +61,7 @@ struct dentry;
  * struct ucsi_operations - UCSI I/O operations
  * @read_version: Read implemented UCSI version
  * @read_cci: Read CCI register
+ * @poll_cci: Read CCI register while polling with notifications disabled
  * @read_message_in: Read message data from UCSI
  * @sync_control: Blocking control operation
  * @async_control: Non-blocking control operation
@@ -75,6 +76,7 @@ struct dentry;
 struct ucsi_operations {
 	int (*read_version)(struct ucsi *ucsi, u16 *version);
 	int (*read_cci)(struct ucsi *ucsi, u32 *cci);
+	int (*poll_cci)(struct ucsi *ucsi, u32 *cci);
 	int (*read_message_in)(struct ucsi *ucsi, void *val, size_t val_len);
 	int (*sync_control)(struct ucsi *ucsi, u64 command);
 	int (*async_control)(struct ucsi *ucsi, u64 command);
diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
index 5c5515551963..ac1ebb5d9527 100644
--- a/drivers/usb/typec/ucsi/ucsi_acpi.c
+++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
@@ -59,19 +59,24 @@ static int ucsi_acpi_read_version(struct ucsi *ucsi, u16 *version)
 static int ucsi_acpi_read_cci(struct ucsi *ucsi, u32 *cci)
 {
 	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
-	int ret;
-
-	if (UCSI_COMMAND(ua->cmd) == UCSI_PPM_RESET) {
-		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_poll_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;
+
+	return ucsi_acpi_read_cci(ucsi, cci);
+}
+
 static int ucsi_acpi_read_message_in(struct ucsi *ucsi, void *val, size_t val_len)
 {
 	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
@@ -94,6 +99,7 @@ static int ucsi_acpi_async_control(struct ucsi *ucsi, u64 command)
 static const struct ucsi_operations ucsi_acpi_ops = {
 	.read_version = ucsi_acpi_read_version,
 	.read_cci = ucsi_acpi_read_cci,
+	.poll_cci = ucsi_acpi_poll_cci,
 	.read_message_in = ucsi_acpi_read_message_in,
 	.sync_control = ucsi_sync_control_common,
 	.async_control = ucsi_acpi_async_control
@@ -142,6 +148,7 @@ static int ucsi_gram_sync_control(struct ucsi *ucsi, u64 command)
 static const struct ucsi_operations ucsi_gram_ops = {
 	.read_version = ucsi_acpi_read_version,
 	.read_cci = ucsi_acpi_read_cci,
+	.poll_cci = ucsi_acpi_poll_cci,
 	.read_message_in = ucsi_gram_read_message_in,
 	.sync_control = ucsi_gram_sync_control,
 	.async_control = ucsi_acpi_async_control
diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
index fcb8e61136cf..bb0dc2005c05 100644
--- a/drivers/usb/typec/ucsi/ucsi_ccg.c
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -664,6 +664,7 @@ static int ucsi_ccg_sync_control(struct ucsi *ucsi, u64 command)
 static const struct ucsi_operations ucsi_ccg_ops = {
 	.read_version = ucsi_ccg_read_version,
 	.read_cci = ucsi_ccg_read_cci,
+	.poll_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,
diff --git a/drivers/usb/typec/ucsi/ucsi_glink.c b/drivers/usb/typec/ucsi/ucsi_glink.c
index 90948cd6d297..a78e53480875 100644
--- a/drivers/usb/typec/ucsi/ucsi_glink.c
+++ b/drivers/usb/typec/ucsi/ucsi_glink.c
@@ -201,6 +201,7 @@ static void pmic_glink_ucsi_connector_status(struct ucsi_connector *con)
 static const struct ucsi_operations pmic_glink_ucsi_ops = {
 	.read_version = pmic_glink_ucsi_read_version,
 	.read_cci = pmic_glink_ucsi_read_cci,
+	.poll_cci = pmic_glink_ucsi_read_cci,
 	.read_message_in = pmic_glink_ucsi_read_message_in,
 	.sync_control = ucsi_sync_control_common,
 	.async_control = pmic_glink_ucsi_async_control,
diff --git a/drivers/usb/typec/ucsi/ucsi_stm32g0.c b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
index 6923fad31d79..57ef7d83a412 100644
--- a/drivers/usb/typec/ucsi/ucsi_stm32g0.c
+++ b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
@@ -424,6 +424,7 @@ static irqreturn_t ucsi_stm32g0_irq_handler(int irq, void *data)
 static const struct ucsi_operations ucsi_stm32g0_ops = {
 	.read_version = ucsi_stm32g0_read_version,
 	.read_cci = ucsi_stm32g0_read_cci,
+	.poll_cci = ucsi_stm32g0_read_cci,
 	.read_message_in = ucsi_stm32g0_read_message_in,
 	.sync_control = ucsi_sync_control_common,
 	.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 f3a5e24ea84d..40e5da4fd2a4 100644
--- a/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
+++ b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
@@ -74,6 +74,7 @@ static int yoga_c630_ucsi_async_control(struct ucsi *ucsi, u64 command)
 const struct ucsi_operations yoga_c630_ucsi_ops = {
 	.read_version = yoga_c630_ucsi_read_version,
 	.read_cci = yoga_c630_ucsi_read_cci,
+	.poll_cci = yoga_c630_ucsi_read_cci,
 	.read_message_in = yoga_c630_ucsi_read_message_in,
 	.sync_control = ucsi_sync_control_common,
 	.async_control = yoga_c630_ucsi_async_control,

RE: [PATCH 5/5] usb: typec: ucsi: Clear UCSI_CCI_RESET_COMPLETE before reset

[Gopal, Saranya]

Hi Christian,

> -----Original Message-----
> From: Christian A. Ehrhardt <lk@c--e.de>
> Sent: Tuesday, December 17, 2024 3:17 AM
> To: Sasha Levin <sashal@kernel.org>
> Cc: linux-kernel@vger.kernel.org; Heikki Krogerus
> <heikki.krogerus@linux.intel.com>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; Prashant Malani
> <pmalani@chromium.org>; Jameson Thies <jthies@google.com>;
> Abhishek Pandit-Subedi <abhishekpandit@chromium.org>; Neil
> Armstrong <neil.armstrong@linaro.org>; Uwe Kleine-König <u.kleine-
> koenig@pengutronix.de>; Samuel Čavoj <samuel@cavoj.net>; linux-
> usb@vger.kernel.org; Kenneth Crudup <kenny@panix.com>; Gopal,
> Saranya <saranya.gopal@intel.com>
> Subject: Re: [PATCH 5/5] usb: typec: ucsi: Clear
> UCSI_CCI_RESET_COMPLETE before reset
> 
> 
> Hi,
> 
> [ CC: Saranya Gopal <saranya.gopal@intel.com> ]
> 
> On Sun, Dec 15, 2024 at 01:34:12PM -0500, Sasha Levin wrote:
> > On Wed, Mar 20, 2024 at 08:39:26AM +0100, Christian A. Ehrhardt
> wrote:
> > > Check the UCSI_CCI_RESET_COMPLETE complete flag before
> starting
> > > another reset. Use a UCSI_SET_NOTIFICATION_ENABLE
> command to clear
> > > the flag if it is set.
> >
> > Hi Christian,
> >
> > It looks like kernelci is able to trigger the warning added by this
> > commit:
> >
> > [   15.988528] WARNING: CPU: 0 PID: 8 at
> drivers/usb/typec/ucsi/ucsi.c:1377 ucsi_reset_ppm+0x1af/0x1c0
> [typec_ucsi]
> > [ ... ]
> 
> I think I can see what's going on.
> 
> First of all: The warning is harmless and UCSI will still work as
> expected (but there is an additional delay during init).
> 
> The trigger for the warning is likely this commit (reviewed by me, so
> ...):
> 
> | commit fa48d7e81624efdf398b990a9049e9cd75a5aead
> | Author: Saranya Gopal <saranya.gopal@intel.com>
> | Date:   Fri Aug 30 14:13:42 2024 +0530
> |
> |     usb: typec: ucsi: Do not call ACPI _DSM method for UCSI read
> |     operations
> 
> The (compile tested) diff below should fix it and I can turn this
> into a proper patch but I lost access to test hardware with UCSI,
> thus this would need a "Tested-by:" from someone else before it can
> be included. Maybe Saranya can do this?

I can get access to a couple of systems using UCSI and get this tested tomorrow.

Thanks,
Saranya
> 
>      Best regards   Christian
> 
> 
> commit b44ba223cd840e6dbab6c7f69da6203c7a8ba570
> Author: Christian A. Ehrhardt <lk@c--e.de>
> Date:   Mon Dec 16 21:52:46 2024 +0100
> 
>     acpi: typec: ucsi: Introduce a ->poll_cci method
> 
>     For the ACPI backend of UCSI the UCSI "registers" are just
>     a memory copy of the register values in an opregion. The ACPI
>     implementation in the BIOS ensures that the opregion contents
>     are synced to the embedded controller and it ensures that the
>     registers (in particular CCI) are synced back to the opregion
>     on notifications. While there is an ACPI call that syncs the
>     actual registers to the opregion there is rarely a need to do
>     this and on some ACPI implementations it actually breaks in
>     various interesting ways.
> 
>     The only reason to force a sync from the embedded controller
>     is to poll CCI while notifications are disabled. Only the
>     ucsi core knows if this is the case and guessing based on
>     the current command is suboptimal.
> 
>     Thus introduce a ->poll_cci() method that works like
>     ->read_cci() with an additional forced sync and document that
>     this should be used when polling with notifications disabled.
>     For all other backends that presumably don't have this issue
>     use the same implementation for both methods.
> 
>     Fixes: fa48d7e81624 ("usb: typec: ucsi: Do not call ACPI _DSM
> method for UCSI read operations")
>     Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c
> b/drivers/usb/typec/ucsi/ucsi.c
> index fcf499cc9458..0fe1476f4c29 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -1346,7 +1346,7 @@ static int ucsi_reset_ppm(struct ucsi *ucsi)
> 
>  	mutex_lock(&ucsi->ppm_lock);
> 
> -	ret = ucsi->ops->read_cci(ucsi, &cci);
> +	ret = ucsi->ops->poll_cci(ucsi, &cci);
>  	if (ret < 0)
>  		goto out;
> 
> @@ -1364,7 +1364,7 @@ static int ucsi_reset_ppm(struct ucsi *ucsi)
> 
>  		tmo = jiffies +
> msecs_to_jiffies(UCSI_TIMEOUT_MS);
>  		do {
> -			ret = ucsi->ops->read_cci(ucsi, &cci);
> +			ret = ucsi->ops->poll_cci(ucsi, &cci);
>  			if (ret < 0)
>  				goto out;
>  			if (cci & UCSI_CCI_COMMAND_COMPLETE)
> @@ -1393,7 +1393,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_cci(ucsi, &cci);
> +		ret = ucsi->ops->poll_cci(ucsi, &cci);
>  		if (ret)
>  			goto out;
> 
> @@ -1929,8 +1929,8 @@ struct ucsi *ucsi_create(struct device
> *dev, const struct ucsi_operations *ops)
>  	struct ucsi *ucsi;
> 
>  	if (!ops ||
> -	    !ops->read_version || !ops->read_cci || !ops-
> >read_message_in ||
> -	    !ops->sync_control || !ops->async_control)
> +	    !ops->read_version || !ops->read_cci || !ops->poll_cci ||
> +	    !ops->read_message_in || !ops->sync_control || !ops-
> >async_control)
>  		return ERR_PTR(-EINVAL);
> 
>  	ucsi = kzalloc(sizeof(*ucsi), GFP_KERNEL);
> diff --git a/drivers/usb/typec/ucsi/ucsi.h
> b/drivers/usb/typec/ucsi/ucsi.h
> index 5ff369c24a2f..e4c563da715f 100644
> --- a/drivers/usb/typec/ucsi/ucsi.h
> +++ b/drivers/usb/typec/ucsi/ucsi.h
> @@ -61,6 +61,7 @@ struct dentry;
>   * struct ucsi_operations - UCSI I/O operations
>   * @read_version: Read implemented UCSI version
>   * @read_cci: Read CCI register
> + * @poll_cci: Read CCI register while polling with notifications
> disabled
>   * @read_message_in: Read message data from UCSI
>   * @sync_control: Blocking control operation
>   * @async_control: Non-blocking control operation
> @@ -75,6 +76,7 @@ struct dentry;
>  struct ucsi_operations {
>  	int (*read_version)(struct ucsi *ucsi, u16 *version);
>  	int (*read_cci)(struct ucsi *ucsi, u32 *cci);
> +	int (*poll_cci)(struct ucsi *ucsi, u32 *cci);
>  	int (*read_message_in)(struct ucsi *ucsi, void *val, size_t
> val_len);
>  	int (*sync_control)(struct ucsi *ucsi, u64 command);
>  	int (*async_control)(struct ucsi *ucsi, u64 command);
> diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c
> b/drivers/usb/typec/ucsi/ucsi_acpi.c
> index 5c5515551963..ac1ebb5d9527 100644
> --- a/drivers/usb/typec/ucsi/ucsi_acpi.c
> +++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
> @@ -59,19 +59,24 @@ static int ucsi_acpi_read_version(struct ucsi
> *ucsi, u16 *version)
>  static int ucsi_acpi_read_cci(struct ucsi *ucsi, u32 *cci)
>  {
>  	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
> -	int ret;
> -
> -	if (UCSI_COMMAND(ua->cmd) == UCSI_PPM_RESET) {
> -		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_poll_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;
> +
> +	return ucsi_acpi_read_cci(ucsi, cci);
> +}
> +
>  static int ucsi_acpi_read_message_in(struct ucsi *ucsi, void *val,
> size_t val_len)
>  {
>  	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
> @@ -94,6 +99,7 @@ static int ucsi_acpi_async_control(struct ucsi
> *ucsi, u64 command)
>  static const struct ucsi_operations ucsi_acpi_ops = {
>  	.read_version = ucsi_acpi_read_version,
>  	.read_cci = ucsi_acpi_read_cci,
> +	.poll_cci = ucsi_acpi_poll_cci,
>  	.read_message_in = ucsi_acpi_read_message_in,
>  	.sync_control = ucsi_sync_control_common,
>  	.async_control = ucsi_acpi_async_control
> @@ -142,6 +148,7 @@ static int ucsi_gram_sync_control(struct ucsi
> *ucsi, u64 command)
>  static const struct ucsi_operations ucsi_gram_ops = {
>  	.read_version = ucsi_acpi_read_version,
>  	.read_cci = ucsi_acpi_read_cci,
> +	.poll_cci = ucsi_acpi_poll_cci,
>  	.read_message_in = ucsi_gram_read_message_in,
>  	.sync_control = ucsi_gram_sync_control,
>  	.async_control = ucsi_acpi_async_control
> diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c
> b/drivers/usb/typec/ucsi/ucsi_ccg.c
> index fcb8e61136cf..bb0dc2005c05 100644
> --- a/drivers/usb/typec/ucsi/ucsi_ccg.c
> +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> @@ -664,6 +664,7 @@ static int ucsi_ccg_sync_control(struct ucsi
> *ucsi, u64 command)
>  static const struct ucsi_operations ucsi_ccg_ops = {
>  	.read_version = ucsi_ccg_read_version,
>  	.read_cci = ucsi_ccg_read_cci,
> +	.poll_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,
> diff --git a/drivers/usb/typec/ucsi/ucsi_glink.c
> b/drivers/usb/typec/ucsi/ucsi_glink.c
> index 90948cd6d297..a78e53480875 100644
> --- a/drivers/usb/typec/ucsi/ucsi_glink.c
> +++ b/drivers/usb/typec/ucsi/ucsi_glink.c
> @@ -201,6 +201,7 @@ static void
> pmic_glink_ucsi_connector_status(struct ucsi_connector *con)
>  static const struct ucsi_operations pmic_glink_ucsi_ops = {
>  	.read_version = pmic_glink_ucsi_read_version,
>  	.read_cci = pmic_glink_ucsi_read_cci,
> +	.poll_cci = pmic_glink_ucsi_read_cci,
>  	.read_message_in = pmic_glink_ucsi_read_message_in,
>  	.sync_control = ucsi_sync_control_common,
>  	.async_control = pmic_glink_ucsi_async_control,
> diff --git a/drivers/usb/typec/ucsi/ucsi_stm32g0.c
> b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
> index 6923fad31d79..57ef7d83a412 100644
> --- a/drivers/usb/typec/ucsi/ucsi_stm32g0.c
> +++ b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
> @@ -424,6 +424,7 @@ static irqreturn_t
> ucsi_stm32g0_irq_handler(int irq, void *data)
>  static const struct ucsi_operations ucsi_stm32g0_ops = {
>  	.read_version = ucsi_stm32g0_read_version,
>  	.read_cci = ucsi_stm32g0_read_cci,
> +	.poll_cci = ucsi_stm32g0_read_cci,
>  	.read_message_in = ucsi_stm32g0_read_message_in,
>  	.sync_control = ucsi_sync_control_common,
>  	.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 f3a5e24ea84d..40e5da4fd2a4 100644
> --- a/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
> +++ b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
> @@ -74,6 +74,7 @@ static int yoga_c630_ucsi_async_control(struct
> ucsi *ucsi, u64 command)
>  const struct ucsi_operations yoga_c630_ucsi_ops = {
>  	.read_version = yoga_c630_ucsi_read_version,
>  	.read_cci = yoga_c630_ucsi_read_cci,
> +	.poll_cci = yoga_c630_ucsi_read_cci,
>  	.read_message_in = yoga_c630_ucsi_read_message_in,
>  	.sync_control = ucsi_sync_control_common,
>  	.async_control = yoga_c630_ucsi_async_control,

RE: [PATCH 5/5] usb: typec: ucsi: Clear UCSI_CCI_RESET_COMPLETE before reset

[Gopal, Saranya]

Hi Christian,

> -----Original Message-----
> From: Gopal, Saranya
> Sent: Tuesday, December 17, 2024 9:55 AM
> To: Christian A. Ehrhardt <lk@c--e.de>; Sasha Levin
> <sashal@kernel.org>
> Cc: linux-kernel@vger.kernel.org; Heikki Krogerus
> <heikki.krogerus@linux.intel.com>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; Prashant Malani
> <pmalani@chromium.org>; Jameson Thies <jthies@google.com>;
> Abhishek Pandit-Subedi <abhishekpandit@chromium.org>; Neil
> Armstrong <neil.armstrong@linaro.org>; Uwe Kleine-König <u.kleine-
> koenig@pengutronix.de>; Samuel Čavoj <samuel@cavoj.net>; linux-
> usb@vger.kernel.org; Kenneth Crudup <kenny@panix.com>
> Subject: RE: [PATCH 5/5] usb: typec: ucsi: Clear
> UCSI_CCI_RESET_COMPLETE before reset
> 
> Hi Christian,
> 
> > -----Original Message-----
> > From: Christian A. Ehrhardt <lk@c--e.de>
> > Sent: Tuesday, December 17, 2024 3:17 AM
> > To: Sasha Levin <sashal@kernel.org>
> > Cc: linux-kernel@vger.kernel.org; Heikki Krogerus
> > <heikki.krogerus@linux.intel.com>; Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org>; Prashant Malani
> > <pmalani@chromium.org>; Jameson Thies <jthies@google.com>;
> > Abhishek Pandit-Subedi <abhishekpandit@chromium.org>; Neil
> > Armstrong <neil.armstrong@linaro.org>; Uwe Kleine-König
> <u.kleine-
> > koenig@pengutronix.de>; Samuel Čavoj <samuel@cavoj.net>;
> linux-
> > usb@vger.kernel.org; Kenneth Crudup <kenny@panix.com>; Gopal,
> > Saranya <saranya.gopal@intel.com>
> > Subject: Re: [PATCH 5/5] usb: typec: ucsi: Clear
> > UCSI_CCI_RESET_COMPLETE before reset
> >
> >
> > Hi,
> >
> > [ CC: Saranya Gopal <saranya.gopal@intel.com> ]
> >
> > On Sun, Dec 15, 2024 at 01:34:12PM -0500, Sasha Levin wrote:
> > > On Wed, Mar 20, 2024 at 08:39:26AM +0100, Christian A.
> Ehrhardt
> > wrote:
> > > > Check the UCSI_CCI_RESET_COMPLETE complete flag before
> > starting
> > > > another reset. Use a UCSI_SET_NOTIFICATION_ENABLE
> > command to clear
> > > > the flag if it is set.
> > >
> > > Hi Christian,
> > >
> > > It looks like kernelci is able to trigger the warning added by this
> > > commit:
> > >
> > > [   15.988528] WARNING: CPU: 0 PID: 8 at
> > drivers/usb/typec/ucsi/ucsi.c:1377 ucsi_reset_ppm+0x1af/0x1c0
> > [typec_ucsi]
> > > [ ... ]
> >
> > I think I can see what's going on.
> >
> > First of all: The warning is harmless and UCSI will still work as
> > expected (but there is an additional delay during init).
> >
> > The trigger for the warning is likely this commit (reviewed by me, so
> > ...):
> >
> > | commit fa48d7e81624efdf398b990a9049e9cd75a5aead
> > | Author: Saranya Gopal <saranya.gopal@intel.com>
> > | Date:   Fri Aug 30 14:13:42 2024 +0530
> > |
> > |     usb: typec: ucsi: Do not call ACPI _DSM method for UCSI read
> > |     operations
> >
> > The (compile tested) diff below should fix it and I can turn this
> > into a proper patch but I lost access to test hardware with UCSI,
> > thus this would need a "Tested-by:" from someone else before it
> can
> > be included. Maybe Saranya can do this?
> 
> I can get access to a couple of systems using UCSI and get this tested
> tomorrow.

I realized that all the systems I currently have need the forced sync and hence do not reproduce the reported issue.
I believe this patch needs to be tested on Chromebooks that support UCSI.
If so, it might take longer for me to get access to those systems considering the holiday season.

Thanks,
Saranya
> 
> Thanks,
> Saranya
> >
> >      Best regards   Christian
> >
> >
> > commit b44ba223cd840e6dbab6c7f69da6203c7a8ba570
> > Author: Christian A. Ehrhardt <lk@c--e.de>
> > Date:   Mon Dec 16 21:52:46 2024 +0100
> >
> >     acpi: typec: ucsi: Introduce a ->poll_cci method
> >
> >     For the ACPI backend of UCSI the UCSI "registers" are just
> >     a memory copy of the register values in an opregion. The ACPI
> >     implementation in the BIOS ensures that the opregion contents
> >     are synced to the embedded controller and it ensures that the
> >     registers (in particular CCI) are synced back to the opregion
> >     on notifications. While there is an ACPI call that syncs the
> >     actual registers to the opregion there is rarely a need to do
> >     this and on some ACPI implementations it actually breaks in
> >     various interesting ways.
> >
> >     The only reason to force a sync from the embedded controller
> >     is to poll CCI while notifications are disabled. Only the
> >     ucsi core knows if this is the case and guessing based on
> >     the current command is suboptimal.
> >
> >     Thus introduce a ->poll_cci() method that works like
> >     ->read_cci() with an additional forced sync and document that
> >     this should be used when polling with notifications disabled.
> >     For all other backends that presumably don't have this issue
> >     use the same implementation for both methods.
> >
> >     Fixes: fa48d7e81624 ("usb: typec: ucsi: Do not call ACPI _DSM
> > method for UCSI read operations")
> >     Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
> >
> > diff --git a/drivers/usb/typec/ucsi/ucsi.c
> > b/drivers/usb/typec/ucsi/ucsi.c
> > index fcf499cc9458..0fe1476f4c29 100644
> > --- a/drivers/usb/typec/ucsi/ucsi.c
> > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > @@ -1346,7 +1346,7 @@ static int ucsi_reset_ppm(struct ucsi
> *ucsi)
> >
> >  	mutex_lock(&ucsi->ppm_lock);
> >
> > -	ret = ucsi->ops->read_cci(ucsi, &cci);
> > +	ret = ucsi->ops->poll_cci(ucsi, &cci);
> >  	if (ret < 0)
> >  		goto out;
> >
> > @@ -1364,7 +1364,7 @@ static int ucsi_reset_ppm(struct ucsi
> *ucsi)
> >
> >  		tmo = jiffies +
> > msecs_to_jiffies(UCSI_TIMEOUT_MS);
> >  		do {
> > -			ret = ucsi->ops->read_cci(ucsi, &cci);
> > +			ret = ucsi->ops->poll_cci(ucsi, &cci);
> >  			if (ret < 0)
> >  				goto out;
> >  			if (cci & UCSI_CCI_COMMAND_COMPLETE)
> > @@ -1393,7 +1393,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_cci(ucsi, &cci);
> > +		ret = ucsi->ops->poll_cci(ucsi, &cci);
> >  		if (ret)
> >  			goto out;
> >
> > @@ -1929,8 +1929,8 @@ struct ucsi *ucsi_create(struct device
> > *dev, const struct ucsi_operations *ops)
> >  	struct ucsi *ucsi;
> >
> >  	if (!ops ||
> > -	    !ops->read_version || !ops->read_cci || !ops-
> > >read_message_in ||
> > -	    !ops->sync_control || !ops->async_control)
> > +	    !ops->read_version || !ops->read_cci || !ops->poll_cci ||
> > +	    !ops->read_message_in || !ops->sync_control || !ops-
> > >async_control)
> >  		return ERR_PTR(-EINVAL);
> >
> >  	ucsi = kzalloc(sizeof(*ucsi), GFP_KERNEL);
> > diff --git a/drivers/usb/typec/ucsi/ucsi.h
> > b/drivers/usb/typec/ucsi/ucsi.h
> > index 5ff369c24a2f..e4c563da715f 100644
> > --- a/drivers/usb/typec/ucsi/ucsi.h
> > +++ b/drivers/usb/typec/ucsi/ucsi.h
> > @@ -61,6 +61,7 @@ struct dentry;
> >   * struct ucsi_operations - UCSI I/O operations
> >   * @read_version: Read implemented UCSI version
> >   * @read_cci: Read CCI register
> > + * @poll_cci: Read CCI register while polling with notifications
> > disabled
> >   * @read_message_in: Read message data from UCSI
> >   * @sync_control: Blocking control operation
> >   * @async_control: Non-blocking control operation
> > @@ -75,6 +76,7 @@ struct dentry;
> >  struct ucsi_operations {
> >  	int (*read_version)(struct ucsi *ucsi, u16 *version);
> >  	int (*read_cci)(struct ucsi *ucsi, u32 *cci);
> > +	int (*poll_cci)(struct ucsi *ucsi, u32 *cci);
> >  	int (*read_message_in)(struct ucsi *ucsi, void *val, size_t
> > val_len);
> >  	int (*sync_control)(struct ucsi *ucsi, u64 command);
> >  	int (*async_control)(struct ucsi *ucsi, u64 command);
> > diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c
> > b/drivers/usb/typec/ucsi/ucsi_acpi.c
> > index 5c5515551963..ac1ebb5d9527 100644
> > --- a/drivers/usb/typec/ucsi/ucsi_acpi.c
> > +++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
> > @@ -59,19 +59,24 @@ static int ucsi_acpi_read_version(struct
> ucsi
> > *ucsi, u16 *version)
> >  static int ucsi_acpi_read_cci(struct ucsi *ucsi, u32 *cci)
> >  {
> >  	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
> > -	int ret;
> > -
> > -	if (UCSI_COMMAND(ua->cmd) == UCSI_PPM_RESET) {
> > -		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_poll_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;
> > +
> > +	return ucsi_acpi_read_cci(ucsi, cci);
> > +}
> > +
> >  static int ucsi_acpi_read_message_in(struct ucsi *ucsi, void *val,
> > size_t val_len)
> >  {
> >  	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
> > @@ -94,6 +99,7 @@ static int ucsi_acpi_async_control(struct ucsi
> > *ucsi, u64 command)
> >  static const struct ucsi_operations ucsi_acpi_ops = {
> >  	.read_version = ucsi_acpi_read_version,
> >  	.read_cci = ucsi_acpi_read_cci,
> > +	.poll_cci = ucsi_acpi_poll_cci,
> >  	.read_message_in = ucsi_acpi_read_message_in,
> >  	.sync_control = ucsi_sync_control_common,
> >  	.async_control = ucsi_acpi_async_control
> > @@ -142,6 +148,7 @@ static int ucsi_gram_sync_control(struct
> ucsi
> > *ucsi, u64 command)
> >  static const struct ucsi_operations ucsi_gram_ops = {
> >  	.read_version = ucsi_acpi_read_version,
> >  	.read_cci = ucsi_acpi_read_cci,
> > +	.poll_cci = ucsi_acpi_poll_cci,
> >  	.read_message_in = ucsi_gram_read_message_in,
> >  	.sync_control = ucsi_gram_sync_control,
> >  	.async_control = ucsi_acpi_async_control
> > diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c
> > b/drivers/usb/typec/ucsi/ucsi_ccg.c
> > index fcb8e61136cf..bb0dc2005c05 100644
> > --- a/drivers/usb/typec/ucsi/ucsi_ccg.c
> > +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> > @@ -664,6 +664,7 @@ static int ucsi_ccg_sync_control(struct ucsi
> > *ucsi, u64 command)
> >  static const struct ucsi_operations ucsi_ccg_ops = {
> >  	.read_version = ucsi_ccg_read_version,
> >  	.read_cci = ucsi_ccg_read_cci,
> > +	.poll_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,
> > diff --git a/drivers/usb/typec/ucsi/ucsi_glink.c
> > b/drivers/usb/typec/ucsi/ucsi_glink.c
> > index 90948cd6d297..a78e53480875 100644
> > --- a/drivers/usb/typec/ucsi/ucsi_glink.c
> > +++ b/drivers/usb/typec/ucsi/ucsi_glink.c
> > @@ -201,6 +201,7 @@ static void
> > pmic_glink_ucsi_connector_status(struct ucsi_connector *con)
> >  static const struct ucsi_operations pmic_glink_ucsi_ops = {
> >  	.read_version = pmic_glink_ucsi_read_version,
> >  	.read_cci = pmic_glink_ucsi_read_cci,
> > +	.poll_cci = pmic_glink_ucsi_read_cci,
> >  	.read_message_in = pmic_glink_ucsi_read_message_in,
> >  	.sync_control = ucsi_sync_control_common,
> >  	.async_control = pmic_glink_ucsi_async_control,
> > diff --git a/drivers/usb/typec/ucsi/ucsi_stm32g0.c
> > b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
> > index 6923fad31d79..57ef7d83a412 100644
> > --- a/drivers/usb/typec/ucsi/ucsi_stm32g0.c
> > +++ b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
> > @@ -424,6 +424,7 @@ static irqreturn_t
> > ucsi_stm32g0_irq_handler(int irq, void *data)
> >  static const struct ucsi_operations ucsi_stm32g0_ops = {
> >  	.read_version = ucsi_stm32g0_read_version,
> >  	.read_cci = ucsi_stm32g0_read_cci,
> > +	.poll_cci = ucsi_stm32g0_read_cci,
> >  	.read_message_in = ucsi_stm32g0_read_message_in,
> >  	.sync_control = ucsi_sync_control_common,
> >  	.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 f3a5e24ea84d..40e5da4fd2a4 100644
> > --- a/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
> > +++ b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
> > @@ -74,6 +74,7 @@ static int
> yoga_c630_ucsi_async_control(struct
> > ucsi *ucsi, u64 command)
> >  const struct ucsi_operations yoga_c630_ucsi_ops = {
> >  	.read_version = yoga_c630_ucsi_read_version,
> >  	.read_cci = yoga_c630_ucsi_read_cci,
> > +	.poll_cci = yoga_c630_ucsi_read_cci,
> >  	.read_message_in = yoga_c630_ucsi_read_message_in,
> >  	.sync_control = ucsi_sync_control_common,
> >  	.async_control = yoga_c630_ucsi_async_control,

Re: [PATCH 5/5] usb: typec: ucsi: Clear UCSI_CCI_RESET_COMPLETE before reset

[Fedor Pchelkin]

Hi,

Christian A. Ehrhardt wrote:
> The (compile tested) diff below should fix it and I can turn this
> into a proper patch but I lost access to test hardware with UCSI,
> thus this would need a "Tested-by:" from someone else before it can
> be included. Maybe Saranya can do this?
> 
>      Best regards   Christian
> 
> 
> commit b44ba223cd840e6dbab6c7f69da6203c7a8ba570
> Author: Christian A. Ehrhardt <lk@c--e.de>
> Date:   Mon Dec 16 21:52:46 2024 +0100
> 
>     acpi: typec: ucsi: Introduce a ->poll_cci method

WARNING: CPU: 0 PID: 8 at drivers/usb/typec/ucsi/ucsi.c:1377 ucsi_reset_ppm+0x1af/0x1c0 [typec_ucsi]
is triggered on my laptop on roughly every system boot. When it's not,
there is a
  ucsi_acpi USBC000:00: error -ETIMEDOUT: PPM init failed
message observed in the log.

I've tried the above patch "acpi: typec: ucsi: Introduce a ->poll_cci
method" but the issue is still triggered [1].

Is there any useful info/logs I can provide you for further
investigation of the warning in question?

As the warning is quite reliably triggered on my system, I may help with
the testing of other patches.

[1]:
[    7.312044] WARNING: CPU: 5 PID: 329 at drivers/usb/typec/ucsi/ucsi.c:1377 ucsi_reset_ppm+0x1b4/0x1c0 [typec_ucsi]
[    7.312181] CPU: 5 UID: 0 PID: 329 Comm: kworker/5:1 Not tainted 6.13.0-rc7+ #4
[    7.312184] Hardware name: LENOVO 21D0/LNVNB161216, BIOS J6CN45WW 03/17/2023
[    7.312186] Workqueue: events_long ucsi_init_work [typec_ucsi]
[    7.312190] RIP: 0010:ucsi_reset_ppm+0x1b4/0x1c0 [typec_ucsi]
[    7.312194] Code: 8b 44 24 04 a9 00 00 00 08 0f 85 36 ff ff ff 4c 89 74 24 10 48 8b 05 8b 6e 1c e0 49 39 c5 79 8f bb 92 ff ff ff e9 1b ff ff ff <0f> 0b e9 50 ff ff ff e8 80 f0 db de 90 90 90 90 90 90 90 90 90 90
[    7.312196] RSP: 0018:ffffc0aec0d0fdc0 EFLAGS: 00010206
[    7.312199] RAX: 0000000008000000 RBX: 0000000000000000 RCX: 000000000043c005
[    7.312200] RDX: 00000000fffb881d RSI: ffffc0aec0d0fdc4 RDI: ffff9f6bbf3b5600
[    7.312202] RBP: ffff9f6bbf3b5600 R08: 0000000000000000 R09: ffff9f6bc3e85360
[    7.312203] R10: ffffc0aec0d0fc68 R11: 00000000ffffffff R12: ffffc0aec0d0fdc4
[    7.312205] R13: 00000000fffb8817 R14: ffff9f6bbf3b5660 R15: ffff9f6bbf3b56c0
[    7.312206] FS:  0000000000000000(0000) GS:ffff9f71e1c80000(0000) knlGS:0000000000000000
[    7.312208] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    7.312210] CR2: 00007fbdaed6f0d8 CR3: 000000022d62c000 CR4: 0000000000f50ef0
[    7.312211] PKRU: 55555554
[    7.312213] Call Trace:
[    7.312215]  <TASK>
[    7.312216]  ? ucsi_reset_ppm+0x1b4/0x1c0 [typec_ucsi]
[    7.312220]  ? __warn.cold+0x93/0xfa
[    7.312223]  ? ucsi_reset_ppm+0x1b4/0x1c0 [typec_ucsi]
[    7.312229]  ? report_bug+0xff/0x140
[    7.312234]  ? handle_bug+0x58/0x90
[    7.312237]  ? exc_invalid_op+0x17/0x70
[    7.312240]  ? asm_exc_invalid_op+0x1a/0x20
[    7.312245]  ? ucsi_reset_ppm+0x1b4/0x1c0 [typec_ucsi]
[    7.312250]  ucsi_init_work+0x3c/0x9d0 [typec_ucsi]
[    7.312254]  ? srso_alias_return_thunk+0x5/0xfbef5
[    7.312257]  process_one_work+0x179/0x330
[    7.312262]  worker_thread+0x252/0x390
[    7.312266]  ? __pfx_worker_thread+0x10/0x10
[    7.312268]  kthread+0xd2/0x100
[    7.312271]  ? __pfx_kthread+0x10/0x10
[    7.312273]  ret_from_fork+0x34/0x50
[    7.312276]  ? __pfx_kthread+0x10/0x10
[    7.312279]  ret_from_fork_asm+0x1a/0x30
[    7.312284]  </TASK>
[    7.312285] ---[ end trace 0000000000000000 ]---

--
Thanks,
Fedor

Re: [PATCH 5/5] usb: typec: ucsi: Clear UCSI_CCI_RESET_COMPLETE before reset

[Christian A. Ehrhardt]

Hi Fedor,

On Sun, Jan 19, 2025 at 04:23:21PM +0300, Fedor Pchelkin wrote:
> Christian A. Ehrhardt wrote:
> > The (compile tested) diff below should fix it and I can turn this
> > into a proper patch but I lost access to test hardware with UCSI,
> > thus this would need a "Tested-by:" from someone else before it can
> > be included. Maybe Saranya can do this?
> > 
> >      Best regards   Christian
> > 
> > 
> > commit b44ba223cd840e6dbab6c7f69da6203c7a8ba570
> > Author: Christian A. Ehrhardt <lk@c--e.de>
> > Date:   Mon Dec 16 21:52:46 2024 +0100
> > 
> >     acpi: typec: ucsi: Introduce a ->poll_cci method
> 
> WARNING: CPU: 0 PID: 8 at drivers/usb/typec/ucsi/ucsi.c:1377 ucsi_reset_ppm+0x1af/0x1c0 [typec_ucsi]
> is triggered on my laptop on roughly every system boot. When it's not,
> there is a
>   ucsi_acpi USBC000:00: error -ETIMEDOUT: PPM init failed
> message observed in the log.
> 
> I've tried the above patch "acpi: typec: ucsi: Introduce a ->poll_cci
> method" but the issue is still triggered [1].
> 
> Is there any useful info/logs I can provide you for further
> investigation of the warning in question?
> 
> As the warning is quite reliably triggered on my system, I may help with
> the testing of other patches.

Hard to say what might be going on. Some obvious questions to
narrow it down, though:
- Is this something new and UCSI worked before or has UCSI been broken
  with older kernels as well (maybe with different or no error
  messages).
- If you get the warning but not the "PPM init failed" message,
  does UCSI actually work? Try to plug something into the USB-C
  ports and watch out for additional error messages (possibly after
  a timeout). Do new files/devices show up in sysfs?
- Printing the value of CCI at various stages of the init process
  might help us to understand what's going on.

Best regards,
Christian

Re: [PATCH 5/5] usb: typec: ucsi: Clear UCSI_CCI_RESET_COMPLETE before reset

[Fedor Pchelkin]

On Wed, 22. Jan 22:11, Christian A. Ehrhardt wrote:
> 
> Hi Fedor,
> 
> On Sun, Jan 19, 2025 at 04:23:21PM +0300, Fedor Pchelkin wrote:
> > Christian A. Ehrhardt wrote:
> > > The (compile tested) diff below should fix it and I can turn this
> > > into a proper patch but I lost access to test hardware with UCSI,
> > > thus this would need a "Tested-by:" from someone else before it can
> > > be included. Maybe Saranya can do this?
> > > 
> > >      Best regards   Christian
> > > 
> > > 
> > > commit b44ba223cd840e6dbab6c7f69da6203c7a8ba570
> > > Author: Christian A. Ehrhardt <lk@c--e.de>
> > > Date:   Mon Dec 16 21:52:46 2024 +0100
> > > 
> > >     acpi: typec: ucsi: Introduce a ->poll_cci method
> > 
> > WARNING: CPU: 0 PID: 8 at drivers/usb/typec/ucsi/ucsi.c:1377 ucsi_reset_ppm+0x1af/0x1c0 [typec_ucsi]
> > is triggered on my laptop on roughly every system boot. When it's not,
> > there is a
> >   ucsi_acpi USBC000:00: error -ETIMEDOUT: PPM init failed
> > message observed in the log.
> > 
> > I've tried the above patch "acpi: typec: ucsi: Introduce a ->poll_cci
> > method" but the issue is still triggered [1].
> > 
> > Is there any useful info/logs I can provide you for further
> > investigation of the warning in question?
> > 
> > As the warning is quite reliably triggered on my system, I may help with
> > the testing of other patches.
> 
> Hard to say what might be going on. Some obvious questions to
> narrow it down, though:
> - Is this something new and UCSI worked before or has UCSI been broken
>   with older kernels as well (maybe with different or no error
>   messages).

Thanks for the feedback and sorry for the delay!

Yep, I've eventually checked this: as it stands, there's always been a
"-ETIMEDOUT: PPM init failed" observed on starting ucsi_init_work during
the boot. Back to v5.12 at least - the oldest kernel I've managed to boot
on this laptop.

On the other hand, the WARNING appears only after the commit fa48d7e81624
("usb: typec: ucsi: Do not call ACPI _DSM method for UCSI read operations").

> - If you get the warning but not the "PPM init failed" message,
>   does UCSI actually work? Try to plug something into the USB-C
>   ports and watch out for additional error messages (possibly after
>   a timeout). Do new files/devices show up in sysfs?

Well, it's interesting. When there is a WARNING and no "PPM init failed"
message, it works because ucsi_init() goes on. When there is a "PPM init
failed", UCSI doesn't actually initialize successfully and it doesn't work.

And I probably didn't pay attention to the "PPM init failed" messages
earlier because I'm not an active UCSI user, utilize Type-C port only for
the power supply (this always works and I guess the not working UCSI
doesn't affect this directly). On the opposite, the big WARNING during the
boot now became more visible :)


It appears like PPM is not ready yet for communication during the boot.

Increasing a timeout just to 2x eliminates the errors in my case:

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index fcf499cc9458..b1a4470214b6 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -1362,7 +1362,7 @@ static int ucsi_reset_ppm(struct ucsi *ucsi)
                if (ret < 0)
                        goto out;
 
-               tmo = jiffies + msecs_to_jiffies(UCSI_TIMEOUT_MS);
+               tmo = jiffies + msecs_to_jiffies(UCSI_TIMEOUT_MS * 2);
                do {
                        ret = ucsi->ops->read_cci(ucsi, &cci);
                        if (ret < 0)
@@ -1382,7 +1382,7 @@ static int ucsi_reset_ppm(struct ucsi *ucsi)
        if (ret < 0)
                goto out;
 
-       tmo = jiffies + msecs_to_jiffies(UCSI_TIMEOUT_MS);
+       tmo = jiffies + msecs_to_jiffies(UCSI_TIMEOUT_MS * 2);
 
        do {
                if (time_is_before_jiffies(tmo)) {


Turning the laptop several times on and off, I'd say the average time
taken for the initial reset takes around 8000ms:

[    2.568534] ucsi_acpi USBC000:00: enter ucsi_reset_ppm()
[   10.875710] ucsi_acpi USBC000:00: exit ucsi_reset_ppm(), ret 0

I see that UCSI_TIMEOUT_MS is already chosen to be a rather significant
value, much bigger than what the specs say. Maybe ucsi_init_work races
with something? Could this ever happen here? Or just a firmware/hardware
issue...

> - Printing the value of CCI at various stages of the init process
>   might help us to understand what's going on.

During ucsi_reset_ppm() in case of a timeout the reported value of CCI is
always zero and doesn't change on read/poll attempts. In case of the
WARNING it's always read as UCSI_CCI_RESET_COMPLETE thus it WARNs but
ucsi_reset_ppm() returns zero and the further initialization goes on
without any errors.

Is the usage of WARN macros justifiable here if it may potentially be
caused only by the firmware/hardware errors (well, at a quick glance) and
not an issue which can fixed at the kernel level? E.g. the timeout
situation here is not reported by WARN, but by simple printks..

Re: [PATCH 5/5] usb: typec: ucsi: Clear UCSI_CCI_RESET_COMPLETE before reset

[Fedor Pchelkin]

On Tue, 28. Jan 16:58, Fedor Pchelkin wrote:
> It appears like PPM is not ready yet for communication during the boot.
> 
> Increasing a timeout just to 2x eliminates the errors in my case:

That said, if I manually load/unload ucsi_acpi module later, it also
initializes perfectly without the need to increase the timeout.

The significant delays are seen only during the boot phase..