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? thanks, greg k-h
