In older PCIe specs, PDS (presence detect) would come up when the
"in-band" presence detect pin connected, and would be up before DLLLA
(link active).
In PCIe 4.0 (as an ECN) and in PCIe 5.0, there is a new bit to show if
in-band presence detection can be disabled for the slot, and another bit
that disables it--and a recommendation that it should be disabled if it
can be. In addition, certain OEMs disable in-band presence detection
without implementing these bits.
This means it is possible to get a "card present" interrupt after the
link is up and the driver is loaded. This causes an erroneous removal
of the device driver, followed by an immediate re-probing.
This patch set defines these new bits, uses them to disable in-band
presence detection if it can be, waits for PDS to go up if in-band
presence detection is disabled, and adds a DMI table that will let us
know if we should assume in-band presence is disabled on a system.
This patch set is based on a patch set [1] submitted many months ago by
Alexandru Gagniuc, who is no longer working on it.
[1] https://patchwork.kernel.org/cover/10909167/
[v3,0/4] PCI: pciehp: Do not turn off slot if presence comes up after link
Stuart Hayes (3):
PCI: pciehp: Add support for disabling in-band presence
PCI: pciehp: Wait for PDS when in-band presence is disabled
PCI: pciehp: Add dmi table for systems with in-band presence disabled
drivers/pci/hotplug/pciehp.h | 1 +
drivers/pci/hotplug/pciehp_hpc.c | 45 +++++++++++++++++++++++++++++++-
include/uapi/linux/pci_regs.h | 2 ++
3 files changed, 47 insertions(+), 1 deletion(-)
--
2.18.1
The presence detect state (PDS) is normally a logical or of in-band and
out-of-band presence. As of PCIe 4.0, there is the option to disable
in-band presence so that the PDS bit always reflects the state of the
out-of-band presence.
The recommendation of the PCIe spec is to disable in-band presence
whenever supported.
Signed-off-by: Stuart Hayes <[email protected]>
---
drivers/pci/hotplug/pciehp.h | 1 +
drivers/pci/hotplug/pciehp_hpc.c | 9 ++++++++-
include/uapi/linux/pci_regs.h | 2 ++
3 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index 654c972b8ea0..27e4cd6529b0 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -83,6 +83,7 @@ struct controller {
struct pcie_device *pcie;
u32 slot_cap; /* capabilities and quirks */
+ unsigned int inband_presence_disabled:1;
u16 slot_ctrl; /* control register access */
struct mutex ctrl_lock;
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 1a522c1c4177..dc109d521f30 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -811,7 +811,7 @@ static inline void dbg_ctrl(struct controller *ctrl)
struct controller *pcie_init(struct pcie_device *dev)
{
struct controller *ctrl;
- u32 slot_cap, link_cap;
+ u32 slot_cap, slot_cap2, link_cap;
u8 poweron;
struct pci_dev *pdev = dev->port;
struct pci_bus *subordinate = pdev->subordinate;
@@ -869,6 +869,13 @@ struct controller *pcie_init(struct pcie_device *dev)
FLAG(link_cap, PCI_EXP_LNKCAP_DLLLARC),
pdev->broken_cmd_compl ? " (with Cmd Compl erratum)" : "");
+ pcie_capability_read_dword(pdev, PCI_EXP_SLTCAP2, &slot_cap2);
+ if (slot_cap2 & PCI_EXP_SLTCAP2_IBPD) {
+ pcie_write_cmd_nowait(ctrl, PCI_EXP_SLTCTL_IBPD_DISABLE,
+ PCI_EXP_SLTCTL_IBPD_DISABLE);
+ ctrl->inband_presence_disabled = 1;
+ }
+
/*
* If empty slot's power status is on, turn power off. The IRQ isn't
* requested yet, so avoid triggering a notification with this command.
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 29d6e93fd15e..ea1cf9546e4d 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -604,6 +604,7 @@
#define PCI_EXP_SLTCTL_PWR_OFF 0x0400 /* Power Off */
#define PCI_EXP_SLTCTL_EIC 0x0800 /* Electromechanical Interlock Control */
#define PCI_EXP_SLTCTL_DLLSCE 0x1000 /* Data Link Layer State Changed Enable */
+#define PCI_EXP_SLTCTL_IBPD_DISABLE 0x4000 /* In-band PD disable */
#define PCI_EXP_SLTSTA 26 /* Slot Status */
#define PCI_EXP_SLTSTA_ABP 0x0001 /* Attention Button Pressed */
#define PCI_EXP_SLTSTA_PFD 0x0002 /* Power Fault Detected */
@@ -676,6 +677,7 @@
#define PCI_EXP_LNKSTA2 50 /* Link Status 2 */
#define PCI_CAP_EXP_ENDPOINT_SIZEOF_V2 52 /* v2 endpoints with link end here */
#define PCI_EXP_SLTCAP2 52 /* Slot Capabilities 2 */
+#define PCI_EXP_SLTCAP2_IBPD 0x0001 /* In-band PD Disable Supported */
#define PCI_EXP_SLTCTL2 56 /* Slot Control 2 */
#define PCI_EXP_SLTSTA2 58 /* Slot Status 2 */
--
2.18.1
Some systems have in-band presence detection disabled for hot-plug PCI slots,
but do not report this in the slot capabilities 2 (SLTCAP2) register. On
these systems, presence detect can become active well after the link is
reported to be active, which can cause the slots to be disabled after a
device is connected.
Add a dmi table to flag these systems as having in-band presence disabled.
Signed-off-by: Stuart Hayes <[email protected]>
---
drivers/pci/hotplug/pciehp_hpc.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 1282641c6458..1dd01e752587 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -14,6 +14,7 @@
#define dev_fmt(fmt) "pciehp: " fmt
+#include <linux/dmi.h>
#include <linux/kernel.h>
#include <linux/types.h>
#include <linux/jiffies.h>
@@ -26,6 +27,16 @@
#include "../pci.h"
#include "pciehp.h"
+static const struct dmi_system_id inband_presence_disabled_dmi_table[] = {
+ {
+ .ident = "Dell System",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc"),
+ },
+ },
+ {}
+};
+
static inline struct pci_dev *ctrl_dev(struct controller *ctrl)
{
return ctrl->pcie->port;
@@ -898,6 +909,9 @@ struct controller *pcie_init(struct pcie_device *dev)
ctrl->inband_presence_disabled = 1;
}
+ if (dmi_first_match(inband_presence_disabled_dmi_table))
+ ctrl->inband_presence_disabled = 1;
+
/*
* If empty slot's power status is on, turn power off. The IRQ isn't
* requested yet, so avoid triggering a notification with this command.
--
2.18.1
On 10/1/19 4:14 PM, Stuart Hayes wrote:
> Some systems have in-band presence detection disabled for hot-plug PCI slots,
> but do not report this in the slot capabilities 2 (SLTCAP2) register. On
> these systems, presence detect can become active well after the link is
> reported to be active, which can cause the slots to be disabled after a
> device is connected.
>
> Add a dmi table to flag these systems as having in-band presence disabled.
>
> Signed-off-by: Stuart Hayes <[email protected]>
> ---
> drivers/pci/hotplug/pciehp_hpc.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index 1282641c6458..1dd01e752587 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -14,6 +14,7 @@
>
> #define dev_fmt(fmt) "pciehp: " fmt
>
> +#include <linux/dmi.h>
> #include <linux/kernel.h>
> #include <linux/types.h>
> #include <linux/jiffies.h>
> @@ -26,6 +27,16 @@
> #include "../pci.h"
> #include "pciehp.h"
>
> +static const struct dmi_system_id inband_presence_disabled_dmi_table[] = {
> + {
> + .ident = "Dell System",
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc"),
> + },
> + },
> + {}
> +};
> +
I'm not sure that all Dell systems that were ever made or will be made
have in-band presence disabled on all their hotplug slots.
This was a problem with the NVMe hot-swap bays on 14G servers. I can't
guarantee that any other slot or machine will need this workaround. The
best way I found to implement this is to check for the PCI-ID of the
switches behind the port.
Alex
On Tue, Oct 1, 2019 at 4:36 PM Alex G. <[email protected]> wrote:
>
>
>
> On 10/1/19 4:14 PM, Stuart Hayes wrote:
> > Some systems have in-band presence detection disabled for hot-plug PCI slots,
> > but do not report this in the slot capabilities 2 (SLTCAP2) register. On
> > these systems, presence detect can become active well after the link is
> > reported to be active, which can cause the slots to be disabled after a
> > device is connected.
> >
> > Add a dmi table to flag these systems as having in-band presence disabled.
> >
> > Signed-off-by: Stuart Hayes <[email protected]>
> > ---
> > drivers/pci/hotplug/pciehp_hpc.c | 14 ++++++++++++++
> > 1 file changed, 14 insertions(+)
> >
> > diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> > index 1282641c6458..1dd01e752587 100644
> > --- a/drivers/pci/hotplug/pciehp_hpc.c
> > +++ b/drivers/pci/hotplug/pciehp_hpc.c
> > @@ -14,6 +14,7 @@
> >
> > #define dev_fmt(fmt) "pciehp: " fmt
> >
> > +#include <linux/dmi.h>
> > #include <linux/kernel.h>
> > #include <linux/types.h>
> > #include <linux/jiffies.h>
> > @@ -26,6 +27,16 @@
> > #include "../pci.h"
> > #include "pciehp.h"
> >
> > +static const struct dmi_system_id inband_presence_disabled_dmi_table[] = {
> > + {
> > + .ident = "Dell System",
> > + .matches = {
> > + DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc"),
> > + },
> > + },
> > + {}
> > +};
> > +
>
> I'm not sure that all Dell systems that were ever made or will be made
> have in-band presence disabled on all their hotplug slots.
>
> This was a problem with the NVMe hot-swap bays on 14G servers. I can't
> guarantee that any other slot or machine will need this workaround. The
> best way I found to implement this is to check for the PCI-ID of the
> switches behind the port.
>
> Alex
That is definitely true, not all Dell systems actually disable in-band
presence detect and need this. However, we have a number of systems
that need this, and we don't have the PCI IDs for all of these yet, so
we decided it was preferable to just make all Dell systems wait for
presence detect to go active. Since all of our systems support
presence detect (either in-band or out-of-band), it shouldn't have any
negative effects--on systems that support in-band presence, it will
already be active and it won't spend any extra time waiting for it.
If someone plugs in a device that has hot-plug slots with no support
for presence detect at all (even though Dell doesn't support any), it
should still work--it'll just take an extra second for them to come
up.
On Tue, Oct 01, 2019 at 05:14:16PM -0400, Stuart Hayes wrote:
> This patch set is based on a patch set [1] submitted many months ago by
> Alexandru Gagniuc, who is no longer working on it.
>
> [1] https://patchwork.kernel.org/cover/10909167/
> [v3,0/4] PCI: pciehp: Do not turn off slot if presence comes up after link
If I'm not mistaken, these two are identical to Alex' patches, right?
PCI: pciehp: Add support for disabling in-band presence
PCI: pciehp: Wait for PDS when in-band presence is disabled
I'm not sure if it's appropriate to change the author and
omit Alex' Signed-off-by.
Otherwise I have no objections against this series.
Thanks,
Lukas
On Tue, Oct 1, 2019 at 11:13 PM Lukas Wunner <[email protected]> wrote:
>
> On Tue, Oct 01, 2019 at 05:14:16PM -0400, Stuart Hayes wrote:
> > This patch set is based on a patch set [1] submitted many months ago by
> > Alexandru Gagniuc, who is no longer working on it.
> >
> > [1] https://patchwork.kernel.org/cover/10909167/
> > [v3,0/4] PCI: pciehp: Do not turn off slot if presence comes up after link
>
> If I'm not mistaken, these two are identical to Alex' patches, right?
>
> PCI: pciehp: Add support for disabling in-band presence
> PCI: pciehp: Wait for PDS when in-band presence is disabled
>
> I'm not sure if it's appropriate to change the author and
> omit Alex' Signed-off-by.
>
> Otherwise I have no objections against this series.
>
> Thanks,
>
> Lukas
Thanks! The first patch is identical to the one Alex submitted, and
the second is nearly so... they both basically his work. I wasn't
sure what proper etiquette was--I was thinking the signed-off-by was
taking responsibility that the patch was ok (functional, not
copyrighted by someone else, etc) rather than giving credit, but he
definitely deserves credit for them. I'm happy to add a signed-off-by
for Alex on the first two and resubmit if he doesn't object.
On 10/1/19 11:13 PM, Lukas Wunner wrote:
> On Tue, Oct 01, 2019 at 05:14:16PM -0400, Stuart Hayes wrote:
>> This patch set is based on a patch set [1] submitted many months ago by
>> Alexandru Gagniuc, who is no longer working on it.
>>
>> [1] https://patchwork.kernel.org/cover/10909167/
>> [v3,0/4] PCI: pciehp: Do not turn off slot if presence comes up after link
>
> If I'm not mistaken, these two are identical to Alex' patches, right?
>
> PCI: pciehp: Add support for disabling in-band presence
> PCI: pciehp: Wait for PDS when in-band presence is disabled
>
> I'm not sure if it's appropriate to change the author and
> omit Alex' Signed-off-by.
Legally Dell owns the patches. I have no objection on my end.
Alex
> Otherwise I have no objections against this series.
>
> Thanks,
>
> Lukas
>
On Wed, Oct 02, 2019 at 05:13:58PM -0500, Alex G. wrote:
> On 10/1/19 11:13 PM, Lukas Wunner wrote:
> > On Tue, Oct 01, 2019 at 05:14:16PM -0400, Stuart Hayes wrote:
> > > This patch set is based on a patch set [1] submitted many months ago by
> > > Alexandru Gagniuc, who is no longer working on it.
> >
> > I'm not sure if it's appropriate to change the author and
> > omit Alex' Signed-off-by.
>
> Legally Dell owns the patches. I have no objection on my end.
From a kernel community POV, I don't think it matters (in this case)
who legally owns the copyright to the contributed code. It's just that
we go to great lengths to provide proper attribution even for small
contributions (e.g. Tested-by).
The benefit to the community is that we know who to cc if that portion
of the code is changed again and someone knowledgable should take a look.
The benefit to contributors is that they can change jobs and their past
contributions are still visible in the git history and attributed to
their names. By contrast, if you've worked on closed source code and
changed jobs, that work isn't visible to future employers or even yourself,
and it may happen that someone else takes credit for your past work
without you even knowing about it or being able to stop that.
(I've seen it before.)
In this case, there should be a S-o-b line for Alex preceding that
for Stuart, and the author of the commit should be Alex unless a
significant portion of the patch was changed.
Thanks,
Lukas