Re: [PATCH v4] usb: typec: tcpci: acknowledge masked alerts and quiesce the IRQ

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] usb: typec: tcpci: race in tcpci_irq() with
  tcpci_quiesce_alerts() skips alert ack
--

commit b4f078603381d8a805defdf53c50acdc29717943
Author: Yaohan Li <shunsheng1218@gmail.com>

usb: typec: tcpci: acknowledge masked alerts and quiesce the IRQ

This commit acknowledges all non-RX_STATUS alert bits before filtering
against the software alert_mask, and quiesces and synchronizes the IRQ
during teardown and probe error cleanup before unregistering the TCPM
port.

> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> index c6165466c54b6..b11c30e67055f 100644
> --- a/drivers/usb/typec/tcpm/tcpci.c
> +++ b/drivers/usb/typec/tcpm/tcpci.c

[ ... ]

> @@ -648,10 +650,21 @@ static bool tcpci_attempt_vconn_swap_discovery(struct tcpc_dev *tcpc)
>  	return false;
>  }
>  
> +static void tcpci_quiesce_alerts(struct tcpci *tcpci)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&tcpci->alert_mask_lock, flags);
> +	tcpci->alerts_quiesced = true;
> +	WRITE_ONCE(tcpci->alert_mask, 0);
> +	spin_unlock_irqrestore(&tcpci->alert_mask_lock, flags);
> +}

[ ... ]

> @@ -710,29 +723,60 @@ static int tcpci_init(struct tcpc_dev *tcpc)

[ ... ]

>  irqreturn_t tcpci_irq(struct tcpci *tcpci)
>  {
> -	u16 status;
> +	u16 status = 0;
>  	int ret;
> -	int irq_ret;
> +	int irq_ret = 0;
>  	unsigned int raw;
>  
> -	tcpci_read16(tcpci, TCPC_ALERT, &status);
> -	irq_ret = status & tcpci->alert_mask;
> +	/*
> +	 * During teardown, acknowledge a latched alert without dispatching
> +	 * it to TCPM.  A level-triggered Alert# must not keep retriggering
> +	 * after the software mask was cleared or the hardware mask write
> +	 * failed; the port may already be going away.
> +	 */
> +	if (READ_ONCE(tcpci->alerts_quiesced)) {
> +		ret = tcpci_read16(tcpci, TCPC_ALERT, &status);
> +		if (ret < 0)
> +			return IRQ_NONE;
> +		if (status) {
> +			ret = tcpci_write16(tcpci, TCPC_ALERT, status);
> +			if (ret < 0)
> +				return IRQ_HANDLED;
> +			irq_ret = 1;
> +		}
> +		return IRQ_RETVAL(irq_ret);
> +	}
> +
> +	if (!READ_ONCE(tcpci->alert_mask))
> +		return IRQ_NONE;
>  
> -process_status:
>  	/*
> -	 * Clear alert status for everything except RX_STATUS, which shouldn't
> -	 * be cleared until we have successfully retrieved message.
> +	 * Acknowledge every non-RX_STATUS bit from the raw alert value, not
> +	 * only the bits enabled in the software mask.  Some controllers
> +	 * enable additional alerts in TCPC_ALERT_MASK (for example RT1711H
> +	 * enables FAULT and RX_BUF_OVF); dropping those bits before the
> +	 * write would leave a level-triggered Alert# asserted.  RX_STATUS is
> +	 * cleared only after the message has been read successfully.
>  	 */
> +	tcpci_read16(tcpci, TCPC_ALERT, &status);
>  	if (status & ~TCPC_ALERT_RX_STATUS)
>  		tcpci_write16(tcpci, TCPC_ALERT,
>  			      status & ~TCPC_ALERT_RX_STATUS);
> +	irq_ret = status;
> +
> +	status &= READ_ONCE(tcpci->alert_mask);
> +
> +process_status:

[ ... ]

> @@ -806,8 +850,13 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
>  		tcpm_pd_transmit_complete(tcpci->port, TCPC_TX_FAILED);
>  
>  	tcpci_read16(tcpci, TCPC_ALERT, &status);
> +	if (status & ~TCPC_ALERT_RX_STATUS)
> +		tcpci_write16(tcpci, TCPC_ALERT,
> +			      status & ~TCPC_ALERT_RX_STATUS);
> +	irq_ret |= status;
>  
> -	if (status & tcpci->alert_mask)
> +	status &= READ_ONCE(tcpci->alert_mask);
> +	if (status)
>  		goto process_status;
>  
>  	return IRQ_RETVAL(irq_ret);

[Severity: Medium]
Can tcpci_irq() race with tcpci_quiesce_alerts() and return without
acknowledging TCPC_ALERT?

CPU0                              CPU1
tcpci_irq()
  READ_ONCE(tcpci->alerts_quiesced)
    -> false
                                  tcpci_remove()
                                    tcpci_quiesce_alerts()
                                      tcpci->alerts_quiesced = true;
                                      WRITE_ONCE(tcpci->alert_mask, 0);
  if (!READ_ONCE(tcpci->alert_mask))
    return IRQ_NONE;              <-- skips TCPC_ALERT read and ack

Because tcpci_irq() reads tcpci->alerts_quiesced and tcpci->alert_mask
in separate unlocked steps, a concurrent tcpci_quiesce_alerts() can zero
tcpci->alert_mask after the quiesced check has already passed.

A second window exists just below and in the loop tail before jumping to
process_status: if tcpci_irq() reads TCPC_ALERT with TCPC_ALERT_RX_STATUS
set, the upfront write clears only status & ~TCPC_ALERT_RX_STATUS, and if
tcpci_quiesce_alerts() zeroes tcpci->alert_mask before
status &= READ_ONCE(tcpci->alert_mask), TCPC_ALERT_RX_STATUS is masked
out and never cleared in the RX handler.  Similarly, if tcpci_init()
fails before publishing tcpci->alert_mask (tcpm_init() ignores the
return value of port->tcpc->init()), tcpci->alert_mask remains 0 and
tcpci_irq() returns IRQ_NONE without acknowledging hardware alerts on
subdrivers such as tcpci_rt1711h that program TCPC_ALERT_MASK
independently.  In all of these cases, a latched alert can keep a
level-triggered Alert# line asserted and cause an interrupt storm or
spurious IRQ disablement.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113084928.211885-1-shunsheng1218@gmail.com?part=1