Date:   Tue, 12 Apr 2022 17:50:47 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Vidya Sagar <vidyas@...dia.com>
Cc:     "Kenneth R. Crudup" <kenny@...ix.com>, bhelgaas@...gle.com,
        lorenzo.pieralisi@....com, hkallweit1@...il.com,
        wangxiongfeng2@...wei.com, mika.westerberg@...ux.intel.com,
        kai.heng.feng@...onical.com, chris.packham@...iedtelesis.co.nz,
        yangyicong@...ilicon.com, treding@...dia.com, jonathanh@...dia.com,
        abhsahu@...dia.com, sagupta@...dia.com, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org, kthota@...dia.com,
        mmaddireddy@...dia.com, sagar.tv@...il.com,
        Ricky Wu <ricky_wu@...ltek.com>,
        Rajat Jain <rajatja@...gle.com>,
        Prasad Malisetty <quic_pmaliset@...cinc.com>,
        Victor Ding <victording@...gle.com>
Subject: Re: [PATCH V1] PCI/ASPM: Save/restore L1SS Capability for
 suspend/resume

[+cc Ricky for rtsx_pci ASPM behavior, Rajat, Prasad for L1 SS stuff,
Victor for interest in disabling ASPM during save/restore]

On Wed, Feb 16, 2022 at 06:41:39PM +0530, Vidya Sagar wrote:
> On 2/16/2022 11:30 AM, Kenneth R. Crudup wrote:
> > On Wed, 16 Feb 2022, Vidya Sagar wrote:
> > 
> > > I see that the ASPM-L1 state of Realtek NIC which was in
> > > disabled state before hibernate got enabled after hibernate.
> > 
> > That's actually my SD-Card reader; there's a good chance the BIOS
> > does "something" to it at boot time, as it's possible to boot from
> > SD-Card on my laptop.
> > 
> > > This patch doesn't do anything to LnkCtl register which has
> > > control for ASPM L1 state.
> > 
> > > Could you please check why ASPM L1 got enabled post hibernation?
> > 
> > I wouldn't know how to do that; if you're still interested in that
> > let me know what to do to determine that.

> I would like Bjorn to take a call on it.
> At this point, there are contradictions in observations.

Remind me what contradictions you see?  I know Kenny saw NVMe errors
on a kernel that included 4257f7e008ea ("PCI/ASPM: Save/restore L1SS
Capability for suspend/resume") in December 2020 [1], and that he did
*not* see those errors on 4257f7e008ea in February 2022 [2].  Is that
what you mean?

> Just to summarize,
> - The root ports in your laptop don't have support for L1SS
> - With the same old code base with which the errors were observed plus my
> patch on top of it, I see that ASPM-L1 state getting enabled for one of the
> endpoints (Realtek SD-Card reader) after system comes out of hibernation
> even though ASPM-L1 was disabled before the system enter into hibernation.
> No errors are reported now.

I assume you refer to [2], where on 4257f7e008ea ("PCI/ASPM:
Save/restore L1SS Capability for suspend/resume"), Kenny saw ASPM L1
disabled before hibernate and enabled afterwards:

  --- pre-hibernate
  +++ post-hibernate
    00:1d.7 PCI bridge [0604]: Intel [8086:34b7]
      Bus: primary=00, secondary=58, subordinate=58
	LnkCtl: ASPM Disabled; RCB 64 bytes, Disabled- CommClk+
    58:00.0 RTS525A PCI Express Card Reader [10ec:525a]
  -     LnkCtl: ASPM Disabled; RCB 64 bytes, Disabled- CommClk-
  -             ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
  +     LnkCtl: ASPM L1 Enabled; RCB 64 bytes, Disabled- CommClk-
  +             ExtSynch- ClockPM+ AutWidDis- BWInt- AutBWInt-

Per PCIe r6.0, sec 7.5.3.7, "ASPM L1 must be enabled by software in
the Upstream component on a Link prior to enabling ASPM L1 in the
Downstream component on that Link," so this definitely seems broken,
but wouldn't explain the NVMe issue.

The PCI core (pcie_config_aspm_link()) always enables L1 in the
upstream component before the downstream one, but 58:00.0 uses the
rtsx_pci driver, which does a lot of its own ASPM fiddling, so my
guess is that it's doing something wrong here.

> - With the linux-next top of the tree plus my patch, no change in the ASPM
> states and no errors also reported.

I don't know which report this refers to.

> This points to BIOS being buggy (both old and new with new one being less
> problematic)

I agree that a BIOS change between [1] and [2] seems plausible, but I
don't think we can prove that yet.  I'm slightly queasy because while
Kenny may have updated his BIOS, most people will not have.

I think we should try this patch again with some changes and maybe
some debug logging:

  - I wonder if we should integrate the LTR, L1 SS, and Link Control
    ASPM restore instead of having them spread around through
    pci_restore_ltr_state(), pci_restore_aspm_l1ss_state(), and
    pci_restore_pcie_state().  Maybe a new pci_restore_aspm() that
    would be called from pci_restore_pcie_state()?

  - For L1 PM Substates configuration, sec 5.5.4 says that both ports
    must be configured while ASPM L1 is disabled, but I don't think we
    currently guarantee this: we restore all the upstream component
    state first, and we don't know the ASPM state of the downstream
    one.  Maybe we need to:

      * When restoring upstream component,
          + disable its ASPM

      * When restoring downstream component,
          + disable its ASPM
	  + restore upstream component's LTR, L1SS
	  + restore downstream component's LTR, L1SS
	  + restore upstream component's ASPM
	  + restore downstream component's ASPM

      This seems pretty messy, but seems like what the spec requires.

    - Add some pci_dbg() logging of all these save/restore values to
      help debug any issues.

Bjorn

[1] https://lore.kernel.org/r/20201228040513.GA611645@bjorn-Precision-5520
[2] https://lore.kernel.org/r/3ca14a7-b726-8430-fe61-a3ac183a1088@panix.com

Powered by blists - more mailing lists