linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Alexandru Gagniuc <mr.nuke.me@gmail.com>
To: bhelgaas@google.com
Cc: austin_bolen@dell.com, alex_gagniuc@dellteam.com,
	keith.busch@intel.com, Shyam_Iyer@Dell.com, lukas@wunner.de,
	Alexandru Gagniuc <mr.nuke.me@gmail.com>,
	"Gustavo A. R. Silva" <gustavo@embeddedor.com>,
	linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: [PATCH] PCI: pciehp: Do not turn off slot if presence comes up after link
Date: Tue,  5 Feb 2019 15:06:56 -0600	[thread overview]
Message-ID: <20190205210701.25387-1-mr.nuke.me@gmail.com> (raw)

According to PCIe 3.0, the presence detect state is a logical OR of
in-band and out-of-band presence. With this, we'd expect the presence
state to always be asserted when the link comes up.

Not all hardware follows this, and it is possible for the presence to
come up after the link. In this case, the PCIe device would be
erroneously disabled and re-probed. It is possible to distinguish
between a delayed presence and a card swap by looking at the DLL state
changed bit -- The link has to come down if the card is removed.

Thus, for a device that is probed, present and has its link active, a
lack of a link state change event guarantees we have the same device,
and shutdown of is not needed.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---

Following some discussion in
"PCI: hotplug: Erroneous removal of hotplug PCI devices" [1]
It became apparent that we can't fully rely presence detect changed
as an indicator to shutdown a device. And in PCIe 4.0 the "logical OR"
requirement is going away, so we can use a way to slightly  decouple
presence detect and link active.

I think the approach here is the simplest, and least likely to
interfere with other mis-behaving hardware (in the PCIe 3.0 sense).

[1] https://www.spinics.net/lists/linux-pci/msg79564.html

 drivers/pci/hotplug/pciehp_ctrl.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index 3f3df4c29f6e..ea0dd3e956be 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -220,13 +220,23 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
 	/*
 	 * If the slot is on and presence or link has changed, turn it off.
 	 * Even if it's occupied again, we cannot assume the card is the same.
+	 * When the card is swapped, we also expect a change in link state,
+	 * without which, it's likely presence became high after link-active.
 	 */
 	mutex_lock(&ctrl->state_lock);
+	present = pciehp_card_present(ctrl);
+	link_active = pciehp_check_link_active(ctrl);
 	switch (ctrl->state) {
 	case BLINKINGOFF_STATE:
 		cancel_delayed_work(&ctrl->button_work);
 		/* fall through */
 	case ON_STATE:
+		if (present && link_active &&
+		   !(events & PCI_EXP_SLTSTA_DLLSC)) {
+			mutex_unlock(&ctrl->state_lock);
+			ctrl_warn(ctrl, "Hardware bug: Presence state came up after link");
+			return;
+		}
 		ctrl->state = POWEROFF_STATE;
 		mutex_unlock(&ctrl->state_lock);
 		if (events & PCI_EXP_SLTSTA_DLLSC)
-- 
2.19.2


             reply	other threads:[~2019-02-05 21:07 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-05 21:06 Alexandru Gagniuc [this message]
2019-02-08 19:56 ` [PATCH] PCI: pciehp: Do not turn off slot if presence comes up after link Bjorn Helgaas
2019-02-09 11:58 ` Lukas Wunner
2019-02-11 23:48   ` Alex_Gagniuc
2019-02-12  8:30     ` Lukas Wunner
2019-02-12 10:37       ` Lukas Wunner
2019-02-12 23:57       ` Alex_Gagniuc
2019-02-13  8:36         ` Lukas Wunner
2019-02-13 18:55           ` Alex_Gagniuc
2019-02-14  7:01             ` Lukas Wunner
2019-02-14 19:26               ` Alex_Gagniuc

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=20190205210701.25387-1-mr.nuke.me@gmail.com \
    --to=mr.nuke.me@gmail.com \
    --cc=Shyam_Iyer@Dell.com \
    --cc=alex_gagniuc@dellteam.com \
    --cc=austin_bolen@dell.com \
    --cc=bhelgaas@google.com \
    --cc=gustavo@embeddedor.com \
    --cc=keith.busch@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=lukas@wunner.de \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).