Re: [PATCH v4 1/9] dmaengine: Add API to combine configuration and preparation (sg and single)

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The implementation contradicts the commit message regarding the fallback behavior when config is NULL.
- [Low] Device configuration side effects are applied before verifying if the channel supports the preparation callback.
--

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

dmaengine: Add API to combine configuration and preparation (sg and single)

This commit introduces combined API functions for DMA configuration and
preparation to simplify operations where parameters must be adjusted per
transfer. It adds dmaengine_prep_config_single() and
dmaengine_prep_config_sg(), along with a new device_prep_config_sg
callback.

> Previously, configuration and preparation required two separate calls. This
> works well when configuration is done only once during initialization.
>
> However, in cases where the burst length or source/destination address must
> be adjusted for each transfer, calling two functions is verbose and
> requires additional locking to ensure both steps complete atomically.
>
> Add a new API dmaengine_prep_config_single() and dmaengine_prep_config_sg()
> and callback device_prep_config_sg() that combines configuration and
> preparation into a single operation. If the configuration argument is
> passed as NULL, fall back to the existing implementation.

This isn't a bug, but the commit message description doesn't exactly match
the implementation.

Looking at the code, if a driver implements the new device_prep_config_sg
callback, it will be called unconditionally, even if the config argument is
NULL. It does not fall back to the existing implementation in that scenario.

While the code behavior appears structurally correct (since drivers moving
to the new API might remove their old device_prep_slave_sg callback), could
the commit message be updated to clarify this? Clarifying that config can
be NULL might prevent driver authors from assuming it is always valid,
avoiding potential null pointer dereferences.

> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index b3d251c9734e9..defa377d2ef54 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h

[ ... ]

> @@ -974,22 +979,44 @@ static inline bool is_slave_direction(enum dma_transfer_direction direction)
>  	       (direction == DMA_DEV_TO_DEV);
>  }
>  
> -static inline struct dma_async_tx_descriptor *dmaengine_prep_slave_single(
> -	struct dma_chan *chan, dma_addr_t buf, size_t len,
> -	enum dma_transfer_direction dir, unsigned long flags)
> +static inline struct dma_async_tx_descriptor *
> +dmaengine_prep_config_single(struct dma_chan *chan, dma_addr_t buf, size_t len,
> +			     enum dma_transfer_direction dir,
> +			     unsigned long flags,
> +			     struct dma_slave_config *config)
>  {
>  	struct scatterlist sg;
> +
> +	if (!chan || !chan->device)
> +		return NULL;
> +
>  	sg_init_table(&sg, 1);
>  	sg_dma_address(&sg) = buf;
>  	sg_dma_len(&sg) = len;
>  
> -	if (!chan || !chan->device || !chan->device->device_prep_slave_sg)
> +	if (chan->device->device_prep_config_sg)
> +		return chan->device->device_prep_config_sg(chan, &sg, 1, dir,
> +							   flags, config);
> +
> +	if (config)
> +		if (dmaengine_slave_config(chan, config))
> +			return NULL;
> +
> +	if (!chan->device->device_prep_slave_sg)
>  		return NULL;

Does this apply device configuration side effects before verifying if the
channel actually supports the preparation callback?

If device_prep_slave_sg is missing, the function correctly aborts and
returns NULL. However, the hardware or software channel state might have
already been mutated by the earlier dmaengine_slave_config() call on a
channel that cannot actually process the transfer.

Would it be safer to check for the presence of device_prep_slave_sg before
attempting the configuration fallback?

[ ... ]

> @@ -1010,17 +1037,37 @@ static inline struct dma_async_tx_descriptor *dmaengine_prep_peripheral_dma_vec(
>  							    dir, flags);
>  }
>  
> -static inline struct dma_async_tx_descriptor *dmaengine_prep_slave_sg(
> -	struct dma_chan *chan, struct scatterlist *sgl,	unsigned int sg_len,
> -	enum dma_transfer_direction dir, unsigned long flags)
> +static inline struct dma_async_tx_descriptor *
> +dmaengine_prep_config_sg(struct dma_chan *chan, struct scatterlist *sgl,
> +			 unsigned int sg_len, enum dma_transfer_direction dir,
> +			 unsigned long flags, struct dma_slave_config *config)
>  {
> -	if (!chan || !chan->device || !chan->device->device_prep_slave_sg)
> +	if (!chan || !chan->device)
> +		return NULL;
> +
> +	if (chan->device->device_prep_config_sg)
> +		return chan->device->device_prep_config_sg(chan, sgl, sg_len,
> +				dir, flags, config);
> +
> +	if (config)
> +		if (dmaengine_slave_config(chan, config))
> +			return NULL;
> +
> +	if (!chan->device->device_prep_slave_sg)
>  		return NULL;

This has the same potential configuration side effect ordering issue as
dmaengine_prep_config_single() above. Should the device_prep_slave_sg check
happen before calling dmaengine_slave_config()?

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