From: Keith Busch <email@example.com> To: Lukas Wunner <firstname.lastname@example.org> Cc: Bjorn Helgaas <email@example.com>, Sathyanarayanan Kuppuswamy <firstname.lastname@example.org>, Dan Williams <email@example.com>, Ethan Zhao <firstname.lastname@example.org>, Sinan Kaya <email@example.com>, Ashok Raj <firstname.lastname@example.org>, email@example.com, Russell Currey <firstname.lastname@example.org>, Oliver O'Halloran <email@example.com>, Stuart Hayes <firstname.lastname@example.org>, Mika Westerberg <email@example.com> Subject: Re: [PATCH] PCI: pciehp: Ignore Link Down/Up caused by DPC Date: Fri, 30 Apr 2021 06:16:41 +0900 [thread overview] Message-ID: <20210429211641.GB26517@redsun51.ssa.fujisawa.hgst.com> (raw) In-Reply-To: <20210429201603.GA18851@wunner.de> On Thu, Apr 29, 2021 at 10:16:03PM +0200, Lukas Wunner wrote: > On Fri, Apr 30, 2021 at 04:36:48AM +0900, Keith Busch wrote: > > On Sun, Mar 28, 2021 at 10:52:00AM +0200, Lukas Wunner wrote: > > > Downstream Port Containment (PCIe Base Spec, sec. 6.2.10) disables the > > > link upon an error and attempts to re-enable it when instructed by the > > > DPC driver. > > > > > > A slot which is both DPC- and hotplug-capable is currently brought down > > > by pciehp once DPC is triggered (due to the link change) and brought up > > > on successful recovery. That's undesirable, the slot should remain up > > > so that the hotplugged device remains bound to its driver. DPC notifies > > > the driver of the error and of successful recovery in pcie_do_recovery() > > > and the driver may then restore the device to working state. > > > > This is a bit strange. The PCIe spec says DPC capable ports suppress > > Link Down events specifically because it will confuse hot-plug > > surprise ports if you don't do that. I'm specifically looking at the > > "Implementation Note" in PCIe Base Spec 5.0 section 188.8.131.52. > > I suppose you mean 184.108.40.206? Oops, yes. > "Similarly, it is recommended that a Port that supports DPC not > Set the Hot-Plug Surprise bit in the Slot Capabilities register. > Having this bit Set blocks the reporting of Surprise Down errors, > preventing DPC from being triggered by this important error, > greatly reducing the benefit of DPC." > > The way I understand this, DPC isn't triggered on Surprise Down if > the port supports surprise removal. Hm, that might be correct, but not sure. I thought the intention was surprise down doesn't trigger on link down if it was because of DPC. > However what this patch aims to fix is the Link Down seen by pciehp > which is caused by DPC containing (other) errors. AER will take links down through the Secondary Bus Reset too, but that's not a problem there. The pciehp_reset_slot() suppresses the event. Can DPC use that? > It seems despite the above-quoted recommendation against it, vendors > do ship ports which support both DPC and surprise removal. > > > > Do these ports have out-of-band Precense Detect capabilities? If so, we > > can ignore Link Down events on DPC capable ports as long as PCIe Slot > > Status PDC isn't set. > > Hm, and what about ports with in-band Presence Detect? That can't be distinguishable from an actual device swap in that case. Suppressing the removal could theoretically bring-up a completely different device as if it were the same one. The NVMe driver replays known device's security keys on initialization. Hot-swap attack?
prev parent reply other threads:[~2021-04-29 21:16 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-03-28 8:52 Lukas Wunner 2021-03-30 20:53 ` Kuppuswamy, Sathyanarayanan 2021-04-28 0:39 ` Kuppuswamy, Sathyanarayanan 2021-04-28 1:42 ` Zhao, Haifeng 2021-04-28 10:08 ` Yicong Yang 2021-04-28 14:40 ` Lukas Wunner 2021-04-29 11:29 ` Yicong Yang 2021-04-29 12:40 ` Zhao, Haifeng 2021-04-29 19:42 ` Lukas Wunner 2021-04-30 8:47 ` Yicong Yang 2021-04-30 12:15 ` Lukas Wunner 2021-04-29 19:36 ` Keith Busch 2021-04-29 20:16 ` Lukas Wunner 2021-04-29 21:16 ` Keith Busch [this message]
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210429211641.GB26517@redsun51.ssa.fujisawa.hgst.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --subject='Re: [PATCH] PCI: pciehp: Ignore Link Down/Up caused by DPC' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).