On Mon, Jan 02, 2023 at 11:15:16AM -0600, Bjorn Helgaas wrote: > On Mon, Jan 02, 2023 at 02:02:51PM +0000, bugzilla-daemon@xxxxxxxxxx wrote: > > https://bugzilla.kernel.org/show_bug.cgi?id=216877 > > > > Bug ID: 216877 > > Summary: Regression in PCI powermanagement breaks resume after > > suspend > > Kernel Version: 6.0.0-rc1 > > > > Created attachment 303512 > > --> https://bugzilla.kernel.org/attachment.cgi?id=303512&action=edit > > output of git bisect log > > > > After commit 5e85eba6f50dc288c22083a7e213152bcc4b8208 "PCI/ASPM: > > Refactor L1 PM Substates Control Register programming" my Laptop > > does not resume PCI devices back from suspend. Thomas, could you try the debug patch below on top of v6.2-rc1? Prior to 5e85eba6f50d, aspm_calc_l1ss_info() did these config writes: if (enables) a child clear L1SS_CTL1 PCIPM_L1_2 ASPM_L1_2 # disable L1.2 b parent clear L1SS_CTL1 PCIPM_L1_2 ASPM_L1_2 # disable L1.2 /* T_POWER_ON in both */ c parent write L1SS_CTL2 T_POWER_ON d child write L1SS_CTL2 T_POWER_ON /* Common_Mode_Restore_Time in parent (rsvd in child) */ e parent write L1SS_CTL1 CM_RESTORE_TIME | LTR_L12_TH # clear CM_REST_TIME /* LTR_L1.2_THRESHOLD in both */ f parent write L1SS_CTL1 CM_RESTORE_TIME | LTR_L12_TH # clear LTR_L2_TH g child write L1SS_CTL1 CM_RESTORE_TIME | LTR_L12_TH # CM_REST_TIME rsvd? if (enables) h parent write L1SS_CTL1 PCIPM_L1_2 ASPM_L1_2 i child write L1SS_CTL1 PCIPM_L1_2 ASPM_L1_2 After 5e85eba6f50d, we do these: A parent write L1SS_CTL2 T_POWER_ON B parent write L1SS_CTL1 (CM_RESTORE_TIME | LTR_L12_TH) & ~(PCIPM_L1_2 ASPM_L1_2) if (enable) C parent write L1SS_CTL1 PCIPM_L1_2 ASPM_L1_2 D child write L1SS_CTL2 T_POWER_ON E child write L1SS_CTL1 (CM_RESTORE_TIME | LTR_L12_TH) & ~(PCIPM_L1_2 ASPM_L1_2) if (enable) F child write L1SS_CTL1 PCIPM_L1_2 ASPM_L1_2 Notes: - Before 5e85eba6f50d, we disable L1.2 at (a,b) before writing T_POWER_ON, CM_RESTORE_TIME, and LTR_L12_TH at (c,d,e,f,g). After 5e85eba6f50d, we write T_POWER_ON, CM_RESTORE_TIME, and LTR_L12_TH at (A,B) without disabling L1.2. Sec 5.5.4 suggests this may be a problem. - Even before 5e85eba6f50d, we write CM_REST_TIME for the child at (g), which is reserved per spec. - Before 5e85eba6f50d, the write at (e) inserts the new CMRT value, but ORs in the LTR_L12_TH values without clearing what was there before. The write at (f) inserts LTR_L12_TH correctly, so the result is probably correct, but it's messy. I think 5e85eba6f50d does this better. commit 98248ea220c8 ("debug https://bugzilla.kernel.org/show_bug.cgi?id=216877";) Author: Bjorn Helgaas <bhelgaas@xxxxxxxxxx> Date: Tue Jan 3 18:15:11 2023 -0600 debug https://bugzilla.kernel.org/show_bug.cgi?id=216877 diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index 53a1fa306e1e..398c817858ac 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -552,6 +552,11 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link, ctl2 == pctl2 && ctl2 == cctl2) return; + pci_clear_and_set_dword(child, child->l1ss + PCI_L1SS_CTL1, + PCI_L1SS_CTL1_L1_2_MASK, 0); + pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1, + PCI_L1SS_CTL1_L1_2_MASK, 0); + pctl1 &= ~(PCI_L1SS_CTL1_CM_RESTORE_TIME | PCI_L1SS_CTL1_LTR_L12_TH_VALUE | PCI_L1SS_CTL1_LTR_L12_TH_SCALE);
