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: 2552361 Return-Path: X-Original-To: patchwork-linux-wireless@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork1.kernel.org (Postfix) with ESMTP id C24A23FD85 for ; Fri, 10 May 2013 22:53:31 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755074Ab3EJWxE (ORCPT ); Fri, 10 May 2013 18:53:04 -0400 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) 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=KpZSRLTP8VJDAA1m93JPtio0wuiZbraZUQn83TGhTIcqRmBBr8GgPgulumC+es+8D+ VzKrOVBKpuMzbTHCS/tqnjA1HKn8xi1h2zCKxME3um/WOzBLHH7ppLFWX80J4WYDtLMD em8dQd//IpbLbgRLYBh5owXRupfCWjIDokipfwTv4ykAMY4Kyd6jT+eAHsoQD6narNrk X8YtbjsE11Dv5c6C/bHvWz8tLgTZsjiAul58POLcS5P/2jidLv8f7qoPJdtdJq8RPthP 6cZrifUpfuD513M/WxUO2FpLYgeDgv553dhko+agnqwObdAXoTUFjqG4Fl9t69EQJ9r5 tVqA== 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: ALoCoQmgPkGZzEw28v2RcGy7jefVMtkLli54iQ30JIv8dkAeCRU1eFDP5N1B0URTx7mGGP7QAOCxhufhtBKp6xbpA1Yw7lKNVRn9pXtzu0HPckxZtbpwaPp+8OQqW+oorvHwD5qXyLchAoziWiPNx/6/Q4REy1PGBzEMZ9BIOkOhf6idu9dQRfx+hv3PzOLv03aK98Wmtfib1ksinfxbc3lM2Tqiz7r8WA== Sender: linux-wireless-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-wireless@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 linux-wireless" 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); } }