Re: [PATCH v4 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:
- [High] Ignored return value of dw_edma_device_config() causes transfer to proceed with rejected configuration.
- [High] Data races and violation of atomic configuration due to lockless global state mutation in dw_edma_device_prep_config_sg().
--

commit c3c3192e70e9eff9723db89a8944f9898577a1fd
Author: Frank Li <Frank.Li@nxp.com>

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

Pass dma_slave_config to dw_edma_device_transfer() to support atomic
configuration and descriptor preparation when a non-NULL config is
provided to device_prep_config_sg().

Tested-by: Niklas Cassel <cassel@kernel.org>
Signed-off-by: Frank Li <Frank.Li@nxp.com>

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index f7f58b0010e26..ec6f6b1e48256 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c

[ ... ]

> @@ -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);

Is it safe to ignore the return value of dw_edma_device_config() here?

If dw_edma_device_config() fails, for example by returning -EINVAL due to a
peripheral_size mismatch, the code will still call dw_edma_device_transfer()
with the rejected config. 

Since dw_edma_device_config() sets chan->non_ll = false before validating the
peripheral config, a failed call might also leave the channel in an
inconsistent state.

Should this check the return value and return NULL on failure?

Additionally, the commit message mentions this change is to support atomic
configuration. When device_prep_config_sg() is implemented, the dmaengine
core deliberately bypasses chan->lock via dmaengine_prep_config_sg_safe(),
expecting the callback to be re-entrant and thread-safe.

However, dw_edma_device_config() modifies global channel state:

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

If multiple threads prepare descriptors on the same channel concurrently,
won't they data race on these fields? 

Furthermore, dw_edma_device_transfer() still reads the mutated chan->non_ll
to determine bursts_max. Does this break the re-entrancy promised by the safe
API, potentially leading to incorrectly programmed hardware descriptors?

> -	return dw_edma_device_transfer(&xfer);
> +	return dw_edma_device_transfer(&xfer, dw_edma_device_get_config(dchan, config));
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260506-dma_prep_config-v4-0-85b3d22babff@nxp.com?part=5