Re: [PATCH v2] usb: typec: ucsi: recover from silent PPM completion in sync_control

[Greg Kroah-Hartman]

On Wed, Jul 08, 2026 at 10:24:20AM +0800, AceLan Kao wrote:
> From: "Chia-Lin Kao (AceLan)" <acelan.kao@xxxxxxxxxxxxx>
> 
> Some firmware completes UCSI commands and sets COMMAND_COMPLETE (or
> ACK_COMPLETE for ACK_CC_CI) in CCI but never fires the ACPI notify that
> would wake ucsi_sync_control_common().  The driver then times out after
> 5 seconds and returns -ETIMEDOUT, even though the EC finished the command
> successfully.
> 
> Fix this by polling CCI once via poll_cci() on timeout.  If the relevant
> completion bit is already set, the EC finished silently; fall through to
> out_clear_bit so the normal read_cci()/read_message_in() path retrieves
> the data and ucsi_run_command() issues ACK_CC_CI as usual.  Only return
> -ETIMEDOUT when the EC has genuinely not completed the command.
> 
> Guard the poll_cci() call with a NULL check: if a backend does not
> provide the op, skip the poll and keep reporting -ETIMEDOUT rather than
> dereferencing a NULL function pointer.
> 
> Fixes: 584e8df58942 ("usb: typec: ucsi: extract common code for command handling")
> Cc: <stable@xxxxxxxxxxxxxxx> # 6.14+
> Signed-off-by: Chia-Lin Kao (AceLan) <acelan.kao@xxxxxxxxxxxxx>
> 
> ---
> v1 -> v2:
> - Add Cc: <stable@xxxxxxxxxxxxxxx> # 6.14+ as flagged by Greg's patch bot:
>   the Fixes: tag targets a commit already in released kernels, so the fix
>   must be nominated for stable.  Scoped to 6.14+ because poll_cci only
>   exists from that release (absent in 6.11-6.13).
> - Guard the poll_cci() call with a NULL check to avoid dereferencing a
>   NULL function pointer when a backend does not provide the op.



> ---
>  drivers/usb/typec/ucsi/ucsi.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 9eeb38e7472c..deda7123b5d1 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -91,8 +91,29 @@ int ucsi_sync_control_common(struct ucsi *ucsi, u64 command, u32 *cci,
>  	if (ret)
>  		goto out_clear_bit;
>  
> -	if (!wait_for_completion_timeout(&ucsi->complete, 5 * HZ))
> -		ret = -ETIMEDOUT;
> +	if (!wait_for_completion_timeout(&ucsi->complete, 5 * HZ)) {
> +		u32 polled_cci = 0;
> +
> +		/*
> +		 * Notification from EC did not arrive.  Poll once to check
> +		 * whether the PPM actually finished without firing a notify.
> +		 * If poll_cci() is missing or fails, polled_cci stays 0 and we
> +		 * correctly report -ETIMEDOUT below.
> +		 */
> +		if (ucsi->ops->poll_cci)
> +			ucsi->ops->poll_cci(ucsi, &polled_cci);
> +
> +		if ((ack && (polled_cci & UCSI_CCI_ACK_COMPLETE)) ||
> +		    (!ack && (polled_cci & UCSI_CCI_COMMAND_COMPLETE))) {
> +			/*
> +			 * EC completed the command silently.  Proceed to
> +			 * out_clear_bit which reads CCI+data normally, and
> +			 * ucsi_run_command() will issue ACK_CC_CI as usual.
> +			 */
> +		} else {
> +			ret = -ETIMEDOUT;
> +		}

This is an odd way to write an if () statement, why not reverse it so
that there is no need for an else {} at all?

thanks,

greg k-h