[PATCH v3 0/3] UCSI fixes

[PATCH v3 0/3] UCSI fixes

[Christian A. Ehrhardt]

This small series contains two general bugfixes to ucsi_acpi
and a quirk to make the UCSI controller on various Dell laptops
work. The changes can be applied idependently but all three
are required to fix the Dell issues.

For details on the general bugfixes please refer to the individual
commit messages.

The UCSI interface on a Dell Latitude 5431 stops working after
the first async event with:

    GET_CONNECTOR_STATUS failed (-110)

The core problem is that when sending the ACK_CC_CI command to
clear the connector status changed condition the PPM expects us
to send anothr ack for the command completion condition. However,
the UCSI spec states that no ack for the command completion is
required when the command is ACK_CC_CI (or PPM_RESET).

There are various reports that suggest that several Dell laptops
are affected by this problem. E.g. the kernel bugzilla has this
report which is most likely an instance of this bug:

    https://bugzilla.kernel.org/show_bug.cgi?id=216426

This led me to the somewhat bold conclusion that the quirk should
probably be applied to on Dell systems.

To mitigate potential problems from this dell quirk includes a
probe mechanism that detect the need for the quirk once and we
only deviate from the UCSI spec if the quirk is actually required.

Changes in v3 from v2:
- Add an info message if the quirk is enabled.
- Fix checkpatch errors etc.
- Add Fixes: and CC: stable.

Changes in v2 from v1:
- Add a second general bugfix.
- Remove module parmater and generic quirk infrastructure.
- Implement quirk directly in ucsi_acpi.c
- Add probe logic to reliably detect the need for the quirk 

Christian A. Ehrhardt (3):
  usb: ucsi: Add missing ppm_lock
  usb: ucsi_acpi: Fix command completion handling
  usb: ucsi_acpi: Quirk to ack a connector change ack cmd

 drivers/usb/typec/ucsi/ucsi.c      |  2 +
 drivers/usb/typec/ucsi/ucsi_acpi.c | 86 +++++++++++++++++++++++++++---
 2 files changed, 81 insertions(+), 7 deletions(-)

-- 
2.40.1

[PATCH v3 1/3] usb: ucsi: Add missing ppm_lock

[Christian A. Ehrhardt]

Calling ->sync_write must be done while holding the PPM lock as
the mailbox logic does not support concurrent commands.

At least since the addition of partner task this means that
ucsi_acknowledge_connector_change should be called with the
PPM lock held as it calls ->sync_write.

Thus protect the only call to ucsi_acknowledge_connector_change
with the PPM. All other calls to ->sync_write already happen
under the PPM lock.

Fixes: b9aa02ca39a4 ("usb: typec: ucsi: Add polling mechanism for partner tasks like alt mode checking")
Cc: stable@vger.kernel.org
Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
---
 drivers/usb/typec/ucsi/ucsi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 61b64558f96c..8f9dff993b3d 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -935,7 +935,9 @@ static void ucsi_handle_connector_change(struct work_struct *work)
 
 	clear_bit(EVENT_PENDING, &con->ucsi->flags);
 
+	mutex_lock(&ucsi->ppm_lock);
 	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 v3 2/3] usb: ucsi_acpi: Fix command completion handling

[Christian A. Ehrhardt]

In case of a spurious or otherwise delayed notification it is
possible that CCI still reports the previous completion. The
UCSI spec is aware of this and provides two completion bits in
CCI, one for normal commands and one for acks. As acks and commands
alternate the notification handler can determine if the completion
bit is from the current command.

The initial UCSI code correctly handled this but the distinction
between the two completion bits was lost with the introduction of
the new API.

To fix this revive the ACK_PENDING bit for ucsi_acpi and only complete
commands if the completion bit matches.

Fixes: f56de278e8ec ("usb: typec: ucsi: acpi: Move to the new API")
Cc: stable@vger.kernel.org
Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
---
 drivers/usb/typec/ucsi/ucsi_acpi.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
index 6bbf490ac401..fa222080887d 100644
--- a/drivers/usb/typec/ucsi/ucsi_acpi.c
+++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
@@ -73,9 +73,13 @@ static int ucsi_acpi_sync_write(struct ucsi *ucsi, unsigned int offset,
 				const void *val, size_t val_len)
 {
 	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
+	bool ack = UCSI_COMMAND(*(u64 *)val) == UCSI_ACK_CC_CI;
 	int ret;
 
-	set_bit(COMMAND_PENDING, &ua->flags);
+	if (ack)
+		set_bit(ACK_PENDING, &ua->flags);
+	else
+		set_bit(COMMAND_PENDING, &ua->flags);
 
 	ret = ucsi_acpi_async_write(ucsi, offset, val, val_len);
 	if (ret)
@@ -85,7 +89,10 @@ static int ucsi_acpi_sync_write(struct ucsi *ucsi, unsigned int offset,
 		ret = -ETIMEDOUT;
 
 out_clear_bit:
-	clear_bit(COMMAND_PENDING, &ua->flags);
+	if (ack)
+		clear_bit(ACK_PENDING, &ua->flags);
+	else
+		clear_bit(COMMAND_PENDING, &ua->flags);
 
 	return ret;
 }
@@ -142,8 +149,10 @@ static void ucsi_acpi_notify(acpi_handle handle, u32 event, void *data)
 	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))
+	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))
 		complete(&ua->complete);
 }
 
-- 
2.40.1

[PATCH v3 3/3] usb: ucsi_acpi: Quirk to ack a connector change ack cmd

[Christian A. Ehrhardt]

The PPM on some Dell laptops seems to expect that the ACK_CC_CI
command to clear the connector change notification is in turn
followed by another ACK_CC_CI to acknowledge the ACK_CC_CI command
itself. This is in violation of the UCSI spec that states:

    "The only notification that is not acknowledged by the OPM is
     the command completion notification for the ACK_CC_CI or the
     PPM_RESET command."

Add a quirk to send this ack anyway.
Apply the quirk to all Dell systems.

On the first command that acks a connector change send a dummy
command to determine if it runs into a timeout. Only activate
the quirk if it does. This ensure that we do not break Dell
systems that do not need the quirk.

Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
---
 drivers/usb/typec/ucsi/ucsi_acpi.c | 71 ++++++++++++++++++++++++++++--
 1 file changed, 68 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
index fa222080887d..928eacbeb21a 100644
--- a/drivers/usb/typec/ucsi/ucsi_acpi.c
+++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
@@ -25,6 +25,8 @@ struct ucsi_acpi {
 	unsigned long flags;
 	guid_t guid;
 	u64 cmd;
+	bool dell_quirk_probed;
+	bool dell_quirk_active;
 };
 
 static int ucsi_acpi_dsm(struct ucsi_acpi *ua, int func)
@@ -126,12 +128,73 @@ static const struct ucsi_operations ucsi_zenbook_ops = {
 	.async_write = ucsi_acpi_async_write
 };
 
-static const struct dmi_system_id zenbook_dmi_id[] = {
+/*
+ * 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.
+ */
+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;
+	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)
+			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));
+}
+
+static const struct ucsi_operations ucsi_dell_ops = {
+	.read = ucsi_acpi_read,
+	.sync_write = ucsi_dell_sync_write,
+	.async_write = ucsi_acpi_async_write
+};
+
+static const struct dmi_system_id ucsi_acpi_quirks[] = {
 	{
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
 			DMI_MATCH(DMI_PRODUCT_NAME, "ZenBook UX325UA_UM325UA"),
 		},
+		.driver_data = (void *)&ucsi_zenbook_ops,
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+		},
+		.driver_data = (void *)&ucsi_dell_ops,
 	},
 	{ }
 };
@@ -160,6 +223,7 @@ static int ucsi_acpi_probe(struct platform_device *pdev)
 {
 	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
 	const struct ucsi_operations *ops = &ucsi_acpi_ops;
+	const struct dmi_system_id *id;
 	struct ucsi_acpi *ua;
 	struct resource *res;
 	acpi_status status;
@@ -189,8 +253,9 @@ static int ucsi_acpi_probe(struct platform_device *pdev)
 	init_completion(&ua->complete);
 	ua->dev = &pdev->dev;
 
-	if (dmi_check_system(zenbook_dmi_id))
-		ops = &ucsi_zenbook_ops;
+	id = dmi_first_match(ucsi_acpi_quirks);
+	if (id)
+		ops = id->driver_data;
 
 	ua->ucsi = ucsi_create(&pdev->dev, ops);
 	if (IS_ERR(ua->ucsi))
-- 
2.40.1

Re: [PATCH v3 1/3] usb: ucsi: Add missing ppm_lock

[Heikki Krogerus]

On Sun, Jan 21, 2024 at 09:41:21PM +0100, Christian A. Ehrhardt wrote:
> Calling ->sync_write must be done while holding the PPM lock as
> the mailbox logic does not support concurrent commands.
> 
> At least since the addition of partner task this means that
> ucsi_acknowledge_connector_change should be called with the
> PPM lock held as it calls ->sync_write.
> 
> Thus protect the only call to ucsi_acknowledge_connector_change
> with the PPM. All other calls to ->sync_write already happen
> under the PPM lock.
> 
> Fixes: b9aa02ca39a4 ("usb: typec: ucsi: Add polling mechanism for partner tasks like alt mode checking")
> Cc: stable@vger.kernel.org
> Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
> ---
>  drivers/usb/typec/ucsi/ucsi.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 61b64558f96c..8f9dff993b3d 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -935,7 +935,9 @@ static void ucsi_handle_connector_change(struct work_struct *work)
>  
>  	clear_bit(EVENT_PENDING, &con->ucsi->flags);
>  
> +	mutex_lock(&ucsi->ppm_lock);
>  	ret = ucsi_acknowledge_connector_change(ucsi);
> +	mutex_unlock(&ucsi->ppm_lock);
>  	if (ret)
>  		dev_err(ucsi->dev, "%s: ACK failed (%d)", __func__, ret);

Is this really actually fixing some issue? The function has already
taken the connector lock, so what exactly could happen without this?

thanks,

-- 
heikki

Re: [PATCH v3 2/3] usb: ucsi_acpi: Fix command completion handling

[Heikki Krogerus]

On Sun, Jan 21, 2024 at 09:41:22PM +0100, Christian A. Ehrhardt wrote:
> In case of a spurious or otherwise delayed notification it is
> possible that CCI still reports the previous completion. The
> UCSI spec is aware of this and provides two completion bits in
> CCI, one for normal commands and one for acks. As acks and commands
> alternate the notification handler can determine if the completion
> bit is from the current command.
> 
> The initial UCSI code correctly handled this but the distinction
> between the two completion bits was lost with the introduction of
> the new API.
> 
> To fix this revive the ACK_PENDING bit for ucsi_acpi and only complete
> commands if the completion bit matches.
> 
> Fixes: f56de278e8ec ("usb: typec: ucsi: acpi: Move to the new API")
> Cc: stable@vger.kernel.org
> Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>

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

> ---
>  drivers/usb/typec/ucsi/ucsi_acpi.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
> index 6bbf490ac401..fa222080887d 100644
> --- a/drivers/usb/typec/ucsi/ucsi_acpi.c
> +++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
> @@ -73,9 +73,13 @@ static int ucsi_acpi_sync_write(struct ucsi *ucsi, unsigned int offset,
>  				const void *val, size_t val_len)
>  {
>  	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
> +	bool ack = UCSI_COMMAND(*(u64 *)val) == UCSI_ACK_CC_CI;
>  	int ret;
>  
> -	set_bit(COMMAND_PENDING, &ua->flags);
> +	if (ack)
> +		set_bit(ACK_PENDING, &ua->flags);
> +	else
> +		set_bit(COMMAND_PENDING, &ua->flags);
>  
>  	ret = ucsi_acpi_async_write(ucsi, offset, val, val_len);
>  	if (ret)
> @@ -85,7 +89,10 @@ static int ucsi_acpi_sync_write(struct ucsi *ucsi, unsigned int offset,
>  		ret = -ETIMEDOUT;
>  
>  out_clear_bit:
> -	clear_bit(COMMAND_PENDING, &ua->flags);
> +	if (ack)
> +		clear_bit(ACK_PENDING, &ua->flags);
> +	else
> +		clear_bit(COMMAND_PENDING, &ua->flags);
>  
>  	return ret;
>  }
> @@ -142,8 +149,10 @@ static void ucsi_acpi_notify(acpi_handle handle, u32 event, void *data)
>  	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))
> +	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))
>  		complete(&ua->complete);
>  }
>  
> -- 
> 2.40.1

-- 
heikki

Re: [PATCH v3 3/3] usb: ucsi_acpi: Quirk to ack a connector change ack cmd

[Heikki Krogerus]

On Sun, Jan 21, 2024 at 09:41:23PM +0100, Christian A. Ehrhardt wrote:
> The PPM on some Dell laptops seems to expect that the ACK_CC_CI
> command to clear the connector change notification is in turn
> followed by another ACK_CC_CI to acknowledge the ACK_CC_CI command
> itself. This is in violation of the UCSI spec that states:
> 
>     "The only notification that is not acknowledged by the OPM is
>      the command completion notification for the ACK_CC_CI or the
>      PPM_RESET command."
> 
> Add a quirk to send this ack anyway.
> Apply the quirk to all Dell systems.
> 
> On the first command that acks a connector change send a dummy
> command to determine if it runs into a timeout. Only activate
> the quirk if it does. This ensure that we do not break Dell
> systems that do not need the quirk.
> 
> 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 | 71 ++++++++++++++++++++++++++++--
>  1 file changed, 68 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
> index fa222080887d..928eacbeb21a 100644
> --- a/drivers/usb/typec/ucsi/ucsi_acpi.c
> +++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
> @@ -25,6 +25,8 @@ struct ucsi_acpi {
>  	unsigned long flags;
>  	guid_t guid;
>  	u64 cmd;
> +	bool dell_quirk_probed;
> +	bool dell_quirk_active;
>  };
>  
>  static int ucsi_acpi_dsm(struct ucsi_acpi *ua, int func)
> @@ -126,12 +128,73 @@ static const struct ucsi_operations ucsi_zenbook_ops = {
>  	.async_write = ucsi_acpi_async_write
>  };
>  
> -static const struct dmi_system_id zenbook_dmi_id[] = {
> +/*
> + * 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.
> + */
> +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;
> +	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)
> +			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));
> +}
> +
> +static const struct ucsi_operations ucsi_dell_ops = {
> +	.read = ucsi_acpi_read,
> +	.sync_write = ucsi_dell_sync_write,
> +	.async_write = ucsi_acpi_async_write
> +};
> +
> +static const struct dmi_system_id ucsi_acpi_quirks[] = {
>  	{
>  		.matches = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
>  			DMI_MATCH(DMI_PRODUCT_NAME, "ZenBook UX325UA_UM325UA"),
>  		},
> +		.driver_data = (void *)&ucsi_zenbook_ops,
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +		},
> +		.driver_data = (void *)&ucsi_dell_ops,
>  	},
>  	{ }
>  };
> @@ -160,6 +223,7 @@ static int ucsi_acpi_probe(struct platform_device *pdev)
>  {
>  	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
>  	const struct ucsi_operations *ops = &ucsi_acpi_ops;
> +	const struct dmi_system_id *id;
>  	struct ucsi_acpi *ua;
>  	struct resource *res;
>  	acpi_status status;
> @@ -189,8 +253,9 @@ static int ucsi_acpi_probe(struct platform_device *pdev)
>  	init_completion(&ua->complete);
>  	ua->dev = &pdev->dev;
>  
> -	if (dmi_check_system(zenbook_dmi_id))
> -		ops = &ucsi_zenbook_ops;
> +	id = dmi_first_match(ucsi_acpi_quirks);
> +	if (id)
> +		ops = id->driver_data;
>  
>  	ua->ucsi = ucsi_create(&pdev->dev, ops);
>  	if (IS_ERR(ua->ucsi))
> -- 
> 2.40.1

-- 
heikki

Re: [PATCH v3 1/3] usb: ucsi: Add missing ppm_lock

[Christian A. Ehrhardt]

Hi Heikki,

Thanks for looking into this.

On Wed, Jan 24, 2024 at 10:09:04AM +0200, Heikki Krogerus wrote:
> On Sun, Jan 21, 2024 at 09:41:21PM +0100, Christian A. Ehrhardt wrote:
> > Calling ->sync_write must be done while holding the PPM lock as
> > the mailbox logic does not support concurrent commands.
> > 
> > At least since the addition of partner task this means that
> > ucsi_acknowledge_connector_change should be called with the
> > PPM lock held as it calls ->sync_write.
> > 
> > Thus protect the only call to ucsi_acknowledge_connector_change
> > with the PPM. All other calls to ->sync_write already happen
> > under the PPM lock.
> > 
> > Fixes: b9aa02ca39a4 ("usb: typec: ucsi: Add polling mechanism for partner tasks like alt mode checking")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
> > ---
> >  drivers/usb/typec/ucsi/ucsi.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> > index 61b64558f96c..8f9dff993b3d 100644
> > --- a/drivers/usb/typec/ucsi/ucsi.c
> > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > @@ -935,7 +935,9 @@ static void ucsi_handle_connector_change(struct work_struct *work)
> >  
> >  	clear_bit(EVENT_PENDING, &con->ucsi->flags);
> >  
> > +	mutex_lock(&ucsi->ppm_lock);
> >  	ret = ucsi_acknowledge_connector_change(ucsi);
> > +	mutex_unlock(&ucsi->ppm_lock);
> >  	if (ret)
> >  		dev_err(ucsi->dev, "%s: ACK failed (%d)", __func__, ret);
> 
> Is this really actually fixing some issue? The function has already
> taken the connector lock, so what exactly could happen without this?

I've definitely _seen_ issues with the quirk from PATCH 3/3 if the
lock is missing. I'm pretty sure from looking at the code that races
with other connectors can happen without the quirk, too.

The PPM lock protects the Mailbox and the ACK_PENDING/COMMAND_PENDING
bits and I could observe cases where ucsi_acpi_sync_write() was entered
with the COMMAND_PENDING bit already set. One possible sequence is this:

ucsi_connector_change() for connector #1
	=> schedules partner tasks
	=> Acks the connector change
	=> Releases locks
ucsi_connecotr_change() for connector #2
	=> acquire con lock for #2
	=> Start to process connector state change
	=> Processing reaches the point right before 
	   ucsi_acknowledge_connector_change()
Connector #1 workqueue starts to process one of the partner tasks
	=> Acquire con lock for connector #1
	=> Acquire ppm lock
	=> Enter ucsi_exec_command()
	=> ->sync_write() starts to use the mailbox and sets
	   COMMAND_PENDING
	=> ->sync_write blocks waiting for the command completion
	   with wait_for_completion_timeout().
ucsi_connector_change() for connector #2 continues
	=> execute ucsi_acknowledge_connector_change() and start using
	   the mailbox that is still in use.
	=> BOOM

There is a simpler an much more likely sequence with the dell quirk active.

     regards  Christian

Re: [PATCH v3 1/3] usb: ucsi: Add missing ppm_lock

[Heikki Krogerus]

On Wed, Jan 24, 2024 at 01:04:06PM +0100, Christian A. Ehrhardt wrote:
> 
> Hi Heikki,
> 
> Thanks for looking into this.
> 
> On Wed, Jan 24, 2024 at 10:09:04AM +0200, Heikki Krogerus wrote:
> > On Sun, Jan 21, 2024 at 09:41:21PM +0100, Christian A. Ehrhardt wrote:
> > > Calling ->sync_write must be done while holding the PPM lock as
> > > the mailbox logic does not support concurrent commands.
> > > 
> > > At least since the addition of partner task this means that
> > > ucsi_acknowledge_connector_change should be called with the
> > > PPM lock held as it calls ->sync_write.
> > > 
> > > Thus protect the only call to ucsi_acknowledge_connector_change
> > > with the PPM. All other calls to ->sync_write already happen
> > > under the PPM lock.
> > > 
> > > Fixes: b9aa02ca39a4 ("usb: typec: ucsi: Add polling mechanism for partner tasks like alt mode checking")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
> > > ---
> > >  drivers/usb/typec/ucsi/ucsi.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> > > index 61b64558f96c..8f9dff993b3d 100644
> > > --- a/drivers/usb/typec/ucsi/ucsi.c
> > > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > > @@ -935,7 +935,9 @@ static void ucsi_handle_connector_change(struct work_struct *work)
> > >  
> > >  	clear_bit(EVENT_PENDING, &con->ucsi->flags);
> > >  
> > > +	mutex_lock(&ucsi->ppm_lock);
> > >  	ret = ucsi_acknowledge_connector_change(ucsi);
> > > +	mutex_unlock(&ucsi->ppm_lock);
> > >  	if (ret)
> > >  		dev_err(ucsi->dev, "%s: ACK failed (%d)", __func__, ret);
> > 
> > Is this really actually fixing some issue? The function has already
> > taken the connector lock, so what exactly could happen without this?
> 
> I've definitely _seen_ issues with the quirk from PATCH 3/3 if the
> lock is missing. I'm pretty sure from looking at the code that races
> with other connectors can happen without the quirk, too.
> 
> The PPM lock protects the Mailbox and the ACK_PENDING/COMMAND_PENDING
> bits and I could observe cases where ucsi_acpi_sync_write() was entered
> with the COMMAND_PENDING bit already set. One possible sequence is this:
> 
> ucsi_connector_change() for connector #1
> 	=> schedules partner tasks
> 	=> Acks the connector change
> 	=> Releases locks
> ucsi_connecotr_change() for connector #2
> 	=> acquire con lock for #2
> 	=> Start to process connector state change
> 	=> Processing reaches the point right before 
> 	   ucsi_acknowledge_connector_change()
> Connector #1 workqueue starts to process one of the partner tasks
> 	=> Acquire con lock for connector #1
> 	=> Acquire ppm lock
> 	=> Enter ucsi_exec_command()
> 	=> ->sync_write() starts to use the mailbox and sets
> 	   COMMAND_PENDING
> 	=> ->sync_write blocks waiting for the command completion
> 	   with wait_for_completion_timeout().
> ucsi_connector_change() for connector #2 continues
> 	=> execute ucsi_acknowledge_connector_change() and start using
> 	   the mailbox that is still in use.
> 	=> BOOM
> 
> There is a simpler an much more likely sequence with the dell quirk active.

Okay.

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

-- 
heikki