From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 185FDECE560 for ; Fri, 14 Sep 2018 19:35:48 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id B0D7E20853 for ; Fri, 14 Sep 2018 19:35:47 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B0D7E20853 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727941AbeIOAvj (ORCPT ); Fri, 14 Sep 2018 20:51:39 -0400 Received: from mga18.intel.com ([134.134.136.126]:38222 "EHLO mga18.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726891AbeIOAvj (ORCPT ); Fri, 14 Sep 2018 20:51:39 -0400 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 14 Sep 2018 12:35:45 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.53,374,1531810800"; d="scan'208";a="83598515" Received: from djiang5-desk3.ch.intel.com ([143.182.136.93]) by orsmga003.jf.intel.com with ESMTP; 14 Sep 2018 12:35:23 -0700 Subject: Re: [PATCH] driver/dma/ioat: Call del_timer_sync() without holding prep_lock To: Waiman Long , Vinod Koul Cc: "linux-kernel@vger.kernel.org" , "dmaengine@vger.kernel.org" , "Williams, Dan J" , Kees Cook , Christophe JAILLET References: <1536951212-18050-1-git-send-email-longman@redhat.com> From: Dave Jiang Openpgp: preference=signencrypt Autocrypt: addr=dave.jiang@intel.com; prefer-encrypt=mutual; keydata= xsPuBE6TbysRDACKOBHZT4ez/3/idMBVQP+cMIJAWfTTLqbHVYLdHMHh4h6IXWLqWgc9AYTx /ajdOrBVGSK9kMuvqRi0iRO1QLOMUAIc2n/44vh/3Fe54QYfgbndeXhHZi7YEwjiTCbpQ336 pS0rS2qQaA8GzFwu96OslLI05j9Ygaqy73qmuk3wxomIYiu9a97aN3oVv1RyTp6gJK1NWT3J On17P1yWUYPvY3KJtpVqnRLkLZeOIiOahgf9+qiYqPhKQI1Ycx4YhbqkNmDG1VqdMtEWREZO DpTti6oecydN37MW1Y+YSzWYDVLWfoLUr2tBveGCRLf/U2n+Tm2PlJR0IZq+BhtuIUVcRLQW vI+XenR8j3vHVNHs9UXW/FPB8Xb5fwY2bJniZ+B4G67nwelhMNWe7H9IcEaI7Eo32fZk+9fo x6GDAhdT0pEetwuhkmI0YYD7cQj1mEx1oEbzX2p/HRW9sHTSv0V2zKbkPvii3qgvCoDb1uLd 4661UoSG0CYaAx8TwBxUqjsBAO9FXDhLHZJadyHmWp64xQGnNgBathuqoSsIWgQWBpfhDACA OYftX52Wp4qc3ZT06NPzGTV35xr4DVftxxUHiwzB/bzARfK8tdoW4A44gN3P03DAu+UqLoqm UP/e8gSLEjoaebjMu8c2iuOhk1ayHkDPc2gugTgLLBWPkhvIEV4rUV9C7TsgAAvNNDAe8X00 Tu1m01A4ToLpYsNWEtM9ZRdKXSo6YS45DFRhel29ZRz24j4ZNIxN9Bee/fn7FrL4HgO01yH+ QULDAtU87AkVoBdU5xBJVj7tGosuV+ia4UCWXjTzb+ERek2503OvNq4xqche3RMoZLsSHiOj 5PjMNX4EA6pf5kRWdNutjmAsXrpZrnviWMPy+zHUzHIw/gaI00lHMjS0P99A7ay/9BjtsIBx lJZ09Kp6SE0EiZpFIxB5D0ji6rHu3Qblwq+WjM2+1pydVxqt2vt7+IZgEB4Qm6rml835UB89 TTkMtiIXJ+hMC/hajIuFSah+CDkfagcrt1qiaVoEAs/1cCuAER+h5ClMnLZPPxNxphsqkXxn 3MVJcMEL/iaMimP3oDXJoK3O+u3gC3p55A/LYZJ7hP9lHTT4MtgwmgBp9xPeVFWx3rwQOKix SPONHlkjfvn4dUHmaOmJyKgtt5htpox+XhBkuCZ5UWpQ40/GyVypWyBXtqNx/0IKByXy4QVm QjUL/U2DchYhW+2w8rghIhkuHX2YOdldyEvXkzN8ysGR31TDwshg600k4Q/UF/MouC2ZNeMa y8I0whHBFTwSjN5T1F9cvko4PsHNB3QH4M4tbArwn4RzSX6Hfxoq59ziyI4Et6sE5SyiVEZQ DhKZ8VU61uUaYHDdid8xKU4sV5IFCERIoIwieEAkITNvCdFtuXl9gugzld7IHbOTRaGy4M+M gOyAvSe5ysBrXhY+B0d+EYif1I8s4PbnkH2xehof++lQuy3+1TZcweSx1f/uF6d92ZDkvJzQ QbkicMLaPy0IS5XIMkkpD1zIO0jeaHcTm3uzB9k8N9y4tA2ELWVR/iFZigrtrwpIJtJLUieB 89EOJLR6xbksSrFhQ80oRGF2ZSBKaWFuZyAoV29yaykgPGRhdmUuamlhbmdAaW50ZWwuY29t PsJ9BBMRCAAlAhsjBgsJCAcDAgYVCAIJCgsEFgIDAQIeAQIXgAUCUZEwDwIZAQAKCRBkFcTx ZqO5Ps8HAP4kF/KAor80fNwT7osSHGG5rLFPR/Yc5V0QpqkU8DhZDgEAoStRa/a6Mtq3Ri1H B84kFIqSQ9ME5049k6k1K7wdXcvOwE0ETpNvKxAEANGHLx0q/R99wzbVdnRthIZttNQ6M4R8 AAtEypE9JG3PLrEd9MUB5wf0fB/2Jypec3x935mRW3Zt1i+TrzjQDzMV5RyTtpWI7PwIh5IZ 0h4OV2yQHFVViHi6lubCRypQYiMzTmEKua3LeBGvUR9vVmpPJZ/UP6VajKqywjPHYBwLAAMF A/9B/PdGc1sZHno0ezuwZO2J9BOsvASNUzamO9to5P9VHTA6UqRvyfXJpNxLF1HjT4ax7Xn4 wGr6V1DCG3JYBmwIZjfinrLINKEK43L+sLbVVi8Mypc32HhNx/cPewROY2vPb4U7y3jhPBtt lt0ZMb75Lh7zY3TnGLOx1AEzmqwZSMJhBBgRCAAJBQJOk28rAhsMAAoJEGQVxPFmo7k+qiUB AKH0QWC+BBBn3pa9tzOz5hTrup+GIzf5TcuCsiAjISEqAPkBTGk5iiGrrHkxsz8VulDVpNxk o6nmKbYpUAltQObU2w== Message-ID: Date: Fri, 14 Sep 2018 12:35:22 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <1536951212-18050-1-git-send-email-longman@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 Thanks! Reviewed-by: Dave Jiang > --- > 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); > } >