Re: [PATCH v5 2/5] dmaengine: arm-dma350: Add slave and cyclic transfer support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dmaengine: arm-dma350: uninitialized sentinel and reset for dch->req
- [Medium] dmaengine: arm-dma350: clearing dch->desc on error in d350_irq() loses error
--

commit 84e756dbc528524c6cc746a33846d36d67c475ad
Author: Jelly Jia <Jelly.Jia@cixtech.com>

dmaengine: arm-dma350: Add slave and cyclic transfer support

This commit adds peripheral scatter-gather and cyclic DMA transfer support to
the arm-dma350 driver, including trigger configuration, coherent command pool
management, period callbacks, and residue calculation.

> diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
> index fdc4cd2245798..cfe3bacfab687 100644
> --- a/drivers/dma/arm-dma350.c
> +++ b/drivers/dma/arm-dma350.c

[ ... ]

> @@ -207,14 +252,489 @@ static inline struct d350_chan *to_d350_chan(struct dma_chan *chan)

[ ... ]

> +static int d350_slave_params(struct d350_chan *dch,
> +			     enum dma_transfer_direction direction,
> +			     phys_addr_t *dev_cpu_addr,
> +			     enum dma_data_direction *dev_dir,
> +			     enum dma_slave_buswidth *width, u32 *maxburst)
> +{
> +	struct dma_slave_config config;
> +	unsigned long flags;
> +	int ret;
> +
> +	spin_lock_irqsave(&dch->vc.lock, flags);
> +	config = dch->sconfig;
> +	spin_unlock_irqrestore(&dch->vc.lock, flags);
> +
> +	ret = d350_check_slave_config(dch, &config);
> +	if (ret)
> +		return ret;
> +	if (dch->req >= d350_chan_to_dmac(dch)->nreq)
> +		return -EINVAL;

[Severity: Medium]
Can this check in d350_slave_params() ever evaluate to true?

In d350_probe(), dmac is allocated with devm_kzalloc(), which zero-initializes
dch->req to 0 for all channels. Because d350_check_slave_config() already
verifies d350_slave_supported(dch) (which requires nreq > 0), and the only
place that updates dch->req is d350_of_xlate() after checking req < dmac->nreq,
dch->req is always less than dmac->nreq. If a channel is requested without
d350_of_xlate(), slave and cyclic transfers will silently use hardware trigger
request line 0 instead of being rejected here.

In addition, d350_free_chan_resources() does not reset dch->req or clear
dch->sconfig when freeing channel resources:

static void d350_free_chan_resources(struct dma_chan *chan)
{
	struct d350_chan *dch = to_d350_chan(chan);

	writel_relaxed(0, dch->base + CH_INTREN);
	d350_terminate_all(chan);
	d350_wait_for_stop(dch);

	free_irq(dch->irq, dch);
	vchan_synchronize(&dch->vc);
	vchan_free_chan_resources(&dch->vc);
}

Should dch->req be initialized to an invalid sentinel such as U32_MAX (or
dmac->nreq) in d350_probe(), and should d350_free_chan_resources() reset
dch->req and clear dch->sconfig so stale request and slave configuration state
is not retained across channel release and reallocation?

[ ... ]

> @@ -487,22 +1193,28 @@ static irqreturn_t d350_irq(int irq, void *data)
>  		else
>  			vd->tx_result.result = DMA_TRANS_ABORTED;
>  
> -		vd->tx_result.residue = d350_get_residue(dch);
> -	} else if (!(ch_status & CH_STAT_INTR_DONE)) {
> -		dev_warn(dev, "Unexpected IRQ source? 0x%08x\n", ch_status);
> -	}
> -	writel_relaxed(ch_status, dch->base + CH_STATUS);
> -
> -	spin_lock(&dch->vc.lock);
> -	vchan_cookie_complete(vd);
> -	if (ch_status & CH_STAT_INTR_DONE) {
> -		dch->status = DMA_COMPLETE;
> -		dch->residue = 0;
> -		d350_start_next(dch);
> -	} else {
> +		residue = d350_get_active_residue(dch);
> +		vd->tx_result.residue = residue;
>  		dch->status = DMA_ERROR;
> -		dch->residue = vd->tx_result.residue;
> +		dch->residue = residue;
> +		dch->desc = NULL;
> +		if (desc->cyclic)
> +			vchan_terminate_vdesc(vd);
> +		else
> +			vchan_cookie_complete(vd);

[Severity: Medium]
Does clearing dch->desc = NULL on the error path in d350_irq() cause error
status to be lost?

When CH_STAT_INTR_ERR is raised for a non-cyclic descriptor, d350_irq() sets
dch->status = DMA_ERROR and dch->desc = NULL, and calls
vchan_cookie_complete(vd), which sets chan->completed_cookie to the failed
descriptor's cookie.

Because dch->desc is now NULL, a subsequent call to d350_issue_pending() sees
!dch->desc and starts the next descriptor:

	spin_lock_irqsave(&dch->vc.lock, flags);
	if (vchan_issue_pending(&dch->vc) && !dch->desc)
		d350_start_next(dch);
	spin_unlock_irqrestore(&dch->vc.lock, flags);

d350_start_next() overwrites dch->cookie and sets dch->status =
DMA_IN_PROGRESS. If the consumer then queries d350_tx_status() for the failed
cookie, dma_cookie_status() returns DMA_COMPLETE, cookie == dch->cookie is
false, and d350_find_queued_desc() returns NULL, so d350_tx_status() reports
DMA_COMPLETE with a residue of 0 for the failed transfer.

Clearing dch->desc in d350_irq() also prevents d350_terminate_all() from
resetting dch->status back to DMA_COMPLETE:

	if (dch->desc) {
		if (dch->status != DMA_ERROR)
			vchan_terminate_vdesc(&dch->desc->vd);
		dch->desc = NULL;
		dch->status = DMA_COMPLETE;
	}

Should dch->desc remain set (or should d350_issue_pending() and
d350_terminate_all() check dch->status == DMA_ERROR) so that the error state
is preserved until the channel is terminated or queried?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104124411.296640-1-Jelly.Jia@cixtech.com?part=2