[PATCH] i2c: stm32f7: synchronize DMA termination before releasing the buffer

[PATCH] i2c: stm32f7: synchronize DMA termination before releasing the buffer

[Samarth Narang]

stm32f7_i2c_dma_callback() is used both as the DMA completion callback
and as a cleanup helper by the threaded IRQ handlers for bus errors,
arbitration loss, PEC errors, NACKs, and DMA completion timeouts.

The helper calls dmaengine_terminate_async() and then immediately unmaps
the DMA buffer and releases the DMA-safe message buffer through
i2c_put_dma_safe_msg_buf().

On the error paths, DMA completion has not been established.
dmaengine_terminate_async() only requests termination; it does not
guarantee that the transfer and its callback have finished when the call
returns. The DMAengine API requires synchronization before memory
accessible to the descriptor is released.

Although the driver calls dmaengine_synchronize() from
stm32f7_i2c_xfer_core() and stm32f7_i2c_smbus_xfer(), these calls occur
after the IRQ thread has already unmapped and released the buffer. They
therefore cannot establish retirement before that release.

If the DMA operation retains access after cleanup, a late RX write could
modify freed or reused memory, while a late TX read could access data
belonging to a subsequent owner.

Introduce stm32f7_i2c_dma_abort() for the threaded IRQ error paths. The
helper disables further peripheral DMA requests and calls
dmaengine_terminate_sync() before invoking the common buffer cleanup.
Since these paths execute in threaded IRQ context, synchronous
termination is permitted.

Remove the redundant termination request from the genuine DMA completion
callback, where the descriptor has already completed.

This establishes the intended ordering between DMA termination and
buffer release under the DMAengine synchronous-termination contract.

Fixes: 6aae87fe7f18 ("i2c: stm32f7: unmap DMA mapped buffer")
Fixes: ffdd20555cc6 ("i2c: stm32f7: support i2c_*_dma_safe_msg_buf APIs")
Assisted-by: LLM
Signed-off-by: Samarth Narang <samarth.colleges@gmail.com>
---
The issue was identified using a research static analyzer for DMA
lifecycle ordering and subsequently reviewed against the driver source.
It has not been reproduced on hardware. The patch has been
compile-tested on arm64 with W=1.

The practical exposure with the in-tree DMA providers may be narrower
than the API-level ordering violation suggests. stm32-dma clears the
stream enable bit and polls for it to clear, while stm32-dma3 suspends
and resets the channel. However, neither behavior alone establishes that
a concurrent completion callback has finished. The stm32-dma polling
operation can also time out without propagating a failure through
device_terminate_all().

Consequently, this change primarily corrects the driver's reliance on an
asynchronous termination request as a final-retirement guarantee. A
physical late write would require an outstanding access to survive the
provider's shutdown behavior; that has not been demonstrated.

Remaining concurrency issue: this patch does not resolve a separate,
pre-existing race between genuine DMA completion and threaded IRQ error
handling. If both paths invoke stm32f7_i2c_dma_callback() for the same
transfer, they may both attempt to unmap and release the same buffer.
Establishing a single owner for cleanup would require an additional
change.

 drivers/i2c/busses/i2c-stm32f7.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c
index 6bd281a9e..891b0eebe 100644
--- a/drivers/i2c/busses/i2c-stm32f7.c
+++ b/drivers/i2c/busses/i2c-stm32f7.c
@@ -736,7 +736,6 @@ static void stm32f7_i2c_dma_callback(void *arg)
 	struct stm32f7_i2c_msg *f7_msg = &i2c_dev->f7_msg;
 
 	stm32f7_i2c_disable_dma_req(i2c_dev);
-	dmaengine_terminate_async(dma->chan_using);
 	dma_unmap_single(i2c_dev->dev, dma->dma_buf, dma->dma_len,
 			 dma->dma_data_dir);
 	if (!f7_msg->smbus)
@@ -744,6 +743,16 @@ static void stm32f7_i2c_dma_callback(void *arg)
 	complete(&dma->dma_complete);
 }
 
+/* Threaded IRQ context only: terminate_sync() may sleep. */
+static void stm32f7_i2c_dma_abort(struct stm32f7_i2c_dev *i2c_dev)
+{
+	struct stm32_i2c_dma *dma = i2c_dev->dma;
+
+	stm32f7_i2c_disable_dma_req(i2c_dev);
+	dmaengine_terminate_sync(dma->chan_using);
+	stm32f7_i2c_dma_callback(i2c_dev);
+}
+
 static void stm32f7_i2c_hw_config(struct stm32f7_i2c_dev *i2c_dev)
 {
 	struct stm32f7_i2c_timings *t = &i2c_dev->timing;
@@ -1552,7 +1561,7 @@ static irqreturn_t stm32f7_i2c_handle_isr_errs(struct stm32f7_i2c_dev *i2c_dev,
 
 	/* Disable dma */
 	if (i2c_dev->use_dma)
-		stm32f7_i2c_dma_callback(i2c_dev);
+		stm32f7_i2c_dma_abort(i2c_dev);
 
 	i2c_dev->master_mode = false;
 	complete(&i2c_dev->complete);
@@ -1617,7 +1626,7 @@ static irqreturn_t stm32f7_i2c_isr_event_thread(int irq, void *data)
 			__func__, f7_msg->addr);
 		writel_relaxed(STM32F7_I2C_ICR_NACKCF, base + STM32F7_I2C_ICR);
 		if (i2c_dev->use_dma)
-			stm32f7_i2c_dma_callback(i2c_dev);
+			stm32f7_i2c_dma_abort(i2c_dev);
 		f7_msg->result = -ENXIO;
 	}
 
@@ -1635,7 +1644,7 @@ static irqreturn_t stm32f7_i2c_isr_event_thread(int irq, void *data)
 			ret = wait_for_completion_timeout(&i2c_dev->dma->dma_complete, HZ);
 			if (!ret) {
 				dev_dbg(i2c_dev->dev, "<%s>: Timed out\n", __func__);
-				stm32f7_i2c_dma_callback(i2c_dev);
+				stm32f7_i2c_dma_abort(i2c_dev);
 				f7_msg->result = -ETIMEDOUT;
 			}
 		}

base-commit: 83f32c773255720a661aa45d83e36278135f23f5
-- 
2.34.1