pciehp: only wait command complete for really hotplug control

On system with 16 PCI express hotplug slots, customer complain every slot will report "Command not completed in 1000 msec" during initialization.

Intel says that we should only wait command complete only for
Electromechanical Interlock Control Power Controller Control
Power Indicator Control
Attention Indicator Control

System with AMD/Nvidia chipset have same problem.

So change to only wait when following bits get changed.
PCI_EXP_SLTCTL_EIC
PCI_EXP_SLTCTL_PCC
PCI_EXP_SLTCTL_PIC
PCI_EXP_SLTCTL_AIC

With that we could set 16 seconds during booting, later with 32 sockets system with 64 pcie hotplug slots we could save 64 seconds.

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

---
 drivers/pci/hotplug/pciehp_hpc.c |   11 ++++++-----
 include/uapi/linux/pci_regs.h    |    5 +++++
 2 files changed, 11 insertions(+), 5 deletions(-)

Index: linux-2.6/drivers/pci/hotplug/pciehp_hpc.c
===================================================================

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

@@ -152,8 +152,8 @@ static void pcie_wait_cmd(struct control
 static void pcie_write_cmd(struct controller *ctrl, u16 cmd, u16 mask)
 {
        struct pci_dev *pdev = ctrl_dev(ctrl);
+       u16 slot_ctrl, old_slot_ctrl;
        u16 slot_status;
-       u16 slot_ctrl;
 
        mutex_lock(&ctrl->ctrl_lock);
 

@@ -181,9 +181,8 @@ static void pcie_write_cmd(struct contro
                }
        }
 
-       pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &slot_ctrl);
-       slot_ctrl &= ~mask;
-       slot_ctrl |= (cmd & mask);
+       pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &old_slot_ctrl);
+       slot_ctrl = (old_slot_ctrl & ~mask) | (cmd & mask);
        ctrl->cmd_busy = 1;
        smp_mb();
        pcie_capability_write_word(pdev, PCI_EXP_SLTCTL, slot_ctrl);

@@ -191,7 +190,9 @@ static void pcie_write_cmd(struct contro
        /*
         * Wait for command completion.
         */
-       if (!ctrl->no_cmd_complete) {
+       if (!ctrl->no_cmd_complete &&
+           ((PCI_EXP_SLTCTL_WAIT_MASK & old_slot_ctrl) !=
+            (PCI_EXP_SLTCTL_WAIT_MASK & slot_ctrl))) {
                int poll = 0;
                /*
                 * if hotplug interrupt is not enabled or command

Index: linux-2.6/include/uapi/linux/pci_regs.h
===================================================================

include/uapi/linux/pci_regs.h +5 -0

@@ -535,6 +535,11 @@
 #define  PCI_EXP_SLTCTL_PWR_OFF        0x0400 /* Power Off */
 #define  PCI_EXP_SLTCTL_EIC    0x0800  /* Electromechanical Interlock Control */
 #define  PCI_EXP_SLTCTL_DLLSCE 0x1000  /* Data Link Layer State Changed Enable */
+/* only need to wait command complete for hpc related */
+#define  PCI_EXP_SLTCTL_WAIT_MASK (PCI_EXP_SLTCTL_EIC | \
+                                  PCI_EXP_SLTCTL_PCC | \
+                                  PCI_EXP_SLTCTL_PIC | \
+                                  PCI_EXP_SLTCTL_AIC)
 #define PCI_EXP_SLTSTA         26      /* Slot Status */
 #define  PCI_EXP_SLTSTA_ABP    0x0001  /* Attention Button Pressed */
 #define  PCI_EXP_SLTSTA_PFD    0x0002  /* Power Fault Detected */

[+cc Rajat]

Quoted 8 lines · Yinghai Lu

On system with 16 PCI express hotplug slots, customer complain every slot will report "Command not completed in 1000 msec" during initialization.

Intel says that we should only wait command complete only for
Electromechanical Interlock Control Power Controller Control
Power Indicator Control
Attention Indicator Control

Is there something in the spec that says this? I'm looking at section 6.7.3.2. It says "If command completed events are supported, then software must wait for a command to complete before issuing the next command." It's obvious that this is conditional on the "No Command Completed Support" bit, but I don't see anything about a connection with the other bits you mentioned.

Is this related to an erratum in the Intel/AMD/Nvidia chipsets? If so, we need at least a comment about that in the source, and preferably, some sort of quirk for these chipsets.

This is similar to the issue Rajat is working on with
http://patchwork.ozlabs.org/patch/322387/ . I haven't merged that yet, but I probably will. Is there any connection between that patch and this issue? Does this issue still happen even with Rajat's patch applied?

Quoted 12 lines · Bjorn Helgaas

[+cc Rajat]

Quoted 8 lines · Yinghai Lu

On system with 16 PCI express hotplug slots, customer complain every slot will report "Command not completed in 1000 msec" during initialization.

Intel says that we should only wait command complete only for
Electromechanical Interlock Control Power Controller Control
Power Indicator Control
Attention Indicator Control

Quoted 6 lines

Is there something in the spec that says this? I'm looking at section 6.7.3.2. It says "If command completed events are supported, then software must wait for a command to complete before issuing the next command." It's obvious that this is conditional on the "No Command Completed Support" bit, but I don't see anything about a connection with the other bits you mentioned.

others told me that other os does not check command complete when enabling notification.

Quoted 1 line

Quoted 3 lines

Is this related to an erratum in the Intel/AMD/Nvidia chipsets? If so, we need at least a comment about that in the source, and preferably, some sort of quirk for these chipsets.

Vendor does not agree that is silicon problem.
But will update EDS to state that CC will be only set when those four control bits are handled.

On Mon, Feb 24, 2014 at 6:55 PM, Yinghai Lu <[email protected]> wrote:

It's quite likely that I'm mistaken, and this is not a silicon problem, especially since two vendors apparently did it the same way. But I'd like to see the explanation from Intel about how this complies with the spec. I provided a spec reference for why I think we should wait for a command completed event. The next step would be for you or Intel to respond with "here's the section in the spec that you missed, and it says the OS should not wait for an event unless it is changing the EIC, PCC, PIC, or AIC bits."

Quoted 10 lines · Bjorn Helgaas

On Mon, Feb 24, 2014 at 6:55 PM, Yinghai Lu <[email protected]> wrote:

It's quite likely that I'm mistaken, and this is not a silicon problem, especially since two vendors apparently did it the same way. But I'd like to see the explanation from Intel about how this complies with the spec. I provided a spec reference for why I think we should wait for a command completed event. The next step would be for you or Intel to respond with "here's the section in the spec that you missed, and it says the OS should not wait for an event unless it is changing the EIC, PCC, PIC, or AIC bits."

Hi, Bjorn,

Intel admits that it is silicon problem.
CC will be set iff EIC || PCC || PIC || AIC is set.

so please me know if you want
1. just use the patch in current form. it will do that for ...
2. or modify the patch to check vendor to set flag for every controller, and use that to decide if need to wait CC.

BTW, other OS does not check CC for other than the four bits.

In any case, it needs to be clear why we expect CC for some bits but not others.

On Wed, Mar 19, 2014 at 12:18 PM, Yinghai Lu <[email protected]> wrote:

I think the current pciehp design is a bit broken. Today we perform commands very synchronously:

    pcie_write_cmd() {
        write Slot Control
        if (Command Complete supported) {
            wait for Command Complete
        }
    }

The typical driver pattern would be more asynchronous, like this:

    pcie_write_cmd() {
        read Slot Status
        if (!Command Completed) {
            wait for Command Complete
        }
        write Slot Control
    }

With the asynchronous pattern, we would probably never wait at all unless we performed two commands in immediate succession. I wouldn't be surprised if doing this asynchronously would effectively cover up these hardware differences.

Quoted 22 lines · Bjorn Helgaas

On Wed, Mar 19, 2014 at 12:18 PM, Yinghai Lu <[email protected]> wrote:

I think the current pciehp design is a bit broken. Today we perform commands very synchronously:

    pcie_write_cmd() {
        write Slot Control
        if (Command Complete supported) {
            wait for Command Complete
        }
    }

The typical driver pattern would be more asynchronous, like this:

    pcie_write_cmd() {
        read Slot Status
        if (!Command Completed) {
            wait for Command Complete
        }
        write Slot Control
    }

Quoted 4 lines

With the asynchronous pattern, we would probably never wait at all unless we performed two commands in immediate succession. I wouldn't be surprised if doing this asynchronously would effectively cover up these hardware differences.

I like this idea. Will give it a try.

Quoted 23 lines · Yinghai Lu

Quoted 22 lines · Bjorn Helgaas

On Wed, Mar 19, 2014 at 12:18 PM, Yinghai Lu <[email protected]> wrote:

I think the current pciehp design is a bit broken. Today we perform commands very synchronously:

    pcie_write_cmd() {
        write Slot Control
        if (Command Complete supported) {
            wait for Command Complete
        }
    }

The typical driver pattern would be more asynchronous, like this:

    pcie_write_cmd() {
        read Slot Status
        if (!Command Completed) {
            wait for Command Complete
        }
        write Slot Control
    }

Quoted 6 lines

Quoted 4 lines

With the asynchronous pattern, we would probably never wait at all unless we performed two commands in immediate succession. I wouldn't be surprised if doing this asynchronously would effectively cover up these hardware differences.

I like this idea. Will give it a try.

It works. Please check the patch that check CC before wait/write command.

Subject: [PATCH v2] pciehp: only wait command complete for real hotplug control

On system with 16 PCI express hotplug slots, customer complain every slot will report "Command not completed in 1000 msec" during initialization.

Intel says that we should only wait command complete only for
Electromechanical Interlock Control Power Controller Control
Power Indicator Control
Attention Indicator Control

System with AMD/Nvidia chipset have same problem.

Two ways to address the problem:
1. change to only wait when (EIC, PCC, PIC, AIC) bits get changed.
2. or check CMD_COMPLETE and wait before write command. Only wait when CC is set and cmd_busy is true. For chipset from intel will only have CC set for real hotplug control, so we could skip the wait for others.

This patch is using second way.

With that we could save 16 seconds during booting, later with 32 sockets system with 64 pcie hotplug slots we could save 64 seconds.

-v2: use second way that is suggested by Bjorn.

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

---
 drivers/pci/hotplug/pciehp_hpc.c |   26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

Index: linux-2.6/drivers/pci/hotplug/pciehp_hpc.c
===================================================================

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

@@ -158,15 +158,8 @@ static void pcie_write_cmd(struct contro
     mutex_lock(&ctrl->ctrl_lock);
 
     pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
-    if (slot_status & 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)) {
+    if (ctrl->no_cmd_complete && slot_status & PCI_EXP_SLTSTA_CC) {
+        if (!NO_CMD_CMPL(ctrl)) {
             /*
              * This controller seems to notify of command completed
              * event even though it supports none of power

@@ -182,16 +175,11 @@ static void pcie_write_cmd(struct contro
     }
 
     pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &slot_ctrl);
-    slot_ctrl &= ~mask;
-    slot_ctrl |= (cmd & mask);
-    ctrl->cmd_busy = 1;
-    smp_mb();
-    pcie_capability_write_word(pdev, PCI_EXP_SLTCTL, slot_ctrl);
-
     /*
      * Wait for command completion.
      */
-    if (!ctrl->no_cmd_complete) {
+    if (!ctrl->no_cmd_complete && (slot_status & PCI_EXP_SLTSTA_CC) &&
+        ctrl->cmd_busy) {
         int poll = 0;
         /*
          * if hotplug interrupt is not enabled or command

@@ -203,6 +191,12 @@ static void pcie_write_cmd(struct contro
             poll = 1;
                 pcie_wait_cmd(ctrl, poll);
     }
+
+    slot_ctrl &= ~mask;
+    slot_ctrl |= (cmd & mask);
+    ctrl->cmd_busy = 1;
+    smp_mb();
+    pcie_capability_write_word(pdev, PCI_EXP_SLTCTL, slot_ctrl);
     mutex_unlock(&ctrl->ctrl_lock);
 }

Thanks, I'll test it out on my systems today.

Quoted 119 lines · Yinghai Lu

Quoted 23 lines · Yinghai Lu

Quoted 22 lines · Bjorn Helgaas

On Wed, Mar 19, 2014 at 12:18 PM, Yinghai Lu <[email protected]> wrote:

I think the current pciehp design is a bit broken. Today we perform commands very synchronously:

    pcie_write_cmd() {
        write Slot Control
        if (Command Complete supported) {
            wait for Command Complete
        }
    }

The typical driver pattern would be more asynchronous, like this:

    pcie_write_cmd() {
        read Slot Status
        if (!Command Completed) {
            wait for Command Complete
        }
        write Slot Control
    }

Quoted 6 lines

Quoted 4 lines

With the asynchronous pattern, we would probably never wait at all unless we performed two commands in immediate succession. I wouldn't be surprised if doing this asynchronously would effectively cover up these hardware differences.

I like this idea. Will give it a try.

It works. Please check the patch that check CC before wait/write command.

Subject: [PATCH v2] pciehp: only wait command complete for real hotplug control

On system with 16 PCI express hotplug slots, customer complain every slot will report "Command not completed in 1000 msec" during initialization.

Intel says that we should only wait command complete only for
Electromechanical Interlock Control Power Controller Control
Power Indicator Control
Attention Indicator Control

System with AMD/Nvidia chipset have same problem.

Two ways to address the problem:
1. change to only wait when (EIC, PCC, PIC, AIC) bits get changed.
2. or check CMD_COMPLETE and wait before write command. Only wait when CC is set and cmd_busy is true. For chipset from intel will only have CC set for real hotplug control, so we could skip the wait for others.

This patch is using second way.

With that we could save 16 seconds during booting, later with 32 sockets system with 64 pcie hotplug slots we could save 64 seconds.

-v2: use second way that is suggested by Bjorn.

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

---
 drivers/pci/hotplug/pciehp_hpc.c |   26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

Index: linux-2.6/drivers/pci/hotplug/pciehp_hpc.c
===================================================================

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

@@ -158,15 +158,8 @@ static void pcie_write_cmd(struct contro
     mutex_lock(&ctrl->ctrl_lock);
 
     pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
-    if (slot_status & 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)) {
+    if (ctrl->no_cmd_complete && slot_status & PCI_EXP_SLTSTA_CC) {
+        if (!NO_CMD_CMPL(ctrl)) {
             /*
              * This controller seems to notify of command completed
              * event even though it supports none of power

@@ -182,16 +175,11 @@ static void pcie_write_cmd(struct contro
     }
 
     pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &slot_ctrl);
-    slot_ctrl &= ~mask;
-    slot_ctrl |= (cmd & mask);
-    ctrl->cmd_busy = 1;
-    smp_mb();
-    pcie_capability_write_word(pdev, PCI_EXP_SLTCTL, slot_ctrl);
-
     /*
      * Wait for command completion.
      */
-    if (!ctrl->no_cmd_complete) {
+    if (!ctrl->no_cmd_complete && (slot_status & PCI_EXP_SLTSTA_CC) &&
+        ctrl->cmd_busy) {
         int poll = 0;
         /*
          * if hotplug interrupt is not enabled or command

@@ -203,6 +191,12 @@ static void pcie_write_cmd(struct contro
             poll = 1;
                 pcie_wait_cmd(ctrl, poll);
     }
+
+    slot_ctrl &= ~mask;
+    slot_ctrl |= (cmd & mask);
+    ctrl->cmd_busy = 1;
+    smp_mb();
+    pcie_capability_write_word(pdev, PCI_EXP_SLTCTL, slot_ctrl);
     mutex_unlock(&ctrl->ctrl_lock);
 }

Hello,

I tested this today, and it works fine. Apparently It even solves the problem my systems were facing, thus I do not need my patch anymore! :-)

-v2: use second way that is suggested by Bjorn.
It also solve the problem on Rajat's sytem that have wrong inital CC

Tested-by Rajat Jain <[email protected]>

Just a note :-)

Quoted 2 lines

-v2: use second way that is suggested by Bjorn.
It also solve the problem on Rajat's sytem that have wrong inital CC

Er, not wrong initial CC.

Just a different (but not necessarily wrong) different CC bit behavior :-). In my systems, the CC bit gets set for every write to the slot control register (even if Button, LED, EMI, power controller are not implemented).

Quoted 1 line

Tested-by Rajat Jain <[email protected]>

Yes, I tested and it seems to work fine on such systems too.

[+cc Rajat]

Quoted 8 lines · Yinghai Lu

On system with 16 PCI express hotplug slots, customer complain every slot will report "Command not completed in 1000 msec" during initialization.

Intel says that we should only wait command complete only for
Electromechanical Interlock Control Power Controller Control
Power Indicator Control
Attention Indicator Control

Is there something in the spec that says this? I'm looking at section 6.7.3.2. It says "If command completed events are supported, then software must wait for a command to complete before issuing the next command." It's obvious that this is conditional on the "No Command Completed Support" bit, but I don't see anything about a connection with the other bits you mentioned.

Is this related to an erratum in the Intel/AMD/Nvidia chipsets? If so, we need at least a comment about that in the source, and preferably, some sort of quirk for these chipsets.

Otherwise, we're likely to break this in the future when somebody reading the code says "this doesn't correspond with what the spec says." In case we *do* break this in the future, I'd like to have a bugzilla that contains the dmesg showing the problem and "lspci -vv" so we have a hope of figuring out what systems this affects.

This is similar to the issue Rajat is working on with
http://patchwork.ozlabs.org/patch/322387/ . I haven't merged that yet, but I probably will. Is there any connection between that patch and this issue? Does this issue still happen even with Rajat's patch applied?

Bjorn

Quoted 71 lines

System with AMD/Nvidia chipset have same problem.

So change to only wait when following bits get changed.
PCI_EXP_SLTCTL_EIC
PCI_EXP_SLTCTL_PCC
PCI_EXP_SLTCTL_PIC
PCI_EXP_SLTCTL_AIC

With that we could set 16 seconds during booting, later with 32 sockets system with 64 pcie hotplug slots we could save 64 seconds.

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

---
 drivers/pci/hotplug/pciehp_hpc.c |   11 ++++++-----
 include/uapi/linux/pci_regs.h    |    5 +++++
 2 files changed, 11 insertions(+), 5 deletions(-)

Index: linux-2.6/drivers/pci/hotplug/pciehp_hpc.c
===================================================================

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

@@ -152,8 +152,8 @@ static void pcie_wait_cmd(struct control
 static void pcie_write_cmd(struct controller *ctrl, u16 cmd, u16 mask)
 {
        struct pci_dev *pdev = ctrl_dev(ctrl);
+       u16 slot_ctrl, old_slot_ctrl;
        u16 slot_status;
-       u16 slot_ctrl;
 
        mutex_lock(&ctrl->ctrl_lock);
 

@@ -181,9 +181,8 @@ static void pcie_write_cmd(struct contro
                }
        }
 
-       pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &slot_ctrl);
-       slot_ctrl &= ~mask;
-       slot_ctrl |= (cmd & mask);
+       pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &old_slot_ctrl);
+       slot_ctrl = (old_slot_ctrl & ~mask) | (cmd & mask);
        ctrl->cmd_busy = 1;
        smp_mb();
        pcie_capability_write_word(pdev, PCI_EXP_SLTCTL, slot_ctrl);

@@ -191,7 +190,9 @@ static void pcie_write_cmd(struct contro
        /*
         * Wait for command completion.
         */
-       if (!ctrl->no_cmd_complete) {
+       if (!ctrl->no_cmd_complete &&
+           ((PCI_EXP_SLTCTL_WAIT_MASK & old_slot_ctrl) !=
+            (PCI_EXP_SLTCTL_WAIT_MASK & slot_ctrl))) {
                int poll = 0;
                /*
                 * if hotplug interrupt is not enabled or command

Index: linux-2.6/include/uapi/linux/pci_regs.h
===================================================================

include/uapi/linux/pci_regs.h +5 -0

@@ -535,6 +535,11 @@
 #define  PCI_EXP_SLTCTL_PWR_OFF        0x0400 /* Power Off */
 #define  PCI_EXP_SLTCTL_EIC    0x0800  /* Electromechanical Interlock Control */
 #define  PCI_EXP_SLTCTL_DLLSCE 0x1000  /* Data Link Layer State Changed Enable */
+/* only need to wait command complete for hpc related */
+#define  PCI_EXP_SLTCTL_WAIT_MASK (PCI_EXP_SLTCTL_EIC | \
+                                  PCI_EXP_SLTCTL_PCC | \
+                                  PCI_EXP_SLTCTL_PIC | \
+                                  PCI_EXP_SLTCTL_AIC)
 #define PCI_EXP_SLTSTA         26      /* Slot Status */
 #define  PCI_EXP_SLTSTA_ABP    0x0001  /* Attention Button Pressed */
 #define  PCI_EXP_SLTSTA_PFD    0x0002  /* Power Fault Detected */

Quoted 11 lines · Bjorn Helgaas

[+cc Rajat]

Quoted 8 lines · Yinghai Lu

On system with 16 PCI express hotplug slots, customer complain every slot will report "Command not completed in 1000 msec" during initialization.

Intel says that we should only wait command complete only for
Electromechanical Interlock Control Power Controller Control
Power Indicator Control
Attention Indicator Control

Hello,

I have a HW that generates Command Completion notifications, even though it does not implement any of the EMI, Power Controller, Power Indicator, Attention indicator. Although I'm not sure whether or not this behavior is compliant to PCIe spec, just wanted to pitch in to convey that there are HW which have this behavior:

PCI bridge: Integrated Device Technology, Inc. Device 807a (rev 02) (prog-if 00 [Normal decode])
Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
Latency: 0
Bus: primary=02, secondary=30, subordinate=3f, sec-latency=0
I/O behind bridge: 00009000-00009fff
Memory behind bridge: 88000000-8bffffff
Prefetchable memory behind bridge: 00000000b1000000-00000000b11fffff Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- <SERR- <PERR-
BridgeCtl: Parity- SERR- NoISA- VGA- MAbort- >Reset- FastB2B-
PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
Capabilities: [40] Express (v2) Downstream Port (Slot+), MSI 00
DevCap: MaxPayload 2048 bytes, PhantFunc 0, Latency L0s <64ns, L1 <1us
ExtTag+ RBE+ FLReset-
DevCtl: Report errors: Correctable- Non-Fatal- Fatal- Unsupported-
RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop- MaxPayload 128 bytes, MaxReadReq 128 bytes
DevSta: CorrErr+ UncorrErr- FatalErr+ UnsuppReq+ AuxPwr- TransPend-
LnkCap: Port #13, Speed 5GT/s, Width x4, ASPM L0s L1, Latency
L0 <4us, L1 <4us
ClockPM- Surprise+ LLActRep+ BwNot+
LnkCtl: ASPM Disabled; Disabled- Retrain- CommClk-
ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
LnkSta: Speed 2.5GT/s, Width x4, TrErr- Train- SlotClk-
DLActive+ BWMgmt- ABWMgmt-
SltCap: AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise-
Slot #13, PowerLimit 0.000W; Interlock- NoCompl-
SltCtl: Enable: AttnBtn- PwrFlt- MRL- PresDet+ CmdCplt+ HPIrq+ LinkChg+
Control: AttnInd Unknown, PwrInd Unknown, Power- Interlock-
SltSta: Status: AttnBtn- PowerFlt- MRL- CmdCplt- PresDet+ Interlock-
Changed: MRL- PresDet- LinkState-
DevCap2: Completion Timeout: Not Supported, TimeoutDis-, LTR-,
OBFF Not Supported ARIFwd+
DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis-, LTR-,
OBFF Disabled ARIFwd-
LnkCtl2: Target Link Speed: 5GT/s, EnterCompliance- SpeedDis-,
Selectable De-emphasis: -6dB
Transmit Margin: Normal Operating Range,
EnterModifiedCompliance- ComplianceSOS-
Compliance De-emphasis: -6dB
LnkSta2: Current De-emphasis Level: -6dB,
EqualizationComplete-, EqualizationPhase1-
EqualizationPhase2-, EqualizationPhase3-,
LinkEqualizationRequest-
Capabilities: [c0] Power Management version 3
Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA
PME(D0+,D1-,D2-,D3hot+,D3cold+)
Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
Capabilities: [d0] MSI: Enable+ Count=1/1 Maskable- 64bit+

        Address: 00000000fff41740  Data: 0008

Capabilities: [100 v1] Advanced Error Reporting
UESta: DLP- SDES+ TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt-
RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
UEMsk: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt-
RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
UESvrt: DLP+ SDES+ TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt-
RxOF+ MalfTLP+ ECRC- UnsupReq- ACSViol-
CESta: RxErr+ BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr+
CEMsk: RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr+
AERCap: First Error Pointer: 05, GenCap+ CGenEn- ChkCap+ ChkEn-
Capabilities: [200 v1] Virtual Channel

        Caps:   LPEVC=0 RefClk=100ns PATEntryBits=4
        Arb:    Fixed- WRR32- WRR64- WRR128-
        Ctrl:   ArbSelect=Fixed

Status: InProgress-
VC0: Caps: PATOffset=04 MaxTimeSlots=1 RejSnoopTrans-
Arb: Fixed+ WRR32- WRR64- WRR128- TWRR128- WRR256-
Ctrl: Enable+ ID=0 ArbSelect=Fixed TC/VC=ff
Status: NegoPending- InProgress-
Port Arbitration Table <?>
Capabilities: [320 v1] Access Control Services
ACSCap: SrcValid+ TransBlk+ ReqRedir+ CmpltRedir+ UpstreamFwd+
EgressCtrl+ DirectTrans+
ACSCtl: SrcValid- TransBlk- ReqRedir- CmpltRedir- UpstreamFwd-
EgressCtrl- DirectTrans-
Capabilities: [330 v1] #12
Kernel driver in use: pcieport

Quoted 18 lines

Is there something in the spec that says this? I'm looking at section 6.7.3.2. It says "If command completed events are supported, then software must wait for a command to complete before issuing the next command." It's obvious that this is conditional on the "No Command Completed Support" bit, but I don't see anything about a connection with the other bits you mentioned.

Is this related to an erratum in the Intel/AMD/Nvidia chipsets? If so, we need at least a comment about that in the source, and preferably, some sort of quirk for these chipsets.

Otherwise, we're likely to break this in the future when somebody reading the code says "this doesn't correspond with what the spec says." In case we *do* break this in the future, I'd like to have a bugzilla that contains the dmesg showing the problem and "lspci -vv" so we have a hope of figuring out what systems this affects.

Quoted 5 lines

This is similar to the issue Rajat is working on with
http://patchwork.ozlabs.org/patch/322387/ . I haven't merged that yet, but I probably will. Is there any connection between that patch and this issue? Does this issue still happen even with Rajat's patch applied?

Also, wanted to my mention that my patch applies equally to the systems that exhibit this behavior and the other systems (Intel etc) that do not exhibit this behavior. That is because if for any reason (e.g. spurious interrupt) the condition "if (slot_status & PCI_EXP_SLTSTA_CC)" becomes trues in pcie_write_cmd(), we do not clear this bit but expect "cmd completed" events to be generated in future. In other words my patch is independent of Yinghai's patch, and the fact it helps my systems is a (desired) side effect.

Lastly, it seems to me that if a platform does not support the 4 HP elements mentioned above, the only other reason to write to Slot_Ctrl register will be to enable and disable notifications (during init / suspend / resume / reset_slot etc). So I was wondering if it would help if we explicitly cleared the CC bit after we enable disable hotplug notifications? If such a change was there, then probably we'd be able to cater both kind of systems, and making sure that none sees a delay at boot up. But yes, I do agree that this is going to create more haphazardness in the system (the code flow is already difficult to follow, specially for "cmd completed")

Thanks,

Rajat

Quoted 74 lines

Bjorn

Quoted 71 lines

System with AMD/Nvidia chipset have same problem.

So change to only wait when following bits get changed.
PCI_EXP_SLTCTL_EIC
PCI_EXP_SLTCTL_PCC
PCI_EXP_SLTCTL_PIC
PCI_EXP_SLTCTL_AIC

With that we could set 16 seconds during booting, later with 32 sockets system with 64 pcie hotplug slots we could save 64 seconds.

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

---
 drivers/pci/hotplug/pciehp_hpc.c |   11 ++++++-----
 include/uapi/linux/pci_regs.h    |    5 +++++
 2 files changed, 11 insertions(+), 5 deletions(-)

Index: linux-2.6/drivers/pci/hotplug/pciehp_hpc.c
===================================================================

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

@@ -152,8 +152,8 @@ static void pcie_wait_cmd(struct control
 static void pcie_write_cmd(struct controller *ctrl, u16 cmd, u16 mask)
 {
        struct pci_dev *pdev = ctrl_dev(ctrl);
+       u16 slot_ctrl, old_slot_ctrl;
        u16 slot_status;
-       u16 slot_ctrl;
 
        mutex_lock(&ctrl->ctrl_lock);
 

@@ -181,9 +181,8 @@ static void pcie_write_cmd(struct contro
                }
        }
 
-       pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &slot_ctrl);
-       slot_ctrl &= ~mask;
-       slot_ctrl |= (cmd & mask);
+       pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &old_slot_ctrl);
+       slot_ctrl = (old_slot_ctrl & ~mask) | (cmd & mask);
        ctrl->cmd_busy = 1;
        smp_mb();
        pcie_capability_write_word(pdev, PCI_EXP_SLTCTL, slot_ctrl);

@@ -191,7 +190,9 @@ static void pcie_write_cmd(struct contro
        /*
         * Wait for command completion.
         */
-       if (!ctrl->no_cmd_complete) {
+       if (!ctrl->no_cmd_complete &&
+           ((PCI_EXP_SLTCTL_WAIT_MASK & old_slot_ctrl) !=
+            (PCI_EXP_SLTCTL_WAIT_MASK & slot_ctrl))) {
                int poll = 0;
                /*
                 * if hotplug interrupt is not enabled or command

Index: linux-2.6/include/uapi/linux/pci_regs.h
===================================================================

include/uapi/linux/pci_regs.h +5 -0

@@ -535,6 +535,11 @@
 #define  PCI_EXP_SLTCTL_PWR_OFF        0x0400 /* Power Off */
 #define  PCI_EXP_SLTCTL_EIC    0x0800  /* Electromechanical Interlock Control */
 #define  PCI_EXP_SLTCTL_DLLSCE 0x1000  /* Data Link Layer State Changed Enable */
+/* only need to wait command complete for hpc related */
+#define  PCI_EXP_SLTCTL_WAIT_MASK (PCI_EXP_SLTCTL_EIC | \
+                                  PCI_EXP_SLTCTL_PCC | \
+                                  PCI_EXP_SLTCTL_PIC | \
+                                  PCI_EXP_SLTCTL_AIC)
 #define PCI_EXP_SLTSTA         26      /* Slot Status */
 #define  PCI_EXP_SLTSTA_ABP    0x0001  /* Attention Button Pressed */
 #define  PCI_EXP_SLTSTA_PFD    0x0002  /* Power Fault Detected */

Quoted 11 lines · Bjorn Helgaas

[+cc Rajat]

Quoted 8 lines · Yinghai Lu

On system with 16 PCI express hotplug slots, customer complain every slot will report "Command not completed in 1000 msec" during initialization.

Intel says that we should only wait command complete only for
Electromechanical Interlock Control Power Controller Control
Power Indicator Control
Attention Indicator Control

Hello,

I have a HW that generates Command Completion notifications, even though it does not implement any of the EMI, Power Controller, Power Indicator, Attention indicator. Although I'm not sure whether or not this behavior is compliant to PCIe spec, just wanted to pitch in to convey that there are HW which have this behavior:

PCI bridge: Integrated Device Technology, Inc. Device 807a (rev 02) (prog-if 00 [Normal decode])
SltCap: AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise-
Slot #13, PowerLimit 0.000W; Interlock- NoCompl-
SltCtl: Enable: AttnBtn- PwrFlt- MRL- PresDet+ CmdCplt+ HPIrq+ LinkChg+
Control: AttnInd Unknown, PwrInd Unknown, Power- Interlock-
SltSta: Status: AttnBtn- PowerFlt- MRL- CmdCplt- PresDet+ Interlock-
Changed: MRL- PresDet- LinkState-