[PATCH v4 0/3] PCI: pciehp: Do not turn off slot if presence comes up after link

[PATCH v4 0/3] PCI: pciehp: Do not turn off slot if presence comes up after link

[Stuart Hayes]

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.

The first two patches in this set come from a patch set that was
submitted but not accepted many months ago by Alexandru Gagniuc [1].
The first is unmodified, the second has the commit message and timeout 
modified.

[1] https://patchwork.kernel.org/cover/10909167/
    [v3,0/4] PCI: pciehp: Do not turn off slot if presence comes up after link

v2:
- modify loop in pcie_wait_for_presence to do..while

v3:
- remove unused variable declaration
- modify text of warning message

v4:
- remove "!!" boolean conversion in an "if" condition for readability
- add explanation comment in dmi table

Alexandru Gagniuc (2):
  PCI: pciehp: Add support for disabling in-band presence
  PCI: pciehp: Wait for PDS if in-band presence is disabled

Stuart Hayes (1):
  PCI: pciehp: Add dmi table for in-band presence disabled

 drivers/pci/hotplug/pciehp.h     |  1 +
 drivers/pci/hotplug/pciehp_hpc.c | 50 +++++++++++++++++++++++++++++++-
 include/uapi/linux/pci_regs.h    |  2 ++
 3 files changed, 52 insertions(+), 1 deletion(-)

-- 
2.18.1

[PATCH v4 1/3] PCI: pciehp: Add support for disabling in-band presence

[Stuart Hayes]

From: Alexandru Gagniuc <mr.nuke.me@gmail.com>

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: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 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

Re: [PATCH v4 1/3] PCI: pciehp: Add support for disabling in-band presence

[Andy Shevchenko]

On Fri, Oct 25, 2019 at 10:00 PM Stuart Hayes <stuart.w.hayes@gmail.com> wrote:
>
> From: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>
> 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.
>

FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  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
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 1/3] PCI: pciehp: Add support for disabling in-band presence

[Bjorn Helgaas]

On Fri, Oct 25, 2019 at 03:00:45PM -0400, Stuart Hayes wrote:
> From: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> 
> 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.

I think I'm fine with this patch, but I would like to include the
specific reference for this recommendation.  If you have it handy, I
can just insert it.

> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  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
> 

Re: [PATCH v4 1/3] PCI: pciehp: Add support for disabling in-band presence

[Stuart Hayes]

On Tue, Nov 26, 2019 at 7:36 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Oct 25, 2019 at 03:00:45PM -0400, Stuart Hayes wrote:
> > From: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> >
> > 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.
>
> I think I'm fine with this patch, but I would like to include the
> specific reference for this recommendation.  If you have it handy, I
> can just insert it.
>

The PCI Express Base Specification Revision 5.0, Version 1.0, in the
implementation note under Appendix I ("Async Hot-Plug Reference
Model"), it says "If OOB PD is being used and the associated DSP
supports In-Band PD Disable, it is recommended that the In-Band PD
Disable bit be Set, ..."


> > Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> > ---
> >  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
> >

Re: [PATCH v4 1/3] PCI: pciehp: Add support for disabling in-band presence

[Stuart Hayes]

On 11/26/19 8:19 PM, Stuart Hayes wrote:
> On Tue, Nov 26, 2019 at 7:36 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>>
>> On Fri, Oct 25, 2019 at 03:00:45PM -0400, Stuart Hayes wrote:
>>> From: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>>>
>>> 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.
>>
>> I think I'm fine with this patch, but I would like to include the
>> specific reference for this recommendation.  If you have it handy, I
>> can just insert it.
>>
> 
> The PCI Express Base Specification Revision 5.0, Version 1.0, in the
> implementation note under Appendix I ("Async Hot-Plug Reference
> Model"), it says "If OOB PD is being used and the associated DSP
> supports In-Band PD Disable, it is recommended that the In-Band PD
> Disable bit be Set, ..."
> 
> 

Is that what you were looking for?  Please let me know if there's anything
else I can do to help.

Re: [PATCH v4 1/3] PCI: pciehp: Add support for disabling in-band presence

[Libor Pechacek]

On Tue 31-12-19 16:06:45, Stuart Hayes wrote:
> On 11/26/19 8:19 PM, Stuart Hayes wrote:
> > On Tue, Nov 26, 2019 at 7:36 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
[...]
> >> I think I'm fine with this patch, but I would like to include the
> >> specific reference for this recommendation.  If you have it handy, I
> >> can just insert it.
> >>
> > 
> > The PCI Express Base Specification Revision 5.0, Version 1.0, in the
> > implementation note under Appendix I ("Async Hot-Plug Reference
> > Model"), it says "If OOB PD is being used and the associated DSP
> > supports In-Band PD Disable, it is recommended that the In-Band PD
> > Disable bit be Set, ..."
> > 
> > 
> 
> Is that what you were looking for?  Please let me know if there's anything
> else I can do to help.

Gentlemen,

I'm wondering about that is the status of this patch set. Reading through the
discussion, it looks to me that it has been accepted. However, I cannot find
the corresponding changes in Bjorn's Git tree.

I'm asking because we've got a request for including the patches in our distro
and I'd like to shepherd the changes there. Knowing if, and possibly also when,
the patch set will hit the repo would help me plan my actions. If, by any
chance, I can help here, let me know.

Thanks!

Libor
-- 
Libor Pechacek
SUSE Labs                                Remember to have fun...

[PATCH v4 2/3] PCI: pciehp: Wait for PDS if in-band presence is disabled

[Stuart Hayes]

From: Alexandru Gagniuc <mr.nuke.me@gmail.com>

When inband presence is disabled, PDS may come up at any time, or not
at all. PDS being low may indicate that the card is still mating, and
we could expect contact bounce to bring down the link as well.

It is reasonable to assume that most cards will mate in a hotplug slot
in about a second. Thus, when we know PDS only reflects out-of-band
presence, it's worthwhile to wait the extra second or so to make sure
the card is properly mated before loading the driver, and to prevent
the hotplug code from disabling a device if the presence detect change
goes active after the device is enabled.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
---
v2:
  replace while(true) loop with do...while
v3
  remove unused variable declaration (pds)
  modify text of warning message
v4
  remove "!!" boolean conversion from "if" condition for readability

 drivers/pci/hotplug/pciehp_hpc.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index dc109d521f30..02d95ab27a12 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -242,6 +242,22 @@ static bool pci_bus_check_dev(struct pci_bus *bus, int devfn)
 	return found;
 }
 
+static void pcie_wait_for_presence(struct pci_dev *pdev)
+{
+	int timeout = 1250;
+	u16 slot_status;
+
+	do {
+		pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
+		if (slot_status & PCI_EXP_SLTSTA_PDS)
+			return;
+		msleep(10);
+		timeout -= 10;
+	} while (timeout > 0);
+
+	pci_info(pdev, "Timeout waiting for Presence Detect state to be set\n");
+}
+
 int pciehp_check_link_status(struct controller *ctrl)
 {
 	struct pci_dev *pdev = ctrl_dev(ctrl);
@@ -251,6 +267,9 @@ int pciehp_check_link_status(struct controller *ctrl)
 	if (!pcie_wait_for_link(pdev, true))
 		return -1;
 
+	if (ctrl->inband_presence_disabled)
+		pcie_wait_for_presence(pdev);
+
 	found = pci_bus_check_dev(ctrl->pcie->port->subordinate,
 					PCI_DEVFN(0, 0));
 
-- 
2.18.1

Re: [PATCH v4 2/3] PCI: pciehp: Wait for PDS if in-band presence is disabled

[Andy Shevchenko]

On Fri, Oct 25, 2019 at 10:01 PM Stuart Hayes <stuart.w.hayes@gmail.com> wrote:
>
> From: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>
> When inband presence is disabled, PDS may come up at any time, or not
> at all. PDS being low may indicate that the card is still mating, and
> we could expect contact bounce to bring down the link as well.
>
> It is reasonable to assume that most cards will mate in a hotplug slot
> in about a second. Thus, when we know PDS only reflects out-of-band
> presence, it's worthwhile to wait the extra second or so to make sure
> the card is properly mated before loading the driver, and to prevent
> the hotplug code from disabling a device if the presence detect change
> goes active after the device is enabled.
>

FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>


> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
> ---
> v2:
>   replace while(true) loop with do...while
> v3
>   remove unused variable declaration (pds)
>   modify text of warning message
> v4
>   remove "!!" boolean conversion from "if" condition for readability
>
>  drivers/pci/hotplug/pciehp_hpc.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index dc109d521f30..02d95ab27a12 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -242,6 +242,22 @@ static bool pci_bus_check_dev(struct pci_bus *bus, int devfn)
>         return found;
>  }
>
> +static void pcie_wait_for_presence(struct pci_dev *pdev)
> +{
> +       int timeout = 1250;
> +       u16 slot_status;
> +
> +       do {
> +               pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
> +               if (slot_status & PCI_EXP_SLTSTA_PDS)
> +                       return;
> +               msleep(10);
> +               timeout -= 10;
> +       } while (timeout > 0);
> +
> +       pci_info(pdev, "Timeout waiting for Presence Detect state to be set\n");
> +}
> +
>  int pciehp_check_link_status(struct controller *ctrl)
>  {
>         struct pci_dev *pdev = ctrl_dev(ctrl);
> @@ -251,6 +267,9 @@ int pciehp_check_link_status(struct controller *ctrl)
>         if (!pcie_wait_for_link(pdev, true))
>                 return -1;
>
> +       if (ctrl->inband_presence_disabled)
> +               pcie_wait_for_presence(pdev);
> +
>         found = pci_bus_check_dev(ctrl->pcie->port->subordinate,
>                                         PCI_DEVFN(0, 0));
>
> --
> 2.18.1
>


-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 3/3] PCI: pciehp: Add dmi table for in-band presence disabled

[Stuart Hayes]

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 <stuart.w.hayes@gmail.com>
---
v4
  add comment to dmi table

 drivers/pci/hotplug/pciehp_hpc.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 02d95ab27a12..9541735bd0aa 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,24 @@
 #include "../pci.h"
 #include "pciehp.h"
 
+static const struct dmi_system_id inband_presence_disabled_dmi_table[] = {
+	/*
+	 * Match all Dell systems, as some Dell systems have inband
+	 * presence disabled on NVMe slots (but don't support the bit to
+	 * report it). Setting inband presence disabled should have no
+	 * negative effect, except on broken hotplug slots that never
+	 * assert presence detect--and those will still work, they will
+	 * just have a bit of extra delay before being probed.
+	 */
+	{
+		.ident = "Dell System",
+		.matches = {
+			DMI_MATCH(DMI_OEM_STRING, "Dell System"),
+		},
+	},
+	{}
+};
+
 static inline struct pci_dev *ctrl_dev(struct controller *ctrl)
 {
 	return ctrl->pcie->port;
@@ -895,6 +914,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

Re: [PATCH v4 3/3] PCI: pciehp: Add dmi table for in-band presence disabled

[Andy Shevchenko]

On Fri, Oct 25, 2019 at 10:01 PM Stuart Hayes <stuart.w.hayes@gmail.com> 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.
>

FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
> ---
> v4
>   add comment to dmi table
>
>  drivers/pci/hotplug/pciehp_hpc.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index 02d95ab27a12..9541735bd0aa 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,24 @@
>  #include "../pci.h"
>  #include "pciehp.h"
>
> +static const struct dmi_system_id inband_presence_disabled_dmi_table[] = {
> +       /*
> +        * Match all Dell systems, as some Dell systems have inband
> +        * presence disabled on NVMe slots (but don't support the bit to
> +        * report it). Setting inband presence disabled should have no
> +        * negative effect, except on broken hotplug slots that never
> +        * assert presence detect--and those will still work, they will
> +        * just have a bit of extra delay before being probed.
> +        */
> +       {
> +               .ident = "Dell System",
> +               .matches = {
> +                       DMI_MATCH(DMI_OEM_STRING, "Dell System"),
> +               },
> +       },
> +       {}
> +};
> +
>  static inline struct pci_dev *ctrl_dev(struct controller *ctrl)
>  {
>         return ctrl->pcie->port;
> @@ -895,6 +914,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
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 3/3] PCI: pciehp: Add dmi table for in-band presence disabled

[Bjorn Helgaas]

On Fri, Oct 25, 2019 at 03:00:47PM -0400, 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.

This doesn't seem quite accurate to me.  PCI_EXP_SLTCAP2_IBPD does not
actually tell us whether in-band presence detection is disabled.  It
only tells us whether it *can* be disabled.

I think I know what you mean, but this text suggests that
PCI_EXP_SLTCAP2_IBPD not being set is the defect, and I don't think it
is.  IIUC, even if PCI_EXP_SLTCAP2_IBPD were set,
PCI_EXP_SLTCTL_IBPD_DISABLE would have no effect because in-band
presence detect just isn't supported at all regardless of how we set
PCI_EXP_SLTCTL_IBPD_DISABLE.

> 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 <stuart.w.hayes@gmail.com>
> ---
> v4
>   add comment to dmi table
> 
>  drivers/pci/hotplug/pciehp_hpc.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index 02d95ab27a12..9541735bd0aa 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,24 @@
>  #include "../pci.h"
>  #include "pciehp.h"
>  
> +static const struct dmi_system_id inband_presence_disabled_dmi_table[] = {
> +	/*
> +	 * Match all Dell systems, as some Dell systems have inband
> +	 * presence disabled on NVMe slots (but don't support the bit to

Is there something that restricts these slots to being used only for
NVMe?  If not, I'd rather simply say that some Root Ports don't
support in-band presence detect.  You say it's "disabled", which
suggests that it could be *enabled*.  But I have the impression that
it's actually just not supported at all (or maybe it's disabled by the
BIOS via some non-architected mechanism).

> +	 * report it). Setting inband presence disabled should have no
> +	 * negative effect, except on broken hotplug slots that never
> +	 * assert presence detect--and those will still work, they will
> +	 * just have a bit of extra delay before being probed.
> +	 */
> +	{
> +		.ident = "Dell System",
> +		.matches = {
> +			DMI_MATCH(DMI_OEM_STRING, "Dell System"),
> +		},
> +	},
> +	{}
> +};
> +
>  static inline struct pci_dev *ctrl_dev(struct controller *ctrl)
>  {
>  	return ctrl->pcie->port;
> @@ -895,6 +914,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;

This doesn't seem quite right: the DMI table should only apply to
built-in ports, not to ports on plugin cards.

If we plug in a switch with hotplug-capable downstream ports, and
those ports do not advertise PCI_EXP_SLTCAP2_IBPD, I think this code
sets "inband_presence_disabled" for those ports even though it is not
disabled.

IIUC, that will make this plugin card behave differently in a Dell
system than it will in other systems, and that doesn't seem right to
me.

>  	/*
>  	 * 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
> 

Re: [PATCH v4 3/3] PCI: pciehp: Add dmi table for in-band presence disabled

[Stuart Hayes]

On 2/20/20 11:21 PM, Bjorn Helgaas wrote:
> On Fri, Oct 25, 2019 at 03:00:47PM -0400, 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.
> 
> This doesn't seem quite accurate to me.  PCI_EXP_SLTCAP2_IBPD does not
> actually tell us whether in-band presence detection is disabled.  It
> only tells us whether it *can* be disabled.
> 
> I think I know what you mean, but this text suggests that
> PCI_EXP_SLTCAP2_IBPD not being set is the defect, and I don't think it
> is.  IIUC, even if PCI_EXP_SLTCAP2_IBPD were set,
> PCI_EXP_SLTCTL_IBPD_DISABLE would have no effect because in-band
> presence detect just isn't supported at all regardless of how we set
> PCI_EXP_SLTCTL_IBPD_DISABLE.
> 

I agree, the patch notes aren't quite correct.  Many Dell systems use
out-of-band presence detect, and there is no way to enable in-band presence
detection.

So I guess the notes would be more accurate if they said something like this:

Some systems use out-of-band presence detection exclusively, and do not
support the SLTCAP2_IBPD and SLTCTL_IBPD_DISABLE bits, but they still need
to have "inband_presence_disabled" set.

Add a dmi table to flag these systems as having in-band presence disabled.

>> 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 <stuart.w.hayes@gmail.com>
>> ---
>> v4
>>   add comment to dmi table
>>
>>  drivers/pci/hotplug/pciehp_hpc.c | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
>> index 02d95ab27a12..9541735bd0aa 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,24 @@
>>  #include "../pci.h"
>>  #include "pciehp.h"
>>  
>> +static const struct dmi_system_id inband_presence_disabled_dmi_table[] = {
>> +	/*
>> +	 * Match all Dell systems, as some Dell systems have inband
>> +	 * presence disabled on NVMe slots (but don't support the bit to
> 
> Is there something that restricts these slots to being used only for
> NVMe?  If not, I'd rather simply say that some Root Ports don't
> support in-band presence detect.  You say it's "disabled", which
> suggests that it could be *enabled*.  But I have the impression that
> it's actually just not supported at all (or maybe it's disabled by the
> BIOS via some non-architected mechanism).
> 

These slots are in the backplane that is meant for hard drives, so the
I don't know that there would be any other devices that would fit in them.
But, if some other device did fit physically and had the right connector,
I believe it would work.

I'm not sure if in-band presence detection is disabled by firmware, or
hardwired that way in the circuitry somehow, but you are correct, in-band
presence detect can't be enabled on these systems.

>> +	 * report it). Setting inband presence disabled should have no
>> +	 * negative effect, except on broken hotplug slots that never
>> +	 * assert presence detect--and those will still work, they will
>> +	 * just have a bit of extra delay before being probed.
>> +	 */
>> +	{
>> +		.ident = "Dell System",
>> +		.matches = {
>> +			DMI_MATCH(DMI_OEM_STRING, "Dell System"),
>> +		},
>> +	},
>> +	{}
>> +};
>> +
>>  static inline struct pci_dev *ctrl_dev(struct controller *ctrl)
>>  {
>>  	return ctrl->pcie->port;
>> @@ -895,6 +914,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;
> 
> This doesn't seem quite right: the DMI table should only apply to
> built-in ports, not to ports on plugin cards.
> 
> If we plug in a switch with hotplug-capable downstream ports, and
> those ports do not advertise PCI_EXP_SLTCAP2_IBPD, I think this code
> sets "inband_presence_disabled" for those ports even though it is not
> disabled.
> 
> IIUC, that will make this plugin card behave differently in a Dell
> system than it will in other systems, and that doesn't seem right to
> me.
> 

Setting "inband_presence_disabled" will only make a hotplug-capable slot
behave differently in two situations:

(1) The slot is using out-of-band presence detection instead of in-band,
in which case the hotplug code will correctly wait for the presence detect
bit to be active (after the link goes active) before proceeding to add the
device.

(2) The slot is broken and the presence detect bit never goes high in spite
of the PCI spec saying "This bit must be implemented on all Downstream Ports
that implement slots" (pcie base spec 5.0r1.0, section 7.5.3.11, slot status
register).  In this case, the hotplug slot will still work when
"inband_presence_disabled" is set, but it will take it a second or so longer
for the device to be added in linux because it will timeout waiting for the
presence detect bit to go active, instead of adding the device as soon as
the link goes active.

Since Dell doesn't support any plugin cards that provide hot plug slots that
are broken in that way, setting "inband_presence_disabled" shouldn't make any
difference.  But even if someone did use a card that was broken in this way,
it would still function.

If you think this is important, I could try to find a way to set
"inband_presence_disabled" just for built-in ports.  I'm not sure it would
be of any practical benefit, though.


>>  	/*
>>  	 * 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
>>

Re: [PATCH v4 0/3] PCI: pciehp: Do not turn off slot if presence comes up after link

[Mika Westerberg]

On Fri, Oct 25, 2019 at 03:00:44PM -0400, Stuart Hayes wrote:
> Alexandru Gagniuc (2):
>   PCI: pciehp: Add support for disabling in-band presence
>   PCI: pciehp: Wait for PDS if in-band presence is disabled
> 
> Stuart Hayes (1):
>   PCI: pciehp: Add dmi table for in-band presence disabled

For the whole series,

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v4 0/3] PCI: pciehp: Do not turn off slot if presence comes up after link

[Lukas Wunner]

On Fri, Oct 25, 2019 at 03:00:44PM -0400, Stuart Hayes wrote:
> 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.

FWIW, this series is

Reviewed-by: Lukas Wunner <lukas@wunner.de>

Looking at the patches again today, I only spotted a minor cosmetic issue:

In patch [1/3] I would have preferred readout of the PCI_EXP_SLTCAP2
register (hunk #3) to be inserted a little further up in pcie_init(),
perhaps before reading the PCI_EXP_LNKCAP register.  It just looks
a little out of place at the end of the function.  I would have
grouped it together with the other quirks and feature checks further
up in the function and I probably would have amended the ctrl_info()
to print the status of the inband_presence_disabled flag.

In patch [3/3] the DMI check would then likewise have to be moved up
in the function.

Maybe Bjorn can make this change when applying, and if not, it's not
a big deal.

Thanks,

Lukas

Re: [PATCH v4 0/3] PCI: pciehp: Do not turn off slot if presence comes up after link

[Bjorn Helgaas]

[+cc Libor (thanks for the ping!)]

On Fri, Oct 25, 2019 at 03:00:44PM -0400, Stuart Hayes wrote:
> 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.
> 
> The first two patches in this set come from a patch set that was
> submitted but not accepted many months ago by Alexandru Gagniuc [1].
> The first is unmodified, the second has the commit message and timeout 
> modified.
> 
> [1] https://patchwork.kernel.org/cover/10909167/
>     [v3,0/4] PCI: pciehp: Do not turn off slot if presence comes up after link
> 
> v2:
> - modify loop in pcie_wait_for_presence to do..while
> 
> v3:
> - remove unused variable declaration
> - modify text of warning message
> 
> v4:
> - remove "!!" boolean conversion in an "if" condition for readability
> - add explanation comment in dmi table
> 
> Alexandru Gagniuc (2):
>   PCI: pciehp: Add support for disabling in-band presence
>   PCI: pciehp: Wait for PDS if in-band presence is disabled
> 
> Stuart Hayes (1):
>   PCI: pciehp: Add dmi table for in-band presence disabled
> 
>  drivers/pci/hotplug/pciehp.h     |  1 +
>  drivers/pci/hotplug/pciehp_hpc.c | 50 +++++++++++++++++++++++++++++++-
>  include/uapi/linux/pci_regs.h    |  2 ++
>  3 files changed, 52 insertions(+), 1 deletion(-)

I added the spec reference to the 1/3 commit log, tried to make the
tweaks Lukas suggested (interdiff below), used ctrl_info() instead of
pci_info() (I would actually like to change the whole driver to use
pci_info(), but better to be consistent for now), and applied to
pci/hotplug for v5.7.

Somebody should also update lspci to:

  - Do something with DevCap AttnBtn, AttnInd, PwrInd to indicate that
    they were only defined for PCIe r1.0 and have been explicitly
    undefined since then.  If there's a way to identify those 1.0
    devices and only decode those fields for 1.0, that would be nice.

  - Add SltCap2 and SltCtrl2 decoding.

Speak up if you plan to do this so we don't duplicate effort.

Bjorn

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index ae0108b92084..469873b44a8e 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -284,7 +284,7 @@ static void pcie_wait_for_presence(struct pci_dev *pdev)
 		timeout -= 10;
 	} while (timeout > 0);
 
-	pci_info(pdev, "Timeout waiting for Presence Detect state to be set\n");
+	ctrl_info(ctrl, "Timeout waiting for Presence Detect\n");
 }
 
 int pciehp_check_link_status(struct controller *ctrl)
@@ -921,6 +921,16 @@ struct controller *pcie_init(struct pcie_device *dev)
 	ctrl->state = list_empty(&subordinate->devices) ? OFF_STATE : ON_STATE;
 	up_read(&pci_bus_sem);
 
+	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 (dmi_first_match(inband_presence_disabled_dmi_table))
+		ctrl->inband_presence_disabled = 1;
+
 	/* Check if Data Link Layer Link Active Reporting is implemented */
 	pcie_capability_read_dword(pdev, PCI_EXP_LNKCAP, &link_cap);
 
@@ -930,7 +940,7 @@ struct controller *pcie_init(struct pcie_device *dev)
 		PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_CC |
 		PCI_EXP_SLTSTA_DLLSC | PCI_EXP_SLTSTA_PDC);
 
-	ctrl_info(ctrl, "Slot #%d AttnBtn%c PwrCtrl%c MRL%c AttnInd%c PwrInd%c HotPlug%c Surprise%c Interlock%c NoCompl%c LLActRep%c%s\n",
+	ctrl_info(ctrl, "Slot #%d AttnBtn%c PwrCtrl%c MRL%c AttnInd%c PwrInd%c HotPlug%c Surprise%c Interlock%c NoCompl%c IbPresDis%c LLActRep%c%s\n",
 		(slot_cap & PCI_EXP_SLTCAP_PSN) >> 19,
 		FLAG(slot_cap, PCI_EXP_SLTCAP_ABP),
 		FLAG(slot_cap, PCI_EXP_SLTCAP_PCP),
@@ -941,19 +951,10 @@ struct controller *pcie_init(struct pcie_device *dev)
 		FLAG(slot_cap, PCI_EXP_SLTCAP_HPS),
 		FLAG(slot_cap, PCI_EXP_SLTCAP_EIP),
 		FLAG(slot_cap, PCI_EXP_SLTCAP_NCCS),
+		ctrl->inband_presence_disabled,
 		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 (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.
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index b464d2f76513..f9701410d3b5 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -681,7 +681,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_SLTCAP2_IBPD	0x00000001 /* In-band PD Disable Supported */
 #define PCI_EXP_SLTCTL2		56	/* Slot Control 2 */
 #define PCI_EXP_SLTSTA2		58	/* Slot Status 2 */
 

Re: [PATCH v4 0/3] PCI: pciehp: Do not turn off slot if presence comes up after link

[Lukas Wunner]

On Mon, Feb 10, 2020 at 06:08:16PM -0600, Bjorn Helgaas wrote:
> used ctrl_info() instead of pci_info() (I would actually like to change
> the whole driver to use pci_info(), but better to be consistent for now)

Most of the ctrl_info() calls prepend "Slot(%s): " to the message.
However that prefix can only be used once pci_hp_initialize() has
been called.

It would probably make sense to change ctrl_info() to always
include the prefix and change those invocations of ctrl_info()
which happen when the slot is not yet or no longer registered,
to pci_info().


> @@ -930,7 +940,7 @@ struct controller *pcie_init(struct pcie_device *dev)
>  		PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_CC |
>  		PCI_EXP_SLTSTA_DLLSC | PCI_EXP_SLTSTA_PDC);
>  
> -	ctrl_info(ctrl, "Slot #%d AttnBtn%c PwrCtrl%c MRL%c AttnInd%c PwrInd%c HotPlug%c Surprise%c Interlock%c NoCompl%c LLActRep%c%s\n",
> +	ctrl_info(ctrl, "Slot #%d AttnBtn%c PwrCtrl%c MRL%c AttnInd%c PwrInd%c HotPlug%c Surprise%c Interlock%c NoCompl%c IbPresDis%c LLActRep%c%s\n",
>  		(slot_cap & PCI_EXP_SLTCAP_PSN) >> 19,
>  		FLAG(slot_cap, PCI_EXP_SLTCAP_ABP),
>  		FLAG(slot_cap, PCI_EXP_SLTCAP_PCP),
> @@ -941,19 +951,10 @@ struct controller *pcie_init(struct pcie_device *dev)
>  		FLAG(slot_cap, PCI_EXP_SLTCAP_HPS),
>  		FLAG(slot_cap, PCI_EXP_SLTCAP_EIP),
>  		FLAG(slot_cap, PCI_EXP_SLTCAP_NCCS),
> +		ctrl->inband_presence_disabled,
>  		FLAG(link_cap, PCI_EXP_LNKCAP_DLLLARC),
>  		pdev->broken_cmd_compl ? " (with Cmd Compl erratum)" : "");

I've just reviewed the resulting commits on pci/hotplug once more and
think there's a small issue here:  If ctrl->inband_presence_disabled is 0,
the string will contain ASCII character 0 (end of string) and if it's 1
it will contain ASCII character 1 (start of header).  A possible solution
would be FLAG(ctrl->inband_presence_disabled, 1).

(The real solution would probably to have a printk format for this kind
of thing.)

Thanks,

Lukas

Re: [PATCH v4 0/3] PCI: pciehp: Do not turn off slot if presence comes up after link

[Bjorn Helgaas]

On Tue, Feb 11, 2020 at 05:49:40AM +0100, Lukas Wunner wrote:
> On Mon, Feb 10, 2020 at 06:08:16PM -0600, Bjorn Helgaas wrote:
> > used ctrl_info() instead of pci_info() (I would actually like to change
> > the whole driver to use pci_info(), but better to be consistent for now)
> 
> Most of the ctrl_info() calls prepend "Slot(%s): " to the message.
> However that prefix can only be used once pci_hp_initialize() has
> been called.
> 
> It would probably make sense to change ctrl_info() to always
> include the prefix and change those invocations of ctrl_info()
> which happen when the slot is not yet or no longer registered,
> to pci_info().

Ouch, my tweaks were definitely half-baked.

I really like your idea of hoisting the "Slot(%s)" text up into
ctrl_*().  I might rename ctrl_*() to slot_*() or similar to connect
it more with the slot registration.

I'm a little confused about why pci_hp_initialize()/
__pci_hp_initialize()/pci_hp_register()/__pci_hp_register() is such a
rat's nest with hotplug drivers using a mix of them.  I wonder if
that could be rationalized and maybe done earlier so all hotplug-
related messages could use the same ctrl_*() logging.

But this is all outside the scope of this patch.  I'll look at the
pcie_wait_for_presence() situation and revert to pci_info() if
if can be called when the slot is not registered.

> > @@ -930,7 +940,7 @@ struct controller *pcie_init(struct pcie_device *dev)
> >  		PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_CC |
> >  		PCI_EXP_SLTSTA_DLLSC | PCI_EXP_SLTSTA_PDC);
> >  
> > -	ctrl_info(ctrl, "Slot #%d AttnBtn%c PwrCtrl%c MRL%c AttnInd%c PwrInd%c HotPlug%c Surprise%c Interlock%c NoCompl%c LLActRep%c%s\n",
> > +	ctrl_info(ctrl, "Slot #%d AttnBtn%c PwrCtrl%c MRL%c AttnInd%c PwrInd%c HotPlug%c Surprise%c Interlock%c NoCompl%c IbPresDis%c LLActRep%c%s\n",
> >  		(slot_cap & PCI_EXP_SLTCAP_PSN) >> 19,
> >  		FLAG(slot_cap, PCI_EXP_SLTCAP_ABP),
> >  		FLAG(slot_cap, PCI_EXP_SLTCAP_PCP),
> > @@ -941,19 +951,10 @@ struct controller *pcie_init(struct pcie_device *dev)
> >  		FLAG(slot_cap, PCI_EXP_SLTCAP_HPS),
> >  		FLAG(slot_cap, PCI_EXP_SLTCAP_EIP),
> >  		FLAG(slot_cap, PCI_EXP_SLTCAP_NCCS),
> > +		ctrl->inband_presence_disabled,
> >  		FLAG(link_cap, PCI_EXP_LNKCAP_DLLLARC),
> >  		pdev->broken_cmd_compl ? " (with Cmd Compl erratum)" : "");
> 
> I've just reviewed the resulting commits on pci/hotplug once more and
> think there's a small issue here:  If ctrl->inband_presence_disabled is 0,
> the string will contain ASCII character 0 (end of string) and if it's 1
> it will contain ASCII character 1 (start of header).  A possible solution
> would be FLAG(ctrl->inband_presence_disabled, 1).

Definitely broken, sorry about that.  Feels like sort of a
double-negative situation, too.  Obviously the hardware bit has to be
"1 means disabled" to be compatible with previous spec versions, but
the code is usually easier to read if we test for something being
*enabled*.  I'll try to figure out something.

Bjorn

Re: [PATCH v4 0/3] PCI: pciehp: Do not turn off slot if presence comes up after link

[Lukas Wunner]

On Tue, Feb 11, 2020 at 08:14:44AM -0600, Bjorn Helgaas wrote:
> I'm a little confused about why pci_hp_initialize()/
> __pci_hp_initialize()/pci_hp_register()/__pci_hp_register() is such a
> rat's nest with hotplug drivers using a mix of them.

This is modeled after device registration, which can be done either
in two steps (device_initialize() + device_add()) or in 1 step
(device_register()).

So it's either pci_hp_initialize() + pci_hp_add() or pci_hp_register().

The rationale is provided in the commit message of 51bbf9bee34f
("PCI: hotplug: Demidlayer registration with the core").


> Feels like sort of a
> double-negative situation, too.  Obviously the hardware bit has to be
> "1 means disabled" to be compatible with previous spec versions, but
> the code is usually easier to read if we test for something being
> *enabled*.

It's a similar situation with the "DisINTx" bit in the Command
register, which, if disabled, is shown as "DisINTx-" in lspci even
though the more intuitive notion is that INTx is *enabled*.  I think
you did the right thing by showing it as "IbPresDis-" because it's
consistent with how it's done elsewhere for similar bits.

Thanks,

Lukas

Re: [PATCH v4 0/3] PCI: pciehp: Do not turn off slot if presence comes up after link

[Bjorn Helgaas]

On Tue, Feb 11, 2020 at 03:32:02PM +0100, Lukas Wunner wrote:
> On Tue, Feb 11, 2020 at 08:14:44AM -0600, Bjorn Helgaas wrote:
> > I'm a little confused about why pci_hp_initialize()/
> > __pci_hp_initialize()/pci_hp_register()/__pci_hp_register() is such a
> > rat's nest with hotplug drivers using a mix of them.
> 
> This is modeled after device registration, which can be done either
> in two steps (device_initialize() + device_add()) or in 1 step
> (device_register()).
> 
> So it's either pci_hp_initialize() + pci_hp_add() or pci_hp_register().
> 
> The rationale is provided in the commit message of 51bbf9bee34f
> ("PCI: hotplug: Demidlayer registration with the core").

Thanks for the pointer.  I wrote that down in case I ever try to
figure that out in the future.  Obviously I haven't looked at this in
any detail, but it seems like the sort of thing that all the hotplug
drivers should do the same way regardless of their internal structure,
and the slot concept seems pretty integral to the bridge leading to
it.  Maybe this is a somehow a consequence of the hotplug drivers
being separated from the enumeration path.  Or maybe the slot part
could be split out from the hotplug drivers and done during
enumeration.  Just blue sky thinking, I don't pretend to have done
any actual research here.

Bjorn

Re: [PATCH v4 0/3] PCI: pciehp: Do not turn off slot if presence comes up after link

[Bjorn Helgaas]

On Tue, Feb 11, 2020 at 03:32:02PM +0100, Lukas Wunner wrote:
> On Tue, Feb 11, 2020 at 08:14:44AM -0600, Bjorn Helgaas wrote:
> > Feels like sort of a
> > double-negative situation, too.  Obviously the hardware bit has to be
> > "1 means disabled" to be compatible with previous spec versions, but
> > the code is usually easier to read if we test for something being
> > *enabled*.
> 
> It's a similar situation with the "DisINTx" bit in the Command
> register, which, if disabled, is shown as "DisINTx-" in lspci even
> though the more intuitive notion is that INTx is *enabled*.  I think
> you did the right thing by showing it as "IbPresDis-" because it's
> consistent with how it's done elsewhere for similar bits.

Everything else we decode is *capability* bits and IBPD is another
one.  So by the principle of least surprise, I propose this:

+       ctrl_info(ctrl, "Slot #%d AttnBtn%c PwrCtrl%c MRL%c AttnInd%c PwrInd%c HotPlug%c Surprise%c Interlock%c NoCompl%c IbPresDis%c LLActRep%c%s\n",
+               FLAG(slot_cap2, PCI_EXP_SLTCAP2_IBPD),

That works out to be the same as printing

  inbound_presence_disabled ? '+' : '-'

because we always set inbound_presence_disabled when
PCI_EXP_SLTCAP2_IBPD is supported.