Re: [PATCH v3] usb: typec: tcpci: filter masked alerts and quiesce the IRQ

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] usb: typec: tcpci: masking status before clearing TCPC_ALERT
  causes IRQ storm
- [Medium] usb: typec: tcpci: zeroing alert_mask before hardware mask
  write causes IRQ storm
--

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

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

This commit filters TCPC_ALERT status bits with tcpci->alert_mask in
tcpci_irq() and adds an alerts_quiesced state and synchronize_irq() calls
to quiesce interrupts before unregistering the TCPM port on removal and
probe failure.

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

[ ... ]

> @@ -722,8 +738,12 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
>  	int irq_ret;
>  	unsigned int raw;
>  
> +	if (!READ_ONCE(tcpci->alert_mask))
> +		return IRQ_NONE;
> +
>  	tcpci_read16(tcpci, TCPC_ALERT, &status);
> -	irq_ret = status & tcpci->alert_mask;
> +	status &= READ_ONCE(tcpci->alert_mask);
> +	irq_ret = status;
>  
>  process_status:
>  	/*

[Severity: High]
Can masking status with tcpci->alert_mask before writing TCPC_ALERT leave
hardware-enabled alerts latched and cause a level-triggered IRQ storm?

tcpci_init() never sets TCPC_ALERT_RX_BUF_OVF, TCPC_ALERT_FAULT or
TCPC_ALERT_VBUS_DISCNCT in tcpci->alert_mask, but vendor drivers that
delegate interrupt handling to tcpci_irq() unmask those bits in hardware:

drivers/usb/typec/tcpm/tcpci_rt1711h.c:rt1711h_probe() {
    ...
	const u16 alert_mask = TCPC_ALERT_TX_SUCCESS | TCPC_ALERT_TX_DISCARDED |
			       TCPC_ALERT_TX_FAILED | TCPC_ALERT_RX_HARD_RST |
			       TCPC_ALERT_RX_STATUS | TCPC_ALERT_POWER_STATUS |
			       TCPC_ALERT_CC_STATUS | TCPC_ALERT_RX_BUF_OVF |
			       TCPC_ALERT_FAULT;
    ...
	ret = rt1711h_write16(chip, TCPC_ALERT_MASK, alert_mask);
}

Previously, tcpci_write16(tcpci, TCPC_ALERT, status & ~TCPC_ALERT_RX_STATUS)
cleared any latched bit read from TCPC_ALERT.  With status masked by
tcpci->alert_mask first (and again after the re-read at the bottom of
tcpci_irq()), TCPC_ALERT_FAULT or TCPC_ALERT_RX_BUF_OVF is never written
back to TCPC_ALERT, so the active-low Alert# line stays asserted while
tcpci_irq() repeatedly returns IRQ_NONE until the spurious IRQ detector
disables the interrupt line.

[ ... ]

> @@ -988,10 +1013,19 @@ static void tcpci_remove(struct i2c_client *client)
>  	struct tcpci_chip *chip = i2c_get_clientdata(client);
>  	int err;
>  
> -	/* Disable chip interrupts before unregistering port */
> +	/*
> +	 * The port's kthread worker is destroyed by tcpci_unregister_port()
> +	 * below, while the threaded IRQ handler can still queue work on it.
> +	 * Publish a zero software mask first: tcpci_irq() then ignores any
> +	 * alert that is still latched, or asserted again on a shared IRQ,
> +	 * even if the hardware mask write fails.  synchronize_irq() waits
> +	 * for handlers that already read the old mask.
> +	 */
> +	tcpci_quiesce_alerts(chip->tcpci);
>  	err = tcpci_write16(chip->tcpci, TCPC_ALERT_MASK, 0);
>  	if (err < 0)
>  		dev_warn(&client->dev, "Failed to disable irqs (%pe)\n", ERR_PTR(err));
> +	synchronize_irq(client->irq);
>  
>  	tcpci_unregister_port(chip->tcpci);
>  }

[Severity: Medium]
Can zeroing tcpci->alert_mask before masking TCPC_ALERT_MASK in hardware
trigger an unhandled IRQ storm if an alert fires during the I2C write?

CPU0                              irq/client->irq (SCHED_FIFO)
tcpci_remove()
  tcpci_quiesce_alerts()
    tcpci->alert_mask = 0
  tcpci_write16(..., TCPC_ALERT_MASK, 0)
    <blocks on I2C transfer>
                                  <Alert# asserted low>
                                  _tcpci_irq()
                                    tcpci_irq()
                                      if (!READ_ONCE(tcpci->alert_mask))
                                        return IRQ_NONE;
                                  irq_finalize_oneshot()
                                    <unmasks level-low IRQ line>
                                  <Alert# re-fires immediately>  <-- IRQ storm

While tcpci_write16() sleeps waiting for the I2C transfer, hardware
interrupts are still enabled in TCPC_ALERT_MASK.  Because tcpci_irq() now
returns IRQ_NONE immediately without sleeping on I2C or clearing
TCPC_ALERT, irq_finalize_oneshot() unmasks the level-triggered line with
Alert# still asserted low, spinning in the real-time IRQ thread until the
I2C write finishes or the spurious IRQ detector disables the shared IRQ.
Should TCPC_ALERT_MASK be cleared in hardware before calling
tcpci_quiesce_alerts()?

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