Re: [PATCH v4 2/9] dmaengine: Add safe API to combine configuration and preparation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Deadlock risk due to using `spin_lock()` instead of `spin_lock_irqsave()`.
- [High] Calling `dmaengine_slave_config()` while holding a spinlock causes 'sleeping while atomic' kernel panics.
- [Medium] Meaningful code and commit message mismatch regarding lock type, and truncated comment.
--

commit 4872a557291e0d82176d77b1a17c5e410863322e
Author: Frank Li <Frank.Li@nxp.com>
dmaengine: Add safe API to combine configuration and preparation

This commit introduces dmaengine_prep_config_single_safe() and
dmaengine_prep_config_sg_safe() to provide a safe way to combine slave
configuration and transfer preparation. Drivers can implement a new
device_prep_config_sg() callback or the code falls back to executing both
steps under a per-channel lock.

>  Drivers may implement the new device_prep_config_sg() callback to perform
>  both steps atomically. If the callback is not provided, the helpers fall
>  back to calling dmaengine_slave_config() followed by
>  dmaengine_prep_slave_sg() under per-channel mutex protection.

This isn't a bug, but the commit message mentions per-channel mutex
protection, while the implementation actually uses a spinlock. Could the
commit message be updated to match the code?

> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -341,6 +343,12 @@ struct dma_chan {
>  	dma_cookie_t cookie;
>  	dma_cookie_t completed_cookie;
>  
> +	/*
> +	 * protect between config and prepare transfer because *_prep() may be
> +	 * called from complete callback, which is in GFP_NOSLEEP context.
> +	 */
> +	spinlock_t lock; /* protect between config and prepare transfer since */

This isn't a bug, but the inline comment for the lock appears to be truncated
mid-sentence after the word since.

[ ... ]

> +static inline struct dma_async_tx_descriptor *
> +dmaengine_prep_config_sg_safe(struct dma_chan *chan, struct scatterlist *sgl,
> +			      unsigned int sg_len,
> +			      enum dma_transfer_direction dir,
> +			      unsigned long flags,
> +			      struct dma_slave_config *config)
> +{
> +	struct dma_async_tx_descriptor *tx;
> +
> +	if (!chan || !chan->device)
> +		return NULL;
> +
> +	if (!chan->device->device_prep_config_sg)
> +		spin_lock(&chan->lock);

Since this API can be called from process context, and completion callbacks
might attempt to re-queue transfers from hardware interrupt context, could
using spin_lock() here cause a deadlock regression if the CPU is interrupted
while holding chan->lock?

Would it be safer to use spin_lock_irqsave() here instead? If so, we might
need a new variable to save the interrupt state to avoid shadowing the
existing flags function argument.

> +
> +	tx = dmaengine_prep_config_sg(chan, sgl, sg_len, dir, flags, config);

Does calling dmaengine_prep_config_sg() while holding a spinlock introduce a
regression where scheduling while atomic panics can occur?

Looking at dmaengine_prep_config_sg(), it calls dmaengine_slave_config(),
which invokes the driver's device_config() callback. Many DMA drivers
expect device_config() to be called in process context and perform sleeping
operations like acquiring mutexes, making sleeping memory allocations, or
calling pm_runtime_get_sync().

> +
> +	if (!chan->device->device_prep_config_sg)
> +		spin_unlock(&chan->lock);
> +
> +	return tx;
> +}

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