PCI fixes

The following changes since commit
839d8810747bbf39e0a5a7f223b67bffa7945f8d:

Merge branch 'i2c-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/jdelvare/staging (2011-10-30 15:54:59 -0700)

are available in the git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/jbarnes/pci for-linus

Nothing too exciting this time, mostly some minor fixes for things normal people don't really hit. Happy Thanksgiving.

Bjorn Helgaas (1):

      PCI hotplug: shpchp: don't blindly claim non-AMD 0x7450 device IDs

Joerg Roedel (2):
PCI: Fix compile errors with PCI_ATS and !PCI_IOV
PCI: Let PCI_PRI depend on PCI

Kenji Kaneshige (2):
PCI: pciehp: wait 1000 ms before Link Training check
PCI: pciehp: wait 100 ms after Link Training check

Rafael J. Wysocki (1):
PCI / ACPI: Make acpiphp ignore root bridges using PCIe native hotplug

Yinghai Lu (1):
PCI: pciehp: Retrieve link speed after link is trained

 drivers/pci/Kconfig                |    1 +
 drivers/pci/hotplug/acpiphp_glue.c |   29 ++++++++++++++++++++++++-----
 drivers/pci/hotplug/pciehp_ctrl.c  |    3 ---
 drivers/pci/hotplug/pciehp_hpc.c   |   27 ++++++++++++++++++---------
 drivers/pci/hotplug/shpchp_core.c  |    4 ++--
 drivers/pci/hotplug/shpchp_hpc.c   |    4 ++--
 include/linux/pci-ats.h            |    6 +++---
 include/linux/pci.h                |    2 +-
 8 files changed, 51 insertions(+), 25 deletions(-)

Jesse Barnes, Intel Open Source Technology Center

Ugh. This looks bogus:

Look at that patch more closely. After the patch, the code looks like this:

         if (ctrl->link_active_reporting)
                 pcie_wait_link_active(ctrl);
         else
                 msleep(1000);

+ /*
+ * Need to wait for 1000 ms after Data Link Layer Link Active
+ * (DLLLA) bit reads 1b before sending configuration request.
+ * We need it before checking Link Training (LT) bit becuase
+ * LT is still set even after DLLLA bit is set on some platform.
+ */
+ msleep(1000);

and I'm pretty sure you should remove the "else msleep(1000)" there. Doing the 1s wait *twice* seems entirely bogus, even if you are missing link_active_reporting. No?

I pulled it since I can't test it, but it really smells fishy to me.

Linus

Sure looks like it... Kenji-san, you went back and forth on this one a little, can you confirm (and preferably test)?

Thanks,

Jesse Barnes, Intel Open Source Technology Center

I'm very sorry for the long delay.

Each 1000 ms wait has defferent meaning. But as Linus pointed out, the first one ("else msleep(1000)") can be removed. I'll work on this. Though the patch is not clean, I tested it from the regression point of view. And Yinghai also confirmed it fix his problem.

Here are explanation for each 1000 ms with history.

This chunk is to wait for Link Active. After PCIe specification 1.1, downstream port with hotplug slots needs to support link active reporting. But older specification PCIe 1.0a doesn't have link active reporting. The "else msleep(1000)" is for PCIe 1.0a. It wait for 1 sec instead of polling data link layer link active bit. In PCIe 1.0a spec, the bit corresponding to the data link layer link active bit in PCIe 1.1 or later is defined reserved zero. So as Linus pointed out, we can remove "else msleep(1000)". The reason why I added "if (crtl->link_active_reporting)" here was that I didn't want to change the behavior for old hot-plug controller (PCIe 1.0a), but I think it was too much.

In the past, pciehp waits 100 ms instead of 1 sec after checking the link state. This 100 ms was based on PCIe spec. But we encountered the problem that configuration access to some HBA card doesn't return the proper value. To fix this problem, we replaced this 100 ms wait with 1 sec wait. This is based on the PCIe description "software must allow 1 second after the Data Link Layer Link Active bit reads 1b before it is permitted to determine that a hot plugged device which fails to return a Successful Completion for a Valid Configuration Request is a broken device". Then, another problem was reported by Yinghai Lu. The problem is that LT bit is still set even after DLLLA bit is set on his platform. To fix the problem 1 sec wait was moved to before LT check, which was placed after LT check.

Regards,
Kenji Kaneshige

I would like to point out that these kinds of delays are *really* annoying to users. And they add up.

One second per se is not a huge problem, but imagine that you're hotplugging some regular user device (think thunderbolt or something that we'd expect normal users to see). First one second for the kernel to even see it, then some random udev rules, then some disk spinup times or whatever, and soon a few delays here and a few delays there, and it takes say three seconds for the folder to show up on the desktop (or whatever acknowledgement of "yes, I see your device now").

That's a *long* time, and it's irritating to the user. It makes the user think "the machine is slow".

We used to have this exact problem with USB hotplugging with slow devices, so I know. It's still not necessarily immediate, but it's better than it has been.

One second *total* is what people will consider pretty much immediate. Any more than that is "thumb twiddling time".

And quite frankly, an unconditional one-second delay here seems bad. Two seconds was unacceptable, one second is just bad.

Quite frankly, the way that reads to me says "you must wait at most 1s before you consider a device broken".

But a *successful* read of the LT bit should abort the wait early. So that good devices that aren't broken can complete setup much faster.

Please try to make something like that work. Instead of always waiting for one second, wait for up to one second only for failure cases. Any possibility of that?

Clearly most devices are perfectly fine almost immediately. It's sad to wait for good devices for no good reason.

Linus

i had another version that was not send out. that worked on my test setups too. it will try to read pci conf several times in 1s.

please check refreshed version against your tree.

Thanks

Yinghai

---
 drivers/pci/hotplug/pciehp_hpc.c |   47 ++++++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 15 deletions(-)

Index: linux-2.6/drivers/pci/hotplug/pciehp_hpc.c
===================================================================
@@ -265,6 +265,35 @@ static void pcie_wait_link_active(struct
 	ctrl_dbg(ctrl, "Data Link Layer Link Active not set in 1000 msec\n");
 }
 
+static bool pci_bus_check_dev(struct pci_bus *bus, int devfn)
+{
+	u32 l;
+	int delay = 1000;
+
+again:
+	if (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, &l))
+		goto wait;
+
+	/* some broken boards return 0 or ~0 if a slot is empty: */
+	if (l == 0xffffffff || l == 0x00000000 ||
+	    l == 0x0000ffff || l == 0xffff0000)
+		goto wait;
+
+	/* Configuration request Retry Status */
+	if (l == 0xffff0001)
+		goto wait;
+
+	return true;
+
+wait:
+	mdelay(100);
+	delay -= 100;
+	if (delay > 0)
+		goto again;
+
+	return false;
+}
+
 int pciehp_check_link_status(struct controller *ctrl)
 {
 	u16 lnk_status;

@@ -280,13 +309,9 @@ int pciehp_check_link_status(struct cont
         else
                 msleep(1000);
 
-	/*
-	 * Need to wait for 1000 ms after Data Link Layer Link Active
-	 * (DLLLA) bit reads 1b before sending configuration request.
-	 * We need it before checking Link Training (LT) bit becuase
-	 * LT is still set even after DLLLA bit is set on some platform.
-	 */
-	msleep(1000);
+	/* wait 100ms before read pci conf, and try in 1s */
+	msleep(100);
+	pci_bus_check_dev(ctrl->pcie->port->subordinate, PCI_DEVFN(0, 0));
 
 	retval = pciehp_readw(ctrl, PCI_EXP_LNKSTA, &lnk_status);
 	if (retval) {

@@ -302,14 +327,6 @@ int pciehp_check_link_status(struct cont
 		return retval;
 	}
 
-	/*
-	 * If the port supports Link speeds greater than 5.0 GT/s, we
-	 * must wait for 100 ms after Link training completes before
-	 * sending configuration request.
-	 */
-	if (ctrl->pcie->port->subordinate->max_bus_speed > PCIE_SPEED_5_0GT)
-		msleep(100);
-
 	pcie_update_link_speed(ctrl->pcie->port->subordinate, lnk_status);
 
 	return retval;

+ /* Configuration request Retry Status */
+ if (l == 0xffff0001)
+ goto wait;

I think this works only when CRS Software Visibility is enabled, isn't it? Currently there is no code to enable CRS Software Visibility. And there can be root ports that doesn't support CRS Software Visibility. We need consideration about this.

+ mdelay(100);
+ delay -= 100;
+ if (delay > 0)
+ goto again;

We must use msleep() instead of mdelay().

Regards,
Kenji Kaneshige

I copied and change from pci_scan_device...

/*
* Read the config data for a PCI device, sanity-check it
* and fill in the dev structure...
*/
static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)

{
        struct pci_dev *dev;
        u32 l;
        int delay = 1;

        if (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, &l))
                return NULL;

        /* some broken boards return 0 or ~0 if a slot is empty: */
        if (l == 0xffffffff || l == 0x00000000 ||
            l == 0x0000ffff || l == 0xffff0000)
                return NULL;

        /* Configuration request Retry Status */
        while (l == 0xffff0001) {
                msleep(delay);
                delay *= 2;
                if (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, &l))
                        return NULL;
                /* Card hasn't responded in 60 seconds?  Must be stuck. */
                if (delay > 60 * 1000) {
                        printk(KERN_WARNING "pci %04x:%02x:%02x.%d: not "
                                        "responding\n", pci_domain_nr(bus),
                                        bus->number, PCI_SLOT(devfn),
                                        PCI_FUNC(devfn));
                        return NULL;
                }
        }

so CRS code is not removed cleanly?

yes.

Thanks

Yinghai Lu

Thank you for comments. Yes, I think we can do more efforts on this.

To improve this, I think what we need is something to know if configuration request to the hot-added device starts working. One possible idea so far is to enable CRS Software Visibility and check vendor ID. The pci_scan_device() function already has vendor ID check, but the code to enable CRS was removed for some reason.

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=ad7edfe0490877864dc0312e5f3315ea37fc4b3a

Regards,
Kenji Kaneshige