Hi Greg, > -----Original Message----- > From: Christian A. Ehrhardt <lk@xxxxxxx> > Sent: Wednesday, September 11, 2024 3:21 AM > To: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx> > Cc: Gopal, Saranya <saranya.gopal@xxxxxxxxx>; linux- > usb@xxxxxxxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; Regupathy, > Rajaram <rajaram.regupathy@xxxxxxxxx> > Subject: Re: [PATCH] usb: typec: ucsi: Do not call ACPI _DSM method > for UCSI read operations > > > Hi Heikki, > > On Tue, Sep 10, 2024 at 02:21:30PM +0300, Heikki Krogerus wrote: > > On Mon, Sep 09, 2024 at 08:32:53PM +0200, Christian A. Ehrhardt > wrote: > > > > > > Hi Heikki, > > > > > > On Mon, Sep 09, 2024 at 12:20:47PM +0300, Heikki Krogerus > wrote: > > > > Hi Saranya, Christian, > > > > > > > > On Fri, Sep 06, 2024 at 11:47:42AM +0000, Gopal, Saranya > wrote: > > > > > Hi Heikki, Christian, > > > > > > > > > > > -----Original Message----- > > > > > > From: Christian A. Ehrhardt <lk@xxxxxxx> > > > > > > Sent: Saturday, August 31, 2024 3:40 AM > > > > > > To: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx> > > > > > > Cc: Gopal, Saranya <saranya.gopal@xxxxxxxxx>; linux- > > > > > > usb@xxxxxxxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; > Regupathy, > > > > > > Rajaram <rajaram.regupathy@xxxxxxxxx> > > > > > > Subject: Re: [PATCH] usb: typec: ucsi: Do not call ACPI _DSM > method > > > > > > for UCSI read operations > > > > > > > > > > > > > > > > > > Hi Heikki, Hi Saranya, > > > > > > > > > > > > On Fri, Aug 30, 2024 at 11:44:33AM +0300, Heikki Krogerus > wrote: > > > > > > > On Fri, Aug 30, 2024 at 02:13:42PM +0530, Saranya Gopal > wrote: > > > > > > > > ACPI _DSM methods are needed only for UCSI write > operations > > > > > > and for reading > > > > > > > > CCI during RESET_PPM operation. So, remove _DSM calls > from > > > > > > other places. > > > > > > > > While there, remove the Zenbook quirk also since the > default > > > > > > behavior > > > > > > > > now aligns with the Zenbook quirk. With this change, > > > > > > GET_CONNECTOR_STATUS > > > > > > > > returns at least 6 seconds faster than before in > Arrowlake-S > > > > > > platforms. > > > > > > > > > > > > > > > > Reviewed-by: Heikki Krogerus > <heikki.krogerus@xxxxxxxxxxxxxxx> > > > > > > > > Signed-off-by: Saranya Gopal > <saranya.gopal@xxxxxxxxx> > > > > > > > > > > > > > > Maybe this should be marked as a fix. I think this covers: > > > > > > > https://lore.kernel.org/linux- > usb/20240829100109.562429-2- > > > > > > lk@xxxxxxx/ > > > > > > > > > > > > > > > > > Heikki, > > > > > I see that Christian's other patch is marked as a fix already > (https://lore.kernel.org/linux-usb/20240906065853.637205-1-lk@c-- > e.de/T/#u). > > > > > > > > The other part still needs a fix. > > > > > > Well technically not. I've established with the reporter of > > > https://bugzilla.kernel.org/show_bug.cgi?id=219108 > > > that the immediate regression (keyboard on ASUS laptop not > working) is > > > fixed with the ucsi.c patch (that got your Reviewed-By today) > alone. > > > > > > UCSI on the ASUS laptop is still broken but it always was, AFAICT. > > > Thus I'd like to push the above mentioned patch as the fix for > > > the regression. > > > > > > The reporter was very helpful and responsive in testing and > > > I intend to look into the reason why UCSI does not work after > > > that with the reporter's help. > > > > > > > On Thu, 5 Sept 2024 at 20:00, Christian A. Ehrhardt <lk@c-- > e.de> wrote: > > > > > > > > > > > > > > Hi again, > > > > > > > > > > attached is version 4 of the patch. This will not fix the error > > > > > messages we talked about (I have to think about this some > more). > > > > > > > > > > It should fix your keyboard issues, though. > > > > > > > > > > Heikki had another request to change the patch and it would > be > > > > > cool if you could test this version to make sure that it really > > > > > fixes your immediate problem. > > > > > > > > > > Best regards, > > > > > Christian > > > > > > > > > > > > [ 0.019168] [Firmware Bug]: CPU8: Topology domain 1 shift 7 > != 6 > > > > [ 0.019168] [Firmware Bug]: CPU8: Topology domain 2 shift 7 > != 6 > > > > [ 0.019168] [Firmware Bug]: CPU8: Topology domain 3 shift 7 > != 6 > > > > [ 0.019168] [Firmware Bug]: CPU8: Topology domain 4 shift 7 > != 6 > > > > [ 0.019168] [Firmware Bug]: CPU8: Topology domain 5 shift 7 > != 6 > > > > [ 0.019168] [Firmware Bug]: CPU8: Topology domain 6 shift 7 > != 6 > > > > [ 0.019168] [Firmware Bug]: CPU9: Topology domain 1 shift 7 > != 6 > > > > [ 0.019168] [Firmware Bug]: CPU9: Topology domain 2 shift 7 > != 6 > > > > [ 0.019168] [Firmware Bug]: CPU9: Topology domain 3 shift 7 > != 6 > > > > [ 0.019168] [Firmware Bug]: CPU9: Topology domain 4 shift 7 > != 6 > > > > [ 0.019168] [Firmware Bug]: CPU9: Topology domain 5 shift 7 > != 6 > > > > [ 0.019168] [Firmware Bug]: CPU9: Topology domain 6 shift 7 > != 6 > > > > [ 0.019168] [Firmware Bug]: CPU10: Topology domain 1 shift 7 > != 6 > > > > [ 0.019168] [Firmware Bug]: CPU10: Topology domain 2 shift 7 > != 6 > > > > [ 0.019168] [Firmware Bug]: CPU10: Topology domain 3 shift 7 > != 6 > > > > [ 0.019168] [Firmware Bug]: CPU10: Topology domain 4 shift 7 > != 6 > > > > [ 0.019168] [Firmware Bug]: CPU10: Topology domain 5 shift 7 > != 6 > > > > [ 0.019168] [Firmware Bug]: CPU10: Topology domain 6 shift 7 > != 6 > > > > [ 0.019168] [Firmware Bug]: CPU11: Topology domain 1 shift 7 > != 6 > > > > [ 0.019168] [Firmware Bug]: CPU11: Topology domain 2 shift 7 > != 6 > > > > [ 0.019168] [Firmware Bug]: CPU11: Topology domain 3 shift 7 > != 6 > > > > [ 0.019168] [Firmware Bug]: CPU11: Topology domain 4 shift 7 > != 6 > > > > [ 0.019168] [Firmware Bug]: CPU11: Topology domain 5 shift 7 > != 6 > > > > [ 0.019168] [Firmware Bug]: CPU11: Topology domain 6 shift 7 > != 6 > > > > [ 0.019168] [Firmware Bug]: CPU12: Topology domain 1 shift 7 > != 6 > > > > [ 0.019168] [Firmware Bug]: CPU12: Topology domain 2 shift 7 > != 6 > > > > [ 0.019168] [Firmware Bug]: CPU12: Topology domain 3 shift 7 > != 6 > > > > [ 0.019168] [Firmware Bug]: CPU12: Topology domain 4 shift 7 > != 6 > > > > > > > > > > > > > > > So, can this patch go in as it is? > > > > > Please let me know if I need to resubmit with any changes. > > > > > > > > If you prefer that we go with Christian's patch to fix the issue > > > > - which is fine by me - you need to rebase this on top of his > patch in > > > > any case. So you will need to resend this either way. > > > > > > > > Christian would you mind resending that second patch after all > where > > > > you take the Zenbook quirk into use on that ASUS system? > > > > > > See above. The immediate regression is fixed with the already > > > reviewed patch alone. The remaining issue with UCSI on the ASUS > > > laptop not working can be fixed separately. > > > > > > I'd rather base a fix for UCSI on the ASUS laptop onto Saranya's > > > patch because I think that patch is the correct thing to do. > > > > > > Unfortunately, testing by the original reporter was inconclusive > > > wrt. this. I have one report of a test run with the (classical) > > > ASUS quirk (and the other patch) where UCSI on the ASUS laptop > > > did work. Patch version v1 was the result of this. > > > > > > With Saranya's patch and my patch to ucsi.c the regression was > gone > > > but UCSI did _not_ work. > > > > > > As this does not make sense because Saranya's patch should be > > > equivalent to the ASUS zenbook quirk. Thus this needs more > > > investigation and dropping the zenbook quirk patch looks like the > > > correct thing to do. > > > > > > > Let's make that as the actual fix for the issue. Maybe it's more > clear > > > > that way. > > > > > > Please let me know if you disagree and I can resend the ASUS > quirk > > > patch. > > > > No, that's not necessary. So we go ahead with this patch from > Saranya > > as is - we don't caim it fixes anything. Then you guys continue > > debugging that UCSI not working on the ASUS laptop issue. If I got > > this correct then: > > Exactly. And > https://lore.kernel.org/all/20240906065853.637205-1-lk@xxxxxxx/ > proceeds but is independent. > > > Reviewed-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx> > > > > If there was nothing else, then my appologies for all the noise. > > No need to. The state of things was confusing and this type of > "noise" is your job as a maintainer :-) > > Thanks for the review. Gentle ping to consider taking this patch. Heikki and Christian have reviewed the patch and no more changes were suggested. The patch still applies clean to linux-next. Please let me know if any more changes need to be made. Thanks, Saranya > > Best regards, > Christian
