Re: [PATCH v2 2/2] PCI: Add missing link delays required by the PCIe spec

From: Bjorn Helgaas <helgaas@kernel.org>
To: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Len Brown <lenb@kernel.org>, Lukas Wunner <lukas@wunner.de>,
	Keith Busch <keith.busch@intel.com>,
	Alex Williamson <alex.williamson@redhat.com>,
	Alexandru Gagniuc <mr.nuke.me@gmail.com>,
	Kai-Heng Feng <kai.heng.feng@canonical.com>,
	Paul Menzel <pmenzel@molgen.mpg.de>,
	Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>,
	linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/2] PCI: Add missing link delays required by the PCIe spec
Date: Tue, 5 Nov 2019 09:00:13 -0600	[thread overview]
Message-ID: <20191105150013.GA202873@google.com> (raw)
In-Reply-To: <20191105095428.GR2552@lahna.fi.intel.com>

On Tue, Nov 05, 2019 at 11:54:28AM +0200, Mika Westerberg wrote:
> On Mon, Nov 04, 2019 at 06:00:00PM -0600, Bjorn Helgaas wrote:

> > > If you think it is fine to do the delay before we have restored
> > > everything I can move it inside pci_power_up() or call it after
> > > pci_pm_default_resume_early() as above. I think at least we should make
> > > sure all the saved registers are restored before so that the link
> > > activation check actually works.
> > 
> > What needs to be restored to make pcie_wait_for_link_delay() work?
> 
> I'm not entirely sure. I think that pci_restore_state() at least should
> be called so that the PCIe capability gets restored. Maybe not even
> that because Data Link Layer Layer Active always reflects the DL_Active
> or not and it does not need to be enabled separately.
> 
> > And what event does the restore need to be ordered with?
> 
> Not sure I follow you here.

You're suggesting that we should restore saved registers first so
pcie_wait_for_link_delay() works.  If the link activation depends on
something being restored and we don't enforce an ordering, the
activation might succeed or fail depending on whether it happens
before or after the restore.  So if there is a dependency, we should
make it explicit to avoid a race like that.

But I'm not saying we *shouldn't* do the restore before the wait; only
that any dependency should be explicit.  Even if there is no actual
dependency it probably makes sense to do the restore first so it can
overlap with the hardware link training, which may reduce the time
pcie_wait_for_link_delay() has to wait when we do call it, e.g.,

  |-----------------|          link activation
     |-----|                   restore state
           |--------|          pcie_wait_for_link_delay()

whereas if we do the restore after waiting for the link to come up, it
probably takes longer:

  |-----------------|          link activation
     |--------------|          pcie_wait_for_link_delay()
                    |-----|    restore state

I actually suspect there *is* a dependency -- we should respect the
Target Link Speed and and width so the link resumes in the same
configuration it was before suspend.  And I suspect that may require
an explicit retrain after restoring PCI_EXP_LNKCTL2.

Bjorn