[PATCH] PCI/ASPM: Fix pci_clear_and_set_config_dword() usage

[PATCH] PCI/ASPM: Fix pci_clear_and_set_config_dword() usage

[Lukas Wunner]

When aspm_calc_l12_info() programs the L1 PM Substates Control 1 register
fields Common_Mode_Restore_Time, LTR_L1.2_THRESHOLD_Value and _Scale, it
invokes pci_clear_and_set_config_dword() in an incorrect way:

For the bits to clear it selects those corresponding to the field.  So far
so good.  But for the bits to set it passes a full register value.
pci_clear_and_set_config_dword() performs a boolean OR operation which
sets all bits of that value, not just the ones that were just cleared.

Thus, when setting the LTR_L1.2_THRESHOLD_Value and _Scale on the child of
an ASPM link, aspm_calc_l12_info() also sets the Common_Mode_Restore_Time.
That's a spec violation:  PCIe r7.0 sec 7.8.3.3 says this field is RsvdP
for Upstream Ports.  On Adrià's Pixelbook Eve, Common_Mode_Restore_Time
of the Intel 7265 "Stone Peak" wifi card is zero, yet aspm_calc_l12_info()
does not preserve the zero bits but instead programs the value calculated
for the Root Port into the wifi card.

Likewise, when setting the Common_Mode_Restore_Time on the Root Port,
aspm_calc_l12_info() also changes the LTR_L1.2_THRESHOLD_Value and _Scale
from the initial 163840 nsec to 237568 nsec (due to ORing those fields),
only to reduce it afterwards to 106496 nsec.

Amend all invocations of pci_clear_and_set_config_dword() to only set bits
which are cleared.

Finally, when setting the T_POWER_ON_Value and _Scale on the Root Port and
the wifi card, aspm_calc_l12_info() fails to preserve bits declared RsvdP
and instead overwrites them with zeroes.  Replace pci_write_config_dword()
with pci_clear_and_set_config_dword() to avoid this.

Fixes: aeda9adebab8 ("PCI/ASPM: Configure L1 substate settings")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=220705#c22
Tested-by: Adrià Vilanova Martínez <me@avm99963.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: stable@vger.kernel.org # v4.11+
---
Only a "Link" tag, not a "Closes" tag because this patch is just a
byproduct that was created while working on the above-linked bugzilla.
The actual root cause of the bugzilla seems to be an ASPM erratum
of the Sunrise Point PCH which needs to be addressed in coreboot.

 drivers/pci/pcie/aspm.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index cedea47..a1f2752 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -706,22 +706,29 @@ static void aspm_calc_l12_info(struct pcie_link_state *link,
 	}
 
 	/* Program T_POWER_ON times in both ports */
-	pci_write_config_dword(parent, parent->l1ss + PCI_L1SS_CTL2, ctl2);
-	pci_write_config_dword(child, child->l1ss + PCI_L1SS_CTL2, ctl2);
+	pci_clear_and_set_config_dword(parent, parent->l1ss + PCI_L1SS_CTL2,
+				       PCI_L1SS_CTL2_T_PWR_ON_VALUE |
+				       PCI_L1SS_CTL2_T_PWR_ON_SCALE, ctl2);
+	pci_clear_and_set_config_dword(child, child->l1ss + PCI_L1SS_CTL2,
+				       PCI_L1SS_CTL2_T_PWR_ON_VALUE |
+				       PCI_L1SS_CTL2_T_PWR_ON_SCALE, ctl2);
 
 	/* Program Common_Mode_Restore_Time in upstream device */
 	pci_clear_and_set_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
-				       PCI_L1SS_CTL1_CM_RESTORE_TIME, ctl1);
+				       PCI_L1SS_CTL1_CM_RESTORE_TIME,
+				       ctl1 & PCI_L1SS_CTL1_CM_RESTORE_TIME);
 
 	/* Program LTR_L1.2_THRESHOLD time in both ports */
 	pci_clear_and_set_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
 				       PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
 				       PCI_L1SS_CTL1_LTR_L12_TH_SCALE,
-				       ctl1);
+				       ctl1 & (PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
+					       PCI_L1SS_CTL1_LTR_L12_TH_SCALE));
 	pci_clear_and_set_config_dword(child, child->l1ss + PCI_L1SS_CTL1,
 				       PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
 				       PCI_L1SS_CTL1_LTR_L12_TH_SCALE,
-				       ctl1);
+				       ctl1 & (PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
+					       PCI_L1SS_CTL1_LTR_L12_TH_SCALE));
 
 	if (pl1_2_enables || cl1_2_enables) {
 		pci_clear_and_set_config_dword(parent,
-- 
2.51.0

Re: [PATCH] PCI/ASPM: Fix pci_clear_and_set_config_dword() usage

[Bjorn Helgaas]

On Mon, Feb 16, 2026 at 08:46:13AM +0100, Lukas Wunner wrote:
> When aspm_calc_l12_info() programs the L1 PM Substates Control 1 register
> fields Common_Mode_Restore_Time, LTR_L1.2_THRESHOLD_Value and _Scale, it
> invokes pci_clear_and_set_config_dword() in an incorrect way:
> 
> For the bits to clear it selects those corresponding to the field.  So far
> so good.  But for the bits to set it passes a full register value.
> pci_clear_and_set_config_dword() performs a boolean OR operation which
> sets all bits of that value, not just the ones that were just cleared.
> 
> Thus, when setting the LTR_L1.2_THRESHOLD_Value and _Scale on the child of
> an ASPM link, aspm_calc_l12_info() also sets the Common_Mode_Restore_Time.
> That's a spec violation:  PCIe r7.0 sec 7.8.3.3 says this field is RsvdP
> for Upstream Ports.  On Adrià's Pixelbook Eve, Common_Mode_Restore_Time
> of the Intel 7265 "Stone Peak" wifi card is zero, yet aspm_calc_l12_info()
> does not preserve the zero bits but instead programs the value calculated
> for the Root Port into the wifi card.
> 
> Likewise, when setting the Common_Mode_Restore_Time on the Root Port,
> aspm_calc_l12_info() also changes the LTR_L1.2_THRESHOLD_Value and _Scale
> from the initial 163840 nsec to 237568 nsec (due to ORing those fields),
> only to reduce it afterwards to 106496 nsec.
> 
> Amend all invocations of pci_clear_and_set_config_dword() to only set bits
> which are cleared.
> 
> Finally, when setting the T_POWER_ON_Value and _Scale on the Root Port and
> the wifi card, aspm_calc_l12_info() fails to preserve bits declared RsvdP
> and instead overwrites them with zeroes.  Replace pci_write_config_dword()
> with pci_clear_and_set_config_dword() to avoid this.
> 
> Fixes: aeda9adebab8 ("PCI/ASPM: Configure L1 substate settings")
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=220705#c22
> Tested-by: Adrià Vilanova Martínez <me@avm99963.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: stable@vger.kernel.org # v4.11+

Applied to pci/aspm for v7.1, thanks!  Will be rebased after v7.0-rc1.

> ---
> Only a "Link" tag, not a "Closes" tag because this patch is just a
> byproduct that was created while working on the above-linked bugzilla.
> The actual root cause of the bugzilla seems to be an ASPM erratum
> of the Sunrise Point PCH which needs to be addressed in coreboot.
> 
>  drivers/pci/pcie/aspm.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index cedea47..a1f2752 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -706,22 +706,29 @@ static void aspm_calc_l12_info(struct pcie_link_state *link,
>  	}
>  
>  	/* Program T_POWER_ON times in both ports */
> -	pci_write_config_dword(parent, parent->l1ss + PCI_L1SS_CTL2, ctl2);
> -	pci_write_config_dword(child, child->l1ss + PCI_L1SS_CTL2, ctl2);
> +	pci_clear_and_set_config_dword(parent, parent->l1ss + PCI_L1SS_CTL2,
> +				       PCI_L1SS_CTL2_T_PWR_ON_VALUE |
> +				       PCI_L1SS_CTL2_T_PWR_ON_SCALE, ctl2);
> +	pci_clear_and_set_config_dword(child, child->l1ss + PCI_L1SS_CTL2,
> +				       PCI_L1SS_CTL2_T_PWR_ON_VALUE |
> +				       PCI_L1SS_CTL2_T_PWR_ON_SCALE, ctl2);
>  
>  	/* Program Common_Mode_Restore_Time in upstream device */
>  	pci_clear_and_set_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
> -				       PCI_L1SS_CTL1_CM_RESTORE_TIME, ctl1);
> +				       PCI_L1SS_CTL1_CM_RESTORE_TIME,
> +				       ctl1 & PCI_L1SS_CTL1_CM_RESTORE_TIME);
>  
>  	/* Program LTR_L1.2_THRESHOLD time in both ports */
>  	pci_clear_and_set_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
>  				       PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
>  				       PCI_L1SS_CTL1_LTR_L12_TH_SCALE,
> -				       ctl1);
> +				       ctl1 & (PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
> +					       PCI_L1SS_CTL1_LTR_L12_TH_SCALE));
>  	pci_clear_and_set_config_dword(child, child->l1ss + PCI_L1SS_CTL1,
>  				       PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
>  				       PCI_L1SS_CTL1_LTR_L12_TH_SCALE,
> -				       ctl1);
> +				       ctl1 & (PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
> +					       PCI_L1SS_CTL1_LTR_L12_TH_SCALE));
>  
>  	if (pl1_2_enables || cl1_2_enables) {
>  		pci_clear_and_set_config_dword(parent,
> -- 
> 2.51.0
>