[RFC PATCH] i2c: mediatek: stop APDMA before releasing DMA buffers on timeout

[RFC PATCH] i2c: mediatek: stop APDMA before releasing DMA buffers on timeout

[Samarth Narang]

mtk_i2c_do_transfer() unmaps the DMA buffers and releases the DMA-safe
message buffers through i2c_put_dma_safe_msg_buf() immediately after the
completion wait, before checking whether the transfer completed
successfully.

If the wait times out, APDMA reset occurs only later, through
mtk_i2c_init_hw(), with i2c_dump_register() running between buffer
release and the reset.

A completion timeout does not establish that APDMA has stopped accessing
memory. For example, a target may hold SCL low until the adapter times
out and then release it. If APDMA resumes the remaining RX transfer
before it has been stopped, it could write into memory whose DMA mapping
has already been released and whose bounce buffer may have been freed or
reused. With SWIOTLB, a released bounce slot could also be reassigned to
another mapping.

Commit aafced673c06 ("i2c: mediatek: move dma reset before i2c reset")
previously addressed APDMA accesses to DRAM after the transfer buffer
was freed by changing the reset order in mtk_i2c_init_hw(). However, the
reset still occurs after mtk_i2c_do_transfer() has released the DMA
buffers.

Move APDMA shutdown ahead of buffer teardown on timeout and ACK-error
paths. Request an APDMA warm reset and wait for the enable bit to clear
before unmapping and releasing the buffers.

If APDMA cannot be confirmed to have stopped, retain the mappings and
buffers rather than potentially returning memory that the controller can
still access. This intentionally sacrifices memory reclamation on the
failed-stop path to avoid unsafe reuse.

The change is conditional on the APDMA reset and status operations
providing the necessary hardware-retirement guarantee.

Fixes: ce38815d39ea ("I2C: mediatek: Add driver for MediaTek I2C controller")
Assisted-by: LLM
Signed-off-by: Samarth Narang <samarth.colleges@gmail.com>
---
This issue was identified through static analysis and model checking and
the patch has been compile-tested with arm64 defconfig and the driver
enabled, using W=1, without new warnings. It has not been tested on
hardware.

I am submitting this as an RFC because the hardware-retirement guarantee
remains uncertain. I would appreciate clarification on the following
points:

1. APDMA warm-reset semantics. Does writing I2C_DMA_WARM_RST to
   OFFSET_RST abort an outstanding transfer, and does observing
   I2C_DMA_START_EN clear in OFFSET_EN guarantee that APDMA will issue
   no further memory writes, including writes already accepted by the
   interconnect?

2. SoC-specific synchronization. The multi-message apdma_sync path polls
   OFFSET_RST until I2C_DMA_WARM_RST clears and additionally sets
   I2C_DMA_HANDSHAKE_RST. Should the timeout path use the same sequence
   on SoCs requiring APDMA synchronization, rather than relying solely
   on the enable-bit status?

3. Reset ordering. Must the I2C controller be reset before or together
   with APDMA to ensure that no further peripheral requests can reach
   it? Commit 05f6f7271a38 ("i2c: mediatek: Fix apdma and i2c hand-shake
   timeout") appears relevant to this ordering.

4. ACK-error handling. An ACK error may occur before the RX data phase,
   in which case no outstanding RX writes are expected. The proposed
   patch also stops APDMA on this path as a precaution. Is that
   appropriate for the supported controllers?

The DMA-safe message buffers were introduced later by fc66b39fe36a
("i2c: mediatek: Use DMA safe buffers for i2c transactions"). Before
that change, the timeout path could unmap the caller's buffer before the
APDMA reset.

The intent of this RFC is to establish the required ordering between
APDMA retirement and DMA-buffer release. I would appreciate guidance on
the correct hardware shutdown sequence before treating the proposed fix
as complete.

 drivers/i2c/busses/i2c-mt65xx.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
index 307925fb7..2a8b8bd1d 100644
--- a/drivers/i2c/busses/i2c-mt65xx.c
+++ b/drivers/i2c/busses/i2c-mt65xx.c
@@ -993,6 +993,19 @@ static void i2c_dump_register(struct mtk_i2c *i2c)
 		readl(i2c->pdmabase + OFFSET_RX_4G_MODE));
 }
 
+static int mtk_i2c_stop_dma(struct mtk_i2c *i2c)
+{
+	u32 reg_dma_en;
+	int ret;
+
+	writel(I2C_DMA_WARM_RST, i2c->pdmabase + OFFSET_RST);
+	ret = readl_poll_timeout(i2c->pdmabase + OFFSET_EN, reg_dma_en,
+				 !(reg_dma_en & I2C_DMA_START_EN), 1, 1000);
+	writel(I2C_DMA_CLR_FLAG, i2c->pdmabase + OFFSET_RST);
+
+	return ret;
+}
+
 static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, struct i2c_msg *msgs,
 			       int num, int left_num)
 {
@@ -1201,6 +1214,15 @@ static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, struct i2c_msg *msgs,
 	mtk_i2c_writew(i2c, ~(restart_flag | I2C_HS_NACKERR | I2C_ACKERR |
 			    I2C_ARB_LOST | I2C_TRANSAC_COMP), OFFSET_INTR_MASK);
 
+	/* Stop the APDMA before unmap; leak the buffers if it won't stop. */
+	if (ret == 0 || (i2c->irq_stat & (I2C_HS_NACKERR | I2C_ACKERR))) {
+		if (mtk_i2c_stop_dma(i2c)) {
+			dev_err(i2c->dev, "APDMA stop timeout, leaking buffers\n");
+			mtk_i2c_init_hw(i2c);
+			return -ETIMEDOUT;
+		}
+	}
+
 	if (i2c->op == I2C_MASTER_WR) {
 		dma_unmap_single(i2c->dev, wpaddr,
 				 msgs->len, DMA_TO_DEVICE);

base-commit: 83f32c773255720a661aa45d83e36278135f23f5
-- 
2.34.1