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=-5.6 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FAKE_REPLY_C,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable 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 80788ECE58C for ; Tue, 15 Oct 2019 19:20:25 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 51C8520854 for ; Tue, 15 Oct 2019 19:20:25 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1571167225; bh=nm/8Au21VodRlss2uP12IzrkFOHeWZtsigxeMX4xdvg=; h=Date:From:To:Cc:Subject:In-Reply-To:List-ID:From; b=JmXA7gdvRoRlv3tRPz7Uq7PHjuAx/+broa2tK4e42JC5zDHrMph/iTNamr0pmRPUN QGZjWfTSD6tn0cde/BqjNCTNHz9y4RZ3q9ZzCg0RGaVkWNNFB75b9KsEMxy0xA2SEz S+WzgQ/2nL3sSTb5VKk7WYjv1BuKBvxq5OE4fNUw= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729946AbfJOTUU (ORCPT ); Tue, 15 Oct 2019 15:20:20 -0400 Received: from mail.kernel.org ([198.145.29.99]:56942 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726620AbfJOTUT (ORCPT ); Tue, 15 Oct 2019 15:20:19 -0400 Received: from localhost (unknown [69.71.4.100]) (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 83F5B20854; Tue, 15 Oct 2019 19:20:17 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1571167217; bh=nm/8Au21VodRlss2uP12IzrkFOHeWZtsigxeMX4xdvg=; h=Date:From:To:Cc:Subject:In-Reply-To:From; b=Y9eQxQ8eeOLe4bEHt5LRnA/jIuE1RuSSEq1ZbkroD6GqEpkEajpxa/6OXzV77b9+N 4RHncj5xNgL9OTFt7KL2cXmo/RBVCL9jF/CrcxHnlxzwY8fvRh7NbyH0ZjkQZeLtba OIexIAahn1cXKgcQKFveVZGW2FqlDpRy7iO5k+QE= Date: Tue, 15 Oct 2019 14:20:13 -0500 From: Bjorn Helgaas To: "Rafael J. Wysocki" Cc: Linux PCI , Daniel Drake , Mathias Nyman , Linux Upstreaming Team , Linux PM , Mika Westerberg , LKML Subject: Re: [PATCH] PCI: PM: Fix pci_power_up() Message-ID: <20191015192013.GA115182@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5720276.eiOaOx1Qyb@kreacher> User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-pm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pm@vger.kernel.org On Mon, Oct 14, 2019 at 01:25:00PM +0200, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki > > There is an arbitrary difference between the system resume and > runtime resume code paths for PCI devices regarding the delay to > apply when switching the devices from D3cold to D0. > > Namely, pci_restore_standard_config() used in the runtime resume > code path calls pci_set_power_state() which in turn invokes > __pci_start_power_transition() to power up the device through the > platform firmware and that function applies the transition delay > (as per PCI Express Base Specification Revision 2.0, Section 6.6.1). > However, pci_pm_default_resume_early() used in the system resume > code path calls pci_power_up() which doesn't apply the delay at > all and that causes issues to occur during resume from > suspend-to-idle on some systems where the delay is required. > > Since there is no reason for that difference to exist, modify > pci_power_up() to follow pci_set_power_state() more closely and > invoke __pci_start_power_transition() from there to call the > platform firmware to power up the device (in case that's necessary). > > Fixes: db288c9c5f9d ("PCI / PM: restore the original behavior of pci_set_power_state()") > Reported-by: Daniel Drake > Link: https://lore.kernel.org/linux-pm/CAD8Lp44TYxrMgPLkHCqF9hv6smEurMXvmmvmtyFhZ6Q4SE+dig@mail.gmail.com/T/#m21be74af263c6a34f36e0fc5c77c5449d9406925 > Signed-off-by: Rafael J. Wysocki > --- > > Daniel, please test this one. > > --- > drivers/pci/pci.c | 24 +++++++++++------------- > 1 file changed, 11 insertions(+), 13 deletions(-) > > Index: linux-pm/drivers/pci/pci.c > =================================================================== > --- linux-pm.orig/drivers/pci/pci.c > +++ linux-pm/drivers/pci/pci.c > @@ -959,19 +959,6 @@ void pci_refresh_power_state(struct pci_ > } > > /** > - * pci_power_up - Put the given device into D0 forcibly > - * @dev: PCI device to power up > - */ > -void pci_power_up(struct pci_dev *dev) > -{ > - if (platform_pci_power_manageable(dev)) > - platform_pci_set_power_state(dev, PCI_D0); > - > - pci_raw_set_power_state(dev, PCI_D0); > - pci_update_current_state(dev, PCI_D0); > -} > - > -/** > * pci_platform_power_transition - Use platform to change device power state > * @dev: PCI device to handle. > * @state: State to put the device into. > @@ -1154,6 +1141,17 @@ int pci_set_power_state(struct pci_dev * > EXPORT_SYMBOL(pci_set_power_state); > > /** > + * pci_power_up - Put the given device into D0 forcibly Not specifically for this patch, but what does "forcibly" mean? > + * @dev: PCI device to power up > + */ > +void pci_power_up(struct pci_dev *dev) > +{ > + __pci_start_power_transition(dev, PCI_D0); > + pci_raw_set_power_state(dev, PCI_D0); > + pci_update_current_state(dev, PCI_D0); There's not very much difference between: pci_power_up(dev); and pci_set_power_state(dev, PCI_D0); It looks like the main difference is that pci_set_power_state() calls __pci_complete_power_transition(), which ultimately calls acpi_pci_set_power_state() (for ACPI systems). So maybe "forcibly" means something like "ignoring any platform power management methods"? It's not obvious to me when we should skip the platform stuff or whether the skipping should be done at the high level (like calling either pci_power_up() or pci_set_power_state()) or at a lower level (e.g., if everybody called pci_set_power_state() and it could internally tell whether we're skipping the platform part). If we could unify the paths as much as possible, that would be nice, but if it's not feasible, it's not feasible. If you'd like me to push this for v5.4, let me know, otherwise you can apply my: Acked-by: Bjorn Helgaas > +} > + > +/** > * pci_choose_state - Choose the power state of a PCI device > * @dev: PCI device to be suspended > * @state: target sleep state for the whole system. This is the value > > >