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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Same-CPU deadlock due to `spin_lock()` usage in `dmaengine_prep_config_sg_safe()` without disabling interrupts or bottom halves.
- [Medium] Meaningful documentation and implementation mismatch regarding the synchronization primitive.
--

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

dmaengine: Add safe API to combine configuration and preparation

This commit introduces dmaengine_prep_config_sg_safe() and
dmaengine_prep_config_single_safe() to provide a reentrant-safe way to
combine slave configuration and transfer preparation. It uses a new device
callback or falls back to 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 mutex protection while
the code implements a spinlock. Could the commit message be updated to
reflect the actual synchronization primitive used?

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

[ ... ]

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

Can this cause a same-CPU deadlock?

The documentation in struct dma_chan notes that *_prep() may be called
from a completion callback. Since completion callbacks often execute in
softirq or hardirq contexts, if a thread calls this function from
process context, local interrupts remain enabled.

If a DMA interrupt fires on the same CPU while the lock is held, the
completion callback could attempt to call this function again to queue
the next transfer, leading it to wait on the already-held chan->lock.

Does this fallback path need to use spin_lock_irqsave() and
spin_unlock_irqrestore() to safely disable interrupts?

> +
> +	tx = dmaengine_prep_config_sg(chan, sgl, sg_len, dir, flags, config);
> +
> +	if (!chan->device->device_prep_config_sg)
> +		spin_unlock(&chan->lock);
> +
> +	return tx;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512-dma_prep_config-v5-0-26865bf7d935@nxp.com?part=2