[PATCH v1 0/4] PCI: shpchp: NULL pointer fix and cleanups

[PATCH v1 0/4] PCI: shpchp: NULL pointer fix and cleanups

[Bjorn Helgaas]

The first patch fixes a NULL pointer dereference reported by Marc and would
be targeted for v4.18.

The others simplify the shpchp probe path and would be for v4.19.

---

Bjorn Helgaas (4):
      PCI: shpchp: Manage SHPC unconditionally on non-ACPI systems
      PCI: shpchp: Separate existence of SHPC and permission to use it
      PCI: shpchp: Inline shpchp_is_native()
      PCI: hotplug: Implement hotplug_is_native() only when CONFIG_ACPI=y


 drivers/pci/hotplug/acpi_pcihp.c  |   40 +++++++++++++++++++++++--------------
 drivers/pci/hotplug/shpchp_core.c |   21 +++++++++++++++++++
 drivers/pci/pci-acpi.c            |   29 ---------------------------
 include/linux/pci.h               |    1 +
 include/linux/pci_hotplug.h       |   20 ++++++++++---------
 5 files changed, 58 insertions(+), 53 deletions(-)

[PATCH v1 1/4] PCI: shpchp: Manage SHPC unconditionally on non-ACPI systems

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

If acpi_pci_find_root() returns NULL, it means there's no ACPI host bridge
device (PNP0A03 or PNP0A08), and the OS is always allowed to manage the
SHPC, so return success in that case.

This fixes a NULL pointer dereference when CONFIG_ACPI=y but the current
hardware/firmware platform doesn't support ACPI.  In that case,
acpi_get_hp_hw_control_from_firmware() is implemented but
acpi_pci_find_root() returns NULL.

Fixes: 90cc0c3cc709 ("PCI: shpchp: Add shpchp_is_native()")
Link: https://lkml.kernel.org/r/20180621164715.28160-1-marc.zyngier@arm.com
Reported-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/hotplug/acpi_pcihp.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/hotplug/acpi_pcihp.c b/drivers/pci/hotplug/acpi_pcihp.c
index 3979f89b250a..5bd6c1573295 100644
--- a/drivers/pci/hotplug/acpi_pcihp.c
+++ b/drivers/pci/hotplug/acpi_pcihp.c
@@ -7,7 +7,6 @@
  * All rights reserved.
  *
  * Send feedback to <kristen.c.accardi@intel.com>
- *
  */
 
 #include <linux/module.h>
@@ -87,8 +86,17 @@ int acpi_get_hp_hw_control_from_firmware(struct pci_dev *pdev)
 		return 0;
 
 	/* If _OSC exists, we should not evaluate OSHP */
+
+	/*
+	 * If there's no ACPI host bridge (i.e., ACPI support is compiled
+	 * into the kernel but the hardware platform doesn't support ACPI),
+	 * there's nothing to do here.
+	 */
 	host = pci_find_host_bridge(pdev->bus);
 	root = acpi_pci_find_root(ACPI_HANDLE(&host->dev));
+	if (!root)
+		return 0;
+
 	if (root->osc_support_set)
 		goto no_control;
 

[PATCH v1 2/4] PCI: shpchp: Separate existence of SHPC and permission to use it

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

The shpchp driver registers for all PCI bridge devices.  Its probe method
should fail if either (1) the bridge doesn't have an SHPC or (2) the OS
isn't allowed to use it (the platform firmware may be operating the SHPC
itself).

Separate these two tests into:

  - A new shpc_capable() that looks for the SHPC hardware and is applicable
    on all systems (ACPI and non-ACPI), and

  - A simplified acpi_get_hp_hw_control_from_firmware() that we call only
    when we already know an SHPC exists and there may be ACPI methods to
    either request permission to use it (_OSC) or transfer control to the
    OS (OSHP).

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/hotplug/acpi_pcihp.c  |   36 +++++++++++++++++++-----------------
 drivers/pci/hotplug/shpchp_core.c |   21 +++++++++++++++++++++
 drivers/pci/pci-acpi.c            |   19 +------------------
 include/linux/pci.h               |    1 +
 4 files changed, 42 insertions(+), 35 deletions(-)

diff --git a/drivers/pci/hotplug/acpi_pcihp.c b/drivers/pci/hotplug/acpi_pcihp.c
index 5bd6c1573295..6b7c1ed58e7e 100644
--- a/drivers/pci/hotplug/acpi_pcihp.c
+++ b/drivers/pci/hotplug/acpi_pcihp.c
@@ -73,20 +73,6 @@ int acpi_get_hp_hw_control_from_firmware(struct pci_dev *pdev)
 	acpi_handle chandle, handle;
 	struct acpi_buffer string = { ACPI_ALLOCATE_BUFFER, NULL };
 
-	/*
-	 * Per PCI firmware specification, we should run the ACPI _OSC
-	 * method to get control of hotplug hardware before using it. If
-	 * an _OSC is missing, we look for an OSHP to do the same thing.
-	 * To handle different BIOS behavior, we look for _OSC on a root
-	 * bridge preferentially (according to PCI fw spec). Later for
-	 * OSHP within the scope of the hotplug controller and its parents,
-	 * up to the host bridge under which this controller exists.
-	 */
-	if (shpchp_is_native(pdev))
-		return 0;
-
-	/* If _OSC exists, we should not evaluate OSHP */
-
 	/*
 	 * If there's no ACPI host bridge (i.e., ACPI support is compiled
 	 * into the kernel but the hardware platform doesn't support ACPI),
@@ -97,9 +83,25 @@ int acpi_get_hp_hw_control_from_firmware(struct pci_dev *pdev)
 	if (!root)
 		return 0;
 
-	if (root->osc_support_set)
-		goto no_control;
+	/*
+	 * If _OSC exists, it determines whether we're allowed to manage
+	 * the SHPC.  We executed it while enumerating the host bridge.
+	 */
+	if (root->osc_support_set) {
+		if (host->native_shpc_hotplug)
+			return 0;
+		return -ENODEV;
+	}
 
+	/*
+	 * In the absence of _OSC, we're always allowed to manage the SHPC.
+	 * However, if an OSHP method is present, we must execute it so the
+	 * firmware can transfer control to the OS, e.g., direct interrupts
+	 * to the OS instead of to the firmware.
+	 *
+	 * N.B. The PCI Firmware Spec (r3.2, sec 4.8) does not endorse
+	 * searching up the ACPI hierarchy, so the loops below are suspect.
+	 */
 	handle = ACPI_HANDLE(&pdev->dev);
 	if (!handle) {
 		/*
@@ -128,7 +130,7 @@ int acpi_get_hp_hw_control_from_firmware(struct pci_dev *pdev)
 		if (ACPI_FAILURE(status))
 			break;
 	}
-no_control:
+
 	pci_info(pdev, "Cannot get control of SHPC hotplug\n");
 	kfree(string.pointer);
 	return -ENODEV;
diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c
index e91be287f292..8e3c6ce12f31 100644
--- a/drivers/pci/hotplug/shpchp_core.c
+++ b/drivers/pci/hotplug/shpchp_core.c
@@ -270,11 +270,30 @@ static int get_adapter_status(struct hotplug_slot *hotplug_slot, u8 *value)
 	return 0;
 }
 
+static bool shpc_capable(struct pci_dev *bridge)
+{
+	/*
+	 * It is assumed that AMD GOLAM chips support SHPC but they do not
+	 * have SHPC capability.
+	 */
+	if (bridge->vendor == PCI_VENDOR_ID_AMD &&
+	    bridge->device == PCI_DEVICE_ID_AMD_GOLAM_7450)
+		return true;
+
+	if (pci_find_capability(bridge, PCI_CAP_ID_SHPC))
+		return true;
+
+	return false;
+}
+
 static int shpc_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	int rc;
 	struct controller *ctrl;
 
+	if (!shpc_capable(pdev))
+		return -ENODEV;
+
 	if (acpi_get_hp_hw_control_from_firmware(pdev))
 		return -ENODEV;
 
@@ -303,6 +322,7 @@ static int shpc_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (rc)
 		goto err_cleanup_slots;
 
+	pdev->shpc_managed = 1;
 	return 0;
 
 err_cleanup_slots:
@@ -319,6 +339,7 @@ static void shpc_remove(struct pci_dev *dev)
 {
 	struct controller *ctrl = pci_get_drvdata(dev);
 
+	dev->shpc_managed = 0;
 	shpchp_remove_ctrl_files(ctrl);
 	ctrl->hpc_ops->release_ctlr(ctrl);
 	kfree(ctrl);
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 65113b6eed14..5100fd2d5a75 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -403,24 +403,7 @@ bool pciehp_is_native(struct pci_dev *bridge)
  */
 bool shpchp_is_native(struct pci_dev *bridge)
 {
-	const struct pci_host_bridge *host;
-
-	if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
-		return false;
-
-	/*
-	 * It is assumed that AMD GOLAM chips support SHPC but they do not
-	 * have SHPC capability.
-	 */
-	if (bridge->vendor == PCI_VENDOR_ID_AMD &&
-	    bridge->device == PCI_DEVICE_ID_AMD_GOLAM_7450)
-		return true;
-
-	if (!pci_find_capability(bridge, PCI_CAP_ID_SHPC))
-		return false;
-
-	host = pci_find_host_bridge(bridge->bus);
-	return host->native_shpc_hotplug;
+	return bridge->shpc_managed;
 }
 
 /**
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 340029b2fb38..f776a1cce120 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -388,6 +388,7 @@ struct pci_dev {
 	unsigned int	is_virtfn:1;
 	unsigned int	reset_fn:1;
 	unsigned int	is_hotplug_bridge:1;
+	unsigned int	shpc_managed:1;		/* SHPC owned by shpchp */
 	unsigned int	is_thunderbolt:1;	/* Thunderbolt controller */
 	unsigned int	__aer_firmware_first_valid:1;
 	unsigned int	__aer_firmware_first:1;

[PATCH v1 3/4] PCI: shpchp: Inline shpchp_is_native()

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

shpchp_is_native() is trivial and has nothing to do with ACPI, so make it
an inline in pci_hotplug.h.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci-acpi.c      |   12 ------------
 include/linux/pci_hotplug.h |    8 ++++++--
 2 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 5100fd2d5a75..52b8434d4d6e 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -394,18 +394,6 @@ bool pciehp_is_native(struct pci_dev *bridge)
 	return host->native_pcie_hotplug;
 }
 
-/**
- * shpchp_is_native - Check whether a hotplug port is handled by the OS
- * @bridge: Hotplug port to check
- *
- * Returns true if the given @bridge is handled by the native SHPC hotplug
- * driver.
- */
-bool shpchp_is_native(struct pci_dev *bridge)
-{
-	return bridge->shpc_managed;
-}
-
 /**
  * pci_acpi_wake_bus - Root bus wakeup notification fork function.
  * @context: Device wakeup context.
diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
index cf5e22103f68..f08a44e1606f 100644
--- a/include/linux/pci_hotplug.h
+++ b/include/linux/pci_hotplug.h
@@ -162,11 +162,15 @@ 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 *bridge);
 int acpi_get_hp_hw_control_from_firmware(struct pci_dev *bridge);
-bool shpchp_is_native(struct pci_dev *bridge);
 int acpi_pci_check_ejectable(struct pci_bus *pbus, acpi_handle handle);
 int acpi_pci_detect_ejectable(acpi_handle handle);
+
+bool pciehp_is_native(struct pci_dev *bridge);
+static inline bool shpchp_is_native(struct pci_dev *bridge)
+{
+	return bridge->shpc_managed;
+}
 #else
 static inline int pci_get_hp_params(struct pci_dev *dev,
 				    struct hotplug_params *hpp)

[PATCH v1 4/4] PCI: hotplug: Implement hotplug_is_native() only when CONFIG_ACPI=y

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

hotplug_is_native() is used only by the acpiphp driver, so we only need to
implement the interface when CONFIG_ACPI=y.

Move the implementation under #ifdef CONFIG_ACPI and drop the unnecessary
stubs for the non-ACPI case.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 include/linux/pci_hotplug.h |   12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
index f08a44e1606f..09eb40299c37 100644
--- a/include/linux/pci_hotplug.h
+++ b/include/linux/pci_hotplug.h
@@ -9,7 +9,6 @@
  * All rights reserved.
  *
  * Send feedback to <kristen.c.accardi@intel.com>
- *
  */
 #ifndef _PCI_HOTPLUG_H
 #define _PCI_HOTPLUG_H
@@ -171,6 +170,11 @@ static inline bool shpchp_is_native(struct pci_dev *bridge)
 {
 	return bridge->shpc_managed;
 }
+
+static inline bool hotplug_is_native(struct pci_dev *bridge)
+{
+	return pciehp_is_native(bridge) || shpchp_is_native(bridge);
+}
 #else
 static inline int pci_get_hp_params(struct pci_dev *dev,
 				    struct hotplug_params *hpp)
@@ -182,12 +186,6 @@ static inline int acpi_get_hp_hw_control_from_firmware(struct pci_dev *bridge)
 {
 	return 0;
 }
-static inline bool pciehp_is_native(struct pci_dev *bridge) { return true; }
-static inline bool shpchp_is_native(struct pci_dev *bridge) { return true; }
 #endif
 
-static inline bool hotplug_is_native(struct pci_dev *bridge)
-{
-	return pciehp_is_native(bridge) || shpchp_is_native(bridge);
-}
 #endif

Re: [PATCH v1 1/4] PCI: shpchp: Manage SHPC unconditionally on non-ACPI systems

[Marc Zyngier]

On 25/06/18 23:59, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> If acpi_pci_find_root() returns NULL, it means there's no ACPI host bridge
> device (PNP0A03 or PNP0A08), and the OS is always allowed to manage the
> SHPC, so return success in that case.
> 
> This fixes a NULL pointer dereference when CONFIG_ACPI=y but the current
> hardware/firmware platform doesn't support ACPI.  In that case,
> acpi_get_hp_hw_control_from_firmware() is implemented but
> acpi_pci_find_root() returns NULL.
> 
> Fixes: 90cc0c3cc709 ("PCI: shpchp: Add shpchp_is_native()")
> Link: https://lkml.kernel.org/r/20180621164715.28160-1-marc.zyngier@arm.com
> Reported-by: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Tested-by: Marc Zyngier <marc.zyngier@arm.com>

Thanks for the quick turnaround!

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v1 0/4] PCI: shpchp: NULL pointer fix and cleanups

[Mika Westerberg]

On Mon, Jun 25, 2018 at 05:59:33PM -0500, Bjorn Helgaas wrote:
> The first patch fixes a NULL pointer dereference reported by Marc and would
> be targeted for v4.18.
> 
> The others simplify the shpchp probe path and would be for v4.19.

Thanks for taking care of this, Bjorn!

For the whole series,

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

Re: [PATCH v1 0/4] PCI: shpchp: NULL pointer fix and cleanups

[Bjorn Helgaas]

On Mon, Jun 25, 2018 at 05:59:33PM -0500, Bjorn Helgaas wrote:
> The first patch fixes a NULL pointer dereference reported by Marc and would
> be targeted for v4.18.
> 
> The others simplify the shpchp probe path and would be for v4.19.
> 
> ---
> 
> Bjorn Helgaas (4):
>       PCI: shpchp: Manage SHPC unconditionally on non-ACPI systems
>       PCI: shpchp: Separate existence of SHPC and permission to use it
>       PCI: shpchp: Inline shpchp_is_native()
>       PCI: hotplug: Implement hotplug_is_native() only when CONFIG_ACPI=y

I applied the first (with Marc's tested-by and Mika's reviewed-by) to
for-linus for v4.18.

I applied the others with Mika's reviewed-by to pci/hotplug for v4.19.

>  drivers/pci/hotplug/acpi_pcihp.c  |   40 +++++++++++++++++++++++--------------
>  drivers/pci/hotplug/shpchp_core.c |   21 +++++++++++++++++++
>  drivers/pci/pci-acpi.c            |   29 ---------------------------
>  include/linux/pci.h               |    1 +
>  include/linux/pci_hotplug.h       |   20 ++++++++++---------
>  5 files changed, 58 insertions(+), 53 deletions(-)