On Fri, Apr 15, 2022 at 12:41 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> 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@xxxxxxxxxx> 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. So I forced the pcie_aspm_pm_state_change() calling path to eventually call aspm_calc_l1ss_info() which solved the problem for me. Let me investigate a bit further. Kai-Heng > > Bjorn
