[PATCH 0/3] UCSI fixes

[PATCH 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 idependantly 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 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 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.

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

Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
---
NOTE: This is not a theoretical issue. I've seen problems resulting
from the missing lock on real hardware.

 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 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")
Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
---
NOTES:
- I've seen real issues as a result of this.
- UCSI implementations other than ACPI might need a similar fix.
  I can give relevant maintainers a heads up once this is properly
  reviewed and integrated.

 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..33dac67154d2 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 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 | 69 ++++++++++++++++++++++++++++--
 1 file changed, 66 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
index 33dac67154d2..a0b08b10c4a9 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,71 @@ 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;
+	}
+
+	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 +221,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 +251,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 1/3] usb: ucsi: Add missing ppm_lock

[Greg Kroah-Hartman]

On Tue, Jan 16, 2024 at 11:40:39PM +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.
> 
> Thus protect the only call to ucsi_acknowledge_connector_change
> with the PPM lock as it calls ->sync_write. All other calls to
> ->sync_write already happen under the PPM lock.
> 
> Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
> ---
> NOTE: This is not a theoretical issue. I've seen problems resulting
> from the missing lock on real hardware.

What commit id does this fix?

Should it be cc: stable?

thanks,

greg k-h

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

[Greg Kroah-Hartman]

On Tue, Jan 16, 2024 at 11:40:40PM +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")
> Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
> ---
> NOTES:
> - I've seen real issues as a result of this.
> - UCSI implementations other than ACPI might need a similar fix.
>   I can give relevant maintainers a heads up once this is properly
>   reviewed and integrated.
> 
>  drivers/usb/typec/ucsi/ucsi_acpi.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- You have marked a patch with a "Fixes:" tag for a commit that is in an
  older released kernel, yet you do not have a cc: stable line in the
  signed-off-by area at all, which means that the patch will not be
  applied to any older kernel releases.  To properly fix this, please
  follow the documented rules in the
  Documentation/process/stable-kernel-rules.rst file for how to resolve
  this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

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

[Christian A. Ehrhardt]

On Wed, Jan 17, 2024 at 06:44:40AM +0100, Greg Kroah-Hartman wrote:
> On Tue, Jan 16, 2024 at 11:40:39PM +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.
> > 
> > Thus protect the only call to ucsi_acknowledge_connector_change
> > with the PPM lock as it calls ->sync_write. All other calls to
> > ->sync_write already happen under the PPM lock.
> > 
> > Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
> > ---
> > NOTE: This is not a theoretical issue. I've seen problems resulting
> > from the missing lock on real hardware.
> 
> What commit id does this fix?

It's hard to tell (due to rewrites, logic and API changes). After
digging a bit more I think it is at least a theoretical issues since
the introduction of partner tasks.

I'll wait a bit for additional feedback and fix this and other issues
noticed by your patch bot (sorry for those) in the next iteration.

> Should it be cc: stable?

Not sure. The race is triggered much more ofter after the quirk added
in patch 3/3, so this may not be a practical issue before that.
I'll add the tag in the next iteration, though.

   thanks   Christian

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

[kernel test robot]

Hi Christian,

kernel test robot noticed the following build warnings:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on usb/usb-next usb/usb-linus westeri-thunderbolt/next linus/master v6.7 next-20240117]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Christian-A-Ehrhardt/usb-ucsi-Add-missing-ppm_lock/20240117-064726
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link:    https://lore.kernel.org/r/20240116224041.220740-4-lk%40c--e.de
patch subject: [PATCH 3/3] usb: ucsi_acpi: Quirk to ack a connector change ack cmd
config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20240118/202401180751.BuI4xr2p-lkp@intel.com/config)
compiler: ClangBuiltLinux clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240118/202401180751.BuI4xr2p-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401180751.BuI4xr2p-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/usb/typec/ucsi/ucsi_acpi.c:132: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
    * Some Dell laptops expect that an ACK command with the


vim +132 drivers/usb/typec/ucsi/ucsi_acpi.c

   130	
   131	/**
 > 132	 * Some Dell laptops expect that an ACK command with the
   133	 * UCSI_ACK_CONNECTOR_CHANGE bit set is followed by a (separate)
   134	 * ACK command that only has the UCSI_ACK_COMMAND_COMPLETE bit set.
   135	 * If this is not done events are not delivered to OSPM and
   136	 * subsequent commands will timeout.
   137	 */
   138	static int
   139	ucsi_dell_sync_write(struct ucsi *ucsi, unsigned int offset,
   140			     const void *val, size_t val_len)
   141	{
   142		struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
   143		u64 cmd = *(u64*)val, ack = 0;
   144		int ret;
   145	
   146		if (UCSI_COMMAND(cmd) == UCSI_ACK_CC_CI &&
   147		    cmd & UCSI_ACK_CONNECTOR_CHANGE)
   148			ack = UCSI_ACK_CC_CI | UCSI_ACK_COMMAND_COMPLETE;
   149	
   150		ret = ucsi_acpi_sync_write(ucsi, offset, val, val_len);
   151		if (ret != 0)
   152			return ret;
   153		if (ack == 0)
   154			return ret;
   155	
   156		if (!ua->dell_quirk_probed) {
   157			ua->dell_quirk_probed = true;
   158	
   159			cmd = UCSI_GET_CAPABILITY;
   160			ret = ucsi_acpi_sync_write(ucsi, UCSI_CONTROL, &cmd,
   161						   sizeof(cmd));
   162			if (ret == 0)
   163				return ucsi_acpi_sync_write(ucsi, UCSI_CONTROL,
   164							    &ack, sizeof(ack));
   165			if (ret != -ETIMEDOUT)
   166				return ret;
   167	
   168			ua->dell_quirk_active = true;
   169		}
   170	
   171		if (!ua->dell_quirk_active)
   172			return ret;
   173	
   174		return ucsi_acpi_sync_write(ucsi, UCSI_CONTROL, &ack, sizeof(ack));
   175	}
   176	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki