Re: [PATCH v5 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 plain C assignment to a `__packed` struct member introduces store tearing, breaking the intended synchronization.
--

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

This commit moves the control field update of the DMA link list entry to the
end of the sequence. It adds a dma_wmb() barrier to ensure correct memory
write ordering, which prepares the driver for supporting link entry updates
during DMA operation.

> 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]
Could this assignment tear on strict-alignment architectures?

Since struct dw_edma_v0_lli is defined as __packed without an __aligned()
attribute, the compiler may assume an alignment of 1. This means a plain C
assignment could be compiled into multiple byte-sized stores (such as strb on
ARM).

If the store tears, could the hardware observe the CB bit as ready before the
rest of the control word is fully written, leading to a corrupted control
state being processed?

>  	} 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]
Are we facing the same store tearing issue here since struct dw_edma_v0_llp
is also __packed?

[ ... ]

> 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]
Might this assignment also tear on strict-alignment architectures, as
struct dw_hdma_v0_lli is marked as __packed?

[ ... ]

> @@ -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]
Would this assignment also be susceptible to store tearing due to
struct dw_hdma_v0_llp being __packed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-edma_ll-v5-0-e199053d4300@nxp.com?part=1