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

[Frank Li <Frank.li@oss.nxp.com>]

On Fri, Sep 11, 2026 at 07:57:12AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
> - [Critical] Double-callback and Use-After-Free race in cyclic transfer completion during channel termination.
> - [High] Masking pending hardware interrupts with software state (`used_chan`) causes an unacknowledged IRQ storm.
> - [High] The DMA device is exposed to clients before its IRQ handler and cache coherency (IOCP) are fully initialized.
> - [High] Polling for up to 100ms with interrupts disabled in `atcdmac_terminate_all` causes soft lockups.
> - [High] The channel-wide `cyclic` flag is sticky, and unconditional resume restarts cause arbitrary DMA execution or kernel panics.
> - [High] Suspend/resume race with the threaded IRQ causes transfer desynchronization and hardware corruption.
> - [High] dmaengine API violation in `atcdmac_terminate_all`: invoking completion callbacks for aborted transfers.
> - [High] System suspend times out and fails if a slave DMA channel is actively waiting for peripheral data.
> - [High] Sleep-in-atomic bug on PREEMPT_RT due to using `spinlock_t` in hardirq context.
> - [High] Kernel panic due to list_del_init() on an uninitialized list_head in error paths.
> - [Medium] Writing a blanket clear mask to `REG_INT_STA` clears unhandled interrupts, permanently losing them.
> --

Please fix these problems. I will start review after fix sashiko report
issues.

Frank

>
> commit e06d7bd262ebaa03646e2e3ac2876918f25083d4
> 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 is a memory-to-memory and peripheral DMA controller
> that provides scatter-gather, cyclic, and slave transfer capabilities.
>
> > diff --git a/drivers/dma/atcdmac300.c b/drivers/dma/atcdmac300.c
> > new file mode 100644
> > --- /dev/null
> > +++ b/drivers/dma/atcdmac300.c
> [ ... ]
> > +static struct atcdmac_desc *atcdmac_alloc_desc(struct dma_chan *chan,
> > +					       gfp_t gfp_flags)
> > +{
> > +	struct atcdmac_dmac *dmac = atcdmac_dev_to_dmac(chan->device);
> > +	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: High]
> Does this code leave desc_node uninitialized?
>
> dma_pool_zalloc zeroes out the descriptor, leaving the pointers for desc_node
> as NULL since INIT_LIST_HEAD is not called for it. If a subsequent descriptor
> allocation fails (for instance, in atcdmac_prep_device_sg), the error path
> calls atcdmac_put_desc on the first descriptor.
>
> This descends into atcdmac_put_desc_nolock, which performs list_del_init on
> desc_node, potentially causing a NULL pointer dereference on the
> uninitialized list head.
>
> [ ... ]
> > +static void atcdmac_put_desc_nolock(struct atcdmac_chan *dmac_chan,
> > +				    struct atcdmac_desc *desc)
> > +{
> [ ... ]
> > +		list_del_init(&desc->desc_node);
>
> [Severity: High]
> Can this trigger a NULL pointer dereference as described above?
>
> [ ... ]
> > +static void atcdmac_start_next_trans(struct atcdmac_chan *dmac_chan)
> > +{
> > +	struct atcdmac_desc *next_tx = NULL;
> > +	struct atcdmac_desc *dma_desc;
> > +
> > +	if (dmac_chan->cyclic) {
> > +		/* Get the next DMA descriptor from tx_list. */
> > +		dma_desc = atcdmac_get_active_head(dmac_chan);
> > +		dma_desc->at = dma_desc->at->next;
>
> [Severity: High]
> Could this dereference an invalid pointer if the active_list is empty?
>
> The channel-wide cyclic flag is sticky and is never cleared when a transfer
> is terminated. If atcdmac_start_next_trans is called (for example, during
> system resume) on a previously cyclic channel that is now idle, active_list
> will be empty.
>
> Using list_first_entry via atcdmac_get_active_head on an empty list returns
> a bogus pointer, leading to a fault when dma_desc->at is accessed.
>
> [ ... ]
> > +static void atcdmac_advance_work(struct atcdmac_chan *dmac_chan)
> > +{
> [ ... ]
> > +	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: Critical]
> Is there a use-after-free race here if atcdmac_terminate_all runs
> concurrently?
>
> dma_desc remains in the unlocked active_list while the channel lock is dropped
> to invoke the client callback. If another thread calls atcdmac_terminate_all,
> it will splice active_list, abort the channel, manually run the callback on
> dma_desc again, and return it to the free_list.
>
> Another CPU can then immediately reallocate the descriptor and overwrite the
> callback pointers while the first CPU is still reading them to execute, which
> could lead to arbitrary code execution.
>
> [Severity: High]
> Can this delayed threaded IRQ cause a double-start race with system resume?
>
> If a transfer finishes right before suspend, the hardware interrupt fires
> but the threaded IRQ handler may not execute before the system freezes. On
> resume, stop_mask is cleared and the transfer is restarted. When normal
> operations resume, this delayed threaded IRQ runs, finds stop cleared, and
> erroneously double-starts the transfer while it is already actively running.
>
> [ ... ]
> > +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);
>
> [Severity: High]
> Does masking hardware interrupts with the software used_chan state risk an
> unacknowledged IRQ storm?
>
> If an interrupt fires for a channel where the software used_chan bit is 0
> (such as an abort interrupt triggered by atcdmac_terminate_all on an idle
> channel), int_ch evaluates to 0.
>
> The handler bypasses the clear routine and returns without acknowledging the
> hardware interrupt, potentially causing an infinite loop of interrupts.
>
> > +
> > +	while (int_ch) {
> > +		spin_lock(&dmac->lock);
>
> [Severity: High]
> Is it safe to use a standard spinlock_t in hardirq context here?
>
> On PREEMPT_RT kernels, spinlock_t is implemented as an RT mutex which can
> sleep. Locks acquired in genuine hardware interrupt context typically need
> to be typed as raw_spinlock_t to avoid scheduling while atomic.
>
> > +		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));
>
> [Severity: Medium]
> Does writing a blanket clear mask erase unhandled interrupts?
>
> The DMA_INT_CLR macro writes 1s to the TC, ABT, and ERR bits for the given
> channel, regardless of which interrupts were actually set in the status read.
> If a new interrupt (like ERR) asserts after the status read but before this
> clear write, it gets cleared but never handled.
>
> [ ... ]
> > +static int atcdmac_terminate_all(struct dma_chan *chan)
> > +{
> [ ... ]
> > +	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]
> Can this cause a soft lockup by polling for up to 100ms with interrupts
> disabled?
>
> Calling regmap_read_poll_timeout_atomic with a 100,000us timeout while
> holding a spinlock in hardirq-disabled context can trigger the NMI watchdog
> and create extreme system latency spikes.
>
> [ ... ]
> > +	list_for_each_entry_safe(desc_cur, desc_next, &list, desc_node) {
> > +		atcdmac_run_tx_complete_actions(desc_cur, DMA_TRANS_ABORTED);
>
> [Severity: High]
> Is it permissible to invoke completion callbacks for aborted transfers?
>
> The dmaengine API explicitly prohibits executing callbacks for aborted
> descriptors because clients (such as ALSA) typically call terminate_all with
> their own stream locks held. Invoking the callback here can cause a deadlock
> when the client attempts to acquire the exact same lock.
>
> [ ... ]
> > +static int atcdmac_probe(struct platform_device *pdev)
> > +{
> [ ... ]
> > +	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 = atcdmac_init_iocp(pdev, dmac);
>
> [Severity: High]
> Are the DMA channels exposed to clients before the IRQ handler and cache
> coherency (IOCP) are fully initialized?
>
> dma_async_device_register makes the DMA channels immediately available to
> kernel clients. If a client immediately requests a channel and starts a
> transfer before atcdmac_init_irq and atcdmac_init_iocp execute, the transfer
> will run without cache snooping configured, which could lead to memory
> corruption.
>
> [ ... ]
> > +static int atcdmac_resume(struct device *dev)
> > +{
> [ ... ]
> > +	list_for_each_entry_safe(chan,
> > +				 chan_next,
> > +				 &dmac->dma_device.channels,
> > +				 device_node) {
> > +		dmac_chan = atcdmac_chan_to_dmac_chan(chan);
> > +		spin_lock_irqsave(&dmac_chan->lock, flags);
> > +		atcdmac_start_next_trans(dmac_chan);
>
> [Severity: High]
> Could this improperly restart idle cyclic channels?
>
> This unconditionally calls atcdmac_start_next_trans for every channel
> regardless of its active software state. If a channel was previously cyclic
> but is currently idle, it will cause a fault as described earlier when
> dereferencing the empty active_list.
>
> [ ... ]
> > +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);
>
> [Severity: High]
> Will system suspend time out and fail if a slave DMA channel is actively
> waiting for peripheral data?
>
> The suspend sequence sets a stop_mask that prevents software from chaining the
> next descriptor, but does not pause actively running transfers. It then blocks
> waiting for all channels to report idle.
>
> If a DEV_TO_MEM slave channel is waiting for hardware flow control or
> incoming data, it will never complete. The polling loop will eventually time
> out, aborting the entire system sleep transition.
>
> [ ... ]
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260911072524.1297406-1-cl634@andestech.com?part=2