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

[Bartosz Golaszewski]

On Wed, 22 Jul 2026 16:20:52 +0200, Stephan Gerhold
<stephan.gerhold@xxxxxxxxxx> said:
> On Wed, Jul 22, 2026 at 07:11:10AM -0700, Bartosz Golaszewski wrote:
>> On Wed, 22 Jul 2026 14:47:56 +0200, Stephan Gerhold
>> <stephan.gerhold@xxxxxxxxxx> 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
>> >> <stephan.gerhold@xxxxxxxxxx> said:
>> >> > 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
>> >> [...]
>> >> >> [ ... ]
>> >> >> > @@ -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
>

I'm not really sure this is correct. From:

    https://www.kernel.org/doc/html/latest/driver-api/dmaengine/client.html

"Some DMA engine drivers may hold a spinlock between a successful preparation
and submission so it is important that these two operations are closely
paired."

What you presented above looks like abuse of the DMA engine API in light of
this statement. I also haven't found any place in the kernel where this would
happen.

What the docs say is legal is preparing/submitting a new transaction from
a completion callback but in this case the lock/unlock descriptors will be
refilled by the call to prep_slave_sg().

I would go with pre-allocated descriptors references from bam_chan,
re-allocated (if needed) on each prep_slave_sg() and just WARN() or even BUG()
if we ever end up not having any descriptors ready.

Bart