[PATCH 0/2] UCSI race condition resulting in wrong port state

[PATCH 0/2] UCSI race condition resulting in wrong port state

[Benjamin Berg]

From: Benjamin Berg <bberg@redhat.com>

Hi all,

so, I kept running in an issue where the UCSI port information was saying
that power was being delivered (online: 1), while no cable was attached.

The core of the problem is that there are scenarios where UCSI change
notifications are lost. This happens because querying the changes that
happened is done using the GET_CONNECTOR_STATUS command while clearing the
bitfield happens from the separate ACK command. Any change in between will
be lost.

Note that the problem may be almost invisible in the UI as e.g. GNOME will
still show the battery as discharging. But some policies like automatic
suspend may be applied incorrectly.

Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>

Benjamin Berg (2):
  usb: typec: ucsi: acpi: Always decode connector change information
  usb: typec: ucsi: Work around PPM losing change information

 drivers/usb/typec/ucsi/ucsi.c      | 125 ++++++++++++++++++++++++-----
 drivers/usb/typec/ucsi/ucsi.h      |   2 +
 drivers/usb/typec/ucsi/ucsi_acpi.c |   5 +-
 3 files changed, 110 insertions(+), 22 deletions(-)

-- 
2.26.2

[PATCH 1/2] usb: typec: ucsi: acpi: Always decode connector change information

[Benjamin Berg]

From: Benjamin Berg <bberg@redhat.com>

Normal commands may be reporting that a connector has changed. Always
call the usci_connector_change handler and let it take care of
scheduling the work when needed.
Doing this makes the ACPI code path identical to the CCG one.

Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Benjamin Berg <bberg@redhat.com>
---
 drivers/usb/typec/ucsi/ucsi_acpi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
index fbfe8f5933af..04976435ad73 100644
--- a/drivers/usb/typec/ucsi/ucsi_acpi.c
+++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
@@ -103,11 +103,12 @@ static void ucsi_acpi_notify(acpi_handle handle, u32 event, void *data)
 	if (ret)
 		return;
 
+	if (UCSI_CCI_CONNECTOR(cci))
+		ucsi_connector_change(ua->ucsi, UCSI_CCI_CONNECTOR(cci));
+
 	if (test_bit(COMMAND_PENDING, &ua->flags) &&
 	    cci & (UCSI_CCI_ACK_COMPLETE | UCSI_CCI_COMMAND_COMPLETE))
 		complete(&ua->complete);
-	else if (UCSI_CCI_CONNECTOR(cci))
-		ucsi_connector_change(ua->ucsi, UCSI_CCI_CONNECTOR(cci));
 }
 
 static int ucsi_acpi_probe(struct platform_device *pdev)
-- 
2.26.2

[PATCH 2/2] usb: typec: ucsi: Work around PPM losing change information

[Benjamin Berg]

From: Benjamin Berg <bberg@redhat.com>

Some/many PPMs are simply clearing the change bitfield when a
notification on a port is acknowledge. Unfortunately, doing so means
that any changes between the GET_CONNECTOR_STATUS and ACK_CC_CI commands
is simply lost.

Work around this by re-fetching the connector status afterwards. We can
then infer any changes that we see have happened but that may not be
respresented 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.

The worker is also changed to re-schedule itself if a new change
notification happened while it was running.

Doing this fixes quite commonly occurring issues where e.g. the UCSI
power supply would remain online even thought the ThunderBolt cable was
unplugged.

Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Benjamin Berg <bberg@redhat.com>
---
 drivers/usb/typec/ucsi/ucsi.c | 125 ++++++++++++++++++++++++++++------
 drivers/usb/typec/ucsi/ucsi.h |   2 +
 2 files changed, 107 insertions(+), 20 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 758b988ac518..fad8680be7ab 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -53,7 +53,7 @@ static int ucsi_acknowledge_connector_change(struct ucsi *ucsi)
 	ctrl = UCSI_ACK_CC_CI;
 	ctrl |= UCSI_ACK_CONNECTOR_CHANGE;
 
-	return ucsi->ops->async_write(ucsi, UCSI_CONTROL, &ctrl, sizeof(ctrl));
+	return ucsi->ops->sync_write(ucsi, UCSI_CONTROL, &ctrl, sizeof(ctrl));
 }
 
 static int ucsi_exec_command(struct ucsi *ucsi, u64 command);
@@ -625,21 +625,113 @@ 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;
+	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 */
+	clear_bit(EVENT_PENDING, &ucsi->flags);
+	ret = ucsi_acknowledge_connector_change(ucsi);
+	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, &con->status,
-				sizeof(con->status));
+	ret = ucsi_send_command(ucsi, command, &post_ack_status,
+				sizeof(post_ack_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;
+
 	role = !!(con->status.flags & UCSI_CONSTAT_PWR_DIR);
 
 	if (con->status.change & UCSI_CONSTAT_POWER_OPMODE_CHANGE ||
@@ -676,28 +768,19 @@ static void ucsi_handle_connector_change(struct work_struct *work)
 			ucsi_unregister_partner(con);
 	}
 
-	if (con->status.change & UCSI_CONSTAT_CAM_CHANGE) {
-		/*
-		 * We don't need to know the currently supported alt modes here.
-		 * Running GET_CAM_SUPPORTED command just to make sure the PPM
-		 * does not get stuck in case it assumes we do so.
-		 */
-		command = UCSI_GET_CAM_SUPPORTED;
-		command |= UCSI_CONNECTOR_NUMBER(con->num);
-		ucsi_send_command(con->ucsi, command, NULL, 0);
-	}
-
 	if (con->status.change & UCSI_CONSTAT_PARTNER_CHANGE)
 		ucsi_partner_change(con);
 
-	ret = ucsi_acknowledge_connector_change(ucsi);
-	if (ret)
-		dev_err(ucsi->dev, "%s: ACK failed (%d)", __func__, ret);
-
 	trace_ucsi_connector_change(con->num, &con->status);
 
 out_unlock:
-	clear_bit(EVENT_PENDING, &ucsi->flags);
+	if (test_and_clear_bit(EVENT_PENDING, &ucsi->flags)) {
+		schedule_work(&con->work);
+		mutex_unlock(&con->lock);
+		return;
+	}
+
+	clear_bit(EVENT_PROCESSING, &ucsi->flags);
 	mutex_unlock(&con->lock);
 }
 
@@ -715,7 +798,9 @@ void ucsi_connector_change(struct ucsi *ucsi, u8 num)
 		return;
 	}
 
-	if (!test_and_set_bit(EVENT_PENDING, &ucsi->flags))
+	set_bit(EVENT_PENDING, &ucsi->flags);
+
+	if (!test_and_set_bit(EVENT_PROCESSING, &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 cba6f77bea61..543d54a33cb9 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -296,6 +296,7 @@ struct ucsi {
 #define EVENT_PENDING	0
 #define COMMAND_PENDING	1
 #define ACK_PENDING	2
+#define EVENT_PROCESSING	3
 };
 
 #define UCSI_MAX_SVID		5
@@ -322,6 +323,7 @@ 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.26.2

Re: [PATCH 0/2] UCSI race condition resulting in wrong port state

[Heikki Krogerus]

On Fri, Oct 09, 2020 at 04:40:45PM +0200, Benjamin Berg wrote:
> From: Benjamin Berg <bberg@redhat.com>
> 
> Hi all,
> 
> so, I kept running in an issue where the UCSI port information was saying
> that power was being delivered (online: 1), while no cable was attached.
> 
> The core of the problem is that there are scenarios where UCSI change
> notifications are lost. This happens because querying the changes that
> happened is done using the GET_CONNECTOR_STATUS command while clearing the
> bitfield happens from the separate ACK command. Any change in between will
> be lost.
> 
> Note that the problem may be almost invisible in the UI as e.g. GNOME will
> still show the battery as discharging. But some policies like automatic
> suspend may be applied incorrectly.
> 
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>

Both patches:

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

> Benjamin Berg (2):
>   usb: typec: ucsi: acpi: Always decode connector change information
>   usb: typec: ucsi: Work around PPM losing change information
> 
>  drivers/usb/typec/ucsi/ucsi.c      | 125 ++++++++++++++++++++++++-----
>  drivers/usb/typec/ucsi/ucsi.h      |   2 +
>  drivers/usb/typec/ucsi/ucsi_acpi.c |   5 +-
>  3 files changed, 110 insertions(+), 22 deletions(-)

thanks,

-- 
heikki

Re: [PATCH 0/2] UCSI race condition resulting in wrong port state

[Greg Kroah-Hartman]

On Fri, Oct 09, 2020 at 04:40:45PM +0200, Benjamin Berg wrote:
> From: Benjamin Berg <bberg@redhat.com>
> 
> Hi all,
> 
> so, I kept running in an issue where the UCSI port information was saying
> that power was being delivered (online: 1), while no cable was attached.
> 
> The core of the problem is that there are scenarios where UCSI change
> notifications are lost. This happens because querying the changes that
> happened is done using the GET_CONNECTOR_STATUS command while clearing the
> bitfield happens from the separate ACK command. Any change in between will
> be lost.
> 
> Note that the problem may be almost invisible in the UI as e.g. GNOME will
> still show the battery as discharging. But some policies like automatic
> suspend may be applied incorrectly.
> 
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> Benjamin Berg (2):
>   usb: typec: ucsi: acpi: Always decode connector change information
>   usb: typec: ucsi: Work around PPM losing change information

Do these need to be backported to stable kernel releases?  If so, how
far back?

thjanks,

greg k-h

Re: [PATCH 0/2] UCSI race condition resulting in wrong port state

[Greg Kroah-Hartman]

On Wed, Oct 28, 2020 at 10:10:43AM +0100, Greg Kroah-Hartman wrote:
> On Fri, Oct 09, 2020 at 04:40:45PM +0200, Benjamin Berg wrote:
> > From: Benjamin Berg <bberg@redhat.com>
> > 
> > Hi all,
> > 
> > so, I kept running in an issue where the UCSI port information was saying
> > that power was being delivered (online: 1), while no cable was attached.
> > 
> > The core of the problem is that there are scenarios where UCSI change
> > notifications are lost. This happens because querying the changes that
> > happened is done using the GET_CONNECTOR_STATUS command while clearing the
> > bitfield happens from the separate ACK command. Any change in between will
> > be lost.
> > 
> > Note that the problem may be almost invisible in the UI as e.g. GNOME will
> > still show the battery as discharging. But some policies like automatic
> > suspend may be applied incorrectly.
> > 
> > Cc: Hans de Goede <hdegoede@redhat.com>
> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > 
> > Benjamin Berg (2):
> >   usb: typec: ucsi: acpi: Always decode connector change information
> >   usb: typec: ucsi: Work around PPM losing change information
> 
> Do these need to be backported to stable kernel releases?  If so, how
> far back?

Due to the lack of response, I guess they don't need to go to any stable
kernel, so will queue them up for 5.11-rc1.

thanks,

greg k-h

Re: [PATCH 0/2] UCSI race condition resulting in wrong port state

[Benjamin Berg]

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

Hi,

On Fri, 2020-11-06 at 11:47 +0100, Greg Kroah-Hartman wrote:
> Due to the lack of response, I guess they don't need to go to any
> stable kernel, so will queue them up for 5.11-rc1.

Sorry, forgot to reply.

Not including them in stable seems reasonable as I have not seen it
cause major trouble in the wild so far (i.e. only one user report that
seems related).

Benjamin

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

Re: [PATCH 0/2] UCSI race condition resulting in wrong port state

[Salvatore Bonaccorso]

Hi Greg,

On Fri, Nov 06, 2020 at 11:47:25AM +0100, Greg Kroah-Hartman wrote:
> On Wed, Oct 28, 2020 at 10:10:43AM +0100, Greg Kroah-Hartman wrote:
> > On Fri, Oct 09, 2020 at 04:40:45PM +0200, Benjamin Berg wrote:
> > > From: Benjamin Berg <bberg@redhat.com>
> > > 
> > > Hi all,
> > > 
> > > so, I kept running in an issue where the UCSI port information was saying
> > > that power was being delivered (online: 1), while no cable was attached.
> > > 
> > > The core of the problem is that there are scenarios where UCSI change
> > > notifications are lost. This happens because querying the changes that
> > > happened is done using the GET_CONNECTOR_STATUS command while clearing the
> > > bitfield happens from the separate ACK command. Any change in between will
> > > be lost.
> > > 
> > > Note that the problem may be almost invisible in the UI as e.g. GNOME will
> > > still show the battery as discharging. But some policies like automatic
> > > suspend may be applied incorrectly.
> > > 
> > > Cc: Hans de Goede <hdegoede@redhat.com>
> > > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > 
> > > Benjamin Berg (2):
> > >   usb: typec: ucsi: acpi: Always decode connector change information
> > >   usb: typec: ucsi: Work around PPM losing change information
> > 
> > Do these need to be backported to stable kernel releases?  If so, how
> > far back?
> 
> Due to the lack of response, I guess they don't need to go to any stable
> kernel, so will queue them up for 5.11-rc1.

At least one user in Debian (https://bugs.debian.org/992004) would be
happy to have those backported as well to the 5.10.y series (which we
will pick up).

So if Benjamin ack's this, this would be great to have in 5.10.y.

Regards,
Salvatore

Re: [PATCH 0/2] UCSI race condition resulting in wrong port state

[Benjamin Berg]

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

Hi,

On Fri, 2021-08-20 at 15:01 +0200, Salvatore Bonaccorso wrote:
> Hi Greg,
> 
> On Fri, Nov 06, 2020 at 11:47:25AM +0100, Greg Kroah-Hartman wrote:
> > On Wed, Oct 28, 2020 at 10:10:43AM +0100, Greg Kroah-Hartman wrote:
> > > On Fri, Oct 09, 2020 at 04:40:45PM +0200, Benjamin Berg wrote:
> > > > From: Benjamin Berg <bberg@redhat.com>
> > > > 
> > > > Hi all,
> > > > 
> > > > so, I kept running in an issue where the UCSI port information was saying
> > > > that power was being delivered (online: 1), while no cable was attached.
> > > > 
> > > > The core of the problem is that there are scenarios where UCSI change
> > > > notifications are lost. This happens because querying the changes that
> > > > happened is done using the GET_CONNECTOR_STATUS command while clearing the
> > > > bitfield happens from the separate ACK command. Any change in between will
> > > > be lost.
> > > > 
> > > > Note that the problem may be almost invisible in the UI as e.g. GNOME will
> > > > still show the battery as discharging. But some policies like automatic
> > > > suspend may be applied incorrectly.
> > > > 
> > > > Cc: Hans de Goede <hdegoede@redhat.com>
> > > > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > 
> > > > Benjamin Berg (2):
> > > >   usb: typec: ucsi: acpi: Always decode connector change information
> > > >   usb: typec: ucsi: Work around PPM losing change information
> > > 
> > > Do these need to be backported to stable kernel releases?  If so, how
> > > far back?
> > 
> > Due to the lack of response, I guess they don't need to go to any stable
> > kernel, so will queue them up for 5.11-rc1.
> 
> At least one user in Debian (https://bugs.debian.org/992004) would be
> happy to have those backported as well to the 5.10.y series (which we
> will pick up).
> 
> So if Benjamin ack's this, this would be great to have in 5.10.y.

Sure, it is reasonable to pull it into 5.10. At the time it just seemed
to me that it was enough of a corner case to not bother.

Note that there was a somewhat related fix later on (for Qualcomm UCSI
firmware), which probably makes sense to pull in too then.

Including Bjorn into the CC list for that.

commit 8c9b3caab3ac26db1da00b8117901640c55a69dd
Author: Bjorn Andersson <bjorn.andersson@linaro.org>
Date:   Sat May 15 21:09:53 2021 -0700

    usb: typec: ucsi: Clear pending after acking connector change
 
Benjamin

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

Re: [PATCH 0/2] UCSI race condition resulting in wrong port state

[Ian Turner]

On 8/20/21 9:29 AM, Benjamin Berg wrote:
> On Fri, 2021-08-20 at 15:01 +0200, Salvatore Bonaccorso wrote:
>> At least one user in Debian (https://bugs.debian.org/992004) would be
>> happy to have those backported as well to the 5.10.y series (which we
>> will pick up).
>>
>> So if Benjamin ack's this, this would be great to have in 5.10.y.
> Sure, it is reasonable to pull it into 5.10. At the time it just seemed
> to me that it was enough of a corner case to not bother.
>
> Note that there was a somewhat related fix later on (for Qualcomm UCSI
> firmware), which probably makes sense to pull in too then.
>
> Including Bjorn into the CC list for that.
>
> commit 8c9b3caab3ac26db1da00b8117901640c55a69dd
> Author: Bjorn Andersson <bjorn.andersson@linaro.org>
> Date:   Sat May 15 21:09:53 2021 -0700
>
>      usb: typec: ucsi: Clear pending after acking connector change
>   
> Benjamin

I feel that I should mention that I haven't actually tested this change, 
so it's just conjecture on my part that it would fix my issue (though it 
does seem to track pretty closely). I am happy to do that testing if it 
would save others time.

Ian Turner

Re: [PATCH 0/2] UCSI race condition resulting in wrong port state

[Salvatore Bonaccorso]

Hi Ian,

On Fri, Aug 20, 2021 at 07:08:57PM -0400, Ian Turner wrote:
> On 8/20/21 9:29 AM, Benjamin Berg wrote:
> > On Fri, 2021-08-20 at 15:01 +0200, Salvatore Bonaccorso wrote:
> > > At least one user in Debian (https://bugs.debian.org/992004) would be
> > > happy to have those backported as well to the 5.10.y series (which we
> > > will pick up).
> > > 
> > > So if Benjamin ack's this, this would be great to have in 5.10.y.
> > Sure, it is reasonable to pull it into 5.10. At the time it just seemed
> > to me that it was enough of a corner case to not bother.
> > 
> > Note that there was a somewhat related fix later on (for Qualcomm UCSI
> > firmware), which probably makes sense to pull in too then.
> > 
> > Including Bjorn into the CC list for that.
> > 
> > commit 8c9b3caab3ac26db1da00b8117901640c55a69dd
> > Author: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Date:   Sat May 15 21:09:53 2021 -0700
> > 
> >      usb: typec: ucsi: Clear pending after acking connector change
> > Benjamin
> 
> I feel that I should mention that I haven't actually tested this change, so
> it's just conjecture on my part that it would fix my issue (though it does
> seem to track pretty closely). I am happy to do that testing if it would
> save others time.

Ah apologies, I was under the impression that you confirmed that this
was already the right fix. It is pretty close to what you describe, so
if you can additionally confirm the fix, that would be great.

Regards,
Salvatore

Re: [PATCH 0/2] UCSI race condition resulting in wrong port state

[Greg Kroah-Hartman]

On Fri, Aug 20, 2021 at 03:01:53PM +0200, Salvatore Bonaccorso wrote:
> Hi Greg,
> 
> On Fri, Nov 06, 2020 at 11:47:25AM +0100, Greg Kroah-Hartman wrote:

Note, you are responding to an email from a very long time ago...

> > Due to the lack of response, I guess they don't need to go to any stable
> > kernel, so will queue them up for 5.11-rc1.
> 
> At least one user in Debian (https://bugs.debian.org/992004) would be
> happy to have those backported as well to the 5.10.y series (which we
> will pick up).
> 
> So if Benjamin ack's this, this would be great to have in 5.10.y.

What are the git commit ids?  Just ask for them to be applied to stable
like normal...

thanks,

greg k-h

Re: [PATCH 0/2] UCSI race condition resulting in wrong port state

[Salvatore Bonaccorso]

Hi Greg,

On Sat, Aug 21, 2021 at 02:09:45PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Aug 20, 2021 at 03:01:53PM +0200, Salvatore Bonaccorso wrote:
> > Hi Greg,
> > 
> > On Fri, Nov 06, 2020 at 11:47:25AM +0100, Greg Kroah-Hartman wrote:
> 
> Note, you are responding to an email from a very long time ago...

Yeah, was sort of purpose :) (to try to retain the original context of
the question of if the commits should be backported to stable series,
which back then had no need raised)
> 
> > > Due to the lack of response, I guess they don't need to go to any stable
> > > kernel, so will queue them up for 5.11-rc1.
> > 
> > At least one user in Debian (https://bugs.debian.org/992004) would be
> > happy to have those backported as well to the 5.10.y series (which we
> > will pick up).
> > 
> > So if Benjamin ack's this, this would be great to have in 5.10.y.
> 
> What are the git commit ids?  Just ask for them to be applied to stable
> like normal...

Right, aplogies. The two commits were
47ea2929d58c35598e681212311d35b240c373ce and
217504a055325fe76ec1142aa15f14d3db77f94f.

47ea2929d58c ("usb: typec: ucsi: acpi: Always decode connector change information")
217504a05532 ("usb: typec: ucsi: Work around PPM losing change information")

and in the followup Benjamin Berg mentioned to pick as well

8c9b3caab3ac26db1da00b8117901640c55a69dd

8c9b3caab3ac ("usb: typec: ucsi: Clear pending after acking connector change"

a related fix later on.

Regards,
Salvatore

Re: [PATCH 0/2] UCSI race condition resulting in wrong port state

[Ian Turner]

On 8/21/21 2:31 AM, Salvatore Bonaccorso wrote:
> Ah apologies, I was under the impression that you confirmed that this
> was already the right fix. It is pretty close to what you describe, so
> if you can additionally confirm the fix, that would be great.
>
> Regards,
> Salvatore

Well, it would seem that I owe everyone here an apology.

Not only did I not observe this issue with a patched kernel, but I'm 
also now unable to reproduce it booting into a stock kernel. I can only 
speculate about the reasons for this.

I'll come back here if I learn more. Sorry about the noise.

Ian Turner

Re: [PATCH 0/2] UCSI race condition resulting in wrong port state

[Greg Kroah-Hartman]

On Sat, Aug 21, 2021 at 03:01:26PM +0200, Salvatore Bonaccorso wrote:
> Hi Greg,
> 
> On Sat, Aug 21, 2021 at 02:09:45PM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Aug 20, 2021 at 03:01:53PM +0200, Salvatore Bonaccorso wrote:
> > > Hi Greg,
> > > 
> > > On Fri, Nov 06, 2020 at 11:47:25AM +0100, Greg Kroah-Hartman wrote:
> > 
> > Note, you are responding to an email from a very long time ago...
> 
> Yeah, was sort of purpose :) (to try to retain the original context of
> the question of if the commits should be backported to stable series,
> which back then had no need raised)
> > 
> > > > Due to the lack of response, I guess they don't need to go to any stable
> > > > kernel, so will queue them up for 5.11-rc1.
> > > 
> > > At least one user in Debian (https://bugs.debian.org/992004) would be
> > > happy to have those backported as well to the 5.10.y series (which we
> > > will pick up).
> > > 
> > > So if Benjamin ack's this, this would be great to have in 5.10.y.
> > 
> > What are the git commit ids?  Just ask for them to be applied to stable
> > like normal...
> 
> Right, aplogies. The two commits were
> 47ea2929d58c35598e681212311d35b240c373ce and
> 217504a055325fe76ec1142aa15f14d3db77f94f.
> 
> 47ea2929d58c ("usb: typec: ucsi: acpi: Always decode connector change information")
> 217504a05532 ("usb: typec: ucsi: Work around PPM losing change information")
> 
> and in the followup Benjamin Berg mentioned to pick as well
> 
> 8c9b3caab3ac26db1da00b8117901640c55a69dd
> 
> 8c9b3caab3ac ("usb: typec: ucsi: Clear pending after acking connector change"
> 
> a related fix later on.

All now queued up, thanks.

greg k-h