Re: [PATCH] usb: typec: ucsi: Stop a power level change notification storm

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usb: typec: ucsi: workaround breaks power level notifications globally
- [High] usb: typec: ucsi: state desync on ucsi->ntfy causes infinite interrupt storm
--

commit 77aa902efb14b4a8b70e32dbfbe471a17454a00e
Author: David Vornholt <david@vornholt.online>
Subject: usb: typec: ucsi: Stop a power level change notification storm

This patch attempts to work around a bug on Lenovo ThinkPad X1 Carbon Gen 13
where the controller continuously signals a negotiated power level change
without an actual contract change. It stops the resulting interrupt storm by
disabling the power level change notification globally if it triggers 10 times
in a row without a new contract.

> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index c1450639c7956..f2677875ad1e8 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
[ ... ]
> +static void ucsi_pwr_level_change(struct ucsi_connector *con)
> +{
> +	struct ucsi *ucsi = con->ucsi;
> +	u64 ntfy;
[ ... ]
> +	ntfy = ucsi->ntfy & ~UCSI_ENABLE_NTFY_PWR_LEVEL_CHANGE;
> +	if (ucsi_send_command(ucsi, UCSI_SET_NOTIFICATION_ENABLE | ntfy, NULL, 0) < 0)
> +		return;
> +
> +	ucsi->ntfy = ntfy;

[Severity: High]
Does this disable power level change notifications globally for all ports on
the system?

The notification command configures the mask for the entire controller,
affecting all connectors.

If this flag is cleared, will legitimate power level changes on other ports
(like a charger reducing power capability due to dynamic sharing) also be
missed?

Additionally, it appears this notification is never re-enabled even after
the buggy partner disconnects.

[Severity: High]
Is there a race condition here that can lead to an infinite interrupt storm?

This code performs a read-modify-write on ucsi->ntfy and sends a hardware
command without holding a lock (like ppm_lock) to serialize against other
readers.

If a concurrent path like ucsi_role_cmd() reads the old ucsi->ntfy:

    c = UCSI_SET_NOTIFICATION_ENABLE | con->ucsi->ntfy;
    ucsi_send_command(con->ucsi, c, NULL, 0);

and writes it back after ucsi_pwr_level_change() has updated the mask to
disabled, the hardware will have notifications enabled again.

However, the software check earlier in ucsi_pwr_level_change() will ignore
subsequent events:

    if (!(ucsi->ntfy & UCSI_ENABLE_NTFY_PWR_LEVEL_CHANGE))
        return;

Will this result in an infinite interrupt storm that permanently ties up the
processor with hardware transactions, since the controller keeps asserting
the interrupt but software ignores it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261010121026.84844-1-dv@etik.com?part=1