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@xxxxxxxxx> > > --- > > 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
