[PATCH v2 1/2] usb: typec: ucsi: Enable ASUS zenbook quirk for VivoBooks

[PATCH v2 1/2] usb: typec: ucsi: Enable ASUS zenbook quirk for VivoBooks

[Christian A. Ehrhardt]

The quirk for some ASUS zenbook models is required for
ASUS VivoBooks. Apply the quirk to these as well.

This is part of the fix for the builtin keyboard on ASUS
VivoBooks.

Reported-by: Anurag Bijea <icaliberdev@gmail.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219108
Bisected-by: Christian Heusel <christian@heusel.eu>
Fixes: de52aca4d9d5 ("usb: typec: ucsi: Never send a lone connector change ack")
Cc: stable@vger.kernel.org
Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
---
 drivers/usb/typec/ucsi/ucsi.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 4039851551c1..540cb1d2822c 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -38,6 +38,10 @@
 
 void ucsi_notify_common(struct ucsi *ucsi, u32 cci)
 {
+	/* Ignore bogus data in CCI if busy indicator is set. */
+	if (cci & UCSI_CCI_BUSY)
+		return;
+
 	if (UCSI_CCI_CONNECTOR(cci))
 		ucsi_connector_change(ucsi, UCSI_CCI_CONNECTOR(cci));
 
@@ -1249,6 +1253,10 @@ static void ucsi_handle_connector_change(struct work_struct *work)
 
 	mutex_lock(&con->lock);
 
+	if (!test_and_set_bit(EVENT_PENDING, &ucsi->flags))
+		dev_err_once(ucsi->dev, "%s entered without EVENT_PENDING\n",
+			     __func__);
+
 	command = UCSI_GET_CONNECTOR_STATUS | UCSI_CONNECTOR_NUMBER(con->num);
 
 	ret = ucsi_send_command_common(ucsi, command, &con->status,
-- 
2.43.0

[PATCH v2 2/2] usb: typec: ucsi: Fix busy loop on ASUS VivoBooks

[Christian A. Ehrhardt]

If the busy indicator is set, all other fields in CCI should be
clear according to the spec. However, some UCSI implementations do
not follow this rule and report bogus data in CCI along with the
busy indicator. Ignore the contents of CCI if the busy indicator is
set.

If a command timeout is hit it is possible that the EVENT_PENDING
bit is cleared while connector work is still scheduled which can
cause the EVENT_PENDING bit to go out of sync with scheduled connector
work. Check and set the EVENT_PENDING bit on entry to
ucsi_handle_connector_change() to fix this.

Reported-by: Anurag Bijea <icaliberdev@gmail.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219108
Bisected-by: Christian Heusel <christian@heusel.eu>
Tested-by: Anurag Bijea <icaliberdev@gmail.com>
Fixes: de52aca4d9d5 ("usb: typec: ucsi: Never send a lone connector change ack")
Cc: stable@vger.kernel.org
Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
---
 drivers/usb/typec/ucsi/ucsi_acpi.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
index 7a5dff8d9cc6..aa586525ab4c 100644
--- a/drivers/usb/typec/ucsi/ucsi_acpi.c
+++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
@@ -197,6 +197,13 @@ static const struct dmi_system_id ucsi_acpi_quirks[] = {
 		},
 		.driver_data = (void *)&ucsi_zenbook_ops,
 	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "VivoBook_ASUSLaptop"),
+		},
+		.driver_data = (void *)&ucsi_zenbook_ops,
+	},
 	{
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "LG Electronics"),
-- 
2.43.0

Re: [PATCH v2 1/2] usb: typec: ucsi: Enable ASUS zenbook quirk for VivoBooks

[Heikki Krogerus]

Hi, Christian,

Sorry, I did not look at this properly in v1.

On Thu, Aug 29, 2024 at 12:01:08PM +0200, Christian A. Ehrhardt wrote:
> The quirk for some ASUS zenbook models is required for
> ASUS VivoBooks. Apply the quirk to these as well.
> 
> This is part of the fix for the builtin keyboard on ASUS
> VivoBooks.

I think that explanation goes to patch 2/2 and vise versa.

> Reported-by: Anurag Bijea <icaliberdev@gmail.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219108
> Bisected-by: Christian Heusel <christian@heusel.eu>
> Fixes: de52aca4d9d5 ("usb: typec: ucsi: Never send a lone connector change ack")
> Cc: stable@vger.kernel.org
> Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
> ---
>  drivers/usb/typec/ucsi/ucsi.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 4039851551c1..540cb1d2822c 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -38,6 +38,10 @@
>  
>  void ucsi_notify_common(struct ucsi *ucsi, u32 cci)
>  {
> +	/* Ignore bogus data in CCI if busy indicator is set. */
> +	if (cci & UCSI_CCI_BUSY)
> +		return;

This does not look correct. Doesn't this mean you'll timeout always if
BUSY is set?

Couldn't you just check the BUSY as the first action, and then clear
the other bits in CCI if it is set, if that is the problem?

Btw. Does 4f322657ade1 ("usb: typec: ucsi: Call CANCEL from single
location") affect the situation in any way?

thanks,

-- 
heikki

Re: [PATCH v2 1/2] usb: typec: ucsi: Enable ASUS zenbook quirk for VivoBooks

[Christian A. Ehrhardt]

Hi Heikki,

On Fri, Aug 30, 2024 at 11:12:52AM +0300, Heikki Krogerus wrote:
> Hi, Christian,
> 
> Sorry, I did not look at this properly in v1.
> 
> On Thu, Aug 29, 2024 at 12:01:08PM +0200, Christian A. Ehrhardt wrote:
> > The quirk for some ASUS zenbook models is required for
> > ASUS VivoBooks. Apply the quirk to these as well.
> > 
> > This is part of the fix for the builtin keyboard on ASUS
> > VivoBooks.
> 
> I think that explanation goes to patch 2/2 and vise versa.

Obviously! Sorry about that.

> 
> > Reported-by: Anurag Bijea <icaliberdev@gmail.com>
> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219108
> > Bisected-by: Christian Heusel <christian@heusel.eu>
> > Fixes: de52aca4d9d5 ("usb: typec: ucsi: Never send a lone connector change ack")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
> > ---
> >  drivers/usb/typec/ucsi/ucsi.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> > index 4039851551c1..540cb1d2822c 100644
> > --- a/drivers/usb/typec/ucsi/ucsi.c
> > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > @@ -38,6 +38,10 @@
> >  
> >  void ucsi_notify_common(struct ucsi *ucsi, u32 cci)
> >  {
> > +	/* Ignore bogus data in CCI if busy indicator is set. */
> > +	if (cci & UCSI_CCI_BUSY)
> > +		return;
> 
> This does not look correct. Doesn't this mean you'll timeout always if
> BUSY is set?

This is only in the notify function that would do the wakeup on
command completion. The very point of this change is that we do no
wakeup if UCSI_CCI_BUSY is set along with other bogus bits.

The UCSI controller is supposed to send another notification without
the busy bit set once the command completes.

Note that the ASUS laptop actually sends notifications with the BUSY
bit set while processing a command. This is presumably to let us know
that the command is being processed but that it takes longer.

For example this is a possible sequence:
ucsi_sync_control_common: cmd=20012	# GET_CONNECT_STATUS
ucsi_notify_common: cci=0x10000002	# BUSY notification
ucsi_notify_common: cci=0x80000904	# Command completion

> Couldn't you just check the BUSY as the first action, and then clear
> the other bits in CCI if it is set, if that is the problem?

That would not make any difference. The value is only used in this function
for a few other checks that look at fields that are supposed to be zero.
Thus zeroing these fields would have the same effect.

I think you had the actual error handling in mind that happens _after_
the timeout hits. CCI is read again there and if it still reports BUSY
the command is canceled.

> Btw. Does 4f322657ade1 ("usb: typec: ucsi: Call CANCEL from single
> location") affect the situation in any way?

I would have to check with the reporter of the bug but I don't think
it makes a difference because this is the error recovery code that
runs after the timeout. I only touched the notification code that would
do the wakeup.

Best regards,
Christian

Re: [PATCH v2 1/2] usb: typec: ucsi: Enable ASUS zenbook quirk for VivoBooks

[Heikki Krogerus]

On Fri, Aug 30, 2024 at 11:40:07PM +0200, Christian A. Ehrhardt wrote:
> 
> Hi Heikki,
> 
> On Fri, Aug 30, 2024 at 11:12:52AM +0300, Heikki Krogerus wrote:
> > Hi, Christian,
> > 
> > Sorry, I did not look at this properly in v1.
> > 
> > On Thu, Aug 29, 2024 at 12:01:08PM +0200, Christian A. Ehrhardt wrote:
> > > The quirk for some ASUS zenbook models is required for
> > > ASUS VivoBooks. Apply the quirk to these as well.
> > > 
> > > This is part of the fix for the builtin keyboard on ASUS
> > > VivoBooks.
> > 
> > I think that explanation goes to patch 2/2 and vise versa.
> 
> Obviously! Sorry about that.
> 
> > 
> > > Reported-by: Anurag Bijea <icaliberdev@gmail.com>
> > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219108
> > > Bisected-by: Christian Heusel <christian@heusel.eu>
> > > Fixes: de52aca4d9d5 ("usb: typec: ucsi: Never send a lone connector change ack")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
> > > ---
> > >  drivers/usb/typec/ucsi/ucsi.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> > > index 4039851551c1..540cb1d2822c 100644
> > > --- a/drivers/usb/typec/ucsi/ucsi.c
> > > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > > @@ -38,6 +38,10 @@
> > >  
> > >  void ucsi_notify_common(struct ucsi *ucsi, u32 cci)
> > >  {
> > > +	/* Ignore bogus data in CCI if busy indicator is set. */
> > > +	if (cci & UCSI_CCI_BUSY)
> > > +		return;
> > 
> > This does not look correct. Doesn't this mean you'll timeout always if
> > BUSY is set?
> 
> This is only in the notify function that would do the wakeup on
> command completion. The very point of this change is that we do no
> wakeup if UCSI_CCI_BUSY is set along with other bogus bits.
> 
> The UCSI controller is supposed to send another notification without
> the busy bit set once the command completes.
> 
> Note that the ASUS laptop actually sends notifications with the BUSY
> bit set while processing a command. This is presumably to let us know
> that the command is being processed but that it takes longer.
> 
> For example this is a possible sequence:
> ucsi_sync_control_common: cmd=20012	# GET_CONNECT_STATUS
> ucsi_notify_common: cci=0x10000002	# BUSY notification
> ucsi_notify_common: cci=0x80000904	# Command completion
> 
> > Couldn't you just check the BUSY as the first action, and then clear
> > the other bits in CCI if it is set, if that is the problem?
> 
> That would not make any difference. The value is only used in this function
> for a few other checks that look at fields that are supposed to be zero.
> Thus zeroing these fields would have the same effect.
> 
> I think you had the actual error handling in mind that happens _after_
> the timeout hits. CCI is read again there and if it still reports BUSY
> the command is canceled.
> 
> > Btw. Does 4f322657ade1 ("usb: typec: ucsi: Call CANCEL from single
> > location") affect the situation in any way?
> 
> I would have to check with the reporter of the bug but I don't think
> it makes a difference because this is the error recovery code that
> runs after the timeout. I only touched the notification code that would
> do the wakeup.

Okay. Can you resend this with the correct commit message.

thanks,

-- 
heikki