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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] DMA API violation due to mismatched device pointers in dma_map_sg and dma_unmap_sg.
- [High] The client-provided physical address for the lock scratchpad is silently truncated to 24 bits.
- [Medium] Transient allocation failure for lock descriptors causes a permanent channel stall.
- [Medium] Pipe locking is restricted to TX channels instead of RX channels, contradicting intended design.
- [Medium] The dummy lock descriptor clears the `reconfigure` flag using a hardcoded TX direction, potentially misconfiguring the channel.
- [Low] Missing integer argument for `%d` in `dev_err_ratelimited` call.
--

--- Patch [6]: [PATCH v22 06/14] dmaengine: qcom: bam_dma: add support for BAM locking ---
commit c786d3c51276cef4b55a461357b399c32982316b
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
Subject: 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 and UNLOCK bits set. It aims to prevent
writes into config registers while the engine is busy.

> Add support for BAM pipe locking. To that end: when starting DMA on an RX
> channel - prepend the existing queue of issued descriptors with an

[Severity: Medium]
Is this intended to be restricted to TX channels? The commit message mentions
starting DMA on an RX channel, but the implementation in bam_setup_pipe_lock()
aborts the lock setup if the direction is not DMA_MEM_TO_DEV.

> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index f3e713a5259c2..edaa694eb9df0 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c

[ ... ]

> @@ -859,6 +887,15 @@ static int bam_resume(struct dma_chan *chan)
>  	return 0;
>  }
>  
> +static void bam_dma_free_lock_desc(struct virt_dma_desc *vd)
> +{
> +	struct bam_async_desc *async_desc = container_of(vd, struct bam_async_desc, vd);
> +	struct dma_chan *chan = vd->tx.chan;
> +
> +	dma_unmap_sg(chan->slave, &async_desc->lock_sg, 1, DMA_TO_DEVICE);

[Severity: High]
Will this cause a DMA API violation? 

The scatterlist is unmapped here using the client device (chan->slave), but it
is mapped in bam_make_lock_desc() using the DMA controller device 
(bchan->bdev->dev).

> +	kfree(async_desc);
> +}

[ ... ]

> @@ -1046,13 +1090,101 @@ 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;

[ ... ]

> +	bam_prep_ce_le32(&async_desc->lock_ce, bchan->lock_scratchpad_addr, BAM_WRITE_COMMAND, 0);

[Severity: High]
Can this truncate the physical address and cause memory corruption?

The client-provided lock_scratchpad_addr is a 64-bit phys_addr_t, but
bam_prep_ce_le32() masks the address to 24 bits:

bam_ce->cmd_and_addr = cpu_to_le32((addr & 0xffffff) | ((cmd & 0xff) << 24));

If the physical address is above 16MB, the BAM engine will perform its dummy
write to an arbitrary truncated address.

> +	sg_set_buf(&async_desc->lock_sg, &async_desc->lock_ce, sizeof(async_desc->lock_ce));
> +
> +	mapped = dma_map_sg(bchan->bdev->dev, &async_desc->lock_sg, 1, DMA_TO_DEVICE);

[ ... ]

> +static int bam_setup_pipe_lock(struct bam_chan *bchan)
> +{
> +	const struct bam_device_data *bdata = bchan->bdev->dev_data;
> +	struct bam_async_desc *lock_desc, *unlock_desc;
> +
> +	lockdep_assert_held(&bchan->vc.lock);
> +
> +	if (!bdata->pipe_lock_supported || !bchan->lock_scratchpad_addr ||
> +	    bchan->direction != DMA_MEM_TO_DEV)
> +		return 0;

[Severity: Medium]
Does this restriction conflict with the commit message? 

The condition limits pipe locking to DMA_MEM_TO_DEV (TX), making this feature
dead code for RX channels (DMA_DEV_TO_MEM).

> +
> +	/*

[ ... ]

> @@ -1072,6 +1205,22 @@ 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");

[Severity: Low]
Is this missing the integer argument for the %d format specifier?

[Severity: Medium]
Will this cause a permanent channel stall on memory pressure? 

If bam_setup_pipe_lock() fails due to a GFP_NOWAIT allocation failure, the
loop breaks and leaves the descriptors on the desc_issued list, but no
mechanism appears to schedule a retry.

> +				break;
> +			}
> +			if (bchan->bam_locked)
> +				vd = vchan_next_desc(&bchan->vc);
> +		}
> +
>  		list_del(&vd->node);
>  
>  		async_desc = container_of(vd, struct bam_async_desc, vd);

[ ... ]

>  		/* on first use, initialize the channel hardware */
>  		if (!bchan->initialized)
>  			bam_chan_init_hw(bchan, async_desc->dir);
>  
>  		/* apply new slave config changes, if necessary */
>  		if (bchan->reconfigure)
>  			bam_apply_new_config(bchan, async_desc->dir);

[Severity: Medium]
Could the dummy lock descriptor clear the reconfigure flag with the wrong
direction?

The lock descriptor is created with async_desc->dir hardcoded to
DMA_MEM_TO_DEV. If pipeline locking is ever enabled for RX channels,
bam_apply_new_config() will configure the BAM engine using dst_maxburst
instead of src_maxburst, and clear the bchan->reconfigure flag before the
real data descriptor is processed.

>  

[ ... ]

> @@ -1191,8 +1344,11 @@ static void bam_issue_pending(struct dma_chan *chan)
>   */
>  static void bam_dma_free_desc(struct virt_dma_desc *vd)
>  {
> -	struct bam_async_desc *async_desc = container_of(vd,
> -			struct bam_async_desc, vd);
> +	struct bam_async_desc *async_desc = container_of(vd, struct bam_async_desc, vd);
> +	struct dma_chan *chan = vd->tx.chan;
> +
> +	if (async_desc->is_lock_desc)
> +		dma_unmap_sg(chan->slave, &async_desc->lock_sg, 1, DMA_TO_DEVICE);

[Severity: High]
Does this share the same DMA API violation mentioned earlier? 

The abort/cleanup path also unmaps using the client device instead of the
DMA controller device.

>  
>  	kfree(async_desc);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717-qcom-qce-cmd-descr-v22-0-ff0d18b27246@oss.qualcomm.com?part=6