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

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

On Thu, Oct 01, 2026 at 02:27:02AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
> - [High] dmaengine: atcdmac300: missing interrupt enable flags in channel control
> - [High] dmaengine: atcdmac300: start unissued pending transfers in atcdmac_resume()
> - [High] dmaengine: atcdmac300: integer overflow of cyclic period variable
> - [High] dmaengine: atcdmac300: missing status clearing in atcdmac_terminate_all()
> - [High] dmaengine: atcdmac300: fail to reset chan_used flag during suspend
> - [High] dmaengine: atcdmac300: unhandled pending interrupt causes IRQ storm
> - [Medium] dmaengine: atcdmac300: ignore client transfer flags in atcdmac_prep_dma_memcpy()
> - [Medium] dmaengine: atcdmac300: memory leak of unsubmitted descriptors
> - [Medium] dmaengine: atcdmac300: integer truncation of len in atcdmac_prep_dma_memcpy()
> - [Medium] dmaengine: atcdmac300: integer truncation of sizes in atcdmac_prep_dma_cyclic()
> - [Medium] dmaengine: atcdmac300: missing args_count check in atcdmac_dma_xlate_handler()
> --

You should fix these issues for new drivers.

Frank
>
> commit 0b5c5b75d701997e0656ae1d35e0499a595165ad
> 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.
>
> > --- /dev/null
> > +++ b/drivers/dma/atcdmac300.c
> > @@ -0,0 +1,1759 @@
> [ ... ]
> > +static void atcdmac_start_transfer(struct atcdmac_chan *dmac_chan,
> > +				   struct atcdmac_desc *first_desc)
> > +{
> > +	struct atcdmac_dmac *dmac = dmac_chan->dma_dev;
> > +	struct regmap *reg = dmac_chan->regmap;
> > +	unsigned long flags;
> > +
> > +	raw_spin_lock_irqsave(&dmac->lock, flags);
> > +	dmac->owned_chan |= BIT(dmac_chan->chan_id);
> > +	raw_spin_unlock_irqrestore(&dmac->lock, flags);
> > +
> > +	regmap_write(reg, REG_CH_CTL_OFF, first_desc->regs.ctrl);
>
> [Severity: High]
> Since INT_TC_MASK, INT_ERR_MASK, and INT_ABT_MASK are missing from the ctrl
> bitmask written here (and constructed in functions like
> atcdmac_build_slave_desc()), won't the hardware fail to assert interrupts
> upon completion, error, or abort? This appears to cause silent transfer
> timeouts.
>
> [ ... ]
> > +static void atcdmac_advance_work(struct atcdmac_chan *dmac_chan)
> > +{
> [ ... ]
> > +	if (dma_desc->cyclic) {
> > +		if (!stop)
> > +			atcdmac_start_next_trans(dmac_chan);
> [ ... ]
> > +	} else {
> > +		if (list_is_singular(&dmac_chan->active_list)) {
> [ ... ]
> > +		if (!stop)
> > +			atcdmac_start_next_trans(dmac_chan);
>
> [Severity: High]
> If stop_mask is active during atcdmac_suspend(), this code skips calling
> atcdmac_start_next_trans(), which bypasses the internal reset of chan_used
> back to 0. Does this leave chan_used permanently stuck at 1?
>
> [ ... ]
> > +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);
> > +	list_splice_init(&dmac_chan->queue_list,  &list);
> > +	list_splice_init(&dmac_chan->active_list, &list);
> > +	dmac_chan->chan_used = 0;
> > +	spin_unlock_irqrestore(&dmac_chan->lock, flags);
>
> [Severity: High]
> Does this code fail to clear the software state dmac_chan->status before
> dropping the lock? If an asynchronous abort interrupt fires as the channel
> is terminated, the threaded handler may process a stale ATCDMAC_STA_TC bit.
> If a client immediately queues and issues a new transfer, could
> atcdmac_advance_work() prematurely complete it?
>
> [ ... ]
> > +static void atcdmac_free_chan_resources(struct dma_chan *chan)
> > +{
> [ ... ]
> > +	atcdmac_terminate_all(chan);
> > +
> > +	/*
> > +	 * Give up ownership only after the abort raised by the termination
> > +	 * above has been serviced.  Dropping the bit first would mask that
> > +	 * status out of the acknowledge and leave the interrupt asserted.
> > +	 */
> > +	atcdmac_synchronize(chan);
> > +
> > +	raw_spin_lock_irq(&dmac->lock);
> > +	dmac->owned_chan &= ~BIT(dmac_chan->chan_id);
> > +	raw_spin_unlock_irq(&dmac->lock);
>
> [Severity: High]
> Since atcdmac_synchronize() only flushes executing handlers and not pending
> hardware interrupts, does clearing owned_chan immediately afterward mask
> out a pending ABORT interrupt in the top-half handler? It seems this would
> leave the level-triggered interrupt line permanently asserted, causing an
> IRQ storm.
>
> > +
> > +	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]
> In the cleanup loop above, unsubmitted descriptors that were allocated but
> never submitted via dmaengine_submit() are unlinked from tracking lists.
> Will these unsubmitted descriptors leak since this loop only reclaims from
> free_list?
>
> [ ... ]
> > +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);
>
> [Severity: Medium]
> When len is a 64-bit size_t value exceeding 4GB, does shifting it by
> src_width and passing it as a 32-bit unsigned int to atcdmac_build_desc()
> cause silent integer truncation, resulting in a partial transfer?
>
> > +	if (!desc)
> > +		goto err_desc_get;
> > +
> > +	return &desc->txd;
>
> [Severity: Medium]
> Is there a missing assignment for the client transfer flags? The flags
> parameter is never assigned to desc->txd.flags, which ignores client
> intents like DMA_PREP_INTERRUPT or DMA_CTRL_REUSE.
>
> [ ... ]
> > +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)
> > +{
> [ ... ]
> > +	unsigned int period;
> [ ... ]
> > +	for (period = 0; period < buf_len; period += period_len) {
> > +		struct atcdmac_desc *desc;
> > +		dma_addr_t mem = buf_addr + period;
> > +		unsigned int len = min_t(unsigned int, period_len,
> > +					 buf_len - period);
>
> [Severity: High]
> Since buf_len is a 64-bit size_t, does using a 32-bit unsigned int period
> cause an integer overflow for buffer lengths exceeding 4GB? This appears
> to create an infinite loop of descriptor allocations until the pool is
> exhausted.
>
> [Severity: Medium]
> Does the use of min_t(unsigned int, period_len, buf_len - period) silently
> truncate 64-bit size_t sizes over 4GB, programming a malformed transfer
> size?
>
> [ ... ]
> > +static struct dma_chan *atcdmac_dma_xlate_handler(struct of_phandle_args *dmac,
> > +						  struct of_dma *of_dma)
> > +{
> > +	struct platform_device *dmac_pdev;
> > +	struct atcdmac_chan *dmac_chan;
> > +	struct dma_chan *chan;
> > +	dma_cap_mask_t mask;
> > +
> > +	if (dmac->args[0] > FIELD_MAX(SRC_REQ_SEL_MASK)) {
>
> [Severity: Medium]
> Does this code access dmac->args[0] without first verifying
> dmac->args_count? If #dma-cells = <0>, this may read uninitialized or
> out-of-bounds memory.
>
> [ ... ]
> > +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);
> > +		if (!list_empty(&dmac_chan->active_list) ||
> > +		    !list_empty(&dmac_chan->queue_list))
> > +			atcdmac_start_next_trans(dmac_chan);
> > +		spin_unlock_irqrestore(&dmac_chan->lock, flags);
> > +	}
>
> [Severity: High]
> If a client has queued descriptors but not yet called
> dma_async_issue_pending() (leaving chan_used == 0), does unconditionally
> starting the next transaction from queue_list violate the API contract by
> starting pending transfers prematurely?
>
> [Severity: High]
> Additionally, if both active_list and queue_list are empty, this bypasses
> atcdmac_start_next_trans(). Coupled with the issue in
> atcdmac_advance_work() above, does this solidify the leaked chan_used == 1
> state from suspend, permanently locking out the channel?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20261001014503.786490-1-cl634@andestech.com?part=2