Re: [PATCH v2 2/6] dmaengine: arm-dma350: Add slave and cyclic transfer support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] dma: inaccurate cyclic residue reporting omits hardware progress
- [High] dmaengine: arm-dma350: Missing DMA sync for command list after CPU writes
- [High] dmaengine: arm-dma350: TOCTOU race in SG residue calculation
- [Medium] dma: DEV_TO_MEM trigger block size ignores maxburst configuration
- [Medium] dmaengine: arm-dma350: false DMA_RESIDUE_GRANULARITY_BURST for cyclic transfers
--

commit 2323319884614c7159efde4dd0a7bd92ada52a37
Author: Jelly Jia <Jelly.Jia@cixtech.com>

dmaengine: arm-dma350: Add slave and cyclic transfer support

This commit adds DMA slave support to the Arm DMA-350 driver. It enables
scatter-gather and cyclic transfers, handles address translations and caching,
and reworks the completion logic for the new descriptor types.

> diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
> index 4e17130de6c87..bbcf4071c3e75 100644
> --- a/drivers/dma/arm-dma350.c
> +++ b/drivers/dma/arm-dma350.c

[ ... ]

> @@ -212,9 +256,419 @@ static inline struct d350_desc *to_d350_desc(struct virt_dma_desc *vd)
>  	return container_of(vd, struct d350_desc, vd);
>  }
>  
> +static void d350_free_cmds(struct device *dev, struct d350_desc *desc)
> +{
[ ... ]
> +static int d350_alloc_cmds(struct dma_chan *dchan, struct d350_desc *desc,
> +			   size_t ncmds)
> +{
[ ... ]
> +	desc->cmds = kmalloc(desc->cmds_size, GFP_NOWAIT);
> +	if (!desc->cmds)
> +		return -ENOMEM;
> +
> +	desc->cmds_dma = dma_map_single(dev, desc->cmds, desc->cmds_size,
> +					DMA_TO_DEVICE);

[Severity: High]
This code maps the command list memory for DMA (which performs a cache
clean) before any commands are actually written to it.

Later, in d350_prep_slave_sg() and d350_prep_dma_cyclic(), the CPU populates
desc->cmds:

> +static struct dma_async_tx_descriptor *
> +d350_prep_slave_sg(struct dma_chan *dchan, struct scatterlist *sgl,
[ ... ]
> +		if (sg_len > 1) {
> +			cmd = desc->cmds + i * D350_SLAVE_CMD_WORDS;
[ ... ]
> +		d350_fill_slave_cmd(dch, desc, cmd, mem, dev_dma_addr, len,
> +				    link, link_addr, direction, width, maxburst,
> +				    donetype);

Is a dma_sync_single_for_device() call missing after these writes?

Because the mapping and its associated cache flush occur before the CPU writes
the command data, the new commands may remain in the CPU cache. When the DMA
controller fetches subsequent commands from RAM, it could read stale memory.
I noticed this is intentionally fixed later in the patch series, but could
it be squashed here to prevent a broken intermediate state?

[ ... ]

> +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, bool link, 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;
[ ... ]
> +	trigcfg = FIELD_PREP(CH_TRIGINCFG_BLKSIZE,
> +			     mem_to_dev ? maxburst - 1 : 0) |

[Severity: Medium]
For DEV_TO_MEM transfers (mem_to_dev == false), the block size is
unconditionally hardcoded to 0, completely ignoring the requested maxburst.

The commit message states that the trigger input is programmed with a
"per-direction block size matching the configured maxburst." Does this
mismatch between the peripheral's FIFO watermark and the DMA controller's
fetch size degrade performance or stall the transfer?

[ ... ]

> @@ -339,6 +795,54 @@ static u32 d350_get_residue(struct d350_chan *dch)
>  	return res << dch->desc->tsz;
>  }
>  
> +static u32 d350_get_sg_residue(struct d350_chan *dch)
> +{
[ ... ]
> +	/* The channel may step the link pointer between the two reads */
> +	hi_new = readl_relaxed(dch->base + CH_LINKADDRHI);
> +	do {
> +		linkaddrhi = hi_new;
> +		linkaddr = readl_relaxed(dch->base + CH_LINKADDR);
> +		hi_new = readl_relaxed(dch->base + CH_LINKADDRHI);
> +	} while (linkaddrhi != hi_new && --retries);
> +
> +	next_cmd = (linkaddr & ~CH_LINKADDR_EN) | ((u64)linkaddrhi << 32);
> +
> +	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);
> +	for (i = cmd + 1; i < desc->ncmds; i++)
> +		residue += desc->cmd_len[i];
> +
> +	return residue;
> +}

[Severity: High]
Could the DMA controller advance to the next command between the CH_LINKADDR
reads and the CH_XSIZE read in d350_get_residue()?

If the hardware advances, CH_XSIZE will reflect the new command's remaining
bytes, but the calculation here adds them to the previous command's length.
This would double-count the new command's bytes, causing the reported residue
to increase rather than remaining strictly monotonic decreasing.

Should the driver read CH_LINKADDR again after CH_XSIZE to verify it hasn't
changed?

[ ... ]

> @@ -369,7 +873,20 @@ static void d350_synchronize(struct dma_chan *chan)
>  
>  static u32 d350_desc_bytes(struct d350_desc *desc)
>  {
> -	return ((u32)desc->xsizehi << 16 | desc->xsize) << desc->tsz;
> +	return desc->bytes;
> +}
> +
> +static u32 d350_get_cyclic_residue(struct d350_desc *desc)
> +{
> +	return desc->bytes - desc->period * desc->period_len;
> +}

[Severity: High]
Does this calculation omit the intra-period transferred bytes by entirely
ignoring the hardware transfer progress (CH_XSIZE)?

[Severity: Medium]
Because this relies solely on software period counters, it only provides
period-level granularity. The driver explicitly advertises
DMA_RESIDUE_GRANULARITY_BURST in d350_probe():

> @@ -568,23 +1107,32 @@ static int d350_probe(struct platform_device *pdev)
[ ... ]
>  	dmac->dma.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;

Does the omission of hardware progress cause audio pointer updates in ALSA
to jump in large discrete steps, leading to stuttering or underruns?

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