Re: [PATCH v3 2/3] dmaengine: dw-edma: Configure remote interrupt routing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Overly restrictive check in dw_edma_device_config_irq_mode unconditionally rejects valid repeated dmaengine_slave_config calls.
--

commit a84fcfcb09487df4c48ca1e301c604e49afc0e26
Author: Koichiro Den <den@valinux.co.jp>

dmaengine: dw-edma: Configure remote interrupt routing

An endpoint function can reserve an endpoint-local channel while the RC
programs it through an exposed register window. Such a channel must route
interrupts remotely and ignore them on the endpoint. Use dma_slave_config
to set per-channel interrupt routing on idle channels of a local
eDMA-compatible instance.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index a678c70a78fe6..e3786d960a431 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
> @@ -177,48 +177,79 @@ dw_edma_get_default_irq_mode(struct dw_edma_chan *chan)
>  						  DW_EDMA_CH_IRQ_REMOTE;
>  }
>  
> +static int dw_edma_device_config_irq_mode(struct dw_edma_chan *chan,
> +					  enum dw_edma_ch_irq_mode mode)
> +{
> +	if (!(chan->dw->chip->flags & DW_EDMA_CHIP_LOCAL) ||
> +	    (mode != DW_EDMA_CH_IRQ_LOCAL && mode != DW_EDMA_CH_IRQ_REMOTE))
> +		return -EINVAL;
> +
> +	guard(spinlock_irqsave)(&chan->vc.lock);
> +
> +	if (chan->configured || chan->status != EDMA_ST_IDLE ||
> +	    chan->request != EDMA_REQ_NONE)
> +		return -EBUSY;

[Severity: High]
Does this unconditionally reject valid repeated dmaengine_slave_config() calls?

A caller that populates dma_slave_config.peripheral_config with
DW_EDMA_CH_CONFIG_IRQ_MODE and calls dmaengine_slave_config() multiple times
to update standard fields (like src_addr or dst_addr) will face an unexpected
-EBUSY error on all calls after the first.

This happens because chan->configured is set to true at the end of
dw_edma_device_config(), so subsequent calls fail here even if the requested
IRQ mode is identical and the channel is idle.

Should this gracefully allow the call if chan->irq_mode == mode?

> +
> +	WRITE_ONCE(chan->irq_mode, mode);
> +
> +	return 0;
> +}
> +
>  static int dw_edma_device_config(struct dma_chan *dchan,
>  				 struct dma_slave_config *config)
>  {

[ ... ]

> +	if (flags & DW_EDMA_CH_CONFIG_IRQ_MODE) {
> +		switch (chan->dw->chip->mf) {
> +		case EDMA_MF_EDMA_LEGACY:
> +		case EDMA_MF_EDMA_UNROLL:
> +		case EDMA_MF_HDMA_COMPAT:
> +			break;
> +		default:
> +			return -EINVAL;
>  		}
>  
> -		if (cfg_non_ll || non_ll)
> -			chan->non_ll = true;
> -	} else if (config->peripheral_config) {
> -		dev_err(dchan->device->dev,
> -			"peripheral config param applicable only for HDMA\n");
> -		return -EINVAL;
> +		ret = dw_edma_device_config_irq_mode(chan, dw_config->irq_mode);
> +		if (ret)
> +			return ret;
>  	}
>  
> +	chan->non_ll = non_ll;
>  	memcpy(&chan->config, config, sizeof(*config));
>  	chan->configured = true;

[Severity: High]
This is where chan->configured is set to true on the first call, triggering
the -EBUSY on all subsequent calls to dw_edma_device_config_irq_mode().

>  
>  	return 0;
>  }

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