Re: [PATCH v6 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:

Pre-existing issues:
- [Medium] Lockless publication of the DMA descriptor is unsafe due to potential store tearing on a `__packed` struct member.
--

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

The control field in a DMA link list entry must be updated as the final
step because it includes the CB bit, which indicates whether the entry is
ready. Add dma_wmb() to ensure the correct memory write ordering.

> 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]
This is a pre-existing issue, but since struct dw_edma_v0_lli is marked
__packed, could the compiler tear this plain 32-bit assignment into multiple
byte-wise stores on strict-alignment architectures?

The goal of this patch is to safely publish the descriptor to the hardware by
updating the control field last. Would using WRITE_ONCE() be appropriate here
to guarantee a single atomic store and prevent compiler reordering?

If the DMA engine fetches a partially updated control word where the ready bit
is set but other control bits are stale, it might cause a hardware malfunction
once concurrent descriptor updates are fully supported.

[ ... ]

> @@ -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]
This is a pre-existing issue, but the same risk appears here for llp->control
since struct dw_edma_v0_llp is also __packed. Is WRITE_ONCE() also needed here
to prevent store tearing?

[ ... ]

> 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]
This is a pre-existing issue, but is there a similar store tearing risk in
dw_hdma_v0_write_ll_data() due to struct dw_hdma_v0_lli being __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]
This is a pre-existing issue, but a plain assignment is also used here on the
__packed struct dw_hdma_v0_llp. Should this assignment be updated to ensure
atomic publication as well?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-edma_ll-v6-0-1471d278b73a@nxp.com?part=1