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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] dma: Premature peripheral unmap in d350_map_resource destroys queued mappings
- [Medium] dma: Cyclic residue calculation rewinds position on race fallback
--

commit 8c6ce8c99f8eafa2efa38cc3958ee695ea49436e
Author: Jelly Jia <Jelly.Jia@cixtech.com>
Subject: dmaengine: arm-dma350: Add slave and cyclic transfer support

This commit adds DMA slave and cyclic transfer support to the Arm DMA-350
driver. It translates and caches peripheral addresses and provides completion
handling and residue reporting for the new descriptor types.

> diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
> --- a/drivers/dma/arm-dma350.c
> +++ b/drivers/dma/arm-dma350.c

[ ... ]

> +static dma_addr_t d350_map_resource(struct d350_chan *dch,
> +				    phys_addr_t cpu_addr, size_t size,
> +				    enum dma_data_direction dir)
> +{
> +	struct device *dev = dch->vc.chan.device->dev;
> +	struct d350_chan_map *map = &dch->map;
> +	dma_addr_t dma_addr;
> +
> +	if (map->dir == dir && map->cpu_addr == cpu_addr &&
> +	    map->size == size)
> +		return map->dma_addr;
> +
> +	d350_unmap_resource(dch);
> +
> +	dma_addr = dma_map_resource(dev, cpu_addr, size, dir, 0);

[Severity: High]
If a client queues multiple descriptors sequentially for different
directions, does unmapping the resource immediately here break any pending
descriptors that still rely on the old mapping? Since this function modifies
the dch->map state without holding the channel lock, could concurrent
preparation calls also race and cause double-unmaps or unmapped accesses?

[ ... ]

> +static u32 d350_get_cyclic_residue(struct d350_chan *dch)
> +{
> +	struct d350_desc *desc = dch->desc;
> +	size_t cmd_size = D350_SLAVE_CMD_WORDS * sizeof(u32);
> +	u32 period_residue = desc->bytes - desc->period * desc->period_len;
> +	u32 linkaddr, linkaddrhi, hi_new, lo_new;
> +	u64 next_cmd;
> +	int retries = 3; /* 1st time unlucky, 2nd improbable, 3rd just broken */
> +
> +	/*
> +	 * desc->period counts the completed periods, so period_residue is the
> +	 * residue at the last period boundary. Subtract the progress made in
> +	 * the running period, read from CH_XSIZE: CH_LINKADDR points at the
> +	 * command after the one executing, so while the channel is still on the
> +	 * command of the running period it names the first command of the next
> +	 * period.
> +	 */
> +	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 != desc->cmds_dma +
> +			((desc->period + 1) % desc->periods) * cmd_size)
> +		return period_residue;

[Severity: Medium]
When the hardware races ahead to the next period, does returning the
start-of-period residue cause the reported position to jump backwards?
Because progress was likely made and reported during previous polling,
reverting to period_residue could violate the monotonically advancing
position expected by the ALSA PCM core. Would returning the cached
dch->residue freeze the pointer safely instead?

> +
> +	period_residue -= desc->period_len - d350_get_residue(dch);
> +
> +	/*
> +	 * If the channel stepped to the next period while CH_XSIZE was read,
> +	 * the size belongs to a command of the next period and mixing it with
> +	 * the current one would overstate the residue: keep the value from the
> +	 * period boundary instead.
> +	 */
> +	hi_new = readl_relaxed(dch->base + CH_LINKADDRHI);
> +	lo_new = readl_relaxed(dch->base + CH_LINKADDR);
> +	if (hi_new != linkaddrhi || lo_new != linkaddr)
> +		return d350_desc_bytes(desc) - desc->period * desc->period_len;

[Severity: Medium]
Similar to the fallback above, if the hardware steps while we read CH_XSIZE,
does returning the start-of-period residue here also cause a backwards jump
in the reported position? Could this also be resolved by returning
dch->residue to freeze the progress safely, matching the fallback behavior
in d350_get_sg_residue()?

> +
> +	return period_residue;
> +}

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