Re: [PATCH v3 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 DMA descriptor's `control` field is updated locklessly via plain assignment, which can cause compiler store tearing and violate the memory write ordering guarantees intended by the patch.
--

commit f34c073c93dcf4643e941a0c9907083868d8f064
Author: Frank Li <Frank.Li@nxp.com>

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

This commit moves the control field update to the end of the descriptor
programming sequence and adds a memory barrier. The goal is to ensure the
descriptor is fully written before the ready bit in the control field is
set, preparing the driver for concurrent descriptor updates while the DMA
is 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]
Could this plain assignment cause compiler store tearing and violate the
intended memory ordering?

Because struct dw_edma_v0_lli is __packed and lacks an explicit __aligned
attribute, the compiler assumes a 1-byte alignment. On architectures
compiled with strict alignment, this 32-bit assignment might be split into
four separate byte-sized stores.

If byte 0 is written first, the hardware could see the ready bit set
before the remaining control bits are written, bypassing the safety of the
preceding dma_wmb(). Would it be safer to use WRITE_ONCE() or a proper
accessor here to guarantee a single 32-bit store?

[ ... ]

> @@ -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 assignment suffer from the same store tearing issue as the link
data descriptor above, given 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]
Could the same store tearing occur here in dw_hdma_v0_write_ll_data,
since struct dw_hdma_v0_lli is also __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]
Could the same store tearing occur here in dw_hdma_v0_write_ll_link,
since struct dw_hdma_v0_llp is also __packed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702-edma_ll-v3-0-877aa463740c@nxp.com?part=1