[v4,1/5] dmaengine: sun6i-dma: Refactor to support A733 interrupt and register handling
Commit Message
The `sun6i-dma` driver currently uses hardcoded logic for interrupt
handling (reading/writing interrupt enable and status registers) and
register dumping.
To support the Allwinner A733, which has a different register layout and
interrupt handling logic, add `irq_stride`, `irq_en_offset`, and
`irq_stat_offset` to `struct sun6i_dma_config`. Implement generic
`sun6i_read/write_irq_*` functions using these new configuration fields
to accommodate different hardware revisions.
Signed-off-by: Yuanshen Cao <alex.caoys@gmail.com>
---
drivers/dma/sun6i-dma.c | 87 +++++++++++++++++++++++++++++++++++++------------
1 file changed, 67 insertions(+), 20 deletions(-)
Comments
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Preexisting logic errors in `sun6i_dma_interrupt` regarding outer loop integer division truncation and inner loop physical channel indexing lead to dropped interrupts, unhandled interrupt storms, and use-after-free conditions for upper channels (>= 8).
- [High] Deadlock risk due to acquiring `sdev->lock` without disabling softirqs in process context.
- [High] Lockless read of `vchan->phy` causes TOCTOU race that can pause or stop unrelated DMA channels.
- [High] Data race on IRQ_EN register during driver remove can cause an unhandled IRQ storm.
--
commit c54e18d9ab8b9baf10207d467edd0a2daf494d43
Author: Yuanshen Cao <alex.caoys@gmail.com>
dmaengine: sun6i-dma: Refactor to support A733 interrupt and register handling
The `sun6i-dma` driver currently uses hardcoded logic for interrupt
handling (reading/writing interrupt enable and status registers) and
register dumping.
To support the Allwinner A733, which has a different register layout and
interrupt handling logic, add `irq_stride`, `irq_en_offset`, and
`irq_stat_offset` to `struct sun6i_dma_config`. Implement generic
`sun6i_read/write_irq_*` functions using these new configuration fields
to accommodate different hardware revisions.
> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index f47a326dd7ffa..f305fbfb35453 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
[ ... ]
> @@ -549,14 +581,14 @@ static irqreturn_t sun6i_dma_interrupt(int irq, void *dev_id)
> u32 status;
>
> for (i = 0; i < sdev->num_pchans / DMA_IRQ_CHAN_NR; i++) {
> - status = readl(sdev->base + DMA_IRQ_STAT(i));
> + status = sun6i_read_irq_stat(sdev, i);
> if (!status)
> continue;
>
> dev_dbg(sdev->slave.dev, "DMA irq status %s: 0x%x\n",
> str_high_low(i), status);
>
> - writel(status, sdev->base + DMA_IRQ_STAT(i));
> + sun6i_write_irq_stat(sdev, i, status);
>
> for (j = 0; (j < DMA_IRQ_CHAN_NR) && status; j++) {
> pchan = sdev->pchans + j;
[Severity: Critical]
This is a pre-existing issue, but does this outer loop truncation and inner
loop physical channel indexing lead to dropped interrupts and unhandled
interrupt storms?
For the outer loop, integer division truncates the number of loops for SoCs
with non-multiple-of-8 channel counts (e.g., H3 with 12 channels). This
ignores interrupts for channels 8-11, causing unhandled hardware interrupts
and CPU hangs.
For the inner loop, does the array offset use only `j` without `i`? For
channels in higher registers (like channel 8, where `i=1`), this incorrectly
attributes the interrupt to channels 0-7 instead of
`i * num_channels_per_reg + j`.
This causes the DMA framework to prematurely complete active transfers on the
wrong channels, leading to use-after-free conditions, data corruption, and
dropped interrupts for the true channel.
Note that for the newly supported A733 SoC, which uses
`num_channels_per_reg = 1`, all interrupts for channels 1-15 will incorrectly
map to channel 0, rendering the DMA controller completely non-functional.
[ ... ]
> @@ -1072,9 +1104,11 @@ static struct dma_chan *sun6i_dma_of_xlate(struct of_phandle_args *dma_spec,
>
> static inline void sun6i_kill_tasklet(struct sun6i_dma_dev *sdev)
> {
> + int i;
> +
> /* Disable all interrupts from DMA */
> - writel(0, sdev->base + DMA_IRQ_EN(0));
> - writel(0, sdev->base + DMA_IRQ_EN(1));
> + for (i = 0; i < 2; i++)
> + sun6i_write_irq_en(sdev, i, 0);
[Severity: High]
This is a pre-existing issue, but can this unlocked write to the IRQ_EN
register race with the tasklet's read-modify-write and leave interrupts
enabled?
During driver unload, sun6i_kill_tasklet() writes 0 to the IRQ_EN register
to disable hardware interrupts without any lock. Concurrently, if a channel
is still completing, sun6i_dma_tasklet() can run on another CPU, calling
sun6i_dma_start_desc() which performs an unprotected read-modify-write on
the same IRQ_EN register.
The tasklet can overwrite the 0 written by sun6i_kill_tasklet() with a
non-zero interrupt enable mask. Because the hardware interrupt is left
enabled but the IRQ handler is immediately freed by devm_free_irq(), any
subsequent DMA interrupt assertion will trigger an unhandled IRQ storm,
permanently hanging the CPU.
[ ... ]
Additionally, there are a few other pre-existing issues in this file:
[Severity: High]
This is a pre-existing issue, but does acquiring sdev->lock without
disabling softirqs in process context create a deadlock risk?
Functions like sun6i_dma_terminate_all() and sun6i_dma_pause() can be
called from process context, acquiring sdev->lock using plain spin_lock():
drivers/dma/sun6i-dma.c:sun6i_dma_terminate_all() {
...
spin_lock(&sdev->lock);
list_del_init(&vchan->node);
spin_unlock(&sdev->lock);
...
}
Concurrently, sun6i_dma_tasklet() executes in softirq context and acquires
the same lock using spin_lock_irq(&sdev->lock). If a hardware interrupt
occurs on the same CPU while the process context holds sdev->lock, the
returning interrupt will trigger do_softirq().
The sun6i_dma_tasklet() will then execute, attempting to acquire sdev->lock
and spinning forever, causing a CPU deadlock.
[Severity: High]
This is a pre-existing issue, but does a lockless read of vchan->phy cause
a TOCTOU race that can pause or stop unrelated DMA channels?
In sun6i_dma_pause(), sun6i_dma_terminate_all(), sun6i_dma_resume(), and
sun6i_dma_tx_status(), vchan->phy is read into a local pchan variable
before any locks are acquired:
drivers/dma/sun6i-dma.c:sun6i_dma_pause() {
...
struct sun6i_pchan *pchan = vchan->phy;
...
}
Concurrently, sun6i_dma_tasklet() can finish a transfer, clear vchan->phy,
and immediately reallocate the physical channel to a completely different
virtual channel. The original functions then proceed to use the stale
pchan pointer without holding locks, inadvertently stopping, pausing, or
querying an unrelated active DMA transfer. Can this lead to data corruption
and hardware timeouts?
@@ -27,7 +27,6 @@
/*
* Common registers
*/
-#define DMA_IRQ_EN(x) ((x) * 0x04)
#define DMA_IRQ_HALF BIT(0)
#define DMA_IRQ_PKG BIT(1)
#define DMA_IRQ_QUEUE BIT(2)
@@ -36,8 +35,6 @@
#define DMA_IRQ_CHAN_WIDTH 4
-#define DMA_IRQ_STAT(x) ((x) * 0x04 + 0x10)
-
#define DMA_STAT 0x30
/* Offset between DMA_IRQ_EN and DMA_IRQ_STAT limits number of channels */
@@ -52,6 +49,14 @@
#define SUNXI_H3_SECURE_REG 0x20
#define SUNXI_H3_DMA_GATE 0x28
#define SUNXI_H3_DMA_GATE_ENABLE 0x4
+
+/*
+ * Interrupts specific registers
+ */
+#define DMA_IRQ_STRIDE_A31 0x04
+#define DMA_IRQ_EN_OFFSET_A31 0x00
+#define DMA_IRQ_STAT_OFFSET_A31 0x10
+
/*
* Channels specific registers
*/
@@ -144,6 +149,9 @@ struct sun6i_dma_config {
u32 dst_addr_widths;
bool has_high_addr;
bool has_mbus_clk;
+ u32 irq_stride;
+ u32 irq_en_offset;
+ u32 irq_stat_offset;
};
/*
@@ -234,19 +242,43 @@ to_sun6i_desc(struct dma_async_tx_descriptor *tx)
return container_of(tx, struct sun6i_desc, vd.tx);
}
+static u32 sun6i_read_irq_en(struct sun6i_dma_dev *sdev, u32 irq_reg)
+{
+ return readl(sdev->base + irq_reg * sdev->cfg->irq_stride + sdev->cfg->irq_en_offset);
+}
+
+static void sun6i_write_irq_en(struct sun6i_dma_dev *sdev, u32 irq_reg, u32 irq_val)
+{
+ writel(irq_val, sdev->base + irq_reg * sdev->cfg->irq_stride + sdev->cfg->irq_en_offset);
+}
+
+static u32 sun6i_read_irq_stat(struct sun6i_dma_dev *sdev, u32 irq_reg)
+{
+ return readl(sdev->base + irq_reg * sdev->cfg->irq_stride + sdev->cfg->irq_stat_offset);
+}
+
+static void sun6i_write_irq_stat(struct sun6i_dma_dev *sdev, u32 irq_reg, u32 status)
+{
+ writel(status, sdev->base + irq_reg * sdev->cfg->irq_stride + sdev->cfg->irq_stat_offset);
+}
+
static inline void sun6i_dma_dump_com_regs(struct sun6i_dma_dev *sdev)
{
- dev_dbg(sdev->slave.dev, "Common register:\n"
- "\tmask0(%04x): 0x%08x\n"
- "\tmask1(%04x): 0x%08x\n"
- "\tpend0(%04x): 0x%08x\n"
- "\tpend1(%04x): 0x%08x\n"
- "\tstats(%04x): 0x%08x\n",
- DMA_IRQ_EN(0), readl(sdev->base + DMA_IRQ_EN(0)),
- DMA_IRQ_EN(1), readl(sdev->base + DMA_IRQ_EN(1)),
- DMA_IRQ_STAT(0), readl(sdev->base + DMA_IRQ_STAT(0)),
- DMA_IRQ_STAT(1), readl(sdev->base + DMA_IRQ_STAT(1)),
- DMA_STAT, readl(sdev->base + DMA_STAT));
+ int i;
+
+ for (i = 0; i < 2; i++) {
+ dev_dbg(sdev->slave.dev, "Common register:\n"
+ "chan num %d\n"
+ "\tmask(%04x): 0x%08x\n"
+ "\tpend(%04x): 0x%08x\n"
+ "\tstats(%04x): 0x%08x\n",
+ i,
+ i * sdev->cfg->irq_stride + sdev->cfg->irq_en_offset,
+ sun6i_read_irq_en(sdev, i),
+ i * sdev->cfg->irq_stride + sdev->cfg->irq_stat_offset,
+ sun6i_read_irq_stat(sdev, i),
+ DMA_STAT, readl(sdev->base + DMA_STAT));
+ }
}
static inline void sun6i_dma_dump_chan_regs(struct sun6i_dma_dev *sdev,
@@ -460,11 +492,11 @@ static int sun6i_dma_start_desc(struct sun6i_vchan *vchan)
vchan->irq_type = vchan->cyclic ? DMA_IRQ_PKG : DMA_IRQ_QUEUE;
- irq_val = readl(sdev->base + DMA_IRQ_EN(irq_reg));
+ irq_val = sun6i_read_irq_en(sdev, irq_reg);
irq_val &= ~((DMA_IRQ_HALF | DMA_IRQ_PKG | DMA_IRQ_QUEUE) <<
(irq_offset * DMA_IRQ_CHAN_WIDTH));
irq_val |= vchan->irq_type << (irq_offset * DMA_IRQ_CHAN_WIDTH);
- writel(irq_val, sdev->base + DMA_IRQ_EN(irq_reg));
+ sun6i_write_irq_en(sdev, irq_reg, irq_val);
writel(pchan->desc->p_lli, pchan->base + DMA_CHAN_LLI_ADDR);
writel(DMA_CHAN_ENABLE_START, pchan->base + DMA_CHAN_ENABLE);
@@ -549,14 +581,14 @@ static irqreturn_t sun6i_dma_interrupt(int irq, void *dev_id)
u32 status;
for (i = 0; i < sdev->num_pchans / DMA_IRQ_CHAN_NR; i++) {
- status = readl(sdev->base + DMA_IRQ_STAT(i));
+ status = sun6i_read_irq_stat(sdev, i);
if (!status)
continue;
dev_dbg(sdev->slave.dev, "DMA irq status %s: 0x%x\n",
str_high_low(i), status);
- writel(status, sdev->base + DMA_IRQ_STAT(i));
+ sun6i_write_irq_stat(sdev, i, status);
for (j = 0; (j < DMA_IRQ_CHAN_NR) && status; j++) {
pchan = sdev->pchans + j;
@@ -1072,9 +1104,11 @@ static struct dma_chan *sun6i_dma_of_xlate(struct of_phandle_args *dma_spec,
static inline void sun6i_kill_tasklet(struct sun6i_dma_dev *sdev)
{
+ int i;
+
/* Disable all interrupts from DMA */
- writel(0, sdev->base + DMA_IRQ_EN(0));
- writel(0, sdev->base + DMA_IRQ_EN(1));
+ for (i = 0; i < 2; i++)
+ sun6i_write_irq_en(sdev, i, 0);
/* Prevent spurious interrupts from scheduling the tasklet */
atomic_inc(&sdev->tasklet_shutdown);
@@ -1098,6 +1132,11 @@ static inline void sun6i_dma_free(struct sun6i_dma_dev *sdev)
}
}
+#define SUN6I_DMA_IRQ_A31_COMMON_CFG \
+ .irq_stride = DMA_IRQ_STRIDE_A31, \
+ .irq_en_offset = DMA_IRQ_EN_OFFSET_A31, \
+ .irq_stat_offset = DMA_IRQ_STAT_OFFSET_A31,
+
/*
* For A31:
*
@@ -1129,6 +1168,7 @@ static struct sun6i_dma_config sun6i_a31_dma_cfg = {
.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
BIT(DMA_SLAVE_BUSWIDTH_4_BYTES),
+ SUN6I_DMA_IRQ_A31_COMMON_CFG
};
/*
@@ -1152,6 +1192,7 @@ static struct sun6i_dma_config sun8i_a23_dma_cfg = {
.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
BIT(DMA_SLAVE_BUSWIDTH_4_BYTES),
+ SUN6I_DMA_IRQ_A31_COMMON_CFG
};
static struct sun6i_dma_config sun8i_a83t_dma_cfg = {
@@ -1170,6 +1211,7 @@ static struct sun6i_dma_config sun8i_a83t_dma_cfg = {
.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
BIT(DMA_SLAVE_BUSWIDTH_4_BYTES),
+ SUN6I_DMA_IRQ_A31_COMMON_CFG
};
/*
@@ -1197,6 +1239,7 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = {
BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
BIT(DMA_SLAVE_BUSWIDTH_8_BYTES),
+ SUN6I_DMA_IRQ_A31_COMMON_CFG
};
/*
@@ -1218,6 +1261,7 @@ static struct sun6i_dma_config sun50i_a64_dma_cfg = {
BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
BIT(DMA_SLAVE_BUSWIDTH_8_BYTES),
+ SUN6I_DMA_IRQ_A31_COMMON_CFG
};
/*
@@ -1241,6 +1285,7 @@ static struct sun6i_dma_config sun50i_a100_dma_cfg = {
BIT(DMA_SLAVE_BUSWIDTH_8_BYTES),
.has_high_addr = true,
.has_mbus_clk = true,
+ SUN6I_DMA_IRQ_A31_COMMON_CFG
};
/*
@@ -1263,6 +1308,7 @@ static struct sun6i_dma_config sun50i_h6_dma_cfg = {
BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
BIT(DMA_SLAVE_BUSWIDTH_8_BYTES),
.has_mbus_clk = true,
+ SUN6I_DMA_IRQ_A31_COMMON_CFG
};
/*
@@ -1286,6 +1332,7 @@ static struct sun6i_dma_config sun8i_v3s_dma_cfg = {
.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
BIT(DMA_SLAVE_BUSWIDTH_4_BYTES),
+ SUN6I_DMA_IRQ_A31_COMMON_CFG
};
static const struct of_device_id sun6i_dma_match[] = {