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

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.

Bartosz

> I was hammering at the code a bit earlier to try the "write the lock
> descriptors directly into the FIFO" approach. It has some advantages,
> but also makes the FIFO management quite a bit trickier, hard to get it
> right without full concentration. :')
>
> I _think_ it could potentially work, but if you have a simpler idea I'm
> also happy to throw away the mess I started. :-)
>
> Thanks,
> Stephan
>