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]

Date:   Tue, 5 Nov 2019 10:10:17 -0600
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Mika Westerberg <mika.westerberg@...ux.intel.com>
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 05:28:32PM +0200, Mika Westerberg wrote:
> 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?

So far all we have is a feeling that maybe we ought to restore before
waiting, but I don't really know why.  If there's an actual
dependency, we should chase down specifically what it is and add a
comment or code (e.g., a link retrain) as appropriate.

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

This patch is about coming from D3cold, isn't it?  I don't think we
can assume sticky bits are preserved in D3cold (except maybe when
auxiliary power is enabled).