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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Unmanaged workqueue `free_wq` leaks on probe errors, and premature tasklet termination in `remove()` causes Use-After-Free due to devres ordering.
- [High] Bitwise ORing a host-endian constant into a little-endian (`__le32`) variable causes data corruption on big-endian architectures.
- [High] Failure to check `platform_get_irq()` return value breaks deferred probing.
- [High] `aml_chan->status` and `aml_chan->err_num` are updated outside the channel lock in `aml_dma_terminate_all()`.
- [High] `aml_dma_chan_resume()` fails to start descriptors that were issued while the channel was paused.
- [Medium] `aml_dma_tx_status()` fails to check the `desc_submitted` list, leading to false errors and incorrect residue.
- [Medium] The interrupt handler unconditionally returns `IRQ_HANDLED`, defeating spurious interrupt detection.
- [Low] Taking the address of a member of a potentially NULL pointer in `aml_dma_terminate_all()`.
--

commit 038e372b66227ebb6d24a3849c98d588ff0edcd7
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
> new file mode 100644
> index 0000000000000..b6687d98096c3
> --- /dev/null
> +++ b/drivers/dma/amlogic-dma.c

[ ... ]

> +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 = 0;
> +	unsigned long flags;
> +	enum dma_status ret;
> +
> +	ret = dma_cookie_status(chan, cookie, txstate);
> +	if (ret == DMA_COMPLETE)
> +		return ret;
> +
> +	if (aml_chan->status == DMA_PAUSED)
> +		ret = DMA_PAUSED;
> +	if (!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) {

[Severity: Medium]
Does this skip checking the desc_submitted list?

When a DMA client submits a descriptor via tx_submit() but has not yet
issued it via issue_pending(), the descriptor resides in desc_submitted.
Since vchan_find_desc() only searches the desc_issued list, calling
dma_tx_status() during this window will fail to find the descriptor.
This causes the driver to incorrectly log a "cookie error" and return a
residue of 0 instead of the full transfer length.

> +		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");
> +	}
> +	spin_unlock_irqrestore(&aml_chan->vchan.lock, flags);
> +	dma_set_residue(txstate, residue);
> +
> +	return ret;
> +}

[ ... ]

> +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)
> +{

[ ... ]

> +		/* set dma address and len to sglink*/
> +		sg_link->addr_low = cpu_to_le32(lower_32_bits(paddr));
> +		sg_link->addr_high = cpu_to_le32(upper_32_bits(paddr));
> +		sg_link->ctl = cpu_to_le32(FIELD_PREP(LINK_LEN, avail));
> +
> +		aml_desc->data_len += sg_dma_len(sg);
> +		sg_link++;
> +	}
> +
> +	/* the last sg set eoc flag */
> +	sg_link--;
> +	sg_link->ctl |= LINK_EOC;

[Severity: High]
Can this cause data corruption on big-endian architectures?

The sg_link->ctl field is populated as an __le32, but the LINK_EOC flag
(which evaluates to a host-endian BIT(28)) is bitwise OR'd directly into it.
On big-endian systems, this will set the wrong bit in the memory-backed
descriptor, meaning the hardware won't see the End-Of-Chain flag and may
fetch uninitialized memory. Wrapping it with cpu_to_le32(LINK_EOC) would
prevent this.

> +
> +	return vchan_tx_prep(&aml_chan->vchan, &aml_desc->vd, flags);
> +}

[ ... ]

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

[Severity: High]
Does aml_dma_chan_resume() fail to start descriptors that were issued while
the channel was paused?

When a channel is paused, aml_chan->status is set to DMA_PAUSED. Any
subsequent calls to aml_dma_issue_pending() append descriptors to the
desc_issued list but return immediately from aml_dma_start() without
triggering the hardware.

Because aml_dma_chan_resume() restores aml_chan->status to DMA_COMPLETE but
does not check if the pending queue is non-empty, descriptors queued during
the pause will stall indefinitely.

> +
> +	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);
> +	regmap_set_bits(aml_dma->regmap, aml_chan->reg_offs + RCH_CFG, CFG_PAUSE);
> +	regmap_set_bits(aml_dma->regmap, aml_chan->reg_offs + RCH_CFG, CFG_CLEAR);
> +
> +	if (aml_chan->direction == DMA_MEM_TO_DEV)
> +		regmap_set_bits(aml_dma->regmap, RCH_INT_MASK, BIT(chan_id));
> +	else if (aml_chan->direction == DMA_DEV_TO_MEM)
> +		regmap_set_bits(aml_dma->regmap, WCH_INT_MASK, BIT(chan_id));
> +
> +	regmap_clear_bits(aml_dma->regmap, aml_chan->reg_offs + RCH_CFG, CFG_PAUSE);
> +	regmap_clear_bits(aml_dma->regmap, aml_chan->reg_offs + RCH_CFG, CFG_CLEAR);
> +
> +	vchan_get_all_descriptors(&aml_chan->vchan, &head);
> +	cur_vd = &aml_chan->cur_desc->vd;
> +	aml_chan->cur_desc = NULL;
> +	spin_unlock_irqrestore(&aml_chan->vchan.lock, flags);

[Severity: Low]
Is this taking the address of a member from a potentially NULL pointer?

If there is no active transfer, aml_chan->cur_desc is NULL. Calculating
&aml_chan->cur_desc->vd before checking for NULL is undefined behavior in
standard C. While it safely evaluates to NULL in the kernel because vd is
at offset 0 and GCC is invoked with -fno-delete-null-pointer-checks, this
diverges from the correct pattern used elsewhere in the driver.

> +	if (cur_vd)
> +		aml_dma_free_desc(cur_vd);
> +
> +	vchan_dma_desc_free_list(&aml_chan->vchan, &head);
> +
> +	aml_chan->status = DMA_COMPLETE;
> +	aml_chan->err_num = 0;

[Severity: High]
Is there a race condition here since aml_chan->status and err_num are
updated outside of the vchan.lock?

If a concurrent aml_dma_issue_pending() acquires the lock immediately after
it is dropped here, it will see the stale aml_chan->status (e.g.,
DMA_PAUSED). aml_dma_start() will then return without touching the hardware.
terminate_all will subsequently set the status to DMA_COMPLETE, but the
newly queued transfer will be permanently stuck.

> +
> +	return 0;
> +}

[ ... ]

> +static irqreturn_t aml_dma_interrupt_handler(int irq, void *dev_id)
> +{

[ ... ]

> +		vchan_cookie_complete(&aml_desc->vd);
> +		aml_chan->cur_desc = NULL;
> +		aml_dma_start(aml_chan);
> +		spin_unlock(&aml_chan->vchan.lock);
> +	}
> +
> +	return IRQ_HANDLED;

[Severity: Medium]
Does this unconditionally return IRQ_HANDLED even when no hardware bits
were set?

If the interrupt handler is invoked but the hardware did not assert the
interrupt, the processing loops are skipped, but the function still returns
IRQ_HANDLED. This violates the IRQ API contract (which requires returning
IRQ_NONE) and prevents the core IRQ subsystem from detecting and disabling
spurious interrupt storms.

> +}

[ ... ]

> +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;
> +
> +	const struct regmap_config aml_regmap_config = {
> +		.reg_bits = 32,
> +		.val_bits = 32,
> +		.reg_stride = 4,
> +		.max_register = 0x3000,
> +	};
> +
> +	ret = of_property_read_u32(np, "dma-channels", &chan_nr);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "failed to read dma-channels\n");
> +	if (chan_nr > (MAX_CHAN_ID * 2))
> +		return dev_err_probe(&pdev->dev, -EINVAL, "dma-channels unusual\n");
> +
> +	len = sizeof(struct aml_dma_dev) + sizeof(struct aml_dma_chan) * chan_nr;
> +	aml_dma = devm_kzalloc(&pdev->dev, len, GFP_KERNEL);
> +	if (!aml_dma)
> +		return -ENOMEM;
> +
> +	aml_dma->free_wq = alloc_ordered_workqueue("aml_dma_free", 0);
> +	if (!aml_dma->free_wq)
> +		return -ENOMEM;
> +
> +	aml_dma->chan_nr = chan_nr;
> +
> +	aml_dma->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(aml_dma->base))
> +		return PTR_ERR(aml_dma->base);

[Severity: High]
Does this leak free_wq on probe errors?

If devm_platform_ioremap_resource() or any subsequent registration fails
during probe, the function returns an error without calling
destroy_workqueue(aml_dma->free_wq), leaking the workqueue memory.

> +
> +	aml_dma->regmap = devm_regmap_init_mmio(&pdev->dev, aml_dma->base,
> +						&aml_regmap_config);
> +	if (IS_ERR_OR_NULL(aml_dma->regmap))
> +		return PTR_ERR(aml_dma->regmap);
> +
> +	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]
Does this break deferred probing by missing a bounds check on the returned
IRQ value?

platform_get_irq() can return a negative error code such as -EPROBE_DEFER.
By unconditionally assigning it to aml_dma->irq and passing it to
devm_request_irq(), the IRQ core will reject the negative IRQ value and
return -EINVAL. This overwrites the -EPROBE_DEFER error and causes a
permanent failure to probe.

> +
> +	dma_dev = &aml_dma->dma_device;

[ ... ]

> +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, INT_MASK);
> +	regmap_write(aml_dma->regmap, WCH_INT_MASK, INT_MASK);
> +
> +	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);
> +	}
> +	destroy_workqueue(aml_dma->free_wq);
> +}

[Severity: High]
Is there a use-after-free race condition during driver unbind?

The tasklets are killed here, but because devm_request_irq() was used
during probe, the IRQ line remains active until devres runs after
aml_dma_remove() returns.

Without a call to synchronize_irq(), an in-flight interrupt on another CPU
could schedule the tasklet again after tasklet_kill() has finished. When
devres subsequently frees the driver's memory, the delayed tasklet will
execute on freed memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903-amlogic-dma-v12-0-f769bc344056@amlogic.com?part=2