diff mbox

PCI/ASPM: Allow drivers to disable ASPM even without OS ASPM control

Message ID 20170124202301.20248.7452.stgit@bhelgaas-glaptop.roam.corp.google.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Bjorn Helgaas Jan. 24, 2017, 8:23 p.m. UTC
The ACPI FADT has a "PCIe ASPM Controls" bit (ACPI 5.0, sec 5.2.9.3), which
means "If set, indicates to OSPM that it must not enable ASPM control on
this platform."  We have historically interpreted that to mean that the OS
should not touch ASPM configuration at all: we leave it as the firmware
configured it.

However, a driver may know that its device has issues with ASPM and should
not have it enabled.  The platform firmware, which cannot be expected to
know this, may have enabled ASPM.  The driver should be able to use
pci_disable_link_state() to disable ASPM for its device, even if the
firmware set the ACPI_FADT_NO_ASPM bit.

Remove the check that prevents pci_disable_link_state() from disabling ASPM
for a device.

In cases where we previously emitted a message like this and did nothing:

  e1000e 0000:03:00.0: can't disable ASPM; OS doesn't have ASPM control

we'll instead actually disable ASPM on the device.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=189951
Reported-by: Mathias Koehrer <mathias.koehrer@etas.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
CC: Matthew Garrett <mjg59@coreos.com>
CC: stable@vger.kernel.org
---
 drivers/pci/pcie/aspm.c |   16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Bjorn Helgaas Feb. 10, 2017, 10:47 p.m. UTC | #1
On Tue, Jan 24, 2017 at 02:23:01PM -0600, Bjorn Helgaas wrote:
> The ACPI FADT has a "PCIe ASPM Controls" bit (ACPI 5.0, sec 5.2.9.3), which
> means "If set, indicates to OSPM that it must not enable ASPM control on
> this platform."  We have historically interpreted that to mean that the OS
> should not touch ASPM configuration at all: we leave it as the firmware
> configured it.
> 
> However, a driver may know that its device has issues with ASPM and should
> not have it enabled.  The platform firmware, which cannot be expected to
> know this, may have enabled ASPM.  The driver should be able to use
> pci_disable_link_state() to disable ASPM for its device, even if the
> firmware set the ACPI_FADT_NO_ASPM bit.
> 
> Remove the check that prevents pci_disable_link_state() from disabling ASPM
> for a device.
> 
> In cases where we previously emitted a message like this and did nothing:
> 
>   e1000e 0000:03:00.0: can't disable ASPM; OS doesn't have ASPM control
> 
> we'll instead actually disable ASPM on the device.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=189951
> Reported-by: Mathias Koehrer <mathias.koehrer@etas.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> CC: Matthew Garrett <mjg59@coreos.com>
> CC: stable@vger.kernel.org

I think this is the right thing to do, but I haven't applied this yet
because I don't think it's quite enough in its current form.

Even if I applied this, it would only allow a driver to disable ASPM
if CONFIG_PCIEASPM is enabled.  Without CONFIG_PCIEASPM, we don't
compile aspm.c and pci_disable_link_state() is a stub that does
nothing.

I think we need to make pci_disable_link_state() work even when
CONFIG_PCIEASPM is not enabled.  If we did that, we could clean up
callers like __e1000e_disable_aspm(), which contains code to disable
ASPM manually if pci_disable_link_state() doesn't work.

There are other drivers that try to disable ASPM because of device
errata, and currently that only works when CONFIG_PCIEASPM is enabled.
If we add a pci_disable_link_state() that works when CONFIG_PCIEASPM
is not enabled, it should make those drivers work better.

> ---
>  drivers/pci/pcie/aspm.c |   16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 17ac1dce3286..9253ae48d777 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -736,18 +736,10 @@ static void __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem)
>  	if (!parent || !parent->link_state)
>  		return;
>  
> -	/*
> -	 * A driver requested that ASPM be disabled on this device, but
> -	 * if we don't have permission to manage ASPM (e.g., on ACPI
> -	 * systems we have to observe the FADT ACPI_FADT_NO_ASPM bit and
> -	 * the _OSC method), we can't honor that request.  Windows has
> -	 * a similar mechanism using "PciASPMOptOut", which is also
> -	 * ignored in this situation.
> -	 */
> -	if (aspm_disabled) {
> -		dev_warn(&pdev->dev, "can't disable ASPM; OS doesn't have ASPM control\n");
> -		return;
> -	}
> +	dev_info(&pdev->dev, "disabling %sASPM%s%s\n",
> +		 (state & PCIE_LINK_STATE_CLKPM) ? "Clock PM " : "",
> +		 (state & PCIE_LINK_STATE_L0S) ? " L0s" : "",
> +		 (state & PCIE_LINK_STATE_L1) ? " L1" : "");
>  
>  	if (sem)
>  		down_read(&pci_bus_sem);
>
diff mbox

Patch

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 17ac1dce3286..9253ae48d777 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -736,18 +736,10 @@  static void __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem)
 	if (!parent || !parent->link_state)
 		return;
 
-	/*
-	 * A driver requested that ASPM be disabled on this device, but
-	 * if we don't have permission to manage ASPM (e.g., on ACPI
-	 * systems we have to observe the FADT ACPI_FADT_NO_ASPM bit and
-	 * the _OSC method), we can't honor that request.  Windows has
-	 * a similar mechanism using "PciASPMOptOut", which is also
-	 * ignored in this situation.
-	 */
-	if (aspm_disabled) {
-		dev_warn(&pdev->dev, "can't disable ASPM; OS doesn't have ASPM control\n");
-		return;
-	}
+	dev_info(&pdev->dev, "disabling %sASPM%s%s\n",
+		 (state & PCIE_LINK_STATE_CLKPM) ? "Clock PM " : "",
+		 (state & PCIE_LINK_STATE_L0S) ? " L0s" : "",
+		 (state & PCIE_LINK_STATE_L1) ? " L1" : "");
 
 	if (sem)
 		down_read(&pci_bus_sem);