From patchwork Tue May 1 19:55:01 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Bjorn Helgaas X-Patchwork-Id: 10374399 X-Patchwork-Delegate: bhelgaas@google.com 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 DDAE96032A for ; Tue, 1 May 2018 19:55:06 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id CCDC428C7A for ; Tue, 1 May 2018 19:55:06 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id C0A8828CD3; Tue, 1 May 2018 19:55:06 +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=-6.9 required=2.0 tests=BAYES_00,HEXHASH_WORD, MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 61C6228D06 for ; Tue, 1 May 2018 19:55:06 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751027AbeEATzE (ORCPT ); Tue, 1 May 2018 15:55:04 -0400 Received: from mail.kernel.org ([198.145.29.99]:34510 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750780AbeEATzD (ORCPT ); Tue, 1 May 2018 15:55:03 -0400 Received: from localhost (unknown [69.55.156.246]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 02F4923690; Tue, 1 May 2018 19:55:02 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 02F4923690 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=helgaas@kernel.org Date: Tue, 1 May 2018 14:55:01 -0500 From: Bjorn Helgaas To: "Rafael J. Wysocki" Cc: Joseph Salisbury , "Rafael J. Wysocki" , Len Brown , Bjorn Helgaas , ACPI Devel Maling List , Linux PCI , "linux-kernel@vger.kernel.org" , 1745646@bugs.launchpad.net, Mika Westerberg Subject: Re: [Regression] PCI / PM: Simplify device wakeup settings code Message-ID: <20180501195501.GB11698@bhelgaas-glaptop.roam.corp.google.com> References: <56a8953c-d833-837c-57d5-fe758d4db02a@canonical.com> <1f67f00a-8141-f9af-2120-c78f7cfecb1d@canonical.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.2 (2017-12-15) Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Tue, May 01, 2018 at 10:34:29AM +0200, Rafael J. Wysocki wrote: > On Mon, Apr 30, 2018 at 4:22 PM, Joseph Salisbury > wrote: > > On 04/16/2018 11:58 AM, Rafael J. Wysocki wrote: > >> On Mon, Apr 16, 2018 at 5:31 PM, Joseph Salisbury > >> wrote: > >>> On 04/13/2018 05:34 PM, Rafael J. Wysocki wrote: > >>>> On Fri, Apr 13, 2018 at 7:56 PM, Joseph Salisbury > >>>> wrote: > >>>>> Hi Rafael, > >>>>> > >>>>> A kernel bug report was opened against Ubuntu [0]. After a kernel > >>>>> bisect, it was found that reverting the following two commits resolved > >>>>> this bug: > >>>>> > >>>>> 0ce3fcaff929 ("PCI / PM: Restore PME Enable after config space restoration") > >>>>> 0847684cfc5f("PCI / PM: Simplify device wakeup settings code") > >>>>> > >>>>> This is a regression introduced in v4.13-rc1 and still exists in > >>>>> mainline. The bug causes the battery to drain when the system is > >>>>> powered down and unplugged, which does not happed prior to these two > >>>>> commits. > >>>> What system and what do you mean by "powered down"? How much time > >>>> does it take for the battery to drain now? > >>> By powered down, the bug reporter is saying physically powered off and > >>> unplugged. The system is a HP laptop: > >>> > >>> dmi.chassis.vendor: HP > >>> dmi.product.family: 103C_5335KV HP Notebook > >>> dmi.product.name: HP Notebook > >>> vendor_id : GenuineIntel > >>> cpu family : 6 > >>> > >>> > >>>>> The bisect actually pointed to commit de3ef1e, but reverting > >>>>> these two commits fixes the issue. > >>>>> > >>>>> I was hoping to get your feedback, since you are the patch author. Do > >>>>> you think gathering any additional data will help diagnose this issue, > >>>>> or would it be best to submit a revert request? > >>>> First, reverting these is not an option or you will break systems > >>>> relying on them now. 4.13 is three releases back at this point. > >>>> > >>>> Second, your issue appears to be related to the suspend/shutdown path > >>>> whereas commit 0ce3fcaff929 is mostly about resume, so presumably the > >>>> change in pci_enable_wake() causes the problem to happen. Can you try > >>>> to revert this one alone and see if that helps? > >>> A test kernel with commits 0ce3fcaff929 and de3ef1eb1cd0 reverted was > >>> tested. However, the test kernel still exhibited the bug. > >> So essentially the bisection result cannot be trusted. > > > > We performed some more testing and confirmed just a revert of the > > following commit resolves the bug: > > > > 0847684cfc5f0 ("PCI / PM: Simplify device wakeup settings code") > > Thanks for confirming this! > > > Can you think of any suggestions to help debug further? > > The root cause of the regression is likely the change in > pci_enable_wake() removing the device_may_wakeup() check from it. > > Probably, one of the drivers in the platform calls pci_enable_wake() > directly from its ->shutdown() callback and that causes the device to > be set up for system wakeup which in turn causes the power draw while > the system is off to increase. > > I would look at the PCI drivers used on that platform to find which of > them call pci_enable_wake() directly from ->shutdown() and I would > make these calls conditional on device_may_wakeup(). I took a quick look with git grep -E "pci_enable_wake\(.*[^0]\);|device_may_wakeup" and didn't notice any pci_enable_wake() callers that called device_may_wakeup() first. Probably a dumb question, but would it make sense to restore the device_may_wakeup() check in pci_enable_wake(), e.g., diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index e597655a5643..9fa64c175f92 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -1932,6 +1932,9 @@ int pci_enable_wake(struct pci_dev *dev, pci_power_t state, bool enable) { int ret = 0; + if (enable && !device_may_wakeup(&dev->dev)) + return -EINVAL; + /* * Bridges can only signal wakeup on behalf of subordinate devices, * but that is set up elsewhere, so skip them.