From: Bjorn Helgaas <helgaas@kernel.org>
To: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Cc: austin_bolen@dell.com, alex_gagniuc@dellteam.com,
keith.busch@intel.com, Shyam_Iyer@Dell.com, lukas@wunner.de,
"Gustavo A. R. Silva" <gustavo@embeddedor.com>,
linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] PCI: pciehp: Do not turn off slot if presence comes up after link
Date: Fri, 8 Feb 2019 13:56:01 -0600 [thread overview]
Message-ID: <20190208195601.GX7268@google.com> (raw)
In-Reply-To: <20190205210701.25387-1-mr.nuke.me@gmail.com>
On Tue, Feb 05, 2019 at 03:06:56PM -0600, Alexandru Gagniuc wrote:
> 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.
Do you have a PCIe 4.0 spec? I think 5.0 is about to come out, so
it'd be nice to have at least a 4.0 citation (including section
number), if you have one.
> 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.
s/of is/is/, I guess?
I'm hoping Lukas will chime in here; thanks for cc'ing him already.
> 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
Would you mind updating this reference to the
https://lore.kernel.org/linux-pci form that doesn't depend on external
organizations?
https://www.kernel.org/lore.html has some details, but unfortunately
lacks a hint about how to construct the URL. What I do is look up the
Message-ID and use, e.g.,
https://lore.kernel.org/linux-pci/<Message-ID>
> 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");
I'm not 100% sure this is worth KERN_WARN unless the user might be
able to do something about it.
> + return;
> + }
> ctrl->state = POWEROFF_STATE;
> mutex_unlock(&ctrl->state_lock);
> if (events & PCI_EXP_SLTSTA_DLLSC)
> --
> 2.19.2
>
next prev parent reply other threads:[~2019-02-08 19:56 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-05 21:06 [PATCH] PCI: pciehp: Do not turn off slot if presence comes up after link Alexandru Gagniuc
2019-02-08 19:56 ` Bjorn Helgaas [this message]
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=20190208195601.GX7268@google.com \
--to=helgaas@kernel.org \
--cc=Shyam_Iyer@Dell.com \
--cc=alex_gagniuc@dellteam.com \
--cc=austin_bolen@dell.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 \
--cc=mr.nuke.me@gmail.com \
/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).