[PATCH] PCI: Don't clear ASPM bits when the FADT declares it's unsupported
Alex Hung
alex.hung at canonical.com
Mon Apr 27 06:05:30 UTC 2015
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?
--
Cheers,
Alex Hung
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pci.dos
Type: application/octet-stream
Size: 103221 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20150427/2938d6ac/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pci.before
Type: application/octet-stream
Size: 13714 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20150427/2938d6ac/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pci.after
Type: application/octet-stream
Size: 13714 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20150427/2938d6ac/attachment-0002.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: facp.dsl
Type: text/x-dsl
Size: 9788 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20150427/2938d6ac/attachment.bin>
More information about the kernel-team
mailing list