Received-SPF: pass (google.com: domain of linux-kernel+bounces-213060-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 13 Jun 2024 13:53:10 +0300
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
To: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] usb: typec: ucsi_acpi: Add LG Gram quirk
Message-ID: <ZmrPlvKarsQLYQgj@kuha.fi.intel.com>
References: <20240612-gram_quirk-v1-1-52b0ff0e1546@tecnico.ulisboa.pt>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240612-gram_quirk-v1-1-52b0ff0e1546@tecnico.ulisboa.pt>

On Wed, Jun 12, 2024 at 02:13:10PM +0100, Diogo Ivo wrote:
> Some LG Gram laptops report a bogus connector change event after a
> GET_PDOS command for the partner's source PDOs, which disappears from
> the CCI after acknowledging the command. However, the subsequent
> GET_CONNECTOR_STATUS in ucsi_handle_connector_change() still reports
> this bogus change in bits 5 and 6, leading to the UCSI core re-checking
> the partner's source PDOs and thus to an infinite loop.
> 
> Fix this by adding a quirk that signals when a potentially buggy GET_PDOS
> command is used, checks the status change report and clears it if it is a
> bogus event before sending it to the UCSI core.
> 
> Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>

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

> ---
> The models affected by this bug have been reported to be of several forms:
> 1xZ90Q, 1xZD90Q, 1xZB90Q, x = {5, 6, 7}, and as such this patch matches
> only on the final 90Q as well as the product family since the "90Q" string
> may collide with other LG models by being too short. If there are other
> better ways of achieving this match I would be happy to hear about them.
> ---
>  drivers/usb/typec/ucsi/ucsi_acpi.c | 61 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
> index 8d112c3edae5..adf32ca0f761 100644
> --- a/drivers/usb/typec/ucsi/ucsi_acpi.c
> +++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
> @@ -25,6 +25,7 @@ struct ucsi_acpi {
>  	unsigned long flags;
>  #define UCSI_ACPI_COMMAND_PENDING	1
>  #define UCSI_ACPI_ACK_PENDING		2
> +#define UCSI_ACPI_CHECK_BOGUS_EVENT	3
>  	guid_t guid;
>  	u64 cmd;
>  };
> @@ -128,6 +129,58 @@ static const struct ucsi_operations ucsi_zenbook_ops = {
>  	.async_write = ucsi_acpi_async_write
>  };
>  
> +static int ucsi_gram_read(struct ucsi *ucsi, unsigned int offset,
> +			  void *val, size_t val_len)
> +{
> +	u16 bogus_change = UCSI_CONSTAT_POWER_LEVEL_CHANGE |
> +			   UCSI_CONSTAT_PDOS_CHANGE;
> +	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
> +	struct ucsi_connector_status *status;
> +	int ret;
> +
> +	ret = ucsi_acpi_read(ucsi, offset, val, val_len);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (UCSI_COMMAND(ua->cmd) == UCSI_GET_CONNECTOR_STATUS &&
> +	    test_bit(UCSI_ACPI_CHECK_BOGUS_EVENT, &ua->flags) &&
> +	    offset == UCSI_MESSAGE_IN) {
> +		status = (struct ucsi_connector_status *)val;
> +
> +		/* Clear the bogus change */
> +		if (status->change == bogus_change)
> +			status->change = 0;
> +
> +		clear_bit(UCSI_ACPI_CHECK_BOGUS_EVENT, &ua->flags);
> +	}
> +
> +	return ret;
> +}
> +
> +static int ucsi_gram_sync_write(struct ucsi *ucsi, unsigned int offset,
> +				const void *val, size_t val_len)
> +{
> +	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
> +	int ret;
> +
> +	ret = ucsi_acpi_sync_write(ucsi, offset, val, val_len);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (UCSI_COMMAND(ua->cmd) == UCSI_GET_PDOS &&
> +	    ua->cmd & UCSI_GET_PDOS_PARTNER_PDO(1) &&
> +	    ua->cmd & UCSI_GET_PDOS_SRC_PDOS)
> +		set_bit(UCSI_ACPI_CHECK_BOGUS_EVENT, &ua->flags);
> +
> +	return ret;
> +}
> +
> +static const struct ucsi_operations ucsi_gram_ops = {
> +	.read = ucsi_gram_read,
> +	.sync_write = ucsi_gram_sync_write,
> +	.async_write = ucsi_acpi_async_write
> +};
> +
>  static const struct dmi_system_id ucsi_acpi_quirks[] = {
>  	{
>  		.matches = {
> @@ -136,6 +189,14 @@ static const struct dmi_system_id ucsi_acpi_quirks[] = {
>  		},
>  		.driver_data = (void *)&ucsi_zenbook_ops,
>  	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "LG Electronics"),
> +			DMI_MATCH(DMI_PRODUCT_FAMILY, "LG gram PC"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "90Q"),
> +		},
> +		.driver_data = (void *)&ucsi_gram_ops,
> +	},
>  	{ }
>  };
>  
> 
> ---
> base-commit: 5821bf2dffbe18fe1f097dbb027415fa15a38e9a
> change-id: 20240612-gram_quirk-ac150257c415
> 
> Best regards,
> -- 
> Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>

-- 
heikki