| Message ID | 20230120233920.752245-1-samuel@cavoj.net |
|---|---|
| State | New |
| Headers | show |
| Series | usb: typec: ucsi: introduce read_explicit operation | expand |
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,
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,
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,
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
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;
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,
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,
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, };
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(-)