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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191105152832.GC2552@lahna.fi.intel.com>
Date:   Tue, 5 Nov 2019 17:28:32 +0200
From:   Mika Westerberg <mika.westerberg@...ux.intel.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Len Brown <lenb@...nel.org>, Lukas Wunner <lukas@...ner.de>,
        Keith Busch <keith.busch@...el.com>,
        Alex Williamson <alex.williamson@...hat.com>,
        Alexandru Gagniuc <mr.nuke.me@...il.com>,
        Kai-Heng Feng <kai.heng.feng@...onical.com>,
        Paul Menzel <pmenzel@...gen.mpg.de>,
        Nicholas Johnson <nicholas.johnson-opensource@...look.com.au>,
        linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] PCI: Add missing link delays required by the PCIe
 spec

On Tue, Nov 05, 2019 at 09:00:13AM -0600, Bjorn Helgaas wrote:
> 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.

OK thanks. By explicit you mean document it in the code, right?

> 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.

According the PCIe spec the PCI_EXP_LNKCTL2 Target Link Speed is marked
as RWS (S for sticky) so I suspect its value is retained after reset in
the same way as PME bits. Assuming I understood it correctly.