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

[Stephan Gerhold <stephan.gerhold@linaro.org>]

On Wed, Jul 22, 2026 at 06:24:10PM +0200, Stephan Gerhold wrote:
> On Wed, Jul 22, 2026 at 08:37:19AM -0700, Bartosz Golaszewski wrote:
> > On Wed, 22 Jul 2026 16:20:52 +0200, Stephan Gerhold
> > <stephan.gerhold@linaro.org> 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@linaro.org> 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@linaro.org> said:
> > >> >> > On Tue, Jul 21, 2026 at 01:48:52PM +0000, sashiko-bot@kernel.org 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@oss.qualcomm.com>
> > >> >> >>
> > >> >> >> 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 ...
> > >> >> >
> [...]
> 
> FWIW, I couldn't stop myself trying to finish my "write lock descriptors
> directly into FIFO" idea, once I started I was curious how it would turn
> out. I think it's actually quite elegant, the loop in bam_start_dma() is
> wrapped with LOCK and UNLOCK, later process_channel_irqs() looks at the
> FIFO again and just skips over these descriptors. lock_ce is allocated
> once at channel creation time, no other allocations are needed. (Could
> probably move lock_ce allocation to slave_config() so it's allocated
> only when locking is configured for a channel).
> 
> See diff below. It doesn't crash badly in a quick test, but didn't check
> in detail if the locking is actually working correctly. :')
> 
> I'm okay with whatever is "free of known races" and works with the use
> cases we have, so use/adapt whatever you like best.
> 
> If you want to use this, feel free to add
> Co-developed-by: Stephan Gerhold <stephan.gerhold@linaro.org>
> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
> 
> Good luck! :D
> 
> Thanks,
> Stephan
> 
> ---
>  drivers/dma/qcom/bam_dma.c       | 94 ++++++++++++++++++++++++++++++--
>  include/linux/dma/qcom_bam_dma.h | 15 +++++
>  2 files changed, 104 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index f3e713a5259c..864531dca2a6 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> [...]
> @@ -607,6 +620,17 @@ static int bam_alloc_chan(struct dma_chan *chan)
>  		return -ENOMEM;
>  	}
>  
> +	if (bdev->dev_data->pipe_lock_supported) {
> +		bchan->lock_ce = dma_alloc_wc(bdev->dev, sizeof(*bchan->lock_ce),
> +					      &bchan->lock_ce_phys, GFP_KERNEL);
> +		if (!bchan->lock_ce) {
> +			dev_err(bdev->dev, "Failed to allocate lock CE\n");
> +			dma_free_wc(bdev->dev, BAM_DESC_FIFO_SIZE, bchan->fifo_virt, bchan->fifo_phys);
> +			bchan->fifo_virt = NULL;
> +			return -ENOMEM;
> +		}
> +	}

Nitpicking on my own diff: This is kind of meh, since this will
typically allocate a full page just for the 16 bytes lock command
element for all channels on BAMs that support locking (which is most BAM
instances nowadays). Barely anything will use locking though (likely
just QCE and NAND, if available). With 32+ channels in the BAM on some
SoCs with 16K page size or more that's a lot of wasted space.

Probably best to go back to what you had in your patch and use
dma_map_single() / dma_sync_single_for_device(), see below (on top of
what I sent already).

Thanks,
Stephan

---
 drivers/dma/qcom/bam_dma.c | 53 +++++++++++++++++++++++---------------
 1 file changed, 32 insertions(+), 21 deletions(-)

diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index 864531dca2a6..93ce99125879 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -433,7 +433,7 @@ struct bam_chan {
 	struct list_head node;
 
 	/* BAM locking infrastructure */
-	struct bam_cmd_element *lock_ce;
+	struct bam_cmd_element lock_ce;
 	dma_addr_t lock_ce_phys;
 	bool locking_enabled;
 	bool bam_locked;
@@ -620,17 +620,6 @@ static int bam_alloc_chan(struct dma_chan *chan)
 		return -ENOMEM;
 	}
 
-	if (bdev->dev_data->pipe_lock_supported) {
-		bchan->lock_ce = dma_alloc_wc(bdev->dev, sizeof(*bchan->lock_ce),
-					      &bchan->lock_ce_phys, GFP_KERNEL);
-		if (!bchan->lock_ce) {
-			dev_err(bdev->dev, "Failed to allocate lock CE\n");
-			dma_free_wc(bdev->dev, BAM_DESC_FIFO_SIZE, bchan->fifo_virt, bchan->fifo_phys);
-			bchan->fifo_virt = NULL;
-			return -ENOMEM;
-		}
-	}
-
 	if (bdev->active_channels++ == 0 && bdev->powered_remotely)
 		bam_reset(bdev);
 
@@ -665,13 +654,15 @@ static void bam_free_chan(struct dma_chan *chan)
 	scoped_guard(spinlock_irqsave, &bchan->vc.lock)
 		bam_reset_channel(bchan);
 
+	if (bchan->lock_ce_phys) {
+		dma_unmap_single(bdev->dev, bchan->lock_ce_phys,
+				 sizeof(bchan->lock_ce), DMA_TO_DEVICE);
+		bchan->lock_ce_phys = 0;
+	}
+
 	dma_free_wc(bdev->dev, BAM_DESC_FIFO_SIZE, bchan->fifo_virt,
 		    bchan->fifo_phys);
 	bchan->fifo_virt = NULL;
-	if (bchan->lock_ce) {
-		dma_free_wc(bdev->dev, sizeof(*bchan->lock_ce), bchan->lock_ce, bchan->lock_ce_phys);
-		bchan->lock_ce = NULL;
-	}
 
 	/* mask irq for pipe/channel */
 	val = readl_relaxed(bam_addr(bdev, 0, BAM_IRQ_SRCS_MSK_EE));
@@ -693,6 +684,24 @@ static void bam_free_chan(struct dma_chan *chan)
 	pm_runtime_put_autosuspend(bdev->dev);
 }
 
+static bool bam_map_lock_ce(struct bam_chan *bchan)
+{
+	if (bchan->lock_ce_phys) {
+		dma_sync_single_for_device(bchan->bdev->dev, bchan->lock_ce_phys,
+					   sizeof(bchan->lock_ce), DMA_TO_DEVICE);
+		return true;
+	}
+
+	bchan->lock_ce_phys = dma_map_single(bchan->bdev->dev, &bchan->lock_ce,
+					     sizeof(bchan->lock_ce), DMA_TO_DEVICE);
+	if (dma_mapping_error(bchan->bdev->dev, bchan->lock_ce_phys)) {
+		bchan->lock_ce_phys = 0;
+		return false;
+	}
+
+	return true;
+}
+
 /**
  * bam_slave_config - set slave configuration for channel
  * @chan: dma channel
@@ -717,11 +726,13 @@ static int bam_slave_config(struct dma_chan *chan,
 		if (cfg->direction != DMA_MEM_TO_DEV)
 			return -EINVAL;
 
-		if (bchan->lock_ce) {
-			bam_prep_ce_le32(bchan->lock_ce, peripheral_cfg->lock_scratchpad_addr,
-					 BAM_WRITE_COMMAND, 0);
-			bchan->locking_enabled = true;
-		}
+		bam_prep_ce_le32(&bchan->lock_ce, peripheral_cfg->lock_scratchpad_addr,
+				 BAM_WRITE_COMMAND, 0);
+
+		if (!bam_map_lock_ce(bchan))
+			return -ENOMEM;
+
+		bchan->locking_enabled = true;
 	} else {
 		/* Don't touch lock_ce here, it might still be used by issued descriptors */
 		bchan->locking_enabled = false;
-- 
2.54.0