Date:   Thu, 9 Feb 2023 18:18:45 -0600
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Vidya Sagar <vidyas@nvidia.com>
Cc:     bhelgaas@google.com, sathyanarayanan.kuppuswamy@linux.intel.com,
        rafael.j.wysocki@intel.com, kai.heng.feng@canonical.com,
        enriquezmark36@gmail.com, tasev.stefanoska@skynet.be,
        linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
        treding@nvidia.com, jonathanh@nvidia.com, kthota@nvidia.com,
        mmaddireddy@nvidia.com, sagar.tv@gmail.com,
        Thomas Witt <kernel@witt.link>
Subject: Re: [PATCH V2] PCI/ASPM: Skip L1SS save/restore if not already
 enabled
Message-ID: <20230210001845.GA2630328@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230208234229.GA2496794@bhelgaas>
Precedence: bulk

[+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@nvidia.com>
> > ---
> > 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