[PATCH] pciehp: only wait command complete for really hotplug control

[PATCH] pciehp: only wait command complete for really hotplug control

[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

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 <yinghai@kernel.org>

---
 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
===================================================================
--- linux-2.6.orig/drivers/pci/hotplug/pciehp_hpc.c
+++ linux-2.6/drivers/pci/hotplug/pciehp_hpc.c
@@ -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
===================================================================
--- linux-2.6.orig/include/uapi/linux/pci_regs.h
+++ linux-2.6/include/uapi/linux/pci_regs.h
@@ -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 */

Re: [PATCH] pciehp: only wait command complete for really hotplug control

[Bjorn Helgaas]

[+cc Rajat]

On Mon, Feb 24, 2014 at 4:59 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> 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

> 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 <yinghai@kernel.org>
>
> ---
>  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
> ===================================================================
> --- linux-2.6.orig/drivers/pci/hotplug/pciehp_hpc.c
> +++ linux-2.6/drivers/pci/hotplug/pciehp_hpc.c
> @@ -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
> ===================================================================
> --- linux-2.6.orig/include/uapi/linux/pci_regs.h
> +++ linux-2.6/include/uapi/linux/pci_regs.h
> @@ -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 */

Re: [PATCH] pciehp: only wait command complete for really hotplug control

[Yinghai Lu]

On Mon, Feb 24, 2014 at 4:46 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> [+cc Rajat]
>
> On Mon, Feb 24, 2014 at 4:59 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> 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.

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

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

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

looks like other way.

The issue still happen with Rajat's patch.

Thanks

Yinghai

Re: [PATCH] pciehp: only wait command complete for really hotplug control

[Bjorn Helgaas]

On Mon, Feb 24, 2014 at 6:55 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Mon, Feb 24, 2014 at 4:46 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> [+cc Rajat]
>>
>> On Mon, Feb 24, 2014 at 4:59 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>>> 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.
>
> others told me that other os does not check command complete
> when enabling notification.
>
>>
>> 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.

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."

Bjorn

Re: [PATCH] pciehp: only wait command complete for really hotplug control

[Rajat Jain]

On Mon, Feb 24, 2014 at 4:46 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> [+cc Rajat]
>
> On Mon, Feb 24, 2014 at 4:59 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> 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


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

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

>
> Bjorn
>
>> 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 <yinghai@kernel.org>
>>
>> ---
>>  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
>> ===================================================================
>> --- linux-2.6.orig/drivers/pci/hotplug/pciehp_hpc.c
>> +++ linux-2.6/drivers/pci/hotplug/pciehp_hpc.c
>> @@ -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
>> ===================================================================
>> --- linux-2.6.orig/include/uapi/linux/pci_regs.h
>> +++ linux-2.6/include/uapi/linux/pci_regs.h
>> @@ -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 */

Re: [PATCH] pciehp: only wait command complete for really hotplug control

[Rajat Jain]

On Tue, Feb 25, 2014 at 11:27 AM, Rajat Jain <rajatxjain@gmail.com> wrote:
> On Mon, Feb 24, 2014 at 4:46 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> [+cc Rajat]
>>
>> On Mon, Feb 24, 2014 at 4:59 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>>> 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
>
>
>>
>> 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?
>
> 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.

Forgot to add, the ideal fix for my systems would have been if the
"no_cmd_complete" could be set to 0 for my HW in the pcie_init(), but
alas, that's not true today:

        if (NO_CMD_CMPL(ctrl) ||
            !(POWER_CTRL(ctrl) | ATTN_LED(ctrl) | PWR_LED(ctrl) | EMI(ctrl)))
            ctrl->no_cmd_complete = 1;

So I use Bjorn's patch for this:
http://www.spinics.net/lists/hotplug/msg05830.html



>
> 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
>
>>
>> Bjorn
>>
>>> 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 <yinghai@kernel.org>
>>>
>>> ---
>>>  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
>>> ===================================================================
>>> --- linux-2.6.orig/drivers/pci/hotplug/pciehp_hpc.c
>>> +++ linux-2.6/drivers/pci/hotplug/pciehp_hpc.c
>>> @@ -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
>>> ===================================================================
>>> --- linux-2.6.orig/include/uapi/linux/pci_regs.h
>>> +++ linux-2.6/include/uapi/linux/pci_regs.h
>>> @@ -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 */

Re: [PATCH] pciehp: only wait command complete for really hotplug control

[Yinghai Lu]

On Tue, Feb 25, 2014 at 11:27 AM, Rajat Jain <rajatxjain@gmail.com> wrote:
> On Mon, Feb 24, 2014 at 4:46 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> [+cc Rajat]
>>
>> On Mon, Feb 24, 2014 at 4:59 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>>> 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-

Looks like different vendors have different understanding of that part.

so may need to add vendor quirk checking for waiting..., aka only wait for IDT
for all bits, and other vendor will only wait for 4 bits.

Thanks

Yinghai

Re: [PATCH] pciehp: only wait command complete for really hotplug control

[Yinghai Lu]

On Tue, Feb 25, 2014 at 9:45 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Mon, Feb 24, 2014 at 6:55 PM, Yinghai Lu <yinghai@kernel.org> 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.

Thanks

Yinghai

Re: [PATCH] pciehp: only wait command complete for really hotplug control

[Bjorn Helgaas]

On Wed, Mar 19, 2014 at 12:18 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Tue, Feb 25, 2014 at 9:45 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Mon, Feb 24, 2014 at 6:55 PM, Yinghai Lu <yinghai@kernel.org> 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.

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.

But as long as we have the current synchronous design, I think we have
to do *something* so we handle both the Intel/AMD/Nvidia-style chips
and the IDT-style chips correctly.  I suspect that means an explicit
quirk, or maybe some sort of test, e.g., set one of the other bits
(not EIC, PCC, PIC, AIC) and see whether we get an interrupt.

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

Bjorn

Re: [PATCH] pciehp: only wait command complete for really hotplug control

[Yinghai Lu]

On Wed, Mar 19, 2014 at 3:01 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:

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

Others bits should be chipset internal set right away.
The 4 bits will need chipset to talk to external controller to do the
work, that take some time.

Thanks

Yinghai

Re: [PATCH] pciehp: only wait command complete for really hotplug control

[Yinghai Lu]

On Wed, Mar 19, 2014 at 3:01 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Wed, Mar 19, 2014 at 12:18 PM, Yinghai Lu <yinghai@kernel.org> 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.

I like this idea. Will give it a try.

Thanks

Yinghai

Re: [PATCH] pciehp: only wait command complete for really hotplug control

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 4313 bytes --]

On Wed, Mar 19, 2014 at 8:16 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Wed, Mar 19, 2014 at 3:01 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Wed, Mar 19, 2014 at 12:18 PM, Yinghai Lu <yinghai@kernel.org> 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.
>
> 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 <yinghai@kernel.org>

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

Index: linux-2.6/drivers/pci/hotplug/pciehp_hpc.c
===================================================================
--- linux-2.6.orig/drivers/pci/hotplug/pciehp_hpc.c
+++ linux-2.6/drivers/pci/hotplug/pciehp_hpc.c
@@ -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);
 }

[-- Attachment #2: pcie_wait_cmd_next.patch --]
[-- Type: text/x-patch, Size: 2993 bytes --]

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 <yinghai@kernel.org>

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

Index: linux-2.6/drivers/pci/hotplug/pciehp_hpc.c
===================================================================
--- linux-2.6.orig/drivers/pci/hotplug/pciehp_hpc.c
+++ linux-2.6/drivers/pci/hotplug/pciehp_hpc.c
@@ -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);
 }
 

Re: [PATCH] pciehp: only wait command complete for really hotplug control

[Rajat Jain]

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

On Wed, Mar 19, 2014 at 11:47 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Wed, Mar 19, 2014 at 8:16 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Wed, Mar 19, 2014 at 3:01 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>> On Wed, Mar 19, 2014 at 12:18 PM, Yinghai Lu <yinghai@kernel.org> 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.
>>
>> 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 <yinghai@kernel.org>
>
> ---
>  drivers/pci/hotplug/pciehp_hpc.c |   26 ++++++++++----------------
>  1 file changed, 10 insertions(+), 16 deletions(-)
>
> Index: linux-2.6/drivers/pci/hotplug/pciehp_hpc.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/hotplug/pciehp_hpc.c
> +++ linux-2.6/drivers/pci/hotplug/pciehp_hpc.c
> @@ -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);
>  }

Re: [PATCH] pciehp: only wait command complete for really hotplug control

[Rajat Jain]

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!
:-)

Thanks,

Rajat

On Thu, Mar 20, 2014 at 6:30 AM, Rajat Jain <rajatxjain@gmail.com> wrote:
> Thanks, I'll test it out on my systems today.
>
> On Wed, Mar 19, 2014 at 11:47 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Wed, Mar 19, 2014 at 8:16 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>>> On Wed, Mar 19, 2014 at 3:01 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>>> On Wed, Mar 19, 2014 at 12:18 PM, Yinghai Lu <yinghai@kernel.org> 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.
>>>
>>> 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 <yinghai@kernel.org>
>>
>> ---
>>  drivers/pci/hotplug/pciehp_hpc.c |   26 ++++++++++----------------
>>  1 file changed, 10 insertions(+), 16 deletions(-)
>>
>> Index: linux-2.6/drivers/pci/hotplug/pciehp_hpc.c
>> ===================================================================
>> --- linux-2.6.orig/drivers/pci/hotplug/pciehp_hpc.c
>> +++ linux-2.6/drivers/pci/hotplug/pciehp_hpc.c
>> @@ -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);
>>  }

Re: [PATCH] pciehp: only wait command complete for really hotplug control

[Yinghai Lu]

On Mon, Mar 24, 2014 at 2:54 PM, Rajat Jain <rajatxjain@gmail.com> wrote:
> 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!
> :-)

Great.

Please post the patch with your tested-by.

Thanks

Yinghai

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

[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

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.
     It also solve the problem on Rajat's sytem that have wrong inital CC

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Tested-by: Rajat Jain <rajatxjain@gmail.com>

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

Index: linux-2.6/drivers/pci/hotplug/pciehp_hpc.c
===================================================================
--- linux-2.6.orig/drivers/pci/hotplug/pciehp_hpc.c
+++ linux-2.6/drivers/pci/hotplug/pciehp_hpc.c
@@ -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);
 }
 

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

[Rajat Jain]

Just a note  :-)

> -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).

> Tested-by: Rajat Jain <rajatxjain@gmail.com>

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

Best Regards,

Rajat

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

[Yinghai Lu]

On Mon, Mar 24, 2014 at 4:19 PM, Rajat Jain <rajatjain@juniper.net> wrote:
> Just a note  :-)
>
>> -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).
>
>> Tested-by: Rajat Jain <rajatxjain@gmail.com>
>
> Yes, I tested and it seems to work fine on such systems too.

Thanks for inputs for change log changes.

Assume Bjorn will sort it out.

Thanks

Yinghai