Re: [PATCH v10 2/3] dmaengine: amlogic: Add general DMA driver for A9

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] Scheduled descriptor free works are never flushed, leading to a use-after-free on module unload.
- [High] Missing IRQ synchronization before killing tasklets in remove() leads to use-after-free.
- [High] Failure to check the return value of platform_get_irq() breaks deferred probing.
- [High] `SG_MAX_LEN` is defined as an unaligned maximum value, causing unaligned physical address boundaries for subsequent DMA segments.
- [High] Missing 64-bit DMA Mask Configuration forces SWIOTLB bounce buffering on ARM64.
- [High] Resuming a paused DMA channel fails to restart pending transfers, leaving the channel permanently stalled.
- [High] Missing src_addr_widths configuration breaks DEV_TO_MEM clients.
- [Medium] Lockless status check leads to spurious errors and incorrect DMA_IN_PROGRESS returns.
- [Medium] aml_dma_tx_status() fails to report the DMA_PAUSED state.
--

--- Patch [2]: [PATCH v10 2/3] dmaengine: amlogic: Add general DMA driver for A9 ---
commit 99b90ddfd63851fd9f1a5c3ca30d4210c8de8c83
Author: Xianwei Zhao <xianwei.zhao@amlogic.com>
Subject: dmaengine: amlogic: Add general DMA driver for A9

This patch introduces a general-purpose DMA controller driver for Amlogic
A9 SoCs. It includes support for virtual channels, SG list scatter/gather
capabilities, and basic transfer modes like Mem-to-Dev and Dev-to-Mem.

> diff --git a/drivers/dma/amlogic-dma.c b/drivers/dma/amlogic-dma.c
> new file mode 100644
> index 0000000000000..388b5a581ae58
> --- /dev/null
> +++ b/drivers/dma/amlogic-dma.c

[ ... ]

> +#define MAX_CHAN_ID		32
> +#define SG_MAX_LEN		GENMASK(26, 0)

[Severity: High]
Does this define the segment boundary as an unaligned odd length?

Since GENMASK(26, 0) equates to 134,217,727 bytes, which is an odd number,
using it to split segments will corrupt the alignment of all subsequent
physical addresses in the chain.

Could this cause hardware alignment faults on the AXI bus for clients
submitting large buffers?

[ ... ]

> +static enum dma_status aml_dma_tx_status(struct dma_chan *chan,
> +					 dma_cookie_t cookie,
> +					 struct dma_tx_state *txstate)
> +{
> +	struct aml_dma_chan *aml_chan = to_aml_dma_chan(chan);
> +	struct aml_dma_dev *aml_dma = aml_chan->aml_dma;
> +	struct aml_dma_desc *aml_desc = NULL;
> +	struct virt_dma_desc *vd;
> +	u32 residue = 0, done;
> +	unsigned long flags;
> +	enum dma_status ret;
> +
> +	ret = dma_cookie_status(chan, cookie, txstate);
> +	if (ret == DMA_COMPLETE || !txstate)
> +		return ret;

[Severity: Medium]
Should this function intercept the status to report DMA_PAUSED?

The core dma_cookie_status() will return DMA_IN_PROGRESS for pending
transfers. Because this unconditionally returns that value, it fails to
substitute the driver's internal DMA_PAUSED state when the channel is
paused.

This prevents clients from detecting that a channel has successfully paused.

> +
> +	spin_lock_irqsave(&aml_chan->vchan.lock, flags);
> +	vd = vchan_find_desc(&aml_chan->vchan, cookie);
> +	if (vd) {
> +		aml_desc = to_aml_dma_desc(vd);
> +		residue = aml_desc->data_len;
> +	} else if (aml_chan->cur_desc && aml_chan->cur_desc->vd.tx.cookie == cookie) {
> +		aml_desc = aml_chan->cur_desc;
> +		regmap_read(aml_dma->regmap, aml_chan->reg_offs + RCH_RD_LEN, &done);
> +		residue = aml_desc->data_len - done;
> +	} else {
> +		dev_err(aml_dma->dma_device.dev, "cookie error\n");
> +	}

[Severity: Medium]
Can this lockless status check lead to spurious cookie errors?

Since dma_cookie_status() is called before acquiring the lock, the transfer
could complete immediately after the check. By the time the lock is
acquired, vchan_find_desc() will fail to find the descriptor, falling
through to the else branch.

This would print a spurious "cookie error" and return a stale
DMA_IN_PROGRESS status with a zero residue. Would re-evaluating the cookie
status inside the lock prevent this race?

[ ... ]

> +	sg_link = aml_desc->sg_link;
> +	for_each_sg(sgl, sg, sg_len, i) {
> +		avail = sg_dma_len(sg);
> +		if (avail == 0)
> +			continue;
> +		paddr = sg->dma_address;
> +		while (avail > SG_MAX_LEN) {
> +			/* set dma address and len to sglink*/
> +			sg_link->addr_low = lower_32_bits(paddr);
> +			sg_link->addr_high = upper_32_bits(paddr);
> +			sg_link->ctl = FIELD_PREP(LINK_LEN, SG_MAX_LEN);
> +			paddr = paddr + SG_MAX_LEN;
> +			avail = avail - SG_MAX_LEN;
> +			sg_link++;
> +		}

[Severity: High]
Will this split corrupt the physical address alignment?

Because SG_MAX_LEN is an odd number, adding it to paddr guarantees that the
subsequent segment will start at an unaligned physical address. Could this
cause hardware faults during transfers?

[ ... ]

> +static int aml_dma_chan_resume(struct dma_chan *chan)
> +{
> +	struct aml_dma_chan *aml_chan = to_aml_dma_chan(chan);
> +	struct aml_dma_dev *aml_dma = aml_chan->aml_dma;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&aml_chan->vchan.lock, flags);
> +	regmap_clear_bits(aml_dma->regmap, aml_chan->reg_offs + RCH_CFG, CFG_PAUSE);
> +	aml_chan->status = aml_chan->pre_status;
> +	spin_unlock_irqrestore(&aml_chan->vchan.lock, flags);
> +
> +	return 0;
> +}

[Severity: High]
Does this function need to inspect the desc_issued list or call
aml_dma_start() after clearing the pause state?

If a client queues a new transfer and calls issue_pending() while the
channel is paused, aml_dma_start() returns early. The transfer stays in
desc_issued.

Without a call to aml_dma_start() during resume, the queued descriptors
remain trapped indefinitely and the channel stalls.

[ ... ]

> +static int aml_dma_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct dma_device *dma_dev;
> +	struct aml_dma_dev *aml_dma;
> +	int ret, i, len;
> +	u32 chan_nr;

[ ... ]

> +	aml_dma->clk = devm_clk_get_enabled(&pdev->dev, NULL);
> +	if (IS_ERR(aml_dma->clk))
> +		return PTR_ERR(aml_dma->clk);
> +
> +	aml_dma->irq = platform_get_irq(pdev, 0);
> +
> +	aml_dma->pdev = pdev;
> +	aml_dma->dma_device.dev = &pdev->dev;

[Severity: High]
Can this break deferred probing if platform_get_irq() fails?

If platform_get_irq() returns -EPROBE_DEFER, storing it directly without
checking for negative errors means it will later be passed to
devm_request_irq().

The request will treat it as a massive invalid IRQ number and reject it
with -EINVAL, permanently failing the probe.

[Severity: High]
Does the DMA device initialization here miss declaring 64-bit addressing
capabilities to the core framework?

The hardware naturally supports 64-bit physical addresses, but since
dma_set_mask_and_coherent() is never called to set a 64-bit mask, the
framework will fall back to a 32-bit mask.

Will this force any memory mapped above 4GB to use SWIOTLB bounce buffers
on ARM64 systems?

[ ... ]

> +	dma_set_max_seg_size(dma_dev->dev, SG_MAX_LEN);
> +	dma_cap_set(DMA_SLAVE, dma_dev->cap_mask);
> +	dma_dev->device_alloc_chan_resources = aml_dma_alloc_chan_resources;
> +	dma_dev->device_free_chan_resources = aml_dma_free_chan_resources;
> +	dma_dev->device_tx_status = aml_dma_tx_status;
> +	dma_dev->device_prep_slave_sg = aml_dma_prep_slave_sg;
> +	dma_dev->device_pause = aml_dma_chan_pause;
> +	dma_dev->device_resume = aml_dma_chan_resume;
> +	dma_dev->device_terminate_all = aml_dma_terminate_all;
> +	dma_dev->device_issue_pending = aml_dma_issue_pending;
> +	/* PIO 4 bytes and I2C 1 byte */
> +	dma_dev->dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) | BIT(DMA_SLAVE_BUSWIDTH_1_BYTE);
> +	dma_dev->directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
> +	dma_dev->residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;

[Severity: High]
Does missing src_addr_widths configuration break DEV_TO_MEM clients?

The driver advertises DMA_DEV_TO_MEM support, but only populates
dst_addr_widths.

When generic DMA clients (like ALSA/ASoC) attempt to configure the channel
for capture (Rx), the capability check will fail because src_addr_widths
is 0, causing the framework to reject the channel.

[ ... ]

> +static void aml_dma_remove(struct platform_device *pdev)
> +{
> +	struct aml_dma_dev *aml_dma = platform_get_drvdata(pdev);
> +	struct aml_dma_chan *aml_chan = NULL;
> +	int i;
> +
> +	of_dma_controller_free((&pdev->dev)->of_node);
> +
> +	regmap_write(aml_dma->regmap, RCH_INT_MASK, 0xffffffff);
> +	regmap_write(aml_dma->regmap, WCH_INT_MASK, 0xffffffff);

[Severity: High]
Could unloading this module lead to a use-after-free from un-flushed work?

The aml_dma_free_desc() function schedules free_work on the global system
workqueue. If a DMA channel is freed just before or during module unload,
the scheduled work is not flushed or waited upon before devres drops the
backing memory.

The asynchronous work will execute after aml_dma is freed, dereferencing a
dangling pointer.

> +
> +	for (i = 0; i < MAX_CHAN_ID; i++) {
> +		aml_chan = aml_dma->aml_rch[i];
> +		if (aml_chan)
> +			tasklet_kill(&aml_chan->vchan.task);
> +		aml_chan = aml_dma->aml_wch[i];
> +		if (aml_chan)
> +			tasklet_kill(&aml_chan->vchan.task);
> +	}
> +}

[Severity: High]
Does killing tasklets here without IRQ synchronization expose a race window?

Because devm_request_irq() is used, the interrupt is freed by devres after
remove() completes. If an interrupt handler is actively executing on another
CPU, it can call tasklet_schedule() immediately after tasklet_kill()
finishes.

Devres then frees the structures, and the newly scheduled tasklet will later
execute on freed memory in the softirq context. Would calling
synchronize_irq() before killing tasklets prevent this?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-amlogic-dma-v10-0-ff4deae837e7@amlogic.com?part=2