On 1/20/25 23:05, Maciej W. Rozycki wrote:
> 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)) {
Thanks for your suggestion. The patch that you modified is better; I will
do some tests tomorrow and send the v3 patch.
Thanks,
Regards,
Jiwei
