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 <[email protected]>
---
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
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 <[email protected]>
> ---
>
> 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
>
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.
Since Bjorn asked for a spec reference:
PCIe r4.0 sec 7.8.11: "Presence Detect State - This bit indicates the
presence of an adapter in the slot, reflected by the logical OR of the
Physical Layer in-band presence detect mechanism and, if present, any
out-of-band presence detect mechanism defined for the slot's
corresponding form factor."
Table 4-14 in sec 4.2.6 is also important because it shows the difference
between Link Up and in-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.
So in reality, PDC and DLLSC events rarely come in simultaneously and
we do allow some leeway in pcie_wait_for_link(), it's just not enough
in your particular case due to the way presence is detected by the
platform.
I think in this case instead of changing the behavior for everyone,
it's more appropriate to add a quirk for your hardware. Two ideas:
* Amend pcie_init() to set slot_cap |= PCI_EXP_SLTCAP_ABP. Insert
this as a quirk right below the existing Thunderbolt quirk and
use PCI vendor/device IDs and/or DMI checks to identify your
particular hardware. If the ABP bit is set, PDC events are not
enabled by pcie_enable_notification(), so you will solely rely on
DLLSC events. Problem solved. (Hopefully.)
* Alternatively, add a DECLARE_PCI_FIXUP_FINAL() quirk which sets
pdev->link_active_reporting = false. This causes pcie_wait_for_link()
to wait 1100 msec before the hot-added device's config space is
accessed for the first time.
Would this work for you?
Thanks,
Lukas
On 2/9/19 5:58 AM, Lukas Wunner wrote:
>
> [EXTERNAL EMAIL]
>
> 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.
>
> Since Bjorn asked for a spec reference:
>
> PCIe r4.0 sec 7.8.11: "Presence Detect State - This bit indicates the
> presence of an adapter in the slot, reflected by the logical OR of the
> Physical Layer in-band presence detect mechanism and, if present, any
> out-of-band presence detect mechanism defined for the slot's
> corresponding form factor."
ECN [1] was approved last November, so it's normative spec text. Sorry
if the Ukranians didn't get ahold of it yet. I'll try to explain the delta.
There's an in-band PD supported/disable set of bits. If 'supported' is
set, then you can set 'disable', which removes the 'logical OR" and now
PD is only out-of-band presence.
> Table 4-14 in sec 4.2.6 is also important because it shows the difference
> between Link Up and in-band presence.
So, we'd expect PD to come up at the same time or before DLL?
>> 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.
>
> So in reality, PDC and DLLSC events rarely come in simultaneously and
> we do allow some leeway in pcie_wait_for_link(), it's just not enough
> in your particular case due to the way presence is detected by the
> platform.
>
> I think in this case instead of changing the behavior for everyone,
> it's more appropriate to add a quirk for your hardware. Two ideas:
>
> * Amend pcie_init() to set slot_cap |= PCI_EXP_SLTCAP_ABP. Insert
> this as a quirk right below the existing Thunderbolt quirk and
> use PCI vendor/device IDs and/or DMI checks to identify your
> particular hardware. If the ABP bit is set, PDC events are not
> enabled by pcie_enable_notification(), so you will solely rely on
> DLLSC events. Problem solved. (Hopefully.)
>
> * Alternatively, add a DECLARE_PCI_FIXUP_FINAL() quirk which sets
> pdev->link_active_reporting = false. This causes pcie_wait_for_link()
> to wait 1100 msec before the hot-added device's config space is
> accessed for the first time.
These are both hacks right? We're declaring something untrue about the
hardware because we want to obtain a specific side-effect. BUt the
side-effects are not guaranteed not to change.
> Would this work for you?
I'm certain it's doable. In theory one could use the PCI ID of the DP,
and the vendor and machine names to filter the quirk. But what happens
in situations where the same PCI ID switch is used in different parts of
the system? You want the quirk here or not there. It quickly becomes a
shartstorm.
I'd like a generic solution. I suspect supporting 'in-band PD disabled'
and quirking for that would be a saner way to do things. If we support
it, then we need to handle PDSC after DLLSC scenarios anyway.
Alex
[1] PCI-SIG ENGINEERING CHANGE NOTICE
Async Hot-Plug Updates
Nov 29, 2018
On Mon, Feb 11, 2019 at 11:48:12PM +0000, [email protected] wrote:
> On 2/9/19 5:58 AM, Lukas Wunner wrote:
> ECN [1] was approved last November, so it's normative spec text. Sorry
> if the Ukranians didn't get ahold of it yet. I'll try to explain the delta.
> There's an in-band PD supported/disable set of bits. If 'supported' is
> set, then you can set 'disable', which removes the 'logical OR" and now
> PD is only out-of-band presence.
I see, thanks a lot for relaying this information. The PCI SIG
should probably consider granting access to specs to open source
developers to ease adoption of new features in Linux et al.
> > Table 4-14 in sec 4.2.6 is also important because it shows the difference
> > between Link Up and in-band presence.
>
> So, we'd expect PD to come up at the same time or before DLL?
Per table 4-14 and figure 4-23 (immediately below the table) in r4.0,
PDC ought to be signaled before DLLSC. As said, Linux can handle PDC
after DLLSC if they're not too far apart (100 ms, see pcie_wait_for_link()).
> I'd like a generic solution. I suspect supporting 'in-band PD disabled'
> and quirking for that would be a saner way to do things. If we support
> it, then we need to handle PDSC after DLLSC scenarios anyway.
I agree, having support for this new ECN would be great.
If you look at struct controller in drivers/pci/hotplug/pciehp.h,
there's a section labelled /* capabilities and quirks */. An idea
would be to add an unsigned int there to indicate whether in-band
presence is disabled. An alternative would be to add a flag to
struct pci_dev.
Instead of modifying the logic in pciehp_handle_presence_or_link_change(),
you could amend pcie_wait_for_link() to poll PDS until it's set, in
addition to DLLLA. The rationale would be that although the link is up,
the hot-added device cannot really be considered accessible until PDS
is also set. Unfortunately we cannot do this for all devices because
(as I've said before), some broken devices hardwire PDS to zero. But
it should be safe to constrain it to those which can disable in-band
presence.
Be mindful however that pcie_wait_for_link() is also called from the
DPC driver. Keith should be able to judge whether a change to that
function breaks DPC.
You'll probably also need to add code to disable in-band presence if
that is supported, either in pcie_init() in pciehp_hpc.c or in generic
PCI code.
Thanks,
Lukas
On Tue, Feb 12, 2019 at 09:30:31AM +0100, Lukas Wunner wrote:
> Instead of modifying the logic in pciehp_handle_presence_or_link_change(),
> you could amend pcie_wait_for_link() to poll PDS until it's set, in
> addition to DLLLA. The rationale would be that although the link is up,
> the hot-added device cannot really be considered accessible until PDS
> is also set. Unfortunately we cannot do this for all devices because
> (as I've said before), some broken devices hardwire PDS to zero. But
> it should be safe to constrain it to those which can disable in-band
> presence.
An alternative approach would of course be to only enable one of PDC or
DLLSC events if in-band presence is disabled. In the case of DLLSC,
you'd also need to modify pciehp_reset_slot() in addition to
pcie_enable_notification().
Thanks,
Lukas
On 2/12/19 2:30 AM, Lukas Wunner wrote:
> The PCI SIG
> should probably consider granting access to specs to open source
> developers to ease adoption of new features in Linux et al.
Don't get me started on just how ridiculous I think PCI-SIG's policy is
with respect to public availability of standards.
>>> Table 4-14 in sec 4.2.6 is also important because it shows the difference
>>> between Link Up and in-band presence.
>>
>> So, we'd expect PD to come up at the same time or before DLL?
>
> Per table 4-14 and figure 4-23 (immediately below the table) in r4.0,
> PDC ought to be signaled before DLLSC. As said, Linux can handle PDC
> after DLLSC if they're not too far apart (100 ms, see pcie_wait_for_link()).
>
>
>> I'd like a generic solution. I suspect supporting 'in-band PD disabled'
>> and quirking for that would be a saner way to do things. If we support
>> it, then we need to handle PDSC after DLLSC scenarios anyway.
>
> I agree, having support for this new ECN would be great.
>
> If you look at struct controller in drivers/pci/hotplug/pciehp.h,
> there's a section labelled /* capabilities and quirks */. An idea
> would be to add an unsigned int there to indicate whether in-band
> presence is disabled. An alternative would be to add a flag to
> struct pci_dev.
> Instead of modifying the logic in pciehp_handle_presence_or_link_change(),
> you could amend pcie_wait_for_link() to poll PDS until it's set, in
> addition to DLLLA. The rationale would be that although the link is up,
> the hot-added device cannot really be considered accessible until PDS
> is also set. Unfortunately we cannot do this for all devices because
> (as I've said before), some broken devices hardwire PDS to zero. But
> it should be safe to constrain it to those which can disable in-band
> presence.
The recommendation is to set the "in-band PD disable" bit, and it's
possible that platform firmware may have set it at boot time (*). I'm
not sure that there is a spec-justifiable reason to not access a device
whose DLL is up, but PD isn't.
(*) A bit hypothetical: There is no hardware yet implementing the ECN.
> Be mindful however that pcie_wait_for_link() is also called from the
> DPC driver. Keith should be able to judge whether a change to that
> function breaks DPC.
That's why I went for ammending pciehp_handle_presence_or_link_change().
Smaller bug surface ;). What I'm thinking at this point is, keep the
patch as is, but, also check for in-band PD disable before aborting the
shutdown. Old behavior stays the same.
I'll rework this a little bit for in-band PD disable (what's a good
acronym for that, BTW?), and then quirk those machines that are known to
'disable' this in hardware.
Alex
On Tue, Feb 12, 2019 at 11:57:55PM +0000, [email protected] wrote:
> The recommendation is to set the "in-band PD disable" bit, and it's
> possible that platform firmware may have set it at boot time
Ok, then I'd suggest to check in pcie_init() whether the feature is
supported and enable it if so. And store that fact in a flag, either
in struct pci_dev or struct controller.
> (*) A bit hypothetical: There is no hardware yet implementing the ECN.
Hm, this contradicts Austin Bolen's e-mail of Jan 25 that "Yes, this
platform disables in-band presence" (when asked whether your host
controller already adheres to the ECN).
> I'm
> not sure that there is a spec-justifiable reason to not access a device
> whose DLL is up, but PD isn't.
Austin asked in an e-mail of Jan 24 whether "the hot-inserted device's
config space [is] accessed immediately after waiting this 20 + 100 ms
delay", which sounded to me like you'd prefer the device not to be
accessed until PDS is 1. This can be achieved by polling PDS in
pcie_wait_for_link() if in-band presence is disabled, which is why
I suggested it.
> > Be mindful however that pcie_wait_for_link() is also called from the
> > DPC driver. Keith should be able to judge whether a change to that
> > function breaks DPC.
>
> That's why I went for ammending pciehp_handle_presence_or_link_change().
> Smaller bug surface ;). What I'm thinking at this point is, keep the
> patch as is, but, also check for in-band PD disable before aborting the
> shutdown. Old behavior stays the same.
I'm worried that amending pciehp_handle_presence_or_link_change() makes
the event handling logic more difficult to understand. Polling PDS in
pcie_wait_for_link() or disabling either PDC or DLLSC if in-band presence
is disabled seems simpler to reason about.
> in-band PD disable (what's a good acronym for that, BTW?)
I don't know, maybe inband_presence_disabled?
Thanks,
Lukas
On 2/13/19 2:36 AM, Lukas Wunner wrote:
>> (*) A bit hypothetical: There is no hardware yet implementing the ECN.
>
> Hm, this contradicts Austin Bolen's e-mail of Jan 25 that "Yes, this
> platform disables in-band presence" (when asked whether your host
> controller already adheres to the ECN).
Both statements are true. The hardware does indeed disable in-band
presence, in a rudimentary way that is not compliant with the ECN -- it
doesn't implement the bits required by the ECN.
>> I'm
>> not sure that there is a spec-justifiable reason to not access a device
>> whose DLL is up, but PD isn't.
>
> Austin asked in an e-mail of Jan 24 whether "the hot-inserted device's
> config space [is] accessed immediately after waiting this 20 + 100 ms
> delay", which sounded to me like you'd prefer the device not to be
> accessed until PDS is 1.
"Unless Readiness Notifications mechanisms are used (see Section 6.23),
the Root Complex and/or system software must allow at least 1.0 s after
a Conventional Reset of a device, before it may determine that a device
which fails to return a Successful Completion status for a valid
Configuration Request is a broken device. This period is independent of
how quickly Link training completes."
(Section 6.6)
The concern was the one second delay, which is addressed by the use of
pci_bus_wait_crs().
>>> Be mindful however that pcie_wait_for_link() is also called from the
>>> DPC driver. Keith should be able to judge whether a change to that
>>> function breaks DPC.
>>
>> That's why I went for ammending pciehp_handle_presence_or_link_change().
>> Smaller bug surface ;). What I'm thinking at this point is, keep the
>> patch as is, but, also check for in-band PD disable before aborting the
>> shutdown. Old behavior stays the same.
>
> I'm worried that amending pciehp_handle_presence_or_link_change() makes
> the event handling logic more difficult to understand.
Don't worry. It's already difficult to understand ;)
> Polling PDS in
> pcie_wait_for_link() or disabling either PDC or DLLSC if in-band presence
> is disabled seems simpler to reason about.
pcie_wait_for_link() is generic PCIe layer. I don't think mixing hotplug
concepts is a good layering violation.
Disabling PDC or DLLSC can work. I've sometimes wondered why we even
care about PDC. I can imagine situations where platform might want to
signal imminent removal by yanking PD.
>> in-band PD disable (what's a good acronym for that, BTW?)
>
> I don't know, maybe inband_presence_disabled?
PCI_EXP_SLTCAP2_IBPD ?
On Wed, Feb 13, 2019 at 06:55:46PM +0000, [email protected] wrote:
> On 2/13/19 2:36 AM, Lukas Wunner wrote:
> > > (*) A bit hypothetical: There is no hardware yet implementing the ECN.
> >
> > Hm, this contradicts Austin Bolen's e-mail of Jan 25 that "Yes, this
> > platform disables in-band presence" (when asked whether your host
> > controller already adheres to the ECN).
>
> Both statements are true. The hardware does indeed disable in-band
> presence, in a rudimentary way that is not compliant with the ECN -- it
> doesn't implement the bits required by the ECN.
Ugh, can a BIOS update make those machines compliant to the ECN
or do we need a quirk specifically for them?
> > Polling PDS in
> > pcie_wait_for_link() or disabling either PDC or DLLSC if in-band presence
> > is disabled seems simpler to reason about.
>
> pcie_wait_for_link() is generic PCIe layer. I don't think mixing hotplug
> concepts is a good layering violation.
The function used to live in pciehp_hpc.c, but commits 9f5a70f18c58
and f0157160b359 moved it to generic code to allow code sharing with
the DPC driver. That's the only reason it's in generic code AFAICS.
> >> in-band PD disable (what's a good acronym for that, BTW?)
> >
> > I don't know, maybe inband_presence_disabled?
>
> PCI_EXP_SLTCAP2_IBPD ?
Yes, something like that. It should match the spec, which I have no
access to.
Thanks,
Lukas
On 2/14/19 1:01 AM, Lukas Wunner wrote:
> On Wed, Feb 13, 2019 at 06:55:46PM +0000, [email protected] wrote:
>> On 2/13/19 2:36 AM, Lukas Wunner wrote:
>>>> (*) A bit hypothetical: There is no hardware yet implementing the ECN.
>>>
>>> Hm, this contradicts Austin Bolen's e-mail of Jan 25 that "Yes, this
>>> platform disables in-band presence" (when asked whether your host
>>> controller already adheres to the ECN).
>>
>> Both statements are true. The hardware does indeed disable in-band
>> presence, in a rudimentary way that is not compliant with the ECN -- it
>> doesn't implement the bits required by the ECN.
>
> Ugh, can a BIOS update make those machines compliant to the ECN
No, that is not possible, I'm told.
> or do we need a quirk specifically for them?
We might have to.
> It should match the spec, which I have no access to.
I am as irritated as you that the spec is not readily available for
public viewing.
Cheers,
Alex