From patchwork Fri May 10 22:52:57 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Bjorn Helgaas X-Patchwork-Id: 243090 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 6849C2C0159 for ; Sat, 11 May 2013 08:53:50 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755133Ab3EJWxF (ORCPT ); Fri, 10 May 2013 18:53:05 -0400 Received: from mail-ie0-f169.google.com ([209.85.223.169]:34502 "EHLO mail-ie0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754275Ab3EJWxB (ORCPT ); Fri, 10 May 2013 18:53:01 -0400 Received: by mail-ie0-f169.google.com with SMTP id u16so9326390iet.0 for ; Fri, 10 May 2013 15:53:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-received:date:from:to:cc:subject:message-id:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; bh=oWjtvU5AbH4q11JUbjcc10METuOZMC07TrUDqI5YY9k=; b=hLFei+cp/BwT/5fxOm3Sz0u6O8Yb1WctEiwfsm6ynDj2eFMhfVuhk0gGdtYLv8nwNE 8Rzd/DI1/2a3VBkqGThbPn0flR+S431fXjeGHbCdqeFivliCykerWuZFD5txwwhwZIKR vSOmetW3of7DWAJC6YMrwh2NYE/kvbCRrh9A4cVHA93JRduKAHOwdFdG1cDUjk9sTLFd PXrNfx0yo32Px9ZLFzbntCKuRSNW6cTeCMQdZCJD7ZcmT+lQAmNSoAcpYCkcOntnNdFi UsT2stpPq2ZUXHyqoCixUoxwcsWeoC38anC3v3lj2d4yL30I4dOw9hRzh3FmPH7x+33H c3lw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-received:date:from:to:cc:subject:message-id:references :mime-version:content-type:content-disposition:in-reply-to :user-agent:x-gm-message-state; bh=oWjtvU5AbH4q11JUbjcc10METuOZMC07TrUDqI5YY9k=; b=BAj3e9hom+efUzzRhkhMWbOpPXAel64oNgmQKKBfyY1zJdJbijZp0/S7aeJXX94ARv nHHsd1xkdE9NwowJyPDxFElTQ2eISnbu9+l/xPn7s1afOM2E3wAesPIShKCDnlL6o/ER zUtboGAYqZ1xuut4VZUhoYFO+h1rNjyZ8sZdZ2+NmTGt+N3gqcX0YPGVJWbYtnmnONPJ FATjtoKO3nDm8SjYzfLno5EyMEAtEkzFrZSpQX37eSoiqC8W2z3ZE79t5Rsi9SF855r+ OfMTXhR+JwYuZ9GQMlCMQdqZpmOqXWwEdLY6L7kiUU8JgiXB4D8J86S+ebPN1y+Wuzus y0Qg== X-Received: by 10.42.64.135 with SMTP id g7mr8591208ici.37.1368226380838; Fri, 10 May 2013 15:53:00 -0700 (PDT) Received: from google.com ([172.29.125.123]) by mx.google.com with ESMTPSA id c2sm957794igv.1.2013.05.10.15.52.59 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Fri, 10 May 2013 15:53:00 -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> References: <1367362536.9976.9.camel@x230> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-Gm-Message-State: ALoCoQlGHspEsaLWQ4sCKoAAZagmNPJN/ywItiGH5RO/Wu5KGKQrG/0vDwX97VyfgGgZgJQNLGlT0triNwgPEgAJUIfkrFDWi2m+5RDH/Vvfcj+3+WAOsLzXdxoAkJSyvvI37ZmtCwDozYA8C6Ma055B79YsC8+vAnnnUYBnhHMAuOQd7a4uO5EdqLHfH0L7sDKRdZ/45ttE Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org [+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 --- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html 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); } }