[v2,2/3] media: sun4i-csi: disable interrupts when stopping streaming
Commit Message
sun4i_csi_start_streaming() enables the frame-done interrupt in
CSI_INT_EN_REG, but sun4i_csi_stop_streaming() only stops the capture
engine (CSI_CPT_CTRL_REG) via sun4i_csi_capture_stop(). It never disables
the interrupt source nor synchronizes with the handler.
Capture stops at the end of the current frame, so a frame-done interrupt
can still fire shortly after stop_streaming() returns. If userspace then
closes the device, sun4i_csi_release() calls pm_runtime_put() and the CSI
block is powered down (clocks gated, reset asserted). A delayed interrupt
handler would then read/write CSI registers on the gated block, which can
hang or crash the system.
Add a helper that clears CSI_INT_EN_REG, reads it back to flush the posted
write, and calls synchronize_irq(), so no handler can run past that point.
Call it from stop_streaming() before returning the active buffers and
freeing the scratch buffer.
The start_streaming() error path (err_disable_device) has the same
problem: the frame-done interrupt is enabled and capture is started
before the s_stream call that can fail, so a handler can run while the
error path frees the scratch buffer and returns the queued buffers.
Apply the same teardown there. Store the IRQ number in struct sun4i_csi
so it is available in both paths.
Fixes: 577bbf23b758 ("media: sunxi: Add A10 CSI driver")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4
Signed-off-by: Cong Nguyen <congnt264@gmail.com>
---
.../platform/sunxi/sun4i-csi/sun4i_csi.h | 1 +
.../platform/sunxi/sun4i-csi/sun4i_dma.c | 20 +++++++++++++++++++
2 files changed, 21 insertions(+)
Comments
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [Critical] DMA use-after-free and memory corruption due to freeing buffers while capture engine is still running
--
--- Patch [2]: [PATCH v2 2/3] media: sun4i-csi: disable interrupts when stopping streaming ---
commit f908e51d92a92db663d06b38deb5bee451c659e7
Author: Cong Nguyen <congnt264@gmail.com>
media: sun4i-csi: disable interrupts when stopping streaming
This commit addresses a crash where a delayed frame-done interrupt could
access the device after it is powered down. It fixes this by explicitly
disabling the interrupt and waiting for the handler to complete before
proceeding with device shutdown.
Cc: stable@vger.kernel.org
> diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
> index 4781db21c2055..3401a91cb310f 100644
> --- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
> +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
[ ... ]
> @@ -330,6 +347,7 @@ static int sun4i_csi_start_streaming(struct vb2_queue *vq, unsigned int count)
>
> err_disable_device:
> sun4i_csi_capture_stop(csi);
> + sun4i_csi_disable_irq(csi);
>
> err_disable_pipeline:
> video_device_pipeline_stop(&csi->vdev);
[Severity: Critical]
This isn't a bug introduced by this patch, but does the hardware DMA engine
continue writing to memory after sun4i_csi_capture_stop() returns?
As noted in the commit message, capture stops at the end of the current
frame. If the DMA engine is still running, it looks like we might free the
scratch buffer immediately afterwards in the error path:
err_free_scratch_buffer:
dma_free_coherent(csi->dev, csi->scratch.size, csi->scratch.vaddr,
csi->scratch.paddr);
Could this cause memory corruption if the hardware writes to the freed
scratch buffer before it finishes the current frame?
> @@ -355,6 +373,7 @@ static void sun4i_csi_stop_streaming(struct vb2_queue *vq)
>
> v4l2_subdev_call(csi->src_subdev, video, s_stream, 0);
> sun4i_csi_capture_stop(csi);
> + sun4i_csi_disable_irq(csi);
>
> /* Release all active buffers */
> spin_lock_irqsave(&csi->qlock, flags);
[Severity: Critical]
This is a pre-existing issue, but similar to the error path above, are we
returning active buffers to userspace while the DMA engine might still be
actively writing to them?
sun4i_csi_capture_stop(csi);
sun4i_csi_disable_irq(csi);
/* Release all active buffers */
spin_lock_irqsave(&csi->qlock, flags);
return_all_buffers(csi, VB2_BUF_STATE_ERROR);
spin_unlock_irqrestore(&csi->qlock, flags);
video_device_pipeline_stop(&csi->vdev);
dma_free_coherent(csi->dev, csi->scratch.size, csi->scratch.vaddr,
csi->scratch.paddr);
If capture stops asynchronously at the end of the frame, returning these
buffers and freeing the scratch buffer immediately afterwards could allow
the DMA engine to overwrite memory that has already been reclaimed or
reused. Is there a need to poll or wait for the DMA engine to completely
halt before cleaning up?
@@ -112,6 +112,7 @@ struct sun4i_csi {
const struct sun4i_csi_traits *traits;
void __iomem *regs;
+ int irq;
struct clk *bus_clk;
struct clk *isp_clk;
struct clk *ram_clk;
@@ -43,6 +43,23 @@ static void sun4i_csi_capture_stop(struct sun4i_csi *csi)
writel(0, csi->regs + CSI_CPT_CTRL_REG);
}
+static void sun4i_csi_disable_irq(struct sun4i_csi *csi)
+{
+ /*
+ * Disable the frame done interrupt and wait for the handler to
+ * finish. A frame may complete right as capture is stopped, so an
+ * interrupt can still be pending here; without this the handler could
+ * run after the device is powered down (pm_runtime_put() on release)
+ * and access registers on a gated block.
+ *
+ * Read the register back to flush the posted write so the disable has
+ * reached the device before synchronize_irq() waits for the handler.
+ */
+ writel(0, csi->regs + CSI_INT_EN_REG);
+ readl(csi->regs + CSI_INT_EN_REG);
+ synchronize_irq(csi->irq);
+}
+
static int sun4i_csi_queue_setup(struct vb2_queue *vq,
unsigned int *nbuffers,
unsigned int *nplanes,
@@ -328,6 +345,7 @@ static int sun4i_csi_start_streaming(struct vb2_queue *vq, unsigned int count)
err_disable_device:
sun4i_csi_capture_stop(csi);
+ sun4i_csi_disable_irq(csi);
err_disable_pipeline:
video_device_pipeline_stop(&csi->vdev);
@@ -353,6 +371,7 @@ static void sun4i_csi_stop_streaming(struct vb2_queue *vq)
v4l2_subdev_call(csi->src_subdev, video, s_stream, 0);
sun4i_csi_capture_stop(csi);
+ sun4i_csi_disable_irq(csi);
/* Release all active buffers */
spin_lock_irqsave(&csi->qlock, flags);
@@ -438,6 +457,7 @@ int sun4i_csi_dma_register(struct sun4i_csi *csi, int irq)
dev_err(csi->dev, "Couldn't register our interrupt\n");
goto err_unregister_device;
}
+ csi->irq = irq;
return 0;