[PATCH 1/1] ACPI: Fix the incorrect behavior of the disabled ASPM on Haswell CPU

[PATCH 1/1] ACPI: Fix the incorrect behavior of the disabled ASPM on Haswell CPU

[Adrian Huang12]

On the Haswell-based server (Grantley platform), there are two PCI
root bridges. One is the PCI-express root bridge (HID: PNP0A08)
for processor IIO devices, and the other is the PCI root bridge
(HID: PNP0A03) for the processor uncore devices.

Some BIOSes do not implement the _OSC object for PCI root bridge
(PNP0A03) since it is the optional object described on the page 286
of ACPI spec 5.0. This causes the variable no_aspm is set to 1 when
evaluating the undefined _OSC object of the PCI root bridge. It
turns out that the ASPM policy cannot be changed even though the
PCI-express root bridge on the Haswell-based server platform is
evaluated successfully. Here is the example:

$ cat /sys/module/pcie_aspm/parameters/policy
[default] performance powersave
$ echo performance | sudo tee /sys/module/pcie_aspm/parameters/policy
performance
tee: /sys/module/pcie_aspm/parameters/policy: Operation not permitted

To fix the above-mentioned issue, this patch does not set the
variable no_aspm to 1 if the returned status is AE_NOT_FOUND and
the bridge is the PCI root bridge.

After applying this patch, the ASPM policy can be changed correctly.
And, the corresponding reaction works correctly after changing a
policy.

Tested on the Grantley platform and v3.14-rc6.

Signed-off-by: Adrian Huang <ahuang12@lenovo.com>
---
 drivers/acpi/pci_root.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index c1c4102..cfb3bca 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -446,6 +446,13 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 	decode_osc_support(root, "OS supports", support);
 	status = acpi_pci_osc_support(root, support);
 	if (ACPI_FAILURE(status)) {
+		if (status == AE_NOT_FOUND &&
+		    !strcmp(acpi_device_hid(device), PCI_ROOT_HID_STRING)) {
+			dev_info(&device->dev,
+				 "No _OSC; skip ASPM configuration\n");
+			return;
+		}
+
 		dev_info(&device->dev, "_OSC failed (%s); disabling ASPM\n",
 			 acpi_format_exception(status));
 		*no_aspm = 1;
-- 
1.8.1.2

Re: [PATCH 1/1] ACPI: Fix the incorrect behavior of the disabled ASPM on Haswell CPU

[Matthew Garrett]

On Mon, Mar 10, 2014 at 09:56:25AM +0000, Adrian Huang12 wrote:
> +		if (status == AE_NOT_FOUND &&

Why limit it to not found? I suspect that we should never be basing our 
ASPM policy on the behaviour of PCI (rather than PCIe) bridges.

I'd recommend changing the patch subject line, too - there's nothing 
Haswell specific about this.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 1/1] ACPI: Fix the incorrect behavior of the disabled ASPM on Haswell CPU

[Adrian Huang12]

On Mon, 2014-03-10 at 15:45 +0000, Matthew Garrett wrote:
> On Mon, Mar 10, 2014 at 09:56:25AM +0000, Adrian Huang12 wrote:
> > +		if (status == AE_NOT_FOUND &&
> 
> Why limit it to not found? 

Just for the undefined _OSC object in order to follow ACPI5.0.
Looks like another approach should be implemented to address
this issue. Is this what you were thinking of: we should never
evaluate the _OSC object if it is the PCI root bridge?

> I suspect that we should never be basing our 
> ASPM policy on the behaviour of PCI (rather than PCIe) bridges.

Yes, agree since the ASPM functionality is supported only for PCIe.
Do you agree we should never evaluate the _OSC object if it is
the PCI root bridge? 

> I'd recommend changing the patch subject line, too - there's nothing 
> Haswell specific about this.

Thanks. I will change the subject line. :-)

--
Adrian

Re: [PATCH 1/1] ACPI: Fix the incorrect behavior of the disabled ASPM on Haswell CPU

[Matthew Garrett]

On Mon, Mar 10, 2014 at 04:17:05PM +0000, Adrian Huang12 wrote:
> On Mon, 2014-03-10 at 15:45 +0000, Matthew Garrett wrote:
> > On Mon, Mar 10, 2014 at 09:56:25AM +0000, Adrian Huang12 wrote:
> > > +		if (status == AE_NOT_FOUND &&
> > 
> > Why limit it to not found? 
> 
> Just for the undefined _OSC object in order to follow ACPI5.0.
> Looks like another approach should be implemented to address
> this issue. Is this what you were thinking of: we should never
> evaluate the _OSC object if it is the PCI root bridge?

No, I meant we should never set no_aspm because _OSC fails for any 
reason on a non-PCIe root bridge. But thinking about it, I suspect that 
the whole way we handle _OSC in this case is wrong. If a PCI host bridge 
does implement _OSC then there's still a good chance that it'll refuse 
to grant us control over ASPM, and so we may still end up with failure 
cases.

> > I suspect that we should never be basing our 
> > ASPM policy on the behaviour of PCI (rather than PCIe) bridges.
> 
> Yes, agree since the ASPM functionality is supported only for PCIe.
> Do you agree we should never evaluate the _OSC object if it is
> the PCI root bridge? 

Just skipping _OSC entirely in that case would certainly fix the issue, 
but it doesn't sound like the best fix. I think we need to revisit some 
assumptions in this code.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 1/1] ACPI: Fix the incorrect behavior of the disabled ASPM on Haswell CPU

[Adrian Huang12]

On Mon, 2014-03-10 at 16:24 +0000, Matthew Garrett wrote:
> On Mon, Mar 10, 2014 at 04:17:05PM +0000, Adrian Huang12 wrote:
> > On Mon, 2014-03-10 at 15:45 +0000, Matthew Garrett wrote:
> > > On Mon, Mar 10, 2014 at 09:56:25AM +0000, Adrian Huang12 wrote:
> 
> Just skipping _OSC entirely in that case would certainly fix the issue, 
> but it doesn't sound like the best fix. I think we need to revisit some 
> assumptions in this code.
> 
Thanks for the comments. I'll try to figure out the best fix. 

-- Adrian