| From foo@baz Sat Nov 10 11:24:34 PST 2018 |
| From: Waiman Long <longman@redhat.com> |
| Date: Fri, 14 Sep 2018 14:53:32 -0400 |
| Subject: driver/dma/ioat: Call del_timer_sync() without holding prep_lock |
| |
| From: Waiman Long <longman@redhat.com> |
| |
| [ Upstream commit cfb03be6c7e8a1591285849c361d67b09f5149f7 ] |
| |
| 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> |
| Reviewed-by: Dave Jiang <dave.jiang@intel.com> |
| Signed-off-by: Vinod Koul <vkoul@kernel.org> |
| Signed-off-by: Sasha Levin <sashal@kernel.org> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| --- |
| drivers/dma/ioat/init.c | 9 ++++++++- |
| 1 file changed, 8 insertions(+), 1 deletion(-) |
| |
| --- a/drivers/dma/ioat/init.c |
| +++ b/drivers/dma/ioat/init.c |
| @@ -1210,8 +1210,15 @@ static void ioat_shutdown(struct pci_dev |
| |
| 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); |
| } |