Re: [Bug 219984] New: [BISECTED] High power usage since 'PCI/ASPM: Correct LTR_L1.2_THRESHOLD computation'

[Bjorn Helgaas <helgaas@kernel.org>]

On Mon, Apr 07, 2025 at 07:33:31PM +0100, Sergey Dolgov wrote:
> Dear Bjorn,
> 
> both attached.

Thank you very much.  Most of the differences are that the
LTR1.2_Threshold values are increased with 7afeb84d14ea ("PCI/ASPM:
Correct LTR_L1.2_THRESHOLD computation"):

  02:00.0 PCI bridge: Intel Corporation JHL7540 Thunderbolt 3 Bridge [Titan Ridge 4C 2018]
    Bus: primary=02, secondary=03, subordinate=6c, sec-latency=0
    Capabilities: [a00 v1] L1 PM Substates
-     T_CommonMode=40us LTR1.2_Threshold=65536ns
+     T_CommonMode=40us LTR1.2_Threshold=90112ns

  6d:00.0 Non-Volatile memory controller: Intel Corporation Optane NVME SSD H10 with Solid State Storage
    Capabilities: [180 v1] L1 PM Substates
-     T_CommonMode=0us LTR1.2_Threshold=3145728ns
+     T_CommonMode=0us LTR1.2_Threshold=3211264ns

  70:00.0 Unassigned class [ff00]: Realtek Semiconductor Co., Ltd. RTS525A PCI Express Card Reader
    Capabilities: [160 v1] L1 PM Substates
-     T_CommonMode=0us LTR1.2_Threshold=98304ns
+     T_CommonMode=0us LTR1.2_Threshold=126976ns


In addition, I wonder if there's something wrong with link training
because the 00:1d.0 - 6d:00.0 link claims training is in progress, and
the 00:1d.2 - 6e:00.0 link is only x1 when it should be x2:

  00:1d.0 PCI bridge: Intel Corporation Cannon Lake PCH PCI Express Root Port #9
      LnkSta: Speed 8GT/s, Width x2
-       TrErr- Train- SlotClk+ DLActive+ BWMgmt- ABWMgmt-
+       TrErr- Train+ SlotClk+ DLActive+ BWMgmt- ABWMgmt-

  00:1d.2 PCI bridge: Intel Corporation Cannon Lake PCH PCI Express Root Port #11
-     LnkSta: Speed 8GT/s, Width x2
+     LnkSta: Speed 8GT/s, Width x1


I don't see the problem right off, so could you please add the patch
below and collect the dmesg logs again?

Bjorn


diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 29fcb0689a91..bd9322bde53a 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -647,12 +647,19 @@ static void aspm_calc_l12_info(struct pcie_link_state *link,
 	val2 = FIELD_GET(PCI_L1SS_CAP_CM_RESTORE_TIME, child_l1ss_cap);
 	t_common_mode = max(val1, val2);
 
+	pci_info(child, "parent CMRT %#04x child CMRT %#04x\n", val1, val2);
+
 	/* Choose the greater of the two Port T_POWER_ON times */
 	val1   = FIELD_GET(PCI_L1SS_CAP_P_PWR_ON_VALUE, parent_l1ss_cap);
 	scale1 = FIELD_GET(PCI_L1SS_CAP_P_PWR_ON_SCALE, parent_l1ss_cap);
 	val2   = FIELD_GET(PCI_L1SS_CAP_P_PWR_ON_VALUE, child_l1ss_cap);
 	scale2 = FIELD_GET(PCI_L1SS_CAP_P_PWR_ON_SCALE, child_l1ss_cap);
 
+	pci_info(child, "parent T_POWER_ON %#04x usec (val %#02x scale %x)\n",
+		 calc_l12_pwron(parent, scale1, val1), val1, scale1);
+	pci_info(child, "child  T_POWER_ON %#04x usec (val %#02x scale %x)\n",
+		 calc_l12_pwron(child, scale2, val2), val2, scale2);
+
 	if (calc_l12_pwron(parent, scale1, val1) >
 	    calc_l12_pwron(child, scale2, val2)) {
 		ctl2 |= FIELD_PREP(PCI_L1SS_CTL2_T_PWR_ON_SCALE, scale1) |
@@ -675,7 +682,11 @@ static void aspm_calc_l12_info(struct pcie_link_state *link,
 	 * least 4us.
 	 */
 	l1_2_threshold = 2 + 4 + t_common_mode + t_power_on;
+	pci_info(child, "t_common_mode %#04x t_power_on %#04x l1_2_threshold %#04x\n",
+		 t_common_mode, t_power_on, l1_2_threshold);
 	encode_l12_threshold(l1_2_threshold, &scale, &value);
+	pci_info(child, "encoded LTR_L1.2_THRESHOLD value %#04x scale %x\n",
+		 value, scale);
 	ctl1 |= FIELD_PREP(PCI_L1SS_CTL1_CM_RESTORE_TIME, t_common_mode) |
 		FIELD_PREP(PCI_L1SS_CTL1_LTR_L12_TH_VALUE, value) |
 		FIELD_PREP(PCI_L1SS_CTL1_LTR_L12_TH_SCALE, scale);
@@ -686,6 +697,11 @@ static void aspm_calc_l12_info(struct pcie_link_state *link,
 	pci_read_config_dword(child, child->l1ss + PCI_L1SS_CTL1, &cctl1);
 	pci_read_config_dword(child, child->l1ss + PCI_L1SS_CTL2, &cctl2);
 
+	pci_info(child, "L1SS_CTL1 %#08x (parent %#08x child %08x)\n",
+		 ctl1, pctl1, cctl1);
+	pci_info(child, "L1SS_CTL2 %#08x (parent %#08x child %08x)\n",
+		 ctl2, pctl2, cctl2);
+
 	if (ctl1 == pctl1 && ctl1 == cctl1 &&
 	    ctl2 == pctl2 && ctl2 == cctl2)
 		return;
@@ -703,14 +719,27 @@ static void aspm_calc_l12_info(struct pcie_link_state *link,
 					       PCI_L1SS_CTL1_L1_2_MASK, 0);
 	}
 
+	pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1, &pctl1);
+	pci_read_config_dword(child, child->l1ss + PCI_L1SS_CTL1, &cctl1);
+	pci_info(child, "L1SS_CTL1 parent %#08x child %#08x (L1.2 disabled)\n",
+		 pctl1, cctl1);
+
 	/* 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_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL2, &pctl2);
+	pci_read_config_dword(child, child->l1ss + PCI_L1SS_CTL2, &cctl2);
+	pci_info(child, "L1SS_CTL2 parent %#08x child %#08x (T_POWER_ON updated)\n",
+		 pctl2, cctl2);
+
 	/* 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_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1, &pctl1);
+	pci_info(child, "L1SS_CTL1 parent %#08x (CMRT updated)\n", pctl1);
+
 	/* 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 |
@@ -721,6 +750,11 @@ static void aspm_calc_l12_info(struct pcie_link_state *link,
 				       PCI_L1SS_CTL1_LTR_L12_TH_SCALE,
 				       ctl1);
 
+	pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1, &pctl1);
+	pci_read_config_dword(child, child->l1ss + PCI_L1SS_CTL1, &cctl1);
+	pci_info(child, "L1SS_CTL1 parent %#08x child %#08x (LTR_L1.2_THRESHOLD updated)\n",
+		 pctl1, cctl1);
+
 	if (pl1_2_enables || cl1_2_enables) {
 		pci_clear_and_set_config_dword(parent,
 					       parent->l1ss + PCI_L1SS_CTL1, 0,
@@ -729,6 +763,11 @@ static void aspm_calc_l12_info(struct pcie_link_state *link,
 					       child->l1ss + PCI_L1SS_CTL1, 0,
 					       cl1_2_enables);
 	}
+
+	pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1, &pctl1);
+	pci_read_config_dword(child, child->l1ss + PCI_L1SS_CTL1, &cctl1);
+	pci_info(child, "L1SS_CTL1 parent %#08x child %#08x (L1.2 restored)\n",
+		 pctl1, cctl1);
 }
 
 static void aspm_l1ss_init(struct pcie_link_state *link)