| Message ID | 20240830084342.460109-1-saranya.gopal@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | fa48d7e81624efdf398b990a9049e9cd75a5aead |
| Headers | show |
| Series | usb: typec: ucsi: Do not call ACPI _DSM method for UCSI read operations | expand |
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@linux.intel.com> > Signed-off-by: Saranya Gopal <saranya.gopal@intel.com> Maybe this should be marked as a fix. I think this covers: https://lore.kernel.org/linux-usb/20240829100109.562429-2-lk@c--e.de/ Christian, can you check this? thanks, > --- > 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; > + } > > 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
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@linux.intel.com> > > Signed-off-by: Saranya Gopal <saranya.gopal@intel.com> > > Maybe this should be marked as a fix. I think this covers: > https://lore.kernel.org/linux-usb/20240829100109.562429-2-lk@c--e.de/ > > 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@c--e.de> > > > > 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 Best regards, Christian
Hi Heikki, Christian, > -----Original Message----- > From: Christian A. Ehrhardt <lk@c--e.de> > Sent: Saturday, August 31, 2024 3:40 AM > To: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Cc: Gopal, Saranya <saranya.gopal@intel.com>; linux- > usb@vger.kernel.org; gregkh@linuxfoundation.org; Regupathy, > Rajaram <rajaram.regupathy@intel.com> > 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@linux.intel.com> > > > Signed-off-by: Saranya Gopal <saranya.gopal@intel.com> > > > > Maybe this should be marked as a fix. I think this covers: > > https://lore.kernel.org/linux-usb/20240829100109.562429-2- > lk@c--e.de/ > > 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). So, can this patch go in as it is? Please let me know if I need to resubmit with any changes. > > 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@c--e.de> 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 > > Best regards, > Christian
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@c--e.de> > > Sent: Saturday, August 31, 2024 3:40 AM > > To: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > Cc: Gopal, Saranya <saranya.gopal@intel.com>; linux- > > usb@vger.kernel.org; gregkh@linuxfoundation.org; Regupathy, > > Rajaram <rajaram.regupathy@intel.com> > > 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@linux.intel.com> > > > > Signed-off-by: Saranya Gopal <saranya.gopal@intel.com> > > > > > > Maybe this should be marked as a fix. I think this covers: > > > https://lore.kernel.org/linux-usb/20240829100109.562429-2- > > lk@c--e.de/ > > > > > 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. > 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@c--e.de> > > 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
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@c--e.de> > > > Sent: Saturday, August 31, 2024 3:40 AM > > > To: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > Cc: Gopal, Saranya <saranya.gopal@intel.com>; linux- > > > usb@vger.kernel.org; gregkh@linuxfoundation.org; Regupathy, > > > Rajaram <rajaram.regupathy@intel.com> > > > 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@linux.intel.com> > > > > > Signed-off-by: Saranya Gopal <saranya.gopal@intel.com> > > > > > > > > Maybe this should be marked as a fix. I think this covers: > > > > https://lore.kernel.org/linux-usb/20240829100109.562429-2- > > > lk@c--e.de/ > > > > > > > > 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. Best regards, Christian
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@c--e.de> > > > > Sent: Saturday, August 31, 2024 3:40 AM > > > > To: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > > Cc: Gopal, Saranya <saranya.gopal@intel.com>; linux- > > > > usb@vger.kernel.org; gregkh@linuxfoundation.org; Regupathy, > > > > Rajaram <rajaram.regupathy@intel.com> > > > > 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@linux.intel.com> > > > > > > Signed-off-by: Saranya Gopal <saranya.gopal@intel.com> > > > > > > > > > > Maybe this should be marked as a fix. I think this covers: > > > > > https://lore.kernel.org/linux-usb/20240829100109.562429-2- > > > > lk@c--e.de/ > > > > > > > > > > > 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: Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> If there was nothing else, then my appologies for all the noise. thanks,
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@c--e.de> > > > > > Sent: Saturday, August 31, 2024 3:40 AM > > > > > To: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > > > Cc: Gopal, Saranya <saranya.gopal@intel.com>; linux- > > > > > usb@vger.kernel.org; gregkh@linuxfoundation.org; Regupathy, > > > > > Rajaram <rajaram.regupathy@intel.com> > > > > > 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@linux.intel.com> > > > > > > > Signed-off-by: Saranya Gopal <saranya.gopal@intel.com> > > > > > > > > > > > > Maybe this should be marked as a fix. I think this covers: > > > > > > https://lore.kernel.org/linux-usb/20240829100109.562429-2- > > > > > lk@c--e.de/ > > > > > > > > > > > > > > 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@c--e.de/ proceeds but is independent. > Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > 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. Best regards, Christian
Hi Greg, > -----Original Message----- > From: Christian A. Ehrhardt <lk@c--e.de> > Sent: Wednesday, September 11, 2024 3:21 AM > To: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Cc: Gopal, Saranya <saranya.gopal@intel.com>; linux- > usb@vger.kernel.org; gregkh@linuxfoundation.org; Regupathy, > Rajaram <rajaram.regupathy@intel.com> > 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@c--e.de> > > > > > > Sent: Saturday, August 31, 2024 3:40 AM > > > > > > To: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > > > > Cc: Gopal, Saranya <saranya.gopal@intel.com>; linux- > > > > > > usb@vger.kernel.org; gregkh@linuxfoundation.org; > Regupathy, > > > > > > Rajaram <rajaram.regupathy@intel.com> > > > > > > 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@linux.intel.com> > > > > > > > > Signed-off-by: Saranya Gopal > <saranya.gopal@intel.com> > > > > > > > > > > > > > > Maybe this should be marked as a fix. I think this covers: > > > > > > > https://lore.kernel.org/linux- > usb/20240829100109.562429-2- > > > > > > lk@c--e.de/ > > > > > > > > > > > > > > > > > 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@c--e.de/ > proceeds but is independent. > > > Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > > 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
On Mon, Sep 30, 2024 at 09:41:51AM +0000, Gopal, Saranya wrote: > Hi Greg, > > -----Original Message----- > > From: Christian A. Ehrhardt <lk@c--e.de> > > Sent: Wednesday, September 11, 2024 3:21 AM > > To: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > Cc: Gopal, Saranya <saranya.gopal@intel.com>; linux- > > usb@vger.kernel.org; gregkh@linuxfoundation.org; Regupathy, > > Rajaram <rajaram.regupathy@intel.com> > > 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@c--e.de> > > > > > > > Sent: Saturday, August 31, 2024 3:40 AM > > > > > > > To: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > > > > > Cc: Gopal, Saranya <saranya.gopal@intel.com>; linux- > > > > > > > usb@vger.kernel.org; gregkh@linuxfoundation.org; > > Regupathy, > > > > > > > Rajaram <rajaram.regupathy@intel.com> > > > > > > > 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@linux.intel.com> > > > > > > > > > Signed-off-by: Saranya Gopal > > <saranya.gopal@intel.com> > > > > > > > > > > > > > > > > Maybe this should be marked as a fix. I think this covers: > > > > > > > > https://lore.kernel.org/linux- > > usb/20240829100109.562429-2- > > > > > > > lk@c--e.de/ > > > > > > > > > > > > > > > > > > > > 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@c--e.de/ > > proceeds but is independent. > > > > > Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > > > > 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. It has been mere hours since the merge window closed and I could even consider applying this change. Please relax and understand the ecosystem in which you are working in (also remember the weeks of conferences most of us have been at right now.) To help make this work better, please take the time to review other pending patches for the subsystem, which will ensure that your changes get moved to the top as others get reviewed. I'll wait for you to do that before getting to this one... thanks, greg k-h
Hi Greg, > -----Original Message----- > From: gregkh@linuxfoundation.org <gregkh@linuxfoundation.org> > Sent: Monday, September 30, 2024 3:44 PM > To: Gopal, Saranya <saranya.gopal@intel.com> > Cc: 'Christian A. Ehrhardt' <lk@c--e.de>; Heikki Krogerus > <heikki.krogerus@linux.intel.com>; linux-usb@vger.kernel.org; > Regupathy, Rajaram <rajaram.regupathy@intel.com> > Subject: Re: [PATCH] usb: typec: ucsi: Do not call ACPI _DSM method > for UCSI read operations > > On Mon, Sep 30, 2024 at 09:41:51AM +0000, Gopal, Saranya wrote: > > Hi Greg, > > > -----Original Message----- > > > From: Christian A. Ehrhardt <lk@c--e.de> > > > Sent: Wednesday, September 11, 2024 3:21 AM > > > To: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > Cc: Gopal, Saranya <saranya.gopal@intel.com>; linux- > > > usb@vger.kernel.org; gregkh@linuxfoundation.org; Regupathy, > > > Rajaram <rajaram.regupathy@intel.com> > > > 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@c--e.de> > > > > > > > > Sent: Saturday, August 31, 2024 3:40 AM > > > > > > > > To: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > > > > > > Cc: Gopal, Saranya <saranya.gopal@intel.com>; linux- > > > > > > > > usb@vger.kernel.org; gregkh@linuxfoundation.org; > > > Regupathy, > > > > > > > > Rajaram <rajaram.regupathy@intel.com> > > > > > > > > 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@linux.intel.com> > > > > > > > > > > Signed-off-by: Saranya Gopal > > > <saranya.gopal@intel.com> > > > > > > > > > > > > > > > > > > Maybe this should be marked as a fix. I think this > covers: > > > > > > > > > https://lore.kernel.org/linux- > > > usb/20240829100109.562429-2- > > > > > > > > lk@c--e.de/ > > > > > > > > > > > > > > > > > > > > > > > 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@c-- > e.de/ > > > proceeds but is independent. > > > > > > > Reviewed-by: Heikki Krogerus > <heikki.krogerus@linux.intel.com> > > > > > > > > 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. > > It has been mere hours since the merge window closed and I could > even > consider applying this change. Please relax and understand the > ecosystem in which you are working in (also remember the weeks of > conferences most of us have been at right now.) > Sorry for my previous mail. I realize that the time I sent it wasn't correct. > To help make this work better, please take the time to review other > pending patches for the subsystem, which will ensure that your > changes > get moved to the top as others get reviewed. I'll wait for you to do > that before getting to this one... Sure, let me take the time to review the other pending patches. Thanks, Saranya > > thanks, > > greg k-h
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; + } 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"),