Return-path: Received: from mail-ia0-f174.google.com ([209.85.210.174]:61111 "EHLO mail-ia0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754577Ab3EJWxB (ORCPT ); Fri, 10 May 2013 18:53:01 -0400 Received: by mail-ia0-f174.google.com with SMTP id j3so3952645iae.19 for ; Fri, 10 May 2013 15:53:01 -0700 (PDT) Date: Fri, 10 May 2013 16:52:57 -0600 From: Bjorn Helgaas To: Emmanuel Grumbach Cc: Matthew Garrett , Stanislaw Gruszka , "linux-pci@vger.kernel.org" , linux-wireless , John Linville , Roman Yepishev , "Guy, Wey-Yi" , Mike Miller , iss_storagedev@hp.com, Guo-Fu Tseng , netdev@vger.kernel.org, Francois Romieu , nic_swsd@realtek.com, aacraid@adaptec.com, "Rafael J. Wysocki" , linux-kernel@vger.kernel.org Subject: Re: is L1 really disabled in iwlwifi Message-ID: <20130510225257.GA10847@google.com> (sfid-20130511_005333_752869_0ABF288B) References: <1367362536.9976.9.camel@x230> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: Sender: linux-wireless-owner@vger.kernel.org List-ID: [+cc Rafael, other pci_disable_link_state() users] On Wed, May 01, 2013 at 11:13:15AM -0600, Bjorn Helgaas wrote: > On Wed, May 1, 2013 at 2:31 AM, Emmanuel Grumbach wrote: > > [from Bjorn's mail] > >> In Emmanuel's case, we don't get _OSC control, so > >> pci_disable_link_state() does nothing. > > > > Right, but this is true with the specific log I sent to you. Is it > > possible that another platform / BIOS, we *will* get _OSC control and > > that pci_disable_link_state() will actually do something? In that case > > I would prefer not to remove the call to pcie_disable_link_state(). > > Yes, absolutely, on many platforms we will get _OSC control, and > pci_disable_link_state() will work as expected. The problem is that > the driver doesn't have a good way to know whether pci_disable_link() > did anything or not. > > Today I think we have: > > 1) If the BIOS grants the OS permission to control PCIe services via > _OSC, pci_disable_link_state() works and L1 will be disabled. > > 2) If the BIOS does not grant permission, pci_disable_link_state() > does nothing and L1 may be enabled or not depending on what > configuration the BIOS did. > > If the device really doesn't work reliably when L1 is enabled, we're > currently at the mercy of the BIOS -- if the BIOS enables L1 but > doesn't grant us permission via _OSC, L1 will remain enabled (as it is > on your system). I propose the following patch. Any comments? commit cd11e3f87c4d2777cf8921c0454500c9baa54b46 Author: Bjorn Helgaas Date: Fri May 10 15:54:35 2013 -0600 PCI/ASPM: Allow drivers to disable ASPM unconditionally Some devices have hardware problems related to using ASPM. Drivers for these devices use pci_disable_link_state() to prevent their device from entering L0s or L1. But on platforms where the OS doesn't have permission to manage ASPM, pci_disable_link_state() does nothing, and the driver has no way to know this. Therefore, if the BIOS enables ASPM but declines (either via the FADT ACPI_FADT_NO_ASPM bit or the _OSC method) to allow the OS to manage it, the device can still use ASPM and trip over the hardware issue. This patch makes pci_disable_link_state() disable ASPM unconditionally, regardless of whether the OS has permission to manage ASPM in general. Reported-by: Emmanuel Grumbach Reference: https://lkml.kernel.org/r/CANUX_P3F5YhbZX3WGU-j1AGpbXb_T9Bis2ErhvKkFMtDvzatVQ@mail.gmail.com Reference: https://bugzilla.kernel.org/show_bug.cgi?id=57331 Signed-off-by: Bjorn Helgaas diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index d320df6..9ef4ab8 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -718,15 +718,11 @@ void pcie_aspm_powersave_config_link(struct pci_dev *pdev) * pci_disable_link_state - disable pci device's link state, so the link will * never enter specific states */ -static void __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem, - bool force) +static void __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem) { struct pci_dev *parent = pdev->bus->self; struct pcie_link_state *link; - if (aspm_disabled && !force) - return; - if (!pci_is_pcie(pdev)) return; @@ -757,13 +753,13 @@ static void __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem, void pci_disable_link_state_locked(struct pci_dev *pdev, int state) { - __pci_disable_link_state(pdev, state, false, false); + __pci_disable_link_state(pdev, state, false); } EXPORT_SYMBOL(pci_disable_link_state_locked); void pci_disable_link_state(struct pci_dev *pdev, int state) { - __pci_disable_link_state(pdev, state, true, false); + __pci_disable_link_state(pdev, state, true); } EXPORT_SYMBOL(pci_disable_link_state); @@ -781,7 +777,7 @@ void pcie_clear_aspm(struct pci_bus *bus) __pci_disable_link_state(child, PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1 | PCIE_LINK_STATE_CLKPM, - false, true); + false); } }