diff mbox series

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

Message ID 20240327-digigram-xdma-fixes-v1-2-45f4a52c0283@bootlin.com (mailing list archive)
State Accepted
Commit 6a40fb8245965b481b4dcce011cd63f20bf91ee0
Headers show
Series dmaengine: xilinx: xdma: Various fixes for xdma | expand

Commit Message

Louis Chauvet March 27, 2024, 9:58 a.m. UTC
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(-)

Comments

Lizhi Hou March 27, 2024, 5:46 p.m. UTC | #1
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 */
>
Miquel Raynal March 28, 2024, 12:23 a.m. UTC | #2
Hi Lizhi,

> > @@ -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;

Maybe -EBUSY in this case?

I thought synchronize() was mandatory in-between. If that's not the
case then indeed we need to block or error-out if a new transfer
gets started.

> 
> at the beginning of xdma_xfer_start().
> 
> xdma_xfer_start() is protected by chan lock.
> 
> >   >   	return 0;  
> >   }

Thanks,
Miquèl
kernel test robot March 28, 2024, 1:09 a.m. UTC | #3
Hi Louis,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 8e938e39866920ddc266898e6ae1fffc5c8f51aa]

url:    https://github.com/intel-lab-lkp/linux/commits/Louis-Chauvet/dmaengine-xilinx-xdma-Fix-wrong-offsets-in-the-buffers-addresses-in-dma-descriptor/20240327-180155
base:   8e938e39866920ddc266898e6ae1fffc5c8f51aa
patch link:    https://lore.kernel.org/r/20240327-digigram-xdma-fixes-v1-2-45f4a52c0283%40bootlin.com
patch subject: [PATCH 2/3] dmaengine: xilinx: xdma: Fix synchronization issue
config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20240328/202403280803.IAFl90ZE-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240328/202403280803.IAFl90ZE-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202403280803.IAFl90ZE-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/dma/xilinx/xdma.c:76: warning: Function parameter or struct member 'last_interrupt' not described in 'xdma_chan'
>> drivers/dma/xilinx/xdma.c:76: warning: Function parameter or struct member 'stop_requested' not described in 'xdma_chan'


vim +76 drivers/dma/xilinx/xdma.c

17ce252266c7f0 Lizhi Hou     2023-01-19  53  
17ce252266c7f0 Lizhi Hou     2023-01-19  54  /**
17ce252266c7f0 Lizhi Hou     2023-01-19  55   * struct xdma_chan - Driver specific DMA channel structure
17ce252266c7f0 Lizhi Hou     2023-01-19  56   * @vchan: Virtual channel
17ce252266c7f0 Lizhi Hou     2023-01-19  57   * @xdev_hdl: Pointer to DMA device structure
17ce252266c7f0 Lizhi Hou     2023-01-19  58   * @base: Offset of channel registers
17ce252266c7f0 Lizhi Hou     2023-01-19  59   * @desc_pool: Descriptor pool
17ce252266c7f0 Lizhi Hou     2023-01-19  60   * @busy: Busy flag of the channel
17ce252266c7f0 Lizhi Hou     2023-01-19  61   * @dir: Transferring direction of the channel
17ce252266c7f0 Lizhi Hou     2023-01-19  62   * @cfg: Transferring config of the channel
17ce252266c7f0 Lizhi Hou     2023-01-19  63   * @irq: IRQ assigned to the channel
17ce252266c7f0 Lizhi Hou     2023-01-19  64   */
17ce252266c7f0 Lizhi Hou     2023-01-19  65  struct xdma_chan {
17ce252266c7f0 Lizhi Hou     2023-01-19  66  	struct virt_dma_chan		vchan;
17ce252266c7f0 Lizhi Hou     2023-01-19  67  	void				*xdev_hdl;
17ce252266c7f0 Lizhi Hou     2023-01-19  68  	u32				base;
17ce252266c7f0 Lizhi Hou     2023-01-19  69  	struct dma_pool			*desc_pool;
17ce252266c7f0 Lizhi Hou     2023-01-19  70  	bool				busy;
17ce252266c7f0 Lizhi Hou     2023-01-19  71  	enum dma_transfer_direction	dir;
17ce252266c7f0 Lizhi Hou     2023-01-19  72  	struct dma_slave_config		cfg;
17ce252266c7f0 Lizhi Hou     2023-01-19  73  	u32				irq;
70e8496bf693e1 Louis Chauvet 2024-03-27  74  	struct completion		last_interrupt;
70e8496bf693e1 Louis Chauvet 2024-03-27  75  	bool				stop_requested;
17ce252266c7f0 Lizhi Hou     2023-01-19 @76  };
17ce252266c7f0 Lizhi Hou     2023-01-19  77
Lizhi Hou March 28, 2024, 4:49 p.m. UTC | #4
On 3/27/24 17:23, Miquel Raynal wrote:
> Hi Lizhi,
>
>>> @@ -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;
> Maybe -EBUSY in this case?
>
> I thought synchronize() was mandatory in-between. If that's not the
> case then indeed we need to block or error-out if a new transfer
> gets started.

Okay. It looks issue_pending is not expected between terminate_all() and 
synchronize().

This check is not needed.


Thanks,

Lizhi

>
>> at the beginning of xdma_xfer_start().
>>
>> xdma_xfer_start() is protected by chan lock.
>>
>>>    >   	return 0;
>>>    }
> Thanks,
> Miquèl
diff mbox series

Patch

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);
 
 	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;
-
-	/* 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));
 
 	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);
+
 	spin_lock(&xchan->vchan.lock);
 
 	/* get submitted request */