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

[sashiko-bot@kernel.org]

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

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

> +	/*
> +	 * 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.

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

[ ... ]
> @@ -1046,13 +1092,100 @@ static void bam_apply_new_config(struct bam_chan *bchan,
>  	bchan->reconfigure = 0;
>  }
>  
> +static struct bam_async_desc *
> +bam_make_lock_desc(struct bam_chan *bchan, unsigned long flag)
> +{
> +	struct bam_async_desc *async_desc;
> +	struct bam_desc_hw *desc;
> +	struct virt_dma_desc *vd;
> +	struct virt_dma_chan *vc;
> +	unsigned int mapped;
> +
> +	async_desc = kzalloc_flex(*async_desc, desc, 1, GFP_NOWAIT);
> +	if (!async_desc) {
> +		dev_err(bchan->bdev->dev, "failed to allocate the BAM lock descriptor\n");
> +		return ERR_PTR(-ENOMEM);
> +	}

[Severity: High]
Could this unratelimited dev_err() cause a log flood? If kzalloc_flex()
fails here, bam_start_dma() requeues the work to system_bh_highpri_wq,
which runs in softirq context immediately without sleeping. This creates a
tight spin loop that will retry the exact same GFP_NOWAIT allocation, fail
again, and rapidly flood the console.

> +
> +	sg_init_table(&async_desc->lock_sg, 1);
> +
> +	async_desc->num_desc = 1;
> +	async_desc->curr_desc = async_desc->desc;
> +	async_desc->dir = DMA_MEM_TO_DEV;
> +	async_desc->is_lock_desc = true;

[Severity: High]
Does hardcoding async_desc->dir to DMA_MEM_TO_DEV risk hardware
misconfiguration? If channel reuse retains lock_scratchpad_addr (as
mentioned in the bam_slave_config() review), a DEV_TO_MEM stream might
erroneously inject this dummy descriptor. This would cause
bam_apply_new_config() to execute with the wrong direction and incorrectly
apply dst_maxburst instead of src_maxburst, while clearing the reconfigure
flag for the actual data transfer.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-qcom-qce-cmd-descr-v23-0-1be02deffda9@oss.qualcomm.com?part=6