Re: [PATCH v1 2/5] dmaengine: arm-dma350: Add slave transfer support

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:

New issues:
- [High] DMA descriptors in memory are written using native endianness, causing corruption on big-endian CPUs.
- [High] Cyclic DMA residue calculation relies on fragile software tracking, lacks sub-period precision, and permanently desynchronizes if interrupts coalesce.
- [High] d350_of_xlate() incorrectly fails to allocate a channel if the first free channel lacks trigger support.
- [High] Valid DMA address of 0 is erroneously treated as a sentinel, disabling descriptor linking and breaking cyclic/SG transfers.
- [High] Calling dma_free_coherent() from the vchan_complete tasklet causes sleeping in atomic context.
- [High] Race condition between reading CH_LINKADDR and CH_XSIZE leads to invalid, non-monotonic DMA residue calculations.
- [Medium] Trigger block size ignores configured maxburst for DEV_TO_MEM transfers, degrading performance.
- [Medium] Torn read of the 64-bit hardware CH_LINKADDR register due to missing retry loop.

Pre-existing issues:
- [High] Spurious DMA_ERROR returned for descriptors that have been submitted but not yet issued.
--

commit e35af7caf5e3e88f06c1135c2c306f3cb90773b3
Author: Jelly Jia [off-list ref]

dmaengine: arm-dma350: Add slave transfer support

Add DMA slave support to the Arm DMA-350 driver, which so far only
supports memory-to-memory transfers.

[ ... ]

+static void d350_free_cmds(struct device *dev, struct d350_desc *desc)
+{
+	if (desc->cmds)
+		dma_free_coherent(dev, desc->cmds_size, desc->cmds,
+				  desc->cmds_dma);
+	kfree(desc->cmd_len);
+}
+
 static void d350_desc_free(struct virt_dma_desc *vd)
 {
-	kfree(to_d350_desc(vd));
+	struct d350_desc *desc = to_d350_desc(vd);
+
+	d350_free_cmds(vd->tx.chan->device->dev, desc);
+	kfree(desc);
+}

[Severity: High]
Can this sleep in atomic context?

d350_desc_free() is registered as the virt-dma desc_free callback, which
executes in a tasklet (softirq) context. However, dma_free_coherent()
might sleep (e.g., when flushing IOMMU page tables), which could trigger
scheduling-while-atomic BUGs.

[ ... ]

+static struct dma_chan *d350_of_xlate(struct of_phandle_args *dma_spec,
+				      struct of_dma *ofdma)
+{
+	struct d350 *dmac = ofdma->of_dma_data;
+	struct dma_chan *chan;
+	struct d350_chan *dch;
+	u32 req;

[ ... ]

+	chan = dma_get_any_slave_channel(&dmac->dma);
+	if (!chan) {
+		dev_err(dmac->dma.dev, "can't get a dma channel\n");
+		return NULL;
+	}
+
+	dch = to_d350_chan(chan);
+	if (!dch->has_trig) {
+		dev_err(dmac->dma.dev, "channel %d has no trigger support\n",
+			chan->chan_id);
+		dma_release_channel(chan);
+		return NULL;
+	}

[Severity: High]
Does this incorrectly reject valid requests on heterogeneous DMA configurations?

If dma_get_any_slave_channel() happens to return a channel lacking trigger
support, the request is dropped entirely instead of continuing to search
the remaining free channels for a capable one.

[ ... ]

+static void d350_fill_slave_cmd(struct d350_chan *dch, struct d350_desc *desc,
+				u32 *cmd, dma_addr_t mem, dma_addr_t dev_dma_addr,
+				size_t len, dma_addr_t link_addr,
+				enum dma_transfer_direction direction,
+				enum dma_slave_buswidth width, u32 maxburst,
+				enum ch_ctrl_donetype donetype)
+{
+	bool mem_to_dev = direction == DMA_MEM_TO_DEV;
+	u16 xsize, xsizehi;
+	u32 devcfg;
+	u32 memcfg;
+	u32 trigcfg;
+
+	desc->tsz = __ffs(width);
+	xsize = lower_16_bits(len >> desc->tsz);
+	xsizehi = upper_16_bits(len >> desc->tsz);
+	devcfg = d350_device_transcfg(maxburst);
+	memcfg = dch->coherent ? TRANSCFG_WB : TRANSCFG_NC;
+
+	trigcfg = FIELD_PREP(CH_TRIGINCFG_BLKSIZE,
+			     mem_to_dev ? maxburst - 1 : 0) |

[Severity: Medium]
Does this degrade performance for device-to-memory transfers?

The trigger block size ignores the configured maxburst and is hardcoded to 0
for DEV_TO_MEM transfers. This means only a single item is processed per
hardware trigger, which might cause peripheral FIFOs to overflow if they
expect burst servicing.

+		  FIELD_PREP(CH_TRIGINCFG_MODE, CH_TRIGINCFG_MODE_PERIPH_FC) |
+		  FIELD_PREP(CH_TRIGINCFG_TYPE, CH_TRIGINCFG_TYPE_HW) |
+		  FIELD_PREP(CH_TRIGINCFG_SEL, dch->req);
+
+	cmd[0] = LINK_CTRL | LINK_SRCADDR | LINK_SRCADDRHI | LINK_DESADDR |
+		 LINK_DESADDRHI | LINK_XSIZE | LINK_XSIZEHI | LINK_SRCTRANSCFG |
+		 LINK_DESTRANSCFG | LINK_XADDRINC | LINK_LINKADDR |
+		 LINK_LINKADDRHI |
+		 (mem_to_dev ? LINK_DESTRIGINCFG : LINK_SRCTRIGINCFG);

[Severity: High]
Will this cause corruption on big-endian CPUs?

Native CPU-endian words are being written directly to the DMA command list
buffer (desc->cmds), which will be fetched by the little-endian hardware.
Should this use cpu_to_le32 wrappers to avoid swapped addresses and sizes?

[ ... ]

+	cmd[11] = trigcfg;
+	cmd[12] = lower_32_bits(link_addr) |
+		  (link_addr ? CH_LINKADDR_EN : 0);

[Severity: High]
Is it safe to use link_addr as a boolean condition here?

dma_alloc_coherent() can validly return 0 as a bus address. If it does,
CH_LINKADDR_EN will not be set, which would prematurely terminate and break
cyclic or scatter-gather transfers.

+	cmd[13] = upper_32_bits(link_addr);
+}

[ ... ]

+static u32 d350_get_sg_residue(struct d350_chan *dch)
+{
+	struct d350_desc *desc = dch->desc;
+	size_t cmd_size = D350_SLAVE_CMD_WORDS * sizeof(u32);
+	size_t cmd = 0, i;
+	u32 residue;
+	u64 next_cmd;
+
+	if (!desc->cmd_len)
+		return d350_get_residue(dch);
+
+	/*
+	 * CH_LINKADDR points at the next command. Match it against the command
+	 * array to find the command currently executing, then add every later
+	 * command which has not started yet.
+	 */
+	next_cmd = readl_relaxed(dch->base + CH_LINKADDR) & ~CH_LINKADDR_EN;
+	next_cmd |= (u64)readl_relaxed(dch->base + CH_LINKADDRHI) << 32;

[Severity: Medium]
Could this result in a torn read of the 64-bit hardware CH_LINKADDR register?

The two 32-bit reads are executed sequentially without protection against
concurrent hardware updates, unlike the safe retry loop you used for CH_XSIZE.
If the hardware advances the command chain between the reads, the CPU constructs
a torn address.

+
+	if (!next_cmd) {
+		cmd = desc->ncmds - 1;
+	} else {
+		for (i = 1; i < desc->ncmds; i++) {
+			if (next_cmd == desc->cmds_bus + i * cmd_size) {
+				cmd = i - 1;
+				break;
+			}
+		}
+		if (i == desc->ncmds)
+			return dch->residue;
+	}
+
+	residue = d350_get_residue(dch);

[Severity: High]
Is there a race condition between reading CH_LINKADDR and CH_XSIZE?

d350_get_residue() reads CH_XSIZE. If the hardware completes command N and
starts N+1 between the earlier read of CH_LINKADDR and the read of CH_XSIZE,
the driver calculates residue using command N's position in the loop but
command N+1's remaining bytes. This would double-count the size of N+1 and
cause the reported residue to jump upwards, breaking monotonicity.

+	for (i = cmd + 1; i < desc->ncmds; i++)
+		residue += desc->cmd_len[i];
+
+	return residue;
+}

[ ... ]

+static u32 d350_get_cyclic_residue(struct d350_desc *desc)
+{
+	return desc->bytes - desc->period * desc->period_len;
+}

[Severity: High]
Does this provide sufficient precision for cyclic consumers like ALSA?

This calculates an inaccurate residue that ignores intra-period hardware
progress, returning only period boundaries. ALSA PCM pointers typically
require high-resolution, accurate sub-period updates.

[ ... ]

 static enum dma_status d350_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
 				      struct dma_tx_state *state)
 {
 	struct d350_chan *dch = to_d350_chan(chan);
 	struct virt_dma_desc *vd;
 	enum dma_status status;
 	unsigned long flags;
 	u32 residue = 0;
 
 	status = dma_cookie_status(chan, cookie, state);
 
 	spin_lock_irqsave(&dch->vc.lock, flags);
 	if (cookie == dch->cookie) {
 		status = dch->status;
 		if (status == DMA_IN_PROGRESS || status == DMA_PAUSED)
-			dch->residue = d350_get_residue(dch);
+			dch->residue = d350_get_active_residue(dch);
 		residue = dch->residue;
 	} else if ((vd = vchan_find_desc(&dch->vc, cookie))) {
 		residue = d350_desc_bytes(to_d350_desc(vd));
 	} else if (status == DMA_IN_PROGRESS) {
 		/* Somebody else terminated it? */
 		status = DMA_ERROR;
 	}

[Severity: High]
This is a pre-existing issue, but does this incorrectly return DMA_ERROR for
submitted but unissued descriptors?

vchan_find_desc() only searches the desc_issued list. If a client queries
status after tx_submit but before issue_pending, this explicit overwrite
converts a valid DMA_IN_PROGRESS status into DMA_ERROR.

[ ... ]

 static irqreturn_t d350_irq(int irq, void *data)
 {

[ ... ]

+	if (irq_status & CH_STAT_INTR_ERR) {

[ ... ]

+	} else {
+		if (desc->cyclic) {
+			desc->period = (desc->period + 1) % desc->periods;
+			dch->residue = d350_get_cyclic_residue(desc);
+			vchan_cyclic_callback(vd);

[Severity: High]
Can this permanently desynchronize the cyclic stream?

Unconditionally advancing the period by 1 software counter will lag behind the
actual hardware position if interrupt coalescing occurs or interrupts are
delayed, causing multiple hardware periods to complete between IRQ handling.

+		} else {
+			dch->status = DMA_COMPLETE;
+			dch->residue = 0;
+			dch->desc = NULL;
+			vchan_cookie_complete(vd);
+			d350_start_next(dch);
+		}
 	}
 	spin_unlock(&dch->vc.lock);
 
 	return IRQ_HANDLED;
 }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907033404.596839-1-Jelly.Jia@cixtech.com?part=2