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

[Stephan Gerhold]

On Thu, Jul 23, 2026 at 05:28:13PM +0000, sashiko-bot@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> 
> New issues:
> - [High] Calling `queue_work()` inside `bam_start_dma()` when there is exactly 1 slot available creates an infinite busy-loop in softirq context (softlockup).
> - [High] The transaction completion callback fires before the hardware pipe is actually unlocked, violating the mutual exclusion requirement.
> - [High] The driver clears the software lock state prematurely, allowing concurrent configuration changes to corrupt in-flight hardware execution.
> - [Medium] The 'reserved' field of the lock command element is left uninitialized before being synced to the device.
> 
> Pre-existing issues:
> - [Critical] Mismatch between FIFO wrapping modulo and CIRC_SPACE size leads to silent descriptor overwrites and data corruption.
> --
> 
> --- Patch [6]: [PATCH v24 06/14] dmaengine: qcom: bam_dma: add support for BAM locking ---
> commit bf87fb2cf384d859e1e0efff5d0a48f0cb3ccea2
> Author: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxxxxxxxx>
> 
> dmaengine: qcom: bam_dma: add support for BAM locking
> 
> Add support for BAM pipe locking. To that end: when starting DMA on a TX
> channel (DMA_MEM_TO_DEV) - prepend the existing queue of issued
> descriptors with an additional "dummy" command descriptor with the LOCK
> bit set. Once the transaction is done (no more issued descriptors),
> issue one more dummy descriptor with the UNLOCK bit.
> 
> > diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> > --- a/drivers/dma/qcom/bam_dma.c
> > +++ b/drivers/dma/qcom/bam_dma.c
> 
> [ ... ]
> 
> > @@ -676,10 +697,51 @@ 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;
> > +
> > +	if (peripheral_cfg && cfg->peripheral_size != sizeof(*peripheral_cfg))
> > +		return -EINVAL;
> >  
> >  	guard(spinlock_irqsave)(&bchan->vc.lock);
> >  
> > +	/*
> > +	 * 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;
> > +
> > +		if (bchan->bam_locked)
> > +			return -EBUSY;
> > +
> > +		if (!bchan->lock_ce) {
> > +			bchan->lock_ce = kmalloc_obj(*bchan->lock_ce, GFP_ATOMIC);
> 
> [Severity: Medium]
> Will this leave the reserved field of the command element uninitialized?
> 
> Looking at how this is initialized in include/linux/dma/qcom_bam_dma.h:
> 
> static inline void
> bam_prep_ce_le32(struct bam_cmd_element *bam_ce, u32 addr,
>                  enum bam_command_type cmd, __le32 data)
> {
>         bam_ce->cmd_and_addr =
>                 cpu_to_le32((addr & 0xffffff) | ((cmd & 0xff) << 24));
>         bam_ce->data = data;
>         bam_ce->mask = cpu_to_le32(0xffffffff);
> }
> 
> The reserved field is not set here, and kmalloc_obj() does not zero the
> memory. Could this map and sync uninitialized kernel heap memory to the DMA
> hardware?
> 

kzalloc_obj()? This is your bug. :-)

> [ ... ]
> 
> > @@ -1064,22 +1145,34 @@ static void bam_start_dma(struct bam_chan *bchan)
> >  
> >  	lockdep_assert_held(&bchan->vc.lock);
> >  
> > -	if (!vd)
> > +	vd = vchan_next_desc(&bchan->vc);
> > +	if (IS_BUSY(bchan) || (!vd && !bchan->bam_locked))
> >  		return;
> >  
> >  	ret = pm_runtime_get_sync(bdev->dev);
> >  	if (ret < 0)
> >  		return;
> >  
> > +	if (!bchan->initialized)
> > +		bam_chan_init_hw(bchan, container_of(vd, struct bam_async_desc, vd)->dir);
> > +
> > +	if (bchan->locking_enabled && !bchan->bam_locked) {
> > +		/* Defer locking until we also have space for a data descriptor */
> > +		avail = CIRC_SPACE(bchan->tail, bchan->head, MAX_DESCRIPTORS + 1);
> 
> [Severity: Critical]
> This is a pre-existing issue, but does CIRC_SPACE() calculate the correct
> available space when bchan->tail wraps at 4095?
> 
> MAX_DESCRIPTORS is 4095. Down below in bam_start_dma(), the pointers wrap
> via:
> 
>     bchan->tail += async_desc->xfer_len;
>     bchan->tail %= MAX_DESCRIPTORS;
> 
> Since bchan->tail wraps to 0 after 4094, it never reaches 4095. However,
> CIRC_SPACE() is called with MAX_DESCRIPTORS + 1 (4096). Because
> CIRC_SPACE() assumes the pointers wrap at the provided power-of-2 size, when
> the buffer is full (tail == head), won't it incorrectly evaluate the space as
> 4095 and cause a massive buffer overrun?
> 

Ooof, this seems correct to me but we can/should fix this separately
probably...

> > +		if (avail < 2) {
> > +			queue_work(system_bh_highpri_wq, &bdev->work);
> > +			goto out;
> > +		}
> 
> [Severity: High]
> Can this early return cause an infinite busy-loop softlockup?
> 
> If exactly one slot is available (avail == 1), queue_work() is called and
> we return. The worker executes bam_dma_work():
> 
>     if ((!list_empty(&bchan->vc.desc_issued) || bchan->bam_locked) &&
>         !IS_BUSY(bchan))
>         bam_start_dma(bchan);
> 
> Since IS_BUSY() only tests for avail == 0, the check passes and
> bam_start_dma() is immediately called again. It sees avail < 2 again,
> queues the work again, and returns. Could this loop indefinitely in softirq
> context until a hardware interrupt eventually fires?
> 

Yes, the queue_work() doesn't make sense here. We need to wait for the
BAM to finish processing some descriptors. Once it has notified us about
that, the IRQ handler will queue_work() it anyway, so you can just drop
this.

Technically, the avail < 2 check isn't even needed. We could start
writing the LOCK descriptor and then wait for the BAM to make further
room for the data descriptors. I just thought it's a bit weird to queue
LOCK without also adding some data descriptors at the same time.

> [ ... ]
> 
> > @@ -1135,11 +1228,24 @@ static void bam_start_dma(struct bam_chan *bchan)
> >  		if (((avail <= async_desc->xfer_len) || !vd ||
> >  		     dmaengine_desc_callback_valid(&cb)) &&
> >  		    !(async_desc->flags & DESC_FLAG_EOT))
> >  			desc[async_desc->xfer_len - 1].flags |=
> >  				cpu_to_le16(DESC_FLAG_INT);
> 
> [Severity: High]
> Does this cause the transaction completion callback to fire before the
> hardware pipe is actually unlocked?
> 
> DESC_FLAG_INT is set on the data descriptor here, regardless of whether an
> UNLOCK descriptor will immediately follow. When the data descriptor completes,
> an interrupt is generated and process_channel_irqs() completes the cookie:
> 
>     if (!async_desc->num_desc)
>         vchan_cookie_complete(&async_desc->vd);
> 
> Could this notify client drivers of transaction completion while the UNLOCK
> descriptor is still pending in the hardware FIFO, allowing clients to modify
> state or power down the device prematurely?
> 

Yes, but the client driver should be only concerned about the completion
of its own data operations, the lock handling is transparent and
internal to the BAM driver IMO. "Works as intended."

> [ ... ]
> 
> > +	/*
> > +	 * Close the bracket once there is no more client work queued. The
> > +	 * UNLOCK is flagged for an interrupt so process_channel_irqs() is
> > +	 * guaranteed to observe its completion and retire it from the FIFO
> > +	 * promptly, instead of leaving bchan->head to lag until a later
> > +	 * bracket's skip-loop catches up with it.
> > +	 */
> > +	if (bchan->bam_locked && !vd && !IS_BUSY(bchan)) {
> > +		bam_fifo_write_lock(bchan, DESC_FLAG_UNLOCK | DESC_FLAG_INT);
> > +		bchan->bam_locked = false;
> > +	}
> 
> [Severity: High]
> Is the software lock state cleared too early here?
> 
> We synchronously set bchan->bam_locked to false immediately after placing the
> UNLOCK descriptor into the software FIFO, before the hardware has actually
> completed the transaction. 
> 
> If a client then calls bam_slave_config(), the -EBUSY check for
> bchan->bam_locked will pass, and the driver might overwrite bchan->lock_ce
> or configuration registers while the hardware is still busy processing
> in-flight descriptors. Does this violate the mutual exclusion requirement?

It's true, but I think this can be ignored. That's why I didn't even
have the -EBUSY check in my diff. bam_start_dma() already re-configures
the channel in the middle of a transaction (if bchan->reconfigure is
set), so that one will already make changes to in-progress descriptors
anyway. It's a bit problematic though that we don't provide a way for
clients to wait until the unlocking has been completed, so they don't
have any chance to avoid affecting ongoing unlock operations from their
side.

Or you just disallow changing the lock_scratchpad_addr once it has been
set. Whatever. I don't think anyone will ever use multiple different
addresses there.

Thanks,
Stephan