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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <tencent_F1DFC27AA8603E10FDB0017CA1C4EF27D107@qq.com>
Date: Mon, 20 Jan 2025 23:20:27 +0800
From: Jiwei <jiwei.sun.bj@...com>
To: "Maciej W. Rozycki" <macro@...am.me.uk>
Cc: Jiwei Sun <sjiwei@....com>, Ilpo Järvinen
 <ilpo.jarvinen@...ux.intel.com>, Bjorn Helgaas <bhelgaas@...gle.com>,
 linux-pci@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
 helgaas@...nel.org, Lukas Wunner <lukas@...ner.de>, ahuang12@...ovo.com,
 sunjw10@...ovo.com, sunjw10@...look.com
Subject: Re: [PATCH v2 2/2] PCI: reread the Link Control 2 Register before
 using



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