RE: [PATCH v3] dmaengine: xilinx_dma: Free descriptor lists in order

From: Radhey Shyam Pandey <radheys@xilinx.com>
To: Nicholas Graumann <nick.graumann@gmail.com>,
	Appana Durga Kedareswara Rao <appanad@xilinx.com>,
	Harini Katakam <harinik@xilinx.com>,
	Vinod Koul <vkoul@kernel.org>, Michal Simek <michals@xilinx.com>,
	Appana Durga Kedareswara Rao <appanad@xilinx.com>
Cc: "dmaengine@vger.kernel.org" <dmaengine@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v3] dmaengine: xilinx_dma: Free descriptor lists in order
Date: Tue, 10 May 2022 09:17:53 +0000	[thread overview]
Message-ID: <SA1PR02MB8560D82039C9EA89E2914D3EC7C99@SA1PR02MB8560.namprd02.prod.outlook.com> (raw)
In-Reply-To: <20220411140348.30252-1-nick.graumann@gmail.com>

[-- Attachment #1: Type: text/plain, Size: 3737 bytes --]

> -----Original Message-----
> From: Nicholas Graumann <nick.graumann@gmail.com>
> Sent: Monday, April 11, 2022 7:34 PM
> To: Radhey Shyam Pandey <radheys@xilinx.com>; Appana Durga Kedareswara
> Rao <appanad@xilinx.com>; Harini Katakam <harinik@xilinx.com>; Vinod
> Koul <vkoul@kernel.org>; Michal Simek <michals@xilinx.com>; Appana Durga
> Kedareswara Rao <appanad@xilinx.com>
> Cc: Nicholas Graumann <nick.graumann@gmail.com>;
> dmaengine@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org
> Subject: [PATCH v3] dmaengine: xilinx_dma: Free descriptor lists in order
> 

Minor nit - It's first version. I think we need to spin a new version fixing it?

> If xilinx_dma_terminate_all is called while the AXI DMA is active, the following
> error messages might be seen upon restarting the DMA:
> 
> [   72.556254] xilinx_dma_irq_handler: Channel d053cb5b has errors 10, cdr
> 2c049f80 tdr 2c04a000
> [   72.557370] xilinx_dma_irq_handler: Channel d053cb5b has errors 100, cdr
> 2c049f80 tdr 2c049f80
> 
> From then on the AXI DMA won't process any more descriptors until the DMA
> channel is released and requested again.
> 
> The following sequence of events is what causes this to happen:
> 
> 1. Some descriptors are prepared with xilinx_dma_tx_submit (so they get
>    added to pending_list).
> 2. The DMA is kicked off via call to xilinx_dma_tx_submit (the
>    descriptors are moved to active_list).
> 3. While the transfer is active, another descriptor is prepared with
>    xilinx_dma_tx_submit (so it goes onto the pending_list) 4. Before the
> transfers complete, xilinx_dma_terminate_all is called.
>    That function resets the channel then calls
>    xilinx_dma_free_descriptors to free the descriptors.
> 
> At that point, pending_list contains a descriptor that is newer (and thus farther
> down the chain of descriptors) than the descriptors prepared in (1). However,
> it gets placed onto the free_seg_list before the older descriptors. From then
> on, the next pointers are no longer valid because the order of the descriptors
> in free_seg_list does not match the order in which the descriptors were
> allocated.
> 
> To remedy this, the descriptor lists need to be freed in order from oldest to
> newest, otherwise segments could be added to the free segment list in a
> different order than they were created. This is not an issue for VDMA nor
> CDMA because the driver does not maintain a list of descriptors to free.
> 
> Fixes: c0bba3a99f07 ("dmaengine: vdma: Add Support for Xilinx AXI Direct
> Memory Access Engine")
> Signed-off-by: Nicholas Graumann <nick.graumann@gmail.com>
> Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>

> ---
>  drivers/dma/xilinx/xilinx_dma.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index 3ff9fa3d8cd5..3b435449cd0c 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -884,9 +884,13 @@ static void xilinx_dma_free_descriptors(struct
> xilinx_dma_chan *chan)
> 
>  	spin_lock_irqsave(&chan->lock, flags);
> 
> -	xilinx_dma_free_desc_list(chan, &chan->pending_list);
> +	/*
> +	 * Descriptor lists must be freed from oldest to newest so that the
> +	 * order of free_seg_list is maintained.
> +	 */
>  	xilinx_dma_free_desc_list(chan, &chan->done_list);
>  	xilinx_dma_free_desc_list(chan, &chan->active_list);
> +	xilinx_dma_free_desc_list(chan, &chan->pending_list);
> 
>  	spin_unlock_irqrestore(&chan->lock, flags);  }
> --
> 2.35.1

[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 15833 bytes --]