| Message ID | 20170817060446.1340-3-ruscur@russell.cc |
|---|---|
| State | Accepted |
| Headers | show |
On 17/08/17 16:04, Russell Currey wrote: > When a complete reset occurs, after the PHB recovers it propagates a > reset down the wire to every device. At the same time, skiboot talks to > every device in order to restore the state of devices to what they were > before the reset. > > In some situations, such as devices that recovered slowly and/or were > behind a switch, skiboot attempted to access config space of the device > before the link was up and the device could respond. > > Fix this by retrying CRS until the device responds correctly, and for > devices behind a switch, making sure the switch has its link up first. > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Signed-off-by: Russell Currey <ruscur@russell.cc> This looks fairly sensible to me. Reviewed-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com> > @@ -1835,14 +1848,26 @@ static int __pci_restore_bridge_buses(struct phb *phb, > struct pci_device *pd, > void *data __unused) > { > - if (!pd->is_bridge) { > - uint32_t vdid; > + uint32_t vdid; > + > + /* If the device is behind a switch, wait for the switch */ > + if (!pd->is_vf && !(pd->bdfn & 7) && pd->parent != NULL && > + pd->parent->dev_type == PCIE_TYPE_SWITCH_DNPORT) { > + if (!pci_bridge_wait_link(phb, pd->parent, true)) { > + PCIERR(phb, pd->bdfn, "Timeout waiting for switch\n"); > + return -1; > + } > + } > + > + /* Wait for config space to stop returning CRS */ > + if (!pci_wait_crs(phb, pd->bdfn, &vdid)) > + return -1; What does returning -1 do - it looks to me that this stops the pci_walk_dev() from descending any further, which seems correct, but I haven't used pci_walk_dev() very much...
On Thursday 17 August 2017 11:34 AM, Russell Currey wrote: > When a complete reset occurs, after the PHB recovers it propagates a > reset down the wire to every device. At the same time, skiboot talks to > every device in order to restore the state of devices to what they were > before the reset. > > In some situations, such as devices that recovered slowly and/or were > behind a switch, skiboot attempted to access config space of the device > before the link was up and the device could respond. > > Fix this by retrying CRS until the device responds correctly, and for > devices behind a switch, making sure the switch has its link up first. > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Signed-off-by: Russell Currey <ruscur@russell.cc> With this patch, PCI devices are initialized properly in kdump environment.. Tested-by: Hari Bathini <hbathini@linux.vnet.ibm.com> > --- > core/pci.c | 51 ++++++++++++++++++++++++++++++++++++++------------- > 1 file changed, 38 insertions(+), 13 deletions(-) > > diff --git a/core/pci.c b/core/pci.c > index a709e27f..917a9e6e 100644 > --- a/core/pci.c > +++ b/core/pci.c > @@ -218,21 +218,18 @@ void pci_init_capabilities(struct phb *phb, struct pci_device *pd) > pci_init_pm_cap(phb, pd); > } > > -static struct pci_device *pci_scan_one(struct phb *phb, struct pci_device *parent, > - uint16_t bdfn) > +static bool pci_wait_crs(struct phb *phb, uint16_t bdfn, uint32_t *out_vdid) > { > - struct pci_device *pd = NULL; > uint32_t retries, vdid; > int64_t rc; > - uint8_t htype; > bool had_crs = false; > > for (retries = 0; retries < 40; retries++) { > rc = pci_cfg_read32(phb, bdfn, PCI_CFG_VENDOR_ID, &vdid); > if (rc) > - return NULL; > + return false; > if (vdid == 0xffffffff || vdid == 0x00000000) > - return NULL; > + return false; > if (vdid != 0xffff0001) > break; > had_crs = true; > @@ -240,11 +237,27 @@ static struct pci_device *pci_scan_one(struct phb *phb, struct pci_device *paren > } > if (vdid == 0xffff0001) { > PCIERR(phb, bdfn, "CRS timeout !\n"); > - return NULL; > + return false; > } > if (had_crs) > PCIDBG(phb, bdfn, "Probe success after %d CRS\n", retries); > > + if (out_vdid) > + *out_vdid = vdid; > + return true; > +} > + > +static struct pci_device *pci_scan_one(struct phb *phb, struct pci_device *parent, > + uint16_t bdfn) > +{ > + struct pci_device *pd = NULL; > + uint32_t vdid; > + int64_t rc; > + uint8_t htype; > + > + if (!pci_wait_crs(phb, bdfn, &vdid)) > + return NULL; > + > /* Perform a dummy write to the device in order for it to > * capture it's own bus number, so any subsequent error > * messages will be properly tagged > @@ -1835,14 +1848,26 @@ static int __pci_restore_bridge_buses(struct phb *phb, > struct pci_device *pd, > void *data __unused) > { > - if (!pd->is_bridge) { > - uint32_t vdid; > + uint32_t vdid; > + > + /* If the device is behind a switch, wait for the switch */ > + if (!pd->is_vf && !(pd->bdfn & 7) && pd->parent != NULL && > + pd->parent->dev_type == PCIE_TYPE_SWITCH_DNPORT) { > + if (!pci_bridge_wait_link(phb, pd->parent, true)) { > + PCIERR(phb, pd->bdfn, "Timeout waiting for switch\n"); > + return -1; > + } > + } > + > + /* Wait for config space to stop returning CRS */ > + if (!pci_wait_crs(phb, pd->bdfn, &vdid)) > + return -1; > > - /* Make all devices below a bridge "re-capture" the bdfn */ > - if (pci_cfg_read32(phb, pd->bdfn, PCI_CFG_VENDOR_ID, &vdid) == 0) > - pci_cfg_write32(phb, pd->bdfn, PCI_CFG_VENDOR_ID, vdid); > + /* Make all devices below a bridge "re-capture" the bdfn */ > + pci_cfg_write32(phb, pd->bdfn, PCI_CFG_VENDOR_ID, vdid); > + > + if (!pd->is_bridge) > return 0; > - } > > pci_cfg_write8(phb, pd->bdfn, PCI_CFG_PRIMARY_BUS, > pd->primary_bus);
On Thu, 2017-08-17 at 18:26 +1000, Andrew Donnellan wrote: > On 17/08/17 16:04, Russell Currey wrote: > > When a complete reset occurs, after the PHB recovers it propagates a > > reset down the wire to every device. At the same time, skiboot talks to > > every device in order to restore the state of devices to what they were > > before the reset. > > > > In some situations, such as devices that recovered slowly and/or were > > behind a switch, skiboot attempted to access config space of the device > > before the link was up and the device could respond. > > > > Fix this by retrying CRS until the device responds correctly, and for > > devices behind a switch, making sure the switch has its link up first. > > > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > Signed-off-by: Russell Currey <ruscur@russell.cc> > > This looks fairly sensible to me. > > Reviewed-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com> > > > @@ -1835,14 +1848,26 @@ static int __pci_restore_bridge_buses(struct phb > > *phb, > > struct pci_device *pd, > > void *data __unused) > > { > > - if (!pd->is_bridge) { > > - uint32_t vdid; > > + uint32_t vdid; > > + > > + /* If the device is behind a switch, wait for the switch */ > > + if (!pd->is_vf && !(pd->bdfn & 7) && pd->parent != NULL && > > + pd->parent->dev_type == PCIE_TYPE_SWITCH_DNPORT) { > > + if (!pci_bridge_wait_link(phb, pd->parent, true)) { > > + PCIERR(phb, pd->bdfn, "Timeout waiting for > > switch\n"); > > + return -1; > > + } > > + } > > + > > + /* Wait for config space to stop returning CRS */ > > + if (!pci_wait_crs(phb, pd->bdfn, &vdid)) > > + return -1; > > What does returning -1 do - it looks to me that this stops the > pci_walk_dev() from descending any further, which seems correct, but I > haven't used pci_walk_dev() very much... > Yes, that's what it does.
diff --git a/core/pci.c b/core/pci.c index a709e27f..917a9e6e 100644 --- a/core/pci.c +++ b/core/pci.c @@ -218,21 +218,18 @@ void pci_init_capabilities(struct phb *phb, struct pci_device *pd) pci_init_pm_cap(phb, pd); } -static struct pci_device *pci_scan_one(struct phb *phb, struct pci_device *parent, - uint16_t bdfn) +static bool pci_wait_crs(struct phb *phb, uint16_t bdfn, uint32_t *out_vdid) { - struct pci_device *pd = NULL; uint32_t retries, vdid; int64_t rc; - uint8_t htype; bool had_crs = false; for (retries = 0; retries < 40; retries++) { rc = pci_cfg_read32(phb, bdfn, PCI_CFG_VENDOR_ID, &vdid); if (rc) - return NULL; + return false; if (vdid == 0xffffffff || vdid == 0x00000000) - return NULL; + return false; if (vdid != 0xffff0001) break; had_crs = true; @@ -240,11 +237,27 @@ static struct pci_device *pci_scan_one(struct phb *phb, struct pci_device *paren } if (vdid == 0xffff0001) { PCIERR(phb, bdfn, "CRS timeout !\n"); - return NULL; + return false; } if (had_crs) PCIDBG(phb, bdfn, "Probe success after %d CRS\n", retries); + if (out_vdid) + *out_vdid = vdid; + return true; +} + +static struct pci_device *pci_scan_one(struct phb *phb, struct pci_device *parent, + uint16_t bdfn) +{ + struct pci_device *pd = NULL; + uint32_t vdid; + int64_t rc; + uint8_t htype; + + if (!pci_wait_crs(phb, bdfn, &vdid)) + return NULL; + /* Perform a dummy write to the device in order for it to * capture it's own bus number, so any subsequent error * messages will be properly tagged @@ -1835,14 +1848,26 @@ static int __pci_restore_bridge_buses(struct phb *phb, struct pci_device *pd, void *data __unused) { - if (!pd->is_bridge) { - uint32_t vdid; + uint32_t vdid; + + /* If the device is behind a switch, wait for the switch */ + if (!pd->is_vf && !(pd->bdfn & 7) && pd->parent != NULL && + pd->parent->dev_type == PCIE_TYPE_SWITCH_DNPORT) { + if (!pci_bridge_wait_link(phb, pd->parent, true)) { + PCIERR(phb, pd->bdfn, "Timeout waiting for switch\n"); + return -1; + } + } + + /* Wait for config space to stop returning CRS */ + if (!pci_wait_crs(phb, pd->bdfn, &vdid)) + return -1; - /* Make all devices below a bridge "re-capture" the bdfn */ - if (pci_cfg_read32(phb, pd->bdfn, PCI_CFG_VENDOR_ID, &vdid) == 0) - pci_cfg_write32(phb, pd->bdfn, PCI_CFG_VENDOR_ID, vdid); + /* Make all devices below a bridge "re-capture" the bdfn */ + pci_cfg_write32(phb, pd->bdfn, PCI_CFG_VENDOR_ID, vdid); + + if (!pd->is_bridge) return 0; - } pci_cfg_write8(phb, pd->bdfn, PCI_CFG_PRIMARY_BUS, pd->primary_bus);