diff mbox series

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

Message ID 20191025190047.38130-2-stuart.w.hayes@gmail.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show
Series PCI: pciehp: Do not turn off slot if presence comes up after link | expand

Commit Message

stuart hayes Oct. 25, 2019, 7 p.m. UTC 
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(-)

Comments

Andy Shevchenko Oct. 25, 2019, 7:15 p.m. UTC | #1 
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
>
Bjorn Helgaas Nov. 27, 2019, 1:36 a.m. UTC | #2 
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
>
stuart hayes Nov. 27, 2019, 2:19 a.m. UTC | #3 
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
> >
stuart hayes Dec. 31, 2019, 10:06 p.m. UTC | #4 
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.
Libor Pechacek Jan. 29, 2020, 1:15 p.m. UTC | #5 
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
diff mbox series

Patch

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