pciehp: Checking pci conf reading to new added device instead of sleep 1s

PATCH In review Yinghai Lu 2 participants 3 messages Dec 13, 2011 – Dec 15, 2011 PCI

lore

Share & tools

lore

Yinghai Lu

author

2011-12-13 09:06 UTC #0 lore raw

PATCH 1/1 pciehp: Checking pci conf reading to new added device instead of sleep 1s

During reviewing
| PCI: pciehp: wait 1000 ms before Link Training check

Quoted 6 lines · Linus

...
That's a *long* time, and it's irritating to the user. It makes the user think "the machine is slow".
...
And quite frankly, an unconditional one-second delay here seems bad. Two seconds was unacceptable, one second is just bad.

Try to access the pci conf of pci device that is supposed to show up in 1s, if could read back valid vender/device id, could bail out early.

Related discussion could be found:

	https://lkml.org/lkml/2011/12/6/339

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

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

drivers/pci/hotplug/pciehp_hpc.c +48 ~~-15~~

Index: linux-2.6/drivers/pci/hotplug/pciehp_hpc.c
===================================================================
@@ -265,6 +265,51 @@ 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 count = 0;
+	u32 lx[50];
+	int delay = 1000, step = 20;
+	bool found = false;
+
+again:
+	if (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, &l))
+		goto wait;
+
+	lx[count++] = l;
+
+	/* 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;
+
+	found = true;
+	goto printout;
+
+wait:
+	msleep(step);
+	delay -= step;
+	if (delay > 0)
+		goto again;
+
+printout:
+	if (count > 1) {
+		int i;
+		printk(KERN_DEBUG "pci %04x:%02x:%02x.%d id reading try %d times with interval %d ms\n",
+			pci_domain_nr(bus), bus->number, PCI_SLOT(devfn),
+			PCI_FUNC(devfn), count, step);
+		for (i = 0; i < count; i++)
+			printk(KERN_DEBUG "  [%02d]=%08x\n", i, lx[i]);
+	}
+
+	return found;
+}
+
 int pciehp_check_link_status(struct controller *ctrl)
 {
 	u16 lnk_status;

@@ -280,13 +325,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 +343,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;

Matthew Wilcox

2011-12-14 04:31 UTC #1 lore raw

Quoted 21 lines · Yinghai Lu

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

+static bool pci_bus_check_dev(struct pci_bus *bus, int devfn)
+{
+	u32 l;
+	int count = 0;
+	u32 lx[50];
+	int delay = 1000, step = 20;
+	bool found = false;
+
+again:
+	if (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, &l))
+		goto wait;
+
+	lx[count++] = l;
+
+	/* 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)

drivers/pci/hotplug/pciehp_hpc.c static void pcie_wait_link_active(struct · line 298

Quoted 10 lines

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

+		goto wait;
+
+	found = true;
+	goto printout;
+
+wait:
+	msleep(step);
+	delay -= step;
+	if (delay > 0)
+		goto again;

This code looks similar to (but not identical to) the code in pci_scan_device(). Please make that code a common function that can be called from all PCI hotplug drivers, not just this one.

Quoted 52 lines

drivers/pci/hotplug/pciehp_hpc.c +16 ~~-16~~

+printout:
+	if (count > 1) {
+		int i;
+		printk(KERN_DEBUG "pci %04x:%02x:%02x.%d id reading try %d times with interval %d ms\n",
+			pci_domain_nr(bus), bus->number, PCI_SLOT(devfn),
+			PCI_FUNC(devfn), count, step);
+		for (i = 0; i < count; i++)
+			printk(KERN_DEBUG "  [%02d]=%08x\n", i, lx[i]);
+	}
+
+	return found;
+}
+
 int pciehp_check_link_status(struct controller *ctrl)
 {
 	u16 lnk_status;

@@ -280,13 +325,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 +343,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;
--

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

Signature · 4 lines

Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Yinghai Lu

author

2011-12-15 19:25 UTC #2 lore raw

Quoted 3 lines · Matthew Wilcox

This code looks similar to (but not identical to) the code in pci_scan_device(). Please make that code a common function that can be called from all PCI hotplug drivers, not just this one.

it seems that other hotplug like acpiphp does not check link status.

Please if you are happy with attached version.
it separates some code from pci_scan_device()

Thanks

Yinghai

From: Yinghai Lu <[email protected]>
Subject: [PATCH -v2] pciehp: Checking pci conf reading to new added device instead of sleep 1s

During reviewing
| PCI: pciehp: wait 1000 ms before Link Training check

Quoted 6 lines · Linus

...
That's a *long* time, and it's irritating to the user. It makes the user think "the machine is slow".
...
And quite frankly, an unconditional one-second delay here seems bad. Two seconds was unacceptable, one second is just bad.

Try to access the pci conf of pci device that is supposed to show up in 1s, if could read back valid vender/device id, could bail out early.

Related discussion could be found:

	https://lkml.org/lkml/2011/12/6/339

-v2: seperate code to pci_bus_read_dev_vendor_id() from pci_scan_device()
and reuse it from pciehp code. Suggested by Matthew Wilcox.

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

---
 drivers/pci/hotplug/pciehp_hpc.c |   49 +++++++++++++++++++++++++++------------
 drivers/pci/pci.h                |    2 +
 drivers/pci/probe.c              |   44 +++++++++++++++++++++--------------
 3 files changed, 63 insertions(+), 32 deletions(-)

drivers/pci/hotplug/pciehp_hpc.c +34 ~~-15~~

Index: linux-2.6/drivers/pci/hotplug/pciehp_hpc.c
===================================================================
@@ -265,6 +265,37 @@ 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 count = 0;
+	u32 lx[50];
+	int delay = 1000, step = 20;
+	bool found = false;
+
+	do {
+		found = pci_bus_read_dev_vendor_id(bus, devfn, &l, step);
+		lx[count++] = l;
+
+		if (found)
+			break;
+
+		msleep(step);
+		delay -= step;
+	} while (delay > 0);
+
+	if (count > 1 && pciehp_debug) {
+		int i;
+		printk(KERN_DEBUG "pci %04x:%02x:%02x.%d id reading try %d times with interval %d ms\n",
+			pci_domain_nr(bus), bus->number, PCI_SLOT(devfn),
+			PCI_FUNC(devfn), count, step);
+		for (i = 0; i < count; i++)
+			printk(KERN_DEBUG "  [%02d]=%08x\n", i, lx[i]);
+	}
+
+	return found;
+}
+
 int pciehp_check_link_status(struct controller *ctrl)
 {
 	u16 lnk_status;

@@ -280,13 +311,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 +329,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;

drivers/pci/probe.c +27 ~~-17~~

67 lines not on this page · raw message

drivers/pci/pci.h +2 -0

Index: linux-2.6/drivers/pci/pci.h
===================================================================
@@ -208,6 +208,8 @@ enum pci_bar_type {
 	pci_bar_mem64,		/* A 64-bit memory BAR */
 };
 
+bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *pl,
+                                 int crs_timeout);
 extern int pci_setup_device(struct pci_dev *dev);
 extern int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 				struct resource *res, unsigned int reg);