Re: [PATCH] dmaengine: mv_xor: use writel() instead of relaxed variant

[Frank Li]

On Thu, Sep 17, 2026 at 04:45:31PM -0700, Rosen Penev wrote:
> On Wed, Sep 16, 2026 at 3:33 PM Frank Li <Frank.li@xxxxxxxxxxx> wrote:
> >
> > On Wed, Sep 16, 2026 at 02:46:34PM -0700, Rosen Penev wrote:
> > > On Wed, Sep 16, 2026 at 12:06 PM Frank Li <Frank.li@xxxxxxxxxxx> wrote:
> > > >
> > > > On Wed, Sep 16, 2026 at 11:24:05AM -0700, Rosen Penev wrote:
> > > > > Fix the ordering guarantees between descriptor memory writes and the
> > > > > MMIO accesses that expose them to the engine. The descriptor pool is
> > > > > allocated with dma_alloc_wc(), so writes sit in the CPU write buffers
> > > > > and must be drained before the engine is allowed to read them.
> > > > >
> > > > > Program the next-descriptor pointer with writel() instead of
> > > > > writel_relaxed(); the full accessor drains prior memory writes before
> > > > > the MMIO write.
> > > > >
> > > > > In mv_xor_tx_submit() the appended chain link must be visible before the
> > > > > busy-status register is read. Use a full mb() so the engine cannot fetch a
> > > > > stale chain link and stall the channel after the CPU decides it is still
> > > > > busy.
> > > > >
> > > > > Fixes: ff7b04796d98 ("dmaengine: DMA engine driver for Marvell XOR engine")
> > > > > Assisted-by: LLM
> > > > > Signed-off-by: Rosen Penev <rosenp@xxxxxxxxx>
> > > > > ---
> > > > >  drivers/dma/mv_xor.c | 10 +++++++++-
> > > > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
> > > > > index c4e0dce3ca64..0de2b1ad5c30 100644
> > > > > --- a/drivers/dma/mv_xor.c
> > > > > +++ b/drivers/dma/mv_xor.c
> > > > > @@ -103,7 +103,10 @@ static u32 mv_chan_get_current_desc(struct mv_xor_chan *chan)
> > > > >  static void mv_chan_set_next_descriptor(struct mv_xor_chan *chan,
> > > > >                                       u32 next_desc_addr)
> > > > >  {
> > > > > -     writel_relaxed(next_desc_addr, XOR_NEXT_DESC(chan));
> > > > > +     /* writel drains descriptor writes to DRAM before the engine
> > > > > +      * is pointed at them
> > > > > +      */
> > > > > +     writel(next_desc_addr, XOR_NEXT_DESC(chan));
> > > >
> > > > I remember this my comments before, should be v2 version?
> > > >
> > > > >  }
> > > > >
> > > > >  static void mv_chan_unmask_interrupts(struct mv_xor_chan *chan)
> > > > > @@ -410,6 +413,11 @@ mv_xor_tx_submit(struct dma_async_tx_descriptor *tx)
> > > > >               /* fix up the hardware chain */
> > > > >               mv_desc_set_next_desc(old_chain_tail, sw_desc->async_tx.phys);
> > > > >
> > > > > +             /* make the new link visible to the engine before we read
> > > > > +              * the channel state, the device may fetch it at any point
> > > > > +              */
> > > > > +             mb();
> > > > > +
> > > >
> > > > The first is readl_relaxed() in mv_chan_is_busy(), all io readl and writel
> > > > is ordered.
> > > >
> > > > which part need mb() here?
> > > It's to deal with readl_relaxed in mv_chan_is_busy. I'll change to
> > > regular readl in v2.
> >
> > I just want to know which variable need read barrier() ?
>
> Review says:
>
>  #### Validity of mb() after mv_desc_set_next_desc()
>
>   • Ordering the memory store (old_chain_tail->phy_next_desc) before
> the MMIO register load (readl_relaxed(XOR_ACTIVATION) in
> mv_xor.c:181-188),

According to Documentation/memory-barriers.txt, read_relaxed() order
with writeX for the same peripherial.

mv_desc_set_next_desc(old_chain_tail, sw_desc->async_tx.phys);
use writel(),

mv_chan_is_busy() call readl_relaxed(XOR_ACTIVATION(chan)); readlX() must
happen after previous writel().

>  is necessary to prevent the
>   CPU from seeing busy == 1 while the link store is still in the CPU's
> store buffer, followed by the hardware completing old_chain_tail,

It is impossible. readl_relatex() guaratee get register value after previous
write data reach finial taget place. Many place use the same method to
guaratee previous write complete.

> reading a stale 0-pointer, and
>   halting without the CPU restarting it.
>   • Since readl_relaxed() does not order against prior memory stores,

readl_relatex() don't order against prior memory stores, but order
with previous writel()

> and dma_wmb() / wmb() only order stores against stores, a mandatory
> full barrier (mb()) is the
>   appropriate barrier for ordering a normal/WC memory store before an MMIO read.

simples said, io read and write is order itself.

1. mv_desc_set_next_desc() writle(),
2. mv_chan_is_busy() read_relax(),

1 and 2 is order, all data if have dependence, it is also order.

a = readl()
b = a * 2.  >> this must happen after readl().


Frank
> >
> > Frank
> >
> > > >
> > > > Frank
> > > >
> > > > >               /* if the channel is not busy */
> > > > >               if (!mv_chan_is_busy(mv_chan)) {
> > > > >                       u32 current_desc = mv_chan_get_current_desc(mv_chan);
> > > > > --
> > > > > 2.55.0
> > > > >