diff mbox series

[-next,7/8] dmaengine: xilinx_dma: Check for both idle and halted state in axidma stop_transfer

Message ID 1567701424-25658-8-git-send-email-radhey.shyam.pandey@xilinx.com (mailing list archive)
State Changes Requested
Headers show
Series AXI DMA driver improvements | expand

Commit Message

Radhey Shyam Pandey Sept. 5, 2019, 4:37 p.m. UTC 
From: Nicholas Graumann <nick.graumann@gmail.com>

When polling for a stopped transfer in AXI DMA mode, in some cases the
status of the channel may indicate IDLE instead of HALTED if the
channel was reset due to an error.

Signed-off-by: Nicholas Graumann <nick.graumann@gmail.com>
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
---
 drivers/dma/xilinx/xilinx_dma.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Vinod Koul Sept. 26, 2019, 5:21 p.m. UTC | #1 
On 05-09-19, 22:07, Radhey Shyam Pandey wrote:
> From: Nicholas Graumann <nick.graumann@gmail.com>
> 
> When polling for a stopped transfer in AXI DMA mode, in some cases the
> status of the channel may indicate IDLE instead of HALTED if the
> channel was reset due to an error.
> 
> Signed-off-by: Nicholas Graumann <nick.graumann@gmail.com>
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> ---
>  drivers/dma/xilinx/xilinx_dma.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index b5dd62a..0896e07 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -1092,8 +1092,9 @@ static int xilinx_dma_stop_transfer(struct xilinx_dma_chan *chan)
>  
>  	/* Wait for the hardware to halt */
>  	return xilinx_dma_poll_timeout(chan, XILINX_DMA_REG_DMASR, val,
> -				       val & XILINX_DMA_DMASR_HALTED, 0,
> -				       XILINX_DMA_LOOP_COUNT);
> +				       val | (XILINX_DMA_DMASR_IDLE |
> +					      XILINX_DMA_DMASR_HALTED),

The condition was bitwise AND and now is OR.. ??

> +				       0, XILINX_DMA_LOOP_COUNT);
>  }
>  
>  /**
> -- 
> 2.7.4
Radhey Shyam Pandey Sept. 27, 2019, 6:48 a.m. UTC | #2 
> -----Original Message-----
> From: Vinod Koul <vkoul@kernel.org>
> Sent: Thursday, September 26, 2019 10:51 PM
> To: Radhey Shyam Pandey <radheys@xilinx.com>
> Cc: dan.j.williams@intel.com; Michal Simek <michals@xilinx.com>;
> nick.graumann@gmail.com; andrea.merello@gmail.com; Appana Durga
> Kedareswara Rao <appanad@xilinx.com>; mcgrof@kernel.org;
> dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH -next 7/8] dmaengine: xilinx_dma: Check for both idle
> and halted state in axidma stop_transfer
> 
> On 05-09-19, 22:07, Radhey Shyam Pandey wrote:
> > From: Nicholas Graumann <nick.graumann@gmail.com>
> >
> > When polling for a stopped transfer in AXI DMA mode, in some cases the
> > status of the channel may indicate IDLE instead of HALTED if the
> > channel was reset due to an error.
> >
> > Signed-off-by: Nicholas Graumann <nick.graumann@gmail.com>
> > Signed-off-by: Radhey Shyam Pandey
> <radhey.shyam.pandey@xilinx.com>
> > ---
> >  drivers/dma/xilinx/xilinx_dma.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/dma/xilinx/xilinx_dma.c
> b/drivers/dma/xilinx/xilinx_dma.c
> > index b5dd62a..0896e07 100644
> > --- a/drivers/dma/xilinx/xilinx_dma.c
> > +++ b/drivers/dma/xilinx/xilinx_dma.c
> > @@ -1092,8 +1092,9 @@ static int xilinx_dma_stop_transfer(struct
> xilinx_dma_chan *chan)
> >
> >  	/* Wait for the hardware to halt */
> >  	return xilinx_dma_poll_timeout(chan, XILINX_DMA_REG_DMASR,
> val,
> > -				       val & XILINX_DMA_DMASR_HALTED, 0,
> > -				       XILINX_DMA_LOOP_COUNT);
> > +				       val | (XILINX_DMA_DMASR_IDLE |
> > +					      XILINX_DMA_DMASR_HALTED),
> 
> The condition was bitwise AND and now is OR.. ??

Ah, it should be same as before . Only _IDLE mask should be in OR.

Also on second thought to this patch- we need to describe which error
scenario "in some cases the status of the channel may indicate IDLE
instead of HALTED" as mentioned in commit description.

@Nick: Can you comment?

> 
> > +				       0, XILINX_DMA_LOOP_COUNT);
> >  }
> >
> >  /**
> > --
> > 2.7.4
> 
> --
> ~Vinod
Nicholas Graumann Sept. 27, 2019, 1:57 p.m. UTC | #3 
On Fri, Sep 27, 2019 at 06:48:29AM +0000, Radhey Shyam Pandey wrote:
> > -----Original Message-----
> > From: Vinod Koul <vkoul@kernel.org>
> > Sent: Thursday, September 26, 2019 10:51 PM
> > To: Radhey Shyam Pandey <radheys@xilinx.com>
> > Cc: dan.j.williams@intel.com; Michal Simek <michals@xilinx.com>;
> > nick.graumann@gmail.com; andrea.merello@gmail.com; Appana Durga
> > Kedareswara Rao <appanad@xilinx.com>; mcgrof@kernel.org;
> > dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH -next 7/8] dmaengine: xilinx_dma: Check for both idle
> > and halted state in axidma stop_transfer
> > 
> > On 05-09-19, 22:07, Radhey Shyam Pandey wrote:
> > > From: Nicholas Graumann <nick.graumann@gmail.com>
> > >
> > > When polling for a stopped transfer in AXI DMA mode, in some cases the
> > > status of the channel may indicate IDLE instead of HALTED if the
> > > channel was reset due to an error.
> > >
> > > Signed-off-by: Nicholas Graumann <nick.graumann@gmail.com>
> > > Signed-off-by: Radhey Shyam Pandey
> > <radhey.shyam.pandey@xilinx.com>
> > > ---
> > >  drivers/dma/xilinx/xilinx_dma.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/dma/xilinx/xilinx_dma.c
> > b/drivers/dma/xilinx/xilinx_dma.c
> > > index b5dd62a..0896e07 100644
> > > --- a/drivers/dma/xilinx/xilinx_dma.c
> > > +++ b/drivers/dma/xilinx/xilinx_dma.c
> > > @@ -1092,8 +1092,9 @@ static int xilinx_dma_stop_transfer(struct
> > xilinx_dma_chan *chan)
> > >
> > >  	/* Wait for the hardware to halt */
> > >  	return xilinx_dma_poll_timeout(chan, XILINX_DMA_REG_DMASR,
> > val,
> > > -				       val & XILINX_DMA_DMASR_HALTED, 0,
> > > -				       XILINX_DMA_LOOP_COUNT);
> > > +				       val | (XILINX_DMA_DMASR_IDLE |
> > > +					      XILINX_DMA_DMASR_HALTED),
> > 
> > The condition was bitwise AND and now is OR.. ??
> 
> Ah, it should be same as before . Only _IDLE mask should be in OR.
> 
> Also on second thought to this patch- we need to describe which error
> scenario "in some cases the status of the channel may indicate IDLE
> instead of HALTED" as mentioned in commit description.
> 
> @Nick: Can you comment?
> 
In regard to the mask question, yes, this looks like a bug.
We should be AND'ing with the mask like before.

As far as the state, usually when we saw the IDLE state when invoking 
dmaengine_terminate_all on a channel that had errors. I have not
proved this, but I believe what happened was the following:

New transactions were queued when chan->err was set, causing
xilinx_dma_chan_reset to be invoked which ultimately results in the
hardware being in an IDLE state by the time xilinx_dma_terminate_all
gets around to invoking stop_transfer. At that point, stop_transfer is
going to time out waiting for the hardware to indicate it has HALTED and
ultimately will time out.


In any case, xilinx_dma_stop_transfer should be fine with the hardware
being in an IDLE state to indicate that the active transfer is stopped.
Case in point: The CDMA core also covered by this driver only has an
IDLE bit and no HALTED bit in its DMASR, and it checks for just the IDLE
bit in xilinx_cdma_stop_transfer().
> > 
> > > +				       0, XILINX_DMA_LOOP_COUNT);
> > >  }
> > >
> > >  /**
> > > --
> > > 2.7.4
> > 
> > --
> > ~Vinod
Radhey Shyam Pandey Sept. 27, 2019, 4:53 p.m. UTC | #4 
> -----Original Message-----
> From: Nicholas Graumann <nick.graumann@gmail.com>
> Sent: Friday, September 27, 2019 7:27 PM
> To: Radhey Shyam Pandey <radheys@xilinx.com>
> Cc: Vinod Koul <vkoul@kernel.org>; dan.j.williams@intel.com; Michal Simek
> <michals@xilinx.com>; andrea.merello@gmail.com; Appana Durga
> Kedareswara Rao <appanad@xilinx.com>; mcgrof@kernel.org;
> dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH -next 7/8] dmaengine: xilinx_dma: Check for both idle
> and halted state in axidma stop_transfer
> 
> On Fri, Sep 27, 2019 at 06:48:29AM +0000, Radhey Shyam Pandey wrote:
> > > -----Original Message-----
> > > From: Vinod Koul <vkoul@kernel.org>
> > > Sent: Thursday, September 26, 2019 10:51 PM
> > > To: Radhey Shyam Pandey <radheys@xilinx.com>
> > > Cc: dan.j.williams@intel.com; Michal Simek <michals@xilinx.com>;
> > > nick.graumann@gmail.com; andrea.merello@gmail.com; Appana Durga
> > > Kedareswara Rao <appanad@xilinx.com>; mcgrof@kernel.org;
> > > dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH -next 7/8] dmaengine: xilinx_dma: Check for both
> idle
> > > and halted state in axidma stop_transfer
> > >
> > > On 05-09-19, 22:07, Radhey Shyam Pandey wrote:
> > > > From: Nicholas Graumann <nick.graumann@gmail.com>
> > > >
> > > > When polling for a stopped transfer in AXI DMA mode, in some cases
> the
> > > > status of the channel may indicate IDLE instead of HALTED if the
> > > > channel was reset due to an error.
> > > >
> > > > Signed-off-by: Nicholas Graumann <nick.graumann@gmail.com>
> > > > Signed-off-by: Radhey Shyam Pandey
> > > <radhey.shyam.pandey@xilinx.com>
> > > > ---
> > > >  drivers/dma/xilinx/xilinx_dma.c | 5 +++--
> > > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/dma/xilinx/xilinx_dma.c
> > > b/drivers/dma/xilinx/xilinx_dma.c
> > > > index b5dd62a..0896e07 100644
> > > > --- a/drivers/dma/xilinx/xilinx_dma.c
> > > > +++ b/drivers/dma/xilinx/xilinx_dma.c
> > > > @@ -1092,8 +1092,9 @@ static int xilinx_dma_stop_transfer(struct
> > > xilinx_dma_chan *chan)
> > > >
> > > >  	/* Wait for the hardware to halt */
> > > >  	return xilinx_dma_poll_timeout(chan, XILINX_DMA_REG_DMASR,
> > > val,
> > > > -				       val & XILINX_DMA_DMASR_HALTED, 0,
> > > > -				       XILINX_DMA_LOOP_COUNT);
> > > > +				       val | (XILINX_DMA_DMASR_IDLE |
> > > > +					      XILINX_DMA_DMASR_HALTED),
> > >
> > > The condition was bitwise AND and now is OR.. ??
> >
> > Ah, it should be same as before . Only _IDLE mask should be in OR.
> >
> > Also on second thought to this patch- we need to describe which error
> > scenario "in some cases the status of the channel may indicate IDLE
> > instead of HALTED" as mentioned in commit description.
> >
> > @Nick: Can you comment?
> >
> In regard to the mask question, yes, this looks like a bug.
> We should be AND'ing with the mask like before.
> 
> As far as the state, usually when we saw the IDLE state when invoking
> dmaengine_terminate_all on a channel that had errors. I have not
> proved this, but I believe what happened was the following:

As per IP produce guide pg021, once DMACR[RS] is set to 0x0
the halted bit in the DMA Status register should asserts to
0x1 when the DMA engine is halted. Also the DMA may be the
in IDLE state, there may be active data on the AXI interface.

I think for now we can skip this patchset in v2 and repost it
when a proper root cause is done.

> 
> New transactions were queued when chan->err was set, causing
> xilinx_dma_chan_reset to be invoked which ultimately results in the
> hardware being in an IDLE state by the time xilinx_dma_terminate_all
> gets around to invoking stop_transfer. At that point, stop_transfer is
> going to time out waiting for the hardware to indicate it has HALTED and
> ultimately will time out.
> 
> 
> In any case, xilinx_dma_stop_transfer should be fine with the hardware
> being in an IDLE state to indicate that the active transfer is stopped.
> Case in point: The CDMA core also covered by this driver only has an
> IDLE bit and no HALTED bit in its DMASR, and it checks for just the IDLE
> bit in xilinx_cdma_stop_transfer().
> > >
> > > > +				       0, XILINX_DMA_LOOP_COUNT);
> > > >  }
> > > >
> > > >  /**
> > > > --
> > > > 2.7.4
> > >
> > > --
> > > ~Vinod
diff mbox series

Patch

diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index b5dd62a..0896e07 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -1092,8 +1092,9 @@  static int xilinx_dma_stop_transfer(struct xilinx_dma_chan *chan)
 
 	/* Wait for the hardware to halt */
 	return xilinx_dma_poll_timeout(chan, XILINX_DMA_REG_DMASR, val,
-				       val & XILINX_DMA_DMASR_HALTED, 0,
-				       XILINX_DMA_LOOP_COUNT);
+				       val | (XILINX_DMA_DMASR_IDLE |
+					      XILINX_DMA_DMASR_HALTED),
+				       0, XILINX_DMA_LOOP_COUNT);
 }
 
 /**