Re: [PATCH v2 2/2] PCI: reread the Link Control 2 Register before using

Maciej W. Rozycki
2025-01-20T15:05:06Z

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)) {