Re: [PATCH v23 06/14] dmaengine: qcom: bam_dma: add support for BAM locking

[Stephan Gerhold]

On Tue, Jul 21, 2026 at 01:48:52PM +0000, sashiko-bot@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [High] State leak causing hardware misconfiguration on channel reuse due to uncleared `lock_scratchpad_addr` and hardcoded direction.
> - [High] Channel state corruption when `bam_slave_config` fails validation.
> - [High] Infinite BH workqueue loop and unratelimited log flood on memory allocation failure.
> - [High] DMA API violation: `lock_ce` buffer shares a cacheline with frequently CPU-modified struct members.
> - [Low] Unused assignment of `hdesc` in IRQ handler.
> --
> 
> --- Patch [6]: [PATCH v23 06/14] dmaengine: qcom: bam_dma: add support for BAM locking ---
> commit 8b44c5e4935cbb61832adb3262df00b3b606a283
> Author: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxxxxxxxx>
> 
> dmaengine: qcom: bam_dma: add support for BAM locking
> 
> This commit adds support for BAM pipe locking by prepending and appending
> dummy command descriptors with LOCK/UNLOCK bits when starting DMA on a TX
> channel. It uses a peripheral-local scratchpad address communicated via slave
> config for the dummy writes.
> 
> > diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> > index f3e713a5259c2..411c60c4433e2 100644
> > --- a/drivers/dma/qcom/bam_dma.c
> > +++ b/drivers/dma/qcom/bam_dma.c
> [ ... ]
> > @@ -72,6 +76,11 @@ struct bam_async_desc {
> >  
> >  	struct bam_desc_hw *curr_desc;
> >  
> > +	/* BAM locking infrastructure */
> > +	bool is_lock_desc;
> > +	struct scatterlist lock_sg;
> > +	struct bam_cmd_element lock_ce;
> > +
> >  	/* list node for the desc in the bam_chan list of descriptors */
> >  	struct list_head desc_node;
> 
> [Severity: High]
> Does placing lock_ce immediately adjacent to desc_node risk a DMA API
> violation? lock_ce is mapped for DMA via dma_map_sg(), but desc_node is
> actively modified by the CPU via list_add_tail() during bam_start_dma()
> while the mapping is active. On non-coherent architectures, could this
> cause cacheline tearing or stale data writebacks if they share the same
> cacheline?
> 

I think this complaint was there in some revisions before. Could be
mostly a false positive given that the lock_ce is just DMA_MEM_TO_DEV
(i.e. read-only for the BAM)...

> [ ... ]
> > @@ -676,13 +691,26 @@ static void bam_free_chan(struct dma_chan *chan)
> >  static int bam_slave_config(struct dma_chan *chan,
> >  			    struct dma_slave_config *cfg)
> >  {
> > +	struct bam_config *peripheral_cfg = cfg->peripheral_config;
> >  	struct bam_chan *bchan = to_bam_chan(chan);
> > +	const struct bam_device_data *bdata = bchan->bdev->dev_data;
> >  
> >  	guard(spinlock_irqsave)(&bchan->vc.lock);
> >  
> >  	memcpy(&bchan->slave, cfg, sizeof(*cfg));
> >  	bchan->reconfigure = 1;
> >  
> 
> [Severity: High]
> Is it safe to overwrite bchan->slave and set bchan->reconfigure = 1 before
> validating the configuration direction below? If cfg->direction !=
> DMA_MEM_TO_DEV, this returns -EINVAL to the caller, but the channel
> retains the rejected parameters and will apply them on the next transfer
> because bchan->reconfigure remains set.
> 

Yeah, better to swap the order.

> > +	/*
> > +	 * This is required to setup the pipe locking and must be done even
> > +	 * before the first call to bam_start_dma().
> > +	 */
> > +	if (bdata->pipe_lock_supported && peripheral_cfg) {
> > +		if (cfg->direction != DMA_MEM_TO_DEV)
> > +			return -EINVAL;
> > +
> > +		bchan->lock_scratchpad_addr = peripheral_cfg->lock_scratchpad_addr;
> > +	}
> 
> [Severity: High]
> If bam_slave_config() is called without peripheral_cfg, should
> bchan->lock_scratchpad_addr be reset to 0 to avoid leaking state from a
> prior configuration? If the previous lock_scratchpad_addr is retained,
> bam_start_dma() might erroneously inject dummy lock descriptors on new
> transfers.
> 

Probably unlikely to happen, but yeah.

I would also check if (cfg->peripheral_size == sizeof(peripheral_cfg)),
just to be safe.

> [ ... ]
> > @@ -919,13 +959,19 @@ static u32 process_channel_irqs(struct bam_device *bdev)
> >  			 * push back to front of desc_issued so that
> >  			 * it gets restarted by the work queue.
> >  			 */
> > +
> > +			list_del(&async_desc->desc_node);
> >  			if (!async_desc->num_desc) {
> > -				vchan_cookie_complete(&async_desc->vd);
> > +				hdesc = async_desc->desc;
> > +
> 
> [Severity: Low]
> Is the assignment to hdesc used anywhere? It appears to be assigned here but
> is never read.
> 

Yeah, I think I pointed this out before.

> [ ... ]
> > @@ -1064,6 +1197,7 @@ static void bam_start_dma(struct bam_chan *bchan)
> >  
> >  	lockdep_assert_held(&bchan->vc.lock);
> >  
> > +	vd = vchan_next_desc(&bchan->vc);
> >  	if (!vd)
> >  		return;
> >  
> > @@ -1072,6 +1206,24 @@ static void bam_start_dma(struct bam_chan *bchan)
> >  		return;
> >  
> >  	while (vd && !IS_BUSY(bchan)) {
> > +		/*
> > +		 * Open a LOCK/UNLOCK bracket around each fresh sequence.
> > +		 * Sentinels inserted by bam_setup_pipe_lock() are skipped: they
> > +		 * already have bam_locked set and must not trigger a second pair.
> > +		 */
> > +		if (!bchan->bam_locked) {
> > +			ret = bam_setup_pipe_lock(bchan);
> > +			if (ret) {
> > +				dev_err_ratelimited(bdev->dev,
> > +					"failed to setup the pipe lock, deferring transfer: %d\n",
> > +					ret);
> > +				queue_work(system_bh_highpri_wq, &bdev->work);
> > +				break;
> > +			}
> 
> [Severity: High]
> Does requeuing to a high-priority BH workqueue upon ENOMEM cause CPU
> starvation? Because system_bh_highpri_wq executes in softirq context,
> requeuing immediately without sleeping prevents the memory reclaim needed
> to resolve the GFP_NOWAIT failure, leading to an infinite spin loop.
> 

It looks like my suggestion to queue_work() again in the error path of
bam_start_dma() wasn't great if we end up causing an infinite spin loop.
We do need to retry somehow or report an error though, I don't think
aborting and leaving the desccriptors completely unhandled is an option
either... :(

One option would be to try to avoid the allocation and write the lock
descriptors directly into the FIFO, but this will probably get really
messy as well, since you would need to carefully modify the FIFO
management in several functions ...

I don't have a good spontaenous idea how to solve this right now. :/

Thanks,
Stephan