Skip to contentSkip to conversation

pciehp: Wait for link get trained in pci_check_link_status()

RFC PATCH In review Yinghai Lu 5 participants 14 messages Nov 8, 2011 – Dec 2, 2011 1 message not in the archive PCI

lore
Share & tools
lore

Yinghai Lu

author
#0 lore raw

RFC PATCH 1/1 pciehp: Wait for link get trained in pci_check_link_status()

Found one PCI Express Modules has link training error after hotplug. It turns out that after DLLLA is set, LT is still set for a while. So pciehp will delcare that hotplug fail in 1s.

HW guys say that pciehp is against PCI-e SPEC:
From PCI Express Base Specification Revision 2.1, Section 6.7.3.3:
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 (see section 6.6).

Try to wait for long enough by adding LT checking in 1s.

Signed-off-by Yinghai Lu <[email protected]>
---
 drivers/pci/hotplug/pciehp_hpc.c |   52 +++++++++++++++++++++++++++------------
 1 file changed, 37 insertions(+), 15 deletions(-)
drivers/pci/hotplug/pciehp_hpc.c +37 -15
Index: linux-2.6/drivers/pci/hotplug/pciehp_hpc.c
===================================================================
@@ -241,28 +241,49 @@ static int pcie_write_cmd(struct control
 	return retval;
 }
 
-static inline int check_link_active(struct controller *ctrl)
+static inline bool
+check_link_status_bits(struct controller *ctrl, u16 mask, u16 val)
 {
 	u16 link_status;
 
 	if (pciehp_readw(ctrl, PCI_EXP_LNKSTA, &link_status))
-		return 0;
-	return !!(link_status & PCI_EXP_LNKSTA_DLLLA);
+		return false;
+
+	if ((link_status & mask) == val)
+		return true;
+
+	return false;
 }
 
-static void pcie_wait_link_active(struct controller *ctrl)
+static bool
+pcie_wait_link_status_bits(struct controller *ctrl, u16 mask, u16 val)
 {
 	int timeout = 1000;
 
-	if (check_link_active(ctrl))
-		return;
+	if (check_link_status_bits(ctrl, mask, val))
+		return true;
 	while (timeout > 0) {
-		msleep(10);
-		timeout -= 10;
-		if (check_link_active(ctrl))
-			return;
+		msleep(20);
+		timeout -= 20;
+		if (check_link_status_bits(ctrl, mask, val))
+			return true;
 	}
-	ctrl_dbg(ctrl, "Data Link Layer Link Active not set in 1000 msec\n");
+
+	return false;
+}
+
+static void pcie_wait_link_active(struct controller *ctrl)
+{
+	if (!pcie_wait_link_status_bits(ctrl, PCI_EXP_LNKSTA_DLLLA,
+					 PCI_EXP_LNKSTA_DLLLA))
+		ctrl_dbg(ctrl,
+			 "Data Link Layer Link Active not set in 1000 msec\n");
+}
+
+static void pcie_wait_link_training_done(struct controller *ctrl)
+{
+	if (!pcie_wait_link_status_bits(ctrl, PCI_EXP_LNKSTA_LT, 0))
+		ctrl_dbg(ctrl, "Link Training is not done in 1000 msec\n");
 }
 
 int pciehp_check_link_status(struct controller *ctrl)

@@ -275,10 +296,11 @@ int pciehp_check_link_status(struct cont
          * hot-plug capable downstream port. But old controller might
          * not implement it. In this case, we wait for 1000 ms.
          */
-        if (ctrl->link_active_reporting)
-                pcie_wait_link_active(ctrl);
-        else
-                msleep(1000);
+	if (ctrl->link_active_reporting) {
+		pcie_wait_link_active(ctrl);
+		pcie_wait_link_training_done(ctrl);
+	} else
+		msleep(1000);
 
 	retval = pciehp_readw(ctrl, PCI_EXP_LNKSTA, &lnk_status);
 	if (retval) {
#1 lore raw
Quoted 4 lines · Yinghai Lu

Found one PCI Express Modules has link training error after hotplug. It turns out that after DLLLA is set, LT is still set for a while. So pciehp will delcare that hotplug fail in 1s.

I think DLLLA bit reads 1b means LT is completed. So I don't know why LT is still set on your platform.

Quoted 9 lines

HW guys say that pciehp is against PCI-e SPEC:
From PCI Express Base Specification Revision 2.1, Section 6.7.3.3:
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 (see section 6.6).

Try to wait for long enough by adding LT checking in 1s.

The pciehp driver already have this 1 second wait in board_added(). So I still don't understand what in pciehp is against PCIe spec clearly. Can you explain more about this?

What about the patch below? I think it's much simpler and has less impact.

Regards,
Kenji Kaneshige

---
 drivers/pci/hotplug/pciehp_ctrl.c |    3 ---
 drivers/pci/hotplug/pciehp_hpc.c  |   14 ++++++++++++++
 2 files changed, 14 insertions(+), 3 deletions(-)
drivers/pci/hotplug/pciehp_ctrl.c +0 -3
Index: linux-3.1/drivers/pci/hotplug/pciehp_ctrl.c
===================================================================
@@ -213,9 +213,6 @@ static int board_added(struct slot *p_sl
 		goto err_exit;
 	}
 
-	/* Wait for 1 second after checking link training status */
-	msleep(1000);
-
 	/* Check for a power fault */
 	if (ctrl->power_fault_detected || pciehp_query_power_fault(p_slot)) {
 		ctrl_err(ctrl, "Power fault on slot %s\n", slot_name(p_slot));
drivers/pci/hotplug/pciehp_hpc.c +14 -0
Index: linux-3.1/drivers/pci/hotplug/pciehp_hpc.c
===================================================================
@@ -280,6 +280,20 @@ int pciehp_check_link_status(struct cont
         else
                 msleep(1000);
 
+	/*
+	 * Need to wait for 1 second after the Data Link Layer Link
+	 * Active bit reads 1b before sending configuration request.
+	 * We also need additional 100 ms wait if the downstream port
+	 * supports Link speeds greater than 5.0 GT/s. We place this
+	 * wait before checking Link Training bit because Link
+	 * Training bit still is set even after Data Link Layer Link
+	 * Active bit is set on some platforms.
+	 */
+	if (ctrl->pcie->port->subordinate->max_bus_speed > PCIE_SPEED_5_0GT)
+		msleep(1100);
+	else
+		msleep(1000);
+
 	retval = pciehp_readw(ctrl, PCI_EXP_LNKSTA, &lnk_status);
 	if (retval) {
 		ctrl_err(ctrl, "Cannot read LNKSTATUS register\n");
author
#2 lore raw
Quoted 8 lines · Kenji Kaneshige
Quoted 4 lines · Yinghai Lu

Found one PCI Express Modules has link training error after hotplug. It turns out that after DLLLA is set, LT is still set for a while. So pciehp will delcare that hotplug fail in 1s.

I think DLLLA bit reads 1b means LT is completed. So I don't know why LT is still set on your platform.

recovery training ?

Quoted 9 lines
Quoted 9 lines

HW guys say that pciehp is against PCI-e SPEC:
From PCI Express Base Specification Revision 2.1, Section 6.7.3.3:
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 (see section 6.6).

Quoted 5 lines
Quoted 1 line

Try to wait for long enough by adding LT checking in 1s.

The pciehp driver already have this 1 second wait in board_added(). So I still don't understand what in pciehp is against PCIe spec clearly. Can you explain more about this?

should be fail in 1s after DLLLA, is set.

even we should not check LT, and should try pci conf reading to new added device.

Quoted 2 lines

What about the patch below? I think it's much simpler and has less impact.

Should work. will try it today.

Thanks

Yinghai Lu

author
#3 lore raw
Quoted 5 lines · Yinghai Lu
Quoted 2 lines · Kenji Kaneshige

What about the patch below? I think it's much simpler and has less impact.

Should work. will try it today.

yes, that works too.

Thanks

Yinghai

#4 lore raw
Quoted 6 lines · Yinghai Lu
Quoted 4 lines · Yinghai Lu
Quoted 2 lines · Kenji Kaneshige

What about the patch below? I think it's much simpler and has less impact.

Quoted 9 lines
Quoted 1 line

Should work. will try it today.

yes, that works too.

Thanks

Yinghai

I updated the patch with description. I split it into two and a logic is a little changed. Could you check these?

Regards,
Kenji Kaneshige

#5 lore raw

PATCH 1/2 pciehp: wait 1000 ms before Link Training check

We need to wait for 1000 ms after Data Link Layer Link Active (DLLLA) bit reads 1b before sending configuration request. Currently pciehp does this wait after checking Link Training (LT) bit. But we need it before checking LT bit because LT is still set even after DLLLA bit is set on some platforms.

Signed-off-by Kenji Kaneshige <[email protected]>
---
 drivers/pci/hotplug/pciehp_ctrl.c |    3 ---
 drivers/pci/hotplug/pciehp_hpc.c  |    8 ++++++++
 2 files changed, 8 insertions(+), 3 deletions(-)
drivers/pci/hotplug/pciehp_ctrl.c +0 -3
Index: linux-3.1/drivers/pci/hotplug/pciehp_ctrl.c
===================================================================
@@ -213,9 +213,6 @@ static int board_added(struct slot *p_sl
 		goto err_exit;
 	}
 
-	/* Wait for 1 second after checking link training status */
-	msleep(1000);
-
 	/* Check for a power fault */
 	if (ctrl->power_fault_detected || pciehp_query_power_fault(p_slot)) {
 		ctrl_err(ctrl, "Power fault on slot %s\n", slot_name(p_slot));
drivers/pci/hotplug/pciehp_hpc.c +8 -0
Index: linux-3.1/drivers/pci/hotplug/pciehp_hpc.c
===================================================================
@@ -280,6 +280,14 @@ 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);
+
 	retval = pciehp_readw(ctrl, PCI_EXP_LNKSTA, &lnk_status);
 	if (retval) {
 		ctrl_err(ctrl, "Cannot read LNKSTATUS register\n");
#7 lore raw

PATCH 2/2 pciehp: wait 100 ms after Link Training check

In reply to Kenji Kaneshige

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.

Signed-off-by Kenji Kaneshige <[email protected]>
---
 drivers/pci/hotplug/pciehp_hpc.c |    8 ++++++++
 1 file changed, 8 insertions(+)
drivers/pci/hotplug/pciehp_hpc.c +8 -0
Index: linux-3.1/drivers/pci/hotplug/pciehp_hpc.c
===================================================================
@@ -302,6 +302,14 @@ 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;
author Acked-by Tested-by on v1 1/2
#6 lore raw

Re: [PATCH 1/2] pciehp: wait 1000 ms before Link Training check

In reply to Kenji Kaneshige
Quoted 7 lines · Kenji Kaneshige

We need to wait for 1000 ms after Data Link Layer Link Active (DLLLA) bit reads 1b before sending configuration request. Currently pciehp does this wait after checking Link Training (LT) bit. But we need it before checking LT bit because LT is still set even after DLLLA bit is set on some platforms.

Signed-off-by Kenji Kaneshige <[email protected]>
Acked-by Yinghai Lu <[email protected]>
Tested-by Yinghai Lu <[email protected]>
Applied
#9 lore raw

Re: [PATCH 2/2] pciehp: wait 100 ms after Link Training check

Quoted 6 lines · On Thu, 10 Nov 2011 11:10:56 -0800 Yinghai Lu <[email protected]> wrote:
Quoted 4 lines · Kenji Kaneshige

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.

Quoted 6 lines
Quoted 1 line
Signed-off-by Kenji Kaneshige <[email protected]>
Acked-by Yinghai Lu <[email protected]>
Tested-by Yinghai Lu <[email protected]>

Applied these two to my for-linus branch, thanks guys.

Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
author
#10 lore raw

Re: [PATCH 2/2] pciehp: wait 100 ms after Link Training check

Quoted 9 lines · Jesse Barnes
Quoted 6 lines · On Thu, 10 Nov 2011 11:10:56 -0800 Yinghai Lu <[email protected]> wrote:
Quoted 4 lines · Kenji Kaneshige

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.

Quoted 9 lines
Quoted 6 lines
Quoted 1 line
Signed-off-by Kenji Kaneshige <[email protected]>
Acked-by Yinghai Lu <[email protected]>
Tested-by Yinghai Lu <[email protected]>

Applied these two to my for-linus branch, thanks guys.

Thanks, We may need to put them into stable too.

Yinghai

#11 lore raw

Re: [PATCH 2/2] pciehp: wait 100 ms after Link Training check

Quoted 15 lines · Yinghai Lu
Quoted 13 lines · Jesse Barnes
Quoted 10 lines · On Thu, 10 Nov 2011 11:10:56 -0800 Yinghai Lu <[email protected]> wrote:
Quoted 5 lines · Kenji Kaneshige

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.

Signed-off-by Kenji Kaneshige <[email protected]>
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Acked-by Yinghai Lu <[email protected]>
Tested-by Yinghai Lu <[email protected]>

Applied these two to my for-linus branch, thanks guys.

Thanks, We may need to put them into stable too.

Then send [email protected] the git commit ids once they show up in Linus's tree.

thanks,

greg k-h

#13 lore raw

Re: pciehp for stable

In reply to a message not in the archive
Quoted 15 lines · Yinghai Lu
Quoted 8 lines · Greg KH
Quoted 1 line

Thanks, We may need to put them into stable too.

Then send [email protected] the git commit ids once they show up in Linus's tree.

From 0027cb3e1947d0f453fece40ed16764fb362bac6 Mon Sep 17 00:00:00 2001

From: Kenji Kaneshige <[email protected]>
Date: Thu, 10 Nov 2011 16:40:37 +0900
Subject: [PATCH] PCI: pciehp: wait 1000 ms before Link Training check

Quoted 4 lines
Quoted 1 line

From b3c004542229099e18198061c737e13eafc8d4d6 Mon Sep 17 00:00:00 2001

From: Kenji Kaneshige <[email protected]>
Date: Thu, 10 Nov 2011 16:42:16 +0900
Subject: [PATCH] PCI: pciehp: wait 100 ms after Link Training check

Are you sure these patches are correct? Linus complained about them, and I will to :)

Please get Jesse to ack them and I'll apply them.

thanks,

greg k-h

#14 lore raw

Re: pciehp for stable

Quoted 17 lines · Greg KH
Quoted 16 lines · Yinghai Lu
Quoted 8 lines · Greg KH
Quoted 1 line

Thanks, We may need to put them into stable too.

Then send [email protected] the git commit ids once they show up in Linus's tree.

From 0027cb3e1947d0f453fece40ed16764fb362bac6 Mon Sep 17 00:00:00 2001

From: Kenji Kaneshige <[email protected]>
Date: Thu, 10 Nov 2011 16:40:37 +0900
Subject: [PATCH] PCI: pciehp: wait 1000 ms before Link Training check

Quoted 1 line

From b3c004542229099e18198061c737e13eafc8d4d6 Mon Sep 17 00:00:00 2001

Quoted 8 lines
Quoted 3 lines

From: Kenji Kaneshige <[email protected]>
Date: Thu, 10 Nov 2011 16:42:16 +0900
Subject: [PATCH] PCI: pciehp: wait 100 ms after Link Training check

Are you sure these patches are correct? Linus complained about them, and I will to :)

Please get Jesse to ack them and I'll apply them.

As I have gotten no reply in the week since I sent this, I'm dropping this request from my queue, sorry.

greg k-h