Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [Critical] The DMA interrupt handler incorrectly resolves the physical channel (`pchan`) from the interrupt status bit by ignoring the outer loop register index `i`.
- [High] Integer division truncation in loop bounds causes upper hardware channels to be completely ignored, and introduces out-of-bounds MMIO accesses.
--
commit e328a2b422946f5a9b0337a1058273c7077c1ac4
Author: Yuanshen Cao <alex.caoys@gmail.com>
dmaengine: sun6i-dma: Implement support for Allwinner A733 DMA controller
This patch implements the actual support for the Allwinner A733 DMA
controller. It defines the new register offsets and bitfield mappings
required for the A733, which slightly differs from the older sun6i
series.
> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index fb1c1a28744bb..9585b4a9e00de 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
[ ... ]
> @@ -1352,6 +1412,33 @@ static struct sun6i_dma_config sun50i_h6_dma_cfg = {
> .has_mbus_clk = true,
> };
>
> +/*
> + * The A733 binding uses the number of dma channels from the
> + * device tree node.
> + */
> +static struct sun6i_dma_config sun60i_a733_dma_cfg = {
> + .clock_autogate_enable = sun6i_enable_clock_autogate_h3,
> + .set_burst_length = sun6i_set_burst_length_h3,
> + .set_drq = sun6i_set_drq_h6,
> + .set_mode = sun6i_set_mode_h6,
> + .set_addr = sun6i_dma_set_addr_a733,
> + .dump_com_regs = sun6i_dma_dump_com_regs_a733,
> + .read_irq_en = sun6i_read_irq_en_a733,
> + .write_irq_en = sun6i_write_irq_en_a733,
> + .read_irq_stat = sun6i_read_irq_stat_a733,
> + .write_irq_stat = sun6i_write_irq_stat_a733,
> + .src_burst_lengths = BIT(1) | BIT(4) | BIT(8) | BIT(16),
> + .dst_burst_lengths = BIT(1) | BIT(4) | BIT(8) | BIT(16),
> + .src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
> + BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> + BIT(DMA_SLAVE_BUSWIDTH_4_BYTES),
> + .dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
> + BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> + BIT(DMA_SLAVE_BUSWIDTH_4_BYTES),
> + .num_channels_per_reg = DMA_IRQ_CHAN_NR_A733,
[Severity: Critical]
This is a pre-existing issue, but does setting num_channels_per_reg to 1
(DMA_IRQ_CHAN_NR_A733) break interrupt handling for all A733 channels > 0?
In sun6i_dma_interrupt(), the physical channel is calculated without the outer
loop index i:
for (i = 0; i < sdev->num_pchans / sdev->cfg->num_channels_per_reg; i++) {
...
for (j = 0; (j < sdev->cfg->num_channels_per_reg) && status; j++) {
pchan = sdev->pchans + j;
Since num_channels_per_reg is 1, j is always 0. As a result, every interrupt
on every channel will incorrectly resolve to sdev->pchans[0]. Doesn't this
completely break DMA functionality for all A733 channels except channel 0 by
causing spurious completions on channel 0 and hanging all others?
[Severity: High]
This is a pre-existing issue, but does using integer division truncation for
loop bounds ignore remainder channels and cause out-of-bounds MMIO accesses?
In sun6i_dma_interrupt(), the loop bound is:
i < sdev->num_pchans / sdev->cfg->num_channels_per_reg
For SoCs where the number of channels is not a perfect multiple of
num_channels_per_reg (like H3 with 12 channels and 8 channels per register),
this division truncates (12 / 8 = 1), causing the remainder channels to be
entirely ignored so their interrupts are never serviced.
Additionally, this patch series introduces a similar loop in
sun6i_kill_tasklet():
for (i = 0; i < DMA_MAX_CHANNELS / sdev->cfg->num_channels_per_reg; i++)
Since DMA_MAX_CHANNELS is 16 and num_channels_per_reg is 1 for A733, the loop
evaluates to 16. If the A733 hardware possesses fewer than 16 channels, won't
this cause out-of-bounds MMIO writes which can trigger bus aborts?
> + .has_mbus_clk = true,
> +};
> +
@@ -52,6 +52,15 @@
#define SUNXI_H3_SECURE_REG 0x20
#define SUNXI_H3_DMA_GATE 0x28
#define SUNXI_H3_DMA_GATE_ENABLE 0x4
+
+/*
+ * sun60i specific registers
+ */
+#define DMA_IRQ_EN_A733(x) ((x) * 0x40 + 0x134)
+#define DMA_IRQ_STAT_A733(x) ((x) * 0x40 + 0x138)
+
+#define DMA_IRQ_CHAN_NR_A733 1
+
/*
* Channels specific registers
*/
@@ -100,6 +109,8 @@
*/
#define SRC_HIGH_ADDR(x) (((x) & 0x3U) << 16)
#define DST_HIGH_ADDR(x) (((x) & 0x3U) << 18)
+#define SRC_HIGH_ADDR_32G(x) (((x) & 0x7U) << 11)
+#define DST_HIGH_ADDR_32G(x) (((x) & 0x7U) << 15)
/*
* Various hardware related defines
@@ -257,6 +268,23 @@ static inline void sun6i_dma_dump_com_regs(struct sun6i_dma_dev *sdev)
DMA_STAT, readl(sdev->base + DMA_STAT));
}
+static inline void sun6i_dma_dump_com_regs_a733(struct sun6i_dma_dev *sdev)
+{
+ int i;
+
+ for (i = 0; i < sdev->num_pchans / sdev->cfg->num_channels_per_reg; 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,
+ DMA_IRQ_EN_A733(i), readl(sdev->base + DMA_IRQ_EN_A733(i)),
+ DMA_IRQ_STAT_A733(i), readl(sdev->base + DMA_IRQ_STAT_A733(i)),
+ DMA_STAT, readl(sdev->base + DMA_STAT));
+ }
+}
+
static inline void sun6i_dma_dump_chan_regs(struct sun6i_dma_dev *sdev,
struct sun6i_pchan *pchan)
{
@@ -360,20 +388,41 @@ static u32 sun6i_read_irq_en(struct sun6i_dma_dev *sdev, u32 chan_num)
return readl(sdev->base + DMA_IRQ_EN(chan_num));
}
+static u32 sun6i_read_irq_en_a733(struct sun6i_dma_dev *sdev, u32 chan_num)
+{
+ return readl(sdev->base + DMA_IRQ_EN_A733(chan_num));
+}
+
static void sun6i_write_irq_en(struct sun6i_dma_dev *sdev, u32 chan_num, u32 irq_val)
{
writel(irq_val, sdev->base + DMA_IRQ_EN(chan_num));
}
+
+static void sun6i_write_irq_en_a733(struct sun6i_dma_dev *sdev, u32 chan_num, u32 irq_val)
+{
+ writel(irq_val, sdev->base + DMA_IRQ_EN_A733(chan_num));
+}
+
static u32 sun6i_read_irq_stat(struct sun6i_dma_dev *sdev, u32 chan_num)
{
return readl(sdev->base + DMA_IRQ_STAT(chan_num));
}
+static u32 sun6i_read_irq_stat_a733(struct sun6i_dma_dev *sdev, u32 chan_num)
+{
+ return readl(sdev->base + DMA_IRQ_STAT_A733(chan_num));
+}
+
static void sun6i_write_irq_stat(struct sun6i_dma_dev *sdev, u32 chan_num, u32 status)
{
writel(status, sdev->base + DMA_IRQ_STAT(chan_num));
}
+static void sun6i_write_irq_stat_a733(struct sun6i_dma_dev *sdev, u32 chan_num, u32 status)
+{
+ writel(status, sdev->base + DMA_IRQ_STAT_A733(chan_num));
+}
+
static size_t sun6i_get_chan_size(struct sun6i_pchan *pchan)
{
struct sun6i_desc *txd = pchan->desc;
@@ -694,6 +743,17 @@ static inline void sun6i_dma_set_addr_a100(struct sun6i_dma_dev *sdev,
DST_HIGH_ADDR(upper_32_bits(dst));
}
+static inline void sun6i_dma_set_addr_a733(struct sun6i_dma_dev *sdev,
+ struct sun6i_dma_lli *v_lli,
+ dma_addr_t src, dma_addr_t dst)
+{
+ v_lli->src = lower_32_bits(src);
+ v_lli->dst = lower_32_bits(dst);
+
+ v_lli->para |= SRC_HIGH_ADDR_32G(upper_32_bits(src)) |
+ DST_HIGH_ADDR_32G(upper_32_bits(dst));
+}
+
static struct dma_async_tx_descriptor *sun6i_dma_prep_dma_memcpy(
struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
size_t len, unsigned long flags)
@@ -1352,6 +1412,33 @@ static struct sun6i_dma_config sun50i_h6_dma_cfg = {
.has_mbus_clk = true,
};
+/*
+ * The A733 binding uses the number of dma channels from the
+ * device tree node.
+ */
+static struct sun6i_dma_config sun60i_a733_dma_cfg = {
+ .clock_autogate_enable = sun6i_enable_clock_autogate_h3,
+ .set_burst_length = sun6i_set_burst_length_h3,
+ .set_drq = sun6i_set_drq_h6,
+ .set_mode = sun6i_set_mode_h6,
+ .set_addr = sun6i_dma_set_addr_a733,
+ .dump_com_regs = sun6i_dma_dump_com_regs_a733,
+ .read_irq_en = sun6i_read_irq_en_a733,
+ .write_irq_en = sun6i_write_irq_en_a733,
+ .read_irq_stat = sun6i_read_irq_stat_a733,
+ .write_irq_stat = sun6i_write_irq_stat_a733,
+ .src_burst_lengths = BIT(1) | BIT(4) | BIT(8) | BIT(16),
+ .dst_burst_lengths = BIT(1) | BIT(4) | BIT(8) | BIT(16),
+ .src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
+ BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
+ BIT(DMA_SLAVE_BUSWIDTH_4_BYTES),
+ .dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
+ BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
+ BIT(DMA_SLAVE_BUSWIDTH_4_BYTES),
+ .num_channels_per_reg = DMA_IRQ_CHAN_NR_A733,
+ .has_mbus_clk = true,
+};
+
/*
* The V3s have only 8 physical channels, a maximum DRQ port id of 23,
* and a total of 24 usable source and destination endpoints.
@@ -1392,6 +1479,7 @@ static const struct of_device_id sun6i_dma_match[] = {
{ .compatible = "allwinner,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg },
{ .compatible = "allwinner,sun50i-a100-dma", .data = &sun50i_a100_dma_cfg },
{ .compatible = "allwinner,sun50i-h6-dma", .data = &sun50i_h6_dma_cfg },
+ { .compatible = "allwinner,sun60i-a733-dma", .data = &sun60i_a733_dma_cfg },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, sun6i_dma_match);