On Sat, Jun 14, 2014 at 2:21 PM, Bjorn Helgaas <bhelgaas@xxxxxxxxxx> wrote: > Yinghai has been working on pciehp timeouts related to a hardware > erratum in Intel, AMD, and Nvidia hotplug controllers. This affects > the way we wait for command completion on those controllers. > > I had some suggestions about how to change pciehp to make this work > better in general, without having to check for specific vendors. We > need something that works well on hardware that conforms to the spec, > as well as the stuff that doesn't. > > I haven't heard anything for a while, so I wrote up these patches to > make my proposals concrete. Unfortunately, I can't easily test any of > this, so I'm posting these for comment and possible testing if anybody > is ambitious. > > The Intel erratum is CF118, described here: > http://www.intel.com/content/www/us/en/processors/xeon/xeon-e7-v2-spec-update.html > --- > > Bjorn Helgaas (4): > PCI: pciehp: Make pcie_wait_cmd() self-contained > PCI: pciehp: Wait for hotplug command completion lazily > PCI: pciehp: Compute timeout from hotplug command start time > PCI: pciehp: Remove assumptions about which commands cause completion events > > > drivers/pci/hotplug/pciehp.h | 2 + > drivers/pci/hotplug/pciehp_hpc.c | 91 +++++++++++++++++--------------------- > 2 files changed, 42 insertions(+), 51 deletions(-) Looks like we missed something. With last kernel I still saw the 1s delay per slot. After adding more debug printout patches, I got following: [ 67.476898] calling pcied_init+0x0/0x74 @ 1 [ 67.477114] pciehp 0000:00:02.0:pcie04: Hotplug Controller: [ 67.477115] pciehp 0000:00:02.0:pcie04: Seg/Bus/Dev/Func/IRQ : 0000:00:02.0 IRQ 58 [ 67.477117] pciehp 0000:00:02.0:pcie04: Vendor ID : 0x8086 [ 67.477118] pciehp 0000:00:02.0:pcie04: Device ID : 0x2f04 [ 67.477119] pciehp 0000:00:02.0:pcie04: Subsystem ID : 0x0000 [ 67.477120] pciehp 0000:00:02.0:pcie04: Subsystem Vendor ID : 0x8086 [ 67.477121] pciehp 0000:00:02.0:pcie04: PCIe Cap offset : 0x90 [ 67.477124] pciehp 0000:00:02.0:pcie04: PCI resource [13] : [io 0x5000-0x5fff] [ 67.477125] pciehp 0000:00:02.0:pcie04: PCI resource [14] : [mem 0x98000000-0x9bffffff] [ 67.477127] pciehp 0000:00:02.0:pcie04: PCI resource [15] : [mem 0x381800000000-0x381bffffffff 64bit pref] [ 67.477128] pciehp 0000:00:02.0:pcie04: Slot Capabilities : 0x00088cdb [ 67.477129] pciehp 0000:00:02.0:pcie04: Physical Slot Number : 1 [ 67.477130] pciehp 0000:00:02.0:pcie04: Attention Button : yes [ 67.477131] pciehp 0000:00:02.0:pcie04: Power Controller : yes [ 67.477132] pciehp 0000:00:02.0:pcie04: MRL Sensor : no [ 67.477132] pciehp 0000:00:02.0:pcie04: Attention Indicator : yes [ 67.477133] pciehp 0000:00:02.0:pcie04: Power Indicator : yes [ 67.477134] pciehp 0000:00:02.0:pcie04: Hot-Plug Surprise : no [ 67.477135] pciehp 0000:00:02.0:pcie04: EMI Present : no [ 67.477136] pciehp 0000:00:02.0:pcie04: Command Completed : yes [ 67.477137] pciehp 0000:00:02.0:pcie04: Slot Status : 0x0010 [ 67.477138] pciehp 0000:00:02.0:pcie04: Slot Control : 0x07cb [ 67.477140] pciehp 0000:00:02.0:pcie04: Link Active Reporting supported [ 67.477144] pciehp 0000:00:02.0:pcie04: pcie_disable_notification: SLOTCTRL a8 write cmd 0 [ 67.477145] pciehp 0000:00:02.0:pcie04: Slot #1 AttnBtn+ AttnInd+ PwrInd+ PwrCtrl+ MRL- Interlock- NoCompl- LLActRep+ [ 67.479926] pciehp 0000:00:02.0:pcie04: Registering domain:bus:dev=0000:01:00 sun=1 [ 67.479975] pci_bus 0000:01: dev 00, created physical slot 1 [ 67.480041] pci_hotplug: __pci_hp_register: Added slot 1 to the list [ 69.078753] pciehp 0000:00:02.0:pcie04: Timeout on hotplug command 0x000007c0 (issued 1604 msec ago) [ 69.078758] pciehp 0000:00:02.0:pcie04: pcie_enable_notification: SLOTCTRL a8 write cmd 1031 [ 69.078763] pciehp 0000:00:02.0:pcie04: pciehp_get_power_status: SLOTCTRL a8 value read 17f1 [ 69.078765] pciehp 0000:00:02.0:pcie04: service driver pciehp loaded so there are pcie_disable_notification and pcie_enable_notification. pcie_enable_notification will wait 1s. wonder if we can just remove pcie_disable_notification calling from pciehp_hpc.c::pcie_init() at all. Thanks Yinghai
Subject: [PATCH] pci: get exact timeout in pciehp 1. pcie_poll_cmd take msecs instead of jiffies. 2. debug print out should count to current now instead of the one before wait_event Signed-off-by: Yinghai Lu <yinghai@xxxxxxxxxx> --- drivers/pci/hotplug/pciehp_hpc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 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 @@ -160,7 +160,7 @@ static void pcie_wait_cmd(struct control ctrl->slot_ctrl & PCI_EXP_SLTCTL_CCIE) rc = wait_event_timeout(ctrl->queue, !ctrl->cmd_busy, timeout); else - rc = pcie_poll_cmd(ctrl, timeout); + rc = pcie_poll_cmd(ctrl, jiffies_to_msecs(timeout)); /* * Controllers with errata like Intel CF118 don't generate @@ -173,7 +173,7 @@ static void pcie_wait_cmd(struct control if (!rc) ctrl_info(ctrl, "Timeout on hotplug command %#010x (issued %u msec ago)\n", ctrl->slot_ctrl, - jiffies_to_msecs(now - ctrl->cmd_started)); + jiffies_to_msecs(jiffies - ctrl->cmd_started)); } /**
Subject: [PATCH] pci: more debug printout for pcie_write_cmd for pciehp Signed-off-by: Yinghai Lu <yinghai@xxxxxxxxxx> --- drivers/pci/hotplug/pciehp_hpc.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) 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 @@ -422,9 +422,9 @@ void pciehp_set_attention_status(struct default: return; } + pcie_write_cmd(ctrl, slot_cmd, PCI_EXP_SLTCTL_AIC); ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__, pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, slot_cmd); - pcie_write_cmd(ctrl, slot_cmd, PCI_EXP_SLTCTL_AIC); } void pciehp_green_led_on(struct slot *slot) @@ -602,6 +602,8 @@ void pcie_enable_notification(struct con PCI_EXP_SLTCTL_DLLSCE); pcie_write_cmd(ctrl, cmd, mask); + ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__, + pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, cmd); } static void pcie_disable_notification(struct controller *ctrl) @@ -613,6 +615,8 @@ static void pcie_disable_notification(st PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_CCIE | PCI_EXP_SLTCTL_DLLSCE); pcie_write_cmd(ctrl, 0, mask); + ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__, + pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, 0); } /* @@ -640,6 +644,8 @@ int pciehp_reset_slot(struct slot *slot, stat_mask |= PCI_EXP_SLTSTA_DLLSC; pcie_write_cmd(ctrl, 0, ctrl_mask); + ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__, + pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, 0); if (pciehp_poll_mode) del_timer_sync(&ctrl->poll_timer); @@ -647,6 +653,8 @@ int pciehp_reset_slot(struct slot *slot, pcie_capability_write_word(pdev, PCI_EXP_SLTSTA, stat_mask); pcie_write_cmd(ctrl, ctrl_mask, ctrl_mask); + ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__, + pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, ctrl_mask); if (pciehp_poll_mode) int_poll_timeout(ctrl->poll_timer.data);
