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

[Maciej W. Rozycki]

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