Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Fri, 15 Apr 2022 16:25:33 -0500
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Kai-Heng Feng <kai.heng.feng@canonical.com>
Cc:     Vidya Sagar <vidyas@nvidia.com>,
        "Kenneth R. Crudup" <kenny@panix.com>, bhelgaas@google.com,
        lorenzo.pieralisi@arm.com, hkallweit1@gmail.com,
        wangxiongfeng2@huawei.com, mika.westerberg@linux.intel.com,
        chris.packham@alliedtelesis.co.nz, yangyicong@hisilicon.com,
        treding@nvidia.com, jonathanh@nvidia.com, abhsahu@nvidia.com,
        sagupta@nvidia.com, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org, kthota@nvidia.com,
        mmaddireddy@nvidia.com, sagar.tv@gmail.com,
        Ricky Wu <ricky_wu@realtek.com>,
        Rajat Jain <rajatja@google.com>,
        Prasad Malisetty <quic_pmaliset@quicinc.com>,
        Victor Ding <victording@google.com>
Subject: Re: [PATCH V1] PCI/ASPM: Save/restore L1SS Capability for
 suspend/resume
Message-ID: <20220415212533.GA844147@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAAd53p6DX2C7KVRV=uu_mmPTTjE7=RsXfNPxjbOBLRbf-pXi5A@mail.gmail.com>
Precedence: bulk

On Fri, Apr 15, 2022 at 10:26:19PM +0800, Kai-Heng Feng wrote:
> On Fri, Apr 15, 2022 at 12:41 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Wed, Apr 13, 2022 at 08:19:26AM +0800, Kai-Heng Feng wrote:
> > > On Wed, Apr 13, 2022 at 6:50 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > ...
> >
> > > >   - 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
> > >
> > > Right now L1SS isn't reprogrammed after S3, and that causes WD NVMe
> > > starts to spew lots of AER errors.
> >
> > Right now we don't fully restore L1SS-related state after S3, so maybe
> > there's some inconsistency that leads to the AER errors.

> > Could you collect the "lspci -vv" state before and after S3 so we can
> > compare them?
> >
> > > So yes please restore L1SS upon resume. Meanwhile I am asking vendor
> > > _why_ restoring L1SS is crucial for it to work.
> > >
> > > I also wonder what's the purpose of pcie_aspm_pm_state_change()? Can't
> > > we just restore ASPM bits like other configs?
> >
> > Good question.  What's the context?  This is in the
> > pci_raw_set_power_state() path, not the pci_restore_state() path, so
> > seems like a separate discussion.
> 
> Because this patch alone doesn't restore T_PwrOn and LTR1.2_Threshold.

I assume the post-S3 path is basically this:

  pci_pm_resume_noirq
    pci_pm_default_resume_early
      pci_power_up
        pci_raw_set_power_state(D0)
          pcie_aspm_pm_state_change
            pcie_config_aspm_path
              pcie_config_aspm_link
                pcie_config_aspm_l1ss
                  clear PCI_EXP_LNKCTL_ASPM_L1 etc
                  set PCI_L1SS_CTL1_ASPM_L1_1 etc
                pcie_config_aspm_dev
                  set PCI_EXP_LNKCTL_ASPM_L1 etc
      pci_restore_state
        pci_restore_ltr_state
        pci_restore_aspm_l1ss_state     # after this patch
        pci_restore_pcie_state

Hmm...  I think I see what you're saying.  pcie_aspm_pm_state_change()
fiddles with ASPM and L1SS enable bits.  It likely disables L1,
enables L1SS, enables L1, but never restores the LTR capability or the
T_PwrOn and LTR1.2_Threshold bits you mention.

Then we turn around and overwrite all that stuff (and the LTR cap) in
pci_restore_state().  That all seems fairly broken, and I agree, I
don't know why pcie_aspm_pm_state_change() exists.

> So I forced the pcie_aspm_pm_state_change() calling path to eventually
> call aspm_calc_l1ss_info() which solved the problem for me.

This would update T_PwrOn and LTR1.2_Threshold, so I guess it makes
sense that this would help.  But I think we need to figure out the
reason why pcie_aspm_pm_state_change() exists and get rid of it or at
least better integrate it with pci_restore_state().

If we call pcie_aspm_pm_state_change() after D3cold or reset, things
still aren't going to work because the LTR cap isn't restored or
programmed.