[+cc Thomas] On Wed, Feb 08, 2023 at 05:42:29PM -0600, Bjorn Helgaas wrote: > On Fri, Jan 20, 2023 at 02:45:40PM +0530, Vidya Sagar wrote: > > Skip save and restore of ASPM L1 Sub-States specific registers if they > > are not already enabled in the system. This is to avoid issues observed > > on certain platforms during restoration process, particularly when > > restoring the L1SS registers contents. > > > > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=216782 > > Signed-off-by: Vidya Sagar <vidyas@xxxxxxxxxx> > > --- > > v2: > > * Address review comments from Kai-Heng Feng and Rafael > > > > drivers/pci/pcie/aspm.c | 17 ++++++++++++++++- > > include/linux/pci.h | 1 + > > 2 files changed, 17 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > > index 53a1fa306e1e..bd2a922081bd 100644 > > --- a/drivers/pci/pcie/aspm.c > > +++ b/drivers/pci/pcie/aspm.c > > @@ -761,11 +761,23 @@ void pci_save_aspm_l1ss_state(struct pci_dev *dev) > > { > > struct pci_cap_saved_state *save_state; > > u16 l1ss = dev->l1ss; > > - u32 *cap; > > + u32 *cap, val; > > > > if (!l1ss) > > return; > > > > + /* > > + * Skip save and restore of L1 Sub-States registers if they are not > > + * already enabled in the system > > + */ > > + pci_read_config_dword(dev, l1ss + PCI_L1SS_CTL1, &val); > > + if (!(val & PCI_L1SS_CTL1_L1SS_MASK)) { > > + dev->skip_l1ss_restore = true; > > + return; > > + } > > I think this fix is still problematic. PCIe r6.0, sec 5.5.4, requires > that > > If setting either or both of the enable bits for ASPM L1 PM > Substates, both ports must be configured as described in this > section while ASPM L1 is disabled. > > The current Linux code does not observe this because ASPM L1 is > enabled by PCI_EXP_LNKCTL (in the PCIe Capability Link Control > register), while ASPM L1 PM Substate configuration is in PCI_L1SS_CTL1 > (in the L1 PM Substates Capability), and these two things are not > integrated: > > pci_restore_state > pci_restore_aspm_l1ss_state > aspm_program_l1ss > pci_write_config_dword(PCI_L1SS_CTL1, ctl1) # L1SS restore > pci_restore_pcie_state > pcie_capability_write_word(PCI_EXP_LNKCTL, cap[i++]) # L1 restore > > So I suspect the problem is that we're writing PCI_L1SS_CTL1 while > ASPM L1 is enabled, and the device gets confused somehow. > > I think it would be better change this restore flow to follow that > spec requirement instead of skipping the save/restore like this. A revert of 4ff116d0d5fd ("PCI/ASPM: Save L1 PM Substates Capability for suspend/resume") has been in linux-next starting with Feb 6. I originally reverted 5e85eba6f50d ("PCI/ASPM: Refactor L1 PM Substates Control Register programming") because it broke suspend/resume differently [1]. I had to revert 4ff116d0d5fd at the same time because 5e85eba6f50d added aspm_program_l1ss(), which was used by 4ff116d0d5fd. I don't think Tasev or Mark have directly tested reverting 4ff116d0d5fd to see if it resolves the problem *they* are seeing. But that would be good to know so I can update the commit logs. Bjorn [1] https://bugzilla.kernel.org/show_bug.cgi?id=216877
