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

[Nickolay Goppen <setotau@mainlining.org>]

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