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@xxxxxxx/T/#u). The other part still needs a fix. > 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? Let's make that as the actual fix for the issue. Maybe it's more clear that way. thanks, > > > Christian, can you check this? > > > > The change certainly looks like the correct thing to do and would > > remove the need for the zenbook quirk. I'll try to get that combination > > tested by the original reporter of > > https://bugzilla.kernel.org/show_bug.cgi?id=219108 > > > > > > > > --- > > > > drivers/usb/typec/ucsi/ucsi_acpi.c | 56 +++------------------------- > > -- > > > > 1 file changed, 5 insertions(+), 51 deletions(-) > > > > > > > > diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c > > b/drivers/usb/typec/ucsi/ucsi_acpi.c > > > > index 7a5dff8d9cc6..accf15ff1306 100644 > > > > --- a/drivers/usb/typec/ucsi/ucsi_acpi.c > > > > +++ b/drivers/usb/typec/ucsi/ucsi_acpi.c > > > > @@ -61,9 +61,11 @@ static int ucsi_acpi_read_cci(struct ucsi > > *ucsi, u32 *cci) > > > > struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi); > > > > int ret; > > > > > > > > - ret = ucsi_acpi_dsm(ua, UCSI_DSM_FUNC_READ); > > > > - if (ret) > > > > - return ret; > > > > + if (UCSI_COMMAND(ua->cmd) == UCSI_PPM_RESET) { > > > > + ret = ucsi_acpi_dsm(ua, UCSI_DSM_FUNC_READ); > > > > + if (ret) > > > > + return ret; > > > > + } > > > > > > This is slightly incorrect because we wait for the completion of at > > least one other command (UCSI_SET_NOTIFICATION_ENABLE) by > > polling cci. > > However, this is a very minor corner case. It could be fixed by adding > > an optional ->poll() method or similar that is NULL on other > > implementations and does the DSM READ on ACPI. We could then call > > this > > before read_cci when polling for completion. If this is done - > > >read_cci() > > would never call the DSM method. > > > > However, the change in its current state is a definitive improvement, > > and looks good to me. Thus feel free to add > > Reviewed-by: Christian A. Ehrhardt <lk@xxxxxxx> > > Thanks for the review, Christian. > > Thanks, > Saranya > > > > > > > > > > memcpy(cci, ua->base + UCSI_CCI, sizeof(*cci)); > > > > > > > > @@ -73,11 +75,6 @@ static int ucsi_acpi_read_cci(struct ucsi > > *ucsi, u32 *cci) > > > > static int ucsi_acpi_read_message_in(struct ucsi *ucsi, void *val, > > size_t val_len) > > > > { > > > > struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi); > > > > - int ret; > > > > - > > > > - ret = ucsi_acpi_dsm(ua, UCSI_DSM_FUNC_READ); > > > > - if (ret) > > > > - return ret; > > > > > > > > memcpy(val, ua->base + UCSI_MESSAGE_IN, val_len); > > > > > > > > @@ -102,42 +99,6 @@ static const struct ucsi_operations > > ucsi_acpi_ops = { > > > > .async_control = ucsi_acpi_async_control > > > > }; > > > > > > > > -static int > > > > -ucsi_zenbook_read_cci(struct ucsi *ucsi, u32 *cci) > > > > -{ > > > > - struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi); > > > > - int ret; > > > > - > > > > - if (UCSI_COMMAND(ua->cmd) == UCSI_PPM_RESET) { > > > > - ret = ucsi_acpi_dsm(ua, UCSI_DSM_FUNC_READ); > > > > - if (ret) > > > > - return ret; > > > > - } > > > > - > > > > - memcpy(cci, ua->base + UCSI_CCI, sizeof(*cci)); > > > > - > > > > - return 0; > > > > -} > > > > - > > > > -static int > > > > -ucsi_zenbook_read_message_in(struct ucsi *ucsi, void *val, > > size_t val_len) > > > > -{ > > > > - struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi); > > > > - > > > > - /* UCSI_MESSAGE_IN is never read for PPM_RESET, return > > stored data */ > > > > - memcpy(val, ua->base + UCSI_MESSAGE_IN, val_len); > > > > - > > > > - return 0; > > > > -} > > > > - > > > > -static const struct ucsi_operations ucsi_zenbook_ops = { > > > > - .read_version = ucsi_acpi_read_version, > > > > - .read_cci = ucsi_zenbook_read_cci, > > > > - .read_message_in = ucsi_zenbook_read_message_in, > > > > - .sync_control = ucsi_sync_control_common, > > > > - .async_control = ucsi_acpi_async_control > > > > -}; > > > > - > > > > static int ucsi_gram_read_message_in(struct ucsi *ucsi, void > > *val, size_t val_len) > > > > { > > > > u16 bogus_change = > > UCSI_CONSTAT_POWER_LEVEL_CHANGE | > > > > @@ -190,13 +151,6 @@ static const struct ucsi_operations > > ucsi_gram_ops = { > > > > }; > > > > > > > > static const struct dmi_system_id ucsi_acpi_quirks[] = { > > > > - { > > > > - .matches = { > > > > - DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK > > COMPUTER INC."), > > > > - DMI_MATCH(DMI_PRODUCT_NAME, > > "ZenBook UX325UA_UM325UA"), > > > > - }, > > > > - .driver_data = (void *)&ucsi_zenbook_ops, > > > > - }, > > > > { > > > > .matches = { > > > > DMI_MATCH(DMI_SYS_VENDOR, "LG > > Electronics"), > > > > -- > > > > 2.34.1 -- heikki
