[PATCH v2 0/2] PCI: Fix the wrong reading of register fields

[PATCH v2 0/2] PCI: Fix the wrong reading of register fields

[Jiwei Sun]

From: Jiwei Sun <sunjw10@lenovo.com>

Since commit de9a6c8d5dbf ("PCI/bwctrl: Add pcie_set_target_speed() to set
PCIe Link Speed"), there are two potential issues in the function
pcie_failed_link_retrain().

(1) The macro PCIE_LNKCTL2_TLS2SPEED() and PCIE_LNKCAP_SLS2SPEED() just
uses the link speed field of the registers. However, there are many other
different function fields in the Link Control 2 Register or the Link
Capabilities Register. If the register value is directly used by the two
macros, it may cause getting an error link speed value (PCI_SPEED_UNKNOWN).

(2) In the pcie_failed_link_retrain(), the local variable lnkctl2 is not
changed after reading from PCI_EXP_LNKCTL2. It might cause that the
removing 2.5GT/s downstream link speed restriction codes are not executed.

In order to avoid the above-mentioned potential issues, only keep link
speed field of the two registers before using and reread the Link Control 2
Register before using.

This series focuses on the first patch of the original series [1]. The
second one of the original series will submitted via the other single
patch.

Fixes: de9a6c8d5dbf ("PCI/bwctrl: Add pcie_set_target_speed() to set PCIe Link Speed")

[1] https://lore.kernel.org/linux-pci/tencent_DD9CBE5B44210B43A04EF8DAF52506A08509@qq.com/
---
v2 changes:
 https://lore.kernel.org/linux-pci/tencent_753C9F9DFEC140A750F2778E6879E1049406@qq.com/
 - divide the two issues into different patches
 - get fixed inside the macros
---

Jiwei Sun (2):
  PCI: Fix the wrong reading of register fields
  PCI: reread the Link Control 2 Register before using

 drivers/pci/pci.h    | 30 +++++++++++++++++-------------
 drivers/pci/quirks.c |  1 +
 2 files changed, 18 insertions(+), 13 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] PCI: Fix the wrong reading of register fields

[Jiwei Sun]

From: Jiwei Sun <sunjw10@lenovo.com>

The macro PCIE_LNKCTL2_TLS2SPEED() and PCIE_LNKCAP_SLS2SPEED() just uses
the link speed field of the registers. However, there are many other
different function fields in the Link Control 2 Register or the Link
Capabilities Register. If the register value is directly used by the two
macros, it may cause getting an error link speed value (PCI_SPEED_UNKNOWN).

In order to avoid the above-mentioned potential issue, only keep link
speed field of the two registers before using.

Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Jiwei Sun <sunjw10@lenovo.com>
---
 drivers/pci/pci.h | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 2e40fc63ba31..b7e5af859517 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -337,12 +337,13 @@ void pci_bus_put(struct pci_bus *bus);
 
 #define PCIE_LNKCAP_SLS2SPEED(lnkcap)					\
 ({									\
-	((lnkcap) == PCI_EXP_LNKCAP_SLS_64_0GB ? PCIE_SPEED_64_0GT :	\
-	 (lnkcap) == PCI_EXP_LNKCAP_SLS_32_0GB ? PCIE_SPEED_32_0GT :	\
-	 (lnkcap) == PCI_EXP_LNKCAP_SLS_16_0GB ? PCIE_SPEED_16_0GT :	\
-	 (lnkcap) == PCI_EXP_LNKCAP_SLS_8_0GB ? PCIE_SPEED_8_0GT :	\
-	 (lnkcap) == PCI_EXP_LNKCAP_SLS_5_0GB ? PCIE_SPEED_5_0GT :	\
-	 (lnkcap) == PCI_EXP_LNKCAP_SLS_2_5GB ? PCIE_SPEED_2_5GT :	\
+	u32 __lnkcap = (lnkcap) & PCI_EXP_LNKCAP_SLS;		\
+	(__lnkcap == PCI_EXP_LNKCAP_SLS_64_0GB ? PCIE_SPEED_64_0GT :	\
+	 __lnkcap == PCI_EXP_LNKCAP_SLS_32_0GB ? PCIE_SPEED_32_0GT :	\
+	 __lnkcap == PCI_EXP_LNKCAP_SLS_16_0GB ? PCIE_SPEED_16_0GT :	\
+	 __lnkcap == PCI_EXP_LNKCAP_SLS_8_0GB ? PCIE_SPEED_8_0GT :	\
+	 __lnkcap == PCI_EXP_LNKCAP_SLS_5_0GB ? PCIE_SPEED_5_0GT :	\
+	 __lnkcap == PCI_EXP_LNKCAP_SLS_2_5GB ? PCIE_SPEED_2_5GT :	\
 	 PCI_SPEED_UNKNOWN);						\
 })
 
@@ -357,13 +358,16 @@ void pci_bus_put(struct pci_bus *bus);
 	 PCI_SPEED_UNKNOWN)
 
 #define PCIE_LNKCTL2_TLS2SPEED(lnkctl2) \
-	((lnkctl2) == PCI_EXP_LNKCTL2_TLS_64_0GT ? PCIE_SPEED_64_0GT : \
-	 (lnkctl2) == PCI_EXP_LNKCTL2_TLS_32_0GT ? PCIE_SPEED_32_0GT : \
-	 (lnkctl2) == PCI_EXP_LNKCTL2_TLS_16_0GT ? PCIE_SPEED_16_0GT : \
-	 (lnkctl2) == PCI_EXP_LNKCTL2_TLS_8_0GT ? PCIE_SPEED_8_0GT : \
-	 (lnkctl2) == PCI_EXP_LNKCTL2_TLS_5_0GT ? PCIE_SPEED_5_0GT : \
-	 (lnkctl2) == PCI_EXP_LNKCTL2_TLS_2_5GT ? PCIE_SPEED_2_5GT : \
-	 PCI_SPEED_UNKNOWN)
+({									\
+	u16 __lnkctl2 = (lnkctl2) & PCI_EXP_LNKCTL2_TLS;	\
+	(__lnkctl2 == PCI_EXP_LNKCTL2_TLS_64_0GT ? PCIE_SPEED_64_0GT : \
+	 __lnkctl2 == PCI_EXP_LNKCTL2_TLS_32_0GT ? PCIE_SPEED_32_0GT : \
+	 __lnkctl2 == PCI_EXP_LNKCTL2_TLS_16_0GT ? PCIE_SPEED_16_0GT : \
+	 __lnkctl2 == PCI_EXP_LNKCTL2_TLS_8_0GT ? PCIE_SPEED_8_0GT : \
+	 __lnkctl2 == PCI_EXP_LNKCTL2_TLS_5_0GT ? PCIE_SPEED_5_0GT : \
+	 __lnkctl2 == PCI_EXP_LNKCTL2_TLS_2_5GT ? PCIE_SPEED_2_5GT : \
+	 PCI_SPEED_UNKNOWN);						\
+})
 
 /* PCIe speed to Mb/s reduced by encoding overhead */
 #define PCIE_SPEED2MBS_ENC(speed) \
-- 
2.34.1

Re: [PATCH v2 1/2] PCI: Fix the wrong reading of register fields

[Ilpo Järvinen]

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

On Wed, 15 Jan 2025, Jiwei Sun wrote:

> From: Jiwei Sun <sunjw10@lenovo.com>
> 
> The macro PCIE_LNKCTL2_TLS2SPEED() and PCIE_LNKCAP_SLS2SPEED() just uses
> the link speed field of the registers. However, there are many other
> different function fields in the Link Control 2 Register or the Link
> Capabilities Register. If the register value is directly used by the two
> macros, it may cause getting an error link speed value (PCI_SPEED_UNKNOWN).
> 
> In order to avoid the above-mentioned potential issue, only keep link
> speed field of the two registers before using.
>
> Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Signed-off-by: Jiwei Sun <sunjw10@lenovo.com>

Missing Fixes tag.

> ---
>  drivers/pci/pci.h | 30 +++++++++++++++++-------------
>  1 file changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 2e40fc63ba31..b7e5af859517 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -337,12 +337,13 @@ void pci_bus_put(struct pci_bus *bus);
>  
>  #define PCIE_LNKCAP_SLS2SPEED(lnkcap)					\
>  ({									\
> -	((lnkcap) == PCI_EXP_LNKCAP_SLS_64_0GB ? PCIE_SPEED_64_0GT :	\
> -	 (lnkcap) == PCI_EXP_LNKCAP_SLS_32_0GB ? PCIE_SPEED_32_0GT :	\
> -	 (lnkcap) == PCI_EXP_LNKCAP_SLS_16_0GB ? PCIE_SPEED_16_0GT :	\
> -	 (lnkcap) == PCI_EXP_LNKCAP_SLS_8_0GB ? PCIE_SPEED_8_0GT :	\
> -	 (lnkcap) == PCI_EXP_LNKCAP_SLS_5_0GB ? PCIE_SPEED_5_0GT :	\
> -	 (lnkcap) == PCI_EXP_LNKCAP_SLS_2_5GB ? PCIE_SPEED_2_5GT :	\
> +	u32 __lnkcap = (lnkcap) & PCI_EXP_LNKCAP_SLS;		\
> +	(__lnkcap == PCI_EXP_LNKCAP_SLS_64_0GB ? PCIE_SPEED_64_0GT :	\

It would be nice to have an empty line here and below as is in "normal 
functions" (obviously the \ continuation at the end of the line is 
required).

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

This is quite important fix IMO.

> +	 __lnkcap == PCI_EXP_LNKCAP_SLS_32_0GB ? PCIE_SPEED_32_0GT :	\
> +	 __lnkcap == PCI_EXP_LNKCAP_SLS_16_0GB ? PCIE_SPEED_16_0GT :	\
> +	 __lnkcap == PCI_EXP_LNKCAP_SLS_8_0GB ? PCIE_SPEED_8_0GT :	\
> +	 __lnkcap == PCI_EXP_LNKCAP_SLS_5_0GB ? PCIE_SPEED_5_0GT :	\
> +	 __lnkcap == PCI_EXP_LNKCAP_SLS_2_5GB ? PCIE_SPEED_2_5GT :	\
>  	 PCI_SPEED_UNKNOWN);						\
>  })
>  
> @@ -357,13 +358,16 @@ void pci_bus_put(struct pci_bus *bus);
>  	 PCI_SPEED_UNKNOWN)
>  
>  #define PCIE_LNKCTL2_TLS2SPEED(lnkctl2) \
> -	((lnkctl2) == PCI_EXP_LNKCTL2_TLS_64_0GT ? PCIE_SPEED_64_0GT : \
> -	 (lnkctl2) == PCI_EXP_LNKCTL2_TLS_32_0GT ? PCIE_SPEED_32_0GT : \
> -	 (lnkctl2) == PCI_EXP_LNKCTL2_TLS_16_0GT ? PCIE_SPEED_16_0GT : \
> -	 (lnkctl2) == PCI_EXP_LNKCTL2_TLS_8_0GT ? PCIE_SPEED_8_0GT : \
> -	 (lnkctl2) == PCI_EXP_LNKCTL2_TLS_5_0GT ? PCIE_SPEED_5_0GT : \
> -	 (lnkctl2) == PCI_EXP_LNKCTL2_TLS_2_5GT ? PCIE_SPEED_2_5GT : \
> -	 PCI_SPEED_UNKNOWN)
> +({									\
> +	u16 __lnkctl2 = (lnkctl2) & PCI_EXP_LNKCTL2_TLS;	\
> +	(__lnkctl2 == PCI_EXP_LNKCTL2_TLS_64_0GT ? PCIE_SPEED_64_0GT : \
> +	 __lnkctl2 == PCI_EXP_LNKCTL2_TLS_32_0GT ? PCIE_SPEED_32_0GT : \
> +	 __lnkctl2 == PCI_EXP_LNKCTL2_TLS_16_0GT ? PCIE_SPEED_16_0GT : \
> +	 __lnkctl2 == PCI_EXP_LNKCTL2_TLS_8_0GT ? PCIE_SPEED_8_0GT : \
> +	 __lnkctl2 == PCI_EXP_LNKCTL2_TLS_5_0GT ? PCIE_SPEED_5_0GT : \
> +	 __lnkctl2 == PCI_EXP_LNKCTL2_TLS_2_5GT ? PCIE_SPEED_2_5GT : \
> +	 PCI_SPEED_UNKNOWN);						\
> +})
>  
>  /* PCIe speed to Mb/s reduced by encoding overhead */
>  #define PCIE_SPEED2MBS_ENC(speed) \
> 


-- 
 i.

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

[Jiwei Sun]

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);
 	}
 
-- 
2.34.1

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

[Ilpo Järvinen]

[-- 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.

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

[Maciej W. Rozycki]

On Thu, 16 Jan 2025, Ilpo Järvinen wrote:

> > 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.

 Correct.

> 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.

 Correct.

> 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.

 Agreed.

> 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.

 Your recollection is correct.  Perhaps we ought to lift the restriction 
instead based on the ID of the downstream device though.

 NB referring our earlier discussion the system in question is now at:

# uptime
 15:31:25 up 160 days,  2:58,  1 user,  load average: 0.00, 0.00, 0.00
# 

and the questionable link has recorded no LBMS events except for one on 
the day it was booted or maybe a day after (which I cleared back then):

# setpci -s 02:03.0 CAP_EXP+0x12.W
3012
# 

There has been some network data traffic across the link, not excessively 
high, but not insignificant either:

        RX packets 181328169  bytes 2178598128 (2.0 GiB)
        RX errors 0  dropped 0  overruns 0  frame 0
        TX packets 267194647  bytes 1149978069 (1.0 GiB)
        TX errors 0  dropped 2 overruns 0  carrier 0  collisions 0

So I think we can safely conclude signalling is free from disruption at 
the electrical level and all the mess with link training at speeds beyond 
2.5GT/s is owing to some kind of a protocol interpretation issue at the 
data link layer with either or both devices, and the downstream device 
being the higher suspect based on other issues with Pericom/Diodes gear.  

 Would you agree with my conclusion?

 NB I continue being busy with other stuff, but please feel free to ping 
me directly if you need any input from me.

  Maciej

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

[Jiwei Sun]

On 1/16/25 22:12, Ilpo Järvinen wrote:
> 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.

Uh, maybe my commit messages wasn't clear. My understanding is as
following,

  98 int pcie_failed_link_retrain(struct pci_dev *dev)
  99 {  
			 ...
 111         pcie_capability_read_word(dev, PCI_EXP_LNKCTL2, &lnkctl2);
 112         pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &lnksta);
 113         if (!(lnksta & PCI_EXP_LNKSTA_DLLLA) && pcie_lbms_seen(dev, lnksta)) {
 114                 u16 oldlnkctl2 = lnkctl2;
 115         
 116                 pci_info(dev, "broken device, retraining non-functional downstream link at 2.5GT/s\n");
 117         
 118                 ret = pcie_set_target_speed(dev, PCIE_SPEED_2_5GT, false);
 119                 if (ret) {
 120                         pci_info(dev, "retraining failed\n");
 121                         pcie_set_target_speed(dev, PCIE_LNKCTL2_TLS2SPEED(oldlnkctl2),
 122                                               true);
 123                         return ret;
 124                 }
 125         
 126                 pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &lnksta);
 127         }
 128         
 129         if ((lnksta & PCI_EXP_LNKSTA_DLLLA) &&
 130             (lnkctl2 & PCI_EXP_LNKCTL2_TLS) == PCI_EXP_LNKCTL2_TLS_2_5GT &&
 131             pci_match_id(ids, dev)) {
 132                 u32 lnkcap;
 133         
 134                 pci_info(dev, "removing 2.5GT/s downstream link speed restriction\n");
 135                 pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
 136                 ret = pcie_set_target_speed(dev, PCIE_LNKCAP_SLS2SPEED(lnkcap), false);


Consider the following scenario:
After the execution of line 111, lnkctl2.tls is 0x3 (Gen 3, such as
for the device ASMedia ASM2824). The function call
pcie_set_target_speed(dev, PCIE_SPEED_2_5GT, false) on line 118 returns 0,
and the tls field of the link control 2 register is modified to 0x1 
(corresponding to 2.5GT/s).

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.

Before the commit de9a6c8d5dbf ("PCI/bwctrl: Add pcie_set_target_speed()
to set PCIe Link Speed"), after setting the minimum speed and successfully
completing retraining, an attempt would be made to clear the minimum speed.
IIUC, even for this device ASMedia ASM2824, it is necessary to reread the
link control 2 register after successful retraining on line 118.

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

Yes, it seems that our testing has entered an inappropriate code flow, 
but how to avoid it is a troublesome issue.

Thanks,
Regards,
Jiwei
> 

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

[Maciej W. Rozycki]

On Fri, 17 Jan 2025, Jiwei Sun 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.

  Maciej

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

[Jiwei]

On 1/18/25 09:03, Maciej W. Rozycki wrote:
> On Fri, 17 Jan 2025, Jiwei Sun 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.

IIUC, there is no regression regarding the lifting 2.5GT/s restriction in
the commit a89c82249c37 ("PCI: Work around PCIe link training failures").
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").

So, this fix patch(PCI: reread the Link Control 2 Register before using) 
is required, right?

Thanks,
Regards,
Jiwei

> 
>   Maciej

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

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

[Jiwei]

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