On Mon, 20 Jan 2025, Jiwei wrote:
> >> However, within this section of code, lnkctl2 is not modified (after
> >> reading from register on line 111) at all and remains 0x5. This results
> >> in the condition on line 130 evaluating to 0 (false), which in turn
> >> prevents the code from line 132 onward from being executed. The removing
> >> 2.5GT/s downstream link speed restriction work can not be done.
> >
> > It seems like a regression from my original code indeed.
>
> Sorry, I am confused by this sentence.
Sorry to be unclear, it refers to the paragraph quoted.
> IIUC, there is no regression regarding the lifting 2.5GT/s restriction in
> the commit a89c82249c37 ("PCI: Work around PCIe link training failures").
That's my original code we have regressed from.
> However, since commit de9a6c8d5dbf ("PCI/bwctrl: Add
> pcie_set_target_speed() to set PCIe Link Speed"), the code to lift the
> restriction is no longer executed. Therefore, commit de9a6c8d5dbf
> ("PCI/bwctrl: Add pcie_set_target_speed() to set PCIe Link Speed") can be
> considered a regression of commit a a89c82249c37 ("PCI: Work around PCIe
> link training failures").
Yes, that accurately reflects the intent of what I wrote above.
> So, this fix patch(PCI: reread the Link Control 2 Register before using)
> is required, right?
Original code just fiddled with `lnkctl2' already retrieved. With that
replaced by a call to `pcie_set_target_speed' I think rereading from Link
Control 2 is probably the best fix, however I'd suggest to clean up all
the leftovers from old code on this occasion, along the lines of the diff
below (untested).
Maciej
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 76f4df75b08a..84267d7f847d 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -108,13 +108,13 @@ int pcie_failed_link_retrain(struct pci_dev *dev)
!pcie_cap_has_lnkctl2(dev) || !dev->link_active_reporting)
return ret;
- pcie_capability_read_word(dev, PCI_EXP_LNKCTL2, &lnkctl2);
pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &lnksta);
if (!(lnksta & PCI_EXP_LNKSTA_DLLLA) && pcie_lbms_seen(dev, lnksta)) {
- u16 oldlnkctl2 = lnkctl2;
+ u16 oldlnkctl2;
pci_info(dev, "broken device, retraining non-functional downstream link at 2.5GT/s\n");
+ pcie_capability_read_word(dev, PCI_EXP_LNKCTL2, &oldlnkctl2);
ret = pcie_set_target_speed(dev, PCIE_SPEED_2_5GT, false);
if (ret) {
pci_info(dev, "retraining failed\n");
@@ -126,6 +126,7 @@ int pcie_failed_link_retrain(struct pci_dev *dev)
pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &lnksta);
}
+ pcie_capability_read_word(dev, PCI_EXP_LNKCTL2, &lnkctl2);
if ((lnksta & PCI_EXP_LNKSTA_DLLLA) &&
(lnkctl2 & PCI_EXP_LNKCTL2_TLS) == PCI_EXP_LNKCTL2_TLS_2_5GT &&
pci_match_id(ids, dev)) {
