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

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 <[email protected]>

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?

[ ... ]

> @@ -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?

> +		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?

[ ... ]

> @@ -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?

[ ... ]

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6