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 <[email protected]>
> Signed-off-by: Bjorn Helgaas <[email protected]>
> CC: Matthew Garrett <[email protected]>
> CC: [email protected]
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);
>
