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

[sashiko-bot@kernel.org]

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()
--

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