Re: [PATCH 2/3] dmaengine: xilinx: xdma: Fix synchronization issue

[Lizhi Hou]

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@xxxxxxxxxxxxxxx
> Suggested-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
> Signed-off-by: Louis Chauvet <louis.chauvet@xxxxxxxxxxx>
> ---
>   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 */