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

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 for Android: free password hash cracker in your pocket

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

Message-ID: <ZtI8N5/SCt+dI/6z@cae.in-ulm.de>
Date: Fri, 30 Aug 2024 23:40:07 +0200
From: "Christian A. Ehrhardt" <lk@...e.de>
To: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc: linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
	Anurag Bijea <icaliberdev@...il.com>,
	Christian Heusel <christian@...sel.eu>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
	Jameson Thies <jthies@...gle.com>,
	Abhishek Pandit-Subedi <abhishekpandit@...omium.org>
Subject: Re: [PATCH v2 1/2] usb: typec: ucsi: Enable ASUS zenbook quirk for
 VivoBooks

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@...il.com>
> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219108
> > Bisected-by: Christian Heusel <christian@...sel.eu>
> > Fixes: de52aca4d9d5 ("usb: typec: ucsi: Never send a lone connector change ack")
> > Cc: stable@...r.kernel.org
> > Signed-off-by: Christian A. Ehrhardt <lk@...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