[PATCH] usb: typec: ucsi: introduce read_explicit operation

[PATCH] usb: typec: ucsi: introduce read_explicit operation

[Samuel Čavoj]

On some ACPI platforms (namely the ASUS Zenbook UM325) the _DSM method must
not be called after a notification is received but instead the mailbox
should be read immediately from RAM. This is because the ACPI interrupt
handler destroys the CCI in ERAM after copying to system memory, and when
_DSM is later called to perform a second copy, it retrieves a garbage
value.

Instead, the _DSM(read) method should only be called when necessary, i.e.
for polling the state after reset and for retrieving the version. Other
reads should not call _DSM and only peek into the RAM region.

For platforms other than ACPI, the read_explicit op uses the same
implementation as read.

Link: https://lore.kernel.org/linux-usb/20210823180626.tb6m7h5tp6adhvt2@fastboi.localdomain/
Signed-off-by: Samuel Čavoj <samuel@cavoj.net>
---
 drivers/usb/typec/ucsi/ucsi.c         |  9 +++++----
 drivers/usb/typec/ucsi/ucsi.h         |  3 +++
 drivers/usb/typec/ucsi/ucsi_acpi.c    | 11 +++++++++++
 drivers/usb/typec/ucsi/ucsi_ccg.c     |  1 +
 drivers/usb/typec/ucsi/ucsi_stm32g0.c |  1 +
 5 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index eabe519013e7..39ee3b63d07d 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -883,7 +883,7 @@ static int ucsi_reset_ppm(struct ucsi *ucsi)
 			goto out;
 		}
 
-		ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci));
+		ret = ucsi->ops->read_explicit(ucsi, UCSI_CCI, &cci, sizeof(cci));
 		if (ret)
 			goto out;
 
@@ -1347,7 +1347,8 @@ struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops)
 {
 	struct ucsi *ucsi;
 
-	if (!ops || !ops->read || !ops->sync_write || !ops->async_write)
+	if (!ops || !ops->read || !ops->read_explicit || !ops->sync_write ||
+	    !ops->async_write)
 		return ERR_PTR(-EINVAL);
 
 	ucsi = kzalloc(sizeof(*ucsi), GFP_KERNEL);
@@ -1382,8 +1383,8 @@ int ucsi_register(struct ucsi *ucsi)
 {
 	int ret;
 
-	ret = ucsi->ops->read(ucsi, UCSI_VERSION, &ucsi->version,
-			      sizeof(ucsi->version));
+	ret = ucsi->ops->read_explicit(ucsi, UCSI_VERSION, &ucsi->version,
+				       sizeof(ucsi->version));
 	if (ret)
 		return ret;
 
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index c968474ee547..8361e1cfc8eb 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -37,6 +37,7 @@ struct ucsi_altmode;
 /**
  * struct ucsi_operations - UCSI I/O operations
  * @read: Read operation
+ * @read_explicit: Read operation with explicit poll if applicable
  * @sync_write: Blocking write operation
  * @async_write: Non-blocking write operation
  * @update_altmodes: Squashes duplicate DP altmodes
@@ -48,6 +49,8 @@ struct ucsi_altmode;
 struct ucsi_operations {
 	int (*read)(struct ucsi *ucsi, unsigned int offset,
 		    void *val, size_t val_len);
+	int (*read_explicit)(struct ucsi *ucsi, unsigned int offset,
+			     void *val, size_t val_len);
 	int (*sync_write)(struct ucsi *ucsi, unsigned int offset,
 			  const void *val, size_t val_len);
 	int (*async_write)(struct ucsi *ucsi, unsigned int offset,
diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
index ce0c8ef80c04..6b3475ed4874 100644
--- a/drivers/usb/typec/ucsi/ucsi_acpi.c
+++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
@@ -45,6 +45,16 @@ static int ucsi_acpi_read(struct ucsi *ucsi, unsigned int offset,
 			  void *val, size_t val_len)
 {
 	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
+
+	memcpy(val, ua->base + offset, val_len);
+
+	return 0;
+}
+
+static int ucsi_acpi_read_explicit(struct ucsi *ucsi, unsigned int offset,
+				   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);
@@ -89,6 +99,7 @@ static int ucsi_acpi_sync_write(struct ucsi *ucsi, unsigned int offset,
 
 static const struct ucsi_operations ucsi_acpi_ops = {
 	.read = ucsi_acpi_read,
+	.read_explicit = ucsi_acpi_read_explicit,
 	.sync_write = ucsi_acpi_sync_write,
 	.async_write = ucsi_acpi_async_write
 };
diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
index 46441f1477f2..d00c262f334f 100644
--- a/drivers/usb/typec/ucsi/ucsi_ccg.c
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -605,6 +605,7 @@ static int ucsi_ccg_sync_write(struct ucsi *ucsi, unsigned int offset,
 
 static const struct ucsi_operations ucsi_ccg_ops = {
 	.read = ucsi_ccg_read,
+	.read_explicit = ucsi_ccg_read,
 	.sync_write = ucsi_ccg_sync_write,
 	.async_write = ucsi_ccg_async_write,
 	.update_altmodes = ucsi_ccg_update_altmodes
diff --git a/drivers/usb/typec/ucsi/ucsi_stm32g0.c b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
index 93fead0096b7..008fa7a3533c 100644
--- a/drivers/usb/typec/ucsi/ucsi_stm32g0.c
+++ b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
@@ -437,6 +437,7 @@ static irqreturn_t ucsi_stm32g0_irq_handler(int irq, void *data)
 
 static const struct ucsi_operations ucsi_stm32g0_ops = {
 	.read = ucsi_stm32g0_read,
+	.read_explicit = ucsi_stm32g0_read,
 	.sync_write = ucsi_stm32g0_sync_write,
 	.async_write = ucsi_stm32g0_async_write,
 };
-- 
2.39.0

Re: [PATCH] usb: typec: ucsi: introduce read_explicit operation

[Greg KH]

On Sat, Jan 21, 2023 at 12:39:21AM +0100, Samuel Čavoj wrote:
> On some ACPI platforms (namely the ASUS Zenbook UM325) the _DSM method must
> not be called after a notification is received but instead the mailbox
> should be read immediately from RAM. This is because the ACPI interrupt
> handler destroys the CCI in ERAM after copying to system memory, and when
> _DSM is later called to perform a second copy, it retrieves a garbage
> value.
> 
> Instead, the _DSM(read) method should only be called when necessary, i.e.
> for polling the state after reset and for retrieving the version. Other
> reads should not call _DSM and only peek into the RAM region.
> 
> For platforms other than ACPI, the read_explicit op uses the same
> implementation as read.
> 
> Link: https://lore.kernel.org/linux-usb/20210823180626.tb6m7h5tp6adhvt2@fastboi.localdomain/
> Signed-off-by: Samuel Čavoj <samuel@cavoj.net>
> ---
>  drivers/usb/typec/ucsi/ucsi.c         |  9 +++++----
>  drivers/usb/typec/ucsi/ucsi.h         |  3 +++
>  drivers/usb/typec/ucsi/ucsi_acpi.c    | 11 +++++++++++
>  drivers/usb/typec/ucsi/ucsi_ccg.c     |  1 +
>  drivers/usb/typec/ucsi/ucsi_stm32g0.c |  1 +
>  5 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index eabe519013e7..39ee3b63d07d 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -883,7 +883,7 @@ static int ucsi_reset_ppm(struct ucsi *ucsi)
>  			goto out;
>  		}
>  
> -		ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci));
> +		ret = ucsi->ops->read_explicit(ucsi, UCSI_CCI, &cci, sizeof(cci));
>  		if (ret)
>  			goto out;
>  
> @@ -1347,7 +1347,8 @@ struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops)
>  {
>  	struct ucsi *ucsi;
>  
> -	if (!ops || !ops->read || !ops->sync_write || !ops->async_write)
> +	if (!ops || !ops->read || !ops->read_explicit || !ops->sync_write ||
> +	    !ops->async_write)
>  		return ERR_PTR(-EINVAL);
>  
>  	ucsi = kzalloc(sizeof(*ucsi), GFP_KERNEL);
> @@ -1382,8 +1383,8 @@ int ucsi_register(struct ucsi *ucsi)
>  {
>  	int ret;
>  
> -	ret = ucsi->ops->read(ucsi, UCSI_VERSION, &ucsi->version,
> -			      sizeof(ucsi->version));
> +	ret = ucsi->ops->read_explicit(ucsi, UCSI_VERSION, &ucsi->version,
> +				       sizeof(ucsi->version));
>  	if (ret)
>  		return ret;
>  
> diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> index c968474ee547..8361e1cfc8eb 100644
> --- a/drivers/usb/typec/ucsi/ucsi.h
> +++ b/drivers/usb/typec/ucsi/ucsi.h
> @@ -37,6 +37,7 @@ struct ucsi_altmode;
>  /**
>   * struct ucsi_operations - UCSI I/O operations
>   * @read: Read operation
> + * @read_explicit: Read operation with explicit poll if applicable

I do not understand what "explicit poll" means here, you are going to
have to make it much more obvious.

But why should this need to be in the usci core?  Shouldn't the
individual driver know what needs to be done here or not?  That's it's
job, you are forcing the usci core to know about specific hardware
problems here, which feels wrong.


>   * @sync_write: Blocking write operation
>   * @async_write: Non-blocking write operation
>   * @update_altmodes: Squashes duplicate DP altmodes
> @@ -48,6 +49,8 @@ struct ucsi_altmode;
>  struct ucsi_operations {
>  	int (*read)(struct ucsi *ucsi, unsigned int offset,
>  		    void *val, size_t val_len);
> +	int (*read_explicit)(struct ucsi *ucsi, unsigned int offset,
> +			     void *val, size_t val_len);
>  	int (*sync_write)(struct ucsi *ucsi, unsigned int offset,
>  			  const void *val, size_t val_len);
>  	int (*async_write)(struct ucsi *ucsi, unsigned int offset,
> diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
> index ce0c8ef80c04..6b3475ed4874 100644
> --- a/drivers/usb/typec/ucsi/ucsi_acpi.c
> +++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
> @@ -45,6 +45,16 @@ static int ucsi_acpi_read(struct ucsi *ucsi, unsigned int offset,
>  			  void *val, size_t val_len)
>  {
>  	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
> +
> +	memcpy(val, ua->base + offset, val_len);

How can you copy directly from ram?  Isn't this i/o memory?  Are you
sure this works on all platforms?

And you just switched the default read to do so, shouldn't you only do
this for the "special" types instead?

thanks,

greg k-h

Re: [PATCH] usb: typec: ucsi: introduce read_explicit operation

[Heikki Krogerus]

Hi,

On Sat, Jan 21, 2023 at 08:17:49AM +0100, Greg KH wrote:
> On Sat, Jan 21, 2023 at 12:39:21AM +0100, Samuel Čavoj wrote:
> > On some ACPI platforms (namely the ASUS Zenbook UM325) the _DSM method must
> > not be called after a notification is received but instead the mailbox
> > should be read immediately from RAM. This is because the ACPI interrupt
> > handler destroys the CCI in ERAM after copying to system memory, and when
> > _DSM is later called to perform a second copy, it retrieves a garbage
> > value.
> > 
> > Instead, the _DSM(read) method should only be called when necessary, i.e.
> > for polling the state after reset and for retrieving the version. Other
> > reads should not call _DSM and only peek into the RAM region.
> > 
> > For platforms other than ACPI, the read_explicit op uses the same
> > implementation as read.
> > 
> > Link: https://lore.kernel.org/linux-usb/20210823180626.tb6m7h5tp6adhvt2@fastboi.localdomain/
> > Signed-off-by: Samuel Čavoj <samuel@cavoj.net>
> > ---
> >  drivers/usb/typec/ucsi/ucsi.c         |  9 +++++----
> >  drivers/usb/typec/ucsi/ucsi.h         |  3 +++
> >  drivers/usb/typec/ucsi/ucsi_acpi.c    | 11 +++++++++++
> >  drivers/usb/typec/ucsi/ucsi_ccg.c     |  1 +
> >  drivers/usb/typec/ucsi/ucsi_stm32g0.c |  1 +
> >  5 files changed, 21 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> > index eabe519013e7..39ee3b63d07d 100644
> > --- a/drivers/usb/typec/ucsi/ucsi.c
> > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > @@ -883,7 +883,7 @@ static int ucsi_reset_ppm(struct ucsi *ucsi)
> >  			goto out;
> >  		}
> >  
> > -		ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci));
> > +		ret = ucsi->ops->read_explicit(ucsi, UCSI_CCI, &cci, sizeof(cci));
> >  		if (ret)
> >  			goto out;
> >  
> > @@ -1347,7 +1347,8 @@ struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops)
> >  {
> >  	struct ucsi *ucsi;
> >  
> > -	if (!ops || !ops->read || !ops->sync_write || !ops->async_write)
> > +	if (!ops || !ops->read || !ops->read_explicit || !ops->sync_write ||
> > +	    !ops->async_write)
> >  		return ERR_PTR(-EINVAL);
> >  
> >  	ucsi = kzalloc(sizeof(*ucsi), GFP_KERNEL);
> > @@ -1382,8 +1383,8 @@ int ucsi_register(struct ucsi *ucsi)
> >  {
> >  	int ret;
> >  
> > -	ret = ucsi->ops->read(ucsi, UCSI_VERSION, &ucsi->version,
> > -			      sizeof(ucsi->version));
> > +	ret = ucsi->ops->read_explicit(ucsi, UCSI_VERSION, &ucsi->version,
> > +				       sizeof(ucsi->version));
> >  	if (ret)
> >  		return ret;
> >  
> > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> > index c968474ee547..8361e1cfc8eb 100644
> > --- a/drivers/usb/typec/ucsi/ucsi.h
> > +++ b/drivers/usb/typec/ucsi/ucsi.h
> > @@ -37,6 +37,7 @@ struct ucsi_altmode;
> >  /**
> >   * struct ucsi_operations - UCSI I/O operations
> >   * @read: Read operation
> > + * @read_explicit: Read operation with explicit poll if applicable
> 
> I do not understand what "explicit poll" means here, you are going to
> have to make it much more obvious.
> 
> But why should this need to be in the usci core?  Shouldn't the
> individual driver know what needs to be done here or not?  That's it's
> job, you are forcing the usci core to know about specific hardware
> problems here, which feels wrong.
> 
> 
> >   * @sync_write: Blocking write operation
> >   * @async_write: Non-blocking write operation
> >   * @update_altmodes: Squashes duplicate DP altmodes
> > @@ -48,6 +49,8 @@ struct ucsi_altmode;
> >  struct ucsi_operations {
> >  	int (*read)(struct ucsi *ucsi, unsigned int offset,
> >  		    void *val, size_t val_len);
> > +	int (*read_explicit)(struct ucsi *ucsi, unsigned int offset,
> > +			     void *val, size_t val_len);
> >  	int (*sync_write)(struct ucsi *ucsi, unsigned int offset,
> >  			  const void *val, size_t val_len);
> >  	int (*async_write)(struct ucsi *ucsi, unsigned int offset,
> > diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
> > index ce0c8ef80c04..6b3475ed4874 100644
> > --- a/drivers/usb/typec/ucsi/ucsi_acpi.c
> > +++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
> > @@ -45,6 +45,16 @@ static int ucsi_acpi_read(struct ucsi *ucsi, unsigned int offset,
> >  			  void *val, size_t val_len)
> >  {
> >  	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
> > +
> > +	memcpy(val, ua->base + offset, val_len);
> 
> How can you copy directly from ram?  Isn't this i/o memory?  Are you
> sure this works on all platforms?
> And you just switched the default read to do so, shouldn't you only do
> this for the "special" types instead?

It's not i/o memory, it's just a mailbox in ram - it's mapped with
memremap() in this driver.

I asked that Samuel to share this here, but perhaps we should consider
it still as an RFC. I have tested this with some of my platforms, but
I want to test more.

I would also like to see if it's possible to take care of this problem
in ucsi_acpi.c so we don't have to touch the ucsi core.

thanks,

-- 
heikki

Re: [PATCH] usb: typec: ucsi: introduce read_explicit operation

[Heikki Krogerus]

Hi Samuel,

> I asked that Samuel to share this here, but perhaps we should consider
> it still as an RFC. I have tested this with some of my platforms, but
> I want to test more.

Sorry about the slow progress, but this is causing commands to fail on
some platforms. I've been trying to figure out how to fix those, but
nothing has worked so far.

I think we have to handle this with a DMI quick in ucsi_acpi.c. I
don't have any other ideas. I'll try a few more things, and then
prepare a patch for that.

thanks,

-- 
heikki

Re: [PATCH] usb: typec: ucsi: introduce read_explicit operation

[Heikki Krogerus]

[-- Attachment #1: Type: text/plain, Size: 929 bytes --]

On Wed, Mar 08, 2023 at 06:17:47PM +0200, Heikki Krogerus wrote:
> Hi Samuel,
> 
> > I asked that Samuel to share this here, but perhaps we should consider
> > it still as an RFC. I have tested this with some of my platforms, but
> > I want to test more.
> 
> Sorry about the slow progress, but this is causing commands to fail on
> some platforms. I've been trying to figure out how to fix those, but
> nothing has worked so far.
> 
> I think we have to handle this with a DMI quick in ucsi_acpi.c. I
> don't have any other ideas. I'll try a few more things, and then
> prepare a patch for that.

Unfortunately nothing seems to work... I'm attaching the DMI quirk
patch here. Can you test it?

I'm not sure if the DMI_PRODUCT_NAME is just "ZenBook" so you may
need to fix that in the patch!!

You can get the correct value by running dmidecode. This should work:

        % dmidecode -s system-product-name

thanks,

-- 
heikki

[-- Attachment #2: 0001-usb-typec-ucsi-acpi-Quirk-for-ZenBook.patch --]
[-- Type: text/plain, Size: 2913 bytes --]

From 6111911b5cd9da86679b0942f5df01b7c1516cae Mon Sep 17 00:00:00 2001
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Date: Thu, 16 Mar 2023 14:59:30 +0200
Subject: [PATCH] usb: typec: ucsi: acpi: Quirk for ZenBook

Interim. Still WIP.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/ucsi/ucsi_acpi.c | 43 +++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
index 62206a6b8ea75..873e64f48b477 100644
--- a/drivers/usb/typec/ucsi/ucsi_acpi.c
+++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
@@ -9,6 +9,7 @@
 #include <linux/platform_device.h>
 #include <linux/module.h>
 #include <linux/acpi.h>
+#include <linux/dmi.h>
 
 #include "ucsi.h"
 
@@ -23,6 +24,7 @@ struct ucsi_acpi {
 	struct completion complete;
 	unsigned long flags;
 	guid_t guid;
+	u64 cmd;
 };
 
 static int ucsi_acpi_dsm(struct ucsi_acpi *ua, int func)
@@ -62,6 +64,7 @@ static int ucsi_acpi_async_write(struct ucsi *ucsi, unsigned int offset,
 	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
 
 	memcpy(ua->base + offset, val, val_len);
+	ua->cmd = *(u64*)val;
 
 	return ucsi_acpi_dsm(ua, UCSI_DSM_FUNC_WRITE);
 }
@@ -93,6 +96,40 @@ static const struct ucsi_operations ucsi_acpi_ops = {
 	.async_write = ucsi_acpi_async_write
 };
 
+static int
+ucsi_zenbook_read(struct ucsi *ucsi, unsigned int offset, void *val, size_t val_len)
+{
+	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
+	int ret;
+
+	if (ua->cmd & (UCSI_VERSION | UCSI_PPM_RESET)) {
+		ret = ucsi_acpi_dsm(ua, UCSI_DSM_FUNC_READ);
+		if (ret)
+			return ret;
+	}
+
+	memcpy(val, ua->base + offset, val_len);
+	ua->cmd = 0;
+
+	return 0;
+}
+
+static const struct ucsi_operations ucsi_zenbook_ops = {
+	.read = ucsi_zenbook_read,
+	.sync_write = ucsi_acpi_sync_write,
+	.async_write = ucsi_acpi_async_write
+};
+
+static const struct dmi_system_id zenbook_dmi_id[] = {
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK Computer Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "ZenBook"),
+		},
+	},
+	{ }
+};
+
 static void ucsi_acpi_notify(acpi_handle handle, u32 event, void *data)
 {
 	struct ucsi_acpi *ua = data;
@@ -114,6 +151,7 @@ static void ucsi_acpi_notify(acpi_handle handle, u32 event, void *data)
 static int ucsi_acpi_probe(struct platform_device *pdev)
 {
 	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+	const struct ucsi_operations *ops = &ucsi_acpi_ops;
 	struct ucsi_acpi *ua;
 	struct resource *res;
 	acpi_status status;
@@ -143,7 +181,10 @@ static int ucsi_acpi_probe(struct platform_device *pdev)
 	init_completion(&ua->complete);
 	ua->dev = &pdev->dev;
 
-	ua->ucsi = ucsi_create(&pdev->dev, &ucsi_acpi_ops);
+	if (dmi_check_system(zenbook_dmi_id))
+		ops = &ucsi_zenbook_ops;
+
+	ua->ucsi = ucsi_create(&pdev->dev, ops);
 	if (IS_ERR(ua->ucsi))
 		return PTR_ERR(ua->ucsi);
 
-- 
2.39.2

Re: [PATCH] usb: typec: ucsi: introduce read_explicit operation

[Samuel Čavoj]

Hi,

On 2023-03-16 14:08, Heikki Krogerus wrote:
> On Wed, Mar 08, 2023 at 06:17:47PM +0200, Heikki Krogerus wrote:
>> Hi Samuel,
>> 
>> > I asked that Samuel to share this here, but perhaps we should consider
>> > it still as an RFC. I have tested this with some of my platforms, but
>> > I want to test more.
>> 
>> Sorry about the slow progress, but this is causing commands to fail on
>> some platforms. I've been trying to figure out how to fix those, but
>> nothing has worked so far.

No worries, I've been very busy with other stuff as well, that's
why I haven't responded for some time--nothing new on my end.

>> I think we have to handle this with a DMI quick in ucsi_acpi.c. I
>> don't have any other ideas. I'll try a few more things, and then
>> prepare a patch for that.
> 
> Unfortunately nothing seems to work... I'm attaching the DMI quirk
> patch here. Can you test it?

I'll definitely try it out, I hope sometime next week!

> I'm not sure if the DMI_PRODUCT_NAME is just "ZenBook" so you may
> need to fix that in the patch!!
> 
> You can get the correct value by running dmidecode. This should work:
> 
>         % dmidecode -s system-product-name
> 
> thanks,

Thanks
Sam

Re: [PATCH] usb: typec: ucsi: introduce read_explicit operation

[Samuel Čavoj]

Hi Heikki,

On 2023-03-18 03:04, Samuel Čavoj wrote:
...
>> Unfortunately nothing seems to work... I'm attaching the DMI quirk
>> patch here. Can you test it?
> 
> I'll definitely try it out, I hope sometime next week!
> 
>> I'm not sure if the DMI_PRODUCT_NAME is just "ZenBook" so you may
>> need to fix that in the patch!!
>> 
>> You can get the correct value by running dmidecode. This should work:
>> 
>>         % dmidecode -s system-product-name

This returns "ZenBook UX325UA_UM325UA", so the DMI_MATCH would work.
However my DMI_SYS_VENDOR is "ASUSTeK COMPUTER INC.", uppercase.

All in all, the patch works after some modifications which I'm
attaching below. In summary:

  - The DMI_MATCH SYS_VENDOR was changed to uppercase
  - The DMI_MATCH PRODUCT_NAME was changed to be more specific, although
    I'm not sure what the best value is here.
  - The conditional in ucsi_zenbook_read was fixed.
  - ua->cmd cannot be reset to 0 after read because the reset
    procedure repeatedly calls read without performing
    any other commands. I don't think this should cause any problems
  - ucsi_acpi_notify needs to call the quirked variant
    as well, so I put an indirect call there.

Otherwise maybe ucsi_acpi_async_write should only set cmd if the offset
is UCSI_CONTROL.

I'm occasionally getting some other errors later on, but I think
those might be specific to a certain cheap USB-C hub I have. They
don't occur with it unplugged.

Thanks,
Sam

diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c 
b/drivers/usb/typec/ucsi/ucsi_acpi.c
index a5cb4b89573f..3b872cb3808b 100644
--- a/drivers/usb/typec/ucsi/ucsi_acpi.c
+++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
@@ -102,14 +102,13 @@ ucsi_zenbook_read(struct ucsi *ucsi, unsigned int 
offset, void *val, size_t val_
  	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
  	int ret;

-	if (ua->cmd & (UCSI_VERSION | UCSI_PPM_RESET)) {
+	if (offset == UCSI_VERSION || UCSI_COMMAND(ua->cmd) == UCSI_PPM_RESET) 
{
  		ret = ucsi_acpi_dsm(ua, UCSI_DSM_FUNC_READ);
  		if (ret)
  			return ret;
  	}

  	memcpy(val, ua->base + offset, val_len);
-	ua->cmd = 0;

  	return 0;
  }
@@ -123,8 +122,8 @@ static const struct ucsi_operations ucsi_zenbook_ops 
= {
  static const struct dmi_system_id zenbook_dmi_id[] = {
  	{
  		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK Computer Inc."),
-			DMI_MATCH(DMI_PRODUCT_NAME, "ZenBook"),
+			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "ZenBook UX325UA_UM325UA"),
  		},
  	},
  	{ }
@@ -136,7 +135,7 @@ static void ucsi_acpi_notify(acpi_handle handle, u32 
event, void *data)
  	u32 cci;
  	int ret;

-	ret = ucsi_acpi_read(ua->ucsi, UCSI_CCI, &cci, sizeof(cci));
+	ret = ua->ucsi->ops->read(ua->ucsi, UCSI_CCI, &cci, sizeof(cci));
  	if (ret)
  		return;

-- 
2.40.0

Re: [PATCH] usb: typec: ucsi: introduce read_explicit operation

[Heikki Krogerus]

Hi Sam,

On Thu, Mar 30, 2023 at 01:48:18AM +0200, Samuel Čavoj wrote:
> Hi Heikki,
> 
> On 2023-03-18 03:04, Samuel Čavoj wrote:
> ...
> > > Unfortunately nothing seems to work... I'm attaching the DMI quirk
> > > patch here. Can you test it?
> > 
> > I'll definitely try it out, I hope sometime next week!
> > 
> > > I'm not sure if the DMI_PRODUCT_NAME is just "ZenBook" so you may
> > > need to fix that in the patch!!
> > > 
> > > You can get the correct value by running dmidecode. This should work:
> > > 
> > >         % dmidecode -s system-product-name
> 
> This returns "ZenBook UX325UA_UM325UA", so the DMI_MATCH would work.
> However my DMI_SYS_VENDOR is "ASUSTeK COMPUTER INC.", uppercase.
> 
> All in all, the patch works after some modifications which I'm
> attaching below. In summary:
> 
>  - The DMI_MATCH SYS_VENDOR was changed to uppercase
>  - The DMI_MATCH PRODUCT_NAME was changed to be more specific, although
>    I'm not sure what the best value is here.
>  - The conditional in ucsi_zenbook_read was fixed.
>  - ua->cmd cannot be reset to 0 after read because the reset
>    procedure repeatedly calls read without performing
>    any other commands. I don't think this should cause any problems
>  - ucsi_acpi_notify needs to call the quirked variant
>    as well, so I put an indirect call there.
> 
> Otherwise maybe ucsi_acpi_async_write should only set cmd if the offset
> is UCSI_CONTROL.

Thanks!

> I'm occasionally getting some other errors later on, but I think
> those might be specific to a certain cheap USB-C hub I have. They
> don't occur with it unplugged.

Okay... Did you see those errors with your original patch?

Br,

-- 
heikki

Re: [PATCH] usb: typec: ucsi: introduce read_explicit operation

[Samuel Čavoj]

> 
> Okay... Did you see those errors with your original patch?

I'm pretty sure that it's the same, yeah. The specific error is
one (or a seemingly random sequence) of the following:

- con2: failed to register partner alt modes (-22)
- con2: failed to register partner alt modes (-5)
- GET_CURRENT_CAM command failed (also caused by a -22 from 
exec_command)

Doesn't occur with nothing or only a charger plugged in. Once I plug
a USB-C to DP adapter or a cheap USB-C hub (with an internal DP->HDMI
converter, USB3 hub and GbE in one of the hub ports), the errors
randomly show up when reloading the module or when plugging in once
already loaded. Not consistent at all.

So seems to be alt-mode related. Will probably need some more
investigation on my part, unless you've got any ideas off the
bat.

Sam

Re: [PATCH] usb: typec: ucsi: introduce read_explicit operation

[Heikki Krogerus]

Hi Sam,

On Sat, Apr 01, 2023 at 08:06:57PM +0200, Samuel Čavoj wrote:
> > 
> > Okay... Did you see those errors with your original patch?
> 
> I'm pretty sure that it's the same, yeah. The specific error is
> one (or a seemingly random sequence) of the following:
> 
> - con2: failed to register partner alt modes (-22)
> - con2: failed to register partner alt modes (-5)
> - GET_CURRENT_CAM command failed (also caused by a -22 from exec_command)
> 
> Doesn't occur with nothing or only a charger plugged in. Once I plug
> a USB-C to DP adapter or a cheap USB-C hub (with an internal DP->HDMI
> converter, USB3 hub and GbE in one of the hub ports), the errors
> randomly show up when reloading the module or when plugging in once
> already loaded. Not consistent at all.
> 
> So seems to be alt-mode related. Will probably need some more
> investigation on my part, unless you've got any ideas off the
> bat.

The alt mode stuff is very annoying with UCSI. I think Windows is only
interested in the connector alt modes. With the partner alt modes the
responses differ on almost every system, and several platforms
actually never return anything when you request the partner alt modes
with GET_ALTERNATE_MODES.

But I think we can move forward with this fix. I'll send it tomorrow.

Br,

-- 
heikki