Re: [PATCH v7 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] Non-atomic update of the DMA descriptor trigger field (`control`) due to `__packed` struct layout and lack of `WRITE_ONCE()`, which undermines the safe concurrent DMA updates intended by the patch.
--

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

dmaengine: dw-edma: Move control field update of DMA link to the last step

This patch reorders the initialization of the DMA link list entries to write
the control field last, followed by a dma_wmb() barrier. This is intended
to safely signal readiness to the hardware and prepare the driver for updating
descriptors 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]
Will this assignment safely guarantee an atomic update for the hardware?

Since struct dw_edma_v0_lli is defined as __packed, the compiler might emit
byte-wise stores or tear the write instead of doing a single 32-bit store.
Without WRITE_ONCE(), the compiler could also potentially reorder or split
the store.

If the goal is to safely update descriptors while the DMA is running, does
this need to use WRITE_ONCE() (along with ensuring proper alignment) so the
hardware doesn't observe a partially updated control field?

>  	} else {
>  		struct dw_edma_v0_lli __iomem *lli = chunk->ll_region.vaddr.io + ofs;
>  
> -		writel(control, &lli->control);
>  		writel(size, &lli->transfer_size);
>  		writeq(sar, &lli->sar.reg);
>  		writeq(dar, &lli->dar.reg);
> +		writel(control, &lli->control);
>  	}
>  }

[ ... ]

> @@ -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]
Would this also need WRITE_ONCE() since struct dw_edma_v0_llp is a __packed
struct?

[ ... ]

> diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> index 632abb8b481cf..1201f1ab5f359 100644
> --- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> @@ -160,17 +160,18 @@ static void dw_hdma_v0_write_ll_data(struct dw_edma_chunk *chunk, int i,
>  	if (chunk->chan->dw->chip->flags & DW_EDMA_CHIP_LOCAL) {
>  		struct dw_hdma_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]
Similar to the v0_lli struct above, dw_hdma_v0_lli is also __packed. Could this
store tear and cause the DMA hardware to observe an incomplete control field?

>  	} else {
>  		struct dw_hdma_v0_lli __iomem *lli = chunk->ll_region.vaddr.io + ofs;
>  
> -		writel(control, &lli->control);
>  		writel(size, &lli->transfer_size);
>  		writeq(sar, &lli->sar.reg);
>  		writeq(dar, &lli->dar.reg);
> +		writel(control, &lli->control);
>  	}
>  }

[ ... ]

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

[Severity: Medium]
Could this assignment to the __packed dw_hdma_v0_llp struct tear as well?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713-edma_ll-v7-0-6fb7498c901e@nxp.com?part=1