On Thu, Feb 06, 2025 at 09:43:14PM +0300, Fedor Pchelkin wrote:
> From: "Christian A. Ehrhardt" <lk@xxxxxxx>
>
> For the ACPI backend of UCSI the UCSI "registers" are just a memory copy
> of the register values in an opregion. The ACPI implementation in the
> BIOS ensures that the opregion contents are synced to the embedded
> controller and it ensures that the registers (in particular CCI) are
> synced back to the opregion on notifications. While there is an ACPI call
> that syncs the actual registers to the opregion there is rarely a need to
> do this and on some ACPI implementations it actually breaks in various
> interesting ways.
>
> The only reason to force a sync from the embedded controller is to poll
> CCI while notifications are disabled. Only the ucsi core knows if this
> is the case and guessing based on the current command is suboptimal, i.e.
> leading to the following spurious assertion splat:
>
> WARNING: CPU: 3 PID: 76 at drivers/usb/typec/ucsi/ucsi.c:1388 ucsi_reset_ppm+0x1b4/0x1c0 [typec_ucsi]
> CPU: 3 UID: 0 PID: 76 Comm: kworker/3:0 Not tainted 6.12.11-200.fc41.x86_64 #1
> Hardware name: LENOVO 21D0/LNVNB161216, BIOS J6CN45WW 03/17/2023
> Workqueue: events_long ucsi_init_work [typec_ucsi]
> RIP: 0010:ucsi_reset_ppm+0x1b4/0x1c0 [typec_ucsi]
> Call Trace:
> <TASK>
> ucsi_init_work+0x3c/0xac0 [typec_ucsi]
> process_one_work+0x179/0x330
> worker_thread+0x252/0x390
> kthread+0xd2/0x100
> ret_from_fork+0x34/0x50
> ret_from_fork_asm+0x1a/0x30
> </TASK>
>
> Thus introduce a ->poll_cci() method that works like ->read_cci() with an
> additional forced sync and document that this should be used when polling
> with notifications disabled. For all other backends that presumably don't
> have this issue use the same implementation for both methods.
>
> Fixes: fa48d7e81624 ("usb: typec: ucsi: Do not call ACPI _DSM method for UCSI read operations")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Christian A. Ehrhardt <lk@xxxxxxx>
> Tested-by: Fedor Pchelkin <boddah8794@xxxxxxxxx>
> Signed-off-by: Fedor Pchelkin <boddah8794@xxxxxxxxx>
Reviewed-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> ---
> Add the explicit WARNING splat and slightly increase the length of text
> lines in the changelog.
> Original patch: https://lore.kernel.org/linux-usb/Z2Cf1AI8CXao5ZAn@xxxxxxxxxxxxx/
>
> drivers/usb/typec/ucsi/ucsi.c | 10 +++++-----
> drivers/usb/typec/ucsi/ucsi.h | 2 ++
> drivers/usb/typec/ucsi/ucsi_acpi.c | 21 ++++++++++++++-------
> drivers/usb/typec/ucsi/ucsi_ccg.c | 1 +
> drivers/usb/typec/ucsi/ucsi_glink.c | 1 +
> drivers/usb/typec/ucsi/ucsi_stm32g0.c | 1 +
> drivers/usb/typec/ucsi/ucsi_yoga_c630.c | 1 +
> 7 files changed, 25 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index fcf499cc9458..0fe1476f4c29 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -1346,7 +1346,7 @@ static int ucsi_reset_ppm(struct ucsi *ucsi)
>
> mutex_lock(&ucsi->ppm_lock);
>
> - ret = ucsi->ops->read_cci(ucsi, &cci);
> + ret = ucsi->ops->poll_cci(ucsi, &cci);
> if (ret < 0)
> goto out;
>
> @@ -1364,7 +1364,7 @@ static int ucsi_reset_ppm(struct ucsi *ucsi)
>
> tmo = jiffies + msecs_to_jiffies(UCSI_TIMEOUT_MS);
> do {
> - ret = ucsi->ops->read_cci(ucsi, &cci);
> + ret = ucsi->ops->poll_cci(ucsi, &cci);
> if (ret < 0)
> goto out;
> if (cci & UCSI_CCI_COMMAND_COMPLETE)
> @@ -1393,7 +1393,7 @@ static int ucsi_reset_ppm(struct ucsi *ucsi)
> /* Give the PPM time to process a reset before reading CCI */
> msleep(20);
>
> - ret = ucsi->ops->read_cci(ucsi, &cci);
> + ret = ucsi->ops->poll_cci(ucsi, &cci);
> if (ret)
> goto out;
>
> @@ -1929,8 +1929,8 @@ struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops)
> struct ucsi *ucsi;
>
> if (!ops ||
> - !ops->read_version || !ops->read_cci || !ops->read_message_in ||
> - !ops->sync_control || !ops->async_control)
> + !ops->read_version || !ops->read_cci || !ops->poll_cci ||
> + !ops->read_message_in || !ops->sync_control || !ops->async_control)
> return ERR_PTR(-EINVAL);
>
> ucsi = kzalloc(sizeof(*ucsi), GFP_KERNEL);
> diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> index 82735eb34f0e..28780acc4af2 100644
> --- a/drivers/usb/typec/ucsi/ucsi.h
> +++ b/drivers/usb/typec/ucsi/ucsi.h
> @@ -62,6 +62,7 @@ struct dentry;
> * struct ucsi_operations - UCSI I/O operations
> * @read_version: Read implemented UCSI version
> * @read_cci: Read CCI register
> + * @poll_cci: Read CCI register while polling with notifications disabled
> * @read_message_in: Read message data from UCSI
> * @sync_control: Blocking control operation
> * @async_control: Non-blocking control operation
> @@ -76,6 +77,7 @@ struct dentry;
> struct ucsi_operations {
> int (*read_version)(struct ucsi *ucsi, u16 *version);
> int (*read_cci)(struct ucsi *ucsi, u32 *cci);
> + int (*poll_cci)(struct ucsi *ucsi, u32 *cci);
> int (*read_message_in)(struct ucsi *ucsi, void *val, size_t val_len);
> int (*sync_control)(struct ucsi *ucsi, u64 command);
> int (*async_control)(struct ucsi *ucsi, u64 command);
> diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
> index 5c5515551963..ac1ebb5d9527 100644
> --- a/drivers/usb/typec/ucsi/ucsi_acpi.c
> +++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
> @@ -59,19 +59,24 @@ static int ucsi_acpi_read_version(struct ucsi *ucsi, u16 *version)
> static int ucsi_acpi_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_acpi_poll_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;
> +
> + return ucsi_acpi_read_cci(ucsi, 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);
> @@ -94,6 +99,7 @@ static int ucsi_acpi_async_control(struct ucsi *ucsi, u64 command)
> static const struct ucsi_operations ucsi_acpi_ops = {
> .read_version = ucsi_acpi_read_version,
> .read_cci = ucsi_acpi_read_cci,
> + .poll_cci = ucsi_acpi_poll_cci,
> .read_message_in = ucsi_acpi_read_message_in,
> .sync_control = ucsi_sync_control_common,
> .async_control = ucsi_acpi_async_control
> @@ -142,6 +148,7 @@ static int ucsi_gram_sync_control(struct ucsi *ucsi, u64 command)
> static const struct ucsi_operations ucsi_gram_ops = {
> .read_version = ucsi_acpi_read_version,
> .read_cci = ucsi_acpi_read_cci,
> + .poll_cci = ucsi_acpi_poll_cci,
> .read_message_in = ucsi_gram_read_message_in,
> .sync_control = ucsi_gram_sync_control,
> .async_control = ucsi_acpi_async_control
> diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
> index 740171f24ef9..4b1668733a4b 100644
> --- a/drivers/usb/typec/ucsi/ucsi_ccg.c
> +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> @@ -664,6 +664,7 @@ static int ucsi_ccg_sync_control(struct ucsi *ucsi, u64 command)
> static const struct ucsi_operations ucsi_ccg_ops = {
> .read_version = ucsi_ccg_read_version,
> .read_cci = ucsi_ccg_read_cci,
> + .poll_cci = ucsi_ccg_read_cci,
> .read_message_in = ucsi_ccg_read_message_in,
> .sync_control = ucsi_ccg_sync_control,
> .async_control = ucsi_ccg_async_control,
> diff --git a/drivers/usb/typec/ucsi/ucsi_glink.c b/drivers/usb/typec/ucsi/ucsi_glink.c
> index fed39d458090..8af79101a2fc 100644
> --- a/drivers/usb/typec/ucsi/ucsi_glink.c
> +++ b/drivers/usb/typec/ucsi/ucsi_glink.c
> @@ -206,6 +206,7 @@ static void pmic_glink_ucsi_connector_status(struct ucsi_connector *con)
> static const struct ucsi_operations pmic_glink_ucsi_ops = {
> .read_version = pmic_glink_ucsi_read_version,
> .read_cci = pmic_glink_ucsi_read_cci,
> + .poll_cci = pmic_glink_ucsi_read_cci,
> .read_message_in = pmic_glink_ucsi_read_message_in,
> .sync_control = ucsi_sync_control_common,
> .async_control = pmic_glink_ucsi_async_control,
> diff --git a/drivers/usb/typec/ucsi/ucsi_stm32g0.c b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
> index 6923fad31d79..57ef7d83a412 100644
> --- a/drivers/usb/typec/ucsi/ucsi_stm32g0.c
> +++ b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
> @@ -424,6 +424,7 @@ static irqreturn_t ucsi_stm32g0_irq_handler(int irq, void *data)
> static const struct ucsi_operations ucsi_stm32g0_ops = {
> .read_version = ucsi_stm32g0_read_version,
> .read_cci = ucsi_stm32g0_read_cci,
> + .poll_cci = ucsi_stm32g0_read_cci,
> .read_message_in = ucsi_stm32g0_read_message_in,
> .sync_control = ucsi_sync_control_common,
> .async_control = ucsi_stm32g0_async_control,
> diff --git a/drivers/usb/typec/ucsi/ucsi_yoga_c630.c b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
> index 4cae85c0dc12..d33e3f2dd1d8 100644
> --- a/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
> +++ b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
> @@ -74,6 +74,7 @@ static int yoga_c630_ucsi_async_control(struct ucsi *ucsi, u64 command)
> static const struct ucsi_operations yoga_c630_ucsi_ops = {
> .read_version = yoga_c630_ucsi_read_version,
> .read_cci = yoga_c630_ucsi_read_cci,
> + .poll_cci = yoga_c630_ucsi_read_cci,
> .read_message_in = yoga_c630_ucsi_read_message_in,
> .sync_control = ucsi_sync_control_common,
> .async_control = yoga_c630_ucsi_async_control,
> --
> 2.48.1
--
heikki
