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

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

[Nickolay Goppen]

The BAM writes back the number of bytes actually received into the
descriptor that ended an RX transfer. Report this through the standard
dmaengine_result (residue = length - transferred) so clients get the
packet length from callback_result().

Assisted-by: LLM
Signed-off-by: Nickolay Goppen <setotau@mainlining.org>
---
IPA v2.x uses BAM to send and receive IP packets, to and from the AP.
However, unlike its predecessor BAM-DMUX, it doesn't send information
about the packet length, but ipa2 driver will need that. To find the 
length of the packet, one must instead read the difference between 
packet length and residue.
---
 drivers/dma/qcom/bam_dma.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index e62e411dc8f8..b5cfe3ce3406 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -65,6 +65,7 @@ struct bam_async_desc {
 
 	u32 num_desc;
 	u32 xfer_len;
+	size_t transferred;
 
 	/* transaction flags, EOT|EOB|NWD */
 	u16 flags;
@@ -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;
+}
+
 /**
  * bam_reset() - reset and initialize BAM registers
  * @bdev: bam device
@@ -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);
 }
 
@@ -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;

---
base-commit: 2cc223d6ffadd48e25eff0c8a03108a5d7a61c41
change-id: 20261115-qcom-bam-dma-metadata-f34e592d400c

Best regards,
--  
Nickolay Goppen <setotau@mainlining.org>

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

[sashiko-bot]

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

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

[Nickolay Goppen]

15.11.2026 19:08, 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

I will address the issues of the patch in next respin, but I don't know 
what to do with issues that |
wasn't introduced by my patch.

-- 
Best regards,
Nickolay