From patchwork Fri Feb 3 23:58:53 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Rafael J. Wysocki" X-Patchwork-Id: 9555375 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id E5EFC602B7 for ; Sat, 4 Feb 2017 00:10:21 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id E27CF2818E for ; Sat, 4 Feb 2017 00:10:21 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id D762128478; Sat, 4 Feb 2017 00:10:21 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 5039E2818E for ; Sat, 4 Feb 2017 00:10:21 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 792726EBF3; Sat, 4 Feb 2017 00:10:19 +0000 (UTC) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org X-Greylist: delayed 399 seconds by postgrey-1.35 at gabe; Sat, 04 Feb 2017 00:10:18 UTC Received: from cloudserver094114.home.net.pl (cloudserver094114.home.net.pl [79.96.170.134]) by gabe.freedesktop.org (Postfix) with ESMTPS id 5663C6E116 for ; Sat, 4 Feb 2017 00:10:18 +0000 (UTC) Received: from adkt101.ipv4.supernova.orange.pl (79.184.253.101) (HELO aspire.rjw.lan) by serwer1319399.home.pl (79.96.170.134) with SMTP (IdeaSmtpServer 0.81.2) id 0836bbef8ad1f74e; Sat, 4 Feb 2017 01:03:40 +0100 From: "Rafael J. Wysocki" To: sedat.dilek@gmail.com Date: Sat, 04 Feb 2017 00:58:53 +0100 Message-ID: <15818372.6qr0ZfnSx1@aspire.rjw.lan> User-Agent: KMail/4.14.10 (Linux/4.10.0-rc3+; KDE/4.14.9; x86_64; ; ) In-Reply-To: References: MIME-Version: 1.0 Cc: "Rafael J. Wysocki" , Jani Nikula , Daniel Vetter , intel-gfx , the arch/x86 maintainers , LKML , Pavel Machek , Linux PM , Thomas Gleixner Subject: [Intel-gfx] [PATCH] PM / runtime: Avoid false-positive warnings from might_sleep_if() X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" X-Virus-Scanned: ClamAV using ClamSMTP On Thursday, February 02, 2017 02:34:42 PM Sedat Dilek wrote: > On Wed, Feb 1, 2017 at 1:22 AM, Rafael J. Wysocki wrote: > > On Mon, Jan 30, 2017 at 11:44 PM, Rafael J. Wysocki > > wrote: > >> On 1/24/2017 2:33 AM, Sedat Dilek wrote: > >>> > >>> On Fri, Dec 30, 2016 at 3:02 PM, Rafael J. Wysocki > >>> wrote: > >>>> > >>>> On Fri, Dec 30, 2016 at 12:40 PM, Sedat Dilek > >>>> wrote: > >>>>> > >>>>> Hi, > >>>>> > >>>>> I have already reported this issue in [1]. > >>>>> One of the issue was solved. > >>>>> Unfortunately, it looks like there is still a different problem here > >>>>> (Ubuntu/precise AMD64). > >>>>> > >>>>> I tried v4.10-rc1 and latest Linus tree up to... > >>>>> > >>>>> commit 98473f9f3f9bd404873cd1178c8be7d6d619f0d1 > >>>>> "mm/filemap: fix parameters to test_bit()" > >>>>> > >>>>> Here we go... > >>>>> > >>>>> [ 29.636047] BUG: sleeping function called from invalid context at > >>>>> drivers/base/power/runtime.c:1032 > >>>>> [ 29.636055] in_atomic(): 1, irqs_disabled(): 0, pid: 1500, name: Xorg > >>>>> [ 29.636058] 1 lock held by Xorg/1500: > >>>>> [ 29.636060] #0: (&dev->struct_mutex){+.+.+.}, at: > >>>>> [] i915_mutex_lock_interruptible+0x43/0x140 [i915] > >>>>> [ 29.636107] CPU: 0 PID: 1500 Comm: Xorg Not tainted > >>>>> 4.10.0-rc1-6-iniza-amd64 #1 > >>>>> [ 29.636109] Hardware name: SAMSUNG ELECTRONICS CO., LTD. > >>>>> 530U3BI/530U4BI/530U4BH/530U3BI/530U4BI/530U4BH, BIOS 13XK 03/28/2013 > >>>>> [ 29.636111] Call Trace: > >>>>> [ 29.636120] dump_stack+0x85/0xc2 > >>>>> [ 29.636124] ___might_sleep+0x196/0x260 > >>>>> [ 29.636127] __might_sleep+0x53/0xb0 > >>>>> [ 29.636131] __pm_runtime_resume+0x7a/0x90 > >>>>> [ 29.636159] intel_runtime_pm_get+0x25/0x90 [i915] > >>>>> [ 29.636189] aliasing_gtt_bind_vma+0xaa/0xf0 [i915] > >>>>> [ 29.636220] i915_vma_bind+0xaf/0x1e0 [i915] > >>>>> [ 29.636248] i915_gem_execbuffer_relocate_entry+0x513/0x6f0 [i915] > >>>>> [ 29.636272] i915_gem_execbuffer_relocate_vma.isra.34+0x188/0x250 > >>>>> [i915] > >>>>> [ 29.636275] ? trace_hardirqs_on+0xd/0x10 > >>>>> [ 29.636294] ? i915_gem_execbuffer_reserve_vma.isra.31+0x152/0x1f0 > >>>>> [i915] > >>>>> [ 29.636316] ? i915_gem_execbuffer_reserve.isra.32+0x372/0x3a0 [i915] > >>>>> [ 29.636342] i915_gem_do_execbuffer.isra.38+0xa70/0x1a40 [i915] > >>>>> [ 29.636347] ? __might_fault+0x4e/0xb0 > >>>>> [ 29.636373] i915_gem_execbuffer2+0xc5/0x260 [i915] > >>>>> [ 29.636376] ? __might_fault+0x4e/0xb0 > >>>>> [ 29.636395] drm_ioctl+0x206/0x450 [drm] > >>>>> [ 29.636420] ? i915_gem_execbuffer+0x340/0x340 [i915] > >>>>> [ 29.636425] ? __fget+0x5/0x200 > >>>>> [ 29.636429] do_vfs_ioctl+0x91/0x6f0 > >>>>> [ 29.636431] ? __fget+0x111/0x200 > >>>>> [ 29.636433] ? __fget+0x5/0x200 > >>>>> [ 29.636436] SyS_ioctl+0x79/0x90 > >>>>> [ 29.636441] entry_SYSCALL_64_fastpath+0x23/0xc6 > >>>>> > >>>>> On suspend/resume I see the same call trace. > >>>>> [2] points to the "BUG" line. > >>>> > >>>> Well, this appears to be an i915 issue, but not a serious one. > >>>> > >>>> Clearly, a function that may sleep (pm_runtime_get_sync() in > >>>> intel_runtime_pm_get()) is called with disabled interrupts. If I > >>>> understand the code correctly, though, it actually is not going to > >>>> sleep in this particular case, because pm_runtime_get_sync() has > >>>> already been called once for this device in the same code path which > >>>> means that this particular instance will return immediately, so this > >>>> is a false-positive (most likely). > >>>> > >>>> Let me see if I the might_sleep_if() assertion in > >>>> __pm_runtime_resume(() can be moved to a better place. > >>>> > >>> Hi Rafael, > >>> > >>> did you had a chance to look at this? > >>> The problem still remains in Linux v4.10-rc5. > >> > >> > >> No, I didn't. > >> > >> As I said, this is not a serious issue. > > > > Something like the attached (untested). > > > > Please try it and let me know if it makes the splat go away. > > > > Your patch fixes the issue here. > I tested against vanilla Linux v4.10-rc5. > > Feel free to give appropriate credits. OK, thanks! Below is a full version with a changelog & tags. Thanks, Rafael --- From: Rafael J. Wysocki Subject: [PATCH] PM / runtime: Avoid false-positive warnings from might_sleep_if() The might_sleep_if() assertions in __pm_runtime_idle(), __pm_runtime_suspend() and __pm_runtime_resume() may generate false-positive warnings in some situations. For example, that happens if a nested pm_runtime_get_sync()/pm_runtime_put() pair is executed with disabled interrupts within an outer pm_runtime_get_sync()/pm_runtime_put() section for the same device. [Generally, pm_runtime_get_sync() may sleep, so it should not be called with disabled interrupts, but in this particular case the previous pm_runtime_get_sync() guarantees that the device will not be suspended, so the inner pm_runtime_get_sync() will return immediately after incrementing the device's usage counter.] That started to happen in the i915 driver in 4.10-rc, leading to the following splat: BUG: sleeping function called from invalid context at drivers/base/power/runtime.c:1032 in_atomic(): 1, irqs_disabled(): 0, pid: 1500, name: Xorg 1 lock held by Xorg/1500: #0: (&dev->struct_mutex){+.+.+.}, at: [] i915_mutex_lock_interruptible+0x43/0x140 [i915] CPU: 0 PID: 1500 Comm: Xorg Not tainted Call Trace: dump_stack+0x85/0xc2 ___might_sleep+0x196/0x260 __might_sleep+0x53/0xb0 __pm_runtime_resume+0x7a/0x90 intel_runtime_pm_get+0x25/0x90 [i915] aliasing_gtt_bind_vma+0xaa/0xf0 [i915] i915_vma_bind+0xaf/0x1e0 [i915] i915_gem_execbuffer_relocate_entry+0x513/0x6f0 [i915] i915_gem_execbuffer_relocate_vma.isra.34+0x188/0x250 [i915] ? trace_hardirqs_on+0xd/0x10 ? i915_gem_execbuffer_reserve_vma.isra.31+0x152/0x1f0 [i915] ? i915_gem_execbuffer_reserve.isra.32+0x372/0x3a0 [i915] i915_gem_do_execbuffer.isra.38+0xa70/0x1a40 [i915] ? __might_fault+0x4e/0xb0 i915_gem_execbuffer2+0xc5/0x260 [i915] ? __might_fault+0x4e/0xb0 drm_ioctl+0x206/0x450 [drm] ? i915_gem_execbuffer+0x340/0x340 [i915] ? __fget+0x5/0x200 do_vfs_ioctl+0x91/0x6f0 ? __fget+0x111/0x200 ? __fget+0x5/0x200 SyS_ioctl+0x79/0x90 entry_SYSCALL_64_fastpath+0x23/0xc6 even though the code triggering it is correct. Unfortunately, the might_sleep_if() assertions in question are too coarse-grained to cover such cases correctly, so make them a bit less sensitive in order to avoid the false-positives. Reported-and-tested-by: Sedat Dilek Signed-off-by: Rafael J. Wysocki --- drivers/base/power/runtime.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) Index: linux-pm/drivers/base/power/runtime.c =================================================================== --- linux-pm.orig/drivers/base/power/runtime.c +++ linux-pm/drivers/base/power/runtime.c @@ -966,13 +966,13 @@ int __pm_runtime_idle(struct device *dev unsigned long flags; int retval; - might_sleep_if(!(rpmflags & RPM_ASYNC) && !dev->power.irq_safe); - if (rpmflags & RPM_GET_PUT) { if (!atomic_dec_and_test(&dev->power.usage_count)) return 0; } + might_sleep_if(!(rpmflags & RPM_ASYNC) && !dev->power.irq_safe); + spin_lock_irqsave(&dev->power.lock, flags); retval = rpm_idle(dev, rpmflags); spin_unlock_irqrestore(&dev->power.lock, flags); @@ -998,13 +998,13 @@ int __pm_runtime_suspend(struct device * unsigned long flags; int retval; - might_sleep_if(!(rpmflags & RPM_ASYNC) && !dev->power.irq_safe); - if (rpmflags & RPM_GET_PUT) { if (!atomic_dec_and_test(&dev->power.usage_count)) return 0; } + might_sleep_if(!(rpmflags & RPM_ASYNC) && !dev->power.irq_safe); + spin_lock_irqsave(&dev->power.lock, flags); retval = rpm_suspend(dev, rpmflags); spin_unlock_irqrestore(&dev->power.lock, flags); @@ -1029,7 +1029,8 @@ int __pm_runtime_resume(struct device *d unsigned long flags; int retval; - might_sleep_if(!(rpmflags & RPM_ASYNC) && !dev->power.irq_safe); + might_sleep_if(!(rpmflags & RPM_ASYNC) && !dev->power.irq_safe && + dev->power.runtime_status != RPM_ACTIVE); if (rpmflags & RPM_GET_PUT) atomic_inc(&dev->power.usage_count);