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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Use-After-Free during driver removal due to missing IRQ synchronization.
- [High] Sleeping in atomic context when freeing virtual DMA descriptors.
- [High] Out-of-bounds write in `aml_dma_prep_slave_sg` when processing zero-length scatterlist elements.
- [High] Broken pause/resume state machine logic leading to channel stall and state corruption.
- [Medium] `aml_dma_terminate_all()` fails to reset channel error state, causing permanent stall.
- [Medium] `aml_dma_tx_status()` improperly handles unissued descriptors, causing false errors and incorrect residues.
- [Medium] Mismatched DMA address registers used for 64-bit physical addresses.
--

commit 7102a7639af273fc6303ccde475117debb548165
Author: Xianwei Zhao <xianwei.zhao@amlogic.com>

dmaengine: amlogic: Add general DMA driver for A9

Amlogic A9 SoCs include a general-purpose DMA controller that can be used
by multiple peripherals, such as I2C PIO and I3C. Each peripheral group
is associated with a dedicated DMA channel in hardware.

> diff --git a/drivers/dma/amlogic-dma.c b/drivers/dma/amlogic-dma.c
> index 0000000000000..bce6932f3a12e
> --- /dev/null
> +++ b/drivers/dma/amlogic-dma.c

[ ... ]

> +static void aml_dma_free_desc(struct virt_dma_desc *vd)
> +{
> +	struct aml_dma_desc *aml_desc = to_aml_dma_desc(vd);
> +
> +	dma_free_coherent(aml_desc->dma_device->dev,
> +			  aml_desc->sg_link_size,
> +			  aml_desc->sg_link,
> +			  aml_desc->sg_link_phys);

[Severity: High]
Does this code sleep in atomic context?

When the virtual DMA descriptor is freed, vchan_dma_desc_free_list() calls
this callback from within vchan_complete(), which runs in tasklet
(softirq) context. Calling a sleepable function like dma_free_coherent()
from a tasklet can result in a scheduling while atomic panic. 

Could this driver use dma_pool_alloc/free or kzalloc/kfree for descriptor
management instead?

> +	kfree(aml_desc);
> +}

[ ... ]

> +/* DMA transfer state  update how many data reside it */
> +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;
> +
> +	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]
Will this incorrectly handle unissued descriptors?

Looking at aml_dma_tx_status(), it attempts to locate a descriptor using
vchan_find_desc(), which exclusively searches the desc_issued list.

If a client queries the status of a descriptor that was submitted (residing
in desc_submitted) but not yet issued via issue_pending(), vchan_find_desc()
will return NULL. 

This causes the driver to log a spurious "cookie error" and return a residue
of 0, which might incorrectly imply the pending transfer is complete.

> +	}
> +	spin_unlock_irqrestore(&aml_chan->vchan.lock, flags);

[ ... ]

> +static struct dma_async_tx_descriptor *aml_dma_prep_slave_sg
> +		(struct dma_chan *chan, struct scatterlist *sgl,
> +		unsigned int sg_len, enum dma_transfer_direction direction,
> +		unsigned long flags, void *context)
> +{
> +	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 aml_dma_sg_link *sg_link = NULL;
> +	struct scatterlist *sg = NULL;
> +	u64 paddr;
> +	u32 link_count, avail;
> +	u32 i;

[ ... ]

> +	sg_link = aml_desc->sg_link;
> +	for_each_sg(sgl, sg, sg_len, i) {
> +		avail = sg_dma_len(sg);
> +		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++;
> +		}
> +		/* 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, avail);
> +
> +		aml_desc->data_len += sg_dma_len(sg);
> +		sg_link++;

[Severity: High]
Can this write out of bounds for zero-length scatterlist elements?

Earlier in aml_dma_prep_slave_sg(), sg_nents_for_dma() calculates the
required number of link descriptors. For an SG element with length 0, it
computes DIV_ROUND_UP(0, SG_MAX_LEN), adding 0 to the link count.

However, this for_each_sg loop unconditionally consumes at least one link
slot per SG element, setting its properties and incrementing sg_link.

If a client provides an empty SG element, this mismatch might result in an
out-of-bounds write past the end of the dma_alloc_coherent array, leading
to memory corruption.

> +	}

[ ... ]

> +static int aml_dma_chan_pause(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_set_bits(aml_dma->regmap, aml_chan->reg_offs + RCH_CFG, CFG_PAUSE);
> +	aml_chan->pre_status = aml_chan->status;
> +	aml_chan->status = DMA_PAUSED;

[Severity: High]
Is the pause and resume state tracking prone to corruption?

If aml_dma_chan_pause() is called twice, it overwrites pre_status with
DMA_PAUSED, which might brick the channel upon resume.

Also, if a client submits a new transaction while paused,
aml_dma_chan_resume() restores DMA_COMPLETE but doesn't call
aml_dma_start(), which could permanently stall the channel.

> +	spin_unlock_irqrestore(&aml_chan->vchan.lock, flags);
> +
> +	return 0;
> +}

[ ... ]

> +static int aml_dma_terminate_all(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;
> +	int chan_id = aml_chan->chan_id;
> +	struct virt_dma_desc *cur_vd;
> +	unsigned long flags;
> +	LIST_HEAD(head);
> +
> +	spin_lock_irqsave(&aml_chan->vchan.lock, flags);

[Severity: Medium]
Will the channel stall permanently if terminated after errors?

When a channel hits 6 hardware errors, aml_dma_start() halts the channel
and abandons pending processing. To recover, a client driver typically calls
dmaengine_terminate_all().

However, aml_dma_terminate_all() doesn't reset aml_chan->status to
DMA_COMPLETE or aml_chan->err_num to 0. Any subsequent transactions
submitted to this channel might hang forever because aml_dma_start() will
immediately return due to the persistent error state.

> +	regmap_set_bits(aml_dma->regmap, aml_chan->reg_offs + RCH_CFG, CFG_PAUSE);

[ ... ]

> +static void aml_dma_start(struct aml_dma_chan *aml_chan)
> +{
> +	struct virt_dma_desc *vd = vchan_next_desc(&aml_chan->vchan);
> +	struct aml_dma_dev *aml_dma = aml_chan->aml_dma;
> +	struct aml_dma_desc *aml_desc = NULL;
> +	int chan_id = aml_chan->chan_id;

[ ... ]

> +	if (aml_chan->direction == DMA_MEM_TO_DEV) {
> +		regmap_write(aml_dma->regmap, aml_chan->reg_offs + RCH_ADDR,
> +			     lower_32_bits(aml_desc->sg_link_phys));
> +		regmap_write(aml_dma->regmap, aml_chan->reg_offs + RCH_ADDR_HIGH,
> +			     upper_32_bits(aml_desc->sg_link_phys));

[Severity: Medium]
Are the correct DMA address registers being used for 64-bit addresses?

The hardware registers define RCH_ADDR (0xc), RCH_ADDR_LOW (0x20), and
RCH_ADDR_HIGH (0x24).

In aml_dma_start(), the lower 32 bits of the physical address are programmed
into the legacy 32-bit RCH_ADDR (0xc) rather than RCH_ADDR_LOW (0x20), while
writing the upper 32 bits to RCH_ADDR_HIGH (0x24).

If the hardware expects the lower bits of a 64-bit address at 0x20, this
might result in an invalid address and immediate DMA failures.

> +		regmap_write(aml_dma->regmap, aml_chan->reg_offs + RCH_LEN, aml_desc->data_len);

[ ... ]

> +static irqreturn_t aml_dma_interrupt_handler(int irq, void *dev_id)
> +{
> +	struct aml_dma_dev *aml_dma = dev_id;
> +	struct aml_dma_chan *aml_chan;
> +	struct aml_dma_desc *aml_desc;
> +	u32 done, eoc_done, err, err_l, end;
> +	u32 cpl_data;
> +	int i = 0;

[ ... ]

> +	while (done) {
> +		i = ffs(done) - 1;
> +		regmap_write(aml_dma->regmap, CLEAR_RCH, BIT(i));
> +		done &= ~BIT(i);
> +		aml_chan = aml_dma->aml_rch[i];
> +		if (!aml_chan) {
> +			dev_err(aml_dma->dma_device.dev, "idx %d rch not initialized\n", i);
> +			continue;
> +		}
> +		spin_lock(&aml_chan->vchan.lock);
> +		aml_chan->status = (err & BIT(i)) ? DMA_ERROR : DMA_COMPLETE;

[Severity: High]
Can an interrupt overwrite the paused state?

If an interrupt fires while the channel is paused, this handler unconditionally
overwrites status to DMA_COMPLETE and calls aml_dma_start(). Could this unpause
the hardware and break the pause semantics?

> +		aml_desc = aml_chan->cur_desc;

[ ... ]

> +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;

[ ... ]

> +	ret = devm_request_irq(&pdev->dev, aml_dma->irq, aml_dma_interrupt_handler,
> +			       0, dev_name(&pdev->dev), aml_dma);

[Severity: High]
Can this cause a use-after-free during driver removal?

Since devm_request_irq() is used in aml_dma_probe(), the IRQ remains active
until after aml_dma_remove() completes. 

In aml_dma_remove(), the driver loops and calls tasklet_kill(&aml_chan->vchan.task):

> +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);
> +
> +	for (i = 0; i < MAX_CHAN_ID; i++) {
> +		aml_chan = aml_dma->aml_rch[i];
> +		if (aml_chan)
> +			tasklet_kill(&aml_chan->vchan.task);

But an executing interrupt handler on another CPU can subsequently call
vchan_cookie_complete() and reschedule the tasklet. 

Once remove() returns, devres unregisters the DMA engine, frees the IRQ, and
frees the driver struct via devm_kfree. The tasklet will then run and access
freed memory, causing a use-after-free panic. 

Should the driver explicitly free or disable the IRQ using devm_free_irq()
before calling tasklet_kill()?

> +		aml_chan = aml_dma->aml_wch[i];
> +		if (aml_chan)
> +			tasklet_kill(&aml_chan->vchan.task);
> +	}
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260626-amlogic-dma-v9-0-558d672c4a95@amlogic.com?part=2