Re: [PATCH] PCI: Remove not needed check in disable aspm link

[Yinghai Lu <yinghai@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1808 bytes --]

On Fri, Jun 14, 2013 at 2:26 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> [+cc Maxim, Jussi]
>
> Yeah, this is a huge mess.  It makes my head hurt.  I don't think it's
> reasonable to add more flags because that will make my head hurt even
> more.
>
> If I understand correctly, on Roman's system (the Acer Aspire One
> AOA150 netbook mentioned in
> https://bugzilla.kernel.org/show_bug.cgi?id=55211):
>
>   - The BIOS leaves ASPM enabled for the ath5k device (03:00.0)
>   - The BIOS does not allow the OS to manage ASPM (via _OSC)
>   - The ath5k device does not work correctly with ASPM enabled
>   - The ath5k driver calls pci_disable_link_state(), but we do not
> disable ASPM because we don't have permission from the BIOS

looks like Matthew Garrett path is causing problem:

 commit 4949be16822e92a18ea0cc1616319926628092ee
 Author: Matthew Garrett <mjg@redhat.com>
 Date:   Tue Mar 6 13:41:49 2012 -0500

     PCI: ignore pre-1.1 ASPM quirking when ASPM is disabled

 commit c9651e70ad0aa499814817cbf3cc1d0b806ed3a1
 Author: Matthew Garrett <mjg@redhat.com>
 Date:   Tue Mar 27 10:17:41 2012 -0400

     ASPM: Fix pcie devices with non-pcie children

after those two patches, it aspm_disabled is set, via _osc early,
pre-1.1 devices aspm register will be touched even aspm_force is not specified.

pcie_aspm_init_link_state will all the way to
 pcie_config_aspm_path ==> pcie_config_aspm_link

in that path, aspm_disabled is not checked nowhere.

BTW, when aspm is not disabled, even the link is allocated, because it is
black listed,  so it is never get touched.
Matthew's patch is not needed in any case.

I would suspect that that aspm enabling in Roman's system could set by that
path instead of BIOS.

Roman, can you please check two patches + linsus' tree on your system?

Thanks

Yinghai

[-- Attachment #2: revert_revert_osc_change_linus.patch --]
[-- Type: application/octet-stream, Size: 5303 bytes --]

Subject: [PATCH] ACPI, PCI: Revert reverting of "PCI/ACPI: Request _OSC control before scanning PCI root bus"

In
| commit b8178f130e25c1bdac1c33e0996f1ff6e20ec08e
| Author: Bjorn Helgaas <bhelgaas@google.com>
| Date:   Mon Apr 1 15:47:39 2013 -0600
|
|    Revert "PCI/ACPI: Request _OSC control before scanning PCI root bus"
|
|    This reverts commit 8c33f51df406e1a1f7fa4e9b244845b7ebd61fa6.
for v3.9, "OSC set early" is reverted.

Now we have problem on v3.10, as it has
       - Remove ACPI PCI subdrivers (Jiang Liu, Myron Stowe)
       - Make acpiphp builtin only, not modular (Jiang Liu)
   acpiphp get registered with pcibios_add_bus very early.
We can not enable pcie native hotplug driver any more if system support
acpiphp and pciehp.

Calling path: in acpi_pci_root_add we have
1. pci_acpi_scan_root
     ==> pci devices enumeration and bus scanning.
       ==> pci_alloc_child_bus
         ==> pcibios_add_bus
           ==> acpi_pci_add_bus
             ==> acpiphp_enumerate_slots
               ==> ...==> register_slot
                 ==> device_is_managed_by_native_pciehp
                   ==> check osc_set with OSC_PCI_EXPRESS_NATIVE_HP_CONTROL
2. _OSC set request
so we always have acpiphp hotplug slot registered at first, as OSC is
not set yet.

We need "OSC set early" before pci_apci_scan_root again.

The point: later aspm, pme, pciehp, aer support would rely on value in
root osc_support_set/osc_control_set.

For v3.10, let's put the "osc control set early" back,
as we have user in pci_acpi_scan_root.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index e427dc5..207d773 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -382,6 +382,7 @@ static int acpi_pci_root_add(struct acpi_device *device,
 	int result;
 	struct acpi_pci_root *root;
 	u32 flags, base_flags;
+	bool is_osc_granted = false;
 
 	root = kzalloc(sizeof(struct acpi_pci_root), GFP_KERNEL);
 	if (!root)
@@ -442,30 +443,6 @@ static int acpi_pci_root_add(struct acpi_device *device,
 	flags = base_flags = OSC_PCI_SEGMENT_GROUPS_SUPPORT;
 	acpi_pci_osc_support(root, flags);
 
-	/*
-	 * TBD: Need PCI interface for enumeration/configuration of roots.
-	 */
-
-	mutex_lock(&acpi_pci_root_lock);
-	list_add_tail(&root->node, &acpi_pci_roots);
-	mutex_unlock(&acpi_pci_root_lock);
-
-	/*
-	 * Scan the Root Bridge
-	 * --------------------
-	 * Must do this prior to any attempt to bind the root device, as the
-	 * PCI namespace does not get created until this call is made (and
-	 * thus the root bridge's pci_dev does not exist).
-	 */
-	root->bus = pci_acpi_scan_root(root);
-	if (!root->bus) {
-		printk(KERN_ERR PREFIX
-			    "Bus %04x:%02x not present in PCI namespace\n",
-			    root->segment, (unsigned int)root->secondary.start);
-		result = -ENODEV;
-		goto out_del_root;
-	}
-
 	/* Indicate support for various _OSC capabilities. */
 	if (pci_ext_cfg_avail())
 		flags |= OSC_EXT_PCI_CONFIG_SUPPORT;
@@ -484,7 +461,6 @@ static int acpi_pci_root_add(struct acpi_device *device,
 			flags = base_flags;
 		}
 	}
-
 	if (!pcie_ports_disabled
 	    && (flags & ACPI_PCIE_REQ_SUPPORT) == ACPI_PCIE_REQ_SUPPORT) {
 		flags = OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL
@@ -505,28 +481,54 @@ static int 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)) {
+			is_osc_granted = true;
 			dev_info(&device->dev,
 				"ACPI _OSC control (0x%02x) granted\n", flags);
-			if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM) {
-				/*
-				 * We have ASPM control, but the FADT indicates
-				 * that it's unsupported. Clear it.
-				 */
-				pcie_clear_aspm(root->bus);
-			}
 		} else {
+			is_osc_granted = false;
 			dev_info(&device->dev,
 				"ACPI _OSC request failed (%s), "
 				"returned control mask: 0x%02x\n",
 				acpi_format_exception(status), flags);
-			pr_info("ACPI _OSC control for PCIe not granted, "
-				"disabling ASPM\n");
-			pcie_no_aspm();
 		}
 	} else {
 		dev_info(&device->dev,
-			 "Unable to request _OSC control "
-			 "(_OSC support mask: 0x%02x)\n", flags);
+			"Unable to request _OSC control "
+			"(_OSC support mask: 0x%02x)\n", flags);
+	}
+
+	/*
+	 * TBD: Need PCI interface for enumeration/configuration of roots.
+	 */
+
+	mutex_lock(&acpi_pci_root_lock);
+	list_add_tail(&root->node, &acpi_pci_roots);
+	mutex_unlock(&acpi_pci_root_lock);
+
+	/*
+	 * Scan the Root Bridge
+	 * --------------------
+	 * Must do this prior to any attempt to bind the root device, as the
+	 * PCI namespace does not get created until this call is made (and 
+	 * thus the root bridge's pci_dev does not exist).
+	 */
+	root->bus = pci_acpi_scan_root(root);
+	if (!root->bus) {
+		printk(KERN_ERR PREFIX
+			    "Bus %04x:%02x not present in PCI namespace\n",
+			    root->segment, (unsigned int)root->secondary.start);
+		result = -ENODEV;
+		goto out_del_root;
+	}
+
+	/* ASPM setting */
+	if (is_osc_granted) {
+		if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM)
+			pcie_clear_aspm(root->bus);
+	} else {
+		pr_info("ACPI _OSC control for PCIe not granted, "
+			"disabling ASPM\n");
+		pcie_no_aspm();
 	}
 
 	pci_acpi_add_bus_pm_notifier(device, root->bus);

[-- Attachment #3: revert_matthew_aspm_disabled.patch --]
[-- Type: application/octet-stream, Size: 583 bytes --]

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 403a443..b795b43 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -493,15 +493,6 @@ static int pcie_aspm_sanity_check(struct pci_dev *pdev)
 			return -EINVAL;
 
 		/*
-		 * If ASPM is disabled then we're not going to change
-		 * the BIOS state. It's safe to continue even if it's a
-		 * pre-1.1 device
-		 */
-
-		if (aspm_disabled)
-			continue;
-
-		/*
 		 * Disable ASPM for pre-1.1 PCIe device, we follow MS to use
 		 * RBER bit to determine if a function is 1.1 version device
 		 */