[v6,03/12] PCI: Request control of native PCIe hotplug only if supported

Message ID 20180510182844.77349-4-mika.westerberg@linux.intel.com
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series
  • PCI: Fixes and cleanups for native PCIe, SHPC and ACPI hotplug
Related show

Commit Message

Mika Westerberg May 10, 2018, 6:28 p.m.
Currently we request control of native PCIe hotplug unconditionally.
That may cause problems because native PCIe hotplug events are handled
by pciehp driver and if it is not enabled those events will be lost.
Make this bit more robust and request control of native PCIe hotplug
only if we are actually prepared to do so (pciehp driver is enabled).

While there rename host->native_hotplug to host->native_pcie_hotplug
because we do the same for SHPC hotplug in subsequent patches.

Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/acpi/pci_root.c         | 6 ++++--
 drivers/pci/pcie/portdrv_core.c | 2 +-
 drivers/pci/probe.c             | 2 +-
 include/linux/pci.h             | 2 +-
 include/linux/pci_hotplug.h     | 7 +++++++
 5 files changed, 14 insertions(+), 5 deletions(-)

Comments

Lukas Wunner May 10, 2018, 7:44 p.m. | #1
On Thu, May 10, 2018 at 09:28:35PM +0300, Mika Westerberg wrote:
> +
> +#ifdef CONFIG_HOTPLUG_PCI_PCIE
> +#define pciehp_available()	true
> +#else
> +#define pciehp_available()	false
> +#endif
> +

More succinctly,

#define pciehp_available() IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE)

(Or use that directly instead of defining an additional macro.)

Thanks,

Lukas
Rafael J. Wysocki May 15, 2018, 9:10 a.m. | #2
On Thursday, May 10, 2018 9:44:33 PM CEST Lukas Wunner wrote:
> On Thu, May 10, 2018 at 09:28:35PM +0300, Mika Westerberg wrote:
> > +
> > +#ifdef CONFIG_HOTPLUG_PCI_PCIE
> > +#define pciehp_available()	true
> > +#else
> > +#define pciehp_available()	false
> > +#endif
> > +
> 
> More succinctly,
> 
> #define pciehp_available() IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE)
> 
> (Or use that directly instead of defining an additional macro.)

Right and I would do the latter.

Thanks,
Rafael
Rafael J. Wysocki May 15, 2018, 9:12 a.m. | #3
On Thursday, May 10, 2018 8:28:35 PM CEST Mika Westerberg wrote:
> Currently we request control of native PCIe hotplug unconditionally.
> That may cause problems because native PCIe hotplug events are handled
> by pciehp driver and if it is not enabled those events will be lost.
> Make this bit more robust and request control of native PCIe hotplug
> only if we are actually prepared to do so (pciehp driver is enabled).
> 
> While there rename host->native_hotplug to host->native_pcie_hotplug
> because we do the same for SHPC hotplug in subsequent patches.
> 
> Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/acpi/pci_root.c         | 6 ++++--
>  drivers/pci/pcie/portdrv_core.c | 2 +-
>  drivers/pci/probe.c             | 2 +-
>  include/linux/pci.h             | 2 +-
>  include/linux/pci_hotplug.h     | 7 +++++++
>  5 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 0da18bde6a16..97590dff6bd8 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -29,6 +29,7 @@
>  #include <linux/pci.h>
>  #include <linux/pci-acpi.h>
>  #include <linux/pci-aspm.h>
> +#include <linux/pci_hotplug.h>
>  #include <linux/dmar.h>
>  #include <linux/acpi.h>
>  #include <linux/slab.h>
> @@ -472,9 +473,10 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
>  	}
>  
>  	control = OSC_PCI_EXPRESS_CAPABILITY_CONTROL
> -		| OSC_PCI_EXPRESS_NATIVE_HP_CONTROL
>  		| OSC_PCI_EXPRESS_PME_CONTROL;
>  
> +	if (pciehp_available())
> +		control |= OSC_PCI_EXPRESS_NATIVE_HP_CONTROL;
>  	if (pci_aer_available()) {
>  		if (aer_acpi_firmware_first())
>  			dev_info(&device->dev,
> @@ -900,7 +902,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>  
>  	host_bridge = to_pci_host_bridge(bus->bridge);
>  	if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
> -		host_bridge->native_hotplug = 0;
> +		host_bridge->native_pcie_hotplug = 0;
>  	if (!(root->osc_control_set & OSC_PCI_EXPRESS_AER_CONTROL))
>  		host_bridge->native_aer = 0;
>  	if (!(root->osc_control_set & OSC_PCI_EXPRESS_PME_CONTROL))
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index c9c0663db282..6cb30aec2452 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -199,7 +199,7 @@ static int get_port_device_capability(struct pci_dev *dev)
>  	int services = 0;
>  
>  	if (dev->is_hotplug_bridge &&
> -	    (pcie_ports_native || host->native_hotplug)) {
> +	    (pcie_ports_native || host->native_pcie_hotplug)) {
>  		services |= PCIE_PORT_SERVICE_HP;
>  
>  		/*
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 7c34a2ccb514..a6c3b8d30f8f 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -552,7 +552,7 @@ struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
>  	 * OS from interfering.
>  	 */
>  	bridge->native_aer = 1;
> -	bridge->native_hotplug = 1;
> +	bridge->native_pcie_hotplug = 1;
>  	bridge->native_pme = 1;
>  
>  	return bridge;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 73178a2fcee0..359a197d0310 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -471,7 +471,7 @@ struct pci_host_bridge {
>  	unsigned int	ignore_reset_delay:1;	/* For entire hierarchy */
>  	unsigned int	no_ext_tags:1;		/* No Extended Tags */
>  	unsigned int	native_aer:1;		/* OS may use PCIe AER */
> -	unsigned int	native_hotplug:1;	/* OS may use PCIe hotplug */
> +	unsigned int	native_pcie_hotplug:1;	/* OS may use PCIe hotplug */
>  	unsigned int	native_pme:1;		/* OS may use PCIe PME */
>  	/* Resource alignment requirements */
>  	resource_size_t (*align_resource)(struct pci_dev *dev,
> diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
> index 26213024e81b..46fb90b5164b 100644
> --- a/include/linux/pci_hotplug.h
> +++ b/include/linux/pci_hotplug.h
> @@ -174,4 +174,11 @@ static inline int pci_get_hp_params(struct pci_dev *dev,
>  }
>  static inline bool pciehp_is_native(struct pci_dev *pdev) { return true; }
>  #endif
> +
> +#ifdef CONFIG_HOTPLUG_PCI_PCIE
> +#define pciehp_available()	true
> +#else
> +#define pciehp_available()	false
> +#endif
> +
>  #endif
> 

Modulo the Lucas' comment on using IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE)
directly instead of the above:

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Patch

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 0da18bde6a16..97590dff6bd8 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -29,6 +29,7 @@ 
 #include <linux/pci.h>
 #include <linux/pci-acpi.h>
 #include <linux/pci-aspm.h>
+#include <linux/pci_hotplug.h>
 #include <linux/dmar.h>
 #include <linux/acpi.h>
 #include <linux/slab.h>
@@ -472,9 +473,10 @@  static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
 	}
 
 	control = OSC_PCI_EXPRESS_CAPABILITY_CONTROL
-		| OSC_PCI_EXPRESS_NATIVE_HP_CONTROL
 		| OSC_PCI_EXPRESS_PME_CONTROL;
 
+	if (pciehp_available())
+		control |= OSC_PCI_EXPRESS_NATIVE_HP_CONTROL;
 	if (pci_aer_available()) {
 		if (aer_acpi_firmware_first())
 			dev_info(&device->dev,
@@ -900,7 +902,7 @@  struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 
 	host_bridge = to_pci_host_bridge(bus->bridge);
 	if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
-		host_bridge->native_hotplug = 0;
+		host_bridge->native_pcie_hotplug = 0;
 	if (!(root->osc_control_set & OSC_PCI_EXPRESS_AER_CONTROL))
 		host_bridge->native_aer = 0;
 	if (!(root->osc_control_set & OSC_PCI_EXPRESS_PME_CONTROL))
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index c9c0663db282..6cb30aec2452 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -199,7 +199,7 @@  static int get_port_device_capability(struct pci_dev *dev)
 	int services = 0;
 
 	if (dev->is_hotplug_bridge &&
-	    (pcie_ports_native || host->native_hotplug)) {
+	    (pcie_ports_native || host->native_pcie_hotplug)) {
 		services |= PCIE_PORT_SERVICE_HP;
 
 		/*
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 7c34a2ccb514..a6c3b8d30f8f 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -552,7 +552,7 @@  struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
 	 * OS from interfering.
 	 */
 	bridge->native_aer = 1;
-	bridge->native_hotplug = 1;
+	bridge->native_pcie_hotplug = 1;
 	bridge->native_pme = 1;
 
 	return bridge;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 73178a2fcee0..359a197d0310 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -471,7 +471,7 @@  struct pci_host_bridge {
 	unsigned int	ignore_reset_delay:1;	/* For entire hierarchy */
 	unsigned int	no_ext_tags:1;		/* No Extended Tags */
 	unsigned int	native_aer:1;		/* OS may use PCIe AER */
-	unsigned int	native_hotplug:1;	/* OS may use PCIe hotplug */
+	unsigned int	native_pcie_hotplug:1;	/* OS may use PCIe hotplug */
 	unsigned int	native_pme:1;		/* OS may use PCIe PME */
 	/* Resource alignment requirements */
 	resource_size_t (*align_resource)(struct pci_dev *dev,
diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
index 26213024e81b..46fb90b5164b 100644
--- a/include/linux/pci_hotplug.h
+++ b/include/linux/pci_hotplug.h
@@ -174,4 +174,11 @@  static inline int pci_get_hp_params(struct pci_dev *dev,
 }
 static inline bool pciehp_is_native(struct pci_dev *pdev) { return true; }
 #endif
+
+#ifdef CONFIG_HOTPLUG_PCI_PCIE
+#define pciehp_available()	true
+#else
+#define pciehp_available()	false
+#endif
+
 #endif