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

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

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@oss.nxp.com> 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@gmail.com>
> > > ---
> > >  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() ?

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