| Message ID | alpine.DEB.2.21.2209130050380.60554@angie.orcam.me.uk (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | pci: Work around ASMedia ASM2824 PCIe link training failures | expand |
[+cc Pali] On Sat, Sep 17, 2022 at 01:03:38PM +0100, Maciej W. Rozycki wrote: > Attempt to handle cases such as with a downstream port of the ASMedia > ASM2824 PCIe switch where link training never completes and the link > continues switching between speeds indefinitely with the data link layer > never reaching the active state. > > It has been observed with a downstream port of the ASMedia ASM2824 Gen 3 > switch wired to the upstream port of the Pericom PI7C9X2G304 Gen 2 > switch, using a Delock Riser Card PCI Express x1 > 2 x PCIe x1 device, > P/N 41433, wired to a SiFive HiFive Unmatched board. In this setup the > switches are supposed to negotiate the link speed of preferably 5.0GT/s, > falling back to 2.5GT/s. > > Instead the link continues oscillating between the two speeds, at the > rate of 34-35 times per second, with link training reported repeatedly > active ~84% of the time. Forcibly limiting the target link speed to > 2.5GT/s with the upstream ASM2824 device however makes the two switches > communicate correctly. Removing the speed restriction afterwards makes > the two devices switch to 5.0GT/s then. > > Make use of these observations then and detect the inability to train > the link, by checking for the Data Link Layer Link Active status bit > being off while the Link Bandwidth Management Status indicating that > hardware has changed the link speed or width in an attempt to correct > unreliable link operation. > > Restrict the speed to 2.5GT/s then with the Target Link Speed field, > request a retrain and wait 200ms for the data link to go up. If this > turns out successful, then lift the restriction, letting the devices > negotiate a higher speed. > > Also check for a 2.5GT/s speed restriction the firmware may have already > arranged and lift it too with ports of devices known to continue working > afterwards, currently the ASM2824 only, that already report their data > link being up. This quirk is run at boot-time and resume-time. What happens after a Secondary Bus Reset, as is done by pci_reset_secondary_bus()? PCIe r6.0, sec 7.5.1.3.13, says "setting Secondary Bus Reset triggers a hot reset on the corresponding PCI Express Port". Sec 4.2.7 says LinkUp is 0 in the LTSSM Hot Reset state, and the Hot Reset state leads to Detect, so it looks like this reset would cause the link to go down and come back up. Can you tell if that's what happens? Does the link negotiation fail then, too? If it does fail then, I don't know how hard we need to work to fix it. Maybe we just accept it? Or maybe we need a "quirk-after-reset" phase or something? Bjorn > Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk> > Link: https://lore.kernel.org/lkml/alpine.DEB.2.21.2203022037020.56670@angie.orcam.me.uk/ > Link: https://source.denx.de/u-boot/u-boot/-/commit/a398a51ccc68 > --- > Changes from v4: > > - Remove <linux/bug.h> inclusion no longer needed. > > - Make the quirk generic based on probing device features rather than > specific to the ASM2824 part only; take the Retrain Link bit erratum > into account. > > - Still lift the 2.5GT/s speed restriction with the ASM2824 only. > > - Increase retrain timeout from 200ms to 1s (PCIE_LINK_RETRAIN_TIMEOUT). > > - Remove retrain success notification. > > - Use PCIe helpers rather than generic PCI functions throughout. > > - Trim down and update the wording of the change description for the > switch from an ASM2824-specific to a generic fixup. > > Changes from v3: > > - Remove the <linux/pci_ids.h> entry for the ASM2824. > > Changes from v2: > > - Regenerate for 5.17-rc2 for a merge conflict. > > - Replace BUG_ON for a missing PCI Express capability with WARN_ON and an > early return. > > Changes from v1: > > - Regenerate for a merge conflict. > --- > drivers/pci/quirks.c | 118 +++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 118 insertions(+) > > linux-pcie-asm2824-manual-retrain.diff > Index: linux-macro/drivers/pci/quirks.c > =================================================================== > --- linux-macro.orig/drivers/pci/quirks.c > +++ linux-macro/drivers/pci/quirks.c > @@ -5956,3 +5956,121 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_I > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c0, aspm_l1_acceptable_latency); > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c1, aspm_l1_acceptable_latency); > #endif > + > +/* > + * Retrain the link of a downstream PCIe port by hand if necessary. > + * > + * This is needed at least where a downstream port of the ASMedia ASM2824 > + * Gen 3 switch is wired to the upstream port of the Pericom PI7C9X2G304 > + * Gen 2 switch, and observed with the Delock Riser Card PCI Express x1 > > + * 2 x PCIe x1 device, P/N 41433, plugged into the SiFive HiFive Unmatched > + * board. > + * > + * In such a configuration the switches are supposed to negotiate the link > + * speed of preferably 5.0GT/s, falling back to 2.5GT/s. However the link > + * continues switching between the two speeds indefinitely and the data > + * link layer never reaches the active state, with link training reported > + * repeatedly active ~84% of the time. Forcing the target link speed to > + * 2.5GT/s with the upstream ASM2824 device makes the two switches talk to > + * each other correctly however. And more interestingly retraining with a > + * higher target link speed afterwards lets the two successfully negotiate > + * 5.0GT/s. > + * > + * With the ASM2824 we can rely on the otherwise optional Data Link Layer > + * Link Active status bit and in the failed link training scenario it will > + * be off along with the Link Bandwidth Management Status indicating that > + * hardware has changed the link speed or width in an attempt to correct > + * unreliable link operation. For a port that has been left unconnected > + * both bits will be clear. So use this information to detect the problem > + * rather than polling the Link Training bit and watching out for flips or > + * at least the active status. > + * > + * Since the exact nature of the problem isn't known and in principle this > + * could trigger where an ASM2824 device is downstream rather upstream, > + * apply this erratum workaround to any downstream ports as long as they > + * support Link Active reporting and have the Link Control 2 register. > + * Restrict the speed to 2.5GT/s then with the Target Link Speed field, > + * request a retrain and wait 200ms for the data link to go up. > + * > + * If this turns out successful and we know by the Vendor:Device ID it is > + * safe to do so, then lift the restriction, letting the devices negotiate > + * a higher speed. Also check for a similar 2.5GT/s speed restriction the > + * firmware may have already arranged and lift it with ports that already > + * report their data link being up. > + */ > +static void pcie_downstream_link_retrain(struct pci_dev *dev) > +{ > + static const struct pci_device_id ids[] = { > + { PCI_VDEVICE(ASMEDIA, 0x2824) }, /* ASMedia ASM2824 */ > + {} > + }; > + u16 lnksta, lnkctl2; > + u32 lnkcap; > + > + if (!pci_is_pcie(dev) || !pcie_downstream_port(dev) > + || !pcie_cap_has_lnkctl2(dev)) > + return; > + > + /* It's too early yet to use `dev->link_active_reporting'. */ > + pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap); > + if (!(lnkcap & PCI_EXP_LNKCAP_DLLLARC)) > + return; > + > + pcie_capability_read_word(dev, PCI_EXP_LNKCTL2, &lnkctl2); > + pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &lnksta); > + if ((lnksta & (PCI_EXP_LNKSTA_LBMS | PCI_EXP_LNKSTA_DLLLA)) == > + PCI_EXP_LNKSTA_LBMS) { > + unsigned long timeout; > + u16 lnkctl; > + > + pci_info(dev, "broken device, retraining non-functional downstream link at 2.5GT/s\n"); > + > + pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &lnkctl); > + lnkctl |= PCI_EXP_LNKCTL_RL; > + lnkctl2 &= ~PCI_EXP_LNKCTL2_TLS; > + lnkctl2 |= PCI_EXP_LNKCTL2_TLS_2_5GT; > + pcie_capability_write_word(dev, PCI_EXP_LNKCTL2, lnkctl2); > + pcie_capability_write_word(dev, PCI_EXP_LNKCTL, lnkctl); > + /* > + * Due to an erratum in some devices the Retrain Link bit > + * needs to be cleared again manually to allow the link > + * training to succeed. > + */ > + lnkctl &= ~PCI_EXP_LNKCTL_RL; > + if (dev->clear_retrain_link) > + pcie_capability_write_word(dev, PCI_EXP_LNKCTL, > + lnkctl); > + > + timeout = jiffies + PCIE_LINK_RETRAIN_TIMEOUT; > + do { > + pcie_capability_read_word(dev, PCI_EXP_LNKSTA, > + &lnksta); > + if (lnksta & PCI_EXP_LNKSTA_DLLLA) > + break; > + usleep_range(10000, 20000); > + } while (time_before(jiffies, timeout)); > + > + if (!(lnksta & PCI_EXP_LNKSTA_DLLLA)) { > + pci_info(dev, "retraining failed\n"); > + return; > + } > + } > + > + if ((lnksta & PCI_EXP_LNKSTA_DLLLA) && > + (lnkctl2 & PCI_EXP_LNKCTL2_TLS) == PCI_EXP_LNKCTL2_TLS_2_5GT && > + pci_match_id(ids, dev)) { > + u16 lnkctl; > + > + pci_info(dev, "removing 2.5GT/s downstream link speed restriction\n"); > + pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &lnkctl); > + lnkctl |= PCI_EXP_LNKCTL_RL; > + lnkctl2 &= ~PCI_EXP_LNKCTL2_TLS; > + lnkctl2 |= lnkcap & PCI_EXP_LNKCAP_SLS; > + pcie_capability_write_word(dev, PCI_EXP_LNKCTL2, lnkctl2); > + pcie_capability_write_word(dev, PCI_EXP_LNKCTL, lnkctl); > + } > +} > +DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, > + pcie_downstream_link_retrain); > +DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_ANY_ID, PCI_ANY_ID, > + pcie_downstream_link_retrain);
On Thursday 03 November 2022 18:13:35 Bjorn Helgaas wrote: > [+cc Pali] > > On Sat, Sep 17, 2022 at 01:03:38PM +0100, Maciej W. Rozycki wrote: > > Attempt to handle cases such as with a downstream port of the ASMedia > > ASM2824 PCIe switch where link training never completes and the link > > continues switching between speeds indefinitely with the data link layer > > never reaching the active state. > > > > It has been observed with a downstream port of the ASMedia ASM2824 Gen 3 > > switch wired to the upstream port of the Pericom PI7C9X2G304 Gen 2 > > switch, using a Delock Riser Card PCI Express x1 > 2 x PCIe x1 device, > > P/N 41433, wired to a SiFive HiFive Unmatched board. In this setup the > > switches are supposed to negotiate the link speed of preferably 5.0GT/s, > > falling back to 2.5GT/s. > > > > Instead the link continues oscillating between the two speeds, at the > > rate of 34-35 times per second, with link training reported repeatedly > > active ~84% of the time. Forcibly limiting the target link speed to > > 2.5GT/s with the upstream ASM2824 device however makes the two switches > > communicate correctly. Removing the speed restriction afterwards makes > > the two devices switch to 5.0GT/s then. > > > > Make use of these observations then and detect the inability to train > > the link, by checking for the Data Link Layer Link Active status bit > > being off while the Link Bandwidth Management Status indicating that > > hardware has changed the link speed or width in an attempt to correct > > unreliable link operation. > > > > Restrict the speed to 2.5GT/s then with the Target Link Speed field, > > request a retrain and wait 200ms for the data link to go up. If this > > turns out successful, then lift the restriction, letting the devices > > negotiate a higher speed. > > > > Also check for a 2.5GT/s speed restriction the firmware may have already > > arranged and lift it too with ports of devices known to continue working > > afterwards, currently the ASM2824 only, that already report their data > > link being up. > > This quirk is run at boot-time and resume-time. What happens after a > Secondary Bus Reset, as is done by pci_reset_secondary_bus()? Flipping SBR bit can be done on any PCI-to-PCI bridge device and in this topology there are following: PCIe Root Port, ASMedia PCIe Switch Upstream Port, ASMedia PCIe Switch Downstream Port, Pericom PCIe Switch Upstream Port, Pericom PCIe Switch Downstream Port. (Maciej, I hope that this is whole topology and there is not some other device of PCI-to-PCI bridge type in your setup; please correct me) Bjorn, to make it clear, on which device you mean to issue secondary bus reset? Because I would not be surprised if different things happen when issuing bus reset on different parts of that topology. > PCIe r6.0, sec 7.5.1.3.13, says "setting Secondary Bus Reset triggers > a hot reset on the corresponding PCI Express Port". Sec 4.2.7 says > LinkUp is 0 in the LTSSM Hot Reset state, and the Hot Reset state > leads to Detect, so it looks like this reset would cause the link to > go down and come back up. > > Can you tell if that's what happens? Does the link negotiation fail > then, too? > > If it does fail then, I don't know how hard we need to work to fix it. > Maybe we just accept it? Or maybe we need a "quirk-after-reset" phase > or something? > > Bjorn > > > Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk> > > Link: https://lore.kernel.org/lkml/alpine.DEB.2.21.2203022037020.56670@angie.orcam.me.uk/ > > Link: https://source.denx.de/u-boot/u-boot/-/commit/a398a51ccc68 > > --- > > Changes from v4: > > > > - Remove <linux/bug.h> inclusion no longer needed. > > > > - Make the quirk generic based on probing device features rather than > > specific to the ASM2824 part only; take the Retrain Link bit erratum > > into account. > > > > - Still lift the 2.5GT/s speed restriction with the ASM2824 only. > > > > - Increase retrain timeout from 200ms to 1s (PCIE_LINK_RETRAIN_TIMEOUT). > > > > - Remove retrain success notification. > > > > - Use PCIe helpers rather than generic PCI functions throughout. > > > > - Trim down and update the wording of the change description for the > > switch from an ASM2824-specific to a generic fixup. > > > > Changes from v3: > > > > - Remove the <linux/pci_ids.h> entry for the ASM2824. > > > > Changes from v2: > > > > - Regenerate for 5.17-rc2 for a merge conflict. > > > > - Replace BUG_ON for a missing PCI Express capability with WARN_ON and an > > early return. > > > > Changes from v1: > > > > - Regenerate for a merge conflict. > > --- > > drivers/pci/quirks.c | 118 +++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 118 insertions(+) > > > > linux-pcie-asm2824-manual-retrain.diff > > Index: linux-macro/drivers/pci/quirks.c > > =================================================================== > > --- linux-macro.orig/drivers/pci/quirks.c > > +++ linux-macro/drivers/pci/quirks.c > > @@ -5956,3 +5956,121 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_I > > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c0, aspm_l1_acceptable_latency); > > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c1, aspm_l1_acceptable_latency); > > #endif > > + > > +/* > > + * Retrain the link of a downstream PCIe port by hand if necessary. > > + * > > + * This is needed at least where a downstream port of the ASMedia ASM2824 > > + * Gen 3 switch is wired to the upstream port of the Pericom PI7C9X2G304 > > + * Gen 2 switch, and observed with the Delock Riser Card PCI Express x1 > > > + * 2 x PCIe x1 device, P/N 41433, plugged into the SiFive HiFive Unmatched > > + * board. > > + * > > + * In such a configuration the switches are supposed to negotiate the link > > + * speed of preferably 5.0GT/s, falling back to 2.5GT/s. However the link > > + * continues switching between the two speeds indefinitely and the data > > + * link layer never reaches the active state, with link training reported > > + * repeatedly active ~84% of the time. Forcing the target link speed to > > + * 2.5GT/s with the upstream ASM2824 device makes the two switches talk to > > + * each other correctly however. And more interestingly retraining with a > > + * higher target link speed afterwards lets the two successfully negotiate > > + * 5.0GT/s. > > + * > > + * With the ASM2824 we can rely on the otherwise optional Data Link Layer > > + * Link Active status bit and in the failed link training scenario it will > > + * be off along with the Link Bandwidth Management Status indicating that > > + * hardware has changed the link speed or width in an attempt to correct > > + * unreliable link operation. For a port that has been left unconnected > > + * both bits will be clear. So use this information to detect the problem > > + * rather than polling the Link Training bit and watching out for flips or > > + * at least the active status. > > + * > > + * Since the exact nature of the problem isn't known and in principle this > > + * could trigger where an ASM2824 device is downstream rather upstream, > > + * apply this erratum workaround to any downstream ports as long as they > > + * support Link Active reporting and have the Link Control 2 register. > > + * Restrict the speed to 2.5GT/s then with the Target Link Speed field, > > + * request a retrain and wait 200ms for the data link to go up. > > + * > > + * If this turns out successful and we know by the Vendor:Device ID it is > > + * safe to do so, then lift the restriction, letting the devices negotiate > > + * a higher speed. Also check for a similar 2.5GT/s speed restriction the > > + * firmware may have already arranged and lift it with ports that already > > + * report their data link being up. > > + */ > > +static void pcie_downstream_link_retrain(struct pci_dev *dev) > > +{ > > + static const struct pci_device_id ids[] = { > > + { PCI_VDEVICE(ASMEDIA, 0x2824) }, /* ASMedia ASM2824 */ > > + {} > > + }; > > + u16 lnksta, lnkctl2; > > + u32 lnkcap; > > + > > + if (!pci_is_pcie(dev) || !pcie_downstream_port(dev) > > + || !pcie_cap_has_lnkctl2(dev)) > > + return; > > + > > + /* It's too early yet to use `dev->link_active_reporting'. */ > > + pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap); > > + if (!(lnkcap & PCI_EXP_LNKCAP_DLLLARC)) > > + return; > > + > > + pcie_capability_read_word(dev, PCI_EXP_LNKCTL2, &lnkctl2); > > + pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &lnksta); > > + if ((lnksta & (PCI_EXP_LNKSTA_LBMS | PCI_EXP_LNKSTA_DLLLA)) == > > + PCI_EXP_LNKSTA_LBMS) { > > + unsigned long timeout; > > + u16 lnkctl; > > + > > + pci_info(dev, "broken device, retraining non-functional downstream link at 2.5GT/s\n"); > > + > > + pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &lnkctl); > > + lnkctl |= PCI_EXP_LNKCTL_RL; > > + lnkctl2 &= ~PCI_EXP_LNKCTL2_TLS; > > + lnkctl2 |= PCI_EXP_LNKCTL2_TLS_2_5GT; > > + pcie_capability_write_word(dev, PCI_EXP_LNKCTL2, lnkctl2); > > + pcie_capability_write_word(dev, PCI_EXP_LNKCTL, lnkctl); > > + /* > > + * Due to an erratum in some devices the Retrain Link bit > > + * needs to be cleared again manually to allow the link > > + * training to succeed. > > + */ > > + lnkctl &= ~PCI_EXP_LNKCTL_RL; > > + if (dev->clear_retrain_link) > > + pcie_capability_write_word(dev, PCI_EXP_LNKCTL, > > + lnkctl); > > + > > + timeout = jiffies + PCIE_LINK_RETRAIN_TIMEOUT; > > + do { > > + pcie_capability_read_word(dev, PCI_EXP_LNKSTA, > > + &lnksta); > > + if (lnksta & PCI_EXP_LNKSTA_DLLLA) > > + break; > > + usleep_range(10000, 20000); > > + } while (time_before(jiffies, timeout)); > > + > > + if (!(lnksta & PCI_EXP_LNKSTA_DLLLA)) { > > + pci_info(dev, "retraining failed\n"); > > + return; > > + } > > + } > > + > > + if ((lnksta & PCI_EXP_LNKSTA_DLLLA) && > > + (lnkctl2 & PCI_EXP_LNKCTL2_TLS) == PCI_EXP_LNKCTL2_TLS_2_5GT && > > + pci_match_id(ids, dev)) { > > + u16 lnkctl; > > + > > + pci_info(dev, "removing 2.5GT/s downstream link speed restriction\n"); > > + pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &lnkctl); > > + lnkctl |= PCI_EXP_LNKCTL_RL; > > + lnkctl2 &= ~PCI_EXP_LNKCTL2_TLS; > > + lnkctl2 |= lnkcap & PCI_EXP_LNKCAP_SLS; > > + pcie_capability_write_word(dev, PCI_EXP_LNKCTL2, lnkctl2); > > + pcie_capability_write_word(dev, PCI_EXP_LNKCTL, lnkctl); > > + } > > +} > > +DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, > > + pcie_downstream_link_retrain); > > +DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_ANY_ID, PCI_ANY_ID, > > + pcie_downstream_link_retrain);
On Fri, Nov 04, 2022 at 12:41:11AM +0100, Pali Rohár wrote: > On Thursday 03 November 2022 18:13:35 Bjorn Helgaas wrote: > > [+cc Pali] > > > > On Sat, Sep 17, 2022 at 01:03:38PM +0100, Maciej W. Rozycki wrote: > > > Attempt to handle cases such as with a downstream port of the ASMedia > > > ASM2824 PCIe switch where link training never completes and the link > > > continues switching between speeds indefinitely with the data link layer > > > never reaching the active state. > > > > > > It has been observed with a downstream port of the ASMedia ASM2824 Gen 3 > > > switch wired to the upstream port of the Pericom PI7C9X2G304 Gen 2 > > > switch, using a Delock Riser Card PCI Express x1 > 2 x PCIe x1 device, > > > P/N 41433, wired to a SiFive HiFive Unmatched board. In this setup the > > > switches are supposed to negotiate the link speed of preferably 5.0GT/s, > > > falling back to 2.5GT/s. > > > > > > Instead the link continues oscillating between the two speeds, at the > > > rate of 34-35 times per second, with link training reported repeatedly > > > active ~84% of the time. Forcibly limiting the target link speed to > > > 2.5GT/s with the upstream ASM2824 device however makes the two switches > > > communicate correctly. Removing the speed restriction afterwards makes > > > the two devices switch to 5.0GT/s then. > > > > > > Make use of these observations then and detect the inability to train > > > the link, by checking for the Data Link Layer Link Active status bit > > > being off while the Link Bandwidth Management Status indicating that > > > hardware has changed the link speed or width in an attempt to correct > > > unreliable link operation. > > > > > > Restrict the speed to 2.5GT/s then with the Target Link Speed field, > > > request a retrain and wait 200ms for the data link to go up. If this > > > turns out successful, then lift the restriction, letting the devices > > > negotiate a higher speed. > > > > > > Also check for a 2.5GT/s speed restriction the firmware may have already > > > arranged and lift it too with ports of devices known to continue working > > > afterwards, currently the ASM2824 only, that already report their data > > > link being up. > > > > This quirk is run at boot-time and resume-time. What happens after a > > Secondary Bus Reset, as is done by pci_reset_secondary_bus()? > > Flipping SBR bit can be done on any PCI-to-PCI bridge device and in this > topology there are following: PCIe Root Port, ASMedia PCIe Switch > Upstream Port, ASMedia PCIe Switch Downstream Port, Pericom PCIe Switch > Upstream Port, Pericom PCIe Switch Downstream Port. > (Maciej, I hope that this is whole topology and there is not some other > device of PCI-to-PCI bridge type in your setup; please correct me) > > Bjorn, to make it clear, on which device you mean to issue secondary bus > reset? IIUC, the problem is observed on the link between the ASM2824 downstream port and the PI7C9X2G304 upstream port, so my question is about asserting SBR on the ASM2824 downstream port. I think that should cause the link between ASM2824 and PI7C9X2G304 to go down and back up. Thanks for the question; I didn't notice before that this quirk applies to *all* devices. I'm a little queasy about trying to fix problems we have not observed. In this case, I think the hardware is *supposed* to establish a link at the highest supported speed automatically. If we need to work around a hardware bug, that's fine, but I'm not sure I want to blindly try to help things along. > Because I would not be surprised if different things happen when issuing > bus reset on different parts of that topology. > > > PCIe r6.0, sec 7.5.1.3.13, says "setting Secondary Bus Reset triggers > > a hot reset on the corresponding PCI Express Port". Sec 4.2.7 says > > LinkUp is 0 in the LTSSM Hot Reset state, and the Hot Reset state > > leads to Detect, so it looks like this reset would cause the link to > > go down and come back up. > > > > Can you tell if that's what happens? Does the link negotiation fail > > then, too? > > > > If it does fail then, I don't know how hard we need to work to fix it. > > Maybe we just accept it? Or maybe we need a "quirk-after-reset" phase > > or something? > > > > Bjorn
Hi Bjorn, Thank you for coming back to this patch series. I'll try to address your concerns, but it may take a little. The reason is I'm currently on site at my lab until the end of the week and barring my day job, etc. I want to focus on items to do (and I do have a bunch) that require local hardware access. The issue concerned with this patch series does not, so I'll get to looking into it in more depth hopefully from next week. For the time being however please see below. On Thu, 3 Nov 2022, Bjorn Helgaas wrote: > > > > Also check for a 2.5GT/s speed restriction the firmware may have already > > > > arranged and lift it too with ports of devices known to continue working > > > > afterwards, currently the ASM2824 only, that already report their data > > > > link being up. > > > > > > This quirk is run at boot-time and resume-time. What happens after a > > > Secondary Bus Reset, as is done by pci_reset_secondary_bus()? > > > > Flipping SBR bit can be done on any PCI-to-PCI bridge device and in this > > topology there are following: PCIe Root Port, ASMedia PCIe Switch > > Upstream Port, ASMedia PCIe Switch Downstream Port, Pericom PCIe Switch > > Upstream Port, Pericom PCIe Switch Downstream Port. > > (Maciej, I hope that this is whole topology and there is not some other > > device of PCI-to-PCI bridge type in your setup; please correct me) There is actually a PCIe-to-PCI bridge device further downstream (device 0000:08:00.0 in the listings below; bus 09 is conventional PCI), but it doesn't matter for the issue concerned; the issue triggers whether the bridge module has been plugged or not. > > Bjorn, to make it clear, on which device you mean to issue secondary bus > > reset? > > IIUC, the problem is observed on the link between the ASM2824 > downstream port and the PI7C9X2G304 upstream port, so my question is > about asserting SBR on the ASM2824 downstream port. I think that > should cause the link between ASM2824 and PI7C9X2G304 to go down and > back up. That would be my expectation as well. Is there a reliable way to request that however without actually writing a piece of code to do so from inside the kernel? Sadly our documentation is vague on the matter, in particular Documentation/ABI/testing/sysfs-bus-pci, but here's what I have obtained: # lspci -t -[0000:00]---00.0-[01-0b]----00.0-[02-0b]--+-00.0-[03]-- +-02.0-[04]----00.0 +-03.0-[05-09]----00.0-[06-09]--+-01.0-[07]--+-00.0 | | \-00.3 | \-02.0-[08-09]----00.0-[09]--+-01.0 | \-02.0 +-04.0-[0a]----00.0 \-08.0-[0b]--+-00.0 \-00.1 # for name in /sys/bus/pci/devices/0000\:??\:??.?/reset_method; do echo "$(basename $(dirname $name)): $(cat $name)"; done 0000:01:00.0: pm bus 0000:02:00.0: pm bus 0000:02:02.0: pm 0000:02:03.0: pm 0000:02:04.0: pm 0000:02:08.0: pm 0000:04:00.0: bus 0000:05:00.0: bus 0000:06:01.0: bus 0000:07:00.0: bus 0000:08:00.0: bus 0000:09:01.0: pm bus 0000:0a:00.0: flr bus 0000:0b:00.0: pm bus 0000:0b:00.1: pm # (mind that the problematic link is between 0000:02:03.0 and 0000:05:00.0), and then: # echo 1 >/sys/bus/pci/devices/0000\:05\:00.0/reset -sh: echo: write error: Inappropriate ioctl for device # (which I gather is supposed to poke at 0000:02:03.0's SBR) so it doesn't seem to be effective. Needless to say: # echo 1 >/sys/bus/pci/devices/0000\:02\:03.0/reset # while apparently succeeds doesn't do anything spectacular. Any suggestions? In any case I would expect resetting a bus tree to cause all the quirks required to be reapplied, preferably after bus re-enumeration even as the topology can change afterwards if not for the hardware erratum handled here, although indeed I would find it reasonable to only perform the actions on the bus subtree reset and not it's parent. In that case the quirk would have to be applied by hand somehow. > Thanks for the question; I didn't notice before that this quirk > applies to *all* devices. I'm a little queasy about trying to fix > problems we have not observed. In this case, I think the hardware is > *supposed* to establish a link at the highest supported speed > automatically. I believe hardware is supposed to establish a link at all in the first place (i.e. initially at 2.5GT/s, and then possibly upgrade afterwards as capabilities have been exchanged between the endpoints), however as this case indicates this is not necessarily universally guaranteed. > If we need to work around a hardware bug, that's fine, but I'm not > sure I want to blindly try to help things along. I have chewed it over before choosing this approach and did this for what I think is a very good reason, first for U-Boot and then, when I figured out a feasible approach, also for Linux. Bear in mind that we have a pair of devices that completely fail to negotiate link ever by default and we do not know whether it's the upstream device (which we can see) or the downstream device (which we can't, because the link is down) that is the culprit. With that we have as follows. 1. Both devices in question are PCIe switches, so they can be swapped with each other. In that case what is now the upstream device will become the downstream device and vice versa. Now the Pericom/Diodes switch only has 2.5GT/s downstream ports, so the issue by definition cannot trigger, but companies/designers tend to reuse hardware IP blocks, and I am quite sure Pericom/Diodes has switches in its product line that support rates beyond 2.5GT/s in their downstream ports. So a swapped scenario is in my opinion quite real. Matching on the vendor:device ID won't work then as the suspect ASMedia part will not be seen due to a failed link. If it's the Pericom/Diodes switch that is the culprit, then it's even worse, as it is the downstream device already. 2. We don't know really what the ultimate cause is and therefore whether the issue can trigger with other vendor devices (and presumably one of the two involved here). And the nature of the failure is such, that I think it is very unlikely a user will reach out to our (Linux kernel) community for a workaround. It does not look like a software problem at all and neither like one that can be solved by software. Therefore they will most likely conclude hardware they have is broken and throw it out causing frustration even though the hardware might be (im)perfectly capable of working, just requiring a little poke to get going. Just as I almost threw it out even though I dare say I am quite an experienced engineer, reasonably familiar with odd hardware and the Linux kernel and ultimately I was able to come up with a solution. It is only because I noticed by chance that 0000:02:03.0 device's Link Training bit is flipping, combined with my imagination as to how to possibly deal with such a situation that I succeeded in bringing the link up. I genuinely thought all was lost and I was exceedingly happy when I overcame it as the PI7C9X2G304 switch module was key to my project with no suitable alternative on the horizon. 3. We apply this quirk to failed links only. As a non-working link is a fatal condition, things cannot get worse by trying to retrain it by hand at 2.5GT/s, as otherwise it doesn't work anyway. So it's just a last ditch attempt to make it run really. Can you think of a scenario where retraining a non-working link would cause more damage than leaving it alone in the non-working state? This quirk only uses generic PCIe features, common to all hardware, and therefore I think it can be applied in a generic manner. Now there could be broken hardware that does not correctly handle the generic features used here, but if it has a failed link too, then it will just continue not to work. So it won't be a regression. Now if I made any mistake in getting the conditions wrong that are supposed to determine that a link is non-working, than I'll be glad to have the code corrected. Such a case shouldn't determine the overall validity of the approach I propose here though. NB I have included code to lift the 2.5GT/s restriction, matched by the downstream port's vendor:device ID, but now that I've been thinking about this issue again I suspect it might be too liberal, because we don't really know if it is safe to do so if the downstream device is not a PI7C9X2G304. So perhaps we need to match on the vendor:device IDs of both endpoints of such a broken link. What do you think? I'll let you chew my reasoning over now too, as I have an issue with a non-working PCIe parallel port device to sort out (device 0000:07:00.0 above) and that actually requires flipping rocker switches in my lab, which I cannot do remotely. Thank you for your review. Maciej
[+cc Alex, in case he has any reset-related comments] On Wed, Nov 09, 2022 at 02:57:57AM +0000, Maciej W. Rozycki wrote: > Hi Bjorn, > > Thank you for coming back to this patch series. I'll try to address your > concerns, but it may take a little. The reason is I'm currently on site > at my lab until the end of the week and barring my day job, etc. I want to > focus on items to do (and I do have a bunch) that require local hardware > access. The issue concerned with this patch series does not, so I'll get > to looking into it in more depth hopefully from next week. For the time > being however please see below. > > On Thu, 3 Nov 2022, Bjorn Helgaas wrote: > > > > > > Also check for a 2.5GT/s speed restriction the firmware may have already > > > > > arranged and lift it too with ports of devices known to continue working > > > > > afterwards, currently the ASM2824 only, that already report their data > > > > > link being up. > > > > > > > > This quirk is run at boot-time and resume-time. What happens after a > > > > Secondary Bus Reset, as is done by pci_reset_secondary_bus()? > > > > > > Flipping SBR bit can be done on any PCI-to-PCI bridge device and in this > > > topology there are following: PCIe Root Port, ASMedia PCIe Switch > > > Upstream Port, ASMedia PCIe Switch Downstream Port, Pericom PCIe Switch > > > Upstream Port, Pericom PCIe Switch Downstream Port. > > > (Maciej, I hope that this is whole topology and there is not some other > > > device of PCI-to-PCI bridge type in your setup; please correct me) > > There is actually a PCIe-to-PCI bridge device further downstream (device > 0000:08:00.0 in the listings below; bus 09 is conventional PCI), but it > doesn't matter for the issue concerned; the issue triggers whether the > bridge module has been plugged or not. > > > > Bjorn, to make it clear, on which device you mean to issue secondary bus > > > reset? > > > > IIUC, the problem is observed on the link between the ASM2824 > > downstream port and the PI7C9X2G304 upstream port, so my question is > > about asserting SBR on the ASM2824 downstream port. I think that > > should cause the link between ASM2824 and PI7C9X2G304 to go down and > > back up. > > That would be my expectation as well. Is there a reliable way to request > that however without actually writing a piece of code to do so from inside > the kernel? Sadly our documentation is vague on the matter, in particular > Documentation/ABI/testing/sysfs-bus-pci, but here's what I have obtained: > > # lspci -t > -[0000:00]---00.0-[01-0b]----00.0-[02-0b]--+-00.0-[03]-- > +-02.0-[04]----00.0 > +-03.0-[05-09]----00.0-[06-09]--+-01.0-[07]--+-00.0 > | | \-00.3 > | \-02.0-[08-09]----00.0-[09]--+-01.0 > | \-02.0 > +-04.0-[0a]----00.0 > \-08.0-[0b]--+-00.0 > \-00.1 > # for name in /sys/bus/pci/devices/0000\:??\:??.?/reset_method; do echo "$(basename $(dirname $name)): $(cat $name)"; done > 0000:01:00.0: pm bus > 0000:02:00.0: pm bus > 0000:02:02.0: pm > 0000:02:03.0: pm > 0000:02:04.0: pm > 0000:02:08.0: pm > 0000:04:00.0: bus > 0000:05:00.0: bus > 0000:06:01.0: bus > 0000:07:00.0: bus > 0000:08:00.0: bus > 0000:09:01.0: pm bus > 0000:0a:00.0: flr bus > 0000:0b:00.0: pm bus > 0000:0b:00.1: pm > # > > (mind that the problematic link is between 0000:02:03.0 and 0000:05:00.0), > and then: > > # echo 1 >/sys/bus/pci/devices/0000\:05\:00.0/reset > -sh: echo: write error: Inappropriate ioctl for device > # > > (which I gather is supposed to poke at 0000:02:03.0's SBR) so it doesn't > seem to be effective. 05:00.0 supports the "bus" method, i.e., pci_reset_bus_function(), which tries pci_dev_reset_slot_function() followed by pci_parent_bus_reset(). Both of them return -ENOTTY if the device (05:00.0) has a secondary bus ("dev->subordinate"), so I think nothing happens here. This was prompted by my question about whether the link comes up correctly after an SBR. It looks like it's not a problem when 02:03.0 and 05:00.0 are both switches because we don't use SBR in that case. But if 05:00.0 were an endpoint instead of a switch, we *would* use SBR, and then (I think) the link would go down, and this quirk would not be applied. > Needless to say: > > # echo 1 >/sys/bus/pci/devices/0000\:02\:03.0/reset > # > > while apparently succeeds doesn't do anything spectacular. Any > suggestions? > > In any case I would expect resetting a bus tree to cause all the quirks > required to be reapplied, preferably after bus re-enumeration even as the > topology can change afterwards if not for the hardware erratum handled > here, although indeed I would find it reasonable to only perform the > actions on the bus subtree reset and not it's parent. In that case the > quirk would have to be applied by hand somehow. A lot of things can change after a reset, especially if it makes a device reload updated firmware, but we don't actually re-enumerate or reapply quirks. I'm looking at pci_reset_function(), where we basically save the parts of config space we know about, do the reset, and restore config space. > > Thanks for the question; I didn't notice before that this quirk > > applies to *all* devices. I'm a little queasy about trying to fix > > problems we have not observed. In this case, I think the hardware is > > *supposed* to establish a link at the highest supported speed > > automatically. > > I believe hardware is supposed to establish a link at all in the first > place (i.e. initially at 2.5GT/s, and then possibly upgrade afterwards as > capabilities have been exchanged between the endpoints), however as this > case indicates this is not necessarily universally guaranteed. > > > If we need to work around a hardware bug, that's fine, but I'm not > > sure I want to blindly try to help things along. > > I have chewed it over before choosing this approach and did this for what > I think is a very good reason, first for U-Boot and then, when I figured > out a feasible approach, also for Linux. Bear in mind that we have a pair > of devices that completely fail to negotiate link ever by default and we > do not know whether it's the upstream device (which we can see) or the > downstream device (which we can't, because the link is down) that is the > culprit. With that we have as follows. > > 1. Both devices in question are PCIe switches, so they can be swapped with > each other. In that case what is now the upstream device will become > the downstream device and vice versa. Now the Pericom/Diodes switch > only has 2.5GT/s downstream ports, so the issue by definition cannot > trigger, but companies/designers tend to reuse hardware IP blocks, and > I am quite sure Pericom/Diodes has switches in its product line that > support rates beyond 2.5GT/s in their downstream ports. So a swapped > scenario is in my opinion quite real. Matching on the vendor:device ID > won't work then as the suspect ASMedia part will not be seen due to a > failed link. If it's the Pericom/Diodes switch that is the culprit, > then it's even worse, as it is the downstream device already. > > 2. We don't know really what the ultimate cause is and therefore whether > the issue can trigger with other vendor devices (and presumably one of > the two involved here). And the nature of the failure is such, that I > think it is very unlikely a user will reach out to our (Linux kernel) > community for a workaround. It does not look like a software problem > at all and neither like one that can be solved by software. > > Therefore they will most likely conclude hardware they have is broken > and throw it out causing frustration even though the hardware might be > (im)perfectly capable of working, just requiring a little poke to get > going. > > Just as I almost threw it out even though I dare say I am quite an > experienced engineer, reasonably familiar with odd hardware and the > Linux kernel and ultimately I was able to come up with a solution. It > is only because I noticed by chance that 0000:02:03.0 device's Link > Training bit is flipping, combined with my imagination as to how to > possibly deal with such a situation that I succeeded in bringing the > link up. I genuinely thought all was lost and I was exceedingly happy > when I overcame it as the PI7C9X2G304 switch module was key to my > project with no suitable alternative on the horizon. > > 3. We apply this quirk to failed links only. As a non-working link is a > fatal condition, things cannot get worse by trying to retrain it by > hand at 2.5GT/s, as otherwise it doesn't work anyway. So it's just a > last ditch attempt to make it run really. > > Can you think of a scenario where retraining a non-working link would > cause more damage than leaving it alone in the non-working state? > > This quirk only uses generic PCIe features, common to all hardware, and > therefore I think it can be applied in a generic manner. Now there > could be broken hardware that does not correctly handle the generic > features used here, but if it has a failed link too, then it will just > continue not to work. So it won't be a regression. The first big chunk of the quirk (that retrains the link) only runs on Downstream Ports, and only when PCI_EXP_LNKSTA_DLLLA is clear, i.e., when the link is down. What happens if this Downstream Port doesn't have anything below it, e.g., it leads to an empty slot? Is there something that prevents us from attempting a retrain and waiting for the timeout? I was about to say the second chunk is always run even when the link was up, but now I see that it's only for the ASMedia ASM2824. So this part is more like a traditional quirk that could specify that device. If we think the first part (attempting the retrain) is safe to do whenever the link is down, maybe we should do that more directly as part of the PCI core instead of as a quirk? For anybody joining late, the patch is here: https://lore.kernel.org/r/alpine.DEB.2.21.2209130050380.60554@angie.orcam.me.uk > Now if I made any mistake in getting the conditions wrong that are > supposed to determine that a link is non-working, than I'll be glad to > have the code corrected. Such a case shouldn't determine the overall > validity of the approach I propose here though. > > NB I have included code to lift the 2.5GT/s restriction, matched by the > downstream port's vendor:device ID, but now that I've been thinking about > this issue again I suspect it might be too liberal, because we don't > really know if it is safe to do so if the downstream device is not a > PI7C9X2G304. So perhaps we need to match on the vendor:device IDs of both > endpoints of such a broken link. What do you think? > > I'll let you chew my reasoning over now too, as I have an issue with a > non-working PCIe parallel port device to sort out (device 0000:07:00.0 > above) and that actually requires flipping rocker switches in my lab, > which I cannot do remotely. Thank you for your review. > > Maciej
On Tue, 8 Nov 2022 23:04:18 -0600 Bjorn Helgaas <helgaas@kernel.org> wrote: > [+cc Alex, in case he has any reset-related comments] > > On Wed, Nov 09, 2022 at 02:57:57AM +0000, Maciej W. Rozycki wrote: > > Hi Bjorn, > > > > Thank you for coming back to this patch series. I'll try to address your > > concerns, but it may take a little. The reason is I'm currently on site > > at my lab until the end of the week and barring my day job, etc. I want to > > focus on items to do (and I do have a bunch) that require local hardware > > access. The issue concerned with this patch series does not, so I'll get > > to looking into it in more depth hopefully from next week. For the time > > being however please see below. > > > > On Thu, 3 Nov 2022, Bjorn Helgaas wrote: > > > > > > > > Also check for a 2.5GT/s speed restriction the firmware may have already > > > > > > arranged and lift it too with ports of devices known to continue working > > > > > > afterwards, currently the ASM2824 only, that already report their data > > > > > > link being up. > > > > > > > > > > This quirk is run at boot-time and resume-time. What happens after a > > > > > Secondary Bus Reset, as is done by pci_reset_secondary_bus()? > > > > > > > > Flipping SBR bit can be done on any PCI-to-PCI bridge device and in this > > > > topology there are following: PCIe Root Port, ASMedia PCIe Switch > > > > Upstream Port, ASMedia PCIe Switch Downstream Port, Pericom PCIe Switch > > > > Upstream Port, Pericom PCIe Switch Downstream Port. > > > > (Maciej, I hope that this is whole topology and there is not some other > > > > device of PCI-to-PCI bridge type in your setup; please correct me) > > > > There is actually a PCIe-to-PCI bridge device further downstream (device > > 0000:08:00.0 in the listings below; bus 09 is conventional PCI), but it > > doesn't matter for the issue concerned; the issue triggers whether the > > bridge module has been plugged or not. > > > > > > Bjorn, to make it clear, on which device you mean to issue secondary bus > > > > reset? > > > > > > IIUC, the problem is observed on the link between the ASM2824 > > > downstream port and the PI7C9X2G304 upstream port, so my question is > > > about asserting SBR on the ASM2824 downstream port. I think that > > > should cause the link between ASM2824 and PI7C9X2G304 to go down and > > > back up. > > > > That would be my expectation as well. Is there a reliable way to request > > that however without actually writing a piece of code to do so from inside > > the kernel? Sadly our documentation is vague on the matter, in particular > > Documentation/ABI/testing/sysfs-bus-pci, but here's what I have obtained: > > > > # lspci -t > > -[0000:00]---00.0-[01-0b]----00.0-[02-0b]--+-00.0-[03]-- > > +-02.0-[04]----00.0 > > +-03.0-[05-09]----00.0-[06-09]--+-01.0-[07]--+-00.0 > > | | \-00.3 > > | \-02.0-[08-09]----00.0-[09]--+-01.0 > > | \-02.0 > > +-04.0-[0a]----00.0 > > \-08.0-[0b]--+-00.0 > > \-00.1 > > # for name in /sys/bus/pci/devices/0000\:??\:??.?/reset_method; do echo "$(basename $(dirname $name)): $(cat $name)"; done > > 0000:01:00.0: pm bus > > 0000:02:00.0: pm bus > > 0000:02:02.0: pm > > 0000:02:03.0: pm > > 0000:02:04.0: pm > > 0000:02:08.0: pm > > 0000:04:00.0: bus > > 0000:05:00.0: bus > > 0000:06:01.0: bus > > 0000:07:00.0: bus > > 0000:08:00.0: bus > > 0000:09:01.0: pm bus > > 0000:0a:00.0: flr bus > > 0000:0b:00.0: pm bus > > 0000:0b:00.1: pm > > # > > > > (mind that the problematic link is between 0000:02:03.0 and 0000:05:00.0), > > and then: > > > > # echo 1 >/sys/bus/pci/devices/0000\:05\:00.0/reset > > -sh: echo: write error: Inappropriate ioctl for device > > # > > > > (which I gather is supposed to poke at 0000:02:03.0's SBR) so it doesn't > > seem to be effective. > > 05:00.0 supports the "bus" method, i.e., pci_reset_bus_function(), > which tries pci_dev_reset_slot_function() followed by > pci_parent_bus_reset(). Both of them return -ENOTTY if the device > (05:00.0) has a secondary bus ("dev->subordinate"), so I think nothing > happens here. Right, the pci-sysfs reset attribute is only meant for a reset scope limited to the device, we'd need something to call pci_reset_bus() to commit to the whole hierarchy, which is not something we typically do. vfio-pci will only bind to endpoint devices, so it shouldn't provide an interface to inject a bus reset here either. Based on the fact that there's a pericom switch in play here, I'll just note that I think this is the same device with other link speed issues as well: https://lore.kernel.org/all/20161026180140.23495.27388.stgit@gimli.home/ This fell off my plate some time ago, but as noted there, enabling ACS when the upstream and downstream ports run at different link rates exposes errata where packets are queued and not delivered within the switch. Could enabling ACS on this device be contributing to the issue here, for example triggering the Asmedia downstream port to get into this link reseting issue? A test with pci=disable_acs_redir=0000:06:01.0;0000:06:02.0 could be interesting assuming this occurs on an platform that has an IOMMU, ie. calls pci_request_acs(). Thanks, Alex
On Tue, 8 Nov 2022, Bjorn Helgaas wrote: > > That would be my expectation as well. Is there a reliable way to request > > that however without actually writing a piece of code to do so from inside > > the kernel? Sadly our documentation is vague on the matter, in particular > > Documentation/ABI/testing/sysfs-bus-pci, but here's what I have obtained: > > > > # lspci -t > > -[0000:00]---00.0-[01-0b]----00.0-[02-0b]--+-00.0-[03]-- > > +-02.0-[04]----00.0 > > +-03.0-[05-09]----00.0-[06-09]--+-01.0-[07]--+-00.0 > > | | \-00.3 > > | \-02.0-[08-09]----00.0-[09]--+-01.0 > > | \-02.0 > > +-04.0-[0a]----00.0 > > \-08.0-[0b]--+-00.0 > > \-00.1 > > # for name in /sys/bus/pci/devices/0000\:??\:??.?/reset_method; do echo "$(basename $(dirname $name)): $(cat $name)"; done > > 0000:01:00.0: pm bus > > 0000:02:00.0: pm bus > > 0000:02:02.0: pm > > 0000:02:03.0: pm > > 0000:02:04.0: pm > > 0000:02:08.0: pm > > 0000:04:00.0: bus > > 0000:05:00.0: bus > > 0000:06:01.0: bus > > 0000:07:00.0: bus > > 0000:08:00.0: bus > > 0000:09:01.0: pm bus > > 0000:0a:00.0: flr bus > > 0000:0b:00.0: pm bus > > 0000:0b:00.1: pm > > # > > > > (mind that the problematic link is between 0000:02:03.0 and 0000:05:00.0), > > and then: > > > > # echo 1 >/sys/bus/pci/devices/0000\:05\:00.0/reset > > -sh: echo: write error: Inappropriate ioctl for device > > # > > > > (which I gather is supposed to poke at 0000:02:03.0's SBR) so it doesn't > > seem to be effective. > > 05:00.0 supports the "bus" method, i.e., pci_reset_bus_function(), > which tries pci_dev_reset_slot_function() followed by > pci_parent_bus_reset(). Both of them return -ENOTTY if the device > (05:00.0) has a secondary bus ("dev->subordinate"), so I think nothing > happens here. > > This was prompted by my question about whether the link comes up > correctly after an SBR. It looks like it's not a problem when 02:03.0 > and 05:00.0 are both switches because we don't use SBR in that case. > > But if 05:00.0 were an endpoint instead of a switch, we *would* use > SBR, and then (I think) the link would go down, and this quirk would > not be applied. After an SBR the link goes down, but the situation is actually somewhat more complex than you describe. Mind that the Target Link Speed field in the Link Control 2 register is sticky. Consequently if the link goes down with TLS set to 2.5GT/s then it comes back up just fine, because it's still clamped at 2.5GT/s after reset. The U-Boot workaround takes advantage of this observation and leaves TLS at 2.5GT/s. This way if an OS is booted that is unaware of this erratum and resets the PCI/e hierarchy then U-Boot's workaround remains in action and the link resumes operation. Now when TLS is reset back to 5GT/s or higher, then the link goes down and starts its infinite training dance again. This happens when TLS has been reset by hand in U-boot and Linux is booted and resets the PCI/e hierarchy in the course (but then our quirk chimes in), or under running Linux with this change applied when an SBR is issued via `setpci' or by a modified kernel that has the `dev->subordinate' condition removed. > > Needless to say: > > > > # echo 1 >/sys/bus/pci/devices/0000\:02\:03.0/reset > > # > > > > while apparently succeeds doesn't do anything spectacular. Any > > suggestions? > > > > In any case I would expect resetting a bus tree to cause all the quirks > > required to be reapplied, preferably after bus re-enumeration even as the > > topology can change afterwards if not for the hardware erratum handled > > here, although indeed I would find it reasonable to only perform the > > actions on the bus subtree reset and not it's parent. In that case the > > quirk would have to be applied by hand somehow. > > A lot of things can change after a reset, especially if it makes a > device reload updated firmware, but we don't actually re-enumerate or > reapply quirks. I'm looking at pci_reset_function(), where we > basically save the parts of config space we know about, do the reset, > and restore config space. Right. That's what the debug messages I have observed indicated as well. For the record, I made an experiment and with the `dev->subordinate' condition removed I reset device 05:00.0 in my system via: # echo 1 >/sys/bus/pci/devices/0000\:05\:00.0/reset With the TLS reset back to its 8GT/s default here the link did not come back up as expected and consequently no device from this bus subhierarchy was accessible anymore: # lspci -s 05: -vnn 05:00.0 PCI bridge [0604]: Pericom Semiconductor PI7C9X2G304 EL/SL PCIe2 3-Port/4-Lane Packet Switch [12d8:2304] (rev ff) (prog-if ff) !!! Unknown header type 7f Retraining by hand and then rescanning brought the subhierarchy back: # setpci -s 02\:03.0 CAP_EXP+0x30.W=0061; setpci -s 02\:03.0 CAP_EXP+0x10.W=0020; sleep 1; setpci -s 02\:03.0 CAP_EXP+0x30.W=0063; setpci -s 02\:03.0 CAP_EXP+0x10.W=0020 # echo 1 >/sys/bus/pci/devices/0000\:02\:03.0/rescan pci_bus 0000:03: busn_res: [bus 03] end is updated to 03 pci_bus 0000:04: busn_res: [bus 04] end is updated to 04 pci 0000:05:00.0: bridge configuration invalid ([bus ff-ff]), reconfiguring pcieport 0000:06:01.0: bridge configuration invalid ([bus ff-ff]), reconfiguring pcieport 0000:06:02.0: bridge configuration invalid ([bus ff-ff]), reconfiguring pci_bus 0000:07: busn_res: [bus 07] end is updated to 07 pci 0000:08:00.0: bridge configuration invalid ([bus ff-ff]), reconfiguring pci_bus 0000:09: busn_res: [bus 09] end is updated to 09 pci_bus 0000:08: busn_res: [bus 08-09] end is updated to 09 pci_bus 0000:06: busn_res: [bus 06-09] end is updated to 09 pci_bus 0000:05: busn_res: [bus 05-09] end is updated to 09 pci_bus 0000:0a: busn_res: [bus 0a] end is updated to 0a pci_bus 0000:0b: busn_res: [bus 0b] end is updated to 0b # lspci -s 05: -vnn 05:00.0 PCI bridge [0604]: Pericom Semiconductor PI7C9X2G304 EL/SL PCIe2 3-Port/4-Lane Packet Switch [12d8:2304] (rev 05) (prog-if 00 [Normal decode]) Flags: fast devsel Bus: primary=00, secondary=00, subordinate=00, sec-latency=0 I/O behind bridge: 00000000-00000fff [size=4K] Memory behind bridge: 00000000-000fffff [size=1M] Prefetchable memory behind bridge: 0000000000000000-00000000000fffff [size=1M] Capabilities: [40] Power Management version 3 Capabilities: [5c] Vital Product Data Capabilities: [64] Vendor Specific Information: Len=34 <?> Capabilities: [b0] Subsystem: Device [0000:0000] Capabilities: [c0] Express Upstream Port, MSI 00 Capabilities: [100] Advanced Error Reporting Capabilities: [140] Virtual Channel Capabilities: [20c] Power Budgeting <?> Capabilities: [230] Latency Tolerance Reporting (though bridge BARs aren't exactly correct anymore). With the TLS clamped at 2.5GT/s the link did come back up and the bus subhierarchy remained accessible except for a downstream conventional PCI bus: # lspci -s 09: -vnn 09:01.0 FDDI network controller [0202]: Digital Equipment Corporation PCI-to-PDQ Interface Chip [PFI] FDDI (DEFPA) [1011:000f] (rev ff) (prog-if ff) !!! Unknown header type 7f Kernel driver in use: defxx 09:02.0 ATM network controller [0203]: Microsemi / PMC / IDT IDT77201/77211 155Mbps ATM SAR Controller [NICStAR] [111d:0001] (rev ff) (prog-if ff) !!! Unknown header type 7f Kernel driver in use: nicstar which had to be restored by hand: # echo 1 >/sys/bus/pci/devices/0000\:08\:00.0/rescan pci_bus 0000:08: scanning bus pci 0000:08:00.0: scanning [bus 00-00] behind bridge, pass 0 pci 0000:08:00.0: bridge configuration invalid ([bus 00-00]), reconfiguring pci 0000:08:00.0: scanning [bus 00-00] behind bridge, pass 1 pci_bus 0000:09: scanning bus pci_bus 0000:09: bus scan returning with max=09 pci_bus 0000:09: busn_res: [bus 09] end is updated to 09 pci_bus 0000:08: bus scan returning with max=09 # lspci -s 09: -vnn 09:01.0 FDDI network controller [0202]: Digital Equipment Corporation PCI-to-PDQ Interface Chip [PFI] FDDI (DEFPA) [1011:000f] (rev 02) Subsystem: Digital Equipment Corporation FDDIcontroller/PCI (DEFPA) [1011:def1] Flags: bus master, medium devsel, latency 64, IRQ 24 Memory at 60831000 (32-bit, non-prefetchable) [size=128] I/O ports at 2100 [size=128] Memory at 60820000 (32-bit, non-prefetchable) [size=64K] Capabilities: [50] Power Management version 2 Kernel driver in use: defxx 09:02.0 ATM network controller [0203]: Microsemi / PMC / IDT IDT77201/77211 155Mbps ATM SAR Controller [NICStAR] [111d:0001] (rev 03) Flags: bus master, medium devsel, latency 64, IRQ 26 I/O ports at 2000 [size=256] Memory at 60830000 (32-bit, non-prefetchable) [size=4K] Expansion ROM at 60800000 [virtual] [disabled] [size=128K] Kernel driver in use: nicstar > > I have chewed it over before choosing this approach and did this for what > > I think is a very good reason, first for U-Boot and then, when I figured > > out a feasible approach, also for Linux. Bear in mind that we have a pair > > of devices that completely fail to negotiate link ever by default and we > > do not know whether it's the upstream device (which we can see) or the > > downstream device (which we can't, because the link is down) that is the > > culprit. With that we have as follows. > > > > 1. Both devices in question are PCIe switches, so they can be swapped with > > each other. In that case what is now the upstream device will become > > the downstream device and vice versa. Now the Pericom/Diodes switch > > only has 2.5GT/s downstream ports, so the issue by definition cannot > > trigger, but companies/designers tend to reuse hardware IP blocks, and > > I am quite sure Pericom/Diodes has switches in its product line that > > support rates beyond 2.5GT/s in their downstream ports. So a swapped > > scenario is in my opinion quite real. Matching on the vendor:device ID > > won't work then as the suspect ASMedia part will not be seen due to a > > failed link. If it's the Pericom/Diodes switch that is the culprit, > > then it's even worse, as it is the downstream device already. > > > > 2. We don't know really what the ultimate cause is and therefore whether > > the issue can trigger with other vendor devices (and presumably one of > > the two involved here). And the nature of the failure is such, that I > > think it is very unlikely a user will reach out to our (Linux kernel) > > community for a workaround. It does not look like a software problem > > at all and neither like one that can be solved by software. > > > > Therefore they will most likely conclude hardware they have is broken > > and throw it out causing frustration even though the hardware might be > > (im)perfectly capable of working, just requiring a little poke to get > > going. > > > > Just as I almost threw it out even though I dare say I am quite an > > experienced engineer, reasonably familiar with odd hardware and the > > Linux kernel and ultimately I was able to come up with a solution. It > > is only because I noticed by chance that 0000:02:03.0 device's Link > > Training bit is flipping, combined with my imagination as to how to > > possibly deal with such a situation that I succeeded in bringing the > > link up. I genuinely thought all was lost and I was exceedingly happy > > when I overcame it as the PI7C9X2G304 switch module was key to my > > project with no suitable alternative on the horizon. > > > > 3. We apply this quirk to failed links only. As a non-working link is a > > fatal condition, things cannot get worse by trying to retrain it by > > hand at 2.5GT/s, as otherwise it doesn't work anyway. So it's just a > > last ditch attempt to make it run really. > > > > Can you think of a scenario where retraining a non-working link would > > cause more damage than leaving it alone in the non-working state? > > > > This quirk only uses generic PCIe features, common to all hardware, and > > therefore I think it can be applied in a generic manner. Now there > > could be broken hardware that does not correctly handle the generic > > features used here, but if it has a failed link too, then it will just > > continue not to work. So it won't be a regression. > > The first big chunk of the quirk (that retrains the link) only runs on > Downstream Ports, and only when PCI_EXP_LNKSTA_DLLLA is clear, i.e., > when the link is down. What happens if this Downstream Port doesn't > have anything below it, e.g., it leads to an empty slot? Is there > something that prevents us from attempting a retrain and waiting for > the timeout? The conditions I came up with for the detection of a failed link negotiation are supposed to tell this situation apart from an absent endpoint on the other end of a link. Indeed in the system affected there is the 02:00.0 downstream port device within the same ASM2824 switch that is not connected to anything (no slot there or an onboard device; essentially just a wasted x2 downstream port the board designers could not come up with a use for) and the quirk just does not trigger for it, because the conditions it checks are not met for an unconnected link. Specifically the Link Bandwidth Management Status bit in the Link Status register cannot be set to 1 for an unconnected link, because setting the bit requires a link to hold the DL_Up status and that does not happen when there is no component on the other side of the link. That's at least my understanding. I have no immediate way to verify other ports, because I am away from my lab now and the system has its PCI/e hierarchy otherwise fully occupied, but the behaviour was consistent as I made extensive verification earlier on. The same condition is used by the U-Boot workaround BTW. > I was about to say the second chunk is always run even when the link > was up, but now I see that it's only for the ASMedia ASM2824. So this > part is more like a traditional quirk that could specify that device. > > If we think the first part (attempting the retrain) is safe to do > whenever the link is down, maybe we should do that more directly as > part of the PCI core instead of as a quirk? At this point in my understanding of the situation I tend to agree. This will guarantee that device resets issued under our control will do the right thing (we can't do anything I suppose about requests issued by `setpci' behind our back) and is also what the U-Boot workaround I have come up with does. I'll make v6 along these lines. Thank you for your inquisitiveness and the review. Maciej
On Wed, 9 Nov 2022, Alex Williamson wrote: > > 05:00.0 supports the "bus" method, i.e., pci_reset_bus_function(), > > which tries pci_dev_reset_slot_function() followed by > > pci_parent_bus_reset(). Both of them return -ENOTTY if the device > > (05:00.0) has a secondary bus ("dev->subordinate"), so I think nothing > > happens here. > > Right, the pci-sysfs reset attribute is only meant for a reset scope > limited to the device, we'd need something to call pci_reset_bus() to > commit to the whole hierarchy, which is not something we typically do. > vfio-pci will only bind to endpoint devices, so it shouldn't provide an > interface to inject a bus reset here either. > > Based on the fact that there's a pericom switch in play here, I'll just > note that I think this is the same device with other link speed issues > as well: > > https://lore.kernel.org/all/20161026180140.23495.27388.stgit@gimli.home/ Thanks for the pointer. This has been superseded by commit acd61ffb2f16 ("PCI: Add ACS quirk for Pericom PI7C9X2G switches"), right? In which case it is a match ([12d8:2304]), though the quirk does not trigger here, i.e. no message is printed about store-forward mode activation: pcieport 0000:05:00.0: calling pci_fixup_pericom_acs_store_forward+0x0/0xba @ 1 pcieport 0000:05:00.0: pci_fixup_pericom_acs_store_forward+0x0/0xba took 0 usecs [...] pci 0000:05:00.0: calling pci_fixup_pericom_acs_store_forward+0x0/0xba @ 1 pci 0000:05:00.0: pci_fixup_pericom_acs_store_forward+0x0/0xba took 0 usecs [...] pcieport 0000:06:01.0: calling pci_fixup_pericom_acs_store_forward+0x0/0xba @ 1 pcieport 0000:06:01.0: pci_fixup_pericom_acs_store_forward+0x0/0xba took 3 usecs [...] pcieport 0000:06:02.0: calling pci_fixup_pericom_acs_store_forward+0x0/0xba @ 1 pcieport 0000:06:02.0: pci_fixup_pericom_acs_store_forward+0x0/0xba took 2 usecs NB I don't know why the quirk for the upstream port (05:00.0) is called twice, both via pcieport and via pci. > This fell off my plate some time ago, but as noted there, enabling ACS > when the upstream and downstream ports run at different link rates > exposes errata where packets are queued and not delivered within the > switch. > > Could enabling ACS on this device be contributing to the issue here, > for example triggering the Asmedia downstream port to get into this > link reseting issue? A test with > pci=disable_acs_redir=0000:06:01.0;0000:06:02.0 could be interesting > assuming this occurs on an platform that has an IOMMU, ie. calls > pci_request_acs(). Thanks, We have no IOMMU support for any RISC-V machine at the moment: config ARCH_RV64I [...] select SWIOTLB if MMU and: software IO TLB: area num 4. software IO TLB: mapped [mem 0x00000000fb732000-0x00000000ff732000] (64MB) so IIUC this issue does not apply. Thank you for your input. Maciej
Index: linux-macro/drivers/pci/quirks.c =================================================================== --- linux-macro.orig/drivers/pci/quirks.c +++ linux-macro/drivers/pci/quirks.c @@ -5956,3 +5956,121 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_I DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c0, aspm_l1_acceptable_latency); DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c1, aspm_l1_acceptable_latency); #endif + +/* + * Retrain the link of a downstream PCIe port by hand if necessary. + * + * This is needed at least where a downstream port of the ASMedia ASM2824 + * Gen 3 switch is wired to the upstream port of the Pericom PI7C9X2G304 + * Gen 2 switch, and observed with the Delock Riser Card PCI Express x1 > + * 2 x PCIe x1 device, P/N 41433, plugged into the SiFive HiFive Unmatched + * board. + * + * In such a configuration the switches are supposed to negotiate the link + * speed of preferably 5.0GT/s, falling back to 2.5GT/s. However the link + * continues switching between the two speeds indefinitely and the data + * link layer never reaches the active state, with link training reported + * repeatedly active ~84% of the time. Forcing the target link speed to + * 2.5GT/s with the upstream ASM2824 device makes the two switches talk to + * each other correctly however. And more interestingly retraining with a + * higher target link speed afterwards lets the two successfully negotiate + * 5.0GT/s. + * + * With the ASM2824 we can rely on the otherwise optional Data Link Layer + * Link Active status bit and in the failed link training scenario it will + * be off along with the Link Bandwidth Management Status indicating that + * hardware has changed the link speed or width in an attempt to correct + * unreliable link operation. For a port that has been left unconnected + * both bits will be clear. So use this information to detect the problem + * rather than polling the Link Training bit and watching out for flips or + * at least the active status. + * + * Since the exact nature of the problem isn't known and in principle this + * could trigger where an ASM2824 device is downstream rather upstream, + * apply this erratum workaround to any downstream ports as long as they + * support Link Active reporting and have the Link Control 2 register. + * Restrict the speed to 2.5GT/s then with the Target Link Speed field, + * request a retrain and wait 200ms for the data link to go up. + * + * If this turns out successful and we know by the Vendor:Device ID it is + * safe to do so, then lift the restriction, letting the devices negotiate + * a higher speed. Also check for a similar 2.5GT/s speed restriction the + * firmware may have already arranged and lift it with ports that already + * report their data link being up. + */ +static void pcie_downstream_link_retrain(struct pci_dev *dev) +{ + static const struct pci_device_id ids[] = { + { PCI_VDEVICE(ASMEDIA, 0x2824) }, /* ASMedia ASM2824 */ + {} + }; + u16 lnksta, lnkctl2; + u32 lnkcap; + + if (!pci_is_pcie(dev) || !pcie_downstream_port(dev) + || !pcie_cap_has_lnkctl2(dev)) + return; + + /* It's too early yet to use `dev->link_active_reporting'. */ + pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap); + if (!(lnkcap & PCI_EXP_LNKCAP_DLLLARC)) + return; + + pcie_capability_read_word(dev, PCI_EXP_LNKCTL2, &lnkctl2); + pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &lnksta); + if ((lnksta & (PCI_EXP_LNKSTA_LBMS | PCI_EXP_LNKSTA_DLLLA)) == + PCI_EXP_LNKSTA_LBMS) { + unsigned long timeout; + u16 lnkctl; + + pci_info(dev, "broken device, retraining non-functional downstream link at 2.5GT/s\n"); + + pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &lnkctl); + lnkctl |= PCI_EXP_LNKCTL_RL; + lnkctl2 &= ~PCI_EXP_LNKCTL2_TLS; + lnkctl2 |= PCI_EXP_LNKCTL2_TLS_2_5GT; + pcie_capability_write_word(dev, PCI_EXP_LNKCTL2, lnkctl2); + pcie_capability_write_word(dev, PCI_EXP_LNKCTL, lnkctl); + /* + * Due to an erratum in some devices the Retrain Link bit + * needs to be cleared again manually to allow the link + * training to succeed. + */ + lnkctl &= ~PCI_EXP_LNKCTL_RL; + if (dev->clear_retrain_link) + pcie_capability_write_word(dev, PCI_EXP_LNKCTL, + lnkctl); + + timeout = jiffies + PCIE_LINK_RETRAIN_TIMEOUT; + do { + pcie_capability_read_word(dev, PCI_EXP_LNKSTA, + &lnksta); + if (lnksta & PCI_EXP_LNKSTA_DLLLA) + break; + usleep_range(10000, 20000); + } while (time_before(jiffies, timeout)); + + if (!(lnksta & PCI_EXP_LNKSTA_DLLLA)) { + pci_info(dev, "retraining failed\n"); + return; + } + } + + if ((lnksta & PCI_EXP_LNKSTA_DLLLA) && + (lnkctl2 & PCI_EXP_LNKCTL2_TLS) == PCI_EXP_LNKCTL2_TLS_2_5GT && + pci_match_id(ids, dev)) { + u16 lnkctl; + + pci_info(dev, "removing 2.5GT/s downstream link speed restriction\n"); + pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &lnkctl); + lnkctl |= PCI_EXP_LNKCTL_RL; + lnkctl2 &= ~PCI_EXP_LNKCTL2_TLS; + lnkctl2 |= lnkcap & PCI_EXP_LNKCAP_SLS; + pcie_capability_write_word(dev, PCI_EXP_LNKCTL2, lnkctl2); + pcie_capability_write_word(dev, PCI_EXP_LNKCTL, lnkctl); + } +} +DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, + pcie_downstream_link_retrain); +DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_ANY_ID, PCI_ANY_ID, + pcie_downstream_link_retrain);
Attempt to handle cases such as with a downstream port of the ASMedia ASM2824 PCIe switch where link training never completes and the link continues switching between speeds indefinitely with the data link layer never reaching the active state. It has been observed with a downstream port of the ASMedia ASM2824 Gen 3 switch wired to the upstream port of the Pericom PI7C9X2G304 Gen 2 switch, using a Delock Riser Card PCI Express x1 > 2 x PCIe x1 device, P/N 41433, wired to a SiFive HiFive Unmatched board. In this setup the switches are supposed to negotiate the link speed of preferably 5.0GT/s, falling back to 2.5GT/s. Instead the link continues oscillating between the two speeds, at the rate of 34-35 times per second, with link training reported repeatedly active ~84% of the time. Forcibly limiting the target link speed to 2.5GT/s with the upstream ASM2824 device however makes the two switches communicate correctly. Removing the speed restriction afterwards makes the two devices switch to 5.0GT/s then. Make use of these observations then and detect the inability to train the link, by checking for the Data Link Layer Link Active status bit being off while the Link Bandwidth Management Status indicating that hardware has changed the link speed or width in an attempt to correct unreliable link operation. Restrict the speed to 2.5GT/s then with the Target Link Speed field, request a retrain and wait 200ms for the data link to go up. If this turns out successful, then lift the restriction, letting the devices negotiate a higher speed. Also check for a 2.5GT/s speed restriction the firmware may have already arranged and lift it too with ports of devices known to continue working afterwards, currently the ASM2824 only, that already report their data link being up. Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk> Link: https://lore.kernel.org/lkml/alpine.DEB.2.21.2203022037020.56670@angie.orcam.me.uk/ Link: https://source.denx.de/u-boot/u-boot/-/commit/a398a51ccc68 --- Changes from v4: - Remove <linux/bug.h> inclusion no longer needed. - Make the quirk generic based on probing device features rather than specific to the ASM2824 part only; take the Retrain Link bit erratum into account. - Still lift the 2.5GT/s speed restriction with the ASM2824 only. - Increase retrain timeout from 200ms to 1s (PCIE_LINK_RETRAIN_TIMEOUT). - Remove retrain success notification. - Use PCIe helpers rather than generic PCI functions throughout. - Trim down and update the wording of the change description for the switch from an ASM2824-specific to a generic fixup. Changes from v3: - Remove the <linux/pci_ids.h> entry for the ASM2824. Changes from v2: - Regenerate for 5.17-rc2 for a merge conflict. - Replace BUG_ON for a missing PCI Express capability with WARN_ON and an early return. Changes from v1: - Regenerate for a merge conflict. --- drivers/pci/quirks.c | 118 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 118 insertions(+) linux-pcie-asm2824-manual-retrain.diff