ACK: [PATCH] PCI: Don't clear ASPM bits when the FADT declares it's unsupported
Colin Ian King
colin.king at canonical.com
Mon Apr 27 09:23:41 UTC 2015
On 27/04/15 07:05, Alex Hung wrote:
> Colin,
>
> Yes it works as expected. I also attached some logs from Latitude 3150
> for the references.
>
> - pci.dos - pci registers dumped in DOS (with ru.exe).
> - pci.before - pci registers without this patch (with lspci -xxx)
> with kernel 3.19
> - pci.before - pci registers with this patch (with lspci -xxx) with kernel 3.19
> - facp.dsl - FACP table with ASPM Not support set to 1
>
> I manually checked 3 devices (BUS 2/3/4 Device 0 Function 0):
>
> 1. Their ASPM is disabled in pci.before
> 2. Their ASPM is enabled in pci.after and is also match to pci.doc
>
> Cheers,
> Alex Hung
>
> On Fri, Apr 24, 2015 at 11:35 PM, Colin Ian King
> <colin.king at canonical.com> wrote:
>> On 24/04/15 07:55, Alex Hung wrote:
>>> From: Matthew Garrett <mjg59 at coreos.com>
>>>
>>> Communications with a hardware vendor confirm that the expected behaviour
>>> on systems that set the FADT ASPM disable bit but which still grant full
>>> PCIe control is for the OS to leave any BIOS configuration intact and
>>> refuse to touch the ASPM bits. This mimics the behaviour of Windows.
>>>
>>> BugLink: http://bugs.launchpad.net/bugs/1441335
>>>
>>> Signed-off-by: Matthew Garrett <mjg59 at coreos.com>
>>> Signed-off-by: Bjorn Helgaas <bhelgaas at google.com>(cherry picked from commit 387d37577fdd05e9472c20885464c2a53b3c945f)
>>>
>>> Conflicts:
>>>
>>> drivers/acpi/pci_root.c
>>>
>>> Signed-off-by: Alex Hung <alex.hung at canonical.com>
>>> ---
>>> drivers/acpi/pci_root.c | 19 ++++++++-----------
>>> drivers/pci/pcie/aspm.c | 18 ------------------
>>> include/linux/pci-aspm.h | 4 ----
>>> 3 files changed, 8 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>>> index 20360e4..1528496 100644
>>> --- a/drivers/acpi/pci_root.c
>>> +++ b/drivers/acpi/pci_root.c
>>> @@ -418,8 +418,7 @@ out:
>>> }
>>> EXPORT_SYMBOL(acpi_pci_osc_control_set);
>>>
>>> -static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>>> - int *clear_aspm)
>>> +static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
>>> {
>>> u32 support, control, requested;
>>> acpi_status status;
>>> @@ -477,10 +476,12 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>>> decode_osc_control(root, "OS now controls", control);
>>> if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM) {
>>> /*
>>> - * We have ASPM control, but the FADT indicates
>>> - * that it's unsupported. Clear it.
>>> + * We have ASPM control, but the FADT indicates that
>>> + * it's unsupported. Leave existing configuration
>>> + * intact and prevent the OS from touching it.
>>> */
>>> - *clear_aspm = 1;
>>> + dev_info(&device->dev, "FADT indicates ASPM is unsupported, using BIOS configuration\n");
>>> + *no_aspm = 1;
>>> }
>>> } else {
>>> decode_osc_control(root, "OS requested", requested);
>>> @@ -507,7 +508,7 @@ static int acpi_pci_root_add(struct acpi_device *device,
>>> int result;
>>> struct acpi_pci_root *root;
>>> acpi_handle handle = device->handle;
>>> - int no_aspm = 0, clear_aspm = 0;
>>> + int no_aspm = 0;
>>>
>>> root = kzalloc(sizeof(struct acpi_pci_root), GFP_KERNEL);
>>> if (!root)
>>> @@ -560,7 +561,7 @@ static int acpi_pci_root_add(struct acpi_device *device,
>>>
>>> root->mcfg_addr = acpi_pci_root_get_mcfg_addr(handle);
>>>
>>> - negotiate_os_control(root, &no_aspm, &clear_aspm);
>>> + negotiate_os_control(root, &no_aspm);
>>>
>>> /*
>>> * TBD: Need PCI interface for enumeration/configuration of roots.
>>> @@ -583,10 +584,6 @@ static int acpi_pci_root_add(struct acpi_device *device,
>>> goto end;
>>> }
>>>
>>> - if (clear_aspm) {
>>> - dev_info(&device->dev, "Disabling ASPM (FADT indicates it is unsupported)\n");
>>> - pcie_clear_aspm(root->bus);
>>> - }
>>> if (no_aspm)
>>> pcie_no_aspm();
>>>
>>> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
>>> index f1272dc..ddd7593 100644
>>> --- a/drivers/pci/pcie/aspm.c
>>> +++ b/drivers/pci/pcie/aspm.c
>>> @@ -782,24 +782,6 @@ void pci_disable_link_state(struct pci_dev *pdev, int state)
>>> }
>>> EXPORT_SYMBOL(pci_disable_link_state);
>>>
>>> -void pcie_clear_aspm(struct pci_bus *bus)
>>> -{
>>> - struct pci_dev *child;
>>> -
>>> - if (aspm_force)
>>> - return;
>>> -
>>> - /*
>>> - * Clear any ASPM setup that the firmware has carried out on this bus
>>> - */
>>> - list_for_each_entry(child, &bus->devices, bus_list) {
>>> - __pci_disable_link_state(child, PCIE_LINK_STATE_L0S |
>>> - PCIE_LINK_STATE_L1 |
>>> - PCIE_LINK_STATE_CLKPM,
>>> - false, true);
>>> - }
>>> -}
>>> -
>>> static int pcie_aspm_set_policy(const char *val, struct kernel_param *kp)
>>> {
>>> int i;
>>> diff --git a/include/linux/pci-aspm.h b/include/linux/pci-aspm.h
>>> index 8af4610..207c561 100644
>>> --- a/include/linux/pci-aspm.h
>>> +++ b/include/linux/pci-aspm.h
>>> @@ -29,7 +29,6 @@ void pcie_aspm_pm_state_change(struct pci_dev *pdev);
>>> void pcie_aspm_powersave_config_link(struct pci_dev *pdev);
>>> void pci_disable_link_state(struct pci_dev *pdev, int state);
>>> void pci_disable_link_state_locked(struct pci_dev *pdev, int state);
>>> -void pcie_clear_aspm(struct pci_bus *bus);
>>> void pcie_no_aspm(void);
>>> #else
>>> static inline void pcie_aspm_init_link_state(struct pci_dev *pdev)
>>> @@ -47,9 +46,6 @@ static inline void pcie_aspm_powersave_config_link(struct pci_dev *pdev)
>>> static inline void pci_disable_link_state(struct pci_dev *pdev, int state)
>>> {
>>> }
>>> -static inline void pcie_clear_aspm(struct pci_bus *bus)
>>> -{
>>> -}
>>> static inline void pcie_no_aspm(void)
>>> {
>>> }
>>>
>>
>> Given that new information has come to light from a hardware vendor that
>> this strategy makes sense to follow the Windows way of doing things that
>> this seems sensible. I recall previously that the previous policy was
>> based on inference on what was best given that there was no written
>> policy on what to do in this particular case.
>>
>> Has this been tested on H/W with this FADT setting with this patch on
>> the target kernel?
>
>
>
Thanks for verifying that Alex.
Acked-by: Colin Ian King <colin.king at canonical.com>
More information about the kernel-team
mailing list