All of lore.kernel.org
 help / color / mirror / Atom feed
* driver/dma/ioat: Call del_timer_sync() without holding prep_lock
@ 2018-09-14 18:53 ` Waiman Long
  0 siblings, 0 replies; 6+ messages in thread
From: Waiman Long @ 2018-09-14 18:53 UTC (permalink / raw)
  To: Vinod Koul
  Cc: linux-kernel, dmaengine, Dan Williams, Dave Jiang, Kees Cook,
	Christophe JAILLET, Waiman Long

The following lockdep splat was observed:

[ 1222.241750] ======================================================
[ 1222.271301] WARNING: possible circular locking dependency detected
[ 1222.301060] 4.16.0-10.el8+5.x86_64+debug #1 Not tainted
[ 1222.326659] ------------------------------------------------------
[ 1222.356565] systemd-shutdow/1 is trying to acquire lock:
[ 1222.382660]  ((&ioat_chan->timer)){+.-.}, at: [<00000000f71e1a28>] del_timer_sync+0x5/0xf0
[ 1222.422928]
[ 1222.422928] but task is already holding lock:
[ 1222.451743]  (&(&ioat_chan->prep_lock)->rlock){+.-.}, at: [<000000008ea98b12>] ioat_shutdown+0x86/0x100 [ioatdma]
   :
[ 1223.524987] Chain exists of:
[ 1223.524987]   (&ioat_chan->timer) --> &(&ioat_chan->cleanup_lock)->rlock --> &(&ioat_chan->prep_lock)->rlock
[ 1223.524987]
[ 1223.594082]  Possible unsafe locking scenario:
[ 1223.594082]
[ 1223.622630]        CPU0                    CPU1
[ 1223.645080]        ----                    ----
[ 1223.667404]   lock(&(&ioat_chan->prep_lock)->rlock);
[ 1223.691535]                                lock(&(&ioat_chan->cleanup_lock)->rlock);
[ 1223.728657]                                lock(&(&ioat_chan->prep_lock)->rlock);
[ 1223.765122]   lock((&ioat_chan->timer));
[ 1223.784095]
[ 1223.784095]  *** DEADLOCK ***
[ 1223.784095]
[ 1223.813492] 4 locks held by systemd-shutdow/1:
[ 1223.834677]  #0:  (reboot_mutex){+.+.}, at: [<0000000056d33456>] SYSC_reboot+0x10f/0x300
[ 1223.873310]  #1:  (&dev->mutex){....}, at: [<00000000258dfdd7>] device_shutdown+0x1c8/0x660
[ 1223.913604]  #2:  (&dev->mutex){....}, at: [<0000000068331147>] device_shutdown+0x1d6/0x660
[ 1223.954000]  #3:  (&(&ioat_chan->prep_lock)->rlock){+.-.}, at: [<000000008ea98b12>] ioat_shutdown+0x86/0x100 [ioatdma]

In the ioat_shutdown() function:

	spin_lock_bh(&ioat_chan->prep_lock);
	set_bit(IOAT_CHAN_DOWN, &ioat_chan->state);
	del_timer_sync(&ioat_chan->timer);
	spin_unlock_bh(&ioat_chan->prep_lock);

According to the synchronization rule for the del_timer_sync() function,
the caller must not hold locks which would prevent completion of the
timer's handler.

The timer structure has its own lock that manages its synchronization.
Setting the IOAT_CHAN_DOWN bit should prevent other CPUs from
trying to use that device anyway, there is probably no need to call
del_timer_sync() while holding the prep_lock. So the del_timer_sync()
call is now moved outside of the prep_lock critical section to prevent
the circular lock dependency.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 drivers/dma/ioat/init.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c
index 4fa4c06..21a5708 100644
--- a/drivers/dma/ioat/init.c
+++ b/drivers/dma/ioat/init.c
@@ -1205,8 +1205,15 @@ static void ioat_shutdown(struct pci_dev *pdev)
 
 		spin_lock_bh(&ioat_chan->prep_lock);
 		set_bit(IOAT_CHAN_DOWN, &ioat_chan->state);
-		del_timer_sync(&ioat_chan->timer);
 		spin_unlock_bh(&ioat_chan->prep_lock);
+		/*
+		 * Synchronization rule for del_timer_sync():
+		 *  - The caller must not hold locks which would prevent
+		 *    completion of the timer's handler.
+		 * So prep_lock cannot be held before calling it.
+		 */
+		del_timer_sync(&ioat_chan->timer);
+
 		/* this should quiesce then reset */
 		ioat_reset_hw(ioat_chan);
 	}

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH] driver/dma/ioat: Call del_timer_sync() without holding prep_lock
@ 2018-09-14 18:53 ` Waiman Long
  0 siblings, 0 replies; 6+ messages in thread
From: Waiman Long @ 2018-09-14 18:53 UTC (permalink / raw)
  To: Vinod Koul
  Cc: linux-kernel, dmaengine, Dan Williams, Dave Jiang, Kees Cook,
	Christophe JAILLET, Waiman Long

The following lockdep splat was observed:

[ 1222.241750] ======================================================
[ 1222.271301] WARNING: possible circular locking dependency detected
[ 1222.301060] 4.16.0-10.el8+5.x86_64+debug #1 Not tainted
[ 1222.326659] ------------------------------------------------------
[ 1222.356565] systemd-shutdow/1 is trying to acquire lock:
[ 1222.382660]  ((&ioat_chan->timer)){+.-.}, at: [<00000000f71e1a28>] del_timer_sync+0x5/0xf0
[ 1222.422928]
[ 1222.422928] but task is already holding lock:
[ 1222.451743]  (&(&ioat_chan->prep_lock)->rlock){+.-.}, at: [<000000008ea98b12>] ioat_shutdown+0x86/0x100 [ioatdma]
   :
[ 1223.524987] Chain exists of:
[ 1223.524987]   (&ioat_chan->timer) --> &(&ioat_chan->cleanup_lock)->rlock --> &(&ioat_chan->prep_lock)->rlock
[ 1223.524987]
[ 1223.594082]  Possible unsafe locking scenario:
[ 1223.594082]
[ 1223.622630]        CPU0                    CPU1
[ 1223.645080]        ----                    ----
[ 1223.667404]   lock(&(&ioat_chan->prep_lock)->rlock);
[ 1223.691535]                                lock(&(&ioat_chan->cleanup_lock)->rlock);
[ 1223.728657]                                lock(&(&ioat_chan->prep_lock)->rlock);
[ 1223.765122]   lock((&ioat_chan->timer));
[ 1223.784095]
[ 1223.784095]  *** DEADLOCK ***
[ 1223.784095]
[ 1223.813492] 4 locks held by systemd-shutdow/1:
[ 1223.834677]  #0:  (reboot_mutex){+.+.}, at: [<0000000056d33456>] SYSC_reboot+0x10f/0x300
[ 1223.873310]  #1:  (&dev->mutex){....}, at: [<00000000258dfdd7>] device_shutdown+0x1c8/0x660
[ 1223.913604]  #2:  (&dev->mutex){....}, at: [<0000000068331147>] device_shutdown+0x1d6/0x660
[ 1223.954000]  #3:  (&(&ioat_chan->prep_lock)->rlock){+.-.}, at: [<000000008ea98b12>] ioat_shutdown+0x86/0x100 [ioatdma]

In the ioat_shutdown() function:

	spin_lock_bh(&ioat_chan->prep_lock);
	set_bit(IOAT_CHAN_DOWN, &ioat_chan->state);
	del_timer_sync(&ioat_chan->timer);
	spin_unlock_bh(&ioat_chan->prep_lock);

According to the synchronization rule for the del_timer_sync() function,
the caller must not hold locks which would prevent completion of the
timer's handler.

The timer structure has its own lock that manages its synchronization.
Setting the IOAT_CHAN_DOWN bit should prevent other CPUs from
trying to use that device anyway, there is probably no need to call
del_timer_sync() while holding the prep_lock. So the del_timer_sync()
call is now moved outside of the prep_lock critical section to prevent
the circular lock dependency.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 drivers/dma/ioat/init.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c
index 4fa4c06..21a5708 100644
--- a/drivers/dma/ioat/init.c
+++ b/drivers/dma/ioat/init.c
@@ -1205,8 +1205,15 @@ static void ioat_shutdown(struct pci_dev *pdev)
 
 		spin_lock_bh(&ioat_chan->prep_lock);
 		set_bit(IOAT_CHAN_DOWN, &ioat_chan->state);
-		del_timer_sync(&ioat_chan->timer);
 		spin_unlock_bh(&ioat_chan->prep_lock);
+		/*
+		 * Synchronization rule for del_timer_sync():
+		 *  - The caller must not hold locks which would prevent
+		 *    completion of the timer's handler.
+		 * So prep_lock cannot be held before calling it.
+		 */
+		del_timer_sync(&ioat_chan->timer);
+
 		/* this should quiesce then reset */
 		ioat_reset_hw(ioat_chan);
 	}
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* driver/dma/ioat: Call del_timer_sync() without holding prep_lock
  2018-09-14 18:53 ` [PATCH] " Waiman Long
@ 2018-09-14 19:35 ` Dave Jiang
  -1 siblings, 0 replies; 6+ messages in thread
From: Dave Jiang @ 2018-09-14 19:35 UTC (permalink / raw)
  To: Waiman Long, Vinod Koul
  Cc: linux-kernel, dmaengine, Williams, Dan J, Kees Cook, Christophe JAILLET

On 09/14/2018 11:53 AM, Waiman Long wrote:
> The following lockdep splat was observed:
> 
> [ 1222.241750] ======================================================
> [ 1222.271301] WARNING: possible circular locking dependency detected
> [ 1222.301060] 4.16.0-10.el8+5.x86_64+debug #1 Not tainted
> [ 1222.326659] ------------------------------------------------------
> [ 1222.356565] systemd-shutdow/1 is trying to acquire lock:
> [ 1222.382660]  ((&ioat_chan->timer)){+.-.}, at: [<00000000f71e1a28>] del_timer_sync+0x5/0xf0
> [ 1222.422928]
> [ 1222.422928] but task is already holding lock:
> [ 1222.451743]  (&(&ioat_chan->prep_lock)->rlock){+.-.}, at: [<000000008ea98b12>] ioat_shutdown+0x86/0x100 [ioatdma]
>    :
> [ 1223.524987] Chain exists of:
> [ 1223.524987]   (&ioat_chan->timer) --> &(&ioat_chan->cleanup_lock)->rlock --> &(&ioat_chan->prep_lock)->rlock
> [ 1223.524987]
> [ 1223.594082]  Possible unsafe locking scenario:
> [ 1223.594082]
> [ 1223.622630]        CPU0                    CPU1
> [ 1223.645080]        ----                    ----
> [ 1223.667404]   lock(&(&ioat_chan->prep_lock)->rlock);
> [ 1223.691535]                                lock(&(&ioat_chan->cleanup_lock)->rlock);
> [ 1223.728657]                                lock(&(&ioat_chan->prep_lock)->rlock);
> [ 1223.765122]   lock((&ioat_chan->timer));
> [ 1223.784095]
> [ 1223.784095]  *** DEADLOCK ***
> [ 1223.784095]
> [ 1223.813492] 4 locks held by systemd-shutdow/1:
> [ 1223.834677]  #0:  (reboot_mutex){+.+.}, at: [<0000000056d33456>] SYSC_reboot+0x10f/0x300
> [ 1223.873310]  #1:  (&dev->mutex){....}, at: [<00000000258dfdd7>] device_shutdown+0x1c8/0x660
> [ 1223.913604]  #2:  (&dev->mutex){....}, at: [<0000000068331147>] device_shutdown+0x1d6/0x660
> [ 1223.954000]  #3:  (&(&ioat_chan->prep_lock)->rlock){+.-.}, at: [<000000008ea98b12>] ioat_shutdown+0x86/0x100 [ioatdma]
> 
> In the ioat_shutdown() function:
> 
> 	spin_lock_bh(&ioat_chan->prep_lock);
> 	set_bit(IOAT_CHAN_DOWN, &ioat_chan->state);
> 	del_timer_sync(&ioat_chan->timer);
> 	spin_unlock_bh(&ioat_chan->prep_lock);
> 
> According to the synchronization rule for the del_timer_sync() function,
> the caller must not hold locks which would prevent completion of the
> timer's handler.
> 
> The timer structure has its own lock that manages its synchronization.
> Setting the IOAT_CHAN_DOWN bit should prevent other CPUs from
> trying to use that device anyway, there is probably no need to call
> del_timer_sync() while holding the prep_lock. So the del_timer_sync()
> call is now moved outside of the prep_lock critical section to prevent
> the circular lock dependency.
> 
> Signed-off-by: Waiman Long <longman@redhat.com>

Thanks!

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>  drivers/dma/ioat/init.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c
> index 4fa4c06..21a5708 100644
> --- a/drivers/dma/ioat/init.c
> +++ b/drivers/dma/ioat/init.c
> @@ -1205,8 +1205,15 @@ static void ioat_shutdown(struct pci_dev *pdev)
>  
>  		spin_lock_bh(&ioat_chan->prep_lock);
>  		set_bit(IOAT_CHAN_DOWN, &ioat_chan->state);
> -		del_timer_sync(&ioat_chan->timer);
>  		spin_unlock_bh(&ioat_chan->prep_lock);
> +		/*
> +		 * Synchronization rule for del_timer_sync():
> +		 *  - The caller must not hold locks which would prevent
> +		 *    completion of the timer's handler.
> +		 * So prep_lock cannot be held before calling it.
> +		 */
> +		del_timer_sync(&ioat_chan->timer);
> +
>  		/* this should quiesce then reset */
>  		ioat_reset_hw(ioat_chan);
>  	}
>

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] driver/dma/ioat: Call del_timer_sync() without holding prep_lock
@ 2018-09-14 19:35 ` Dave Jiang
  0 siblings, 0 replies; 6+ messages in thread
From: Dave Jiang @ 2018-09-14 19:35 UTC (permalink / raw)
  To: Waiman Long, Vinod Koul
  Cc: linux-kernel, dmaengine, Williams, Dan J, Kees Cook, Christophe JAILLET



On 09/14/2018 11:53 AM, Waiman Long wrote:
> The following lockdep splat was observed:
> 
> [ 1222.241750] ======================================================
> [ 1222.271301] WARNING: possible circular locking dependency detected
> [ 1222.301060] 4.16.0-10.el8+5.x86_64+debug #1 Not tainted
> [ 1222.326659] ------------------------------------------------------
> [ 1222.356565] systemd-shutdow/1 is trying to acquire lock:
> [ 1222.382660]  ((&ioat_chan->timer)){+.-.}, at: [<00000000f71e1a28>] del_timer_sync+0x5/0xf0
> [ 1222.422928]
> [ 1222.422928] but task is already holding lock:
> [ 1222.451743]  (&(&ioat_chan->prep_lock)->rlock){+.-.}, at: [<000000008ea98b12>] ioat_shutdown+0x86/0x100 [ioatdma]
>    :
> [ 1223.524987] Chain exists of:
> [ 1223.524987]   (&ioat_chan->timer) --> &(&ioat_chan->cleanup_lock)->rlock --> &(&ioat_chan->prep_lock)->rlock
> [ 1223.524987]
> [ 1223.594082]  Possible unsafe locking scenario:
> [ 1223.594082]
> [ 1223.622630]        CPU0                    CPU1
> [ 1223.645080]        ----                    ----
> [ 1223.667404]   lock(&(&ioat_chan->prep_lock)->rlock);
> [ 1223.691535]                                lock(&(&ioat_chan->cleanup_lock)->rlock);
> [ 1223.728657]                                lock(&(&ioat_chan->prep_lock)->rlock);
> [ 1223.765122]   lock((&ioat_chan->timer));
> [ 1223.784095]
> [ 1223.784095]  *** DEADLOCK ***
> [ 1223.784095]
> [ 1223.813492] 4 locks held by systemd-shutdow/1:
> [ 1223.834677]  #0:  (reboot_mutex){+.+.}, at: [<0000000056d33456>] SYSC_reboot+0x10f/0x300
> [ 1223.873310]  #1:  (&dev->mutex){....}, at: [<00000000258dfdd7>] device_shutdown+0x1c8/0x660
> [ 1223.913604]  #2:  (&dev->mutex){....}, at: [<0000000068331147>] device_shutdown+0x1d6/0x660
> [ 1223.954000]  #3:  (&(&ioat_chan->prep_lock)->rlock){+.-.}, at: [<000000008ea98b12>] ioat_shutdown+0x86/0x100 [ioatdma]
> 
> In the ioat_shutdown() function:
> 
> 	spin_lock_bh(&ioat_chan->prep_lock);
> 	set_bit(IOAT_CHAN_DOWN, &ioat_chan->state);
> 	del_timer_sync(&ioat_chan->timer);
> 	spin_unlock_bh(&ioat_chan->prep_lock);
> 
> According to the synchronization rule for the del_timer_sync() function,
> the caller must not hold locks which would prevent completion of the
> timer's handler.
> 
> The timer structure has its own lock that manages its synchronization.
> Setting the IOAT_CHAN_DOWN bit should prevent other CPUs from
> trying to use that device anyway, there is probably no need to call
> del_timer_sync() while holding the prep_lock. So the del_timer_sync()
> call is now moved outside of the prep_lock critical section to prevent
> the circular lock dependency.
> 
> Signed-off-by: Waiman Long <longman@redhat.com>

Thanks!

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>  drivers/dma/ioat/init.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c
> index 4fa4c06..21a5708 100644
> --- a/drivers/dma/ioat/init.c
> +++ b/drivers/dma/ioat/init.c
> @@ -1205,8 +1205,15 @@ static void ioat_shutdown(struct pci_dev *pdev)
>  
>  		spin_lock_bh(&ioat_chan->prep_lock);
>  		set_bit(IOAT_CHAN_DOWN, &ioat_chan->state);
> -		del_timer_sync(&ioat_chan->timer);
>  		spin_unlock_bh(&ioat_chan->prep_lock);
> +		/*
> +		 * Synchronization rule for del_timer_sync():
> +		 *  - The caller must not hold locks which would prevent
> +		 *    completion of the timer's handler.
> +		 * So prep_lock cannot be held before calling it.
> +		 */
> +		del_timer_sync(&ioat_chan->timer);
> +
>  		/* this should quiesce then reset */
>  		ioat_reset_hw(ioat_chan);
>  	}
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread
* driver/dma/ioat: Call del_timer_sync() without holding prep_lock
  2018-09-14 18:53 ` [PATCH] " Waiman Long
@ 2018-09-18 19:20 ` Vinod
  -1 siblings, 0 replies; 6+ messages in thread
From: Vinod Koul @ 2018-09-18 19:20 UTC (permalink / raw)
  To: Waiman Long
  Cc: linux-kernel, dmaengine, Dan Williams, Dave Jiang, Kees Cook,
	Christophe JAILLET

On 14-09-18, 14:53, Waiman Long wrote:
> The following lockdep splat was observed:
> 
> [ 1222.241750] ======================================================
> [ 1222.271301] WARNING: possible circular locking dependency detected
> [ 1222.301060] 4.16.0-10.el8+5.x86_64+debug #1 Not tainted
> [ 1222.326659] ------------------------------------------------------
> [ 1222.356565] systemd-shutdow/1 is trying to acquire lock:
> [ 1222.382660]  ((&ioat_chan->timer)){+.-.}, at: [<00000000f71e1a28>] del_timer_sync+0x5/0xf0
> [ 1222.422928]
> [ 1222.422928] but task is already holding lock:
> [ 1222.451743]  (&(&ioat_chan->prep_lock)->rlock){+.-.}, at: [<000000008ea98b12>] ioat_shutdown+0x86/0x100 [ioatdma]
>    :
> [ 1223.524987] Chain exists of:
> [ 1223.524987]   (&ioat_chan->timer) --> &(&ioat_chan->cleanup_lock)->rlock --> &(&ioat_chan->prep_lock)->rlock
> [ 1223.524987]
> [ 1223.594082]  Possible unsafe locking scenario:
> [ 1223.594082]
> [ 1223.622630]        CPU0                    CPU1
> [ 1223.645080]        ----                    ----
> [ 1223.667404]   lock(&(&ioat_chan->prep_lock)->rlock);
> [ 1223.691535]                                lock(&(&ioat_chan->cleanup_lock)->rlock);
> [ 1223.728657]                                lock(&(&ioat_chan->prep_lock)->rlock);
> [ 1223.765122]   lock((&ioat_chan->timer));
> [ 1223.784095]
> [ 1223.784095]  *** DEADLOCK ***
> [ 1223.784095]
> [ 1223.813492] 4 locks held by systemd-shutdow/1:
> [ 1223.834677]  #0:  (reboot_mutex){+.+.}, at: [<0000000056d33456>] SYSC_reboot+0x10f/0x300
> [ 1223.873310]  #1:  (&dev->mutex){....}, at: [<00000000258dfdd7>] device_shutdown+0x1c8/0x660
> [ 1223.913604]  #2:  (&dev->mutex){....}, at: [<0000000068331147>] device_shutdown+0x1d6/0x660
> [ 1223.954000]  #3:  (&(&ioat_chan->prep_lock)->rlock){+.-.}, at: [<000000008ea98b12>] ioat_shutdown+0x86/0x100 [ioatdma]
> 
> In the ioat_shutdown() function:
> 
> 	spin_lock_bh(&ioat_chan->prep_lock);
> 	set_bit(IOAT_CHAN_DOWN, &ioat_chan->state);
> 	del_timer_sync(&ioat_chan->timer);
> 	spin_unlock_bh(&ioat_chan->prep_lock);
> 
> According to the synchronization rule for the del_timer_sync() function,
> the caller must not hold locks which would prevent completion of the
> timer's handler.
> 
> The timer structure has its own lock that manages its synchronization.
> Setting the IOAT_CHAN_DOWN bit should prevent other CPUs from
> trying to use that device anyway, there is probably no need to call
> del_timer_sync() while holding the prep_lock. So the del_timer_sync()
> call is now moved outside of the prep_lock critical section to prevent
> the circular lock dependency.

Applied, thanks

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] driver/dma/ioat: Call del_timer_sync() without holding prep_lock
@ 2018-09-18 19:20 ` Vinod
  0 siblings, 0 replies; 6+ messages in thread
From: Vinod @ 2018-09-18 19:20 UTC (permalink / raw)
  To: Waiman Long
  Cc: linux-kernel, dmaengine, Dan Williams, Dave Jiang, Kees Cook,
	Christophe JAILLET

On 14-09-18, 14:53, Waiman Long wrote:
> The following lockdep splat was observed:
> 
> [ 1222.241750] ======================================================
> [ 1222.271301] WARNING: possible circular locking dependency detected
> [ 1222.301060] 4.16.0-10.el8+5.x86_64+debug #1 Not tainted
> [ 1222.326659] ------------------------------------------------------
> [ 1222.356565] systemd-shutdow/1 is trying to acquire lock:
> [ 1222.382660]  ((&ioat_chan->timer)){+.-.}, at: [<00000000f71e1a28>] del_timer_sync+0x5/0xf0
> [ 1222.422928]
> [ 1222.422928] but task is already holding lock:
> [ 1222.451743]  (&(&ioat_chan->prep_lock)->rlock){+.-.}, at: [<000000008ea98b12>] ioat_shutdown+0x86/0x100 [ioatdma]
>    :
> [ 1223.524987] Chain exists of:
> [ 1223.524987]   (&ioat_chan->timer) --> &(&ioat_chan->cleanup_lock)->rlock --> &(&ioat_chan->prep_lock)->rlock
> [ 1223.524987]
> [ 1223.594082]  Possible unsafe locking scenario:
> [ 1223.594082]
> [ 1223.622630]        CPU0                    CPU1
> [ 1223.645080]        ----                    ----
> [ 1223.667404]   lock(&(&ioat_chan->prep_lock)->rlock);
> [ 1223.691535]                                lock(&(&ioat_chan->cleanup_lock)->rlock);
> [ 1223.728657]                                lock(&(&ioat_chan->prep_lock)->rlock);
> [ 1223.765122]   lock((&ioat_chan->timer));
> [ 1223.784095]
> [ 1223.784095]  *** DEADLOCK ***
> [ 1223.784095]
> [ 1223.813492] 4 locks held by systemd-shutdow/1:
> [ 1223.834677]  #0:  (reboot_mutex){+.+.}, at: [<0000000056d33456>] SYSC_reboot+0x10f/0x300
> [ 1223.873310]  #1:  (&dev->mutex){....}, at: [<00000000258dfdd7>] device_shutdown+0x1c8/0x660
> [ 1223.913604]  #2:  (&dev->mutex){....}, at: [<0000000068331147>] device_shutdown+0x1d6/0x660
> [ 1223.954000]  #3:  (&(&ioat_chan->prep_lock)->rlock){+.-.}, at: [<000000008ea98b12>] ioat_shutdown+0x86/0x100 [ioatdma]
> 
> In the ioat_shutdown() function:
> 
> 	spin_lock_bh(&ioat_chan->prep_lock);
> 	set_bit(IOAT_CHAN_DOWN, &ioat_chan->state);
> 	del_timer_sync(&ioat_chan->timer);
> 	spin_unlock_bh(&ioat_chan->prep_lock);
> 
> According to the synchronization rule for the del_timer_sync() function,
> the caller must not hold locks which would prevent completion of the
> timer's handler.
> 
> The timer structure has its own lock that manages its synchronization.
> Setting the IOAT_CHAN_DOWN bit should prevent other CPUs from
> trying to use that device anyway, there is probably no need to call
> del_timer_sync() while holding the prep_lock. So the del_timer_sync()
> call is now moved outside of the prep_lock critical section to prevent
> the circular lock dependency.

Applied, thanks

-- 
~Vinod

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-09-18 19:20 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-14 18:53 driver/dma/ioat: Call del_timer_sync() without holding prep_lock Waiman Long
2018-09-14 18:53 ` [PATCH] " Waiman Long
2018-09-14 19:35 Dave Jiang
2018-09-14 19:35 ` [PATCH] " Dave Jiang
2018-09-18 19:20 Vinod Koul
2018-09-18 19:20 ` [PATCH] " Vinod

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.