Re: [PATCH v4 01/10] dmaengine: dw-edma: Move control field update of DMA link to the last step

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The lockless update to the `control` field in the local DMA descriptor is vulnerable to compiler tearing due to the `__packed` attribute, undermining the hardware synchronization intended by this patch.
--

--- Patch [1]: [PATCH v4 01/10] dmaengine: dw-edma: Move control field update of DMA link to the last step ---
commit baf2c8f9cfe7a0520e9aa385d467515afdd0b585
Author: Frank Li <Frank.Li@nxp.com>
dmaengine: dw-edma: Move control field update of DMA link to the last step

This commit modifies the initialization order of DMA link list entries
so that the control field, which includes the ready bit, is written last
after a dma_wmb(). This prepares the driver to support updating link
entries while the DMA is actively running.

> diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
> index cfdd6463252e6..ee5c3c317557b 100644
> --- a/drivers/dma/dw-edma/dw-edma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c
> @@ -284,17 +284,18 @@ static void dw_edma_v0_write_ll_data(struct dw_edma_chunk *chunk, int i,
>  	if (chunk->chan->dw->chip->flags & DW_EDMA_CHIP_LOCAL) {
>  		struct dw_edma_v0_lli *lli = chunk->ll_region.vaddr.mem + ofs;
>  
> -		lli->control = control;
>  		lli->transfer_size = size;
>  		lli->sar.reg = sar;
>  		lli->dar.reg = dar;
> +		dma_wmb();
> +		lli->control = control;

[Severity: Medium]
Since struct dw_edma_v0_lli is marked as __packed, is it possible for the
compiler to tear this lli->control assignment into multiple byte-sized
stores on some architectures with strict alignment rules?

If the DMA engine is running and observes a torn write, it could read a
partially updated control word. While the commit message notes concurrent
updates aren't enabled yet, would using WRITE_ONCE() (along with any
necessary casting to avoid packed-member warnings) ensure this
synchronization flag is written indivisibly once concurrent hardware access
is enabled?

[ ... ]

> @@ -306,13 +307,14 @@ static void dw_edma_v0_write_ll_link(struct dw_edma_chunk *chunk,
>  	if (chunk->chan->dw->chip->flags & DW_EDMA_CHIP_LOCAL) {
>  		struct dw_edma_v0_llp *llp = chunk->ll_region.vaddr.mem + ofs;
>  
> -		llp->control = control;
>  		llp->llp.reg = pointer;
> +		dma_wmb();
> +		llp->control = control;

[Severity: Medium]
Does this llp->control assignment have the same tearing vulnerability
due to struct dw_edma_v0_llp being __packed?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-edma_ll-v4-0-cc128f0afb61@nxp.com?part=1