Re: [PATCH] dmaengine: qcom: bam_dma: Report the residue of packet

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] dmaengine: qcom: bam_dma: missing opening brace in
  bam_fifo_xfer_len()
- [High] dmaengine: qcom: bam_dma: undeclared metadata_ops in
  bam_prep_slave_sg()
- [Medium] dmaengine: qcom: bam_dma: missing read barrier in
  process_channel_irqs()
It also noticed 2 related pre-existing issues this patch does not
introduce:
- [Critical] dmaengine: qcom: bam_dma: ring buffer space miscalculation
  in bam_start_dma() [1]
- [High] dmaengine: qcom: bam_dma: unclocked register access in
  bam_dma_irq() [2]
--

commit e40d68fe8bef09bdcbef42721a06c3fd7ad7197d
Author: Nickolay Goppen <setotau@mainlining.org>

dmaengine: qcom: bam_dma: Report the residue of packet

This commit calculates the number of bytes transferred from the BAM FIFO
descriptors and reports the transfer residue in async_desc->vd.tx_result.residue
upon descriptor completion.

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

[ ... ]

> @@ -447,6 +448,30 @@ static inline void __iomem *bam_addr(struct bam_device *bdev, u32 pipe,
>  		r.ee_mult * bdev->ee;
>  }
>  
> +/**
> + * bam_fifo_xfer_len - bytes transferred for a completed descriptor chunk
> + * @bchan: BAM channel the chunk was queued on
> + * @async_desc: BAM async descriptor owning the chunk
> + *
> + * For RX pipes the BAM overwrites the size field of the descriptor that
> + * ended the transfer with the number of bytes actually written to memory,
> + * which can be less than the buffer size (e.g. an IPA packet).  Those
> + * values live in the FIFO copy, not in async_desc->desc[].
> + */
> +
> +static u32 bam_fifo_xfer_len(struct bam_chan *bchan,
> +			     const struct bam_async_desc *async_desc)
> +	struct bam_desc_hw *fifo = PTR_ALIGN(bchan->fifo_virt,
> +					     sizeof(struct bam_desc_hw));
> +	u32 len = 0;
> +	unsigned int i;
> +
> +	for (i = bchan->head; i < bchan->head + async_desc->xfer_len; i++)
> +		len += le16_to_cpu(fifo[i % MAX_DESCRIPTORS].size);
> +
> +	return len;
> +}

[Severity: High]
bam_fifo_xfer_len() is missing its opening '{' after the parameter list,
which breaks the build when CONFIG_QCOM_BAM_DMA is enabled.

[Severity: Critical]
This problem wasn't introduced by this patch, but in bam_start_dma(), the
producer side of the same descriptor FIFO ring buffer miscalculates
available space [1] and can overwrite in-flight DMA descriptors:

drivers/dma/qcom/bam_dma.c:bam_start_dma() {
	...
		avail = CIRC_SPACE(bchan->tail, bchan->head,
				   MAX_DESCRIPTORS + 1);
	...
		bchan->tail += async_desc->xfer_len;
		bchan->tail %= MAX_DESCRIPTORS;
}

CIRC_SPACE() is called with MAX_DESCRIPTORS + 1 (4096), which returns 4095
when the ring is empty (tail == head), while bchan->tail and bchan->head
wrap modulo MAX_DESCRIPTORS (4095).  Enqueuing 4095 descriptors wraps
bchan->tail back to bchan->head, making a full ring appear empty so
subsequent submissions overwrite active descriptors before hardware
processes them.

> @@ -732,6 +757,8 @@ static struct dma_async_tx_descriptor *bam_prep_slave_sg(struct dma_chan *chan,
>  		} while (remainder > 0);
>  	}
>  
> +	async_desc->vd.tx.metadata_ops = &metadata_ops;
> +
>  	return vchan_tx_prep(&bchan->vc, &async_desc->vd, flags);
>  }
>  

[Severity: High]
metadata_ops is not declared anywhere in this file or its included headers,
which causes compilation to fail.

> @@ -881,6 +908,11 @@ static u32 process_channel_irqs(struct bam_device *bdev)
>  			if (avail < async_desc->xfer_len)
>  				break;
>  
> +			async_desc->transferred +=
> +				bam_fifo_xfer_len(bchan, async_desc);
> +			async_desc->vd.tx_result.residue =
> +				async_desc->length - async_desc->transferred;
> +
>  			/* manage FIFO */
>  			bchan->head += async_desc->xfer_len;
>  			bchan->head %= MAX_DESCRIPTORS;

[Severity: Medium]
Can bam_fifo_xfer_len() read stale fifo[].size values before the hardware
descriptor writeback is observed?

bchan->fifo_virt is allocated with dma_alloc_wc() (Normal Non-Cacheable
memory on ARM/ARM64, which allows speculative loads), and earlier in
process_channel_irqs() BAM_P_SW_OFSTS is read with readl_relaxed():

drivers/dma/qcom/bam_dma.c:process_channel_irqs() {
	...
		offset = readl_relaxed(bam_addr(bdev, i, BAM_P_SW_OFSTS)) &
				       P_SW_OFSTS_MASK;
}

Because readl_relaxed() has no read barrier and the address of
fifo[i % MAX_DESCRIPTORS].size does not depend on offset, the CPU can
speculatively load the initial buffer size written by bam_start_dma()
instead of the byte count written back by the BAM hardware, reporting the
wrong residue to clients.  Using readl() for BAM_P_SW_OFSTS or adding a
dma_rmb() before reading fifo[].size would order the reads.

[Severity: High]
This problem wasn't introduced by this patch, but bam_dma_irq() calls
process_channel_irqs() before acquiring a runtime PM reference [2]:

drivers/dma/qcom/bam_dma.c:bam_dma_irq() {
	...
	srcs |= process_channel_irqs(bdev);
	...
	ret = pm_runtime_get_sync(bdev->dev);
}

If a shared or spurious interrupt fires while the BAM controller is
runtime-suspended with its clocks disabled, the readl_relaxed() of
BAM_IRQ_SRCS_EE at the start of process_channel_irqs() accesses an
unclocked register and triggers a synchronous external abort.

[1] https://sashiko.dev/#/bug/linux-a877fd6c-244a-404b-88a7-7d77506711a9
[2] https://sashiko.dev/#/bug/linux-0ca05019-399c-4f9d-a7e1-d74c7ddced75

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115-qcom-bam-dma-metadata-v1-1-52e4de54ffd3@mainlining.org?part=1