[PATCH v2] usb: typec: tcpci: quiesce the IRQ before unregistering the TCPM port

[PATCH v2] usb: typec: tcpci: quiesce the IRQ before unregistering the TCPM port

[Yaohan Li]

tcpci_remove() unregisters the TCPM port before the managed threaded
IRQ has been released.  tcpci_unregister_port() destroys the TCPM
kthread worker, while tcpci_irq() can still call the TCPM event entry
points (tcpm_cc_change() and friends), which queue work on port->wq
without checking whether the port is still registered.  Writing
TCPC_ALERT_MASK = 0 prevents new chip alerts, but it does not
synchronize with a threaded handler that was already woken or running.

Quiesce the handler before unregistering the port:

- Set chip->stopping before synchronize_irq(), so handlers that already
  entered tcpci_irq() are waited for, while handlers that start later
  return IRQ_NONE from _tcpci_irq() without touching the TCPM port.
- The IRQ remains devm-managed and is released on driver detach, as
  suggested by Xu Yang; no custom early free_irq is needed.
- synchronize_irq() alone would not be enough because the IRQ is shared:
  another device on the same line can trigger the handler after
  synchronize_irq() returns; the stopping flag makes such late calls
  harmless.

The same ordering issue exists on the probe error path: if the final
TCPC_ALERT_MASK enable write fails after devm_request_threaded_irq()
succeeded, the error path unregisters the port before devres frees the
IRQ.  Route that failure through a quiesce_irq label that sets stopping
and synchronizes the IRQ before unregistering.  If the IRQ request
itself failed, the error goes directly to unregister_port.

This issue was investigated and the fix was developed with AI
assistance (ZCode/GLM-4).

Testing with QEMU, a simulated I2C TCPCI device and KASAN (7.4-rc1):

- Remove path with a real threaded handler parked at
  kthread_queue_work() while the device is unbound: synchronize_irq()
  waits for the handler before tcpci_unregister_port() destroys the
  worker; no report.  Without synchronization the worker is freed first
  and KASAN reports a slab-use-after-free in kthread_queue_work().
- Probe failure path (the final ALERT_MASK enable write is forced to
  fail): the handler woken by the injected alert is rejected by the
  stopping flag, and the teardown reports nothing.
- A shared IRQ raised at tcpci_unregister_port() after
  synchronize_irq() returns enters _tcpci_irq(), returns IRQ_NONE due
  to chip->stopping, and never reaches tcpm_cc_change(); no report.
- tcpci_remove() with the ALERT_MASK = 0 write forced to fail (the
  driver only warns) while a continuous alert stream is active: the
  teardown completes, no tcpm_* entry point is reached after the TCPM
  worker is destroyed, and no report appears.

These are injection-assisted reproductions; they do not measure how
often the race occurs on physical hardware.

Fixes: ae8a2ca8a2215 ("usb: typec: Group all TCPCI/TCPM code together")
Signed-off-by: Yaohan Li <shunsheng1218@gmail.com>

---
Changes since v1:
- Follow Xu Yang's suggestion: keep the IRQ devm-managed and use
  synchronize_irq() instead of an explicit devm_free_irq().
- Add chip->stopping and check it in _tcpci_irq(), because the IRQ is
  shared and a handler can run again after synchronize_irq() returns.
- Quiesce the IRQ on the probe error path after the final ALERT_MASK
  write fails; an IRQ request failure still goes directly to
  unregister_port.
- Revalidate the remove path, probe failure and a new shared-IRQ
  trigger after synchronize_irq().
v1: https://lore.kernel.org/r/179378589227.74015.8259496684787732118@gmail.com/
---
 drivers/usb/typec/tcpm/tcpci.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
index 6717ac914c6a6..2a3ed5195abd7 100644
--- a/drivers/usb/typec/tcpm/tcpci.c
+++ b/drivers/usb/typec/tcpm/tcpci.c
@@ -50,6 +50,7 @@ struct tcpci {
 struct tcpci_chip {
 	struct tcpci *tcpci;
 	struct tcpci_data data;
+	bool stopping;
 };
 
 struct tcpm_port *tcpci_get_tcpm_port(struct tcpci *tcpci)
@@ -818,6 +819,9 @@ static irqreturn_t _tcpci_irq(int irq, void *dev_id)
 {
 	struct tcpci_chip *chip = dev_id;
 
+	if (READ_ONCE(chip->stopping))
+		return IRQ_NONE;
+
 	return tcpci_irq(chip->tcpci);
 }
 
@@ -972,12 +976,15 @@ static int tcpci_probe(struct i2c_client *client)
 	/* Enable chip interrupts at last */
 	err = tcpci_write16(chip->tcpci, TCPC_ALERT_MASK, chip->tcpci->alert_mask);
 	if (err < 0)
-		goto unregister_port;
+		goto quiesce_irq;
 
 	device_set_wakeup_capable(chip->tcpci->dev, true);
 
 	return 0;
 
+quiesce_irq:
+	WRITE_ONCE(chip->stopping, true);
+	synchronize_irq(client->irq);
 unregister_port:
 	tcpci_unregister_port(chip->tcpci);
 	return err;
@@ -993,6 +1000,18 @@ static void tcpci_remove(struct i2c_client *client)
 	if (err < 0)
 		dev_warn(&client->dev, "Failed to disable irqs (%pe)\n", ERR_PTR(err));
 
+	/*
+	 * The port's kthread worker is freed by tcpci_unregister_port()
+	 * below, and the threaded IRQ handler queues work on it without
+	 * any port-registered check.  Publish the stop state and wait for
+	 * handlers that already entered tcpci_irq() before returning;
+	 * handlers that start afterwards return IRQ_NONE without touching
+	 * the port.  The devm-managed IRQ is released as usual on driver
+	 * detach.
+	 */
+	WRITE_ONCE(chip->stopping, true);
+	synchronize_irq(client->irq);
+
 	tcpci_unregister_port(chip->tcpci);
 }
 
-- 
2.34.1

Re: [PATCH v2] usb: typec: tcpci: quiesce the IRQ before unregistering the TCPM port

[Xu Yang]

On Fri, Nov 06, 2026 at 05:17:25PM +0800, Yaohan Li wrote:
> tcpci_remove() unregisters the TCPM port before the managed threaded
> IRQ has been released.  tcpci_unregister_port() destroys the TCPM
> kthread worker, while tcpci_irq() can still call the TCPM event entry
> points (tcpm_cc_change() and friends), which queue work on port->wq
> without checking whether the port is still registered.  Writing
> TCPC_ALERT_MASK = 0 prevents new chip alerts, but it does not
> synchronize with a threaded handler that was already woken or running.
> 
> Quiesce the handler before unregistering the port:
> 
> - Set chip->stopping before synchronize_irq(), so handlers that already
>   entered tcpci_irq() are waited for, while handlers that start later
>   return IRQ_NONE from _tcpci_irq() without touching the TCPM port.
> - The IRQ remains devm-managed and is released on driver detach, as
>   suggested by Xu Yang; no custom early free_irq is needed.
> - synchronize_irq() alone would not be enough because the IRQ is shared:
>   another device on the same line can trigger the handler after
>   synchronize_irq() returns; the stopping flag makes such late calls
>   harmless.

It's unnecessary to add chip->stopping to return IRQ_NONE, because the
ALERT is already masked at this point. In shared IRQ case, this device's
irq handler may be called if another device trigger it, but it will do
nothing (status will be 0) and return IRQ_NONE at the end.

> 
> The same ordering issue exists on the probe error path: if the final
> TCPC_ALERT_MASK enable write fails after devm_request_threaded_irq()
> succeeded, the error path unregisters the port before devres frees the
> IRQ.  Route that failure through a quiesce_irq label that sets stopping
> and synchronizes the IRQ before unregistering.  If the IRQ request
> itself failed, the error goes directly to unregister_port.

It's unnecessary as well. Firstly, TCPC_ALERT_MASK is set to 0 before it
request threded irq. If it fails to set TCPC_ALERT_MASK, it may follow
above behavior if this device's irq handler is called in shared IRQ case.

> 
> This issue was investigated and the fix was developed with AI
> assistance (ZCode/GLM-4).
> 
> Testing with QEMU, a simulated I2C TCPCI device and KASAN (7.4-rc1):
> 
> - Remove path with a real threaded handler parked at
>   kthread_queue_work() while the device is unbound: synchronize_irq()
>   waits for the handler before tcpci_unregister_port() destroys the
>   worker; no report.  Without synchronization the worker is freed first
>   and KASAN reports a slab-use-after-free in kthread_queue_work().
> - Probe failure path (the final ALERT_MASK enable write is forced to
>   fail): the handler woken by the injected alert is rejected by the
>   stopping flag, and the teardown reports nothing.
> - A shared IRQ raised at tcpci_unregister_port() after
>   synchronize_irq() returns enters _tcpci_irq(), returns IRQ_NONE due
>   to chip->stopping, and never reaches tcpm_cc_change(); no report.
> - tcpci_remove() with the ALERT_MASK = 0 write forced to fail (the
>   driver only warns) while a continuous alert stream is active: the
>   teardown completes, no tcpm_* entry point is reached after the TCPM
>   worker is destroyed, and no report appears.
> 
> These are injection-assisted reproductions; they do not measure how
> often the race occurs on physical hardware.

Above words should not go to commit message.

Thanks,
Xu Yang

Re: [PATCH v2] usb: typec: tcpci: quiesce the IRQ before unregistering the TCPM port

[Yaohan Li]

Hi Xu,

Thanks for taking another look. I'll shorten the commit message.

I checked the IRQ path again. tcpci_irq() reads TCPC_ALERT and handles the status bits directly. The alert_mask field is used for the IRQ return value and the loop condition, but it doesn't filter those initial TCPM callbacks.

My concern is the case where an alert is still latched, or the mask write fails during removal. Since the IRQ is shared, another device could cause _tcpci_irq() to run after synchronize_irq() has returned. Without the stopping check, that handler could still reach TCPM after the port has been unregistered.

Does the TCPCI specification guarantee that TCPC_ALERT reads as zero once its mask is cleared, including for previously latched alerts? If so, I've missed that guarantee and can simplify the patch. Otherwise, I think the stopping check is still needed for that last window.

Thanks,
Yaohan