On Sat, Apr 16, 2022 at 5:25 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> 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@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. > > 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@xxxxxxxxxxxxx/ Kai-Heng
