| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Zf1Wz5veKzKgm18H@kuha.fi.intel.com>
Date: Fri, 22 Mar 2024 12:00:47 +0200
From: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To: "Christian A. Ehrhardt" <lk@...e.de>
Cc: linux-kernel@...r.kernel.org,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Prashant Malani <pmalani@...omium.org>,
Jameson Thies <jthies@...gle.com>,
Abhishek Pandit-Subedi <abhishekpandit@...omium.org>,
Neil Armstrong <neil.armstrong@...aro.org>,
Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
Samuel Čavoj <samuel@...oj.net>,
linux-usb@...r.kernel.org, Kenneth Crudup <kenny@...ix.com>
Subject: Re: [PATCH 4/5] usb: typec: ucsi_acpi: Refactor and fix DELL quirk
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@...r.kernel.org
> Signed-off-by: Christian A. Ehrhardt <lk@...e.de>
Reviewed-by: Heikki Krogerus <heikki.krogerus@...ux.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
Powered by blists - more mailing lists