dmaengine: xilinx_dma: Fix freeup active list based on descriptor completion bit

Commit Message

Marek Vasut Oct. 31, 2024, 4:58 p.m. UTC

The xilinx_dma is completely broken since the referenced commit,
because if the (seg->hw.status & XILINX_DMA_BD_COMP_MASK) is not
set for whatever reason, the current descriptor is never moved to
the done list, and the DMA stops moving data.

Isolate the newly added check to DMA which does implement irq_delay,
that way the new check is matching what is likely some new bit in a
new core, without breaking the DMA for older versions of the same
core.

Fixes: 7bcdaa658102 ("dmaengine: xilinx_dma: Freeup active list based on descriptor completion bit")
Signed-off-by: Marek Vasut <[e-mail]>
---
Cc: "Uwe Kleine-König" <[e-mail]>
Cc: Michal Simek <[e-mail]>
Cc: Peter Korsgaard <[e-mail]>
Cc: Radhey Shyam Pandey <[e-mail]>
Cc: Vinod Koul <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
---
 drivers/dma/xilinx/xilinx_dma.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Radhey Shyam Pandey Nov. 6, 2024, 9:48 a.m. UTC | #1

> -----Original Message-----
> From: Marek Vasut <[e-mail]>
> Sent: Thursday, October 31, 2024 10:28 PM
> To: [e-mail]
> Cc: Marek Vasut <[e-mail]>; Uwe Kleine-König <u.kleine-
> [e-mail]>; Simek, Michal <[e-mail]>; Peter Korsgaard
> <[e-mail]>; Pandey, Radhey Shyam
> <[e-mail]>; Vinod Koul <[e-mail]>; linux-arm-
> [e-mail]
> Subject: [PATCH] dmaengine: xilinx_dma: Fix freeup active list based on descriptor
> completion bit
> 
> The xilinx_dma is completely broken since the referenced commit,
> because if the (seg->hw.status & XILINX_DMA_BD_COMP_MASK) is not
> set for whatever reason, the current descriptor is never moved to

I want to understand more on this failure scenario.  How to replicate it?

Why is completion bit not set ? Based on the documentation completed bit
indicates to the software that the DMA Engine has completed the transfer
as described by the associated descriptor. The DMA Engine sets this bit 
to 1 when the transfer is completed.


> the done list, and the DMA stops moving data.
> 
> Isolate the newly added check to DMA which does implement irq_delay,
> that way the new check is matching what is likely some new bit in a
> new core, without breaking the DMA for older versions of the same
> core.
> 
> Fixes: 7bcdaa658102 ("dmaengine: xilinx_dma: Freeup active list based on
> descriptor completion bit")
> Signed-off-by: Marek Vasut <[e-mail]>
> ---
> Cc: "Uwe Kleine-König" <[e-mail]>
> Cc: Michal Simek <[e-mail]>
> Cc: Peter Korsgaard <[e-mail]>
> Cc: Radhey Shyam Pandey <[e-mail]>
> Cc: Vinod Koul <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> ---
>  drivers/dma/xilinx/xilinx_dma.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index 1bdd57de87a6e..48647c8a64a5b 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -1718,7 +1718,8 @@ static void xilinx_dma_complete_descriptor(struct
> xilinx_dma_chan *chan)
>  		return;
> 
>  	list_for_each_entry_safe(desc, next, &chan->active_list, node) {
> -		if (chan->xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
> +		if (chan->xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA
> &&
> +		    chan->irq_delay) {
>  			struct xilinx_axidma_tx_segment *seg;
> 
>  			seg = list_last_entry(&desc->segments,
> --
> 2.45.2

Marek Vasut Nov. 6, 2024, 11:41 a.m. UTC | #2

On 11/6/24 10:48 AM, Pandey, Radhey Shyam wrote:
>> -----Original Message-----
>> From: Marek Vasut <[e-mail]>
>> Sent: Thursday, October 31, 2024 10:28 PM
>> To: [e-mail]
>> Cc: Marek Vasut <[e-mail]>; Uwe Kleine-König <u.kleine-
>> [e-mail]>; Simek, Michal <[e-mail]>; Peter Korsgaard
>> <[e-mail]>; Pandey, Radhey Shyam
>> <[e-mail]>; Vinod Koul <[e-mail]>; linux-arm-
>> [e-mail]
>> Subject: [PATCH] dmaengine: xilinx_dma: Fix freeup active list based on descriptor
>> completion bit
>>
>> The xilinx_dma is completely broken since the referenced commit,
>> because if the (seg->hw.status & XILINX_DMA_BD_COMP_MASK) is not
>> set for whatever reason, the current descriptor is never moved to
> 
> I want to understand more on this failure scenario.  How to replicate it?

The very basic test is to use AXI DMA for DMA transfer, which fails to 
complete because of this new chunk of code. By reading the commit 
history, I can only conclude this was something that got added due to 
the irq_delay, but was never tested on core without irq_delay support ?

> Why is completion bit not set ? Based on the documentation completed bit
> indicates to the software that the DMA Engine has completed the transfer
> as described by the associated descriptor. The DMA Engine sets this bit
> to 1 when the transfer is completed.
I don't know, the bit was not used before you added the bit and check of 
the bit in the problematic commit 7bcdaa658102 ("dmaengine: xilinx_dma: 
Freeup active list based on descriptor completion bit")

Radhey Shyam Pandey Nov. 11, 2024, 1:04 p.m. UTC | #3

> -----Original Message-----
> From: Marek Vasut <[e-mail]>
> Sent: Wednesday, November 6, 2024 5:12 PM
> To: Pandey, Radhey Shyam <[e-mail]>;
> [e-mail]
> Cc: Uwe Kleine-König <[e-mail]>; Simek, Michal
> <[e-mail]>; Peter Korsgaard <[e-mail]>; Vinod Koul
> <[e-mail]>; [e-mail]; Katakam, Harini
> <[e-mail]>
> Subject: Re: [PATCH] dmaengine: xilinx_dma: Fix freeup active list based on
> descriptor completion bit
> 
> On 11/6/24 10:48 AM, Pandey, Radhey Shyam wrote:
> >> -----Original Message-----
> >> From: Marek Vasut <[e-mail]>
> >> Sent: Thursday, October 31, 2024 10:28 PM
> >> To: [e-mail]
> >> Cc: Marek Vasut <[e-mail]>; Uwe Kleine-König <u.kleine-
> >> [e-mail]>; Simek, Michal <[e-mail]>; Peter
> >> Korsgaard <[e-mail]>; Pandey, Radhey Shyam
> >> <[e-mail]>; Vinod Koul <[e-mail]>;
> >> linux-arm- [e-mail]
> >> Subject: [PATCH] dmaengine: xilinx_dma: Fix freeup active list based
> >> on descriptor completion bit
> >>
> >> The xilinx_dma is completely broken since the referenced commit,
> >> because if the (seg->hw.status & XILINX_DMA_BD_COMP_MASK) is not set
> >> for whatever reason, the current descriptor is never moved to
> >
> > I want to understand more on this failure scenario.  How to replicate it?
> 
> The very basic test is to use AXI DMA for DMA transfer, which fails to complete
> because of this new chunk of code. By reading the commit history, I can only
> conclude this was something that got added due to the irq_delay, but was never
> tested on core without irq_delay support ?

This commit is also present in downstream xilinx kernel and all legacy regression
are working fine without xlnx,irq-delay.  Added Abin to this thread who confirmed
that axidmatest client[1] works without IRQ delay.

[1]: https://github.com/Xilinx/linux-xlnx/blob/master/drivers/dma/xilinx/axidmatest.c

So I want to understand what is the test client you are using?

> 
> > Why is completion bit not set ? Based on the documentation completed
> > bit indicates to the software that the DMA Engine has completed the
> > transfer as described by the associated descriptor. The DMA Engine
> > sets this bit to 1 when the transfer is completed.
> I don't know, the bit was not used before you added the bit and check of the bit in the
> problematic commit 7bcdaa658102 ("dmaengine: xilinx_dma:
> Freeup active list based on descriptor completion bit")

Patch

diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index 1bdd57de87a6e..48647c8a64a5b 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -1718,7 +1718,8 @@  static void xilinx_dma_complete_descriptor(struct xilinx_dma_chan *chan)
 		return;
 
 	list_for_each_entry_safe(desc, next, &chan->active_list, node) {
-		if (chan->xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
+		if (chan->xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA &&
+		    chan->irq_delay) {
 			struct xilinx_axidma_tx_segment *seg;
 
 			seg = list_last_entry(&desc->segments,