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

[Rosen Penev]

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