From 7645d26f3b35c2bd6e0e1d3867636e5bdad97b1b Mon Sep 17 00:00:00 2001 From: Zhaoxiong Yuan Date: Sat, 8 Sep 2018 06:02:10 +1100 Subject: [PATCH 1/3] dmaengine: idma64: replace spin_lock_irqsave with spin_lock idma64_chan_irq() is invoked in hardirq handle function, it is unnecessary to call spin_lock_irqsave. Signed-off-by: Zhaoxiong Yuan Signed-off-by: Vinod Koul --- drivers/dma/idma64.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/dma/idma64.c b/drivers/dma/idma64.c index 1fbf9cb9b742..5b9c1566ad73 100644 --- a/drivers/dma/idma64.c +++ b/drivers/dma/idma64.c @@ -142,9 +142,8 @@ static void idma64_chan_irq(struct idma64 *idma64, unsigned short c, { struct idma64_chan *idma64c = &idma64->chan[c]; struct idma64_desc *desc; - unsigned long flags; - spin_lock_irqsave(&idma64c->vchan.lock, flags); + spin_lock(&idma64c->vchan.lock); desc = idma64c->desc; if (desc) { if (status_err & (1 << c)) { @@ -161,7 +160,7 @@ static void idma64_chan_irq(struct idma64 *idma64, unsigned short c, if (idma64c->desc == NULL || desc->status == DMA_ERROR) idma64_stop_transfer(idma64c); } - spin_unlock_irqrestore(&idma64c->vchan.lock, flags); + spin_unlock(&idma64c->vchan.lock); } static irqreturn_t idma64_irq(int irq, void *dev) From cfb03be6c7e8a1591285849c361d67b09f5149f7 Mon Sep 17 00:00:00 2001 From: Waiman Long Date: Fri, 14 Sep 2018 14:53:32 -0400 Subject: [PATCH 2/3] driver/dma/ioat: Call del_timer_sync() without holding prep_lock 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 Reviewed-by: Dave Jiang Signed-off-by: Vinod Koul --- 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 4fa4c06c9edb..21a5708985bc 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); } From f4d34aa8c887a8a2d23ef546da0efa10e3f77241 Mon Sep 17 00:00:00 2001 From: Rami Rosen Date: Fri, 5 Oct 2018 00:03:10 +0300 Subject: [PATCH 3/3] dmaengine: ioat: fix prototype of ioat_enumerate_channels Signed-off-by: Rami Rosen Signed-off-by: Vinod Koul --- drivers/dma/ioat/init.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c index 21a5708985bc..0fec3c554fe3 100644 --- a/drivers/dma/ioat/init.c +++ b/drivers/dma/ioat/init.c @@ -129,7 +129,7 @@ static void ioat_init_channel(struct ioatdma_device *ioat_dma, struct ioatdma_chan *ioat_chan, int idx); static void ioat_intr_quirk(struct ioatdma_device *ioat_dma); -static int ioat_enumerate_channels(struct ioatdma_device *ioat_dma); +static void ioat_enumerate_channels(struct ioatdma_device *ioat_dma); static int ioat3_dma_self_test(struct ioatdma_device *ioat_dma); static int ioat_dca_enabled = 1; @@ -575,7 +575,7 @@ static void ioat_dma_remove(struct ioatdma_device *ioat_dma) * ioat_enumerate_channels - find and initialize the device's channels * @ioat_dma: the ioat dma device to be enumerated */ -static int ioat_enumerate_channels(struct ioatdma_device *ioat_dma) +static void ioat_enumerate_channels(struct ioatdma_device *ioat_dma) { struct ioatdma_chan *ioat_chan; struct device *dev = &ioat_dma->pdev->dev; @@ -594,7 +594,7 @@ static int ioat_enumerate_channels(struct ioatdma_device *ioat_dma) xfercap_log = readb(ioat_dma->reg_base + IOAT_XFERCAP_OFFSET); xfercap_log &= 0x1f; /* bits [4:0] valid */ if (xfercap_log == 0) - return 0; + return; dev_dbg(dev, "%s: xfercap = %d\n", __func__, 1 << xfercap_log); for (i = 0; i < dma->chancnt; i++) { @@ -611,7 +611,6 @@ static int ioat_enumerate_channels(struct ioatdma_device *ioat_dma) } } dma->chancnt = i; - return i; } /**