[v6,04/12] PCI: Make pciehp_is_native() stricter

Message ID 20180510182844.77349-5-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 pciehp_is_native() returns true for any PCI device in a
hierarchy where _OSC says we can use pciehp. This is not correct because
there may be bridges without PCI_EXP_SLTCAP_HPC capability and those
should be managed by acpiphp instead.

Improve pciehp_is_native() to return true only if the given bridge is
actually expected to be handled by pciehp. In any other case return
false instead to let acpiphp handle those.

Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/pci-acpi.c      | 28 ++++++++++++++++------------
 drivers/pci/pcie/portdrv.h  |  2 --
 include/linux/pci.h         |  2 ++
 include/linux/pci_hotplug.h |  4 ++--
 4 files changed, 20 insertions(+), 16 deletions(-)

Comments

Lukas Wunner May 10, 2018, 7:51 p.m. | #1
On Thu, May 10, 2018 at 09:28:36PM +0300, Mika Westerberg wrote:
> +	pcie_capability_read_dword(bridge, PCI_EXP_SLTCAP, &slot_cap);
> +	if (!(slot_cap & PCI_EXP_SLTCAP_HPC))
>  		return false;

Hm, any reason not to use "if (!bridge->is_hotplug_bridge)" ?

Thanks,

Lukas
Mika Westerberg May 14, 2018, 5:32 a.m. | #2
On Thu, May 10, 2018 at 09:51:49PM +0200, Lukas Wunner wrote:
> On Thu, May 10, 2018 at 09:28:36PM +0300, Mika Westerberg wrote:
> > +	pcie_capability_read_dword(bridge, PCI_EXP_SLTCAP, &slot_cap);
> > +	if (!(slot_cap & PCI_EXP_SLTCAP_HPC))
> >  		return false;
> 
> Hm, any reason not to use "if (!bridge->is_hotplug_bridge)" ?

That can be set for non-native PCIe bridges. For example acpiphp sets it
in some cases. Here we read the capability directly to make sure we are
really dealing with native PCIe hotplug.
Rafael J. Wysocki May 15, 2018, 9:17 a.m. | #3
On Thursday, May 10, 2018 8:28:36 PM CEST Mika Westerberg wrote:
> Currently pciehp_is_native() returns true for any PCI device in a
> hierarchy where _OSC says we can use pciehp. This is not correct because
> there may be bridges without PCI_EXP_SLTCAP_HPC capability and those
> should be managed by acpiphp instead.
> 
> Improve pciehp_is_native() to return true only if the given bridge is
> actually expected to be handled by pciehp. In any other case return
> false instead to let acpiphp handle those.
> 
> Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/pci/pci-acpi.c      | 28 ++++++++++++++++------------
>  drivers/pci/pcie/portdrv.h  |  2 --
>  include/linux/pci.h         |  2 ++
>  include/linux/pci_hotplug.h |  4 ++--
>  4 files changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 1abdbf267c19..d3114f3a7ab8 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -370,26 +370,30 @@ EXPORT_SYMBOL_GPL(pci_get_hp_params);
>  
>  /**
>   * pciehp_is_native - Check whether a hotplug port is handled by the OS
> - * @pdev: Hotplug port to check
> + * @bridge: Hotplug port to check
>   *
> - * Walk up from @pdev to the host bridge, obtain its cached _OSC Control Field
> - * and return the value of the "PCI Express Native Hot Plug control" bit.
> - * On failure to obtain the _OSC Control Field return %false.
> + * Returns true if the given @bridge is handled by the native PCIe hotplug
> + * driver.
>   */
> -bool pciehp_is_native(struct pci_dev *pdev)
> +bool pciehp_is_native(struct pci_dev *bridge)
>  {
> -	struct acpi_pci_root *root;
> -	acpi_handle handle;
> +	const struct pci_host_bridge *host;
> +	u32 slot_cap;
>  
> -	handle = acpi_find_root_bridge_handle(pdev);
> -	if (!handle)
> +	if (!pciehp_available())
> +		return false;
> +	if (!bridge)
>  		return false;

You could write this as 

    if (!pciehp_available() || !bridge)
        return false;

Would be equivalent, but more concise IMO.

But anyway

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Bjorn Helgaas May 24, 2018, 6:08 p.m. | #4
On Tue, May 15, 2018 at 11:17:14AM +0200, Rafael J. Wysocki wrote:
> On Thursday, May 10, 2018 8:28:36 PM CEST Mika Westerberg wrote:
> > Currently pciehp_is_native() returns true for any PCI device in a
> > hierarchy where _OSC says we can use pciehp. This is not correct because
> > there may be bridges without PCI_EXP_SLTCAP_HPC capability and those
> > should be managed by acpiphp instead.
> > 
> > Improve pciehp_is_native() to return true only if the given bridge is
> > actually expected to be handled by pciehp. In any other case return
> > false instead to let acpiphp handle those.
> > 
> > Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  drivers/pci/pci-acpi.c      | 28 ++++++++++++++++------------
> >  drivers/pci/pcie/portdrv.h  |  2 --
> >  include/linux/pci.h         |  2 ++
> >  include/linux/pci_hotplug.h |  4 ++--
> >  4 files changed, 20 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > index 1abdbf267c19..d3114f3a7ab8 100644
> > --- a/drivers/pci/pci-acpi.c
> > +++ b/drivers/pci/pci-acpi.c
> > @@ -370,26 +370,30 @@ EXPORT_SYMBOL_GPL(pci_get_hp_params);
> >  
> >  /**
> >   * pciehp_is_native - Check whether a hotplug port is handled by the OS
> > - * @pdev: Hotplug port to check
> > + * @bridge: Hotplug port to check
> >   *
> > - * Walk up from @pdev to the host bridge, obtain its cached _OSC Control Field
> > - * and return the value of the "PCI Express Native Hot Plug control" bit.
> > - * On failure to obtain the _OSC Control Field return %false.
> > + * Returns true if the given @bridge is handled by the native PCIe hotplug
> > + * driver.
> >   */
> > -bool pciehp_is_native(struct pci_dev *pdev)
> > +bool pciehp_is_native(struct pci_dev *bridge)
> >  {
> > -	struct acpi_pci_root *root;
> > -	acpi_handle handle;
> > +	const struct pci_host_bridge *host;
> > +	u32 slot_cap;
> >  
> > -	handle = acpi_find_root_bridge_handle(pdev);
> > -	if (!handle)
> > +	if (!pciehp_available())
> > +		return false;
> > +	if (!bridge)
> >  		return false;
> 
> You could write this as 
> 
>     if (!pciehp_available() || !bridge)
>         return false;
> 
> Would be equivalent, but more concise IMO.

Why do we even need to bother checking "bridge" for NULL?

I don't believe in gratuitously checking for NULL because it can hide
higher-level bugs, i.e., if this is called with a NULL pointer, that's
likely a bug in the caller, and if we silently return "false", we'll
never discover the caller's bug.
Mika Westerberg May 24, 2018, 6:31 p.m. | #5
On Thu, May 24, 2018 at 01:08:37PM -0500, Bjorn Helgaas wrote:
> Why do we even need to bother checking "bridge" for NULL?
> 
> I don't believe in gratuitously checking for NULL because it can hide
> higher-level bugs, i.e., if this is called with a NULL pointer, that's
> likely a bug in the caller, and if we silently return "false", we'll
> never discover the caller's bug.

But it also makes caller's life easier because now you don't need to do
this:

   if (bus->self && hotplug_is_native(bus->self))

because it is perfectly legal to call that function for host bridge. So
you do this instead

   if (hotplug_is_native(bus->self))

and it is not hiding a bug IMHO.
Bjorn Helgaas May 24, 2018, 7:35 p.m. | #6
On Thu, May 24, 2018 at 09:31:07PM +0300, Mika Westerberg wrote:
> On Thu, May 24, 2018 at 01:08:37PM -0500, Bjorn Helgaas wrote:
> > Why do we even need to bother checking "bridge" for NULL?
> > 
> > I don't believe in gratuitously checking for NULL because it can hide
> > higher-level bugs, i.e., if this is called with a NULL pointer, that's
> > likely a bug in the caller, and if we silently return "false", we'll
> > never discover the caller's bug.
> 
> But it also makes caller's life easier because now you don't need to do
> this:
> 
>    if (bus->self && hotplug_is_native(bus->self))
> 
> because it is perfectly legal to call that function for host bridge. So
> you do this instead
> 
>    if (hotplug_is_native(bus->self))
> 
> and it is not hiding a bug IMHO.

True, it's not hiding a bug, but I think it is useful for readers of
acpiphp_add_context() to see the NULL check there and realize that
bus->self (or "pdev" in current upstream) is allowed to be NULL.  That
helps understand the way acpiphp works (well, I understand very little
of it, but every little clue helps).

Patch

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 1abdbf267c19..d3114f3a7ab8 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -370,26 +370,30 @@  EXPORT_SYMBOL_GPL(pci_get_hp_params);
 
 /**
  * pciehp_is_native - Check whether a hotplug port is handled by the OS
- * @pdev: Hotplug port to check
+ * @bridge: Hotplug port to check
  *
- * Walk up from @pdev to the host bridge, obtain its cached _OSC Control Field
- * and return the value of the "PCI Express Native Hot Plug control" bit.
- * On failure to obtain the _OSC Control Field return %false.
+ * Returns true if the given @bridge is handled by the native PCIe hotplug
+ * driver.
  */
-bool pciehp_is_native(struct pci_dev *pdev)
+bool pciehp_is_native(struct pci_dev *bridge)
 {
-	struct acpi_pci_root *root;
-	acpi_handle handle;
+	const struct pci_host_bridge *host;
+	u32 slot_cap;
 
-	handle = acpi_find_root_bridge_handle(pdev);
-	if (!handle)
+	if (!pciehp_available())
+		return false;
+	if (!bridge)
 		return false;
 
-	root = acpi_pci_find_root(handle);
-	if (!root)
+	pcie_capability_read_dword(bridge, PCI_EXP_SLTCAP, &slot_cap);
+	if (!(slot_cap & PCI_EXP_SLTCAP_HPC))
 		return false;
 
-	return root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL;
+	if (pcie_ports_native)
+		return true;
+
+	host = pci_find_host_bridge(bridge->bus);
+	return host->native_pcie_hotplug;
 }
 
 /**
diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h
index d0c6783dbfe3..aa542dc10d23 100644
--- a/drivers/pci/pcie/portdrv.h
+++ b/drivers/pci/pcie/portdrv.h
@@ -11,8 +11,6 @@ 
 
 #include <linux/compiler.h>
 
-extern bool pcie_ports_native;
-
 /* Service Type */
 #define PCIE_PORT_SERVICE_PME_SHIFT	0	/* Power Management Event */
 #define PCIE_PORT_SERVICE_PME		(1 << PCIE_PORT_SERVICE_PME_SHIFT)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 359a197d0310..2f6689a14e8d 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1451,8 +1451,10 @@  static inline int pci_irqd_intx_xlate(struct irq_domain *d,
 
 #ifdef CONFIG_PCIEPORTBUS
 extern bool pcie_ports_disabled;
+extern bool pcie_ports_native;
 #else
 #define pcie_ports_disabled	true
+#define pcie_ports_native	false
 #endif
 
 #ifdef CONFIG_PCIEASPM
diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
index 46fb90b5164b..ee2b1674a601 100644
--- a/include/linux/pci_hotplug.h
+++ b/include/linux/pci_hotplug.h
@@ -162,7 +162,7 @@  struct hotplug_params {
 #ifdef CONFIG_ACPI
 #include <linux/acpi.h>
 int pci_get_hp_params(struct pci_dev *dev, struct hotplug_params *hpp);
-bool pciehp_is_native(struct pci_dev *pdev);
+bool pciehp_is_native(struct pci_dev *bridge);
 int acpi_get_hp_hw_control_from_firmware(struct pci_dev *dev, u32 flags);
 int acpi_pci_check_ejectable(struct pci_bus *pbus, acpi_handle handle);
 int acpi_pci_detect_ejectable(acpi_handle handle);
@@ -172,7 +172,7 @@  static inline int pci_get_hp_params(struct pci_dev *dev,
 {
 	return -ENODEV;
 }
-static inline bool pciehp_is_native(struct pci_dev *pdev) { return true; }
+static inline bool pciehp_is_native(struct pci_dev *bridge) { return true; }
 #endif
 
 #ifdef CONFIG_HOTPLUG_PCI_PCIE