[PATCH v3 0/5] Add Error Disconnect Recover (EDR) support

[PATCH v3 0/5] Add Error Disconnect Recover (EDR) support

[sathyanarayanan.kuppuswamy]

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

This patchset adds support for following features:

1. Error Disconnect Recover (EDR) support.
2. _OSC based negotiation support for DPC.

You can find EDR spec in the following link.

https://members.pcisig.com/wg/PCI-SIG/document/12614

Changes since v1:
 * Rebased on top of v5.1-rc1

Changes since v2:
 * Split EDR support patch into multiple patches.
 * Addressed Bjorn comments.

Kuppuswamy Sathyanarayanan (5):
  PCI/ACPI: Add _OSC based negotiation support for DPC
  PCI/DPC: Allow dpc_probe() even if DPC is handled in firmware
  PCI/DPC: Add dpc_process_error() wrapper function
  PCI/DPC: Add Error Disconnect Recover (EDR) support
  PCI/ACPI: Expose EDR support via _OSC to BIOS

 drivers/acpi/pci_root.c         |   9 ++
 drivers/pci/pcie/Kconfig        |  10 ++
 drivers/pci/pcie/dpc.c          | 265 ++++++++++++++++++++++++++++++--
 drivers/pci/pcie/portdrv_core.c |   8 +-
 drivers/pci/probe.c             |   1 +
 include/linux/acpi.h            |   6 +-
 include/linux/pci.h             |   1 +
 7 files changed, 282 insertions(+), 18 deletions(-)

-- 
2.20.1

[PATCH v3 1/5] PCI/ACPI: Add _OSC based negotiation support for DPC

[sathyanarayanan.kuppuswamy]

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

As per PCI Firmware Specification, r3.2 ECN
(https://members.pcisig.com/wg/PCI-SIG/document/12614), OS can use
bit 7 of _OSC Control Field to negotiate control over Downstream Port
Containment (DPC) configuration of PCIe port.

After _OSC negotiation, firmware will Set this bit to grant OS control
over PCIe DPC configuration and Clear it if this feature was requested
and denied, or was not requested.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/acpi/pci_root.c         | 6 ++++++
 drivers/pci/pcie/portdrv_core.c | 3 ++-
 drivers/pci/probe.c             | 1 +
 include/linux/acpi.h            | 3 ++-
 include/linux/pci.h             | 1 +
 5 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 707aafc7c2aa..a60261e50b08 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -154,6 +154,7 @@ static struct pci_osc_bit_struct pci_osc_control_bit[] = {
 	{ OSC_PCI_EXPRESS_AER_CONTROL, "AER" },
 	{ OSC_PCI_EXPRESS_CAPABILITY_CONTROL, "PCIeCapability" },
 	{ OSC_PCI_EXPRESS_LTR_CONTROL, "LTR" },
+	{ OSC_PCI_EXPRESS_DPC_CONTROL, "DPC" },
 };
 
 static void decode_osc_bits(struct acpi_pci_root *root, char *msg, u32 word,
@@ -499,6 +500,9 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 			control |= OSC_PCI_EXPRESS_AER_CONTROL;
 	}
 
+	if (IS_ENABLED(CONFIG_PCIE_DPC))
+		control |= OSC_PCI_EXPRESS_DPC_CONTROL;
+
 	requested = control;
 	status = acpi_pci_osc_control_set(handle, &control,
 					  OSC_PCI_EXPRESS_CAPABILITY_CONTROL);
@@ -927,6 +931,8 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 		host_bridge->native_pme = 0;
 	if (!(root->osc_control_set & OSC_PCI_EXPRESS_LTR_CONTROL))
 		host_bridge->native_ltr = 0;
+	if (!(root->osc_control_set & OSC_PCI_EXPRESS_DPC_CONTROL))
+		host_bridge->native_dpc = 0;
 
 	pci_scan_child_bus(bus);
 	pci_set_host_bridge_release(host_bridge, acpi_pci_root_release_info,
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 1b330129089f..0a59ac574be1 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -251,7 +251,8 @@ static int get_port_device_capability(struct pci_dev *dev)
 	}
 
 	if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
-	    pci_aer_available() && services & PCIE_PORT_SERVICE_AER)
+	    pci_aer_available() && services & PCIE_PORT_SERVICE_AER &&
+	    (pcie_ports_native || host->native_dpc))
 		services |= PCIE_PORT_SERVICE_DPC;
 
 	if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 7e12d0163863..31639ef8a95b 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -608,6 +608,7 @@ struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
 	bridge->native_shpc_hotplug = 1;
 	bridge->native_pme = 1;
 	bridge->native_ltr = 1;
+	bridge->native_dpc = 1;
 
 	return bridge;
 }
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index d5dcebd7aad3..f68e9513996b 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -522,7 +522,8 @@ extern bool osc_pc_lpi_support_confirmed;
 #define OSC_PCI_EXPRESS_AER_CONTROL		0x00000008
 #define OSC_PCI_EXPRESS_CAPABILITY_CONTROL	0x00000010
 #define OSC_PCI_EXPRESS_LTR_CONTROL		0x00000020
-#define OSC_PCI_CONTROL_MASKS			0x0000003f
+#define OSC_PCI_EXPRESS_DPC_CONTROL		0x00000080
+#define OSC_PCI_CONTROL_MASKS			0x000000ff
 
 #define ACPI_GSB_ACCESS_ATTRIB_QUICK		0x00000002
 #define ACPI_GSB_ACCESS_ATTRIB_SEND_RCV         0x00000004
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 77448215ef5b..1c38bcd200d1 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -502,6 +502,7 @@ struct pci_host_bridge {
 	unsigned int	native_shpc_hotplug:1;	/* OS may use SHPC hotplug */
 	unsigned int	native_pme:1;		/* OS may use PCIe PME */
 	unsigned int	native_ltr:1;		/* OS may use PCIe LTR */
+	unsigned int	native_dpc:1;		/* OS may use PCIe DPC */
 	/* Resource alignment requirements */
 	resource_size_t (*align_resource)(struct pci_dev *dev,
 			const struct resource *res,
-- 
2.20.1

[PATCH v3 2/5] PCI/DPC: Allow dpc_probe() even if DPC is handled in firmware

[sathyanarayanan.kuppuswamy]

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

Error Disconnect Recover (EDR) support allows OS to handle the error
recovery even when DPC configuration is handled by firmware. So allow
dpc_probe() to continue even if firmware first mode is enabed. This is
a prepratory patch for adding EDR support.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/pci/pcie/dpc.c | 49 +++++++++++++++++++++++++++++++-----------
 1 file changed, 36 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 7b77754a82de..0c9ce876e450 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -20,6 +20,8 @@ struct dpc_dev {
 	u16			cap_pos;
 	bool			rp_extensions;
 	u8			rp_log_size;
+	/* Set True if DPC is handled in firmware */
+	bool			firmware_dpc;
 };
 
 static const char * const rp_pio_error_string[] = {
@@ -67,6 +69,9 @@ void pci_save_dpc_state(struct pci_dev *dev)
 	if (!dpc)
 		return;
 
+	if (dpc->firmware_dpc)
+		return;
+
 	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_DPC);
 	if (!save_state)
 		return;
@@ -88,6 +93,9 @@ void pci_restore_dpc_state(struct pci_dev *dev)
 	if (!dpc)
 		return;
 
+	if (dpc->firmware_dpc)
+		return;
+
 	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_DPC);
 	if (!save_state)
 		return;
@@ -292,9 +300,6 @@ static int dpc_probe(struct pcie_device *dev)
 	int status;
 	u16 ctl, cap;
 
-	if (pcie_aer_get_firmware_first(pdev))
-		return -ENOTSUPP;
-
 	dpc = devm_kzalloc(device, sizeof(*dpc), GFP_KERNEL);
 	if (!dpc)
 		return -ENOMEM;
@@ -303,13 +308,25 @@ static int dpc_probe(struct pcie_device *dev)
 	dpc->dev = dev;
 	set_service_data(dev, 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;
+	if (pcie_aer_get_firmware_first(pdev))
+		dpc->firmware_dpc = 1;
+
+	/*
+	 * If DPC is handled in firmware and ACPI support is not enabled in OS,
+	 * then its not useful to proceed with probe. So, return error.
+	 */
+	if (dpc->firmware_dpc && !IS_ENABLED(CONFIG_ACPI))
+		return -ENODEV;
+
+	if (!dpc->firmware_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;
+		}
 	}
 
 	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CAP, &cap);
@@ -324,9 +341,12 @@ static int dpc_probe(struct pcie_device *dev)
 			dpc->rp_log_size = 0;
 		}
 	}
-
-	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->firmware_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 +364,9 @@ static void dpc_remove(struct pcie_device *dev)
 	struct pci_dev *pdev = dev->port;
 	u16 ctl;
 
+	if (dpc->firmware_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);
-- 
2.20.1

[PATCH v3 3/5] PCI/DPC: Add dpc_process_error() wrapper function

[sathyanarayanan.kuppuswamy]

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

Add dpc_process_error() wrapper function for handling the DPC
errors when receiving DPC IRQ or Error Disconnect Recover (EDR)
event. This is a prepratory patch for adding EDR support.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/pci/pcie/dpc.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 0c9ce876e450..95810b4b0adc 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -232,10 +232,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;
@@ -269,6 +268,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;
 }
-- 
2.20.1

[PATCH v3 4/5] PCI/DPC: Add Error Disconnect Recover (EDR) support

[sathyanarayanan.kuppuswamy]

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

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 <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/pci/pcie/Kconfig        |  10 ++
 drivers/pci/pcie/dpc.c          | 206 ++++++++++++++++++++++++++++++++
 drivers/pci/pcie/portdrv_core.c |   9 +-
 3 files changed, 223 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
index 362eb8cfa53b..9a05015af7cd 100644
--- a/drivers/pci/pcie/Kconfig
+++ b/drivers/pci/pcie/Kconfig
@@ -150,3 +150,13 @@ config PCIE_BW
 	  This enables PCI Express Bandwidth Change Notification.  If
 	  you know link width or rate changes occur only to correct
 	  unreliable links, you may answer Y.
+
+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.
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 95810b4b0adc..131fadd29ae2 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -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"
@@ -22,8 +23,22 @@ struct dpc_dev {
 	u8			rp_log_size;
 	/* Set True if DPC is handled in firmware */
 	bool			firmware_dpc;
+	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  */
@@ -297,6 +312,167 @@ static irqreturn_t dpc_irq(int irq, void *context)
 	return IRQ_HANDLED;
 }
 
+static void dpc_error_resume(struct pci_dev *dev)
+{
+	struct dpc_dev *dpc = to_dpc_dev(dev);
+
+	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;
+	union acpi_object argv4;
+	struct pci_dev *pdev = dpc->dev->port;
+
+	dev_dbg(&pdev->dev, "Enable DPC port, value:%d\n", enable);
+
+	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.
+ */
+static struct pci_dev *acpi_locate_dpc_port(struct dpc_dev *dpc)
+{
+	union acpi_object *obj;
+	u16 port;
+	struct pci_dev *pdev = dpc->dev->port;
+
+	dev_dbg(&pdev->dev, "Locate DPC port\n");
+
+	obj = acpi_evaluate_dsm(dpc->adev->handle, &pci_acpi_dsm_guid, 1,
+				EDR_PORT_LOCATE_DSM, NULL);
+	if (!obj)
+		return dpc->dev->port;
+
+	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 dpc->dev->port;
+	}
+
+	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.
+ */
+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;
+
+	status = acpi_evaluate_ost(dpc->adev->handle,
+				   ACPI_NOTIFY_DISCONNECT_RECOVER,
+				   ost_status, NULL);
+	if (ACPI_FAILURE(status))
+		return -EINVAL;
+
+	return 0;
+}
+
+static void edr_handle_event(acpi_handle handle, u32 event, void *data)
+{
+	struct dpc_dev *dpc = (struct dpc_dev *) data;
+	struct pci_dev *pdev = dpc->dev->port;
+	u16 status, cap;
+
+	dev_info(&pdev->dev, "ACPI event %x received\n", event);
+
+	if (event != ACPI_NOTIFY_DISCONNECT_RECOVER)
+		return;
+
+	dev_info(&pdev->dev, "Valid EDR event received\n");
+
+	/*
+	 * Check if _DSM(0xD) is available, and if present locate the
+	 * port which issued EDR event.
+	 */
+	pdev = acpi_locate_dpc_port(dpc);
+	if (!pdev) {
+		pdev = dpc->dev->port;
+		dev_err(&pdev->dev, "No valid port found\n");
+		return;
+	}
+
+	dpc = to_dpc_dev(pdev);
+	dpc->error_state = PCI_ERS_RESULT_DISCONNECT;
+	cap = dpc->cap_pos;
+
+	/*
+	 * Check if the port supports DPC:
+	 *
+	 * If port supports DPC, then fall back to default error
+	 * recovery.
+	 */
+	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 %x\n", status);
+			return;
+		}
+		dpc_process_error(dpc);
+	}
+
+	/*
+	 * If recovery is successful, send _OST(0xF, BDF << 16 | 0x80)
+	 * to firmware. If not successful, send _OST(0xF, BDF << 16 | 0x81).
+	 */
+	if (dpc->error_state == PCI_ERS_RESULT_RECOVERED)
+		status = 0x80;
+	else
+		status = 0x81;
+
+	acpi_send_edr_status(dpc, status);
+}
+
+#endif
+
 #define FLAG(x, y) (((x) & (y)) ? '+' : '-')
 static int dpc_probe(struct pcie_device *dev)
 {
@@ -305,6 +481,10 @@ static int dpc_probe(struct pcie_device *dev)
 	struct device *device = &dev->device;
 	int status;
 	u16 ctl, cap;
+#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)
@@ -313,6 +493,7 @@ 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))
 		dpc->firmware_dpc = 1;
@@ -335,6 +516,30 @@ static int dpc_probe(struct pcie_device *dev)
 		}
 	}
 
+#ifdef CONFIG_ACPI
+	if (dpc->firmware_dpc) {
+		if (!adev) {
+			dev_err(device, "No valid ACPI device found\n");
+			return -ENODEV;
+		}
+
+		dpc->adev = adev;
+
+		/* Register notifier for ACPI_SYSTEM_NOTIFY events */
+		astatus = acpi_install_notify_handler(adev->handle,
+						      ACPI_SYSTEM_NOTIFY,
+						      edr_handle_event,
+						      dpc);
+
+		if (ACPI_FAILURE(astatus)) {
+			dev_err(device,
+				"Install ACPI_SYSTEM_NOTIFY handler failed\n");
+			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);
 
@@ -385,6 +590,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)
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 0a59ac574be1..1b54a39df795 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -250,9 +250,14 @@ static int get_port_device_capability(struct pci_dev *dev)
 		pcie_pme_interrupt_enable(dev, false);
 	}
 
+	/*
+	 * 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))))
 		services |= PCIE_PORT_SERVICE_DPC;
 
 	if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
-- 
2.20.1

[PATCH v3 5/5] PCI/ACPI: Expose EDR support via _OSC to BIOS

[sathyanarayanan.kuppuswamy]

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

As per PCI Firmware Specification, r3.2 ECN
(https://members.pcisig.com/wg/PCI-SIG/document/12614), if OS supports
EDR, it should expose its support to BIOS by setting bit 7 of _OSC
Support Field.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/acpi/pci_root.c | 3 +++
 include/linux/acpi.h    | 3 ++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index a60261e50b08..aeb3fcc40e2f 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -145,6 +145,7 @@ static struct pci_osc_bit_struct pci_osc_support_bit[] = {
 	{ OSC_PCI_CLOCK_PM_SUPPORT, "ClockPM" },
 	{ OSC_PCI_SEGMENT_GROUPS_SUPPORT, "Segments" },
 	{ OSC_PCI_MSI_SUPPORT, "MSI" },
+	{ OSC_PCI_EDR_SUPPORT, "EDR" },
 };
 
 static struct pci_osc_bit_struct pci_osc_control_bit[] = {
@@ -453,6 +454,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 		support |= OSC_PCI_ASPM_SUPPORT | OSC_PCI_CLOCK_PM_SUPPORT;
 	if (pci_msi_enabled())
 		support |= OSC_PCI_MSI_SUPPORT;
+	if (IS_ENABLED(CONFIG_PCIE_EDR))
+		support |= OSC_PCI_EDR_SUPPORT;
 
 	decode_osc_support(root, "OS supports", support);
 	status = acpi_pci_osc_support(root, support);
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index f68e9513996b..c60b7d72e526 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -513,7 +513,8 @@ extern bool osc_pc_lpi_support_confirmed;
 #define OSC_PCI_CLOCK_PM_SUPPORT		0x00000004
 #define OSC_PCI_SEGMENT_GROUPS_SUPPORT		0x00000008
 #define OSC_PCI_MSI_SUPPORT			0x00000010
-#define OSC_PCI_SUPPORT_MASKS			0x0000001f
+#define OSC_PCI_EDR_SUPPORT			0x00000080
+#define OSC_PCI_SUPPORT_MASKS			0x000000ff
 
 /* PCI Host Bridge _OSC: Capabilities DWORD 3: Control Field */
 #define OSC_PCI_EXPRESS_NATIVE_HP_CONTROL	0x00000001
-- 
2.20.1

Re: [PATCH v3 1/5] PCI/ACPI: Add _OSC based negotiation support for DPC

[Bjorn Helgaas]

On Tue, May 14, 2019 at 03:18:13PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
> As per PCI Firmware Specification, r3.2 ECN
> (https://members.pcisig.com/wg/PCI-SIG/document/12614), OS can use
> bit 7 of _OSC Control Field to negotiate control over Downstream Port
> Containment (DPC) configuration of PCIe port.

I think this citation should reference the "Downstream Port
Containment Related Enhancements ECN", i.e., it should include the
title of the ECN.  I first thought you were citing the PCI Firmware
Spec, r3.2, but of course it's not in there.

Bjorn

Re: [PATCH v3 2/5] PCI/DPC: Allow dpc_probe() even if DPC is handled in firmware

[Bjorn Helgaas]

On Tue, May 14, 2019 at 03:18:14PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
> Error Disconnect Recover (EDR) support allows OS to handle the error
> recovery even when DPC configuration is handled by firmware. So allow
> dpc_probe() to continue even if firmware first mode is enabed. This is
> a prepratory patch for adding EDR support.

I assume this code is based on some language in the spec, and it needs
to be tied back to it.  The only mention of EDR I'm aware of is in
ACPI v6.3, sec 5.6.6 and 6.3.5.2, so it needs at least a citation.

I don't see any specific reference to firmware-first there, so please
also connect the dots about how we conclude that we need DPC even when
firmware-first is enabled.

> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  drivers/pci/pcie/dpc.c | 49 +++++++++++++++++++++++++++++++-----------
>  1 file changed, 36 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index 7b77754a82de..0c9ce876e450 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -20,6 +20,8 @@ struct dpc_dev {
>  	u16			cap_pos;
>  	bool			rp_extensions;
>  	u8			rp_log_size;
> +	/* Set True if DPC is handled in firmware */
> +	bool			firmware_dpc;
>  };
>  
>  static const char * const rp_pio_error_string[] = {
> @@ -67,6 +69,9 @@ void pci_save_dpc_state(struct pci_dev *dev)
>  	if (!dpc)
>  		return;
>  
> +	if (dpc->firmware_dpc)
> +		return;
> +
>  	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_DPC);
>  	if (!save_state)
>  		return;
> @@ -88,6 +93,9 @@ void pci_restore_dpc_state(struct pci_dev *dev)
>  	if (!dpc)
>  		return;
>  
> +	if (dpc->firmware_dpc)
> +		return;
> +
>  	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_DPC);
>  	if (!save_state)
>  		return;
> @@ -292,9 +300,6 @@ static int dpc_probe(struct pcie_device *dev)
>  	int status;
>  	u16 ctl, cap;
>  
> -	if (pcie_aer_get_firmware_first(pdev))
> -		return -ENOTSUPP;
> -
>  	dpc = devm_kzalloc(device, sizeof(*dpc), GFP_KERNEL);
>  	if (!dpc)
>  		return -ENOMEM;
> @@ -303,13 +308,25 @@ static int dpc_probe(struct pcie_device *dev)
>  	dpc->dev = dev;
>  	set_service_data(dev, 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;
> +	if (pcie_aer_get_firmware_first(pdev))
> +		dpc->firmware_dpc = 1;
> +
> +	/*
> +	 * If DPC is handled in firmware and ACPI support is not enabled in OS,
> +	 * then its not useful to proceed with probe. So, return error.
> +	 */
> +	if (dpc->firmware_dpc && !IS_ENABLED(CONFIG_ACPI))
> +		return -ENODEV;
> +
> +	if (!dpc->firmware_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;
> +		}
>  	}
>  
>  	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CAP, &cap);
> @@ -324,9 +341,12 @@ static int dpc_probe(struct pcie_device *dev)
>  			dpc->rp_log_size = 0;
>  		}
>  	}
> -
> -	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->firmware_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 +364,9 @@ static void dpc_remove(struct pcie_device *dev)
>  	struct pci_dev *pdev = dev->port;
>  	u16 ctl;
>  
> +	if (dpc->firmware_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);
> -- 
> 2.20.1
> 

Re: [PATCH v3 4/5] PCI/DPC: Add Error Disconnect Recover (EDR) support

[Bjorn Helgaas]

On Tue, May 14, 2019 at 03:18:16PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
> 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.

Please also include the ECN title and the ACPI reference(s) related to
EDR.

> 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 <sathyanarayanan.kuppuswamy@linux.intel.com>

I'm looking for an ack from Keith eventually.

> ---
>  drivers/pci/pcie/Kconfig        |  10 ++
>  drivers/pci/pcie/dpc.c          | 206 ++++++++++++++++++++++++++++++++
>  drivers/pci/pcie/portdrv_core.c |   9 +-
>  3 files changed, 223 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
> index 362eb8cfa53b..9a05015af7cd 100644
> --- a/drivers/pci/pcie/Kconfig
> +++ b/drivers/pci/pcie/Kconfig
> @@ -150,3 +150,13 @@ config PCIE_BW
>  	  This enables PCI Express Bandwidth Change Notification.  If
>  	  you know link width or rate changes occur only to correct
>  	  unreliable links, you may answer Y.
> +
> +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.
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index 95810b4b0adc..131fadd29ae2 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -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"
> @@ -22,8 +23,22 @@ struct dpc_dev {
>  	u8			rp_log_size;
>  	/* Set True if DPC is handled in firmware */
>  	bool			firmware_dpc;
> +	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);

This is a duplicate of the global pci_acpi_dsm_guid in
drivers/pci/pci-acpi.c.  You should be able to at least use that
global definition if you include linux/pci-acpi.h.  There might be an
argument for moving the _DSM evaluation to pci/pci-acpi.c.  We do
already have a use in pci/pci-label.c, so not *everything* is in
pci/pci-acpi.c anyway.

But I think it would be nice to have the EDR_PORT_ENABLE_DSM
definitions in pci-acpi.h alongside the existing DEVICE_LABEL_DSM,
RESET_DELAY_DSM, etc.

> +#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  */
> @@ -297,6 +312,167 @@ static irqreturn_t dpc_irq(int irq, void *context)
>  	return IRQ_HANDLED;
>  }
>  
> +static void dpc_error_resume(struct pci_dev *dev)
> +{
> +	struct dpc_dev *dpc = to_dpc_dev(dev);
> +
> +	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.

You don't check the return value, so why bother returning an
indication?  Just make it void so it's obvious that we don't care.

> + */
> +static int acpi_enable_dpc_port(struct dpc_dev *dpc, bool enable)
> +{
> +	union acpi_object *obj;
> +	int status;
> +	union acpi_object argv4;
> +	struct pci_dev *pdev = dpc->dev->port;
> +
> +	dev_dbg(&pdev->dev, "Enable DPC port, value:%d\n", enable);

Use pci_dbg(pdev) here and elsewhere.

Actually, what I think would be the *best* would be drop this message
and include an indication in the log message from dpc_probe() about
DPC/EDR being enabled so we can tell from the dmesg log what model is
being used.  That would probably require keeping the return value and
paying attention to it in dpc_probe().

> +	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.
> + */
> +static struct pci_dev *acpi_locate_dpc_port(struct dpc_dev *dpc)
> +{
> +	union acpi_object *obj;
> +	u16 port;
> +	struct pci_dev *pdev = dpc->dev->port;
> +
> +	dev_dbg(&pdev->dev, "Locate DPC port\n");

I think this message is pointless.

> +	obj = acpi_evaluate_dsm(dpc->adev->handle, &pci_acpi_dsm_guid, 1,
> +				EDR_PORT_LOCATE_DSM, NULL);
> +	if (!obj)
> +		return dpc->dev->port;
> +
> +	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 dpc->dev->port;

This is an error case (firmware didn't implement the _DSM correctly).
Structure it like this so it *looks* like an error:

  if (obj->type != ACPI_TYPE_INTEGER) {
    ACPI_FREE(obj);
    return dpc->dev->port;
  }

  port = obj->integer.value;
  ACPI_FREE(obj);
  ...

Maybe the !ACPI_TYPE_INTEGER case should even return NULL?  If
firmware didn't implement the _DSM correctly, why should we assume
dpc->dev->port is the correct device?

> +	}
> +
> +	return pci_get_domain_bus_and_slot(0, PCI_BUS_NUM(port), port & 0xff);

The hard-coded domain==0 looks wrong.  Seems like it should be
something like pci_domain_nr(pdev->bus).

Using pci_get_domain_bus_and_slot() requires a matching pci_dev_put()
somewhere.

> +}
> +
> +/*
> + * _OST wrapper function to let firmware know the status of EDR event.
> + * @dpc   : DPC device structure.
> + * @status: Status of EDR event.
> + */
> +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;
> +
> +	status = acpi_evaluate_ost(dpc->adev->handle,
> +				   ACPI_NOTIFY_DISCONNECT_RECOVER,
> +				   ost_status, NULL);
> +	if (ACPI_FAILURE(status))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static void edr_handle_event(acpi_handle handle, u32 event, void *data)
> +{
> +	struct dpc_dev *dpc = (struct dpc_dev *) data;
> +	struct pci_dev *pdev = dpc->dev->port;
> +	u16 status, cap;
> +
> +	dev_info(&pdev->dev, "ACPI event %x received\n", event);
> +
> +	if (event != ACPI_NOTIFY_DISCONNECT_RECOVER)
> +		return;
> +
> +	dev_info(&pdev->dev, "Valid EDR event received\n");

These two dev_info()s should be combined; I don't think we really need
both.  (And convert to pci_info(), of course.)

> +	/*
> +	 * Check if _DSM(0xD) is available, and if present locate the
> +	 * port which issued EDR event.
> +	 */
> +	pdev = acpi_locate_dpc_port(dpc);
> +	if (!pdev) {
> +		pdev = dpc->dev->port;
> +		dev_err(&pdev->dev, "No valid port found\n");
> +		return;
> +	}
> +
> +	dpc = to_dpc_dev(pdev);

Ugh.  I hate the spaghetti inside to_dpc_dev() (which has nothing to
do with these patches).  In any event, "dpc" may be NULL here for a
variety of reasons.

> +	dpc->error_state = PCI_ERS_RESULT_DISCONNECT;
> +	cap = dpc->cap_pos;
> +
> +	/*
> +	 * Check if the port supports DPC:
> +	 *
> +	 * If port supports DPC, then fall back to default error
> +	 * recovery.
> +	 */
> +	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 %x\n", status);
> +			return;
> +		}

Why is this check of PCI_EXP_DPC_STATUS_TRIGGER not part of
dpc_process_error()?  Seems strange that we read PCI_EXP_DPC_STATUS
twice (here and then again in dpc_process_error()) and that we handle
it a little differently here than in the non-EDR path.

> +		dpc_process_error(dpc);
> +	}
> +
> +	/*
> +	 * If recovery is successful, send _OST(0xF, BDF << 16 | 0x80)
> +	 * to firmware. If not successful, send _OST(0xF, BDF << 16 | 0x81).
> +	 */
> +	if (dpc->error_state == PCI_ERS_RESULT_RECOVERED)
> +		status = 0x80;
> +	else
> +		status = 0x81;
> +
> +	acpi_send_edr_status(dpc, status);
> +}
> +
> +#endif
> +
>  #define FLAG(x, y) (((x) & (y)) ? '+' : '-')
>  static int dpc_probe(struct pcie_device *dev)
>  {
> @@ -305,6 +481,10 @@ static int dpc_probe(struct pcie_device *dev)
>  	struct device *device = &dev->device;
>  	int status;
>  	u16 ctl, cap;
> +#ifdef CONFIG_ACPI
> +	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
> +	acpi_status astatus;
> +#endif

If you move these declarations inside the "if (dpc->firmware_dpc)"
block before, you won't need this #ifdef.
>  
>  	dpc = devm_kzalloc(device, sizeof(*dpc), GFP_KERNEL);
>  	if (!dpc)
> @@ -313,6 +493,7 @@ 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))
>  		dpc->firmware_dpc = 1;
> @@ -335,6 +516,30 @@ static int dpc_probe(struct pcie_device *dev)
>  		}
>  	}
>  
> +#ifdef CONFIG_ACPI
> +	if (dpc->firmware_dpc) {
> +		if (!adev) {
> +			dev_err(device, "No valid ACPI device found\n");

This and similar messages should use pci_err() with the pci_dev (not
the device from the pcie_device).  This is to follow 10a9990c1044
("PCI/DPC: Log messages with pci_dev, not pcie_device"), which I think
happened after you first posted these patches.

> +			return -ENODEV;
> +		}
> +
> +		dpc->adev = adev;
> +
> +		/* Register notifier for ACPI_SYSTEM_NOTIFY events */

Superfluous comment, since that's exactly what the line below says.

> +		astatus = acpi_install_notify_handler(adev->handle,
> +						      ACPI_SYSTEM_NOTIFY,
> +						      edr_handle_event,
> +						      dpc);
> +
> +		if (ACPI_FAILURE(astatus)) {
> +			dev_err(device,
> +				"Install ACPI_SYSTEM_NOTIFY handler failed\n");
> +			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);
>  
> @@ -385,6 +590,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)
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index 0a59ac574be1..1b54a39df795 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -250,9 +250,14 @@ static int get_port_device_capability(struct pci_dev *dev)
>  		pcie_pme_interrupt_enable(dev, false);
>  	}
>  
> +	/*
> +	 * 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))))
>  		services |= PCIE_PORT_SERVICE_DPC;
>  
>  	if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
> -- 
> 2.20.1
> 

Re: [PATCH v3 0/5] Add Error Disconnect Recover (EDR) support

[Bjorn Helgaas]

On Tue, May 14, 2019 at 03:18:12PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
> This patchset adds support for following features:
> 
> 1. Error Disconnect Recover (EDR) support.
> 2. _OSC based negotiation support for DPC.
> 
> You can find EDR spec in the following link.
> 
> https://members.pcisig.com/wg/PCI-SIG/document/12614
> 
> Changes since v1:
>  * Rebased on top of v5.1-rc1

I'm sure you'll do this anyway, but please rebase the v4 on my pci/master
branch (v5.2-rc1).