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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 24 Jan 2024 10:10:24 +0200
From: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To: "Christian A. Ehrhardt" <lk@...e.de>
Cc: linux-usb@...r.kernel.org, Dell.Client.Kernel@...l.com,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Neil Armstrong <neil.armstrong@...aro.org>,
	Hans de Goede <hdegoede@...hat.com>,
	Jack Pham <quic_jackp@...cinc.com>,
	Fabrice Gasnier <fabrice.gasnier@...s.st.com>,
	Samuel Čavoj <samuel@...oj.net>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/3] usb: ucsi_acpi: Fix command completion handling

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@...r.kernel.org
> Signed-off-by: Christian A. Ehrhardt <lk@...e.de>

Acked-by: Heikki Krogerus <heikki.krogerus@...ux.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