Re: [PATCH v4 5/6] dmaengine: dw-edma: Add delegated channel request helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] The patch references nonexistent constants, functions, and struct fields, which will cause an immediate compilation failure.
- [Medium] The `irq_mode` field is modified without holding `chan->vc.lock` during channel release, violating the locking discipline established during delegation.
--

--- Patch [5]: [PATCH v4 5/6] dmaengine: dw-edma: Add delegated channel request helpers ---
commit 15a8e802b8e938899858a00862b359f85ce5f547
Author: Koichiro Den <den@valinux.co.jp>

dmaengine: dw-edma: Add delegated channel request helpers

This commit adds DW eDMA-specific helpers that request a write/read
hardware channel through DMAengine. It switches the selected endpoint-local
channel to remote interrupt routing after reservation, and restores default
routing upon release.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 89a4c498a17b8..cbf6f539d4369 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
[ ... ]
> @@ -1178,6 +1178,92 @@ int dw_edma_remove(struct dw_edma_chip *chip)
[ ... ]
> +static int dw_edma_delegate_chan(struct dma_chan *dchan)
> +{
> +	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	if (!(chan->dw->chip->flags & DW_EDMA_CHIP_LOCAL))
> +		return -EINVAL;
> +
> +	spin_lock_irqsave(&chan->vc.lock, flags);
> +	if (chan->configured || chan->status != EDMA_ST_IDLE ||
> +	    chan->request != EDMA_REQ_NONE)
> +		ret = -EBUSY;
> +	else
> +		chan->irq_mode = DW_EDMA_CH_IRQ_REMOTE;

[Severity: Critical]
Are we missing a prerequisite patch in this series? The field irq_mode does
not seem to exist in struct dw_edma_chan, and the DW_EDMA_CH_IRQ_REMOTE
macro appears to be undefined, which will cause a build regression here.

> +	spin_unlock_irqrestore(&chan->vc.lock, flags);
> +
> +	return ret;
> +}
[ ... ]
> +void dw_edma_release_delegated_chan(struct dma_chan *dchan, bool quiesce)
> +{
> +	struct dw_edma_chan *chan;
> +
> +	if (!dchan)
> +		return;
> +
> +	chan = dchan2dw_edma_chan(dchan);
> +	if (quiesce)
> +		dw_edma_core_ch_quiesce(chan);
> +	chan->irq_mode = dw_edma_get_default_irq_mode(chan);

[Severity: Critical]
Are there missing prerequisites for this code as well? 
dw_edma_core_ch_quiesce() and dw_edma_get_default_irq_mode() appear to be
undeclared functions in this tree.

[Severity: Medium]
Does chan->irq_mode need to be protected by chan->vc.lock here?

In dw_edma_delegate_chan() above, chan->irq_mode is modified while holding
the chan->vc.lock. Here, it is modified locklessly. 

If quiesce is false, could an ongoing interrupt handler concurrently read
irq_mode while it is being restored?

> +	dma_release_channel(dchan);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710082156.2395844-1-den@valinux.co.jp?part=5