pci_disable_link_state PCIE_LINK_STATE_L0S no longer disables ASPM for ath5k

https://bugzilla.kernel.org/show_bug.cgi?id=55211

--- Comment #9 from Roman Yepishev <[email protected]> 2013-03-17 15:50:23 --- Re-tested on two laptops - AOA150, ath5k device got ASPM disabled and on a Lenovo E420 I got ASPM disabled for iwlwifi-driven Intel Corporation Centrino Wireless-N 1000 [8086:0084].

Also I found http://article.gmane.org/gmane.linux.kernel.pci/20640 where a question was raised why pci_disable_link_state stopped doing anything for iwlwifi devices too.

Roman reported ath5k does not work anymore on 3.8. Bisected to

| commit 8c33f51df406e1a1f7fa4e9b244845b7ebd61fa6
| Author: Taku Izumi <[email protected]>
| Date:   Tue Oct 30 15:27:13 2012 +0900
|
|    PCI/ACPI: Request _OSC control before scanning PCI root bus
|
|    This patch moves up the code block to request _OSC control in order to
|    separate ACPI work and PCI work in acpi_pci_root_add().

It make pci_disable_link_state does not work anymore as acpi_disabled is set before pci root bus scanning.
It will skip that in quirks and pcie_aspm_sanity_check.

We could revert to old logic, but that will make booting path and hotplug path with different aspm_disabled again.

Acctually we don't need to check aspm_disabled in disable link, as we already have protection about link state following.

https://bugzilla.kernel.org/show_bug.cgi?id=55211
http://article.gmane.org/gmane.linux.kernel.pci/20640

Need it for 3.8 stable.

Reported-by Roman Yepishev <[email protected]>

Bisected-by Roman Yepishev <[email protected]>

Tested-by Roman Yepishev <[email protected]>

Signed-off-by Yinghai Lu <[email protected]>

Cc Taku Izumi <[email protected]>

Cc Kenji Kaneshige <[email protected]>

Cc [email protected]

---
 drivers/pci/pcie/aspm.c |   21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

Index: linux-2.6/drivers/pci/pcie/aspm.c
===================================================================

drivers/pci/pcie/aspm.c +3 -16

@@ -493,15 +493,6 @@ static int pcie_aspm_sanity_check(struct
                        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
                 */

@@ -718,15 +709,11 @@ void pcie_aspm_powersave_config_link(str
  * pci_disable_link_state - disable pci device's link state, so the link will
  * never enter specific states
  */
-static void __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem,
-                                    bool force)
+static void __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem)
 {
        struct pci_dev *parent = pdev->bus->self;
        struct pcie_link_state *link;
 
-       if (aspm_disabled && !force)
-               return;
-
        if (!pci_is_pcie(pdev))
                return;
 

@@ -757,13 +744,13 @@ static void __pci_disable_link_state(str
 
 void pci_disable_link_state_locked(struct pci_dev *pdev, int state)
 {
-       __pci_disable_link_state(pdev, state, false, false);
+       __pci_disable_link_state(pdev, state, false);
 }
 EXPORT_SYMBOL(pci_disable_link_state_locked);
 
 void pci_disable_link_state(struct pci_dev *pdev, int state)
 {
-       __pci_disable_link_state(pdev, state, true, false);
+       __pci_disable_link_state(pdev, state, true);
 }
 EXPORT_SYMBOL(pci_disable_link_state);
 

Why can't we set all the ASPM flags *first*, before calling pci_acpi_scan_root()? That way we could just do the correct ASPM setup as we discover devices during enumeration, rather than trying to fix things up afterwards. I suspect pcie_clear_aspm() is broken anyway, because it looks like it only touches one level of the hierarchy, without recursively descending it.

Quoted 7 lines · Bjorn Helgaas

Why can't we set all the ASPM flags *first*, before calling pci_acpi_scan_root()? That way we could just do the correct ASPM setup as we discover devices during enumeration, rather than trying to fix things up afterwards. I suspect pcie_clear_aspm() is broken anyway, because it looks like it only touches one level of the hierarchy, without recursively descending it.

Yes, we can clean up aspm stop/clear up.
and that should be for 3.10 right?

But this patch should be safe for 3.9 and stable.

This patch might be *safe*, but it (and the changelog) are completely unintelligible.

The problem with applying an unintelligible stop-gap patch is that it becomes forever part of the changelog, and it's a huge waste of time to everybody who tries to understand the history later. That's why I think it's worth spending some time to make a good patch now.

This patch might be *safe*, but it (and the changelog) are completely unintelligible.

The problem with applying an unintelligible stop-gap patch is that it becomes forever part of the changelog, and it's a huge waste of time to everybody who tries to understand the history later. That's why I think it's worth spending some time to make a good patch now.

resending with adding To Roman.

Quoted 7 lines · Bjorn Helgaas

This patch might be *safe*, but it (and the changelog) are completely unintelligible.

The problem with applying an unintelligible stop-gap patch is that it becomes forever part of the changelog, and it's a huge waste of time to everybody who tries to understand the history later. That's why I think it's worth spending some time to make a good patch now.

Please check if attached patch is doing what you want.

Thanks

Yinghai

patch for Roman

Quoted 12 lines · Yinghai Lu

resending with adding To Roman.

Quoted 7 lines · Bjorn Helgaas

This patch might be *safe*, but it (and the changelog) are completely unintelligible.

The problem with applying an unintelligible stop-gap patch is that it becomes forever part of the changelog, and it's a huge waste of time to everybody who tries to understand the history later. That's why I think it's worth spending some time to make a good patch now.

Please check if attached patch is doing what you want.

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

Roman reported ath5k does not work anymore on 3.8. Bisected to

| commit 8c33f51df406e1a1f7fa4e9b244845b7ebd61fa6
| Author: Taku Izumi <[email protected]>
| Date:   Tue Oct 30 15:27:13 2012 +0900
|
|    PCI/ACPI: Request _OSC control before scanning PCI root bus
|
|    This patch moves up the code block to request _OSC control in order to
|    separate ACPI work and PCI work in acpi_pci_root_add().

It make pci_disable_link_state does not work anymore as acpi_disabled is set before pci root bus scanning.
It will skip that in quirks and pcie_aspm_sanity_check.

drivers/pci/pcie/aspm.c +0 -2

@@ -493,15 +492,6 @@ static int pcie_aspm_sanity_check(struct
 			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

We could revert to old logic, but that will make booting path and hotplug path with different aspm_disabled again.

Acctually we don't need to check aspm_disabled in disable link, as we already have protection about link state following.

https://bugzilla.kernel.org/show_bug.cgi?id=55211
http://article.gmane.org/gmane.linux.kernel.pci/20640

Need it for 3.8 stable.

-v2: more cleanup
1. remove aspm_support_enabled, as if it compiled in, support is there so even user pass aspm=off, link_state still get allocated, then we will have chance to disable aspm on devices from buggy setting of BIOS.
2. move pcie_no_aspm() calling for fadt disabling before scanning requested by Bjorn.

Reported-by Roman Yepishev <[email protected]>

Bisected-by Roman Yepishev <[email protected]>

Signed-off-by Yinghai Lu <[email protected]>

Cc Taku Izumi <[email protected]>

Cc Kenji Kaneshige <[email protected]>

---
 drivers/acpi/pci_root.c  |   25 +++++++++---------------
 drivers/pci/pcie/aspm.c  |   48 ++---------------------------------------------
 include/linux/pci-aspm.h |    4 ---
 include/linux/pci.h      |    2 -
 4 files changed, 14 insertions(+), 65 deletions(-)

Index: linux-2.6/drivers/acpi/pci_root.c
===================================================================

drivers/acpi/pci_root.c +10 -15

@@ -415,7 +415,6 @@ static int acpi_pci_root_add(struct acpi
 	struct acpi_pci_root *root;
 	struct acpi_pci_driver *driver;
 	u32 flags, base_flags;
-	bool is_osc_granted = false;
 
 	root = kzalloc(sizeof(struct acpi_pci_root), GFP_KERNEL);
 	if (!root)

@@ -494,6 +493,11 @@ static int acpi_pci_root_add(struct acpi
 			flags = base_flags;
 		}
 	}
+
+	/* ASPM setting */
+	if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM)
+		pcie_no_aspm();
+
 	if (!pcie_ports_disabled
 	    && (flags & ACPI_PCIE_REQ_SUPPORT) == ACPI_PCIE_REQ_SUPPORT) {
 		flags = OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL

@@ -513,16 +517,17 @@ static int acpi_pci_root_add(struct acpi
 
 		status = acpi_pci_osc_control_set(device->handle, &flags,
 				       OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL);
-		if (ACPI_SUCCESS(status)) {
-			is_osc_granted = true;
+		if (ACPI_SUCCESS(status))
 			dev_info(&device->dev,
 				"ACPI _OSC control (0x%02x) granted\n", flags);
-		} else {
-			is_osc_granted = false;
+		else {
 			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,

@@ -554,16 +559,6 @@ static int acpi_pci_root_add(struct acpi
 		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);
 	if (device->wakeup.flags.run_wake)
 		device_set_run_wake(root->bus->bridge, true);

Index: linux-2.6/drivers/pci/pcie/aspm.c
===================================================================

drivers/pci/pcie/aspm.c +3 -45

@@ -69,7 +69,6 @@ struct pcie_link_state {
 };
 
 static int aspm_disabled, aspm_force;
-static bool aspm_support_enabled = true;
 static DEFINE_MUTEX(aspm_lock);
 static LIST_HEAD(link_list);
 

@@ -493,15 +492,6 @@ static int pcie_aspm_sanity_check(struct
 			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
 		 */

@@ -556,9 +546,6 @@ void pcie_aspm_init_link_state(struct pc
 	struct pcie_link_state *link;
 	int blacklist = !!pcie_aspm_sanity_check(pdev);
 
-	if (!aspm_support_enabled)
-		return;
-
 	if (!pci_is_pcie(pdev) || pdev->link_state)
 		return;
 	if (pci_pcie_type(pdev) != PCI_EXP_TYPE_ROOT_PORT &&

@@ -718,15 +705,11 @@ void pcie_aspm_powersave_config_link(str
  * pci_disable_link_state - disable pci device's link state, so the link will
  * never enter specific states
  */
-static void __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem,
-				     bool force)
+static void __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem)
 {
 	struct pci_dev *parent = pdev->bus->self;
 	struct pcie_link_state *link;
 
-	if (aspm_disabled && !force)
-		return;
-
 	if (!pci_is_pcie(pdev))
 		return;
 

@@ -757,34 +740,16 @@ static void __pci_disable_link_state(str
 
 void pci_disable_link_state_locked(struct pci_dev *pdev, int state)
 {
-	__pci_disable_link_state(pdev, state, false, false);
+	__pci_disable_link_state(pdev, state, false);
 }
 EXPORT_SYMBOL(pci_disable_link_state_locked);
 
 void pci_disable_link_state(struct pci_dev *pdev, int state)
 {
-	__pci_disable_link_state(pdev, state, true, false);
+	__pci_disable_link_state(pdev, state, true);
 }
 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;

@@ -944,7 +909,6 @@ static int __init pcie_aspm_disable(char
 	if (!strcmp(str, "off")) {
 		aspm_policy = POLICY_DEFAULT;
 		aspm_disabled = 1;
-		aspm_support_enabled = false;
 		printk(KERN_INFO "PCIe ASPM is disabled\n");
 	} else if (!strcmp(str, "force")) {
 		aspm_force = 1;

@@ -980,9 +944,3 @@ int pcie_aspm_enabled(void)
        return !aspm_disabled;
 }
 EXPORT_SYMBOL(pcie_aspm_enabled);
-
-bool pcie_aspm_support_enabled(void)
-{
-	return aspm_support_enabled;
-}
-EXPORT_SYMBOL(pcie_aspm_support_enabled);

Index: linux-2.6/include/linux/pci-aspm.h
===================================================================

include/linux/pci-aspm.h +0 -4

@@ -29,7 +29,6 @@ extern void pcie_aspm_pm_state_change(st
 extern void pcie_aspm_powersave_config_link(struct pci_dev *pdev);
 extern void pci_disable_link_state(struct pci_dev *pdev, int state);
 extern void pci_disable_link_state_locked(struct pci_dev *pdev, int state);
-extern void pcie_clear_aspm(struct pci_bus *bus);
 extern 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_c
 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)
 {
 }

Index: linux-2.6/include/linux/pci.h
===================================================================

include/linux/pci.h +1 -1

@@ -1168,7 +1168,7 @@ static inline int pcie_aspm_enabled(void
 static inline bool pcie_aspm_support_enabled(void) { return false; }
 #else
 extern int pcie_aspm_enabled(void);
-extern bool pcie_aspm_support_enabled(void);
+static inline bool pcie_aspm_support_enabled(void) { return true; }
 #endif
 
 #ifdef CONFIG_PCIEAER

[+cc Matthew]
[+cc [email protected] for suspected 82575/82598 regression]

Quoted 6 lines · Yinghai Lu

patch for Roman

Quoted 3 lines · Yinghai Lu

resending with adding To Roman.

On Thu, Mar 28, 2013 at 5:46 AM, Bjorn Helgaas <[email protected]>

Quoted 9 lines · wrote:

Quoted 9 lines

Quoted 7 lines

This patch might be *safe*, but it (and the changelog) are completely unintelligible.

The problem with applying an unintelligible stop-gap patch is that it becomes forever part of the changelog, and it's a huge waste of time to everybody who tries to understand the history later. That's why I think it's worth spending some time to make a good patch now.

Please check if attached patch is doing what you want.

Patch inlined below for convenience.

Quoted 11 lines

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

Roman reported ath5k does not work anymore on 3.8. Bisected to

| commit 8c33f51df406e1a1f7fa4e9b244845b7ebd61fa6
| Author: Taku Izumi <[email protected]>
| Date:   Tue Oct 30 15:27:13 2012 +0900
|
|    PCI/ACPI: Request _OSC control before scanning PCI root bus
|
|    This patch moves up the code block to request _OSC control in order

to

Quoted 5 lines

| separate ACPI work and PCI work in acpi_pci_root_add().

It make pci_disable_link_state does not work anymore as acpi_disabled is set before pci root bus scanning.
It will skip that in quirks and pcie_aspm_sanity_check.

I think this regression has nothing to do with pci_disable_link_state().

When aspm_disabled is set, pci_disable_link_state() doesn't do anything.

In both 3.7 and 3.8, aspm_disabled is set in acpi_pci_root_add() before any driver probe routines are run, so it looks like calling pci_disable_link_state() from a driver had no effect even in 3.7. This is a problem, of course, but not the one Roman is seeing, because ath5k calls pci_disable_link_state() from the driver probe routine.

There are also PCI_FIXUP_FINAL quirks for 82575 and 82598 NICs that call pci_disable_link_state(). In 3.7, these quirks are run before aspm_disabled is set, but 8c33f51d moved the pcie_no_aspm() call up before we start scanning the bus, so in 3.8, aspm_disabled is set *before* we run them. I think that means 8c33f51d broke all these quirks. That's also a problem, of course, but this isn't the one Roman is seeing either.

I think the problem Roman is seeing happens when pcie_aspm_init_link_state() calls pcie_aspm_sanity_check() during device enumeration. In 3.8, the fact that aspm_disabled is already set by the time we get here means we skip the check for pre-1.1 PCIe devices, and I think *this* is what Roman is seeing.

I suspect the following hunk of your patch is enough to fix things for Roman:

Quoted 6 lines

drivers/pci/pcie/aspm.c +0 -0

@@ -493,15 +492,6 @@ static int pcie_aspm_sanity_check(struct
                      return -EINVAL;
 
              /*

drivers/pci/pcie/aspm.c static int pcie_aspm_sanity_check(struct · line 495

Quoted 10 lines

drivers/pci/pcie/aspm.c +0 -9

-              * 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

drivers/pci/pcie/aspm.c static int pcie_aspm_sanity_check(struct · line 496

Quoted 1 line

drivers/pci/pcie/aspm.c +0 -0

               * RBER bit to determine if a function is 1.1 version

device

drivers/pci/pcie/aspm.c static int pcie_aspm_sanity_check(struct · line 497

Quoted 1 line

drivers/pci/pcie/aspm.c +0 -0

               */

However, this test was added by Matthew in c9651e70, and I can't remove it unless we have an explanation of why removing it will not reintroduce the bug he was fixing.

This code is such a terrible mess that it's not surprising at all that we have all these issues. But there's too much to untangle in v3.9; all we can hope for is to fix the regressions in v3.9 and clean it up later.

Quoted 95 lines · Bjorn Helgaas

[+cc Matthew]
[+cc [email protected] for suspected 82575/82598 regression]

Quoted 16 lines · Yinghai Lu

patch for Roman

Quoted 13 lines · Yinghai Lu

resending with adding To Roman.

Quoted 7 lines · Bjorn Helgaas

This patch might be *safe*, but it (and the changelog) are completely unintelligible.

The problem with applying an unintelligible stop-gap patch is that it becomes forever part of the changelog, and it's a huge waste of time to everybody who tries to understand the history later. That's why I think it's worth spending some time to make a good patch now.

Please check if attached patch is doing what you want.

Patch inlined below for convenience.

Quoted 11 lines

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

Roman reported ath5k does not work anymore on 3.8. Bisected to

| commit 8c33f51df406e1a1f7fa4e9b244845b7ebd61fa6
| Author: Taku Izumi <[email protected]>
| Date:   Tue Oct 30 15:27:13 2012 +0900
|
|    PCI/ACPI: Request _OSC control before scanning PCI root bus
|
|    This patch moves up the code block to request _OSC control in order

Quoted 6 lines

to
| separate ACPI work and PCI work in acpi_pci_root_add().

It make pci_disable_link_state does not work anymore as acpi_disabled is set before pci root bus scanning.
It will skip that in quirks and pcie_aspm_sanity_check.

I think this regression has nothing to do with pci_disable_link_state().

When aspm_disabled is set, pci_disable_link_state() doesn't do anything.

In both 3.7 and 3.8, aspm_disabled is set in acpi_pci_root_add() before any driver probe routines are run, so it looks like calling pci_disable_link_state() from a driver had no effect even in 3.7. This is a problem, of course, but not the one Roman is seeing, because ath5k calls pci_disable_link_state() from the driver probe routine.

There are also PCI_FIXUP_FINAL quirks for 82575 and 82598 NICs that call pci_disable_link_state(). In 3.7, these quirks are run before aspm_disabled is set, but 8c33f51d moved the pcie_no_aspm() call up before we start scanning the bus, so in 3.8, aspm_disabled is set *before* we run them. I think that means 8c33f51d broke all these quirks. That's also a problem, of course, but this isn't the one Roman is seeing either.

I think the problem Roman is seeing happens when pcie_aspm_init_link_state() calls pcie_aspm_sanity_check() during device enumeration. In 3.8, the fact that aspm_disabled is already set by the time we get here means we skip the check for pre-1.1 PCIe devices, and I think *this* is what Roman is seeing.

I suspect the following hunk of your patch is enough to fix things for Roman:

Quoted 19 lines

drivers/pci/pcie/aspm.c +0 -9

@@ -493,15 +492,6 @@ static int pcie_aspm_sanity_check(struct
                      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

Quoted 1 line

               */

However, this test was added by Matthew in c9651e70, and I can't remove it unless we have an explanation of why removing it will not reintroduce the bug he was fixing.

Quoted 3 lines

This code is such a terrible mess that it's not surprising at all that we have all these issues. But there's too much to untangle in v3.9; all we can hope for is to fix the regressions in v3.9 and clean it up later.

v1 will fix quirks and pcie_aspm_sanity_check path.
v2. will go further even user pass "aspm=off", those quirks and disable aspm in driver
will still work, and also call pcie_no_aspm for disable aspm for FADT path early.

So now you want half of v1, and not want to fix quirk path. Is my understanding right?

Half of your v1 patch (removing the pcie_aspm_sanity_check() test) *might* be the right thing, but only if you can clearly explain why that will not reintroduce the bug Matthew fixed with c9651e70.

I think we also need to fix the PCI_FIXUP_FINAL quirk regression, but that's a separate issue and should be a separate patch.

Quoted 7 lines · Bjorn Helgaas

Half of your v1 patch (removing the pcie_aspm_sanity_check() test) *might* be the right thing, but only if you can clearly explain why that will not reintroduce the bug Matthew fixed with c9651e70.

I think we also need to fix the PCI_FIXUP_FINAL quirk regression, but that's a separate issue and should be a separate patch.

First commit from Matthew
0ae5eaf10 PCI: ignore pre-1.1 ASPM quirking when ASPM is disabled
Right now we won't touch ASPM state if ASPM is disabled, except in the case where we find a device that appears to be too old to reliably support ASPM. Right now we'll clear it in that case, which is almost certainly the wrong thing to do

Try to not touch pre-1.1 ASPM for all, and it causes lots of regression.

So second commit

cdb0f9a1ad2e ASPM: Fix pcie devices with non-pcie children
Since 3.2.12 and 3.3, some systems are failing to boot with a BUG_ON. Some other systems using the pata_jmicron driver fail to boot because no disks are detected. Passing pcie_aspm=force on the kernel command line works around it.

move the check aspm_disabled down.

but ath5 and etc (pre-1.1) really need to aspm_disable to change their hw setting.

So the right solution would be dropping pcie_aspm_sanity_check() change -in v2 should make all both happy, as quirk and disable that in driver for ath5 are calling
pcie_disable_aspm_state explicitly.

In v2, we already removed pcie_clear_aspm() that is calling pcie_disable_aspm_state.

Please check attached -v3.

Thanks

Yinghai

attatched -v3 again

Quoted 42 lines · Yinghai Lu

Quoted 7 lines · Bjorn Helgaas

Half of your v1 patch (removing the pcie_aspm_sanity_check() test) *might* be the right thing, but only if you can clearly explain why that will not reintroduce the bug Matthew fixed with c9651e70.

I think we also need to fix the PCI_FIXUP_FINAL quirk regression, but that's a separate issue and should be a separate patch.

First commit from Matthew
0ae5eaf10 PCI: ignore pre-1.1 ASPM quirking when ASPM is disabled
Right now we won't touch ASPM state if ASPM is disabled, except in the case where we find a device that appears to be too old to reliably support ASPM. Right now we'll clear it in that case, which is almost certainly the wrong thing to do

Try to not touch pre-1.1 ASPM for all, and it causes lots of regression.

So second commit

cdb0f9a1ad2e ASPM: Fix pcie devices with non-pcie children
Since 3.2.12 and 3.3, some systems are failing to boot with a BUG_ON. Some other systems using the pata_jmicron driver fail to boot because no disks are detected. Passing pcie_aspm=force on the kernel command line works around it.

move the check aspm_disabled down.

but ath5 and etc (pre-1.1) really need to aspm_disable to change their hw setting.

So the right solution would be dropping pcie_aspm_sanity_check() change -in v2 should make all both happy, as quirk and disable that in driver for ath5 are calling
pcie_disable_aspm_state explicitly.

In v2, we already removed pcie_clear_aspm() that is calling pcie_disable_aspm_state.

Please check attached -v3.

Quoted 45 lines · Yinghai Lu

attatched -v3 again

Quoted 42 lines · Yinghai Lu

Quoted 7 lines · Bjorn Helgaas

Half of your v1 patch (removing the pcie_aspm_sanity_check() test) *might* be the right thing, but only if you can clearly explain why that will not reintroduce the bug Matthew fixed with c9651e70.

I think we also need to fix the PCI_FIXUP_FINAL quirk regression, but that's a separate issue and should be a separate patch.

First commit from Matthew
0ae5eaf10 PCI: ignore pre-1.1 ASPM quirking when ASPM is disabled
Right now we won't touch ASPM state if ASPM is disabled, except in the case where we find a device that appears to be too old to reliably support ASPM. Right now we'll clear it in that case, which is almost certainly the wrong thing to do

Try to not touch pre-1.1 ASPM for all, and it causes lots of regression.

So second commit

cdb0f9a1ad2e ASPM: Fix pcie devices with non-pcie children
Since 3.2.12 and 3.3, some systems are failing to boot with a BUG_ON. Some other systems using the pata_jmicron driver fail to boot because no disks are detected. Passing pcie_aspm=force on the kernel command line works around it.

move the check aspm_disabled down.

but ath5 and etc (pre-1.1) really need to aspm_disable to change their hw setting.

So the right solution would be dropping pcie_aspm_sanity_check() change -in v2 should make all both happy, as quirk and disable that in driver for ath5 are calling
pcie_disable_aspm_state explicitly.

In v2, we already removed pcie_clear_aspm() that is calling pcie_disable_aspm_state.

Please check attached -v3.

It's getting late in the v3.9 cycle already, and while your v3 patch probably fixes Roman's problem, I can't convince myself that it is safe in general.

I think the safest thing to do at this point is to revert 8c33f51df ("PCI/ACPI: Request _OSC control before scanning PCI root bus") with a patch like the one below.

That does mean the booting path and hotplug paths will be different (we set aspm_disabled after boot but before hotplug), but it was that way for a long time before 8c33f51df. I think it's more important to fix this recent ath5k regression than to fix a long-standing hotplug bug that nobody ever complained about.

Obviously, I think we should fix the hotplug bug and clean up the ASPM mess, too. But we need to do that when we have more time to do it right and test it.

Bjorn

commit 96e5d01cd536458435ef0678d9fa3dc542afb41f

Author: Bjorn Helgaas <[email protected]>
Date:   Mon Apr 1 15:47:39 2013 -0600

Revert "PCI/ACPI: Request _OSC control before scanning PCI root bus"

This reverts commit 8c33f51df406e1a1f7fa4e9b244845b7ebd61fa6.

Conflicts:
drivers/acpi/pci_root.c

Reference: https://bugzilla.kernel.org/show_bug.cgi?id=55211

Signed-off-by Bjorn Helgaas <[email protected]>

drivers/acpi/pci_root.c +37 -39

index 0ac546d..c740364 100644
@@ -415,7 +415,6 @@ static int acpi_pci_root_add(struct acpi_device *device,
 	struct acpi_pci_root *root;
 	struct acpi_pci_driver *driver;
 	u32 flags, base_flags;
-	bool is_osc_granted = false;
 
 	root = kzalloc(sizeof(struct acpi_pci_root), GFP_KERNEL);
 	if (!root)

@@ -476,6 +475,30 @@ 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;

@@ -494,6 +517,7 @@ 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

@@ -514,54 +538,28 @@ 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);
-	}
-
-	/*
-	 * 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();
+			 "Unable to request _OSC control "
+			 "(_OSC support mask: 0x%02x)\n", flags);
 	}
 
 	pci_acpi_add_bus_pm_notifier(device, root->bus);

Quoted 3 lines · Yinghai Lu

attatched -v3 again

Quoted 1 line

Please check attached -v3.

It's getting late in the v3.9 cycle already, and while your v3 patch probably fixes Roman's problem, I can't convince myself that it is safe in general.

I think the safest thing to do at this point is to revert 8c33f51df ("PCI/ACPI: Request _OSC control before scanning PCI root bus") with a patch like the one below.

That does mean the booting path and hotplug paths will be different (we set aspm_disabled after boot but before hotplug), but it was that way for a long time before 8c33f51df. I think it's more important to fix this recent ath5k regression than to fix a long-standing hotplug bug that nobody ever complained about.

Obviously, I think we should fix the hotplug bug and clean up the ASPM mess, too. But we need to do that when we have more time to do it right and test it.

Bjorn

commit 96e5d01cd536458435ef0678d9fa3dc542afb41f

Author: Bjorn Helgaas <[email protected]>
Date:   Mon Apr 1 15:47:39 2013 -0600

Revert "PCI/ACPI: Request _OSC control before scanning PCI root bus"

This reverts commit 8c33f51df406e1a1f7fa4e9b244845b7ebd61fa6.

Conflicts:
drivers/acpi/pci_root.c

Reference: https://bugzilla.kernel.org/show_bug.cgi?id=55211

Signed-off-by Bjorn Helgaas <[email protected]>

Quoted 5 lines · Bjorn Helgaas

Quoted 3 lines · Yinghai Lu

attatched -v3 again

Quoted 1 line

Please check attached -v3.

Quoted 7 lines

It's getting late in the v3.9 cycle already, and while your v3 patch probably fixes Roman's problem, I can't convince myself that it is safe in general.

I think the safest thing to do at this point is to revert 8c33f51df ("PCI/ACPI: Request _OSC control before scanning PCI root bus") with a patch like the one below.

Agreed.

Quoted 10 lines

That does mean the booting path and hotplug paths will be different (we set aspm_disabled after boot but before hotplug), but it was that way for a long time before 8c33f51df. I think it's more important to fix this recent ath5k regression than to fix a long-standing hotplug bug that nobody ever complained about.

Obviously, I think we should fix the hotplug bug and clean up the ASPM mess, too. But we need to do that when we have more time to do it right and test it.

Sure.

Quoted 15 lines

Bjorn

commit 96e5d01cd536458435ef0678d9fa3dc542afb41f

Author: Bjorn Helgaas <[email protected]>
Date:   Mon Apr 1 15:47:39 2013 -0600

Revert "PCI/ACPI: Request _OSC control before scanning PCI root bus"

This reverts commit 8c33f51df406e1a1f7fa4e9b244845b7ebd61fa6.

Conflicts:
drivers/acpi/pci_root.c

Quoted 2 lines

Reference: https://bugzilla.kernel.org/show_bug.cgi?id=55211

Signed-off-by Bjorn Helgaas <[email protected]>

Acked-by Yinghai Lu <[email protected]>

stable need this reverting too.

Quoted 11 lines · Yinghai Lu

Quoted 11 lines

commit 96e5d01cd536458435ef0678d9fa3dc542afb41f

Author: Bjorn Helgaas <[email protected]>
Date:   Mon Apr 1 15:47:39 2013 -0600

Revert "PCI/ACPI: Request _OSC control before scanning PCI root bus"

This reverts commit 8c33f51df406e1a1f7fa4e9b244845b7ebd61fa6.

Conflicts:
drivers/acpi/pci_root.c

Quoted 6 lines

Quoted 2 lines

Reference: https://bugzilla.kernel.org/show_bug.cgi?id=55211

Signed-off-by Bjorn Helgaas <[email protected]>

Acked-by Yinghai Lu <[email protected]>

stable need this reverting too.

I updated the changelog and added this to my for-linus branch, headed for v3.9.

Quoted 12 lines · Bjorn Helgaas

Quoted 11 lines · Yinghai Lu

Quoted 11 lines

commit 96e5d01cd536458435ef0678d9fa3dc542afb41f

Author: Bjorn Helgaas <[email protected]>
Date:   Mon Apr 1 15:47:39 2013 -0600

Revert "PCI/ACPI: Request _OSC control before scanning PCI root bus"

This reverts commit 8c33f51df406e1a1f7fa4e9b244845b7ebd61fa6.

Conflicts:
drivers/acpi/pci_root.c

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

Reference: https://bugzilla.kernel.org/show_bug.cgi?id=55211

Signed-off-by Bjorn Helgaas <[email protected]>

Acked-by Yinghai Lu <[email protected]>

stable need this reverting too.

I updated the changelog and added this to my for-linus branch, headed for v3.9.

We may need to revert this reverting for 3.10.

Today noticed that acpiphp jump in before pciehp on one setup.

current code from 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.

so either we need to
A. revert reverting about _OSC
B. move pcibios_add_bus down to pci_bus_add_devices()
as acpiphp and apci pci slot driver are some kind of drivers for pci_bus
C. A+B

Quoted 13 lines

current code from 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

Quoted 7 lines

so we always have acpiphp hotplug slot registered at first.

so either we need to
A. revert reverting about _OSC
B. move pcibios_add_bus down to pci_bus_add_devices()
as acpiphp and apci pci slot driver are some kind of drivers for pci_bus
C. A+B

It doesn't surprise me at all that there are problems in the _OSC code and the acpiphp/pciehp interaction. That whole area is a complete disaster. It'd really be nice if somebody stepped up and reworked it so it makes sense.

But this report is useless to me. I don't have time to work out what the problem is and how it affects users and come up with a fix.

My advice is to simplify the path first, and worry about fixing the bug afterwards. We've already done several iterations of fiddling with things, and I think all we're doing is playing "whack-a-mole" and pushing the bugs around from one place to another.

Quoted 21 lines · Bjorn Helgaas

Quoted 13 lines

current code from 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

Quoted 7 lines

so we always have acpiphp hotplug slot registered at first.

so either we need to
A. revert reverting about _OSC
B. move pcibios_add_bus down to pci_bus_add_devices()
as acpiphp and apci pci slot driver are some kind of drivers for pci_bus
C. A+B

Quoted 7 lines

It doesn't surprise me at all that there are problems in the _OSC code and the acpiphp/pciehp interaction. That whole area is a complete disaster. It'd really be nice if somebody stepped up and reworked it so it makes sense.

But this report is useless to me. I don't have time to work out what the problem is and how it affects users and come up with a fix.

effects: without fix the problem, user can not use pcie native hotplug if their system's firmware support acpihp and pciehp.
And make it worse, that acpiphp have to be built-in, so they have no way to blacklist acpiphp in config.

Quoted 5 lines

My advice is to simplify the path first, and worry about fixing the bug afterwards. We've already done several iterations of fiddling with things, and I think all we're doing is playing "whack-a-mole" and pushing the bugs around from one place to another.

We need to address regression at first.
my suggestion is : revert the reverting and apply my -v3 version that will fix regression that Roman Yepishev met.

please check attached two patches, hope it could save your some time.

Quoted 41 lines · Yinghai Lu

Quoted 21 lines · Bjorn Helgaas

Quoted 13 lines

current code from 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

Quoted 7 lines

so we always have acpiphp hotplug slot registered at first.

so either we need to
A. revert reverting about _OSC
B. move pcibios_add_bus down to pci_bus_add_devices()
as acpiphp and apci pci slot driver are some kind of drivers for pci_bus
C. A+B

Quoted 7 lines

It doesn't surprise me at all that there are problems in the _OSC code and the acpiphp/pciehp interaction. That whole area is a complete disaster. It'd really be nice if somebody stepped up and reworked it so it makes sense.

But this report is useless to me. I don't have time to work out what the problem is and how it affects users and come up with a fix.

effects: without fix the problem, user can not use pcie native hotplug if their system's firmware support acpihp and pciehp.
And make it worse, that acpiphp have to be built-in, so they have no way to blacklist acpiphp in config.

Quoted 5 lines

My advice is to simplify the path first, and worry about fixing the bug afterwards. We've already done several iterations of fiddling with things, and I think all we're doing is playing "whack-a-mole" and pushing the bugs around from one place to another.

Quoted 5 lines

We need to address regression at first.
my suggestion is : revert the reverting and apply my -v3 version that will fix regression that Roman Yepishev met.

please check attached two patches, hope it could save your some time.

OK, you're right. It's not reasonable to do anything more than a minimal fix when we're at -rc5.

Sigh. I'll spend tomorrow trying to understand your patches and write changelogs for you.

I think you're saying that in systems that support both acpiphp and pciehp, we should be using pciehp, but we currently use acpiphp. If so, that's certainly a bug. How serious is it? Is it a disaster if we use acpiphp until we can resolve this cleanly? Are there a lot of systems that claim to support acpiphp but it doesn't actually work?

Bjorn

.

Hi Bjorn,
I'm working on several acpiphp related bugfixes, and feel some
are materials for 3.10 too. Actually we have identified four bugs related to dock station support on Sony VAIO VPCZ23A4R laptop. I will try to send out patchset to address these bugs tonight. Seems we really need to rethink about acpiphp and pciehp now.

On Wed, Jun 12, 2013 at 1:41 PM, Yinghai Lu <[email protected]> wrote:

I think you're saying that in systems that support both acpiphp and pciehp, we should be using pciehp, but we currently use acpiphp. If so, that's certainly a bug. How serious is it? Is it a disaster if we use acpiphp until we can resolve this cleanly? Are there a lot of systems that claim to support acpiphp but it doesn't actually work?

Quoted 7 lines · Bjorn Helgaas

On Wed, Jun 12, 2013 at 1:41 PM, Yinghai Lu <[email protected]> wrote:

I think you're saying that in systems that support both acpiphp and pciehp, we should be using pciehp, but we currently use acpiphp. If so, that's certainly a bug. How serious is it? Is it a disaster if we use acpiphp until we can resolve this cleanly? Are there a lot of systems that claim to support acpiphp but it doesn't actually work?

No sure. To make acpiphp would need more expertise in bios. Normally BIOS vendor would have half done work there, and will need OEM or system vendor have someone to make it work .... You would not want to read asl code in DSDT to help them out. That is not something that we can control.

Quoted 21 lines · Bjorn Helgaas

Quoted 13 lines

current code from 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

Quoted 7 lines

so we always have acpiphp hotplug slot registered at first.

so either we need to
A. revert reverting about _OSC
B. move pcibios_add_bus down to pci_bus_add_devices()
as acpiphp and apci pci slot driver are some kind of drivers for pci_bus
C. A+B

Quoted 7 lines

It doesn't surprise me at all that there are problems in the _OSC code and the acpiphp/pciehp interaction. That whole area is a complete disaster. It'd really be nice if somebody stepped up and reworked it so it makes sense.

But this report is useless to me. I don't have time to work out what the problem is and how it affects users and come up with a fix.

effects: without fix the problem, user can not use pcie native hotplug if their system's firmware support acpihp and pciehp.
And make it worse, that acpiphp have to be built-in, so they have no way to blacklist acpiphp in config.

Quoted 5 lines

My advice is to simplify the path first, and worry about fixing the bug afterwards. We've already done several iterations of fiddling with things, and I think all we're doing is playing "whack-a-mole" and pushing the bugs around from one place to another.

We need to address regression at first.
my suggestion is : revert the reverting and apply my -v3 version that will fix regression that Roman Yepishev met.

please check attached two patches, hope it could save your some time.

Here are some of my notes from trying to sort this out, in chronological order:

29594404 v3.7
Bus scanned before requesting _OSC control pre-1.1 ath5k has ASPM disabled (works fine)

8c33f51d "request _OSC control before scanning bus"

19f949f5 v3.8
_OSC control requested before scanning bus
Now pre-1.1 ath5k has ASPM enabled and doesn't work
https://bugzilla.kernel.org/show_bug.cgi?id=55211 opened

b8178f13 "revert 'request _OSC control before scanning bus' (8c33f51d)"
Bus now scanned before requesting _OSC control (as in v3.7)

c1be5a5b v3.9
pciehp claims slots first, even when both pciehp & acpiphp are built-in, because pciehp module_init precedes acpiphp module_init in link order

6037a803 "Convert acpiphp to be builtin only"
This also adds "acpiphp.disable" boot option

3b63aaa7 "Do not use ACPI PCI subdriver mechanism"
Now acpiphp claims slots first because we call acpiphp_enumerate_slots() from pcibios_add_bus() during PCI device enumeration. This happens before pciehp, which still uses module_init.

f722406f v3.10-rc1

........ "Revert reverting of 'request _OSC control before scanning bus' (b8178f13)"
_OSC control requested before scanning bus (as in v3.8)
pre-1.1 ath5k probably has ASPM enabled and doesn't work

........ "Remove not needed check in disable aspm link"
Now pci_disable_link_state() unconditionally disables ASPM, even when BIOS hasn't given us ASPM control

1) The problem you're trying to fix is that when both acpiphp and pciehp are supported for the same slot, acpiphp claims the slot first and pciehp will not claim it. I think this problem was introduced by 3b63aaa7, which was merged after v3.9. Therefore, v3.9 should work correctly, and this regression appeared in v3.10-rc1.

2) As you say, acpiphp cannot be a module, so the user would have to rebuild the kernel to remove it. However, 6037a803 *did* add a "acpiphp.disable" boot option, so that should be a workaround that allows pciehp to claim the slot.

3) I think your "revert reverting" patch gets us back to the same situation we had after 8c33f51d, i.e., Roman's pre-1.1 ath5k device will have ASPM enabled and won't work. I don't want to leave the tree in this broken state, even though you intend to fix it in the next patch. If you can reorder your patches so the ASPM fix is first, that would be better.

4) Your "Remove not needed check in disable aspm link" patch makes pci_disable_link_state() disable ASPM even when the OS doesn't have permission to control ASPM. I think this is a mistake. I proposed a similar change in [1], but Rafael and Matthew thought it was too risky, and I agree.

Quoted 48 lines · Bjorn Helgaas

Here are some of my notes from trying to sort this out, in chronological order:

29594404 v3.7
Bus scanned before requesting _OSC control pre-1.1 ath5k has ASPM disabled (works fine)

8c33f51d "request _OSC control before scanning bus"

19f949f5 v3.8
_OSC control requested before scanning bus
Now pre-1.1 ath5k has ASPM enabled and doesn't work
https://bugzilla.kernel.org/show_bug.cgi?id=55211 opened

b8178f13 "revert 'request _OSC control before scanning bus' (8c33f51d)"
Bus now scanned before requesting _OSC control (as in v3.7)

c1be5a5b v3.9
pciehp claims slots first, even when both pciehp & acpiphp are built-in, because pciehp module_init precedes acpiphp module_init in link order

6037a803 "Convert acpiphp to be builtin only"
This also adds "acpiphp.disable" boot option

3b63aaa7 "Do not use ACPI PCI subdriver mechanism"
Now acpiphp claims slots first because we call acpiphp_enumerate_slots() from pcibios_add_bus() during PCI device enumeration. This happens before pciehp, which still uses module_init.

f722406f v3.10-rc1

........ "Revert reverting of 'request _OSC control before scanning bus' (b8178f13)"
_OSC control requested before scanning bus (as in v3.8)
pre-1.1 ath5k probably has ASPM enabled and doesn't work

........ "Remove not needed check in disable aspm link"
Now pci_disable_link_state() unconditionally disables ASPM, even when BIOS hasn't given us ASPM control

1) The problem you're trying to fix is that when both acpiphp and pciehp are supported for the same slot, acpiphp claims the slot first and pciehp will not claim it. I think this problem was introduced by 3b63aaa7, which was merged after v3.9. Therefore, v3.9 should work correctly, and this regression appeared in v3.10-rc1.

Quoted 4 lines

2) As you say, acpiphp cannot be a module, so the user would have to rebuild the kernel to remove it. However, 6037a803 *did* add a "acpiphp.disable" boot option, so that should be a workaround that allows pciehp to claim the slot.

How about the same system that some slots need to be handled by acpiphp and some others need to be handled by pciehp ?

for example: laptop that have dock that will need acpiphp, and also have pci express card that need pciehp.

Quoted 7 lines

3) I think your "revert reverting" patch gets us back to the same situation we had after 8c33f51d, i.e., Roman's pre-1.1 ath5k device will have ASPM enabled and won't work. I don't want to leave the tree in this broken state, even though you intend to fix it in the next patch. If you can reorder your patches so the ASPM fix is first, that would be better.

yes.

We could apply your patch in [1] at first, and revert the reverting. and do not touch pcie_clear_aspm now.

Quoted 1 line

Quoted 5 lines

4) Your "Remove not needed check in disable aspm link" patch makes pci_disable_link_state() disable ASPM even when the OS doesn't have permission to control ASPM. I think this is a mistake. I proposed a similar change in [1], but Rafael and Matthew thought it was too risky, and I agree.

before all those changes, and in current state:
quirk disable aspm is before _osc support and control are set.

aka in pci_acpi_scan_root will allocate all link state struct, and quirk call pci_disable_link_state, and later will _osc support or control can not be set, pcie_no_aspm is called, can will block all aspm operation.

That is risky too?, why booting path quirk could do that, but driver and hot-add quirk path can not do that ?

or we can have another pci_disable_link_state always work on quirk path only?

Yinghai

On Fri, Jun 14, 2013 at 10:17 AM, Yinghai Lu <[email protected]> wrote:

Can you please refer to specific function names? I can't read your mind.

You might be referring to quirk_disable_aspm_l0s(). This is a pci_fixup_final quirk that calls pci_disable_link_state(). In the current tree, we enumerate devices before requesting _OSC control. However, pci_fixup_final quirks are not run until the pci_apply_final_quirks() fs_initcall, which is after we request _OSC control.

As far as I can tell, we never call pci_disable_link_state() before calling pcie_no_aspm().

Quoted 11 lines · Bjorn Helgaas

On Fri, Jun 14, 2013 at 10:17 AM, Yinghai Lu <[email protected]> wrote:

Can you please refer to specific function names? I can't read your mind.

You might be referring to quirk_disable_aspm_l0s(). This is a pci_fixup_final quirk that calls pci_disable_link_state(). In the current tree, we enumerate devices before requesting _OSC control. However, pci_fixup_final quirks are not run until the pci_apply_final_quirks() fs_initcall, which is after we request _OSC control.

Quoted 2 lines

As far as I can tell, we never call pci_disable_link_state() before calling pcie_no_aspm().

ok, you are right, that is not pci_disable_link_state.

It is pcie_aspm_init_link_state ==> pcie_aspm_sanity_check in booting path that disable aspm. It has "if (aspm_disabled)" in it, and it cause the difference.

Quoted 12 lines · Yinghai Lu

Quoted 11 lines · Bjorn Helgaas

On Fri, Jun 14, 2013 at 10:17 AM, Yinghai Lu <[email protected]> wrote:

Can you please refer to specific function names? I can't read your mind.

You might be referring to quirk_disable_aspm_l0s(). This is a pci_fixup_final quirk that calls pci_disable_link_state(). In the current tree, we enumerate devices before requesting _OSC control. However, pci_fixup_final quirks are not run until the pci_apply_final_quirks() fs_initcall, which is after we request _OSC control.

Quoted 8 lines

Quoted 2 lines

As far as I can tell, we never call pci_disable_link_state() before calling pcie_no_aspm().

ok, you are right, that is not pci_disable_link_state.

It is pcie_aspm_init_link_state ==> pcie_aspm_sanity_check in booting path that disable aspm. It has "if (aspm_disabled)" in it, and it cause the difference.

Yes, I agree, the pcie_aspm_init_link_state() path uses aspm_disabled before we set it:

acpi_pci_root_add
pci_acpi_scan_root
pci_scan_child_bus
pci_scan_slot
pcie_aspm_init_link_state
pcie_aspm_sanity_check
if (aspm_disabled) # used before set
...
acpi_pci_osc_control_set
pcie_no_aspm
aspm_disabled = 1 # set

That might mean we do some ASPM configuration during enumeration (in pci_scan_slot()) even though the BIOS hasn't given us permission. It looks like we did that even in v3.7, since we did the enumeration before the _OSC there as well. That looks like a bug to me.

I don't think the fact that we have been doing ASPM config during enumeration before _OSC is an argument for dropping the check in pci_disable_link_state().

Quoted 37 lines · Bjorn Helgaas

Quoted 12 lines · Yinghai Lu

Quoted 11 lines · Bjorn Helgaas

On Fri, Jun 14, 2013 at 10:17 AM, Yinghai Lu <[email protected]> wrote:

Can you please refer to specific function names? I can't read your mind.

You might be referring to quirk_disable_aspm_l0s(). This is a pci_fixup_final quirk that calls pci_disable_link_state(). In the current tree, we enumerate devices before requesting _OSC control. However, pci_fixup_final quirks are not run until the pci_apply_final_quirks() fs_initcall, which is after we request _OSC control.

Quoted 8 lines

Quoted 2 lines

As far as I can tell, we never call pci_disable_link_state() before calling pcie_no_aspm().

ok, you are right, that is not pci_disable_link_state.

It is pcie_aspm_init_link_state ==> pcie_aspm_sanity_check in booting path that disable aspm. It has "if (aspm_disabled)" in it, and it cause the difference.

Yes, I agree, the pcie_aspm_init_link_state() path uses aspm_disabled before we set it:

acpi_pci_root_add
pci_acpi_scan_root
pci_scan_child_bus
pci_scan_slot
pcie_aspm_init_link_state
pcie_aspm_sanity_check
if (aspm_disabled) # used before set
...
acpi_pci_osc_control_set
pcie_no_aspm
aspm_disabled = 1 # set

Quoted 4 lines

That might mean we do some ASPM configuration during enumeration (in pci_scan_slot()) even though the BIOS hasn't given us permission. It looks like we did that even in v3.7, since we did the enumeration before the _OSC there as well. That looks like a bug to me.

agreed. that means commits from Matthew Garrett

commit 4949be16822e92a18ea0cc1616319926628092ee

Author: Matthew Garrett <[email protected]>
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 <[email protected]>
Date:   Tue Mar 27 10:17:41 2012 -0400

ASPM: Fix pcie devices with non-pcie children

will only works when the user specify "aspm=off" in boot command line.

(Roman's should have problem when he boot current linus tree with "aspm=off", as no one will disable aspm for the offending pci devices)

To close the hole that Matthew' commits miss, that we should move _OSC support/control set ahead.

For Roman's system it will have to fail, as BIOS enable prep-1.1 pcie devices aspm, and do not handle over control to OS, so os can not disable aspm link state for it.

To workaround the problem in Roman's system, we can add pcie_aspm=force_off

so we will have
pcie_aspm=off
pcie_aspm=force
pcie_aspm=force_off

What a mess!

[+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

Quoted 2 lines · Yinghai Lu

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.

I don't follow. We were previously automatically disabling ASPM on pre-1.1 devices even if _OSC didn't give us control.

I've confirmed that
this was the wrong thing for us to be doing, and my patch changed the behaviour such that if the firmware enables ASPM on a pre-1.1 device and refuses to give us control via _OSC we will leave ASPM enabled.

Quoted 2 lines · Matthew Garrett

On Fri, 2013-06-14 at 15:17 -0700, Yinghai Lu wrote:

Quoted 5 lines

Quoted 2 lines

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.

I don't follow. We were previously automatically disabling ASPM on pre-1.1 devices even if _OSC didn't give us control.

I don't think so, we just moved _OSC support/control setting before pci scan in 3.8 and revert that in v3.9.

Quoted 10 lines · Yinghai Lu

Quoted 7 lines · Matthew Garrett

Quoted 2 lines · Yinghai Lu

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.

I don't follow. We were previously automatically disabling ASPM on pre-1.1 devices even if _OSC didn't give us control.

Quoted 2 lines

I don't think so, we just moved _OSC support/control setting before pci scan in 3.8 and revert that in v3.9.

Right, sorry, I don't mean _OSC, I mean the FADT flag. We were previously automatically disabling ASPM on pre-1.1 devices even if the FADT flag was set.

Previously we issued a hotplug command and waited for it to complete. But there's no need to wait until we're ready to issue the *next* command. The next command will probably be much later, so the first one may have already completed and we may not have to actually wait at all.

Because of hardware errata, some controllers generate command completion events for some commands but not others. In the case of Intel CF118 (see spec update reference), the controller indicates command completion only for Slot Control writes that change the value of the following bits:

Power Controller Control
Power Indicator Control
Attention Indicator Control
Electromechanical Interlock Control

Changes to other bits, e.g., the interrupt enable bits, do not cause the Command Completed bit to be set. Controllers from AMD and Nvidia are reported to have similar errata.

These errata cause timeouts when pcie_enable_notification() enables interrupts. Previously that timeout occurred at boot-time. With this change, the timeout occurs later, when we change the state of the slot power, indicators, or interlock. This speeds up boot but causes a timeout at the first hotplug event on the slot. Subsequent events don't timeout because only the first (boot-time) hotplug command updates Slot Control without touching the power/indicator/interlock controls.

Link http://www.intel.com/content/www/us/en/processors/xeon/xeon-e7-v2-spec-update.html

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/pci/hotplug/pciehp_hpc.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

drivers/pci/hotplug/pciehp_hpc.c +3 -5

index 0e76e9d9d134..f44fdb5b0b08 100644
@@ -165,6 +165,9 @@ static void pcie_write_cmd(struct controller *ctrl, u16 cmd, u16 mask)
 
 	mutex_lock(&ctrl->ctrl_lock);
 
+	/* Wait for any previous command that might still be in progress */
+	pcie_wait_cmd(ctrl);
+
 	pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
 	if (slot_status & PCI_EXP_SLTSTA_CC) {
 		pcie_capability_write_word(pdev, PCI_EXP_SLTSTA,

@@ -197,10 +200,6 @@ static void pcie_write_cmd(struct controller *ctrl, u16 cmd, u16 mask)
 	pcie_capability_write_word(pdev, PCI_EXP_SLTCTL, slot_ctrl);
 	ctrl->slot_ctrl = slot_ctrl;
 
-	/*
-	 * Wait for command completion.
-	 */
-	pcie_wait_cmd(ctrl);
 	mutex_unlock(&ctrl->ctrl_lock);
 }
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Sorry, I'm digging up and old patch here and this RFC was the only copy I could find in my mailbox.

Quoted 4 lines · Bjorn Helgaas

Previously we issued a hotplug command and waited for it to complete. But there's no need to wait until we're ready to issue the *next* command. The next command will probably be much later, so the first one may have already completed and we may not have to actually wait at all.

I'm seeing a regression as a result of this patch. Consider the following function:

int pciehp_reset_slot(struct slot *slot, int probe)

{
	...
	pcie_write_cmd(ctrl, 0, ctrl_mask);
	...
	pci_reset_bridge_secondary_bus(ctrl->pcie->port);

Our command write is clearing bits that control whether the slot has presence detection and link layer state change enabled. These are the things that we're trying to clear to prevent a secondary bus reset from turning into a surprise hotplug. If we don't wait for the command to complete, what state are when in as we initiate the secondary bus reset? On my system, it's like we never issued the write command at all, the previous behavior where the hotplug controller detects the link down as a surprise hotplug returns. So I'm afraid we do need to wait until we're ready to issue the *next* command, because it's our only indication that the current command has completed.

pciehp_set_attention_status
pciehp_green_led_on
pciehp_green_led_off
pciehp_green_led_blink
pciehp_power_on_slot
pciehp_power_off_slot
pcie_enable_notification
pcie_disable_notification

The slot power ones concern me the same as the reset issue. In the 'on' case, we immediately call link enable, so the slot better have acknowledged the power on command. In the 'off' case, it seems pretty bad to complete the 'off' function, but we don't know if the slot is actually off yet.

For notifications, I can imagine that the caller of
those functions is going to want on or off at the completion of those functions, not some in-between state.

Even in the case of the LED
functions, where do we want the error to occur, at the time the command is issued, or maybe some time in the future when the next unrelated command comes through.

So, rather than posting a new patch adding a flush everywhere that it seems a little scary, should we consider scrapping this patch altogether as unsafe? We need to wait, not only to issue the next command, but to have any sort of acknowledgment that the current command has completed.

Quoted 14 lines

Because of hardware errata, some controllers generate command completion events for some commands but not others. In the case of Intel CF118 (see spec update reference), the controller indicates command completion only for Slot Control writes that change the value of the following bits:

Power Controller Control
Power Indicator Control
Attention Indicator Control
Electromechanical Interlock Control

Changes to other bits, e.g., the interrupt enable bits, do not cause the Command Completed bit to be set. Controllers from AMD and Nvidia are reported to have similar errata.

Quoted 7 lines

These errata cause timeouts when pcie_enable_notification() enables interrupts. Previously that timeout occurred at boot-time. With this change, the timeout occurs later, when we change the state of the slot power, indicators, or interlock. This speeds up boot but causes a timeout at the first hotplug event on the slot. Subsequent events don't timeout because only the first (boot-time) hotplug command updates Slot Control without touching the power/indicator/interlock controls.

It sounds like we need some sort of completion mask to handle devices like this, instead of effectively removing the write-barrier for the general case. Thanks,

Quoted 20 lines · Alex Williamson

Sorry, I'm digging up and old patch here and this RFC was the only copy I could find in my mailbox.

Quoted 4 lines · Bjorn Helgaas

Previously we issued a hotplug command and waited for it to complete. But there's no need to wait until we're ready to issue the *next* command. The next command will probably be much later, so the first one may have already completed and we may not have to actually wait at all.

I'm seeing a regression as a result of this patch. Consider the following function:

int pciehp_reset_slot(struct slot *slot, int probe)

{
	...
	pcie_write_cmd(ctrl, 0, ctrl_mask);
	...
	pci_reset_bridge_secondary_bus(ctrl->pcie->port);

Quoted 10 lines

Our command write is clearing bits that control whether the slot has presence detection and link layer state change enabled. These are the things that we're trying to clear to prevent a secondary bus reset from turning into a surprise hotplug. If we don't wait for the command to complete, what state are when in as we initiate the secondary bus reset? On my system, it's like we never issued the write command at all, the previous behavior where the hotplug controller detects the link down as a surprise hotplug returns. So I'm afraid we do need to wait until we're ready to issue the *next* command, because it's our only indication that the current command has completed.

I think that's a bug. If we do something that depends on a command completion, we'll have to wait for it.

Quoted 9 lines

pciehp_set_attention_status
pciehp_green_led_on
pciehp_green_led_off
pciehp_green_led_blink
pciehp_power_on_slot
pciehp_power_off_slot
pcie_enable_notification
pcie_disable_notification

Quoted 5 lines

The slot power ones concern me the same as the reset issue. In the 'on' case, we immediately call link enable, so the slot better have acknowledged the power on command. In the 'off' case, it seems pretty bad to complete the 'off' function, but we don't know if the slot is actually off yet.

I agree.

Quoted 3 lines

For notifications, I can imagine that the caller of
those functions is going to want on or off at the completion of those functions, not some in-between state.

These control interrupt generation by the hotplug controller in the switch. When disabling interrupt generation, I think we should wait for completion before we call free_irq(); waiting should prevent spurious interrupts. When enabling interrupt generation, we've already hooked up the ISR before we enable interrupt generation, so I don't see a need to wait.

Quoted 4 lines

Even in the case of the LED
functions, where do we want the error to occur, at the time the command is issued, or maybe some time in the future when the next unrelated command comes through.

The only error here is the "Timeout on hotplug command" message, and we don't do anything other than print the message, so I'm not too worried about this one.

Quoted 4 lines

So, rather than posting a new patch adding a flush everywhere that it seems a little scary, should we consider scrapping this patch altogether as unsafe? We need to wait, not only to issue the next command, but to have any sort of acknowledgment that the current command has completed.

I definitely screwed up by assuming that completion was only useful for pacing writes to the command register. I think lazy checking is fine for command pacing, but we certainly need to know about completions for other things, too.

If we added calls to pcie_wait_cmd() in these places:

pciehp_power_on_slot
pciehp_power_off_slot
pcie_disable_notification
pciehp_reset_slot

do you think that would be enough?

Quoted 22 lines

Quoted 14 lines

Because of hardware errata, some controllers generate command completion events for some commands but not others. In the case of Intel CF118 (see spec update reference), the controller indicates command completion only for Slot Control writes that change the value of the following bits:

Power Controller Control
Power Indicator Control
Attention Indicator Control
Electromechanical Interlock Control

Changes to other bits, e.g., the interrupt enable bits, do not cause the Command Completed bit to be set. Controllers from AMD and Nvidia are reported to have similar errata.

Quoted 7 lines

These errata cause timeouts when pcie_enable_notification() enables interrupts. Previously that timeout occurred at boot-time. With this change, the timeout occurs later, when we change the state of the slot power, indicators, or interlock. This speeds up boot but causes a timeout at the first hotplug event on the slot. Subsequent events don't timeout because only the first (boot-time) hotplug command updates Slot Control without touching the power/indicator/interlock controls.

Quoted 3 lines

It sounds like we need some sort of completion mask to handle devices like this, instead of effectively removing the write-barrier for the general case. Thanks,

You mean some quirk-based solution, where we know certain devices don't indicate command completion for certain events? That seems hard because I think there are many devices that have this erratum. It was difficult to extract this out of Intel, and I'm pretty sure other vendors have the same issue.

Quoted 7 lines · Bjorn Helgaas

If we added calls to pcie_wait_cmd() in these places:

pciehp_power_on_slot
pciehp_power_off_slot
pcie_disable_notification
pciehp_reset_slot

Quoted 1 line

do you think that would be enough?

I think that would solve the problem, but the API becomes very difficult to use correctly if the programmer just needs to know that a pcie_wait_cmd() is necessary following a pcie_write_cmd() if you really want to be sure it's synchronous. pcie_write_cmd() should probably incorporate the "safe" behavior and some new _nowait version should handle the special cases where we don't need to wait

Quoted 8 lines · Bjorn Helgaas

drivers/pci/hotplug/pciehp_hpc.c +0 -0

index f44fdb5b0b08..18ac24d84f9b 100644
@@ -142,11 +143,24 @@ static void pcie_wait_cmd(struct controller *ctrl)
        if (ctrl->no_cmd_complete)
                return;
 

drivers/pci/hotplug/pciehp_hpc.c static void pcie_wait_cmd(struct controller *ctrl) · line 147

Quoted 2 lines

drivers/pci/hotplug/pciehp_hpc.c +2 -0

+       if (!ctrl->cmd_busy)
+               return;

Can you move those two lines to second patch ?

Other than that, for all 4 patches

Acked-by Yinghai Lu <[email protected]>

Quoted 9 lines · Yinghai Lu

Quoted 8 lines · Bjorn Helgaas

drivers/pci/hotplug/pciehp_hpc.c +0 -0

index f44fdb5b0b08..18ac24d84f9b 100644
@@ -142,11 +143,24 @@ static void pcie_wait_cmd(struct controller *ctrl)
        if (ctrl->no_cmd_complete)
                return;
 

Quoted 4 lines

drivers/pci/hotplug/pciehp_hpc.c static void pcie_wait_cmd(struct controller *ctrl) · line 147

Quoted 2 lines

drivers/pci/hotplug/pciehp_hpc.c +2 -0

+       if (!ctrl->cmd_busy)
+               return;

Can you move those two lines to second patch ?

I moved them to the first patch ("Make pcie_wait_cmd() self-contained") since that is more concerned with cleaning up pcie_wait_cmd(). It could even be a separate patch; it would have made sense to check cmd_busy before calling pcie_wait_cmd(). But it seemed like overkill to make another patch just for that.

We use incorrect logic to decide whether a PCIe hotplug controller generates command completion events.

5808639bfa98 ("pciehp: fix slow probing") assumed that the Slot Status "Command Completed" bit was set only for commands affecting slot power, indicators, or electromechanical interlock. That assumption is false: per sec. 6.7.3.2 of PCIe spec r3.0, a write targeting any portion of the Slot Control register is a command, and (if command completed events are supported) software must wait for a command to complete before issuing the next command.

5808639bfa98 was to fix boot-time timeouts (see bugzilla below) on a Lenovo Thinkpad R61 with an Intel hotplug controller. The controller probably has the Intel CF118 erratum, which means it doesn't report Command Completed unless the Slot Control power, indicator, or interlock bits are changed. This causes a timeout because pciehp always waits for Command Complete (if supported), regardless of which bits are changed.

Remove the incorrect logic because the timeouts have been addressed differently by these changes:

PCI: pciehp: Wait for hotplug command completion lazily
PCI: pciehp: Compute timeout from hotplug command start time

Link https://bugzilla.kernel.org/show_bug.cgi?id=10751

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/pci/hotplug/pciehp_hpc.c |   39 ++++++++------------------------------
 1 file changed, 8 insertions(+), 31 deletions(-)

drivers/pci/hotplug/pciehp_hpc.c +8 -31

index 18ac24d84f9b..1f70de5359fb 100644
@@ -161,8 +161,10 @@ static void pcie_wait_cmd(struct controller *ctrl)
                rc = wait_event_timeout(ctrl->queue, !ctrl->cmd_busy, timeout);
        else
                rc = pcie_poll_cmd(ctrl, timeout);
+
        if (!rc)
-               ctrl_dbg(ctrl, "Command not completed in 1000 msec\n");
+               ctrl_info(ctrl, "Timeout on hotplug command %#010x\n",
+                         ctrl->slot_ctrl);
 }
 
 /**

@@ -174,7 +176,6 @@ static void pcie_wait_cmd(struct controller *ctrl)
 static void pcie_write_cmd(struct controller *ctrl, u16 cmd, u16 mask)
 {
        struct pci_dev *pdev = ctrl_dev(ctrl);
-       u16 slot_status;
        u16 slot_ctrl;
 
        mutex_lock(&ctrl->ctrl_lock);

@@ -182,30 +183,6 @@ static void pcie_write_cmd(struct controller *ctrl, u16 cmd, u16 mask)
        /* Wait for any previous command that might still be in progress */
        pcie_wait_cmd(ctrl);
 
-       pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
-       if (slot_status & PCI_EXP_SLTSTA_CC) {
-               pcie_capability_write_word(pdev, PCI_EXP_SLTSTA,
-                                          PCI_EXP_SLTSTA_CC);
-               if (!ctrl->no_cmd_complete) {
-                       /*
-                        * After 1 sec and CMD_COMPLETED still not set, just
-                        * proceed forward to issue the next command according
-                        * to spec. Just print out the error message.
-                        */
-                       ctrl_dbg(ctrl, "CMD_COMPLETED not clear after 1 sec\n");
-               } else if (!NO_CMD_CMPL(ctrl)) {
-                       /*
-                        * This controller seems to notify of command completed
-                        * event even though it supports none of power
-                        * controller, attention led, power led and EMI.
-                        */
-                       ctrl_dbg(ctrl, "Unexpected CMD_COMPLETED. Need to wait for command completed event\n");
-                       ctrl->no_cmd_complete = 0;
-               } else {
-                       ctrl_dbg(ctrl, "Unexpected CMD_COMPLETED. Maybe the controller is broken\n");
-               }
-       }
-
        pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &slot_ctrl);
        slot_ctrl &= ~mask;
        slot_ctrl |= (cmd & mask);

@@ -785,14 +762,14 @@ struct controller *pcie_init(struct pcie_device *dev)
        mutex_init(&ctrl->ctrl_lock);
        init_waitqueue_head(&ctrl->queue);
        dbg_ctrl(ctrl);
+
        /*
         * Controller doesn't notify of command completion if the "No
-        * Command Completed Support" bit is set in Slot Capability
-        * register or the controller supports none of power
-        * controller, attention led, power led and EMI.
+        * Command Completed Support" bit is set in Slot Capabilities.
+        * If set, it means the controller can accept hotplug commands
+        * with no delay between them.
         */
-       if (NO_CMD_CMPL(ctrl) ||
-           !(POWER_CTRL(ctrl) | ATTN_LED(ctrl) | PWR_LED(ctrl) | EMI(ctrl)))
+       if (NO_CMD_CMPL(ctrl))
                ctrl->no_cmd_complete = 1;
 
        /* Check if Data Link Layer Link Active Reporting is implemented */