[PATCH AUTOSEL 6.19-6.12] i3c: mipi-i3c-hci: Ensure proper bus clean-up

[Sasha Levin <sashal@kernel.org>]

From: Adrian Hunter <adrian.hunter@intel.com>

[ Upstream commit 8bb96575883d3b201ce37046b3903ea1d2d50bbc ]

Wait for the bus to fully disable before proceeding, ensuring that no
operations are still in progress.  Synchronize the IRQ handler only after
interrupt signals have been disabled.  This approach also handles cases
where bus disable might fail, preventing race conditions and ensuring a
consistent shutdown sequence.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Link: https://patch.msgid.link/20260113072702.16268-3-adrian.hunter@intel.com
Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

Good. `readx_poll_timeout` has been used in this file since its
introduction. The new `i3c_hci_bus_disable()` function uses the same
pattern. No compatibility issue.

---

## Detailed Analysis

### 1. COMMIT MESSAGE ANALYSIS

The commit message clearly states the intent: "ensuring that no
operations are still in progress", "Synchronize the IRQ handler only
after interrupt signals have been disabled", "preventing race conditions
and ensuring a consistent shutdown sequence." This is unambiguously a
race condition and shutdown ordering fix.

**Author**: Adrian Hunter (Intel), a prolific and trusted kernel
developer.
**Reviewer**: Frank Li (NXP), an I3C subsystem reviewer.
**Maintainer**: Merged by Alexandre Belloni, the I3C subsystem
maintainer.

### 2. CODE CHANGE ANALYSIS - The Race Conditions

**Bug 1: Bus not fully disabled before cleanup**

The OLD `i3c_hci_bus_cleanup()` does:

```155:165:/home/sasha/linux-
autosel/drivers/i3c/master/mipi-i3c-hci/core.c
static void i3c_hci_bus_cleanup(struct i3c_master_controller *m)
{
        struct i3c_hci *hci = to_i3c_hci(m);
        struct platform_device *pdev =
to_platform_device(m->dev.parent);

        reg_clear(HC_CONTROL, HC_CONTROL_BUS_ENABLE);
        synchronize_irq(platform_get_irq(pdev, 0));
        hci->io->cleanup(hci);
        if (hci->cmd == &mipi_i3c_hci_cmd_v1)
                mipi_i3c_hci_dat_v1.cleanup(hci);
}
```

`reg_clear(HC_CONTROL, HC_CONTROL_BUS_ENABLE)` merely writes to the
register. The hardware may still be mid-operation. Without waiting for
hardware acknowledgment, the subsequent cleanup proceeds on a bus that
is still active, which can cause undefined hardware behavior.

The new `i3c_hci_bus_disable()` uses `readx_poll_timeout()` with a
generous 500ms timeout to wait until the `HC_CONTROL_BUS_ENABLE` bit is
confirmed cleared by the hardware, ensuring the bus has truly stopped.

**Bug 2: Use-after-free race in DMA cleanup path**

This is the critical bug. The old flow is:

1. `reg_clear(HC_CONTROL, HC_CONTROL_BUS_ENABLE)` - request bus disable
2. `synchronize_irq()` - waits for any currently-running IRQ handler to
   finish
3. `hci_dma_cleanup()` starts - iterates rings, disabling per-ring
   interrupt signals and freeing DMA buffers in the **same loop**

The race: `synchronize_irq()` only guarantees the *currently running*
handler has finished. It does **not** prevent new interrupts from
arriving. Since the top-level `INTR_SIGNAL_ENABLE` register is still
armed (it's only cleared inside `io->cleanup()`), the hardware can
deliver a new interrupt *after* `synchronize_irq()` returns.

If that happens, `i3c_hci_irq_handler()` runs, calls
`hci_dma_irq_handler()`, which accesses `rings->headers[i]` (including
`rh->ibi_status`, `rh->resp`, `rh->xfer`) - DMA memory that
`hci_dma_cleanup()` may be concurrently freeing with
`dma_free_coherent()`. This is a **use-after-free**.

The fix splits the DMA cleanup loop: first loop disables all ring
interrupt signals and ring control, then calls
`i3c_hci_sync_irq_inactive()` (which disables top-level interrupt
signals AND synchronize_irq), and only then the second loop frees DMA
resources.

**Bug 3: Same race in PIO cleanup path**

The `hci_pio_irq_handler()` accesses `pio->lock`, `pio->enabled_irqs`,
and other `hci_pio_data` fields. If an interrupt fires after
`synchronize_irq()` but before `hci_pio_cleanup()` disables PIO signals
and frees `pio`, the handler will access freed memory.

The fix inserts `i3c_hci_sync_irq_inactive()` after disabling PIO
interrupt signals and before freeing the `pio` structure.

### 3. CLASSIFICATION

This is a **race condition fix** preventing **use-after-free** during
driver cleanup/shutdown. It falls squarely in the category of fixes that
should be backported to stable.

### 4. SCOPE AND RISK ASSESSMENT

- **Files changed**: 4 files, all within the same driver
- **Lines added**: ~40 lines of new code (two helper functions + loop
  restructuring)
- **Lines removed**: ~5 lines
- **Complexity**: Moderate but well-contained
- **Risk**: LOW - changes only affect the cleanup/shutdown path, using
  established kernel patterns (`readx_poll_timeout`, `synchronize_irq`)
- **Subsystem**: I3C bus driver used by Intel and AMD hardware (active,
  maintained)

### 5. USER IMPACT

- Affects users with MIPI I3C HCI hardware (Intel, AMD platforms via
  ACPI)
- The bug could trigger during driver removal (module unload), system
  shutdown, or suspend/resume cycles
- Consequence if triggered: kernel crash, memory corruption, or
  undefined behavior from use-after-free
- The AMD ACPI entry `AMDI5017` shows this driver is used on real
  production hardware

### 6. STABILITY INDICATORS

- **Reviewed-by**: Frank Li (NXP) - I3C expert
- **Merged by**: Alexandre Belloni - I3C maintainer
- Already in mainline (v6.19)

### 7. DEPENDENCY CHECK

The commit is **self-contained**:
- New functions `i3c_hci_bus_disable()` and
  `i3c_hci_sync_irq_inactive()` are introduced and used only within this
  commit
- Uses `readx_poll_timeout()` and `synchronize_irq()` which exist in all
  stable kernels
- `<linux/iopoll.h>` is already included in `core.c`
- No dependency on other patches from the series (the other patches in
  the series appear to be PCI-related, not core driver changes)

### 8. BACKPORT CONSIDERATIONS

For older stable trees that lack `9e23897bca622` ("Use physical device
pointer with DMA API"), the `dma_free_coherent` calls in
`hci_dma_cleanup` use `&hci->master.dev` instead of `rings->sysdev`.
However, this is in the *unchanged* part of the code (the second loop),
so it should apply cleanly or with minimal context adjustment.

### Final Assessment

This commit fixes a real race condition that can cause use-after-free
during driver shutdown/cleanup in the MIPI I3C HCI driver. The bug
mechanism is clear: interrupt handlers can fire after
`synchronize_irq()` returns because interrupt signal enable registers
weren't disabled first, leading to concurrent access of memory being
freed. The fix is well-structured, properly orders the shutdown sequence
(disable signals -> synchronize IRQ -> free resources), and is self-
contained with low regression risk.

**YES**

 drivers/i3c/master/mipi-i3c-hci/core.c | 32 +++++++++++++++++++++++---
 drivers/i3c/master/mipi-i3c-hci/dma.c  |  7 ++++++
 drivers/i3c/master/mipi-i3c-hci/hci.h  |  1 +
 drivers/i3c/master/mipi-i3c-hci/pio.c  |  2 ++
 4 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
index 607d77ab0e546..0a4d8c9968c9b 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -152,13 +152,39 @@ static int i3c_hci_bus_init(struct i3c_master_controller *m)
 	return 0;
 }
 
+/* Bus disable should never fail, so be generous with the timeout */
+#define BUS_DISABLE_TIMEOUT_US (500 * USEC_PER_MSEC)
+
+static int i3c_hci_bus_disable(struct i3c_hci *hci)
+{
+	u32 regval;
+	int ret;
+
+	reg_clear(HC_CONTROL, HC_CONTROL_BUS_ENABLE);
+
+	/* Ensure controller is disabled */
+	ret = readx_poll_timeout(reg_read, HC_CONTROL, regval,
+				 !(regval & HC_CONTROL_BUS_ENABLE), 0, BUS_DISABLE_TIMEOUT_US);
+	if (ret)
+		dev_err(&hci->master.dev, "%s: Failed to disable bus\n", __func__);
+
+	return ret;
+}
+
+void i3c_hci_sync_irq_inactive(struct i3c_hci *hci)
+{
+	struct platform_device *pdev = to_platform_device(hci->master.dev.parent);
+	int irq = platform_get_irq(pdev, 0);
+
+	reg_write(INTR_SIGNAL_ENABLE, 0x0);
+	synchronize_irq(irq);
+}
+
 static void i3c_hci_bus_cleanup(struct i3c_master_controller *m)
 {
 	struct i3c_hci *hci = to_i3c_hci(m);
-	struct platform_device *pdev = to_platform_device(m->dev.parent);
 
-	reg_clear(HC_CONTROL, HC_CONTROL_BUS_ENABLE);
-	synchronize_irq(platform_get_irq(pdev, 0));
+	i3c_hci_bus_disable(hci);
 	hci->io->cleanup(hci);
 	if (hci->cmd == &mipi_i3c_hci_cmd_v1)
 		mipi_i3c_hci_dat_v1.cleanup(hci);
diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
index 951abfea5a6fd..7061c44243424 100644
--- a/drivers/i3c/master/mipi-i3c-hci/dma.c
+++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
@@ -162,6 +162,13 @@ static void hci_dma_cleanup(struct i3c_hci *hci)
 
 		rh_reg_write(INTR_SIGNAL_ENABLE, 0);
 		rh_reg_write(RING_CONTROL, 0);
+	}
+
+	i3c_hci_sync_irq_inactive(hci);
+
+	for (i = 0; i < rings->total; i++) {
+		rh = &rings->headers[i];
+
 		rh_reg_write(CR_SETUP, 0);
 		rh_reg_write(IBI_SETUP, 0);
 
diff --git a/drivers/i3c/master/mipi-i3c-hci/hci.h b/drivers/i3c/master/mipi-i3c-hci/hci.h
index 249ccb13c9092..5add9c68434bf 100644
--- a/drivers/i3c/master/mipi-i3c-hci/hci.h
+++ b/drivers/i3c/master/mipi-i3c-hci/hci.h
@@ -147,5 +147,6 @@ void mipi_i3c_hci_pio_reset(struct i3c_hci *hci);
 void mipi_i3c_hci_dct_index_reset(struct i3c_hci *hci);
 void amd_set_od_pp_timing(struct i3c_hci *hci);
 void amd_set_resp_buf_thld(struct i3c_hci *hci);
+void i3c_hci_sync_irq_inactive(struct i3c_hci *hci);
 
 #endif
diff --git a/drivers/i3c/master/mipi-i3c-hci/pio.c b/drivers/i3c/master/mipi-i3c-hci/pio.c
index 710faa46a00fa..9bf6c3ba6bce9 100644
--- a/drivers/i3c/master/mipi-i3c-hci/pio.c
+++ b/drivers/i3c/master/mipi-i3c-hci/pio.c
@@ -212,6 +212,8 @@ static void hci_pio_cleanup(struct i3c_hci *hci)
 
 	pio_reg_write(INTR_SIGNAL_ENABLE, 0x0);
 
+	i3c_hci_sync_irq_inactive(hci);
+
 	if (pio) {
 		dev_dbg(&hci->master.dev, "status = %#x/%#x",
 			pio_reg_read(INTR_STATUS), pio_reg_read(INTR_SIGNAL_ENABLE));
-- 
2.51.0