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 <[email protected]>
Cc: Rafael J. Wysocki <[email protected]>
Cc: Matthew Garrett <[email protected]>
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)
{
}
(2011/03/18 13:21), 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.
Which _OSC controls do we need for configuring ASPM?
Regards,
Kenji Kaneshige
>-----Original Message-----
>From: Kenji Kaneshige [mailto:[email protected]]
>Sent: Friday, March 18, 2011 12:29 AM
>To: Chumbalkar, Nagananda
>Cc: [email protected]; [email protected]; [email protected]; linux-
>[email protected]; [email protected]
>Subject: Re: [RFC][PATCH v2]: PCI: PCIe links may not get configured for
>ASPM under POWERSAVE mode
>
>(2011/03/18 13:21), 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.
>
>Which _OSC controls do we need for configuring ASPM?
There is no _OSC Control bit for ASPM. However, we expect the BIOS to
support _OSC for a Root Bridge that originates a PCIe hierarchy. If this
is not the case - would it be okay to disable ASPM also?
Commit 852972acff8f10f3a15679be2059bb94916cba5d (ACPI: Disable ASPM if the
Platform won't provide _OSC control for PCIe) describes the above scenario.
To quote from there:
The PCI SIG documentation for the _OSC OS/firmware handshaking interface
states:
"If the _OSC control method is absent from the scope of a host bridge
device, then the operating system must not enable or attempt to use any
features defined in this section for the hierarchy originated by the host
bridge."
The obvious interpretation of this is that the OS should not attempt to use
PCIe hotplug, PME or AER - however, the specification also notes that an
_OSC method is *required* for PCIe hierarchies, and experimental validation
with An Alternative OS indicates that it doesn't use any PCIe functionality
if the _OSC method is missing. That arguably means we shouldn't be using
MSI or extended config space, but right now our problems seem to be limited
to vendors being surprised when ASPM gets enabled on machines when other
OSs refuse to do so. So, for now, let's just disable ASPM if the _OSC
method doesn't exist or refuses to hand over PCIe capability control.
- naga -
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 <[email protected]>
> Cc: Rafael J. Wysocki <[email protected]>
> Cc: Matthew Garrett <[email protected]>
Acked-by: Rafael J. Wysocki <[email protected]>
> 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)
> {
> }
>
>
(2011/03/18 23:52), Chumbalkar, Nagananda wrote:
>
>> -----Original Message-----
>> From: Kenji Kaneshige [mailto:[email protected]]
>> Sent: Friday, March 18, 2011 12:29 AM
>> To: Chumbalkar, Nagananda
>> Cc: [email protected]; [email protected]; [email protected]; linux-
>> [email protected]; [email protected]
>> Subject: Re: [RFC][PATCH v2]: PCI: PCIe links may not get configured for
>> ASPM under POWERSAVE mode
>>
>> (2011/03/18 13:21), 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.
>>
>> Which _OSC controls do we need for configuring ASPM?
>
> There is no _OSC Control bit for ASPM. However, we expect the BIOS to
> support _OSC for a Root Bridge that originates a PCIe hierarchy. If this
> is not the case - would it be okay to disable ASPM also?
>
> Commit 852972acff8f10f3a15679be2059bb94916cba5d (ACPI: Disable ASPM if the
> Platform won't provide _OSC control for PCIe) describes the above scenario.
>
> To quote from there:
> The PCI SIG documentation for the _OSC OS/firmware handshaking interface
> states:
>
> "If the _OSC control method is absent from the scope of a host bridge
> device, then the operating system must not enable or attempt to use any
> features defined in this section for the hierarchy originated by the host
> bridge."
>
> The obvious interpretation of this is that the OS should not attempt to use
> PCIe hotplug, PME or AER - however, the specification also notes that an
> _OSC method is *required* for PCIe hierarchies, and experimental validation
> with An Alternative OS indicates that it doesn't use any PCIe functionality
> if the _OSC method is missing. That arguably means we shouldn't be using
> MSI or extended config space, but right now our problems seem to be limited
> to vendors being surprised when ASPM gets enabled on machines when other
> OSs refuse to do so. So, for now, let's just disable ASPM if the _OSC
> method doesn't exist or refuses to hand over PCIe capability control.
>
I understood. Thank you for clarification.
Regards,
Kenji Kaneshige