| Message ID | 52797238.8070304@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
On Tue, Nov 05, 2013 at 02:33:28PM -0800, Rajat Jain wrote: > In case of a spurious "cmd completed", pcie_write_cmd() does not > clear it, but yet expects more "cmd completed" events to be generated. > This does not happen because the previous (spurious) event has not > been acknowledged. Fix that. > > Signed-off-by: Rajat Jain <rajatjain@juniper.net> > Signed-off-by: Guenter Roeck <groeck@juniper.net> > --- > This is how I saw it in action: my controller does not implement any > hot-plug elements (LED, power ctrl, EMI etc) but still supports Command > completed bit. > - During initialization, > pcie_disable_notification() > -> pcie_write_cmd() > -> writes to Slot control register > -> which causes PCI_EXP_SLTSTA_CC to get set, which is not > cleared, because IRQ is not generated (we just disabled > notifications). > - After some time, > pcie_enable_notification() > -> pcie_write_cmd() > -> finds PCI_EXP_SLTSTA_CC is set, assumes it is spurious. > -> Does not clear it, yet expects more command completed > events to be generated (never happens). I'm not sure this "cmd completed" is actually spurious. Spec section 7.8.10 is very clear that any write to Slot Control must cause a hot-plug command to be generated (if the port is hot-plug capable). Can you collect "lspci -vv" output for your controller? I assume you're hitting this case in pcie_init() (added by 5808639bfa98 ("pciehp: fix slow probing")): /* * Controller doesn't notify of command completion if the "No * Command Completed Support" bit is set in Slot Capability * register or the controller supports none of power * controller, attention led, power led and EMI. */ if (NO_CMD_CMPL(ctrl) || !(POWER_CTRL(ctrl) | ATTN_LED(ctrl) | PWR_LED(ctrl) | EMI(ctrl))) ctrl->no_cmd_complete = 1; and we're setting "no_cmd_complete = 1" for your controller, which keeps us from waiting for completion in pcie_write_cmd(). I'm dubious about the assertion that a controller without power control, attention LED, power LED, or EMI can't support command completion. I don't see anything in the spec to that effect. Since you're seeing PCI_EXP_SLTSTA_CC=1, your controller *should* support Command Completion notification and PCI_EXP_SLTCAP_NCCS should be 0 (per Table 7-20), so I wonder what happens on your system if you change pcie_init() so it leaves "ctrl->no_cmd_complete = 0" instead? Does it work correctly then? I know we can't just drop the "!(POWER_CTRL(ctrl) | ...)" tests because we don't want to reintroduce the problem fixed by 5808639bfa98, but I wonder if we can find a better fix that addresses both problems. Bjorn > > drivers/pci/hotplug/pciehp_hpc.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c > index 5b8d749..ba8e06f 100644 > --- a/drivers/pci/hotplug/pciehp_hpc.c > +++ b/drivers/pci/hotplug/pciehp_hpc.c > @@ -185,6 +185,7 @@ static int pcie_write_cmd(struct controller *ctrl, u16 cmd, u16 mask) > } > > if (slot_status & PCI_EXP_SLTSTA_CC) { > + pciehp_writew(ctrl, PCI_EXP_SLTSTA, PCI_EXP_SLTSTA_CC); > if (!ctrl->no_cmd_complete) { > /* > * After 1 sec and CMD_COMPLETED still not set, just > -- > 1.7.9.5 > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Bjorn, On Tue, Nov 5, 2013 at 4:25 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > On Tue, Nov 05, 2013 at 02:33:28PM -0800, Rajat Jain wrote: >> In case of a spurious "cmd completed", pcie_write_cmd() does not >> clear it, but yet expects more "cmd completed" events to be generated. >> This does not happen because the previous (spurious) event has not >> been acknowledged. Fix that. >> >> Signed-off-by: Rajat Jain <rajatjain@juniper.net> >> Signed-off-by: Guenter Roeck <groeck@juniper.net> >> --- >> This is how I saw it in action: my controller does not implement any >> hot-plug elements (LED, power ctrl, EMI etc) but still supports Command >> completed bit. >> - During initialization, >> pcie_disable_notification() >> -> pcie_write_cmd() >> -> writes to Slot control register >> -> which causes PCI_EXP_SLTSTA_CC to get set, which is not >> cleared, because IRQ is not generated (we just disabled >> notifications). >> - After some time, >> pcie_enable_notification() >> -> pcie_write_cmd() >> -> finds PCI_EXP_SLTSTA_CC is set, assumes it is spurious. >> -> Does not clear it, yet expects more command completed >> events to be generated (never happens). > > I'm not sure this "cmd completed" is actually spurious. Spec section > 7.8.10 is very clear that any write to Slot Control must cause a > hot-plug command to be generated (if the port is hot-plug capable). I agree, and I think I am witnessing a "genuine" command completion (caused by disabling of notifications). > > Can you collect "lspci -vv" output for your controller? > Sure: 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=50, subordinate=5f, sec-latency=0 I/O behind bridge: 00003000-00003fff Memory behind bridge: 8c000000-8fffffff Prefetchable memory behind bridge: 00000000b0600000-00000000b07fffff 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 #3, 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 x0, TrErr- Train- SlotClk- DLActive- BWMgmt- ABWMgmt- SltCap: AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise- Slot #3, 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: 00000000ff041740 Data: 0003 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: 00, 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 In addition, here is what the pciehp driver spews out: Hotplug Controller: Seg/Bus/Dev/Func/IRQ : 0000:02:03.0 IRQ 21 Vendor ID : 0x111d Device ID : 0x807a Subsystem ID : 0x0000 Subsystem Vendor ID : 0x0000 PCIe Cap offset : 0x40 PCI resource [7] : [io 0x13000-0x13fff] PCI resource [8] : [mem 0xc0c000000-0xc0fffffff] PCI resource [9] : [mem 0xc30600000-0xc307fffff 64bit pref] Slot Capabilities : 0x00180040 Physical Slot Number : 3 Attention Button : no Power Controller : no MRL Sensor : no Attention Indicator : no Power Indicator : no Hot-Plug Surprise : no EMI Present : no Command Completed : yes Slot Status : 0x0010 Slot Control : 0x0000 Link Active Reporting supported HPC vendor_id 111d device_id 807a ss_vid 0 ss_did 0 Registering domain:bus:dev=0000:50:00 sun=3 Unexpected CMD_COMPLETED. Need to wait for command completed event. Command not completed in 1000 msec The last two output lines are part of the problem. Each controller takes 1 second to initialized as the message shows. > I assume > you're hitting this case in pcie_init() (added by 5808639bfa98 > ("pciehp: fix slow probing")): > > /* > * Controller doesn't notify of command completion if the "No > * Command Completed Support" bit is set in Slot Capability > * register or the controller supports none of power > * controller, attention led, power led and EMI. > */ > if (NO_CMD_CMPL(ctrl) || > !(POWER_CTRL(ctrl) | ATTN_LED(ctrl) | PWR_LED(ctrl) | EMI(ctrl))) > ctrl->no_cmd_complete = 1; > > and we're setting "no_cmd_complete = 1" for your controller, which > keeps us from waiting for completion in pcie_write_cmd(). > > I'm dubious about the assertion that a controller without power > control, attention LED, power LED, or EMI can't support command > completion. I don't see anything in the spec to that effect. I agree, specially since I am seeing this contrary behavior (to that assertion) in action :-) > > Since you're seeing PCI_EXP_SLTSTA_CC=1, your controller *should* > support Command Completion notification and PCI_EXP_SLTCAP_NCCS should > be 0 (per Table 7-20), so I wonder what happens on your system if you > change pcie_init() so it leaves "ctrl->no_cmd_complete = 0" instead? > Does it work correctly then? Yes, it work well after that (Both the error output lines go away as well). And the 1 second delay per controller is also gone. > > I know we can't just drop the "!(POWER_CTRL(ctrl) | ...)" tests > because we don't want to reintroduce the problem fixed by > 5808639bfa98, but I wonder if we can find a better fix that addresses > both problems. Please let me know if any one has any suggestions? IMHO, this patch should yet not cause the original problem to reappear because we are clearing the bit *only* when we know that it is already set (and no way to clear it because at least in my controller no subsequent interrupts can be generated). But I do not have enough understanding, and will be glad to get any pointers. Thanks, - Rajat > > Bjorn > >> >> drivers/pci/hotplug/pciehp_hpc.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c >> index 5b8d749..ba8e06f 100644 >> --- a/drivers/pci/hotplug/pciehp_hpc.c >> +++ b/drivers/pci/hotplug/pciehp_hpc.c >> @@ -185,6 +185,7 @@ static int pcie_write_cmd(struct controller *ctrl, u16 cmd, u16 mask) >> } >> >> if (slot_status & PCI_EXP_SLTSTA_CC) { >> + pciehp_writew(ctrl, PCI_EXP_SLTSTA, PCI_EXP_SLTSTA_CC); >> if (!ctrl->no_cmd_complete) { >> /* >> * After 1 sec and CMD_COMPLETED still not set, just >> -- >> 1.7.9.5 >> -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bjorn / list, I think there are two independent problems that need to be addressed. Problem #1: In any condition where (e.g. spurious interrupt) once the execution reaches inside the if condition below, the PCI_EXP_SLTSTA_CC bit is already set, and needs to be cleared. This does not happen today and hence this patch was aimed at solving that. Please note that this will not cause any problems to the systems that were fixed by 5808639bfa98, because this is equally applicable to any case. >>> >>> drivers/pci/hotplug/pciehp_hpc.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c >>> index 5b8d749..ba8e06f 100644 >>> --- a/drivers/pci/hotplug/pciehp_hpc.c >>> +++ b/drivers/pci/hotplug/pciehp_hpc.c >>> @@ -185,6 +185,7 @@ static int pcie_write_cmd(struct controller *ctrl, u16 cmd, u16 mask) >>> } >>> >>> if (slot_status & PCI_EXP_SLTSTA_CC) { >>> + pciehp_writew(ctrl, PCI_EXP_SLTSTA, PCI_EXP_SLTSTA_CC); >>> if (!ctrl->no_cmd_complete) { >>> /* >>> * After 1 sec and CMD_COMPLETED still not set, just >>> -- >>> 1.7.9.5 >>> Problem #2: Looks like we have now 2 classes of systems: a) The 5808639bfa98 was addressed towards systems that advertise the "Command completion notification" capability, but do not notify of command completion in case MRL, Power Ctrl, Attn LED and Pwr LED bits are not written in SLOT_CTRL register (not implemented in capabilities). Thus no command completion shall be generated for pcie_disable_notifications() for e.g. b) I have a system that where none of the above described HP elements are present implemented, but the controller supports command completion, and sends it out for every write of the slot control register. Thus notification for command complete is generated for pcie_disable_notifiction(). I don't believe there is a way to differentiate between these 2 classes of systems at init time, unless we try to generate a notification and do or do not get one. The PCIe specification section 7.8.10 seems to lean towards category (b) of systems, but today this class of systems shall get penalized by delay of 1 second (per controller) during probe. I can try and address that, but admittedly I could not think of a better solution than moving this code from pcie_init() to pcie_write_cmd(). > if (NO_CMD_CMPL(ctrl) || > !(POWER_CTRL(ctrl) | ATTN_LED(ctrl) | PWR_LED(ctrl) | EMI(ctrl))) > ctrl->no_cmd_complete = 1; Please advise. Also, I'd appreciate if you could please let me know if there are any comments on the patch for problem #1 above. Thanks, Rajat -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 07, 2013 at 01:53:44PM -0800, Rajat Jain wrote: > Hi Bjorn / list, > > I think there are two independent problems that need to be addressed. > > Problem #1: In any condition where (e.g. spurious interrupt) once the > execution reaches inside the if condition below, the PCI_EXP_SLTSTA_CC > bit is already set, and needs to be cleared. This does not happen > today and hence this patch was aimed at solving that. Please note that > this will not cause any problems to the systems that were fixed by > 5808639bfa98, because this is equally applicable to any case. Your patch might be the right thing to do, but something still niggles at me, and I can't quite put my finger on it yet. > >>> > >>> drivers/pci/hotplug/pciehp_hpc.c | 1 + > >>> 1 file changed, 1 insertion(+) > >>> > >>> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c > >>> index 5b8d749..ba8e06f 100644 > >>> --- a/drivers/pci/hotplug/pciehp_hpc.c > >>> +++ b/drivers/pci/hotplug/pciehp_hpc.c > >>> @@ -185,6 +185,7 @@ static int pcie_write_cmd(struct controller *ctrl, u16 cmd, u16 mask) > >>> } > >>> > >>> if (slot_status & PCI_EXP_SLTSTA_CC) { > >>> + pciehp_writew(ctrl, PCI_EXP_SLTSTA, PCI_EXP_SLTSTA_CC); > >>> if (!ctrl->no_cmd_complete) { > >>> /* > >>> * After 1 sec and CMD_COMPLETED still not set, just > >>> -- > >>> 1.7.9.5 > >>> > > Problem #2: Looks like we have now 2 classes of systems: > > a) The 5808639bfa98 was addressed towards systems that advertise the > "Command completion notification" capability, but do not notify of > command completion in case MRL, Power Ctrl, Attn LED and Pwr LED bits > are not written in SLOT_CTRL register (not implemented in > capabilities). Thus no command completion shall be generated for > pcie_disable_notifications() for e.g. This seems pretty clearly out of spec. I read the 82801H spec for the part used in the system in bz 10751, and the documentation seems to comply with the spec. Possibly the part doesn't work as advertised, but I think it's more likely there's some subtlety we're still missing. I'm a little bit surprised that we'd be using pciehp on that Thinkpad R61 instead of acpiphp. The kernel in bz 10751 is so ancient that it might not be negotiating correctly for control of pciehp. > b) I have a system that where none of the above described HP elements > are present implemented, but the controller supports command > completion, and sends it out for every write of the slot control > register. Thus notification for command complete is generated for > pcie_disable_notifiction(). > > I don't believe there is a way to differentiate between these 2 > classes of systems at init time, unless we try to generate a > notification and do or do not get one. > > The PCIe specification section 7.8.10 seems to lean towards category > (b) of systems, but today this class of systems shall get penalized by > delay of 1 second (per controller) during probe. Where does this delay happen on your system? I tried to work through the path but I don't see it yet. Here's what I expect to happen on your system: pciehp_probe pcie_init readl(SLTCAP) if (NO_CMD_CMPL || !(POWER_CTRL | ATTN_LED | PWR_LED | EMI)) # true ctrl->no_cmd_complete = 1 # set (condition above is true) writew(SLTSTA, 0x1f) # clear ABP PFD MRLSC PDC CC pcie_disable_notification pcie_write_cmd(0, PDCE | ABPE | MRLSCE | PFDE | HPIE | CCIE | DLLSCE) readw(SLTSTA) # CC == 0 (was cleared above) writew(SLTCTL) ... # no waiting here because no_cmd_complete == 1 ... hardware sets SLTSTA.CC ... pcie_init_notification pcie_enable_notification pcie_write_cmd readw(SLTSTA) # CC == 1 (was set by HW above) if (SLTSTA.CC) # true if (!NO_CMD_CMPL)) # true dbg("Unexpected CMD_COMPLETED. ...") ctrl->no_cmd_complete = 0 writew(SLTCTL) ... # this time we wait because no_cmd_complete == 0 ... pcie_wait_cmd(..., poll == 1) # now no_cmd_complete == 0 pcie_poll_cmd readw(SLTSTA) # CC == 1 on first read if (SLTSTA.CC) writew(SLTSTA, CC) return 1 I would think you'd see the "Unexpected CMD_COMPLETED. Need to wait for command completed event" message (if enabled), but I don't see where the one second timeout would happen. It looks like we would call pcie_poll_cmd(), but it would return immediately because SLTSTA.CC is already set. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Thu, Nov 7, 2013 at 5:23 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > On Thu, Nov 07, 2013 at 01:53:44PM -0800, Rajat Jain wrote: >> Hi Bjorn / list, >> >> I think there are two independent problems that need to be addressed. >> >> Problem #1: In any condition where (e.g. spurious interrupt) once the >> execution reaches inside the if condition below, the PCI_EXP_SLTSTA_CC >> bit is already set, and needs to be cleared. This does not happen >> today and hence this patch was aimed at solving that. Please note that >> this will not cause any problems to the systems that were fixed by >> 5808639bfa98, because this is equally applicable to any case. > > Your patch might be the right thing to do, but something still niggles > at me, and I can't quite put my finger on it yet. > >> >>> >> >>> drivers/pci/hotplug/pciehp_hpc.c | 1 + >> >>> 1 file changed, 1 insertion(+) >> >>> >> >>> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c >> >>> index 5b8d749..ba8e06f 100644 >> >>> --- a/drivers/pci/hotplug/pciehp_hpc.c >> >>> +++ b/drivers/pci/hotplug/pciehp_hpc.c >> >>> @@ -185,6 +185,7 @@ static int pcie_write_cmd(struct controller *ctrl, u16 cmd, u16 mask) >> >>> } >> >>> >> >>> if (slot_status & PCI_EXP_SLTSTA_CC) { >> >>> + pciehp_writew(ctrl, PCI_EXP_SLTSTA, PCI_EXP_SLTSTA_CC); >> >>> if (!ctrl->no_cmd_complete) { >> >>> /* >> >>> * After 1 sec and CMD_COMPLETED still not set, just >> >>> -- >> >>> 1.7.9.5 >> >>> >> >> Problem #2: Looks like we have now 2 classes of systems: >> >> a) The 5808639bfa98 was addressed towards systems that advertise the >> "Command completion notification" capability, but do not notify of >> command completion in case MRL, Power Ctrl, Attn LED and Pwr LED bits >> are not written in SLOT_CTRL register (not implemented in >> capabilities). Thus no command completion shall be generated for >> pcie_disable_notifications() for e.g. > > This seems pretty clearly out of spec. I read the 82801H spec for the > part used in the system in bz 10751, and the documentation seems to > comply with the spec. Possibly the part doesn't work as advertised, > but I think it's more likely there's some subtlety we're still missing. > > I'm a little bit surprised that we'd be using pciehp on that Thinkpad R61 > instead of acpiphp. The kernel in bz 10751 is so ancient that it might > not be negotiating correctly for control of pciehp. > >> b) I have a system that where none of the above described HP elements >> are present implemented, but the controller supports command >> completion, and sends it out for every write of the slot control >> register. Thus notification for command complete is generated for >> pcie_disable_notifiction(). >> >> I don't believe there is a way to differentiate between these 2 >> classes of systems at init time, unless we try to generate a >> notification and do or do not get one. >> >> The PCIe specification section 7.8.10 seems to lean towards category >> (b) of systems, but today this class of systems shall get penalized by >> delay of 1 second (per controller) during probe. > > Where does this delay happen on your system? I tried to work through > the path but I don't see it yet. Here's what I expect to happen on your > system: > > pciehp_probe > pcie_init > readl(SLTCAP) > if (NO_CMD_CMPL || !(POWER_CTRL | ATTN_LED | PWR_LED | EMI)) # true > ctrl->no_cmd_complete = 1 # set (condition above is true) > writew(SLTSTA, 0x1f) # clear ABP PFD MRLSC PDC CC > pcie_disable_notification > pcie_write_cmd(0, PDCE | ABPE | MRLSCE | PFDE | HPIE | CCIE | DLLSCE) > readw(SLTSTA) # CC == 0 (was cleared above) > writew(SLTCTL) > ... > # no waiting here because no_cmd_complete == 1 > ... > hardware sets SLTSTA.CC > ... > pcie_init_notification > pcie_enable_notification > pcie_write_cmd > readw(SLTSTA) # CC == 1 (was set by HW above) > if (SLTSTA.CC) # true > if (!NO_CMD_CMPL)) # true > dbg("Unexpected CMD_COMPLETED. ...") > ctrl->no_cmd_complete = 0 > writew(SLTCTL) > ... > # this time we wait because no_cmd_complete == 0 > ... > pcie_wait_cmd(..., poll == 1) # now no_cmd_complete == 0 Actually pcie_wait_cmd() shall be called with poll=0. That is because the following conditions are both false (we just enabled both the interrupts while enabling notifications): if (!(slot_ctrl & PCI_EXP_SLTCTL_HPIE) || !(slot_ctrl & PCI_EXP_SLTCTL_CCIE)) poll = 1; Thus it does not poll, but waits for interrupt. Which does not happen because the CC bit in the slot status register was not cleared. As a result, we get the following messages on the console for each controller (and 1 second delay): =========================== Unexpected CMD_COMPLETED. Need to wait for command completed event. Command not completed in 1000 msec =========================== With my patch, we'd eliminate the 1 second delay and the second line of output above. If we also want to get rid of the first line "Unexpected CMD_COMPLETED..." also, that would probably have to be solved by changing the behavior to assume that CC shall be generated for every SLOTCTRL register write. Please suggest. Thanks, Rajat > pcie_poll_cmd > readw(SLTSTA) # CC == 1 on first read > if (SLTSTA.CC) > writew(SLTSTA, CC) > return 1 > > I would think you'd see the "Unexpected CMD_COMPLETED. Need to wait for > command completed event" message (if enabled), > but I don't see where the > one second timeout would happen. It looks like we would call > pcie_poll_cmd(), but it would return immediately because SLTSTA.CC is > already set. > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 8, 2013 at 10:30 AM, Rajat Jain <rajatjain.linux@gmail.com> wrote: > On Thu, Nov 7, 2013 at 5:23 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >> Where does this delay happen on your system? I tried to work through >> the path but I don't see it yet. Here's what I expect to happen on your >> system: >> >> pciehp_probe >> pcie_init >> readl(SLTCAP) >> if (NO_CMD_CMPL || !(POWER_CTRL | ATTN_LED | PWR_LED | EMI)) # true >> ctrl->no_cmd_complete = 1 # set (condition above is true) >> writew(SLTSTA, 0x1f) # clear ABP PFD MRLSC PDC CC >> pcie_disable_notification >> pcie_write_cmd(0, PDCE | ABPE | MRLSCE | PFDE | HPIE | CCIE | DLLSCE) >> readw(SLTSTA) # CC == 0 (was cleared above) >> writew(SLTCTL) >> ... >> # no waiting here because no_cmd_complete == 1 >> ... >> hardware sets SLTSTA.CC >> ... >> pcie_init_notification >> pcie_enable_notification >> pcie_write_cmd >> readw(SLTSTA) # CC == 1 (was set by HW above) >> if (SLTSTA.CC) # true >> if (!NO_CMD_CMPL)) # true >> dbg("Unexpected CMD_COMPLETED. ...") >> ctrl->no_cmd_complete = 0 >> writew(SLTCTL) >> ... >> # this time we wait because no_cmd_complete == 0 >> ... >> pcie_wait_cmd(..., poll == 1) # now no_cmd_complete == 0 > > Actually pcie_wait_cmd() shall be called with poll=0. That is because > the following conditions are both false (we just enabled both the > interrupts while enabling notifications): > > if (!(slot_ctrl & PCI_EXP_SLTCTL_HPIE) || > !(slot_ctrl & PCI_EXP_SLTCTL_CCIE)) > poll = 1; > > Thus it does not poll, but waits for interrupt. Which does not happen > because the CC bit in the slot status register was not cleared. Right, of course. Thanks for pointing out my error. > As a > result, we get the following messages on the console for each > controller (and 1 second delay): > =========================== > Unexpected CMD_COMPLETED. Need to wait for command completed event. > Command not completed in 1000 msec > =========================== > With my patch, we'd eliminate the 1 second delay and the second line > of output above. If we also want to get rid of the first line > "Unexpected CMD_COMPLETED..." also, that would probably have to be > solved by changing the behavior to assume that CC shall be generated > for every SLOTCTRL register write. I *do* want to get rid of the "Unexpected CMD_COMPLETED" complaint. We shouldn't complain about hardware that is working perfectly fine. I don't know the best way to do that yet. I have found a box with the same hardware that was fixed by 5808639bfa98, so I hope to play with it and figure out something that will work nicely for both scenarios. BTW, can you open a report at bugzilla.kernel.org and attach your "lspci -vvxxx" and dmesg output? When we eventually merge a fix, I'd like to have the details archived somewhere. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, >> With my patch, we'd eliminate the 1 second delay and the second line >> of output above. If we also want to get rid of the first line >> "Unexpected CMD_COMPLETED..." also, that would probably have to be >> solved by changing the behavior to assume that CC shall be generated >> for every SLOTCTRL register write. > > I *do* want to get rid of the "Unexpected CMD_COMPLETED" complaint. > We shouldn't complain about hardware that is working perfectly fine. > I don't know the best way to do that yet. I have found a box with the > same hardware that was fixed by 5808639bfa98, so I hope to play with > it and figure out something that will work nicely for both scenarios. Please keep posted :-) > > BTW, can you open a report at bugzilla.kernel.org and attach your > "lspci -vvxxx" and dmesg output? When we eventually merge a fix, I'd > like to have the details archived somewhere. Done: https://bugzilla.kernel.org/show_bug.cgi?id=64821 Please let me know if you need any help in trying something out - I'd be more than keen to help - write code or test. BTW, I figured another way that solves the current problem at hand (for both kind of controllers). Today the events are cleared just before call to pcie_disable_notification, if we clear them afterwards also, the current problem is taken care of for all controllers. Of course this does not look the right way to solve the problem though (since the ctrl->no_cmd_completed shall still be 1, which is not right). @@ -948,6 +948,10 @@ struct controller *pcie_init(struct pcie_device *dev) /* Clear all remaining event bits in Slot Status register */ if (pciehp_writew(ctrl, PCI_EXP_SLTSTA, 0x1f)) goto abort_ctrl; /* Disable sotfware notification */ pcie_disable_notification(ctrl); + /* Clear all remaining event bits in Slot Status register */ + if (pciehp_writew(ctrl, PCI_EXP_SLTSTA, 0x1f)) + goto abort_ctrl; + ctrl_info(ctrl, "HPC vendor_id %x device_id %x ss_vid %x ss_did %x\n", pdev->vendor, pdev->device, pdev->subsystem_vendor, pdev->subsystem_device); Thanks, Rajat -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c index 5b8d749..ba8e06f 100644 --- a/drivers/pci/hotplug/pciehp_hpc.c +++ b/drivers/pci/hotplug/pciehp_hpc.c @@ -185,6 +185,7 @@ static int pcie_write_cmd(struct controller *ctrl, u16 cmd, u16 mask) } if (slot_status & PCI_EXP_SLTSTA_CC) { + pciehp_writew(ctrl, PCI_EXP_SLTSTA, PCI_EXP_SLTSTA_CC); if (!ctrl->no_cmd_complete) { /* * After 1 sec and CMD_COMPLETED still not set, just