Re: [Bug 216877] New: Regression in PCI powermanagement breaks resume after suspend

[Bjorn Helgaas <helgaas@kernel.org>]

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@kernel.org 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@google.com>
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);