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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Critical] Out-of-order DMA cookie assignment causes premature transaction completion for clients.
- [Critical] Native integers assigned directly to little-endian hardware descriptors.
- [High] Unconditional injection of LOCK/UNLOCK descriptors corrupts the pending transaction queue.
- [High] Unhandled allocation failure for UNLOCK descriptor leaves the BAM pipe permanently locked.
- [High] DMA engine control flag is invalidly passed as a DMA mapping attribute.
- [High] Descriptor-specific metadata unsafely overwrites global channel state.
- [Medium] Direction check contradicts commit message intent, explicitly skipping pipe locking for RX channels.
- [Medium] Missing length validation in `bam_metadata_attach` allows out-of-bounds reads.
--

commit 44d1fcb40281218d18c6b56492066d2d5e6beb61
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

dmaengine: qcom: bam_dma: add support for BAM locking

Add support for BAM pipe locking by prepending a dummy command descriptor
with the LOCK bit set, and appending one with the UNLOCK bit once the
transaction is done. The dummy writes use a scratchpad address provided via
descriptor metadata.

> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index 30318cf01ee20..2c9f90313c313 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
[ ... ]
> @@ -652,6 +664,35 @@ static int bam_slave_config(struct dma_chan *chan,
>  	return 0;
>  }
>  
> +static int bam_metadata_attach(struct dma_async_tx_descriptor *desc, void *data, size_t len)
> +{
> +	struct bam_chan *bchan = to_bam_chan(desc->chan);
> +	const struct bam_device_data *bdata = bchan->bdev->dev_data;
> +	struct bam_desc_metadata *metadata = data;
> +
> +	if (!data)
> +		return -EINVAL;
> +

Does this code need to validate the len parameter? If the caller passes a
buffer smaller than sizeof(struct bam_desc_metadata), will this read
out-of-bounds memory when accessing the metadata fields?

> +	if (!bdata->pipe_lock_supported)
> +		/*
> +		 * The client wants to use locking but this BAM version doesn't
> +		 * support it. Don't return an error here as this will stop the
> +		 * client from using DMA at all for no reason.
> +		 */
> +		return 0;
> +
> +	guard(spinlock_irqsave)(&bchan->vc.lock);
> +
> +	bchan->scratchpad_addr = metadata->scratchpad_addr;
> +	bchan->direction = metadata->direction;

By saving descriptor-specific metadata directly into the global channel
structure, could this overwrite the state if a client queues multiple
descriptors with different metadata?

Also, does the scratchpad_addr remain set for subsequent unrelated
transactions, potentially causing dummy writes to a stale address?

> +
> +	return 0;
> +}
[ ... ]
> +static struct bam_async_desc *
> +bam_make_lock_desc(struct bam_chan *bchan, unsigned long flag)
> +{
> +	struct dma_chan *chan = &bchan->vc.chan;
> +	struct bam_async_desc *async_desc;
> +	struct bam_desc_hw *desc;
> +	struct virt_dma_desc *vd;
> +	struct virt_dma_chan *vc;
> +	unsigned int mapped;
> +	dma_cookie_t cookie;
> +	int ret;
> +
> +	async_desc = kzalloc_flex(*async_desc, desc, 1, GFP_NOWAIT);
[ ... ]
> +	bam_prep_ce_le32(&async_desc->lock_ce, bchan->scratchpad_addr, BAM_WRITE_COMMAND, 0);
> +	sg_set_buf(&async_desc->lock_sg, &async_desc->lock_ce, sizeof(async_desc->lock_ce));
> +
> +	mapped = dma_map_sg_attrs(chan->slave, &async_desc->lock_sg,
> +				  1, DMA_TO_DEVICE, DMA_PREP_CMD);

DMA_PREP_CMD is a dmaengine control flag, but it's being passed as the attrs
parameter here. Could this cause undefined DMA mapping behavior by being
interpreted as a mapping attribute like DMA_ATTR_ALLOC_SINGLE_PAGES?

Additionally, since unmapping is done without attributes later, will this
violate the requirement that mapping and unmapping attributes must match?

> +	if (!mapped) {
> +		kfree(async_desc);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	desc->flags |= cpu_to_le16(DESC_FLAG_CMD | flag);
> +	desc->addr = sg_dma_address(&async_desc->lock_sg);
> +	desc->size = sizeof(struct bam_cmd_element);

Since the addr and size fields of struct bam_desc_hw are little-endian,
will assigning native integers here corrupt the physical address and size
on big-endian systems?

> +
> +	vc = &bchan->vc;
> +	vd = &async_desc->vd;
> +
> +	dma_async_tx_descriptor_init(&vd->tx, &vc->chan);
> +	vd->tx.flags = DMA_PREP_CMD;
> +	vd->tx.desc_free = vchan_tx_desc_free;
> +	vd->tx_result.result = DMA_TRANS_NOERROR;
> +	vd->tx_result.residue = 0;
> +
> +	cookie = dma_cookie_assign(&vd->tx);

Because this happens inside bam_start_dma(), after the client has already
prepared and queued their own descriptors, will the new LOCK descriptor
receive a cookie value that is numerically larger than the pending client
transactions?

Since the LOCK descriptor is inserted at the head of desc_issued and will
complete first, will this update the channel's completed_cookie to this
higher value, causing clients to see their own transactions as prematurely
completed?

> +	ret = dma_submit_error(cookie);
> +	if (ret) {
> +		dma_unmap_sg(chan->slave, &async_desc->lock_sg, 1, DMA_TO_DEVICE);
> +		kfree(async_desc);
> +		return ERR_PTR(ret);
> +	}
> +
> +	return async_desc;
> +}
> +
> +static int bam_do_setup_pipe_lock(struct bam_chan *bchan, bool lock)
> +{
> +	struct bam_device *bdev = bchan->bdev;
> +	const struct bam_device_data *bdata = bdev->dev_data;
> +	struct bam_async_desc *lock_desc;
> +	unsigned long flag;
> +
> +	lockdep_assert_held(&bchan->vc.lock);
> +
> +	if (!bdata->pipe_lock_supported || !bchan->scratchpad_addr ||
> +	    bchan->direction != DMA_MEM_TO_DEV)

The commit message explicitly states this is intended for RX channels.
However, DMA_MEM_TO_DEV specifies a TX transfer. Does this check skip
locking for RX channels (DMA_DEV_TO_MEM), contradicting the commit message?

> +		return 0;
> +
> +	flag = lock ? DESC_FLAG_LOCK : DESC_FLAG_UNLOCK;
> +
> +	lock_desc = bam_make_lock_desc(bchan, flag);
> +	if (IS_ERR(lock_desc))
> +		return PTR_ERR(lock_desc);
> +
> +	if (lock)
> +		list_add(&lock_desc->vd.node, &bchan->vc.desc_issued);
> +	else
> +		list_add_tail(&lock_desc->vd.node, &bchan->vc.desc_issued);
> +
> +	return 0;
> +}
> +
> +static void bam_setup_pipe_lock(struct bam_chan *bchan)
> +{
> +	if (bam_do_setup_pipe_lock(bchan, true) || bam_do_setup_pipe_lock(bchan, false))

If the first call successfully creates the LOCK descriptor (returning 0)
but the second call fails to allocate the UNLOCK descriptor, will the LOCK
descriptor remain queued without its corresponding UNLOCK, permanently
deadlocking the channel?

> +		dev_err(bchan->vc.chan.slave, "Failed to setup BAM pipe lock descriptors");
> +}
> +
>  /**
>   * bam_start_dma - start next transaction
>   * @bchan: bam dma channel
>   */
>  static void bam_start_dma(struct bam_chan *bchan)
>  {
> -	struct virt_dma_desc *vd = vchan_next_desc(&bchan->vc);
> +	struct virt_dma_desc *vd;
>  	struct bam_device *bdev = bchan->bdev;
>  	struct bam_async_desc *async_desc = NULL;
>  	struct bam_desc_hw *desc;
> @@ -1030,6 +1170,9 @@ static void bam_start_dma(struct bam_chan *bchan)
>  
>  	lockdep_assert_held(&bchan->vc.lock);
>  
> +	bam_setup_pipe_lock(bchan);

If bam_start_dma() encounters a full BAM FIFO and exits early, it leaves
unprocessed descriptors in the desc_issued list. When bam_start_dma() is
called again later, won't it prepend another LOCK and append another UNLOCK
into the middle of the pending transaction sequence, corrupting the hardware
stream?

> +
> +	vd = vchan_next_desc(&bchan->vc);
>  	if (!vd)
>  		return;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519-qcom-qce-cmd-descr-v17-0-53a595414b79@oss.qualcomm.com?part=6