Received-SPF: pass (google.com: domain of linux-kernel+bounces-121736-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Received-SPF: TempError (protection.outlook.com: error in processing during
 lookup of amd.com: DNS Timeout)
Message-ID: <b59dd8cd-fd75-5342-d411-817f33e0ff48@amd.com>
Date: Wed, 27 Mar 2024 10:46:04 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH 2/3] dmaengine: xilinx: xdma: Fix synchronization issue
Content-Language: en-US
To: Louis Chauvet <louis.chauvet@bootlin.com>, Brian Xu <brian.xu@amd.com>,
	Raj Kumar Rampelli <raj.kumar.rampelli@amd.com>, Vinod Koul
	<vkoul@kernel.org>, Michal Simek <michal.simek@amd.com>, Jan Kuliga
	<jankul@alatek.krakow.pl>, Miquel Raynal <miquel.raynal@bootlin.com>
CC: <dmaengine@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>,
	<linux-kernel@vger.kernel.org>, <stable@vger.kernel.org>
References: <20240327-digigram-xdma-fixes-v1-0-45f4a52c0283@bootlin.com>
 <20240327-digigram-xdma-fixes-v1-2-45f4a52c0283@bootlin.com>
From: Lizhi Hou <lizhi.hou@amd.com>
In-Reply-To: <20240327-digigram-xdma-fixes-v1-2-45f4a52c0283@bootlin.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Received-SPF: None (SATLEXMB03.amd.com: lizhi.hou@amd.com does not designate
 permitted sender hosts)
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Mar 2024 17:46:13.9575
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 1b29a999-b1d1-43c0-cc42-08dc4e85cc08
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=3dd8961f-e488-4e60-8e11-a82d994e183d;Ip=[165.204.84.17];Helo=[SATLEXMB03.amd.com]
X-MS-Exchange-CrossTenant-AuthSource:
	DS1PEPF00017091.namprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA3PR12MB9129


On 3/27/24 02:58, Louis Chauvet wrote:
> The current xdma_synchronize method does not properly wait for the last
> transfer to be done. Due to limitations of the XMDA engine, it is not
> possible to stop a transfer in the middle of a descriptor. Said
> otherwise, if a stop is requested at the end of descriptor "N" and the OS
> is fast enough, the DMA controller will effectively stop immediately.
> However, if the OS is slightly too slow to request the stop and the DMA
> engine starts descriptor "N+1", the N+1 transfer will be performed until
> its end. This means that after a terminate_all, the last descriptor must
> remain valid and the synchronization must wait for this last descriptor to
> be terminated.
>
> Fixes: 855c2e1d1842 ("dmaengine: xilinx: xdma: Rework xdma_terminate_all()")
> Fixes: f5c392d106e7 ("dmaengine: xilinx: xdma: Add terminate_all/synchronize callbacks")
> Cc: stable@vger.kernel.org
> Suggested-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
>   drivers/dma/xilinx/xdma-regs.h |  3 +++
>   drivers/dma/xilinx/xdma.c      | 26 ++++++++++++++++++--------
>   2 files changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/dma/xilinx/xdma-regs.h b/drivers/dma/xilinx/xdma-regs.h
> index 98f5f6fb9ff9..6ad08878e938 100644
> --- a/drivers/dma/xilinx/xdma-regs.h
> +++ b/drivers/dma/xilinx/xdma-regs.h
> @@ -117,6 +117,9 @@ struct xdma_hw_desc {
>   			 CHAN_CTRL_IE_WRITE_ERROR |			\
>   			 CHAN_CTRL_IE_DESC_ERROR)
>   
> +/* bits of the channel status register */
> +#define XDMA_CHAN_STATUS_BUSY			BIT(0)
> +
>   #define XDMA_CHAN_STATUS_MASK CHAN_CTRL_START
>   
>   #define XDMA_CHAN_ERROR_MASK (CHAN_CTRL_IE_DESC_ALIGN_MISMATCH |	\
> diff --git a/drivers/dma/xilinx/xdma.c b/drivers/dma/xilinx/xdma.c
> index b9788aa8f6b7..5a3a3293b21b 100644
> --- a/drivers/dma/xilinx/xdma.c
> +++ b/drivers/dma/xilinx/xdma.c
> @@ -71,6 +71,8 @@ struct xdma_chan {
>   	enum dma_transfer_direction	dir;
>   	struct dma_slave_config		cfg;
>   	u32				irq;
> +	struct completion		last_interrupt;
> +	bool				stop_requested;
>   };
>   
>   /**
> @@ -376,6 +378,8 @@ static int xdma_xfer_start(struct xdma_chan *xchan)
>   		return ret;
>   
>   	xchan->busy = true;
> +	xchan->stop_requested = false;
> +	reinit_completion(&xchan->last_interrupt);

If stop_requested is true, it should not start another transfer. So I 
would suggest to add

      if (xchan->stop_requested)

                 return -ENODEV;

at the beginning of xdma_xfer_start().

xdma_xfer_start() is protected by chan lock.

>   
>   	return 0;
>   }
> @@ -387,7 +391,6 @@ static int xdma_xfer_start(struct xdma_chan *xchan)
>   static int xdma_xfer_stop(struct xdma_chan *xchan)
>   {
>   	int ret;
> -	u32 val;
>   	struct xdma_device *xdev = xchan->xdev_hdl;
>   
>   	/* clear run stop bit to prevent any further auto-triggering */
> @@ -395,13 +398,7 @@ static int xdma_xfer_stop(struct xdma_chan *xchan)
>   			   CHAN_CTRL_RUN_STOP);
>   	if (ret)
>   		return ret;
Above two lines can be removed with your change.
> -
> -	/* Clear the channel status register */
> -	ret = regmap_read(xdev->rmap, xchan->base + XDMA_CHAN_STATUS_RC, &val);
> -	if (ret)
> -		return ret;
> -
> -	return 0;
> +	return ret;
>   }
>   
>   /**
> @@ -474,6 +471,8 @@ static int xdma_alloc_channels(struct xdma_device *xdev,
>   		xchan->xdev_hdl = xdev;
>   		xchan->base = base + i * XDMA_CHAN_STRIDE;
>   		xchan->dir = dir;
> +		xchan->stop_requested = false;
> +		init_completion(&xchan->last_interrupt);
>   
>   		ret = xdma_channel_init(xchan);
>   		if (ret)
> @@ -521,6 +520,7 @@ static int xdma_terminate_all(struct dma_chan *chan)
>   	spin_lock_irqsave(&xdma_chan->vchan.lock, flags);
>   
>   	xdma_chan->busy = false;
> +	xdma_chan->stop_requested = true;
>   	vd = vchan_next_desc(&xdma_chan->vchan);
>   	if (vd) {
>   		list_del(&vd->node);
> @@ -542,6 +542,13 @@ static int xdma_terminate_all(struct dma_chan *chan)
>   static void xdma_synchronize(struct dma_chan *chan)
>   {
>   	struct xdma_chan *xdma_chan = to_xdma_chan(chan);
> +	struct xdma_device *xdev = xdma_chan->xdev_hdl;
> +	int st = 0;
> +
> +	/* If the engine continues running, wait for the last interrupt */
> +	regmap_read(xdev->rmap, xdma_chan->base + XDMA_CHAN_STATUS, &st);
> +	if (st & XDMA_CHAN_STATUS_BUSY)
> +		wait_for_completion_timeout(&xdma_chan->last_interrupt, msecs_to_jiffies(1000));
I suggest to add error message for timeout case.
>   
>   	vchan_synchronize(&xdma_chan->vchan);
>   }
> @@ -876,6 +883,9 @@ static irqreturn_t xdma_channel_isr(int irq, void *dev_id)
>   	u32 st;
>   	bool repeat_tx;
>   
> +	if (xchan->stop_requested)
> +		complete(&xchan->last_interrupt);
> +

This should be moved to the end of function to make sure processing 
previous transfer is completed.

out:

     if (xchan->stop_requested) {

             xchan->busy = false;

             complete(&xchan->last_interrupt);

     }

     spin_unlock(&xchan->vchan.lock);

     return IRQ_HANDLED;


Thanks,

Lizhi

>   	spin_lock(&xchan->vchan.lock);
>   
>   	/* get submitted request */
>