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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2642 bytes --]

On Wed, 15 Jan 2025, Jiwei Sun wrote:

> From: Jiwei Sun <sunjw10@lenovo.com>
> 
> Since commit de9a6c8d5dbf ("PCI/bwctrl: Add pcie_set_target_speed() to set
> PCIe Link Speed"), the local variable "lnkctl2" is not changed after
> reading from PCI_EXP_LNKCTL2 in the pcie_failed_link_retrain(). It might
> cause that the removing 2.5GT/s downstream link speed restriction codes
> are not executed.
> 
> Reread the Link Control 2 Register before using.
> 
> Fixes: de9a6c8d5dbf ("PCI/bwctrl: Add pcie_set_target_speed() to set PCIe Link Speed")
> Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Signed-off-by: Jiwei Sun <sunjw10@lenovo.com>
> ---
>  drivers/pci/quirks.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 76f4df75b08a..02d2e16672a8 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -123,6 +123,7 @@ int pcie_failed_link_retrain(struct pci_dev *dev)
>  			return ret;
>  		}
>  
> +		pcie_capability_read_word(dev, PCI_EXP_LNKCTL2, &lnkctl2);
>  		pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &lnksta);
>  	}

I started to wonder if there would be a better way to fix this. If I've 
understood Maciej's intent correctly, there are two cases when the 2nd 
step (the one lifting the 2.5GT/s restriction) should be used:

1) TLS is 2.5GT/s at the entry to the quirk and it's an ASMedia switch.

2) If the quirk lowered TLS to 2.5GT/s and the link became up fine 
because of that. This also currently checks for ASMedia but in the v1 you 
also proposed to change that. We know it works in some cases with ASMedia 
but are unsure what happens with other switches.

In the case 2, we don't need to check TLS since the function itself asked 
TLS to be lowered which did not return an error, so we know the speed was 
lowered so why spend time on rereading the register? A simple local 
variable could convey the same information.

In case 1, I don't think ASMedia check should be removed. It was about a 
case where FW has a workaround to lower the speed (IIRC). If Link Speed is 
2.5GT/s at entry but it's not ASMedia switch, there's no 2.5GT/s 
restriction to lift.


As a general comment abouth your test case, I don't think this Target 
Speed quirk really is compatible with your test case. The quirk makes 
assumption that the Link Up/Down status changes are because of the changes 
the quirk made itself but your rapid add/remove test alters the 
environment, which produces unrelated Link Up/Down status changes that get 
picked up by the quirk (a false signal).

-- 
 i.