Message-ID: <CAAd53p6jh7Jx3pzg9TzrSrW8+3pzLN6EBAFjFv+7T4tMODmX+g@mail.gmail.com>
Date:   Thu, 21 Apr 2022 14:16:29 +0800
From:   Kai-Heng Feng <kai.heng.feng@...onical.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     Vidya Sagar <vidyas@...dia.com>,
        "Kenneth R. Crudup" <kenny@...ix.com>, bhelgaas@...gle.com,
        lorenzo.pieralisi@....com, hkallweit1@...il.com,
        wangxiongfeng2@...wei.com, mika.westerberg@...ux.intel.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

On Sat, Apr 16, 2022 at 5:25 AM Bjorn Helgaas <helgaas@...nel.org> wrote:
>
> 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@...nel.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@...nel.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.

I went through the whole discussion again, maybe Kenneth's case is
also the result of pcie_aspm_pm_state_change()?
Since Kenneth is using TLP to switch ASPM between performance and
powersaving/powersupersaving, that means 'aspm_disabled' is false.
Hence the KOXIA NVMe stops working post suspend and Realtek card
reader toggles L1ss post hibernation.

Kenneth, can you please see if removing pcie_aspm_pm_state_change()
from pci_raw_set_power_state() helps?

Anyway, this can be easier to spot if dmesg was attached.

>
> > 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.

More than that, the ASPM sysfs won't be restored correctly after
resume [1] because of it.
So I'd like to post a patch to drop pcie_aspm_pm_state_change() if
there's no objection.

[1] https://lore.kernel.org/linux-pci/20211021035159.1117456-2-kai.heng.feng@canonical.com/

Kai-Heng

Powered by blists - more mailing lists