| Message ID | 20240829100109.562429-1-lk@c--e.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] usb: typec: ucsi: Enable ASUS zenbook quirk for VivoBooks | expand |
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,
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
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,
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,
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(+)