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

[Frank Li <Frank.li@oss.nxp.com>]

On Mon, Jul 13, 2026 at 05:12:51PM +0000, sashiko-bot@kernel.org wrote:
> 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.

This problem is not existing because each field is 32bit aligned.

Frank

>
> 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