| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <201103181839.23664.rjw@sisk.pl>
Date: Fri, 18 Mar 2011 18:39:23 +0100
From: "Rafael J. Wysocki" <rjw@...k.pl>
To: Naga Chumbalkar <nagananda.chumbalkar@...com>
Cc: jbarnes@...tuousgeek.org, mjg59@...f.ucam.org,
linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org
Subject: Re: [RFC][PATCH v2]: PCI: PCIe links may not get configured for ASPM under POWERSAVE mode
On Friday, March 18, 2011, Naga Chumbalkar wrote:
> v2 -> v1:
> . Kept the logic in pci_raw_set_power_state
> . Changed the ASPM enabling logic
> . Modified the text that describes the problem
>
> v1 : http://marc.info/?l=linux-pci&m=130013164703283&w=2
>
> The assumption made in commit 41cd766b065970ff6f6c89dd1cf55fa706c84a3d
> (PCI: Don't enable aspm before drivers have had a chance to veto it) that
> pci_enable_device() will result in re-configuring ASPM when aspm_policy is
> POWERSAVE is no longer valid. This is due to commit
> 97c145f7c87453cec90e91238fba5fe2c1561b32 (PCI: read current power state
> at enable time) which resets dev->current_state to D0. This makes the
> equality check (below) become true, so pcie_aspm_pm_state_change() never
> gets called.
> ./drivers/pci/pci.c: pci_raw_set_pci_power_state()
> 546 /* Check if we're already there */
> 547 if (dev->current_state == state)
> 548 return 0;
>
> So OSPM doesnn't configure the PCIe links for ASPM.
>
> The patch below does the following:
> At the end of each Root Bridge scan make a call to configure ASPM when the
> ASPM policy is set to "powersave" mode. Note that if a previous pass had
> completed the configuration for all devices under that Bridge then the
> configuration will not take place again because pcie_config_aspm_link()
> checks to see if the link is already in the requested state.
> We won't reconfigure ASPM when _OSC control is not granted. In fact, we will
> disable ASPM in that situation. This is to protect systems with a buggy BIOS
> that did not set the FADT bit even though the underlying HW can't do ASPM.
>
> To turn "on" ASPM the minimum the BIOS needs to do:
> 1. Clear the ACPI FADT "ASPM Controls" bit.
> 2. Support _OSC appropriately
>
> Signed-off-by: Naga Chumbalkar <nagananda.chumbalkar@...com>
> Cc: Rafael J. Wysocki <rjw@...k.pl>
> Cc: Matthew Garrett <mjg59@...f.ucam.org>
Acked-by: Rafael J. Wysocki <rjw@...k.pl>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 8524939..c0655aa 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -32,6 +32,7 @@
> #include <linux/pm_runtime.h>
> #include <linux/pci.h>
> #include <linux/pci-acpi.h>
> +#include <linux/pci-aspm.h>
> #include <linux/acpi.h>
> #include <linux/slab.h>
> #include <acpi/acpi_bus.h>
> @@ -591,12 +592,22 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
>
> status = acpi_pci_osc_control_set(device->handle, &flags,
> OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL);
> - if (ACPI_SUCCESS(status))
> +
> + if (ACPI_SUCCESS(status)) {
> dev_info(root->bus->bridge,
> "ACPI _OSC control (0x%02x) granted\n", flags);
> - else
> + /*
> + * When aspm_policy is POWERSAVE, this
> + * call ensures that ASPM is configured.
> + */
> + pcie_aspm_reconfig_link(root->bus->self);
> + } else {
> dev_dbg(root->bus->bridge,
> "ACPI _OSC request failed (code %d)\n", status);
> + printk(KERN_INFO "Unable to assume _OSC PCIe control."
> + " Disabling ASPM.\n");
> + pcie_no_aspm();
> + }
> }
>
> pci_acpi_add_bus_pm_notifier(device, root->bus);
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 3188cd9..1316745 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -742,6 +742,29 @@ void pci_disable_link_state(struct pci_dev *pdev, int state)
> }
> EXPORT_SYMBOL(pci_disable_link_state);
>
> +void pcie_aspm_reconfig_link(struct pci_dev *pdev)
> +{
> + struct pcie_link_state *link = NULL;
> +
> + if (aspm_disabled)
> + return;
> + if (aspm_policy != POLICY_POWERSAVE)
> + return;
> +
> + down_read(&pci_bus_sem);
> + mutex_lock(&aspm_lock);
> +
> + if (list_empty(&link_list))
> + goto out;
> + list_for_each_entry(link, &link_list, sibling) {
> + pcie_config_aspm_link(link, policy_to_aspm_state(link));
> + pcie_set_clkpm(link, policy_to_clkpm_state(link));
> + }
> +out:
> + mutex_unlock(&aspm_lock);
> + up_read(&pci_bus_sem);
> +}
> +
> static int pcie_aspm_set_policy(const char *val, struct kernel_param *kp)
> {
> int i;
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index 5130d0d..595654a 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -15,7 +15,6 @@
> #include <linux/slab.h>
> #include <linux/pcieport_if.h>
> #include <linux/aer.h>
> -#include <linux/pci-aspm.h>
>
> #include "../pci.h"
> #include "portdrv.h"
> @@ -356,10 +355,8 @@ int pcie_port_device_register(struct pci_dev *dev)
>
> /* Get and check PCI Express port services */
> capabilities = get_port_device_capability(dev);
> - if (!capabilities) {
> - pcie_no_aspm();
> + if (!capabilities)
> return 0;
> - }
>
> pci_set_master(dev);
> /*
> diff --git a/include/linux/pci-aspm.h b/include/linux/pci-aspm.h
> index ce68105..216b196 100644
> --- a/include/linux/pci-aspm.h
> +++ b/include/linux/pci-aspm.h
> @@ -26,6 +26,7 @@
> extern void pcie_aspm_init_link_state(struct pci_dev *pdev);
> extern void pcie_aspm_exit_link_state(struct pci_dev *pdev);
> extern void pcie_aspm_pm_state_change(struct pci_dev *pdev);
> +extern void pcie_aspm_reconfig_link(struct pci_dev *pdev);
> extern void pci_disable_link_state(struct pci_dev *pdev, int state);
> extern void pcie_clear_aspm(void);
> extern void pcie_no_aspm(void);
> @@ -39,6 +40,9 @@ static inline void pcie_aspm_exit_link_state(struct pci_dev *pdev)
> static inline void pcie_aspm_pm_state_change(struct pci_dev *pdev)
> {
> }
> +static inline void pcie_aspm_reconfig_link(struct pci_dev *pdev)
> +{
> +}
> static inline void pci_disable_link_state(struct pci_dev *pdev, int state)
> {
> }
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists