Add Error Disconnect Recover (EDR) support.

From: Kuppuswamy Sathyanarayanan <[email protected]>

As per PCI firmware specification v3.2 ECN
(https://members.pcisig.com/wg/PCI-SIG/document/12614), when firmware owns Downstream Port Containment (DPC), its expected to use the "Error Disconnect Recover" (EDR) notification to alert OSPM of a DPC event and if OS supports EDR, its expected to handle the software state invalidation and port recovery in OS and let firmware know the recovery status via _OST ACPI call.

Since EDR is a hybrid service, DPC service shall be enabled in OS even if AER is not natively enabled in OS.

Signed-off-by Kuppuswamy Sathyanarayanan <[email protected]>

---
 drivers/pci/pcie/Kconfig        |  10 +
 drivers/pci/pcie/dpc.c          | 326 ++++++++++++++++++++++++++++++--

 drivers/pci/pcie/portdrv_core.c |   9 +-
 3 files changed, 329 insertions(+), 16 deletions(-)

drivers/pci/pcie/Kconfig +10 -0

index 5cbdbca904ac..55f65d1cbc9e 100644
@@ -142,3 +142,13 @@ config PCIE_PTM
 
 	  This is only useful if you have devices that support PTM, but it
 	  is safe to enable even if you don't.
+
+config PCIE_EDR
+	bool "PCI Express Error Disconnect Recover support"
+	default n
+	depends on PCIE_DPC && ACPI
+	help
+	  This options adds Error Disconnect Recover support as specified
+	  in PCI firmware specification v3.2 Downstream Port Containment
+	  Related Enhancements ECN. Enable this if you want to support hybrid
+	  DPC model which uses both firmware and OS to implement DPC.

drivers/pci/pcie/dpc.c +1 -0

index 7b77754a82de..bdf4ca8a0acb 100644
@@ -11,6 +11,7 @@
 #include <linux/interrupt.h>
 #include <linux/init.h>
 #include <linux/pci.h>
+#include <linux/acpi.h>
 
 #include "portdrv.h"
 #include "../pci.h"

drivers/pci/pcie/dpc.c +28 -2

+	pci_ers_result_t	error_state;
+#ifdef CONFIG_ACPI
+	struct acpi_device	*adev;
+#endif
 };
 
+#ifdef CONFIG_ACPI
+
+#define EDR_PORT_ENABLE_DSM     0x0C
+#define EDR_PORT_LOCATE_DSM     0x0D
+
+static const guid_t pci_acpi_dsm_guid =
+		GUID_INIT(0xe5c937d0, 0x3553, 0x4d7a,
+			  0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d);
+#endif
+
 static const char * const rp_pio_error_string[] = {
 	"Configuration Request received UR Completion",	 /* Bit Position 0  */
 	"Configuration Request received CA Completion",	 /* Bit Position 1  */

@@ -67,6 +83,9 @@ void pci_save_dpc_state(struct pci_dev *dev)
 	if (!dpc)
 		return;
 
+	if (!dpc->native_dpc)
+		return;
+
 	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_DPC);
 	if (!save_state)
 		return;

@@ -88,6 +107,9 @@ void pci_restore_dpc_state(struct pci_dev *dev)
 	if (!dpc)
 		return;
 
+	if (!dpc->native_dpc)
+		return;
+
 	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_DPC);
 	if (!save_state)
 		return;

@@ -224,10 +246,9 @@ static int dpc_get_aer_uncorrect_severity(struct pci_dev *dev,
 	return 1;
 }
 
-static irqreturn_t dpc_handler(int irq, void *context)
+static void dpc_process_error(struct dpc_dev *dpc)
 {
 	struct aer_err_info info;
-	struct dpc_dev *dpc = context;
 	struct pci_dev *pdev = dpc->dev->port;
 	struct device *dev = &dpc->dev->device;
 	u16 cap = dpc->cap_pos, status, source, reason, ext_reason;

@@ -261,6 +282,13 @@ static irqreturn_t dpc_handler(int irq, void *context)
 
 	/* We configure DPC so it only triggers on ERR_FATAL */
 	pcie_do_recovery(pdev, pci_channel_io_frozen, PCIE_PORT_SERVICE_DPC);
+}
+
+static irqreturn_t dpc_handler(int irq, void *context)
+{
+	struct dpc_dev *dpc = context;
+
+	dpc_process_error(dpc);
 
 	return IRQ_HANDLED;
 }

drivers/pci/pcie/dpc.c +11 -0

+	dpc->error_state = PCI_ERS_RESULT_RECOVERED;
+}
+
+#ifdef CONFIG_ACPI
+
+/*
+ * _DSM wrapper function to enable/disable DPC port.
+ * @dpc   : DPC device structure
+ * @enable: status of DPC port (0 or 1).
+ *
+ * returns 0 on success or errno on failure.

drivers/pci/pcie/dpc.c +26 -0

+	if (!status)
+		return -ENOTSUPP;
+
+	argv4.type = ACPI_TYPE_INTEGER;
+	argv4.integer.value = enable;
+
+	obj = acpi_evaluate_dsm(dpc->adev->handle, &pci_acpi_dsm_guid, 1,
+				EDR_PORT_ENABLE_DSM, &argv4);
+	if (!obj)
+		return -EIO;
+
+	if (obj->type == ACPI_TYPE_INTEGER && obj->integer.value == enable)
+		status = 0;
+	else
+		status = -EIO;
+
+	ACPI_FREE(obj);
+
+	return status;
+}
+
+/*
+ * _DSM wrapper function to locate DPC port.
+ * @dpc   : DPC device structure
+ *
+ * returns pci_dev or NULL.

drivers/pci/pcie/dpc.c +17 -0

+	obj = acpi_evaluate_dsm(dpc->adev->handle, &pci_acpi_dsm_guid, 1,
+				EDR_PORT_LOCATE_DSM, NULL);
+	if (!obj)
+		return NULL;
+
+	if (obj->type == ACPI_TYPE_INTEGER) {
+		/*
+		 * Firmware returns DPC port BDF details in following format:
+		 *	15:8 = bus
+		 *	7:3 = device
+		 *	2:0 = function
+		 */
+		port = obj->integer.value;
+		ACPI_FREE(obj);
+	} else {
+		ACPI_FREE(obj);
+		return NULL;

drivers/pci/pcie/dpc.c +40 -0

+	status = acpi_evaluate_ost(dpc->adev->handle,
+				   ACPI_NOTIFY_DISCONNECT_RECOVER,
+				   ost_status, NULL);
+	if (ACPI_FAILURE(status))
+		return -EINVAL;
+
+	return 0;
+}
+
+/*
+ * Helper function used for disconnecting the child devices when EDR event is
+ * received from firmware.
+ */
+static void dpc_disconnect_devices(struct pci_dev *dev)
+{
+	struct pci_dev *udev;
+	struct pci_bus *parent;
+	struct pci_dev *pdev, *temp;
+
+	dev_dbg(&dev->dev, "Disconnecting the child devices\n");
+
+	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
+		udev = dev;
+	else
+		udev = dev->bus->self;
+
+	parent = udev->subordinate;
+	pci_walk_bus(parent, pci_dev_set_disconnected, NULL);
+
+	pci_lock_rescan_remove();
+	pci_dev_get(dev);
+	list_for_each_entry_safe_reverse(pdev, temp, &parent->devices,
+					 bus_list) {
+		pci_stop_and_remove_bus_device(pdev);
+	}
+	pci_dev_put(dev);
+	pci_unlock_rescan_remove();
+}
+
+static void edr_handle_event(acpi_handle handle, u32 event, void *data)

drivers/pci/pcie/dpc.c +46 -10

+			return;
+		}
+		dpc_process_error(dpc);
+	}
+
+	if (dpc->error_state == PCI_ERS_RESULT_RECOVERED) {
+		/*
+		 * Recovery is successful, so send
+		 * _OST(0xF, BDF << 16 | 0x80, "") to firmware.
+		 */
+		status = 0x80;
+	} else {
+		/*
+		 * Recovery is not successful, so disconnect child devices
+		 * and send _OST(0xF, BDF << 16 | 0x81, "") to firmware.
+		 */
+		dpc_disconnect_devices(pdev);
+		status = 0x81;
+	}
+
+	acpi_send_edr_status(dpc, status);
+}
+
+#endif
+
 #define FLAG(x, y) (((x) & (y)) ? '+' : '-')
 static int dpc_probe(struct pcie_device *dev)
 {

@@ -291,9 +543,10 @@ static int dpc_probe(struct pcie_device *dev)
 	struct device *device = &dev->device;
 	int status;
 	u16 ctl, cap;
-
-	if (pcie_aer_get_firmware_first(pdev))
-		return -ENOTSUPP;
+#ifdef CONFIG_ACPI
+	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+	acpi_status astatus;
+#endif
 
 	dpc = devm_kzalloc(device, sizeof(*dpc), GFP_KERNEL);
 	if (!dpc)

@@ -302,16 +555,53 @@ static int dpc_probe(struct pcie_device *dev)
 	dpc->cap_pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DPC);
 	dpc->dev = dev;
 	set_service_data(dev, dpc);
+	dpc->error_state = PCI_ERS_RESULT_NONE;
+
+	if (!pcie_aer_get_firmware_first(pdev))
+		if (pci_aer_available() && dpc->cap_pos)
+			dpc->native_dpc = 1;
 
-	status = devm_request_threaded_irq(device, dev->irq, dpc_irq,
-					   dpc_handler, IRQF_SHARED,
-					   "pcie-dpc", dpc);
-	if (status) {
-		dev_warn(device, "request IRQ%d failed: %d\n", dev->irq,
-			 status);
-		return status;
+	/*
+	 * If native support is not enabled and ACPI is not
+	 * enabled then return error.
+	 */
+	if (!dpc->native_dpc && !IS_ENABLED(CONFIG_APCI))
+		return -ENODEV;
+
+	if (dpc->native_dpc) {
+		status = devm_request_threaded_irq(device, dev->irq, dpc_irq,
+						   dpc_handler, IRQF_SHARED,
+						   "pcie-dpc", dpc);
+		if (status) {

drivers/pci/pcie/dpc.c +16 -2

+			return -EBUSY;
+		}
+
+		acpi_enable_dpc_port(dpc, true);
+	}
+#endif
 	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CAP, &cap);
 	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, &ctl);
 

@@ -325,8 +615,12 @@ static int dpc_probe(struct pcie_device *dev)
 		}
 	}
 
-	ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
-	pci_write_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, ctl);
+	if (dpc->native_dpc) {
+		ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_FATAL |
+			PCI_EXP_DPC_CTL_INT_EN;
+		pci_write_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL,
+				      ctl);
+	}
 
 	dev_info(device, "DPC error containment capabilities: Int Msg #%d, RPExt%c PoisonedTLP%c SwTrigger%c RP PIO Log %d, DL_ActiveErr%c\n",
 		cap & PCI_EXP_DPC_IRQ, FLAG(cap, PCI_EXP_DPC_CAP_RP_EXT),

@@ -344,6 +638,9 @@ static void dpc_remove(struct pcie_device *dev)
 	struct pci_dev *pdev = dev->port;
 	u16 ctl;
 
+	if (!dpc->native_dpc)
+		return;
+
 	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, &ctl);
 	ctl &= ~(PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN);
 	pci_write_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, ctl);

@@ -356,6 +653,7 @@ static struct pcie_port_service_driver dpcdriver = {
 	.probe		= dpc_probe,
 	.remove		= dpc_remove,
 	.reset_link	= dpc_reset_link,
+	.error_resume   = dpc_error_resume,
 };
 
 int __init pcie_dpc_init(void)

drivers/pci/pcie/portdrv_core.c +0 -0

index 865f12f4b314..050cbb7a5083 100644
@@ -249,9 +249,14 @@ static int get_port_device_capability(struct pci_dev *dev)
 		pcie_pme_interrupt_enable(dev, false);
 	}
 

drivers/pci/pcie/portdrv_core.c +0 -0

 		services |= PCIE_PORT_SERVICE_DPC;
 
 	if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM || -- 2.20.1

Quoted 10 lines · On Tue, Mar 19, 2019 at 01:47:29PM -0700…

From: Kuppuswamy Sathyanarayanan <[email protected]>

As per PCI firmware specification v3.2 ECN
(https://members.pcisig.com/wg/PCI-SIG/document/12614), when firmware owns Downstream Port Containment (DPC), its expected to use the "Error Disconnect Recover" (EDR) notification to alert OSPM of a DPC event and if OS supports EDR, its expected to handle the software state invalidation and port recovery in OS and let firmware know the recovery status via _OST ACPI call.

Quoted 7 lines

Since EDR is a hybrid service, DPC service shall be enabled in OS even if AER is not natively enabled in OS.

Signed-off-by Kuppuswamy Sathyanarayanan <[email protected]>

---
  drivers/pci/pcie/Kconfig        |  10 +
  drivers/pci/pcie/dpc.c          | 326 ++++++++++++++++++++++++++++++--

I'll be looking for Keith's reviewed-by for this eventually.

It looks like this could be split into some smaller patches:

- Add dpc_dev.native_dpc (hopefully we can find a less confusing name)
- Convert dpc_handler() to dpc_handler() + dpc_process_error()
- Add EDR support

Quoted 32 lines

  drivers/pci/pcie/portdrv_core.c |   9 +-
  3 files changed, 329 insertions(+), 16 deletions(-)

drivers/pci/pcie/Kconfig +10 -0

index 5cbdbca904ac..55f65d1cbc9e 100644
@@ -142,3 +142,13 @@ config PCIE_PTM
 
  	  This is only useful if you have devices that support PTM, but it
  	  is safe to enable even if you don't.
+
+config PCIE_EDR
+	bool "PCI Express Error Disconnect Recover support"
+	default n
+	depends on PCIE_DPC && ACPI
+	help
+	  This options adds Error Disconnect Recover support as specified
+	  in PCI firmware specification v3.2 Downstream Port Containment
+	  Related Enhancements ECN. Enable this if you want to support hybrid
+	  DPC model which uses both firmware and OS to implement DPC.

drivers/pci/pcie/dpc.c +1 -0

index 7b77754a82de..bdf4ca8a0acb 100644
@@ -11,6 +11,7 @@
  #include <linux/interrupt.h>
  #include <linux/init.h>
  #include <linux/pci.h>
+#include <linux/acpi.h>
 

drivers/pci/pcie/dpc.c struct dpc_dev { · line 24

Quoted 7 lines

drivers/pci/pcie/dpc.c +1 -0

  #include "portdrv.h"
  #include "../pci.h"

@@ -20,8 +21,23 @@ struct dpc_dev {
  	u16			cap_pos;
  	bool			rp_extensions;
  	u8			rp_log_size;
+	bool			native_dpc;

This is going to be way too confusing with a "native_dpc" in both the struct pci_host_bridge and the struct dpc_dev.

Quoted 64 lines

drivers/pci/pcie/dpc.c +28 -2

+	pci_ers_result_t	error_state;
+#ifdef CONFIG_ACPI
+	struct acpi_device	*adev;
+#endif
  };
 
+#ifdef CONFIG_ACPI
+
+#define EDR_PORT_ENABLE_DSM     0x0C
+#define EDR_PORT_LOCATE_DSM     0x0D
+
+static const guid_t pci_acpi_dsm_guid =
+		GUID_INIT(0xe5c937d0, 0x3553, 0x4d7a,
+			  0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d);
+#endif
+
  static const char * const rp_pio_error_string[] = {
  	"Configuration Request received UR Completion",	 /* Bit Position 0  */
  	"Configuration Request received CA Completion",	 /* Bit Position 1  */

@@ -67,6 +83,9 @@ void pci_save_dpc_state(struct pci_dev *dev)
  	if (!dpc)
  		return;
 
+	if (!dpc->native_dpc)
+		return;
+
  	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_DPC);
  	if (!save_state)
  		return;

@@ -88,6 +107,9 @@ void pci_restore_dpc_state(struct pci_dev *dev)
  	if (!dpc)
  		return;
 
+	if (!dpc->native_dpc)
+		return;
+
  	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_DPC);
  	if (!save_state)
  		return;

@@ -224,10 +246,9 @@ static int dpc_get_aer_uncorrect_severity(struct pci_dev *dev,
  	return 1;
  }
 
-static irqreturn_t dpc_handler(int irq, void *context)
+static void dpc_process_error(struct dpc_dev *dpc)
  {
  	struct aer_err_info info;
-	struct dpc_dev *dpc = context;
  	struct pci_dev *pdev = dpc->dev->port;
  	struct device *dev = &dpc->dev->device;
  	u16 cap = dpc->cap_pos, status, source, reason, ext_reason;

@@ -261,6 +282,13 @@ static irqreturn_t dpc_handler(int irq, void *context)
 
  	/* We configure DPC so it only triggers on ERR_FATAL */
  	pcie_do_recovery(pdev, pci_channel_io_frozen, PCIE_PORT_SERVICE_DPC);
+}
+
+static irqreturn_t dpc_handler(int irq, void *context)
+{
+	struct dpc_dev *dpc = context;
+
+	dpc_process_error(dpc);
 

drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 314

Quoted 7 lines

drivers/pci/pcie/dpc.c +1 -0

  	return IRQ_HANDLED;
  }

@@ -283,6 +311,230 @@ static irqreturn_t dpc_irq(int irq, void *context)
  	return IRQ_HANDLED;
  }
 
+void dpc_error_resume(struct pci_dev *dev)

Looks like this should be static?

drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 320

Quoted 7 lines

drivers/pci/pcie/dpc.c +6 -0

+{
+	struct dpc_dev *dpc;
+
+	dpc = to_dpc_dev(dev);
+	if (!dpc)
+		return;

I don't think it's possible for dpc to be NULL, is it? Remove the test if not.

Quoted 17 lines

drivers/pci/pcie/dpc.c +16 -0

+	dpc->error_state = PCI_ERS_RESULT_RECOVERED;
+}
+
+#ifdef CONFIG_ACPI
+
+/*
+ * _DSM wrapper function to enable/disable DPC port.
+ * @dpc   : DPC device structure
+ * @enable: status of DPC port (0 or 1).
+ *
+ * returns 0 on success or errno on failure.
+ */
+static int acpi_enable_dpc_port(struct dpc_dev *dpc, bool enable)
+{
+	union acpi_object *obj;
+	int status;

drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 342

Quoted 5 lines

drivers/pci/pcie/dpc.c +5 -0

+	union acpi_object argv4;
+
+	/* Check whether EDR_PORT_ENABLE_DSM is supported in firmware */
+	status = acpi_check_dsm(dpc->adev->handle, &pci_acpi_dsm_guid, 1,
+				1 << EDR_PORT_ENABLE_DSM);

I don't think this acpi_check_dsm() is necessary, is it? I expect acpi_evaluate_dsm() to fail nicely if there's no _DSM or it doesn't support the function.

Quoted 30 lines

drivers/pci/pcie/dpc.c +29 -0

+	if (!status)
+		return -ENOTSUPP;
+
+	argv4.type = ACPI_TYPE_INTEGER;
+	argv4.integer.value = enable;
+
+	obj = acpi_evaluate_dsm(dpc->adev->handle, &pci_acpi_dsm_guid, 1,
+				EDR_PORT_ENABLE_DSM, &argv4);
+	if (!obj)
+		return -EIO;
+
+	if (obj->type == ACPI_TYPE_INTEGER && obj->integer.value == enable)
+		status = 0;
+	else
+		status = -EIO;
+
+	ACPI_FREE(obj);
+
+	return status;
+}
+
+/*
+ * _DSM wrapper function to locate DPC port.
+ * @dpc   : DPC device structure
+ *
+ * returns pci_dev or NULL.
+ */
+static struct pci_dev *acpi_locate_dpc_port(struct dpc_dev *dpc)
+{

drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 378

Quoted 7 lines

drivers/pci/pcie/dpc.c +7 -0

+	union acpi_object *obj;
+	int status;
+	u16 port;
+
+	/* Check whether EDR_PORT_LOCATE_DSM is supported in firmware */
+	status = acpi_check_dsm(dpc->adev->handle, &pci_acpi_dsm_guid, 1,
+				1 << EDR_PORT_LOCATE_DSM);

Unnecessary?

drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 379

Quoted 3 lines

drivers/pci/pcie/dpc.c +2 -0

+	if (!status)
+		return dpc->dev->port;

If you *do* need the acpi_check_dsm(), I would have expected to return NULL in this error case?

drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 381

Quoted 2 lines

drivers/pci/pcie/dpc.c +2 -0

+
+

Extra blank line here.

Quoted 17 lines

drivers/pci/pcie/dpc.c +17 -0

+	obj = acpi_evaluate_dsm(dpc->adev->handle, &pci_acpi_dsm_guid, 1,
+				EDR_PORT_LOCATE_DSM, NULL);
+	if (!obj)
+		return NULL;
+
+	if (obj->type == ACPI_TYPE_INTEGER) {
+		/*
+		 * Firmware returns DPC port BDF details in following format:
+		 *	15:8 = bus
+		 *	7:3 = device
+		 *	2:0 = function
+		 */
+		port = obj->integer.value;
+		ACPI_FREE(obj);
+	} else {
+		ACPI_FREE(obj);
+		return NULL;

drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 409

Quoted 10 lines

drivers/pci/pcie/dpc.c +10 -0

+	}
+
+	return pci_get_domain_bus_and_slot(0, PCI_BUS_NUM(port), port & 0xff);
+}
+
+/*
+ * _OST wrapper function to let firmware know the status of EDR event.
+ * @dpc   : DPC device structure.
+ * @status: Status of EDR event.
+ *

Spurious blank line in the comment.

drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 422

Quoted 13 lines

drivers/pci/pcie/dpc.c +13 -0

+ */
+static int acpi_send_edr_status(struct dpc_dev *dpc,  u16 status)
+{
+	u32 ost_status;
+	struct pci_dev *pdev = dpc->dev->port;
+
+	dev_dbg(&pdev->dev, "Sending EDR status :%x\n", status);
+
+	ost_status =  PCI_DEVID(pdev->bus->number, pdev->devfn);
+	ost_status = (ost_status << 16) | status;
+
+	if (!acpi_has_method(dpc->adev->handle, "_OST"))
+		return -ENOTSUPP;

This acpi_has_method() check is superfluous, isn't it? I would expect acpi_evaluate_ost() to fail gracefully if there's no _OST.

I do see several other instances of code of the form:

   if (!acpi_has_method(handle, "XXXX"))
     return false;

   status = acpi_evaluate_*(handle, "XXXX", ...);

But I think that's a pointless pattern. I think we could just try to evaluate the method directly, since the evaluation will fail if the method doesn't exist:

   status = acpi_evaluate_*(handle, "XXXX", ...);

Quoted 50 lines

drivers/pci/pcie/dpc.c +49 -0

+	status = acpi_evaluate_ost(dpc->adev->handle,
+				   ACPI_NOTIFY_DISCONNECT_RECOVER,
+				   ost_status, NULL);
+	if (ACPI_FAILURE(status))
+		return -EINVAL;
+
+	return 0;
+}
+
+/*
+ * Helper function used for disconnecting the child devices when EDR event is
+ * received from firmware.
+ */
+static void dpc_disconnect_devices(struct pci_dev *dev)
+{
+	struct pci_dev *udev;
+	struct pci_bus *parent;
+	struct pci_dev *pdev, *temp;
+
+	dev_dbg(&dev->dev, "Disconnecting the child devices\n");
+
+	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
+		udev = dev;
+	else
+		udev = dev->bus->self;
+
+	parent = udev->subordinate;
+	pci_walk_bus(parent, pci_dev_set_disconnected, NULL);
+
+	pci_lock_rescan_remove();
+	pci_dev_get(dev);
+	list_for_each_entry_safe_reverse(pdev, temp, &parent->devices,
+					 bus_list) {
+		pci_stop_and_remove_bus_device(pdev);
+	}
+	pci_dev_put(dev);
+	pci_unlock_rescan_remove();
+}
+
+static void edr_handle_event(acpi_handle handle, u32 event, void *data)
+{
+	struct dpc_dev *dpc = (struct dpc_dev *) data;
+	struct pci_dev *pdev;
+	u16 status, cap;
+
+	if (event != ACPI_NOTIFY_DISCONNECT_RECOVER)
+		return;
+
+	if (!data) {

drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 473

Quoted 1 line

drivers/pci/pcie/dpc.c +1 -0

+		pr_err("Invalid EDR event\n");

In what instance can "data" be NULL? I think *never*, unless ACPI screwed up and lost the context you supplied to acpi_install_notify_handler(). In that case, we should do something more significant than print a message and just return. I think we should just omit this test, and if ACPI screwed up, we'll take a null pointer dereference oops, we'll see exactly where, and we'll have a chance to fix the problem.

drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 477

Quoted 5 lines

drivers/pci/pcie/dpc.c +4 -0

+		return;
+	}
+
+	dev_dbg(&dpc->dev->port->dev, "Valid EDR event received\n");

Set "pdev" at declaration and use it in these printks.

drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 483

Quoted 6 lines

drivers/pci/pcie/dpc.c +6 -0

+
+	/*
+	 * Check if _DSM(0xD) is available, and if present locate the
+	 * port which issued EDR event.
+	 */
+	pdev = acpi_locate_dpc_port(dpc);

Can this be done at probe-time instead of event-time?

drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 494

Quoted 11 lines

drivers/pci/pcie/dpc.c +11 -0

+	if (!pdev) {
+		dev_err(&dpc->dev->port->dev, "No valid port found\n");
+		return;
+	}
+
+	/*
+	 * Get DPC priv data for given pdev
+	 */
+	dpc = to_dpc_dev(pdev);
+	dpc->error_state = PCI_ERS_RESULT_DISCONNECT;
+	pdev = dpc->dev->port;

It's quite confusing to reassign pdev here. The comment about "Get DPC priv data for given pdev" is really superfluous since that much is obvious from the code. What's *not* obvious is whether this "pdev = dpc->dev->port" changes anything and if so, why.

drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 505

Quoted 12 lines

drivers/pci/pcie/dpc.c +11 -0

+	cap = dpc->cap_pos;
+
+	/*
+	 * Check if the port supports DPC:
+	 *
+	 * if port does not support DPC, then let firmware handle
+	 * the error recovery and OS is responsible for cleaning
+	 * up the child devices.
+	 *
+	 * if port supports DPC, then fall back to default error
+	 * recovery.

Capitalize first letter of sentences.

drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 512

Quoted 8 lines

drivers/pci/pcie/dpc.c +7 -0

+	 *
+	 */
+	if (cap) {
+		/* Check if there is a valid DPC trigger */
+		pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
+		if (!(status & PCI_EXP_DPC_STATUS_TRIGGER)) {
+			dev_err(&pdev->dev, "Invalid DPC trigger\n");

Include "status" value in the printk.

Quoted 78 lines

drivers/pci/pcie/dpc.c +50 -10

+			return;
+		}
+		dpc_process_error(dpc);
+	}
+
+	if (dpc->error_state == PCI_ERS_RESULT_RECOVERED) {
+		/*
+		 * Recovery is successful, so send
+		 * _OST(0xF, BDF << 16 | 0x80, "") to firmware.
+		 */
+		status = 0x80;
+	} else {
+		/*
+		 * Recovery is not successful, so disconnect child devices
+		 * and send _OST(0xF, BDF << 16 | 0x81, "") to firmware.
+		 */
+		dpc_disconnect_devices(pdev);
+		status = 0x81;
+	}
+
+	acpi_send_edr_status(dpc, status);
+}
+
+#endif
+
  #define FLAG(x, y) (((x) & (y)) ? '+' : '-')
  static int dpc_probe(struct pcie_device *dev)
  {

@@ -291,9 +543,10 @@ static int dpc_probe(struct pcie_device *dev)
  	struct device *device = &dev->device;
  	int status;
  	u16 ctl, cap;
-
-	if (pcie_aer_get_firmware_first(pdev))
-		return -ENOTSUPP;
+#ifdef CONFIG_ACPI
+	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+	acpi_status astatus;
+#endif
 
  	dpc = devm_kzalloc(device, sizeof(*dpc), GFP_KERNEL);
  	if (!dpc)

@@ -302,16 +555,53 @@ static int dpc_probe(struct pcie_device *dev)
  	dpc->cap_pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DPC);
  	dpc->dev = dev;
  	set_service_data(dev, dpc);
+	dpc->error_state = PCI_ERS_RESULT_NONE;
+
+	if (!pcie_aer_get_firmware_first(pdev))
+		if (pci_aer_available() && dpc->cap_pos)
+			dpc->native_dpc = 1;
 
-	status = devm_request_threaded_irq(device, dev->irq, dpc_irq,
-					   dpc_handler, IRQF_SHARED,
-					   "pcie-dpc", dpc);
-	if (status) {
-		dev_warn(device, "request IRQ%d failed: %d\n", dev->irq,
-			 status);
-		return status;
+	/*
+	 * If native support is not enabled and ACPI is not
+	 * enabled then return error.
+	 */
+	if (!dpc->native_dpc && !IS_ENABLED(CONFIG_APCI))
+		return -ENODEV;
+
+	if (dpc->native_dpc) {
+		status = devm_request_threaded_irq(device, dev->irq, dpc_irq,
+						   dpc_handler, IRQF_SHARED,
+						   "pcie-dpc", dpc);
+		if (status) {
+			dev_warn(device, "request IRQ%d failed: %d\n", dev->irq,
+				 status);
+			return status;
+		}
  	}
 

drivers/pci/pcie/dpc.c static int dpc_probe(struct pcie_device *dev) · line 585

Quoted 4 lines

drivers/pci/pcie/dpc.c +4 -0

+#ifdef CONFIG_ACPI
+	if (!dpc->native_dpc) {
+		if (!adev) {
+			dev_err(device, "No valid acpi device found\n");

s/acpi/ACPI/ (in comments, printk, other English text)

drivers/pci/pcie/dpc.c static int dpc_probe(struct pcie_device *dev) · line 591

Quoted 7 lines

drivers/pci/pcie/dpc.c +6 -0

+			return -ENODEV;
+		}
+
+		dpc->adev = adev;
+
+		/* Register ACPI notifier for EDR event */

"Register handler for System events, one of which is the EDR event"?

drivers/pci/pcie/dpc.c static int dpc_probe(struct pcie_device *dev) · line 598

Quoted 8 lines

drivers/pci/pcie/dpc.c +7 -0

+		astatus = acpi_install_notify_handler(adev->handle,
+						      ACPI_SYSTEM_NOTIFY,
+						      edr_handle_event,
+						      dpc);
+
+		if (ACPI_FAILURE(astatus)) {
+			dev_err(device, "Install notifier failed\n");

Mention "ACPI notify handler" here.

Quoted 51 lines

drivers/pci/pcie/dpc.c +16 -2

+			return -EBUSY;
+		}
+
+		acpi_enable_dpc_port(dpc, true);
+	}
+#endif
  	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CAP, &cap);
  	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, &ctl);
 

@@ -325,8 +615,12 @@ static int dpc_probe(struct pcie_device *dev)
  		}
  	}
 
-	ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
-	pci_write_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, ctl);
+	if (dpc->native_dpc) {
+		ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_FATAL |
+			PCI_EXP_DPC_CTL_INT_EN;
+		pci_write_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL,
+				      ctl);
+	}
 
  	dev_info(device, "DPC error containment capabilities: Int Msg #%d, RPExt%c PoisonedTLP%c SwTrigger%c RP PIO Log %d, DL_ActiveErr%c\n",
  		cap & PCI_EXP_DPC_IRQ, FLAG(cap, PCI_EXP_DPC_CAP_RP_EXT),

@@ -344,6 +638,9 @@ static void dpc_remove(struct pcie_device *dev)
  	struct pci_dev *pdev = dev->port;
  	u16 ctl;
 
+	if (!dpc->native_dpc)
+		return;
+
  	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, &ctl);
  	ctl &= ~(PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN);
  	pci_write_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, ctl);

@@ -356,6 +653,7 @@ static struct pcie_port_service_driver dpcdriver = {
  	.probe		= dpc_probe,
  	.remove		= dpc_remove,
  	.reset_link	= dpc_reset_link,
+	.error_resume   = dpc_error_resume,
  };
 
  int __init pcie_dpc_init(void)

drivers/pci/pcie/portdrv_core.c +0 -0

index 865f12f4b314..050cbb7a5083 100644
@@ -249,9 +249,14 @@ static int get_port_device_capability(struct pci_dev *dev)
  		pcie_pme_interrupt_enable(dev, false);
  	}
 

drivers/pci/pcie/portdrv_core.c static int get_port_device_capability(struct pci_dev *dev) · line 259

Quoted 10 lines

drivers/pci/pcie/portdrv_core.c +7 -2

+	/*
+	 * If EDR support is enabled in OS, then even if AER is not handled in
+	 * OS, DPC service can be enabled.
+	 */
  	if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
-	    pci_aer_available() && services & PCIE_PORT_SERVICE_AER &&
-	    (pcie_ports_native || host->native_dpc))
+	    ((IS_ENABLED(CONFIG_PCIE_EDR) && !host->native_dpc) ||
+	    (pci_aer_available() && services & PCIE_PORT_SERVICE_AER &&
+	    (pcie_ports_native || host->native_dpc))))

Holy cow, I think I'll have to schedule an hour sometime to figure out what's going on here :)

Quoted 5 lines

drivers/pci/pcie/portdrv_core.c +0 -0

  		services |= PCIE_PORT_SERVICE_DPC;
 
  	if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM || -- 2.20.1

Quoted 11 lines · Bjorn Helgaas

Quoted 10 lines · On Tue, Mar 19, 2019 at 01:47:29PM -0700…

From: Kuppuswamy Sathyanarayanan <[email protected]>

As per PCI firmware specification v3.2 ECN
(https://members.pcisig.com/wg/PCI-SIG/document/12614), when firmware owns Downstream Port Containment (DPC), its expected to use the "Error Disconnect Recover" (EDR) notification to alert OSPM of a DPC event and if OS supports EDR, its expected to handle the software state invalidation and port recovery in OS and let firmware know the recovery status via _OST ACPI call.

Quoted 6 lines

drivers/pci/pcie/dpc.c struct dpc_dev { · line 24

Quoted 4 lines

Since EDR is a hybrid service, DPC service shall be enabled in OS even if AER is not natively enabled in OS.

drivers/pci/pcie/dpc.c +1 -0

+	bool			native_dpc;

This is going to be way too confusing with a "native_dpc" in both the struct pci_host_bridge and the struct dpc_dev.

Any suggestions? what about native_mode ?

Quoted 2 lines

drivers/pci/pcie/dpc.c static int dpc_probe(struct pcie_device *dev) · line 591

Quoted 1 line

drivers/pci/pcie/dpc.c +1 -0

+		/* Register ACPI notifier for EDR event */

"Register handler for System events, one of which is the EDR event"?

In our case, we only handle EDR event. So I just explicitly mentioned it.

Quoted 4 lines

Quoted 4 lines

drivers/pci/pcie/portdrv_core.c +4 -0

+	/*
+	 * If EDR support is enabled in OS, then even if AER is not handled in
+	 * OS, DPC service can be enabled.
+	 */

Quoted 8 lines

drivers/pci/pcie/portdrv_core.c static int get_port_device_capability(struct pci_dev *dev) · line 259

Quoted 6 lines

if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&

drivers/pci/pcie/portdrv_core.c +3 -2

-	    pci_aer_available() && services & PCIE_PORT_SERVICE_AER &&
-	    (pcie_ports_native || host->native_dpc))
+	    ((IS_ENABLED(CONFIG_PCIE_EDR) && !host->native_dpc) ||
+	    (pci_aer_available() && services & PCIE_PORT_SERVICE_AER &&
+	    (pcie_ports_native || host->native_dpc))))

Holy cow, I think I'll have to schedule an hour sometime to figure out what's going on here :)

Please check the previous patch in this series for details related to host->native_dpc.