usb: typec: ucsi_acpi: Refactor and fix DELL quirk

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>
Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8550-QRD
Link: https://lore.kernel.org/r/20240320073927.1641788-5-lk@c--e.de
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
index 928eacbeb21ac4cc5b8857644969bff7aba7a8a1..7b3ac133ef86180823bf92e8a5f85cfe3e03b4dd 100644 (file)
--- 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);
 }