Re: [PATCH v4 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] Missing `.remove` callback leads to memory leaks and Use-After-Free when the driver is unbound via sysfs.
- [High] Strict power-of-two validation on all address widths and burst sizes breaks standard DMA clients.
- [High] Hardware interrupts are incorrectly masked by a software state variable before clearing, leading to an unhandled interrupt storm and system lockup.
- [High] The channel's `cyclic` state flag is never cleared, causing invalid pointer dereferences on resume or when subsequently reused for non-cyclic transfers.
- [High] The DMA device is registered and exposed to clients before its interrupt handler and hardware coherence (IOCP) are fully initialized.
- [High] Write-1-to-Clear (W1C) race condition in `atcdmac_interrupt` unconditionally clears all interrupt types, losing concurrently triggered unhandled interrupts.
- [High] Lack of synchronization between the custom threaded IRQ and channel teardown causes list corruption and memory leaks.
- [High] If `atcdmac_suspend` times out waiting for channels to go idle, it permanently bricks the DMA controller by leaving `stop_mask` fully asserted.
- [High] The primary interrupt handler acquires a standard `spinlock_t` and performs MMIO regmap operations, which will cause fatal sleep-in-atomic bugs on PREEMPT_RT.
- [High] In `atcdmac_irq_thread()`, handling Terminal Count (TC) before Error (ERR) falsely reports failed transactions as successful and aborts innocent queued transactions.
- [High] System resume blindly restarts the head of `active_list` for non-cyclic channels, which will erroneously re-execute completed transactions if the IRQ thread was delayed prior to suspend.
- [Medium] atcdmac_prep_dma_memcpy ignores user-provided dmaengine flags.
--

commit 717d2125491de1fc8bc4cf0c28112507cb655dda
Author: CL Wang <cl634@andestech.com>
Subject: 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.h b/drivers/dma/atcdmac300.h
> --- /dev/null
> +++ b/drivers/dma/atcdmac300.h
[ ... ]
> +#define DMA_TC_FIELD		GENMASK(TC_OFFSET + 7, TC_OFFSET)
> +#define DMA_ABT_FIELD		GENMASK(ABT_OFFSET + 7, ABT_OFFSET)
> +#define DMA_ERR_FIELD		GENMASK(ERR_OFFSET + 7, ERR_OFFSET)
> +#define DMA_INT_ALL(val)	(FIELD_GET(DMA_TC_FIELD, (val)) |	\
> +				 FIELD_GET(DMA_ABT_FIELD, (val)) |	\
> +				 FIELD_GET(DMA_ERR_FIELD, (val)))
> +#define DMA_INT_CLR(val)	(FIELD_PREP(DMA_TC_FIELD, (val)) |	\
> +				 FIELD_PREP(DMA_ABT_FIELD, (val)) |	\
> +				 FIELD_PREP(DMA_ERR_FIELD, (val)))

[Severity: High]
Does this Write-1-to-Clear (W1C) mask unconditionally clear all three event
sources for the interrupting channel?

If an ERR or ABT event occurs in the microscopic window between reading the
status register and writing this mask back in the interrupt handler, will
those unread events be permanently lost?

> diff --git a/drivers/dma/atcdmac300.c b/drivers/dma/atcdmac300.c
> --- /dev/null
> +++ b/drivers/dma/atcdmac300.c
[ ... ]
> +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]
If the channel was previously used for cyclic transfers but is now being used
with non-cyclic transfers or an empty list, could dma_desc->at be NULL or
uninitialized here?

The cyclic state flag is set in atcdmac_prep_dma_cyclic() but doesn't seem to
be cleared when preparing other transfer types or freeing resources, which
might lead to a NULL pointer dereference.

[ ... ]
> +static irqreturn_t atcdmac_irq_thread(int irq, void *dev_id)
> +{
> +	struct atcdmac_dmac *dmac = dev_id;
> +	struct atcdmac_chan *dmac_chan;
> +	int i;
> +	bool handled = false;
> +
> +	for (i = 0; i < dmac->num_ch; i++) {
> +		dmac_chan = &dmac->chan[i];
> +
> +		if (test_and_clear_bit(ATCDMAC_STA_TC, &dmac_chan->status)) {
> +			atcdmac_advance_work(dmac_chan);
> +			handled = true;
> +		}
> +
> +		if (test_and_clear_bit(ATCDMAC_STA_ERR, &dmac_chan->status)) {
> +			atcdmac_handle_error(dmac_chan);
> +			handled = true;
> +		}

[Severity: High]
Does evaluating the Terminal Count (TC) status before the Error (ERR) status
cause misattribution of hardware errors?

If both bits are set on the final beat of a transfer, atcdmac_advance_work()
runs first, marks the current transaction as successful, and starts the next
one. Then the ERR block executes and aborts the innocent transaction that just
started.

[ ... ]
> +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);
> +		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: High]
Are hardware interrupts dangerously masked by the used_chan software state?

If atcdmac_terminate_all() aborts a channel, the hardware asserts an ABORT
interrupt. If used_chan was already cleared, int_ch evaluates to 0.

Will the handler skip the clear loop and return IRQ_NONE, leaving the interrupt
line asserted and trapping the CPU in a hardirq loop?

[Severity: High]
Additionally, this hardirq handler acquires a standard spinlock_t and performs
regmap I/O operations.

Since the regmap is not configured with use_raw_spinlock, it defaults to a
sleepable spinlock internally. Will this cause sleep-in-atomic crashes on
PREEMPT_RT kernels?

[ ... ]
> +static struct dma_async_tx_descriptor *
> +atcdmac_prep_dma_memcpy(struct dma_chan *chan,
> +			dma_addr_t dst,
> +			dma_addr_t src,
> +			size_t len,
> +			unsigned long flags)
> +{
[ ... ]
> +	desc = atcdmac_build_desc(dmac_chan, src, dst, ctrl,
> +				  len >> src_width, 1);
> +	if (!desc)
> +		goto err_desc_get;
> +
> +	return &desc->txd;

[Severity: Medium]
Are the user-provided dmaengine flags ignored here?

It appears the flags argument is never assigned to desc->txd.flags before
returning, which means client requests like DMA_PREP_INTERRUPT or DMA_CTRL_ACK
will not be evaluated.

[ ... ]
> +static struct dma_async_tx_descriptor *
> +atcdmac_prep_dma_cyclic(struct dma_chan *chan,
> +			dma_addr_t buf_addr,
> +			size_t buf_len,
> +			size_t period_len,
> +			enum dma_transfer_direction direction,
> +			unsigned long flags)
> +{
[ ... ]
> +	first->txd.flags = flags;
> +	dmac_chan->cyclic = true;
> +
> +	return &first->txd;

[Severity: High]
As mentioned earlier, does this flag ever get cleared when the channel is
repurposed for non-cyclic transfers?

[ ... ]
> +static int atcdmac_set_device_config(struct dma_chan *chan,
> +				     struct dma_slave_config *sconfig)
> +{
> +	struct atcdmac_chan *dmac_chan = atcdmac_chan_to_dmac_chan(chan);
> +
> +	/* Check if this chan is configured for device transfers */
> +	if (!dmac_chan->dev_chan)
> +		return -EINVAL;
> +
> +	/* Must be powers of two according to ATCDMAC300 spec */
> +	if (!is_power_of_2(sconfig->src_maxburst) ||
> +	    !is_power_of_2(sconfig->dst_maxburst) ||
> +	    !is_power_of_2(sconfig->src_addr_width) ||
> +	    !is_power_of_2(sconfig->dst_addr_width))
> +		return -EINVAL;

[Severity: High]
Does this unconditional check on all fields break standard DMA clients?

For unidirectional transfers (like DMA_MEM_TO_DEV), clients typically leave the
opposite direction's fields zeroed. Since is_power_of_2(0) returns false, will
this incorrectly reject valid configurations?

[ ... ]
> +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,

[Severity: High]
Is there a race condition here with the threaded IRQ?

If a client frees a channel concurrently while the IRQ thread is executing a
callback, this function will clear out free_list without calling
synchronize_irq().

Once the threaded IRQ finishes its callback, it calls atcdmac_put_desc(),
adding the descriptor back to the supposedly empty free_list, leading to memory
leaks and list corruption.

[ ... ]
> +static int atcdmac_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	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]
Is it safe to register the DMA device and expose it to clients before its
interrupt handler and hardware coherence (IOCP) are initialized?

If a client requests a channel and initiates a transfer during this window,
could the hardware perform non-coherent memory accesses and assert interrupts
before the handler is attached?

[Severity: Critical]
Additionally, since the device is registered with non-devm functions, does the
lack of a .remove callback allow a privileged user to unbind the driver via
sysfs?

This would trigger devm cleanups while leaving the DMA controller actively
registered in the core dmaengine list, leading to a Use-After-Free on
subsequent DMA requests.

[ ... ]
> +static int atcdmac_resume(struct device *dev)
> +{
[ ... ]
> +	spin_lock_irqsave(&dmac->lock, flags);
> +	dmac->stop_mask = 0;
> +	spin_unlock_irqrestore(&dmac->lock, flags);
> +	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);
> +		spin_unlock_irqrestore(&dmac_chan->lock, flags);
> +	}
> +
> +	return 0;
> +}

[Severity: High]
Does blindly restarting the head of active_list here risk re-executing
completed transactions?

If a non-cyclic transfer completes just before suspend, the hardirq fires but
the threaded IRQ might not have run before tasks were frozen.

The completed descriptor remains on active_list, and this resume path will
mistakenly pop it and submit it to the hardware again.

[ ... ]
> +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]
If this function times out waiting for channels to go idle, it returns an
error, which causes the PM core to abort suspend without calling resume.

Will leaving stop_mask fully asserted silently drop all future transactions and
permanently brick the DMA controller?

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