Re: [PATCH v6 5/9] dmaengine: dw-edma: Pass dma_slave_config to dw_edma_device_transfer()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The patch fails to provide atomic configuration because dw_edma_device_prep_config_sg() calls dw_edma_device_config(), mutating global channel state without synchronization.

Pre-existing issues:
- [High] Channel control functions like dw_edma_device_pause() modify state without vc.lock, causing a TOCTOU race that permanently hangs the channel.
--

commit 248bbd394732f32660b56f83243ae2005c3e1952
Author: Frank Li <Frank.Li@nxp.com>

dmaengine: dw-edma: Pass dma_slave_config to dw_edma_device_transfer()

This commit passes dma_slave_config to dw_edma_device_transfer() to enable
atomic configuration and descriptor preparation when a non-NULL config is
provided to device_prep_config_sg().

Tested-by: Niklas Cassel <cassel@kernel.org>
Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 92572dd8131e6..ba37bc983dcd2 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
[ ... ]
> @@ -267,6 +267,20 @@ static int dw_edma_device_config(struct dma_chan *dchan,
>  	return 0;
>  }
>  
> +static struct dma_slave_config *
> +dw_edma_device_get_config(struct dma_chan *dchan,
> +			  struct dma_slave_config *config)
> +{
> +	struct dw_edma_chan *chan;
> +
> +	if (config)
> +		return config;
> +
> +	chan = dchan2dw_edma_chan(dchan);
> +
> +	return &chan->config;
> +}
> +
>  static int dw_edma_device_pause(struct dma_chan *dchan)
>  {
>  	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);

[Severity: High]
This is a pre-existing issue, but does this function have a TOCTOU race
condition?

Because chan->status and chan->request are checked and modified without
acquiring chan->vc.lock:

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_device_pause() {
    ...
    if (!chan->configured)
        err = -EPERM;
    else if (chan->status != EDMA_ST_BUSY)
        err = -EPERM;
    else if (chan->request != EDMA_REQ_NONE)
        err = -EPERM;
    else
        chan->request = EDMA_REQ_PAUSE;
    ...
}

Could the interrupt handler dw_edma_done_interrupt() run concurrently
(with vc.lock held) and transition chan->status to EDMA_ST_IDLE before
chan->request is set to EDMA_REQ_PAUSE?

If that happens, the channel would be left in an invalid state (IDLE with a
PAUSE request), which would cause subsequent calls to dw_edma_device_resume()
and dw_edma_device_issue_pending() to fail permanently.

[ ... ]
> @@ -595,7 +610,7 @@ dw_edma_device_prep_config_sg(struct dma_chan *dchan, struct scatterlist *sgl,
>  	if (config && dw_edma_device_config(dchan, config))
>  		return NULL;
>  
> -	return dw_edma_device_transfer(&xfer);
> +	return dw_edma_device_transfer(&xfer, dw_edma_device_get_config(dchan, config));
>  }

[Severity: High]
Is a lockless mutation of global channel state still occurring here?

The commit message mentions supporting atomic configuration, but
dw_edma_device_config() is still called here before dw_edma_device_transfer().

Looking at dw_edma_device_config(), it modifies chan->non_ll and performs
a lockless memcpy over chan->config:

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_device_config() {
    ...
    if (cfg_non_ll || non_ll)
        chan->non_ll = true;
    ...
    memcpy(&chan->config, config, sizeof(*config));
    chan->configured = true;
    ...
}

Will concurrent calls to prepare descriptors data race on chan->config and
chan->non_ll, corrupting the transfer configuration?

Additionally, dw_edma_device_transfer() still incorrectly relies on the
mutated chan->non_ll to calculate bursts_max.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520-dma_prep_config-v6-0-06e49b7acb38@nxp.com?part=5