Re: [PATCH v3 2/3] dmaengine: atcdmac300: Add driver for Andes ATCDMAC300 DMA controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider:
- [Critical] Uninitialized `list_head` in dynamically allocated descriptors causes a NULL pointer dereference in error paths.
- [Critical] Reused DMA descriptors do not have their linked list pointers cleared, potentially causing wild DMA accesses.
- [High] The driver registers the DMA device with the dmaengine and OF frameworks before completing hardware and IRQ initialization.
- [High] The interrupt handler masks the hardware interrupt status with `used_chan` before clearing, causing unexpected interrupts to remain permanently asserted.
- [High] `atcdmac_terminate_all()` fails to clear pending hardware interrupts, allowing stale interrupts to erroneously corrupt newly submitted transfers.
- [High] The driver omits a `remove` function, causing a severe use-after-free if the device is unbound via sysfs.
- [High] A race condition in `atcdmac_advance_work()` causes a use-after-free when invoking callbacks for cyclic transfers.
- [High] Spinning with interrupts disabled for up to 100ms during `atcdmac_terminate_all()` can cause severe system lockups.
- [High] Acquiring a `spinlock_t` in the primary hardware interrupt handler violates PREEMPT_RT locking rules.
- [High] Cyclic transfers are implemented using software polling and restarts instead of hardware linked lists, introducing unacceptable latency.
- [High] If the wait timeout expires during suspend, the DMA controller is left permanently disabled because `dmac->stop_mask` is never restored.
- [Medium] Descriptors are leaked during channel teardown if they are not in the free list.
--

commit e6e8d9a6da52e397472687ec989dd87d328e0650
Author: CL Wang <cl634@andestech.com>

dmaengine: atcdmac300: Add driver for Andes ATCDMAC300 DMA controller

This patch adds support for the Andes ATCDMAC300 DMA controller.
The ATCDMAC300 provides scatter-gather, cyclic, and slave transfer
capabilities.

> diff --git a/drivers/dma/atcdmac300.c b/drivers/dma/atcdmac300.c
> index 0000000000000..367a920cd001c
> --- /dev/null
> +++ b/drivers/dma/atcdmac300.c

[ ... ]

> @@ -76,6 +76,21 @@ static struct atcdmac_desc *atcdmac_alloc_desc(struct dma_chan *chan,
>  	struct atcdmac_desc *desc;
>  	dma_addr_t phys;
>  
>  	desc = dma_pool_zalloc(dmac->dma_desc_pool, gfp_flags, &phys);
>  	if (desc) {
>  		INIT_LIST_HEAD(&desc->tx_list);
>  		dma_async_tx_descriptor_init(&desc->txd, chan);

[Severity: Critical]
Does this leave the desc_node list uninitialized? If an error occurs
during transfer preparation, the cleanup routines might access uninitialized
pointers and cause a kernel panic. Should INIT_LIST_HEAD(&desc->desc_node)
be added here?

>  		desc->txd.flags = DMA_CTRL_ACK;
>  		desc->txd.tx_submit = atcdmac_tx_submit;
>  		desc->txd.phys = phys;
>  	}
>  
>  	return desc;
>  }

[ ... ]

> @@ -215,6 +215,22 @@ static void atcdmac_chain_desc(struct atcdmac_desc **first,
>  			       struct atcdmac_desc *desc,
>  			       bool cyclic)
>  {
>  	if (!(*first)) {
>  		*first = desc;
>  		desc->at = &desc->tx_list;
>  	} else {
>  		if (!cyclic) {
>  			(*prev)->regs.ll_ptr_lo =
>  				lower_32_bits(desc->txd.phys);
>  			(*prev)->regs.ll_ptr_hi =
>  				upper_32_bits(desc->txd.phys);
>  		}

[Severity: High]
Is hardware linked list chaining skipped for cyclic transfers here?
By leaving the linked list pointers zeroed during cyclic transfers, the
hardware stops at the end of each period. Does this require the CPU
interrupt handler to manually restart the channel, potentially introducing
audio dropouts and scheduling latency for time-sensitive clients?

>  		list_add_tail(&desc->desc_node, &(*first)->tx_list);
>  	}
>  	*prev = desc;
>  
>  	desc->regs.ll_ptr_hi = 0;
>  	desc->regs.ll_ptr_lo = 0;
>  }

[ ... ]

> @@ -345,6 +345,43 @@ static void atcdmac_advance_work(struct atcdmac_chan *dmac_chan)
>  	struct dmaengine_result res;
>  	LIST_HEAD(completed);
>  	unsigned long flags;
>  	unsigned short stop;
>  
>  	spin_lock_irqsave(&dmac_chan->lock, flags);
>  	if (list_empty(&dmac_chan->active_list)) {
>  		spin_unlock_irqrestore(&dmac_chan->lock, flags);
>  		return;
>  	}
>  
>  	dma_desc = atcdmac_get_active_head(dmac_chan);
>  	stop = READ_ONCE(dmac->stop_mask) & BIT(dmac_chan->chan_id);
>  	if (dmac_chan->cyclic) {
>  		if (!stop)
>  			atcdmac_start_next_trans(dmac_chan);
>  
>  		spin_unlock_irqrestore(&dmac_chan->lock, flags);
>  		res.result = DMA_TRANS_NOERROR;
>  		dmaengine_desc_get_callback_invoke(&dma_desc->txd, &res);

[Severity: High]
Could this lead to a race condition? The lock is dropped while dma_desc
is still in the active_list. If dmaengine_terminate_all is called from
another CPU, could it free the descriptor and allow it to be reallocated
before the callback pointer is evaluated, potentially executing an
overwritten callback?

>  	} else {
>  		if (list_is_singular(&dmac_chan->active_list)) {
>  			list_splice_init(&dmac_chan->active_list, &completed);
>  			list_splice_init(&dmac_chan->queue_list,
>  					 &dmac_chan->active_list);

[ ... ]

> @@ -472,6 +472,44 @@ static irqreturn_t atcdmac_interrupt(int irq, void *dev_id)
>  	struct atcdmac_dmac *dmac = dev_id;
>  	struct atcdmac_chan *dmac_chan;
>  	unsigned int status;
>  	unsigned int int_ch;
>  	int ret = IRQ_NONE;
>  	int i;
>  
>  	regmap_read(dmac->regmap, REG_INT_STA, &status);
>  	int_ch = READ_ONCE(dmac->used_chan) & DMA_INT_ALL(status);
>  
>  	while (int_ch) {
>  		spin_lock(&dmac->lock);

[Severity: High]
Does acquiring this spinlock in the primary hardirq handler cause issues
on PREEMPT_RT kernels? Because spinlock_t is a preemptible sleepable lock
on RT, acquiring it in hardirq context can trigger lockdep splats and
deadlocks. Should dmac->lock be converted to a raw_spinlock_t?

[Severity: High]
By masking the interrupt status with used_chan, are we ignoring interrupts
from hardware when used_chan is 0? If an unexpected interrupt fires, int_ch
becomes 0 and the loop terminates without clearing the interrupt in
REG_INT_STA. Since the IRQ is level-triggered, could this result in an
infinite interrupt storm?

>  		dmac->used_chan = READ_ONCE(dmac->used_chan) & ~int_ch;
>  		spin_unlock(&dmac->lock);
>  		regmap_write(dmac->regmap, REG_INT_STA, DMA_INT_CLR(int_ch));
>  
>  		for (i = 0; i < dmac->num_ch; i++) {

[ ... ]

> @@ -638,6 +638,23 @@ static struct atcdmac_desc *
>  atcdmac_build_desc(struct atcdmac_chan *dmac_chan,
>  		   dma_addr_t src,
>  		   dma_addr_t dst,
>  		   unsigned int ctrl,
>  		   unsigned int trans_size,
>  		   unsigned int num_sg)
>  {
>  	struct atcdmac_desc *desc;
>  
>  	desc = atcdmac_get_desc(dmac_chan);
>  	if (!desc)
>  		return NULL;
>  
>  	desc->regs.src_addr_lo = lower_32_bits(src);
>  	desc->regs.src_addr_hi = upper_32_bits(src);
>  	desc->regs.dst_addr_lo = lower_32_bits(dst);
>  	desc->regs.dst_addr_hi = upper_32_bits(dst);
>  	desc->regs.ctrl = ctrl;
>  	desc->regs.trans_size = trans_size;
>  	desc->num_sg = num_sg;
>  
>  	return desc;
>  }

[Severity: Critical]
Are the linked list pointers cleared when a descriptor is reused?
atcdmac_build_desc does not reset ll_ptr_lo and ll_ptr_hi. If this
descriptor was previously used in a scatter-gather chain, could the hardware
erroneously follow stale pointers and cause uncontrolled memory corruption?

[ ... ]

> @@ -953,6 +953,30 @@ static int atcdmac_terminate_all(struct dma_chan *chan)
>  	struct atcdmac_chan *dmac_chan = atcdmac_chan_to_dmac_chan(chan);
>  	struct atcdmac_desc *desc_cur, *desc_next;
>  	LIST_HEAD(list);
>  	unsigned long flags;
>  	unsigned int val;
>  	int ret;
>  
>  	spin_lock_irqsave(&dmac_chan->lock, flags);
>  	atcdmac_abort_chan(dmac_chan);
>  	atcdmac_enable_chan(dmac_chan, 0);
>  	ret = regmap_read_poll_timeout_atomic(dmac_chan->dma_dev->regmap,
>  					      REG_CH_EN,
>  					      val,
>  					      !(val & BIT(dmac_chan->chan_id)),
>  					      10,
>  					      ATCDMAC_CHAN_TIMEOUT_US);

[Severity: High]
Is it safe to poll for up to 100ms with local interrupts disabled?
ATCDMAC_CHAN_TIMEOUT_US is 100,000. Busy-waiting this long in an atomic
context can trigger soft lockups and trip the NMI watchdog.

[Severity: High]
Does aborting the channel here leave an unhandled abort interrupt pending?
atcdmac_abort_chan() triggers a hardware abort, but the subsequent
dmac_chan->chan_used = 0 causes the interrupt handler to ignore it. If a
new transfer is submitted later, could the stale abort interrupt fire
immediately, disrupting the new transfer state?

>  	if (ret)
>  		dev_err(atcdmac_chan_to_dev(chan),
>  			"Timed out waiting for channel to disable\n");

[ ... ]

> @@ -1066,6 +1066,32 @@ static void atcdmac_free_chan_resources(struct dma_chan *chan)
>  {
>  	struct atcdmac_chan *dmac_chan = atcdmac_chan_to_dmac_chan(chan);
>  	struct atcdmac_dmac *dmac = atcdmac_dev_to_dmac(chan->device);
>  	struct atcdmac_desc *desc_next, *desc;
>  	unsigned long flags;
>  
>  	WARN_ON_ONCE(atcdmac_is_chan_enable(dmac_chan));
>  
>  	spin_lock_irq(&dmac->lock);
>  	dmac->stop_mask |= BIT(dmac_chan->chan_id);
>  	spin_unlock_irq(&dmac->lock);
>  
>  	atcdmac_terminate_all(chan);
>  
>  	spin_lock_irqsave(&dmac_chan->lock, flags);
>  	list_for_each_entry_safe(desc,
>  				 desc_next,
>  				 &dmac_chan->free_list,
>  				 desc_node) {
>  		list_del(&desc->desc_node);
>  		dma_pool_free(dmac->dma_desc_pool, desc, desc->txd.phys);
>  	}

[Severity: Medium]
Could descriptors be leaked if a client submits a new transfer from within
the completion callback during atcdmac_terminate_all? Since the loop above
only iterates over free_list, any newly added descriptors in queue_list
would not be freed and permanently leaked.

>  
>  	INIT_LIST_HEAD(&dmac_chan->free_list);
>  	dmac_chan->descs_allocated = 0;

[ ... ]

> @@ -1438,6 +1438,40 @@ static int atcdmac_probe(struct platform_device *pdev)
>  	struct atcdmac_dmac *dmac;
>  	int ret;
>  
>  	ret = atcdmac_init_ioremap_and_regmap(pdev, &dmac);
>  	if (ret)
>  		return ret;
>  
>  	ret = atcdmac_reset_and_wait_chan_idle(dmac);
>  	if (ret)
>  		return ret;
>  
>  	ret = atcdmac_init_desc_pool(pdev, dmac);
>  	if (ret)
>  		return ret;
>  
>  	ret = atcdmac_init_channels(pdev, dmac);
>  	if (ret)
>  		return ret;
>  
>  	atcdmac_init_dma_device(pdev, dmac);
>  
>  	ret = dma_async_device_register(&dmac->dma_device);
>  	if (ret)
>  		return ret;
>  
>  	ret = atcdmac_init_irq(pdev, dmac);
>  	if (ret)
>  		goto err_dma_async_register;
>  
>  	ret = of_dma_controller_register(pdev->dev.of_node,
>  					 atcdmac_dma_xlate_handler,
>  					 dmac);
>  	if (ret)
>  		goto err_dma_async_register;
>  
>  	ret = atcdmac_init_iocp(pdev, dmac);
>  	if (ret)
>  		goto err_of_dma_register;

[Severity: High]
Are the frameworks being notified before the device is fully initialized?
dma_async_device_register and of_dma_controller_register expose the device
to clients, but atcdmac_init_irq and atcdmac_init_iocp are called after.
If a client immediately queues a transfer, could it execute before the IRQ
handler is ready, leading to unhandled interrupts?

>  
>  	return 0;

[ ... ]

> @@ -1480,6 +1480,24 @@ static int atcdmac_suspend(struct device *dev)
>  {
>  	struct atcdmac_dmac *dmac = dev_get_drvdata(dev);
>  	int ret;
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&dmac->lock, flags);
>  	dmac->stop_mask = BIT(dmac->num_ch) - 1;
>  	spin_unlock_irqrestore(&dmac->lock, flags);
>  	ret = atcdmac_wait_chan_idle(dmac,
>  				     dmac->stop_mask,
>  				     ATCDMAC_CHAN_TIMEOUT_US * dmac->num_ch);
>  
>  	return ret;
>  }

[Severity: High]
Does a timeout here leave the controller permanently disabled? If
atcdmac_wait_chan_idle times out and returns an error, the PM core skips
the resume callback. Because stop_mask is never cleared, does the DMA
controller reject all future transfers until reboot?

>  
>  static DEFINE_SIMPLE_DEV_PM_OPS(atcdmac_pm_ops,

[ ... ]

> @@ -1498,6 +1498,14 @@ static const struct of_device_id atcdmac_dt_ids[] = {
>  	{ .compatible = "andestech,ae350-dma", },
>  	{ /* sentinel */ },
>  };
>  MODULE_DEVICE_TABLE(of, atcdmac_dt_ids);
>  
>  static struct platform_driver atcdmac_driver = {
>  	.probe = atcdmac_probe,
>  	.driver = {
>  		.name = "atcdmac300",
>  		.of_match_table = atcdmac_dt_ids,
>  		.pm = pm_sleep_ptr(&atcdmac_pm_ops),
>  	},
>  };

[Severity: High]
Is a remove function needed here? Memory is allocated via devm_ functions,
but the DMA controller and dmaengine are registered using non-devm APIs.
If the driver is unbound via sysfs, could this lead to a use-after-free
when clients attempt to access the freed memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527132815.1211195-1-cl634@andestech.com?part=2