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

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
>> [ ... ]
>> > @@ -72,6 +76,11 @@ struct bam_async_desc {
>> >
>> >  	struct bam_desc_hw *curr_desc;
>> >
>> > +	/* BAM locking infrastructure */
>> > +	bool is_lock_desc;
>> > +	struct scatterlist lock_sg;
>> > +	struct bam_cmd_element lock_ce;
>> > +
>> >  	/* list node for the desc in the bam_chan list of descriptors */
>> >  	struct list_head desc_node;
>>
>> [Severity: High]
>> Does placing lock_ce immediately adjacent to desc_node risk a DMA API
>> violation? lock_ce is mapped for DMA via dma_map_sg(), but desc_node is
>> actively modified by the CPU via list_add_tail() during bam_start_dma()
>> while the mapping is active. On non-coherent architectures, could this
>> cause cacheline tearing or stale data writebacks if they share the same
>> cacheline?
>>
>
> I think this complaint was there in some revisions before. Could be
> mostly a false positive given that the lock_ce is just DMA_MEM_TO_DEV
> (i.e. read-only for the BAM)...
>

Yes, I ignored it because it's not a problem for us in this case.

>> [ ... ]
>> > @@ -676,13 +691,26 @@ 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;
>> >
>> >  	guard(spinlock_irqsave)(&bchan->vc.lock);
>> >
>> >  	memcpy(&bchan->slave, cfg, sizeof(*cfg));
>> >  	bchan->reconfigure = 1;
>> >
>>
>> [Severity: High]
>> Is it safe to overwrite bchan->slave and set bchan->reconfigure = 1 before
>> validating the configuration direction below? If cfg->direction !=
>> DMA_MEM_TO_DEV, this returns -EINVAL to the caller, but the channel
>> retains the rejected parameters and will apply them on the next transfer
>> because bchan->reconfigure remains set.
>>
>
> Yeah, better to swap the order.
>

Sigh... ok.

>> > +	/*
>> > +	 * 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;
>> > +
>> > +		bchan->lock_scratchpad_addr = peripheral_cfg->lock_scratchpad_addr;
>> > +	}
>>
>> [Severity: High]
>> If bam_slave_config() is called without peripheral_cfg, should
>> bchan->lock_scratchpad_addr be reset to 0 to avoid leaking state from a
>> prior configuration? If the previous lock_scratchpad_addr is retained,
>> bam_start_dma() might erroneously inject dummy lock descriptors on new
>> transfers.
>>
>
> Probably unlikely to happen, but yeah.
>
> I would also check if (cfg->peripheral_size == sizeof(peripheral_cfg)),
> just to be safe.
>

Ok.

>> [ ... ]
>> > @@ -919,13 +959,19 @@ static u32 process_channel_irqs(struct bam_device *bdev)
>> >  			 * push back to front of desc_issued so that
>> >  			 * it gets restarted by the work queue.
>> >  			 */
>> > +
>> > +			list_del(&async_desc->desc_node);
>> >  			if (!async_desc->num_desc) {
>> > -				vchan_cookie_complete(&async_desc->vd);
>> > +				hdesc = async_desc->desc;
>> > +
>>
>> [Severity: Low]
>> Is the assignment to hdesc used anywhere? It appears to be assigned here but
>> is never read.
>>
>
> Yeah, I think I pointed this out before.
>

Dang it, I removed one but seem to have missed this.

>> [ ... ]
>> > @@ -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.

Bart