Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Pre-existing logic errors in `sun6i_dma_interrupt` fundamentally break interrupt handling for physical channels >= 8 via integer division truncation and misdirected channel array indexing. This actively breaks the newly added A733 support as well as existing SoCs.
--
commit e5054eef1eb5c40444453238af77d7ab1cfceae5
Author: Yuanshen Cao <alex.caoys@gmail.com>
dmaengine: sun6i-dma: Refactor to support A733 interrupt and register handling
This patch refactors the sun6i-dma driver to support flexible register layouts
and interrupt handling. It introduces irq_stride, irq_en_offset, and
irq_stat_offset to struct sun6i_dma_config to accommodate hardware variations.
These changes specifically enable support for the Allwinner A733 SoC.
> 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++) {
[Severity: High]
This is a pre-existing issue, but does this integer division truncate partial
register blocks?
For example, on the 12-channel H3 SoC, 12 / 8 = 1, causing the loop to drop
i=1 and permanently abandon interrupts for channels 8-11. Interrupts for these
upper physical channels are completely ignored, which can lead to unrecoverable
hardware interrupt storms and system hangs when a device allocates them.
> - 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: High]
This is also a pre-existing issue, but does this indexing fail to include the
outer loop's base channel offset (e.g., i * DMA_IRQ_CHAN_NR)?
This blindly routes interrupts from upper registers (e.g., channels 8-15) to
physical channels 0-7, causing premature transfer completions and cross-channel
data corruption.
Furthermore, for the newly introduced A733, where num_channels_per_reg is 1,
the inner loop always evaluates j as 0. This means every interrupt from any
channel maps to pchans[0], rendering the entire DMA controller non-functional.
I have verified that this is not fixed in any subsequent patch in this series.
> vchan = pchan->vchan;
@@ -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[] = {