[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