1. Patchew
  2. linux
  3. View series

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

Rosen Penev posted 1 patch 1 week, 1 day ago 
drivers/dma/mv_xor.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
[PATCH] dmaengine: mv_xor: use writel() instead of relaxed variant
Posted by Rosen Penev 1 week, 1 day ago 
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));
 }
 
 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();
+
 		/* 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
Re: [PATCH] dmaengine: mv_xor: use writel() instead of relaxed variant
Posted by Frank Li 1 week, 1 day ago 
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?

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
>
Re: [PATCH] dmaengine: mv_xor: use writel() instead of relaxed variant
Posted by Rosen Penev 1 week, 1 day ago 
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.
>
> 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
> >
Re: [PATCH] dmaengine: mv_xor: use writel() instead of relaxed variant
Posted by Frank Li 1 week, 1 day ago 
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
> > >
Re: [PATCH] dmaengine: mv_xor: use writel() instead of relaxed variant
Posted by Rosen Penev 1 week ago 
On Wed, Sep 16, 2026 at 3:33 PM Frank Li <Frank.li@oss.nxp.com> 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@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() ?

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

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.