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

On Wed, Jul 22, 2026 at 07:11:10AM -0700, Bartosz Golaszewski wrote:
> On Wed, 22 Jul 2026 14:47:56 +0200, Stephan Gerhold
> <[email protected]> said:
> > On Wed, Jul 22, 2026 at 02:34:52AM -0700, Bartosz Golaszewski wrote:
> >> On Wed, 22 Jul 2026 10:59:09 +0200, Stephan Gerhold
> >> <[email protected]> said:
> >> > On Tue, Jul 21, 2026 at 01:48:52PM +0000, [email protected] 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 <[email protected]>
> >> >>
> >> >> 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
> >> [...]
> >> >> [ ... ]
> >> >> > @@ -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. :/
> >> >
> >>
> >> Pre-allocate the lock descriptors (if needed) in bam_prep_slave_sg()? As in:
> >> call bam_make_lock_desc() in bam_prep_slave_sg(), store the descriptors and
> >> make bam_setup_pipe_lock() return void as it will no longer be possible for it
> >> to fail? It would just grab the preallocated descriptors.
> >>
> >
> > I considered suggesting this, but how do you know which descriptor needs
> > it in bam_prep_slave_sg()? It's just the allocation, afaict it doesn't
> > tell you anything about the order in which they will be submitted. :/
> >
> > We could always allocate the extra lock descriptors and waste the extra
> > memory for the descriptors that won't need it. That should work, but is
> > also not great ...
> >
> 
> Oh, I was thinking about having pointers to lock/unlock descriptors in struct
> bam_chan and to just grab them in bam_setup_pipe_lock() when needed and then
> next time we enter bam_slave_prep_sg(), we see we consumed them so let's
> re-allocate them. And of course: don't do it at all if pipe locking is not
> supported.
> 

Sadly, I think(?) from the API perspective it is valid to
dmaengine_prep_slave_*() a couple of buffers and them issue them
separately, e.g.:

	desc1= dmaengine_prep_slave_*();
	desc2 = dmaengine_prep_slave_*();

	dmaengine_submit(desc1);
	dma_async_issue_pending(chan);

	// do a bunch of other random stuff
	// bam_setup_pipe_lock() consumes lock descriptors

	dmaengine_submit(desc2);
	dma_async_issue_pending(chan);

In this situation, we wouldn't have any lock descriptors allocated
anymore?

Thanks,
Stephan