[PATCH v13 0/8] Add Error Disconnect Recover (EDR) support

[PATCH v13 0/8] 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 v12:
 * Addressed Bjorns comments.
 * Added check for CONFIG_PCIE_EDR before requesting DPC control from firmware.
 * Removed ff_check parameter from AER APIs.
 * Used macros for _OST return status values in DPC driver.

Changes since v11:
 * Allowed error recovery to proceed after successful reset_link().
 * Used correct ACPI handle for sending EDR status.
 * Rebased on top of v5.5-rc5

Changes since v10:
 * Added "edr_enabled" member to dpc priv structure, which is used to cache EDR
   enabling status based on status of pcie_ports_dpc_native and FF mode.
 * Changed type of _DSM argument from Integer to Package in acpi_enable_dpc_port()
   function to fix ACPI related boot warnings.
 * Rebased on top of v5.5-rc3

Changes since v9:
 * Removed caching of pcie_aer_get_firmware_first() in dpc driver.
 * Added proper spec reference in git log for patch 5 & 7.
 * Added new function parameter "ff_check" to pci_cleanup_aer_uncorrect_error_status(),
   pci_aer_clear_fatal_status() and pci_cleanup_aer_error_status_regs() functions.
 * Rebased on top of v5.4-rc5

Changes since v8:
 * Rebased on top of v5.4-rc1

Changes since v7:
 * Updated DSM version number to match the spec.

Changes since v6:
 * Modified the order of patches to enable EDR only after all necessary support is added in kernel.
 * Addressed Bjorn comments.

Changes since v5:
 * Addressed Keith's comments.
 * Added additional check for FF mode in pci_aer_init().
 * Updated commit history of "PCI/DPC: Add support for DPC recovery on NON_FATAL errors" patch.

Changes since v4:
 * Rebased on top of v5.3-rc1
 * Fixed lock/unlock issue in edr_handle_event().
 * Merged "Update error status after reset_link()" patch into this patchset.

Changes since v3:
 * Moved EDR related ACPI functions/definitions to pci-acpi.c
 * Modified commit history in few patches to include spec reference.
 * Added support to handle DPC triggered by NON_FATAL errors.
 * Added edr_lock to protect PCI device receiving duplicate EDR notifications.
 * Addressed Bjorn comments.

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

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

Kuppuswamy Sathyanarayanan (8):
  PCI/ERR: Update error status after reset_link()
  PCI/DPC: Allow dpc_probe() even if firmware first mode is enabled
  PCI/DPC: Add dpc_process_error() wrapper function
  PCI/DPC: Add Error Disconnect Recover (EDR) support
  PCI/AER: Allow clearing Error Status Register in FF mode
  PCI/DPC: Update comments related to DPC recovery on NON_FATAL errors
  PCI/DPC: Clear AER registers in EDR mode
  PCI/ACPI: Enable EDR support

 drivers/acpi/pci_root.c         |  16 +++
 drivers/pci/pci-acpi.c          |  99 ++++++++++++++
 drivers/pci/pci.h               |   3 +
 drivers/pci/pcie/Kconfig        |  10 ++
 drivers/pci/pcie/aer.c          |  41 ++++--
 drivers/pci/pcie/dpc.c          | 221 +++++++++++++++++++++++++++++---
 drivers/pci/pcie/err.c          |   8 +-
 drivers/pci/pcie/portdrv_core.c |   7 +-
 drivers/pci/probe.c             |   1 +
 include/linux/acpi.h            |   6 +-
 include/linux/pci-acpi.h        |  13 ++
 include/linux/pci.h             |   1 +
 12 files changed, 388 insertions(+), 38 deletions(-)

-- 
2.21.0

[PATCH v13 1/8] PCI/ERR: Update error status after reset_link()

[sathyanarayanan.kuppuswamy]

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

Commit bdb5ac85777d ("PCI/ERR: Handle fatal error recovery") uses
reset_link() to recover from fatal errors. But during fatal error
recovery, if the initial value of error status is
PCI_ERS_RESULT_DISCONNECT or PCI_ERS_RESULT_NO_AER_DRIVER then
even after successful recovery (using reset_link()) pcie_do_recovery()
will report the recovery result as failure. So update the status of
error after reset_link().

Fixes: bdb5ac85777d ("PCI/ERR: Handle fatal error recovery")
Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Keith Busch <keith.busch@intel.com>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Acked-by: Keith Busch <keith.busch@intel.com>
---
 drivers/pci/pcie/err.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index b0e6048a9208..71639055511e 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -204,9 +204,11 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
 	else
 		pci_walk_bus(bus, report_normal_detected, &status);
 
-	if (state == pci_channel_io_frozen &&
-	    reset_link(dev, service) != PCI_ERS_RESULT_RECOVERED)
-		goto failed;
+	if (state == pci_channel_io_frozen) {
+		status = reset_link(dev, service);
+		if (status != PCI_ERS_RESULT_RECOVERED)
+			goto failed;
+	}
 
 	if (status == PCI_ERS_RESULT_CAN_RECOVER) {
 		status = PCI_ERS_RESULT_RECOVERED;
-- 
2.21.0

[PATCH v13 2/8] PCI/DPC: Allow dpc_probe() even if firmware first mode is enabled

[sathyanarayanan.kuppuswamy]

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

As per ACPI specification v6.3, sec 5.6.6, Error Disconnect Recover
(EDR) notification used by firmware to let OS know about the DPC event
and permit OS to perform error recovery when processing the EDR
notification. Also, as per PCI firmware specification r3.2 Downstream
Port Containment Related Enhancements ECN, sec 4.5.1, table 4-6, if DPC
is controlled by firmware (firmware first mode), it's responsible for
initializing Downstream Port Containment Extended Capability Structures
per firmware policy. And, OS is permitted to read or write DPC Control
and Status registers of a port while processing an Error Disconnect
Recover (EDR) notification from firmware on that port.

Currently, if firmware controls DPC (firmware first mode), OS will not
create/enumerate DPC PCIe port services. But, if OS supports EDR
feature, then as mentioned in above spec references, it should permit
enumeration of DPC driver and also support handling ACPI EDR
notification. So as first step, allow dpc_probe() to continue even if
firmware first mode is enabled. Also add appropriate checks to ensure
device registers are not modified outside EDR notification window in
firmware first mode. This is a preparatory patch for adding EDR support.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Acked-by: Keith Busch <keith.busch@intel.com>
---
 drivers/pci/pcie/dpc.c | 74 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 61 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index e06f42f58d3d..c583a90fa90d 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -22,6 +22,7 @@ struct dpc_dev {
 	u16			cap_pos;
 	bool			rp_extensions;
 	u8			rp_log_size;
+	bool			edr_enabled; /* EDR mode is supported */
 };
 
 static const char * const rp_pio_error_string[] = {
@@ -69,6 +70,14 @@ void pci_save_dpc_state(struct pci_dev *dev)
 	if (!dpc)
 		return;
 
+	/*
+	 * If DPC is controlled by firmware then save/restore tasks are also
+	 * controlled by firmware. So skip rest of the function if DPC is
+	 * controlled by firmware.
+	 */
+	if (dpc->edr_enabled)
+		return;
+
 	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_DPC);
 	if (!save_state)
 		return;
@@ -90,6 +99,14 @@ void pci_restore_dpc_state(struct pci_dev *dev)
 	if (!dpc)
 		return;
 
+	/*
+	 * If DPC is controlled by firmware then save/restore tasks are also
+	 * controlled by firmware. So skip rest of the function if DPC is
+	 * controlled by firmware.
+	 */
+	if (dpc->edr_enabled)
+		return;
+
 	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_DPC);
 	if (!save_state)
 		return;
@@ -291,24 +308,48 @@ static int dpc_probe(struct pcie_device *dev)
 	int status;
 	u16 ctl, cap;
 
-	if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native)
-		return -ENOTSUPP;
-
 	dpc = devm_kzalloc(device, sizeof(*dpc), GFP_KERNEL);
 	if (!dpc)
 		return -ENOMEM;
 
 	dpc->cap_pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DPC);
 	dpc->dev = dev;
+
+	/*
+	 * As per PCIe r5.0, sec 6.2.10, implementation note titled
+	 * "Determination of DPC Control", to avoid conflicts over whether
+	 * platform firmware or the operating system have control of DPC,
+	 * it is recommended that platform firmware and operating systems
+	 * always link the control of DPC to the control of Advanced Error
+	 * Reporting.
+	 *
+	 * So use AER FF mode check API pcie_aer_get_firmware_first() to decide
+	 * whether DPC is controlled by software or firmware. And EDR support
+	 * can only be enabled if DPC is controlled by firmware.
+	 */
+
+	if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native)
+		dpc->edr_enabled = 1;
+
+	/*
+	 * If DPC is handled in firmware and ACPI support is not enabled
+	 * in OS, skip probe and return error.
+	 */
+	if (dpc->edr_enabled && !IS_ENABLED(CONFIG_ACPI))
+		return -ENODEV;
+
 	set_service_data(dev, dpc);
 
-	status = devm_request_threaded_irq(device, dev->irq, dpc_irq,
-					   dpc_handler, IRQF_SHARED,
-					   "pcie-dpc", dpc);
-	if (status) {
-		pci_warn(pdev, "request IRQ%d failed: %d\n", dev->irq,
-			 status);
-		return status;
+	/* Register interrupt handler only if OS controls DPC */
+	if (!dpc->edr_enabled) {
+		status = devm_request_threaded_irq(device, dev->irq, dpc_irq,
+						   dpc_handler, IRQF_SHARED,
+						   "pcie-dpc", dpc);
+		if (status) {
+			pci_warn(pdev, "request IRQ%d failed: %d\n", dev->irq,
+				 status);
+			return status;
+		}
 	}
 
 	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CAP, &cap);
@@ -323,9 +364,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->edr_enabled) {
+		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);
+	}
 
 	pci_info(pdev, "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),
@@ -343,6 +387,10 @@ static void dpc_remove(struct pcie_device *dev)
 	struct pci_dev *pdev = dev->port;
 	u16 ctl;
 
+	/* Skip updating DPC registers if DPC is controlled by firmware */
+	if (dpc->edr_enabled)
+		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.21.0

[PATCH v13 3/8] PCI/DPC: Add dpc_process_error() wrapper function

[sathyanarayanan.kuppuswamy]

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

With Error Disconnect Recover (EDR) support, we need to support
processing DPC event either from DPC IRQ or ACPI EDR event. So create
a wrapper function dpc_process_error() and move common error handling
code in to it. It will be used to process the DPC event in both DPC IRQ
and EDR ACPI event contexts.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Acked-by: Keith Busch <keith.busch@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 c583a90fa90d..9f58e91f8852 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -241,10 +241,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;
 	u16 cap = dpc->cap_pos, status, source, reason, ext_reason;
 
@@ -277,6 +276,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.21.0

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

[sathyanarayanan.kuppuswamy]

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

As per ACPI specification r6.3, sec 5.6.6, 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 also let firmware know the recovery status via
_OST ACPI call. Related _OST status codes can be found in ACPI
specification r6.3, sec 6.3.5.2.

Also, as per PCI firmware specification r3.2 Downstream Port Containment
Related Enhancements ECN, sec 4.5.1, table 4-6, If DPC is controlled by
firmware (firmware first mode), firmware is responsible for
configuring the DPC and OS is responsible for error recovery. Also, OS
is allowed to modify DPC registers only during the EDR notification
window. So with EDR support, OS should provide DPC port services even in
firmware first mode.

Cc: Keith Busch <keith.busch@intel.com>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Acked-by: Keith Busch <keith.busch@intel.com>
Tested-by: Huong Nguyen <huong.nguyen@dell.com>
Tested-by: Austin Bolen <Austin.Bolen@dell.com>
---
 drivers/pci/pci-acpi.c   |  99 +++++++++++++++++++++++++++++++
 drivers/pci/pcie/Kconfig |  10 ++++
 drivers/pci/pcie/dpc.c   | 123 ++++++++++++++++++++++++++++++++++++++-
 include/linux/pci-acpi.h |  13 +++++
 4 files changed, 244 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 0c02d500158f..920928f0d55c 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -103,6 +103,105 @@ int acpi_get_rc_resources(struct device *dev, const char *hid, u16 segment,
 }
 #endif
 
+#if defined(CONFIG_PCIE_DPC)
+
+/*
+ * _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.
+ */
+int acpi_enable_dpc_port(struct pci_dev *pdev, acpi_handle handle, bool enable)
+{
+	union acpi_object *obj, argv4, req;
+	int status = 0;
+
+	req.type = ACPI_TYPE_INTEGER;
+	req.integer.value = enable;
+
+	argv4.type = ACPI_TYPE_PACKAGE;
+	argv4.package.count = 1;
+	argv4.package.elements = &req;
+
+	/*
+	 * Per the Downstream Port Containment Related Enhancements ECN to
+	 * the PCI Firmware Specification r3.2, sec 4.6.12,
+	 * EDR_PORT_ENABLE_DSM is optional.  Return success if it's not
+	 * implemented.
+	 */
+	obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, 5,
+				EDR_PORT_ENABLE_DSM, &argv4);
+	if (!obj)
+		return 0;
+
+	if (obj->type != ACPI_TYPE_INTEGER || obj->integer.value != enable)
+		status = -EIO;
+
+	ACPI_FREE(obj);
+
+	return status;
+}
+
+/*
+ * _DSM wrapper function to locate DPC port.
+ * @dpc   : DPC device structure
+ *
+ * returns pci_dev or NULL.
+ */
+struct pci_dev *acpi_locate_dpc_port(struct pci_dev *pdev, acpi_handle handle)
+{
+	union acpi_object *obj;
+	u16 port;
+
+	obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, 5,
+				EDR_PORT_LOCATE_DSM, NULL);
+	if (!obj)
+		return pci_dev_get(pdev);
+
+	if (obj->type != ACPI_TYPE_INTEGER) {
+		ACPI_FREE(obj);
+		return NULL;
+	}
+
+	/*
+	 * 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);
+
+	return pci_get_domain_bus_and_slot(pci_domain_nr(pdev->bus),
+					   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.
+ */
+int acpi_send_edr_status(struct pci_dev *pdev, acpi_handle handle, u16 status)
+{
+	u32 ost_status;
+
+	pci_dbg(pdev, "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(handle,
+				   ACPI_NOTIFY_DISCONNECT_RECOVER,
+				   ost_status, NULL);
+	if (ACPI_FAILURE(status))
+		return -EINVAL;
+
+	return 0;
+}
+#endif /* CONFIG_PCIE_DPC */
+
 phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle)
 {
 	acpi_status status = AE_NOT_EXIST;
diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
index 6e3c04b46fb1..772b1f4cb19e 100644
--- a/drivers/pci/pcie/Kconfig
+++ b/drivers/pci/pcie/Kconfig
@@ -140,3 +140,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"
+	depends on PCIE_DPC && ACPI
+	help
+	  This option adds Error Disconnect Recover support as specified
+	  in the Downstream Port Containment Related Enhancements ECN to
+	  the PCI Firmware Specification r3.2.  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 9f58e91f8852..8a8ee374d9b1 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -13,6 +13,8 @@
 #include <linux/interrupt.h>
 #include <linux/init.h>
 #include <linux/pci.h>
+#include <linux/acpi.h>
+#include <linux/pci-acpi.h>
 
 #include "portdrv.h"
 #include "../pci.h"
@@ -23,6 +25,11 @@ struct dpc_dev {
 	bool			rp_extensions;
 	u8			rp_log_size;
 	bool			edr_enabled; /* EDR mode is supported */
+	pci_ers_result_t	error_state;
+	struct mutex		edr_lock;
+#ifdef CONFIG_ACPI
+	struct acpi_device	*adev;
+#endif
 };
 
 static const char * const rp_pio_error_string[] = {
@@ -161,6 +168,9 @@ static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
 	if (!pcie_wait_for_link(pdev, true))
 		return PCI_ERS_RESULT_DISCONNECT;
 
+	/* Since the device recovery is done just update the error state */
+	dpc->error_state = PCI_ERS_RESULT_RECOVERED;
+
 	return PCI_ERS_RESULT_RECOVERED;
 }
 
@@ -305,14 +315,94 @@ 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
+
+static void edr_handle_event(acpi_handle handle, u32 event, void *data)
+{
+	struct dpc_dev *dpc = data;
+	struct pci_dev *pdev = dpc->dev->port;
+	u16 status;
+
+	pci_info(pdev, "ACPI event %#x received\n", event);
+
+	if (event != ACPI_NOTIFY_DISCONNECT_RECOVER)
+		return;
+
+	/*
+	 * Check if _DSM(0xD) is available, and if present locate the
+	 * port which issued EDR event.
+	 */
+	pdev = acpi_locate_dpc_port(pdev, dpc->adev->handle);
+	if (!pdev) {
+		pdev = dpc->dev->port;
+		pci_err(pdev, "No valid port found\n");
+		return;
+	}
+
+	dpc = to_dpc_dev(pdev);
+	if (!dpc) {
+		pci_err(pdev, "DPC port is NULL\n");
+		goto done;
+	}
+
+	mutex_lock(&dpc->edr_lock);
+
+	dpc->error_state = PCI_ERS_RESULT_DISCONNECT;
+
+	/*
+	 * Check if the port supports DPC:
+	 *
+	 * If port supports DPC, then fall back to default error
+	 * recovery.
+	 */
+	if (dpc->cap_pos) {
+		/* Check if there is a valid DPC trigger */
+		pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_STATUS,
+				     &status);
+		if ((status & PCI_EXP_DPC_STATUS_TRIGGER))
+			dpc_process_error(dpc);
+		else
+			pci_err(pdev, "Invalid DPC trigger %#010x\n", status);
+	}
+
+	/*
+	 * 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 = EDR_OST_SUCCESS;
+	else
+		status = EDR_OST_FAILED;
+
+	/* Use ACPI handle of DPC event device for sending EDR status */
+	dpc = data;
+
+	acpi_send_edr_status(pdev, dpc->adev->handle, status);
+
+	mutex_unlock(&dpc->edr_lock);
+done:
+	pci_dev_put(pdev);
+}
+#endif
+
 #define FLAG(x, y) (((x) & (y)) ? '+' : '-')
 static int dpc_probe(struct pcie_device *dev)
 {
 	struct dpc_dev *dpc;
 	struct pci_dev *pdev = dev->port;
 	struct device *device = &dev->device;
-	int status;
+	int status = 0;
 	u16 ctl, cap;
+#ifdef CONFIG_ACPI
+	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+#endif
 
 	dpc = devm_kzalloc(device, sizeof(*dpc), GFP_KERNEL);
 	if (!dpc)
@@ -321,6 +411,9 @@ static int dpc_probe(struct pcie_device *dev)
 	dpc->cap_pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DPC);
 	dpc->dev = dev;
 
+	dpc->error_state = PCI_ERS_RESULT_NONE;
+	mutex_init(&dpc->edr_lock);
+
 	/*
 	 * As per PCIe r5.0, sec 6.2.10, implementation note titled
 	 * "Determination of DPC Control", to avoid conflicts over whether
@@ -358,6 +451,33 @@ static int dpc_probe(struct pcie_device *dev)
 		}
 	}
 
+#ifdef CONFIG_ACPI
+	if (pcie_aer_get_firmware_first(pdev) && adev) {
+		acpi_status astatus;
+
+		dpc->adev = adev;
+
+		astatus = acpi_install_notify_handler(adev->handle,
+						      ACPI_SYSTEM_NOTIFY,
+						      edr_handle_event,
+						      dpc);
+
+		if (ACPI_FAILURE(astatus)) {
+			pci_err(pdev,
+				"Install ACPI_SYSTEM_NOTIFY handler failed\n");
+			return -EBUSY;
+		}
+
+		status = acpi_enable_dpc_port(pdev, adev->handle, true);
+		if (status) {
+			pci_warn(pdev, "Enable DPC port failed\n");
+			acpi_remove_notify_handler(adev->handle,
+						   ACPI_SYSTEM_NOTIFY,
+						   edr_handle_event);
+			return status;
+		}
+	}
+#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);
 
@@ -409,6 +529,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/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 62b7fdcc661c..f4e0d5d984b0 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -111,6 +111,19 @@ extern const guid_t pci_acpi_dsm_guid;
 #define DEVICE_LABEL_DSM		0x07
 #define RESET_DELAY_DSM			0x08
 #define FUNCTION_DELAY_DSM		0x09
+#ifdef CONFIG_PCIE_DPC
+#define EDR_PORT_ENABLE_DSM		0x0C
+#define EDR_PORT_LOCATE_DSM		0x0D
+#define EDR_OST_SUCCESS			0x80
+#define EDR_OST_FAILED			0x81
+
+int acpi_enable_dpc_port(struct pci_dev *pdev, acpi_handle handle,
+			 bool enable);
+struct pci_dev *acpi_locate_dpc_port(struct pci_dev *pdev,
+				     acpi_handle handle);
+int acpi_send_edr_status(struct pci_dev *pdev,
+			 acpi_handle handle, u16 status);
+#endif /* CONFIG_PCIE_DPC */
 
 #else	/* CONFIG_ACPI */
 static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
-- 
2.21.0

[PATCH v13 5/8] PCI/AER: Allow clearing Error Status Register in FF mode

[sathyanarayanan.kuppuswamy]

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

As per PCI firmware specification r3.2 System Firmware Intermediary
(SFI) _OSC and DPC Updates ECR
(https://members.pcisig.com/wg/PCI-SIG/document/13563), sec titled
"DPC Event Handling Implementation Note", page 10, Error Disconnect
Recover (EDR) support allows OS to handle error recovery and clearing
Error Registers even in FF mode. So create new APIs to replace
pci_cleanup_aer_uncorrect_error_status() and
pci_aer_clear_fatal_status() function implementations without FF mode
checks.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Acked-by: Keith Busch <keith.busch@intel.com>
---
 drivers/pci/pci.h      |  2 ++
 drivers/pci/pcie/aer.c | 28 ++++++++++++++++++++--------
 drivers/pci/pcie/dpc.c |  4 ++--
 3 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index a0a53bd05a0b..d71b7c07c6d2 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -440,6 +440,8 @@ struct aer_err_info {
 
 int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info);
 void aer_print_error(struct pci_dev *dev, struct aer_err_info *info);
+int pci_aer_clear_err_uncor_status(struct pci_dev *dev);
+void pci_aer_clear_err_fatal_status(struct pci_dev *dev);
 #endif	/* CONFIG_PCIEAER */
 
 #ifdef CONFIG_PCIE_DPC
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 1ca86f2e0166..4527c30db4f5 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -376,7 +376,7 @@ void pci_aer_clear_device_status(struct pci_dev *dev)
 	pcie_capability_write_word(dev, PCI_EXP_DEVSTA, sta);
 }
 
-int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
+int pci_aer_clear_err_uncor_status(struct pci_dev *dev)
 {
 	int pos;
 	u32 status, sev;
@@ -385,9 +385,6 @@ int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
 	if (!pos)
 		return -EIO;
 
-	if (pcie_aer_get_firmware_first(dev))
-		return -EIO;
-
 	/* Clear status bits for ERR_NONFATAL errors only */
 	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, &status);
 	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, &sev);
@@ -396,10 +393,19 @@ int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
 		pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, status);
 
 	return 0;
+
+}
+
+int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
+{
+	if (pcie_aer_get_firmware_first(dev))
+		return -EIO;
+
+	return pci_aer_clear_err_uncor_status(dev);
 }
 EXPORT_SYMBOL_GPL(pci_cleanup_aer_uncorrect_error_status);
 
-void pci_aer_clear_fatal_status(struct pci_dev *dev)
+void pci_aer_clear_err_fatal_status(struct pci_dev *dev)
 {
 	int pos;
 	u32 status, sev;
@@ -408,15 +414,21 @@ void pci_aer_clear_fatal_status(struct pci_dev *dev)
 	if (!pos)
 		return;
 
-	if (pcie_aer_get_firmware_first(dev))
-		return;
-
 	/* Clear status bits for ERR_FATAL errors only */
 	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, &status);
 	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, &sev);
 	status &= sev;
 	if (status)
 		pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, status);
+
+}
+
+void pci_aer_clear_fatal_status(struct pci_dev *dev)
+{
+	if (pcie_aer_get_firmware_first(dev))
+		return;
+
+	return pci_aer_clear_err_fatal_status(dev);
 }
 
 int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 8a8ee374d9b1..bb237af8dac1 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -280,8 +280,8 @@ static void dpc_process_error(struct dpc_dev *dpc)
 		 dpc_get_aer_uncorrect_severity(pdev, &info) &&
 		 aer_get_device_error_info(pdev, &info)) {
 		aer_print_error(pdev, &info);
-		pci_cleanup_aer_uncorrect_error_status(pdev);
-		pci_aer_clear_fatal_status(pdev);
+		pci_aer_clear_err_uncor_status(pdev);
+		pci_aer_clear_err_fatal_status(pdev);
 	}
 
 	/* We configure DPC so it only triggers on ERR_FATAL */
-- 
2.21.0

[PATCH v13 6/8] PCI/DPC: Update comments related to DPC recovery on NON_FATAL errors

[sathyanarayanan.kuppuswamy]

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

Currently, in native mode, DPC driver is configured to trigger DPC only
for FATAL errors and hence it only supports port recovery for FATAL
errors. But with Error Disconnect Recover (EDR) support, DPC
configuration is done by firmware, and hence we should expect DPC
triggered for both FATAL/NON_FATAL errors. So update comments and add
details about how NON_FATAL dpc recovery is handled.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Acked-by: Keith Busch <keith.busch@intel.com>
---
 drivers/pci/pcie/dpc.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index bb237af8dac1..97eb032f9a29 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -284,7 +284,11 @@ static void dpc_process_error(struct dpc_dev *dpc)
 		pci_aer_clear_err_fatal_status(pdev);
 	}
 
-	/* We configure DPC so it only triggers on ERR_FATAL */
+	/*
+	 * Irrespective of whether the DPC event is triggered by
+	 * ERR_FATAL or ERR_NONFATAL, since the link is already down,
+	 * use the FATAL error recovery path for both cases.
+	 */
 	pcie_do_recovery(pdev, pci_channel_io_frozen, PCIE_PORT_SERVICE_DPC);
 }
 
-- 
2.21.0

[PATCH v13 7/8] PCI/DPC: Clear AER registers in EDR mode

[sathyanarayanan.kuppuswamy]

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

As per PCI firmware specification r3.2 System Firmware Intermediary
(SFI) _OSC and DPC Updates ECR
(https://members.pcisig.com/wg/PCI-SIG/document/13563), sec titled
"DPC Event Handling Implementation Note", page 10, OS is responsible
for clearing the AER registers in EDR mode. So clear AER registers in
dpc_process_error() function.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Acked-by: Keith Busch <keith.busch@intel.com>
---
 drivers/pci/pci.h      |  1 +
 drivers/pci/pcie/aer.c | 13 +++++++++----
 drivers/pci/pcie/dpc.c |  4 ++++
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index d71b7c07c6d2..d55eb1f0cd7c 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -442,6 +442,7 @@ int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info);
 void aer_print_error(struct pci_dev *dev, struct aer_err_info *info);
 int pci_aer_clear_err_uncor_status(struct pci_dev *dev);
 void pci_aer_clear_err_fatal_status(struct pci_dev *dev);
+int pci_aer_clear_err_status_regs(struct pci_dev *dev);
 #endif	/* CONFIG_PCIEAER */
 
 #ifdef CONFIG_PCIE_DPC
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 4527c30db4f5..f267f7adbc30 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -431,7 +431,7 @@ void pci_aer_clear_fatal_status(struct pci_dev *dev)
 	return pci_aer_clear_err_fatal_status(dev);
 }
 
-int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
+int pci_aer_clear_err_status_regs(struct pci_dev *dev)
 {
 	int pos;
 	u32 status;
@@ -444,9 +444,6 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
 	if (!pos)
 		return -EIO;
 
-	if (pcie_aer_get_firmware_first(dev))
-		return -EIO;
-
 	port_type = pci_pcie_type(dev);
 	if (port_type == PCI_EXP_TYPE_ROOT_PORT) {
 		pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, &status);
@@ -462,6 +459,14 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
 	return 0;
 }
 
+int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
+{
+	if (pcie_aer_get_firmware_first(dev))
+		return -EIO;
+
+	return pci_aer_clear_err_status_regs(dev);
+}
+
 void pci_save_aer_state(struct pci_dev *dev)
 {
 	struct pci_cap_saved_state *save_state;
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 97eb032f9a29..00ed77ea9380 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -284,6 +284,10 @@ static void dpc_process_error(struct dpc_dev *dpc)
 		pci_aer_clear_err_fatal_status(pdev);
 	}
 
+	/* In EDR mode, OS is responsible for clearing AER registers */
+	if (dpc->edr_enabled)
+		pci_aer_clear_err_status_regs(pdev);
+
 	/*
 	 * Irrespective of whether the DPC event is triggered by
 	 * ERR_FATAL or ERR_NONFATAL, since the link is already down,
-- 
2.21.0

[PATCH v13 8/8] PCI/ACPI: Enable EDR support

[sathyanarayanan.kuppuswamy]

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

As per PCI firmware specification r3.2 Downstream Port Containment
Related Enhancements ECN, sec 4.5.1, OS must implement following steps
to enable/use EDR feature.

1. 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.

2. Also, if OS supports EDR, it should expose its support to BIOS by
setting bit 7 of _OSC Support Field. And if OS sets bit 7 of _OSC
Control Field it must also expose support for EDR by setting bit 7 of
_OSC Support Field.

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Acked-by: Keith Busch <keith.busch@intel.com>
Tested-by: Huong Nguyen <huong.nguyen@dell.com>
Tested-by: Austin Bolen <Austin.Bolen@dell.com>
---
 drivers/acpi/pci_root.c         | 16 ++++++++++++++++
 drivers/pci/pcie/portdrv_core.c |  7 +++++--
 drivers/pci/probe.c             |  1 +
 include/linux/acpi.h            |  6 ++++--
 include/linux/pci.h             |  1 +
 5 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index d1e666ef3fcc..ad1be5941a00 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -131,6 +131,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" },
 	{ OSC_PCI_HPX_TYPE_3_SUPPORT, "HPX-Type3" },
 };
 
@@ -141,6 +142,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,
@@ -440,6 +442,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);
@@ -487,6 +491,16 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 			control |= OSC_PCI_EXPRESS_AER_CONTROL;
 	}
 
+	/*
+	 * Per the Downstream Port Containment Related Enhancements ECN to
+	 * the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-5,
+	 * OSC_PCI_EXPRESS_DPC_CONTROL indicates the OS supports both DPC
+	 * and EDR. So use CONFIG_PCIE_EDR for requesting DPC control which
+	 * will only be turned on if both EDR and DPC is enabled.
+	 */
+	if (IS_ENABLED(CONFIG_PCIE_EDR))
+		control |= OSC_PCI_EXPRESS_DPC_CONTROL;
+
 	requested = control;
 	status = acpi_pci_osc_control_set(handle, &control,
 					  OSC_PCI_EXPRESS_CAPABILITY_CONTROL);
@@ -916,6 +930,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;
 
 	/*
 	 * Evaluate the "PCI Boot Configuration" _DSM Function.  If it
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 5075cb9e850c..009742c865d6 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -253,10 +253,13 @@ static int get_port_device_capability(struct pci_dev *dev)
 	/*
 	 * With dpc-native, allow Linux to use DPC even if it doesn't have
 	 * permission to use AER.
+	 * 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() &&
-	    (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER)))
+	    ((IS_ENABLED(CONFIG_PCIE_EDR) && !host->native_dpc) ||
+	    (pci_aer_available() &&
+	    (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER)))))
 		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 512cb4312ddd..c9a9c5b42e72 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -598,6 +598,7 @@ static void pci_init_host_bridge(struct pci_host_bridge *bridge)
 	bridge->native_shpc_hotplug = 1;
 	bridge->native_pme = 1;
 	bridge->native_ltr = 1;
+	bridge->native_dpc = 1;
 }
 
 struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 0f37a7d5fa77..0a7aaa452a98 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -515,8 +515,9 @@ 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_EDR_SUPPORT			0x00000080
 #define OSC_PCI_HPX_TYPE_3_SUPPORT		0x00000100
-#define OSC_PCI_SUPPORT_MASKS			0x0000011f
+#define OSC_PCI_SUPPORT_MASKS			0x0000019f
 
 /* PCI Host Bridge _OSC: Capabilities DWORD 3: Control Field */
 #define OSC_PCI_EXPRESS_NATIVE_HP_CONTROL	0x00000001
@@ -525,7 +526,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			0x000000bf
 
 #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 c393dff2d66f..d0739e90f4e7 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -510,6 +510,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 */
 	unsigned int	preserve_config:1;	/* Preserve FW resource setup */
 
 	/* Resource alignment requirements */
-- 
2.21.0

Re: [PATCH v13 2/8] PCI/DPC: Allow dpc_probe() even if firmware first mode is enabled

[Bjorn Helgaas]

On Sat, Jan 18, 2020 at 08:00:31PM -0800, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
> As per ACPI specification v6.3, sec 5.6.6, Error Disconnect Recover
> (EDR) notification used by firmware to let OS know about the DPC event
> and permit OS to perform error recovery when processing the EDR
> notification. 

I want to clear up some of this description because this is a very
confusing area and we should follow the spec carefully so we all
understand each other.

> Also, as per PCI firmware specification r3.2 Downstream
> Port Containment Related Enhancements ECN, sec 4.5.1, table 4-6, if DPC
> is controlled by firmware (firmware first mode), it's responsible for
> initializing Downstream Port Containment Extended Capability Structures
> per firmware policy. 

The above is actually not what the ECN says.  What it does say is
this:

  If control of this feature [DPC configuration] was requested and
  denied, firmware is responsible for initializing Downstream Port
  Containment Extended Capability Structures per firmware policy.

Neither the PCI Firmware Spec (r3.2) nor the ECN contains any
reference to "firmware first".  As far as I can tell, "Firmware First"
is defined by the ACPI spec, and the FIRMWARE_FIRST bit in various
HEST entries (sec 18.3.2) is what tells us when a device is in
firmware-first mode.

That's a really long way of saying that from a spec point of view, DPC
being controlled by firmware does NOT imply anything about
firmware-first mode.

> And, OS is permitted to read or write DPC Control
> and Status registers of a port while processing an Error Disconnect
> Recover (EDR) notification from firmware on that port.
> 
> Currently, if firmware controls DPC (firmware first mode), OS will not
> create/enumerate DPC PCIe port services. But, if OS supports EDR
> feature, then as mentioned in above spec references, it should permit
> enumeration of DPC driver and also support handling ACPI EDR
> notification. So as first step, allow dpc_probe() to continue even if
> firmware first mode is enabled. Also add appropriate checks to ensure
> device registers are not modified outside EDR notification window in
> firmware first mode. This is a preparatory patch for adding EDR support.
> 
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Acked-by: Keith Busch <keith.busch@intel.com>
> ---
>  drivers/pci/pcie/dpc.c | 74 ++++++++++++++++++++++++++++++++++--------
>  1 file changed, 61 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index e06f42f58d3d..c583a90fa90d 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -22,6 +22,7 @@ struct dpc_dev {
>  	u16			cap_pos;
>  	bool			rp_extensions;
>  	u8			rp_log_size;
> +	bool			edr_enabled; /* EDR mode is supported */

This needs a better name, or perhaps it should be removed completely.
EDR is not a mode that can be enabled or disabled.  The _OSC "EDR
supported" bit tells the firmware whether the OS supports EDR
notification, but there's no corresponding bit that tells the OS
whether firmware will actually *send* those notifications.

Also, EDR is an ACPI concept, and dpc.c is a generic driver not
specific to ACPI, so we should try to avoid polluting it with
ACPI-isms.

>  };
>  
>  static const char * const rp_pio_error_string[] = {
> @@ -69,6 +70,14 @@ void pci_save_dpc_state(struct pci_dev *dev)
>  	if (!dpc)
>  		return;
>  
> +	/*
> +	 * If DPC is controlled by firmware then save/restore tasks are also
> +	 * controlled by firmware. So skip rest of the function if DPC is
> +	 * controlled by firmware.
> +	 */
> +	if (dpc->edr_enabled)
> +		return;

I think this should be something like:

  if (!host->native_dpc)
    return;

That's what the spec says: if firmware does not grant control of DPC
to the OS via _OSC, the OS may not read/write the DPC capability.

The usual situation would be that if firmware doesn't grant the OS
permission to use DPC, we wouldn't use the dpc.c driver at all.

But IIUC, the point of this EDR stuff is that we're adding a case
where the OS doesn't have permission to use DPC, but we *do* want to
use parts of dpc.c in limited cases while handling EDR notifications.

I think you should probably take the DPC-related _OSC stuff from the
last patch ("PCI/ACPI: Enable EDR support") and move it before this
patch, so it *only* negotiates DPC ownership, per the ECN.  That would
probably result in dpc.c being used only if _OSC grants DPC ownership
to the OS.

Then subsequent patches like this one would relax that slightly and
allow dpc.c to be used even without DPC ownership, but it would only
touch the DPC capability in limited cases.

>  	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_DPC);
>  	if (!save_state)
>  		return;
> @@ -90,6 +99,14 @@ void pci_restore_dpc_state(struct pci_dev *dev)
>  	if (!dpc)
>  		return;
>  
> +	/*
> +	 * If DPC is controlled by firmware then save/restore tasks are also
> +	 * controlled by firmware. So skip rest of the function if DPC is
> +	 * controlled by firmware.
> +	 */
> +	if (dpc->edr_enabled)
> +		return;
> +
>  	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_DPC);
>  	if (!save_state)
>  		return;
> @@ -291,24 +308,48 @@ static int dpc_probe(struct pcie_device *dev)
>  	int status;
>  	u16 ctl, cap;
>  
> -	if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native)
> -		return -ENOTSUPP;
> -
>  	dpc = devm_kzalloc(device, sizeof(*dpc), GFP_KERNEL);
>  	if (!dpc)
>  		return -ENOMEM;
>  
>  	dpc->cap_pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DPC);
>  	dpc->dev = dev;
> +
> +	/*
> +	 * As per PCIe r5.0, sec 6.2.10, implementation note titled
> +	 * "Determination of DPC Control", to avoid conflicts over whether
> +	 * platform firmware or the operating system have control of DPC,
> +	 * it is recommended that platform firmware and operating systems
> +	 * always link the control of DPC to the control of Advanced Error
> +	 * Reporting.
> +	 *
> +	 * So use AER FF mode check API pcie_aer_get_firmware_first() to decide
> +	 * whether DPC is controlled by software or firmware. 

AFAICT, this is not what the spec says.  Firmware-first is not what
tells us whether DPC is controlled by firmware or the OS.  For ACPI
systems, _OSC tells us whether the OS is allowed control of DPC.

> +	 * And EDR support
> +	 * can only be enabled if DPC is controlled by firmware.
> +	 */
> +
> +	if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native)
> +		dpc->edr_enabled = 1;

> +	/*
> +	 * If DPC is handled in firmware and ACPI support is not enabled
> +	 * in OS, skip probe and return error.
> +	 */
> +	if (dpc->edr_enabled && !IS_ENABLED(CONFIG_ACPI))
> +		return -ENODEV;
> +
>  	set_service_data(dev, dpc);
>  
> -	status = devm_request_threaded_irq(device, dev->irq, dpc_irq,
> -					   dpc_handler, IRQF_SHARED,
> -					   "pcie-dpc", dpc);
> -	if (status) {
> -		pci_warn(pdev, "request IRQ%d failed: %d\n", dev->irq,
> -			 status);
> -		return status;
> +	/* Register interrupt handler only if OS controls DPC */
> +	if (!dpc->edr_enabled) {
> +		status = devm_request_threaded_irq(device, dev->irq, dpc_irq,
> +						   dpc_handler, IRQF_SHARED,
> +						   "pcie-dpc", dpc);
> +		if (status) {
> +			pci_warn(pdev, "request IRQ%d failed: %d\n", dev->irq,
> +				 status);
> +			return status;
> +		}
>  	}
>  
>  	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CAP, &cap);
> @@ -323,9 +364,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->edr_enabled) {
> +		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);
> +	}
>  
>  	pci_info(pdev, "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),
> @@ -343,6 +387,10 @@ static void dpc_remove(struct pcie_device *dev)
>  	struct pci_dev *pdev = dev->port;
>  	u16 ctl;
>  
> +	/* Skip updating DPC registers if DPC is controlled by firmware */
> +	if (dpc->edr_enabled)
> +		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.21.0
> 

Re: [PATCH v13 2/8] PCI/DPC: Allow dpc_probe() even if firmware first mode is enabled

[Kuppuswamy Sathyanarayanan]

Hi Bjorn,

On 1/22/20 3:17 PM, Bjorn Helgaas wrote:
> On Sat, Jan 18, 2020 at 08:00:31PM -0800, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>
>> As per ACPI specification v6.3, sec 5.6.6, Error Disconnect Recover
>> (EDR) notification used by firmware to let OS know about the DPC event
>> and permit OS to perform error recovery when processing the EDR
>> notification.
> I want to clear up some of this description because this is a very
> confusing area and we should follow the spec carefully so we all
> understand each other.
>
>> Also, as per PCI firmware specification r3.2 Downstream
>> Port Containment Related Enhancements ECN, sec 4.5.1, table 4-6, if DPC
>> is controlled by firmware (firmware first mode), it's responsible for
>> initializing Downstream Port Containment Extended Capability Structures
>> per firmware policy.
> The above is actually not what the ECN says.  What it does say is
> this:
>
>    If control of this feature [DPC configuration] was requested and
>    denied, firmware is responsible for initializing Downstream Port
>    Containment Extended Capability Structures per firmware policy.
>
> Neither the PCI Firmware Spec (r3.2) nor the ECN contains any
> reference to "firmware first".  As far as I can tell, "Firmware First"
> is defined by the ACPI spec, and the FIRMWARE_FIRST bit in various
> HEST entries (sec 18.3.2) is what tells us when a device is in
> firmware-first mode.
>
> That's a really long way of saying that from a spec point of view, DPC
> being controlled by firmware does NOT imply anything about
> firmware-first mode.
But current AER and DPC driver uses ACPI FIRMWARE_FIRST bit
(in pcie_aer_get_firmware_first()) to decide whether AER/DPC is
controlled by firmware or OS. That's why I used the term
"firmware first mode" interchangeably with a mode in which
DPC/AER is controlled by firmware.
>
>> And, OS is permitted to read or write DPC Control
>> and Status registers of a port while processing an Error Disconnect
>> Recover (EDR) notification from firmware on that port.
>>
>> Currently, if firmware controls DPC (firmware first mode), OS will not
>> create/enumerate DPC PCIe port services. But, if OS supports EDR
>> feature, then as mentioned in above spec references, it should permit
>> enumeration of DPC driver and also support handling ACPI EDR
>> notification. So as first step, allow dpc_probe() to continue even if
>> firmware first mode is enabled. Also add appropriate checks to ensure
>> device registers are not modified outside EDR notification window in
>> firmware first mode. This is a preparatory patch for adding EDR support.
>>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> Acked-by: Keith Busch <keith.busch@intel.com>
>> ---
>>   drivers/pci/pcie/dpc.c | 74 ++++++++++++++++++++++++++++++++++--------
>>   1 file changed, 61 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
>> index e06f42f58d3d..c583a90fa90d 100644
>> --- a/drivers/pci/pcie/dpc.c
>> +++ b/drivers/pci/pcie/dpc.c
>> @@ -22,6 +22,7 @@ struct dpc_dev {
>>   	u16			cap_pos;
>>   	bool			rp_extensions;
>>   	u8			rp_log_size;
>> +	bool			edr_enabled; /* EDR mode is supported */
> This needs a better name, or perhaps it should be removed completely.
Any suggestions ? native_mode or firmware_dpc ?
> EDR is not a mode that can be enabled or disabled.  The _OSC "EDR
> supported" bit tells the firmware whether the OS supports EDR
> notification, but there's no corresponding bit that tells the OS
> whether firmware will actually *send* those notifications.
I agree that EDR is not a mode. But with EDR support, DPC driver functions
can be triggered/executed in two contexts.

1. DPC is controlled by OS.
2. Via EDR notification if DPC is controlled by firmware.

Since we want to use same code for both scenarios, we need some method
to detect which context is currently in use.
>
> Also, EDR is an ACPI concept, and dpc.c is a generic driver not
> specific to ACPI, so we should try to avoid polluting it with
> ACPI-isms.
>
>>   };
>>   
>>   static const char * const rp_pio_error_string[] = {
>> @@ -69,6 +70,14 @@ void pci_save_dpc_state(struct pci_dev *dev)
>>   	if (!dpc)
>>   		return;
>>   
>> +	/*
>> +	 * If DPC is controlled by firmware then save/restore tasks are also
>> +	 * controlled by firmware. So skip rest of the function if DPC is
>> +	 * controlled by firmware.
>> +	 */
>> +	if (dpc->edr_enabled)
>> +		return;
> I think this should be something like:
>
>    if (!host->native_dpc)
>      return;
>
> That's what the spec says: if firmware does not grant control of DPC
> to the OS via _OSC, the OS may not read/write the DPC capability.
>
> The usual situation would be that if firmware doesn't grant the OS
> permission to use DPC, we wouldn't use the dpc.c driver at all.
>
> But IIUC, the point of this EDR stuff is that we're adding a case
> where the OS doesn't have permission to use DPC, but we *do* want to
> use parts of dpc.c in limited cases while handling EDR notifications.
Yes, your assumption is correct.
>
> I think you should probably take the DPC-related _OSC stuff from the
> last patch ("PCI/ACPI: Enable EDR support") and move it before this
> patch, so it *only* negotiates DPC ownership, per the ECN.  That would
> probably result in dpc.c being used only if _OSC grants DPC ownership
> to the OS.
Patch titled ("PCI/ACPI: Enable EDR support") exposes EDR support to 
firmware in
_OSC negotiation. So you wanted me to move this patch to the end of the 
series
to make sure we don't expose EDR support until we have necessary code 
changes
in kernel.
>
> Then subsequent patches like this one would relax that slightly and
> allow dpc.c to be used even without DPC ownership, but it would only
> touch the DPC capability in limited cases.
>
>>   	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_DPC);
>>   	if (!save_state)
>>   		return;
>> @@ -90,6 +99,14 @@ void pci_restore_dpc_state(struct pci_dev *dev)
>>   	if (!dpc)
>>   		return;
>>   
>> +	/*
>> +	 * If DPC is controlled by firmware then save/restore tasks are also
>> +	 * controlled by firmware. So skip rest of the function if DPC is
>> +	 * controlled by firmware.
>> +	 */
>> +	if (dpc->edr_enabled)
>> +		return;
>> +
>>   	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_DPC);
>>   	if (!save_state)
>>   		return;
>> @@ -291,24 +308,48 @@ static int dpc_probe(struct pcie_device *dev)
>>   	int status;
>>   	u16 ctl, cap;
>>   
>> -	if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native)
>> -		return -ENOTSUPP;
>> -
>>   	dpc = devm_kzalloc(device, sizeof(*dpc), GFP_KERNEL);
>>   	if (!dpc)
>>   		return -ENOMEM;
>>   
>>   	dpc->cap_pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DPC);
>>   	dpc->dev = dev;
>> +
>> +	/*
>> +	 * As per PCIe r5.0, sec 6.2.10, implementation note titled
>> +	 * "Determination of DPC Control", to avoid conflicts over whether
>> +	 * platform firmware or the operating system have control of DPC,
>> +	 * it is recommended that platform firmware and operating systems
>> +	 * always link the control of DPC to the control of Advanced Error
>> +	 * Reporting.
>> +	 *
>> +	 * So use AER FF mode check API pcie_aer_get_firmware_first() to decide
>> +	 * whether DPC is controlled by software or firmware.
> AFAICT, this is not what the spec says.  Firmware-first is not what
> tells us whether DPC is controlled by firmware or the OS.  For ACPI
> systems, _OSC tells us whether the OS is allowed control of DPC.
But current DPC/AER driver use FIRMWARE_FIRST bit (in 
pcie_aer_get_firmware_first())
determine firmware/OS ownership.
>
>> +	 * And EDR support
>> +	 * can only be enabled if DPC is controlled by firmware.
>> +	 */
>> +
>> +	if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native)
>> +		dpc->edr_enabled = 1;
>> +	/*
>> +	 * If DPC is handled in firmware and ACPI support is not enabled
>> +	 * in OS, skip probe and return error.
>> +	 */
>> +	if (dpc->edr_enabled && !IS_ENABLED(CONFIG_ACPI))
>> +		return -ENODEV;
>> +
>>   	set_service_data(dev, dpc);
>>   
>> -	status = devm_request_threaded_irq(device, dev->irq, dpc_irq,
>> -					   dpc_handler, IRQF_SHARED,
>> -					   "pcie-dpc", dpc);
>> -	if (status) {
>> -		pci_warn(pdev, "request IRQ%d failed: %d\n", dev->irq,
>> -			 status);
>> -		return status;
>> +	/* Register interrupt handler only if OS controls DPC */
>> +	if (!dpc->edr_enabled) {
>> +		status = devm_request_threaded_irq(device, dev->irq, dpc_irq,
>> +						   dpc_handler, IRQF_SHARED,
>> +						   "pcie-dpc", dpc);
>> +		if (status) {
>> +			pci_warn(pdev, "request IRQ%d failed: %d\n", dev->irq,
>> +				 status);
>> +			return status;
>> +		}
>>   	}
>>   
>>   	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CAP, &cap);
>> @@ -323,9 +364,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->edr_enabled) {
>> +		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);
>> +	}
>>   
>>   	pci_info(pdev, "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),
>> @@ -343,6 +387,10 @@ static void dpc_remove(struct pcie_device *dev)
>>   	struct pci_dev *pdev = dev->port;
>>   	u16 ctl;
>>   
>> +	/* Skip updating DPC registers if DPC is controlled by firmware */
>> +	if (dpc->edr_enabled)
>> +		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.21.0
>>
-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

Re: [PATCH v13 2/8] PCI/DPC: Allow dpc_probe() even if firmware first mode is enabled

[Bjorn Helgaas]

On Wed, Jan 22, 2020 at 04:42:48PM -0800, Kuppuswamy Sathyanarayanan wrote:
> Hi Bjorn,
> 
> On 1/22/20 3:17 PM, Bjorn Helgaas wrote:
> > On Sat, Jan 18, 2020 at 08:00:31PM -0800, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > 
> > > As per ACPI specification v6.3, sec 5.6.6, Error Disconnect Recover
> > > (EDR) notification used by firmware to let OS know about the DPC event
> > > and permit OS to perform error recovery when processing the EDR
> > > notification.
> > I want to clear up some of this description because this is a very
> > confusing area and we should follow the spec carefully so we all
> > understand each other.
> > 
> > > Also, as per PCI firmware specification r3.2 Downstream
> > > Port Containment Related Enhancements ECN, sec 4.5.1, table 4-6, if DPC
> > > is controlled by firmware (firmware first mode), it's responsible for
> > > initializing Downstream Port Containment Extended Capability Structures
> > > per firmware policy.
> > The above is actually not what the ECN says.  What it does say is
> > this:
> > 
> >    If control of this feature [DPC configuration] was requested and
> >    denied, firmware is responsible for initializing Downstream Port
> >    Containment Extended Capability Structures per firmware policy.
> > 
> > Neither the PCI Firmware Spec (r3.2) nor the ECN contains any
> > reference to "firmware first".  As far as I can tell, "Firmware First"
> > is defined by the ACPI spec, and the FIRMWARE_FIRST bit in various
> > HEST entries (sec 18.3.2) is what tells us when a device is in
> > firmware-first mode.
> > 
> > That's a really long way of saying that from a spec point of view, DPC
> > being controlled by firmware does NOT imply anything about
> > firmware-first mode.
>
> But current AER and DPC driver uses ACPI FIRMWARE_FIRST bit
> (in pcie_aer_get_firmware_first()) to decide whether AER/DPC is
> controlled by firmware or OS. That's why I used the term
> "firmware first mode" interchangeably with a mode in which
> DPC/AER is controlled by firmware.

Yes.  I think the current use of pcie_aer_get_firmware_first() there
is probably not optimal and we should change that so it corresponds
better with the spec.

> > > And, OS is permitted to read or write DPC Control
> > > and Status registers of a port while processing an Error Disconnect
> > > Recover (EDR) notification from firmware on that port.
> > > 
> > > Currently, if firmware controls DPC (firmware first mode), OS will not
> > > create/enumerate DPC PCIe port services. But, if OS supports EDR
> > > feature, then as mentioned in above spec references, it should permit
> > > enumeration of DPC driver and also support handling ACPI EDR
> > > notification. So as first step, allow dpc_probe() to continue even if
> > > firmware first mode is enabled. Also add appropriate checks to ensure
> > > device registers are not modified outside EDR notification window in
> > > firmware first mode. This is a preparatory patch for adding EDR support.
> > > 
> > > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > Acked-by: Keith Busch <keith.busch@intel.com>
> > > ---
> > >   drivers/pci/pcie/dpc.c | 74 ++++++++++++++++++++++++++++++++++--------
> > >   1 file changed, 61 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > > index e06f42f58d3d..c583a90fa90d 100644
> > > --- a/drivers/pci/pcie/dpc.c
> > > +++ b/drivers/pci/pcie/dpc.c
> > > @@ -22,6 +22,7 @@ struct dpc_dev {
> > >   	u16			cap_pos;
> > >   	bool			rp_extensions;
> > >   	u8			rp_log_size;
> > > +	bool			edr_enabled; /* EDR mode is supported */
> > This needs a better name, or perhaps it should be removed completely.
> Any suggestions ? native_mode or firmware_dpc ?
> > EDR is not a mode that can be enabled or disabled.  The _OSC "EDR
> > supported" bit tells the firmware whether the OS supports EDR
> > notification, but there's no corresponding bit that tells the OS
> > whether firmware will actually *send* those notifications.
> I agree that EDR is not a mode. But with EDR support, DPC driver functions
> can be triggered/executed in two contexts.
> 
> 1. DPC is controlled by OS.
> 2. Via EDR notification if DPC is controlled by firmware.
> 
> Since we want to use same code for both scenarios, we need some method
> to detect which context is currently in use.

I haven't gotten as far as looking at the actual EDR support yet, but
I wonder if this could be rearranged so that dpc.c contained two
things: (1) the existing DPC driver that claims DPC capability via
dpc_probe() and (2) a few functions exported for use by the EDR path.
Then maybe the EDR path could live in pci-acpi.c (or maybe a new edr.c
file) and would not depend on dpc_probe() actually *claiming* the
capability.

> > Also, EDR is an ACPI concept, and dpc.c is a generic driver not
> > specific to ACPI, so we should try to avoid polluting it with
> > ACPI-isms.
> > 
> > >   };
> > >   static const char * const rp_pio_error_string[] = {
> > > @@ -69,6 +70,14 @@ void pci_save_dpc_state(struct pci_dev *dev)
> > >   	if (!dpc)
> > >   		return;
> > > +	/*
> > > +	 * If DPC is controlled by firmware then save/restore tasks are also
> > > +	 * controlled by firmware. So skip rest of the function if DPC is
> > > +	 * controlled by firmware.
> > > +	 */
> > > +	if (dpc->edr_enabled)
> > > +		return;
> > I think this should be something like:
> > 
> >    if (!host->native_dpc)
> >      return;
> > 
> > That's what the spec says: if firmware does not grant control of DPC
> > to the OS via _OSC, the OS may not read/write the DPC capability.
> > 
> > The usual situation would be that if firmware doesn't grant the OS
> > permission to use DPC, we wouldn't use the dpc.c driver at all.
> > 
> > But IIUC, the point of this EDR stuff is that we're adding a case
> > where the OS doesn't have permission to use DPC, but we *do* want to
> > use parts of dpc.c in limited cases while handling EDR notifications.
> Yes, your assumption is correct.
> > 
> > I think you should probably take the DPC-related _OSC stuff from the
> > last patch ("PCI/ACPI: Enable EDR support") and move it before this
> > patch, so it *only* negotiates DPC ownership, per the ECN.  That would
> > probably result in dpc.c being used only if _OSC grants DPC ownership
> > to the OS.

> Patch titled ("PCI/ACPI: Enable EDR support") exposes EDR support to
> firmware in _OSC negotiation. So you wanted me to move this patch to
> the end of the series to make sure we don't expose EDR support until
> we have necessary code changes in kernel.

It actually does *two* things: (1) adds OSC_PCI_EXPRESS_DPC_CONTROL
and native_dpc, and (2) adds OSC_PCI_EDR_SUPPORT and related code.  I
think these should be split into two patches, and part 1 is what I
think could be done early before adding EDR support.

> > > +	 * As per PCIe r5.0, sec 6.2.10, implementation note titled
> > > +	 * "Determination of DPC Control", to avoid conflicts over whether
> > > +	 * platform firmware or the operating system have control of DPC,
> > > +	 * it is recommended that platform firmware and operating systems
> > > +	 * always link the control of DPC to the control of Advanced Error
> > > +	 * Reporting.
> > > +	 *
> > > +	 * So use AER FF mode check API pcie_aer_get_firmware_first() to decide
> > > +	 * whether DPC is controlled by software or firmware.

> > AFAICT, this is not what the spec says.  Firmware-first is not what
> > tells us whether DPC is controlled by firmware or the OS.  For ACPI
> > systems, _OSC tells us whether the OS is allowed control of DPC.

> But current DPC/AER driver use FIRMWARE_FIRST bit (in
> pcie_aer_get_firmware_first()) determine firmware/OS ownership.

I think that's a problem and we should fix it.  It's arguably a little
bit of feature creep to do that in this series, but it makes it very
confusing when the specs talk about ownership negotiated via _OSC and
the code talks about "firmware-first" and makes assumptions about
connections between the two.

Those things may very well *be* related in the firmware
implementation, but assuming those connections in Linux makes the code
hard to read and maintain.  This series adds quite a bit of
ACPI-related stuff, and I think we should untangle confusions like
this before we add more.

Bjorn

Re: [PATCH v13 2/8] PCI/DPC: Allow dpc_probe() even if firmware first mode is enabled

[Kuppuswamy Sathyanarayanan]

On 1/22/20 7:10 PM, Bjorn Helgaas wrote:
> On Wed, Jan 22, 2020 at 04:42:48PM -0800, Kuppuswamy Sathyanarayanan wrote:
>> Hi Bjorn,
>>
>> On 1/22/20 3:17 PM, Bjorn Helgaas wrote:
>>> On Sat, Jan 18, 2020 at 08:00:31PM -0800, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>>>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>>
>>>> As per ACPI specification v6.3, sec 5.6.6, Error Disconnect Recover
>>>> (EDR) notification used by firmware to let OS know about the DPC event
>>>> and permit OS to perform error recovery when processing the EDR
>>>> notification.
>>> I want to clear up some of this description because this is a very
>>> confusing area and we should follow the spec carefully so we all
>>> understand each other.
>>>
>>>> Also, as per PCI firmware specification r3.2 Downstream
>>>> Port Containment Related Enhancements ECN, sec 4.5.1, table 4-6, if DPC
>>>> is controlled by firmware (firmware first mode), it's responsible for
>>>> initializing Downstream Port Containment Extended Capability Structures
>>>> per firmware policy.
>>> The above is actually not what the ECN says.  What it does say is
>>> this:
>>>
>>>     If control of this feature [DPC configuration] was requested and
>>>     denied, firmware is responsible for initializing Downstream Port
>>>     Containment Extended Capability Structures per firmware policy.
>>>
>>> Neither the PCI Firmware Spec (r3.2) nor the ECN contains any
>>> reference to "firmware first".  As far as I can tell, "Firmware First"
>>> is defined by the ACPI spec, and the FIRMWARE_FIRST bit in various
>>> HEST entries (sec 18.3.2) is what tells us when a device is in
>>> firmware-first mode.
>>>
>>> That's a really long way of saying that from a spec point of view, DPC
>>> being controlled by firmware does NOT imply anything about
>>> firmware-first mode.
>> But current AER and DPC driver uses ACPI FIRMWARE_FIRST bit
>> (in pcie_aer_get_firmware_first()) to decide whether AER/DPC is
>> controlled by firmware or OS. That's why I used the term
>> "firmware first mode" interchangeably with a mode in which
>> DPC/AER is controlled by firmware.
> Yes.  I think the current use of pcie_aer_get_firmware_first() there
> is probably not optimal and we should change that so it corresponds
> better with the spec.
>
>>>> And, OS is permitted to read or write DPC Control
>>>> and Status registers of a port while processing an Error Disconnect
>>>> Recover (EDR) notification from firmware on that port.
>>>>
>>>> Currently, if firmware controls DPC (firmware first mode), OS will not
>>>> create/enumerate DPC PCIe port services. But, if OS supports EDR
>>>> feature, then as mentioned in above spec references, it should permit
>>>> enumeration of DPC driver and also support handling ACPI EDR
>>>> notification. So as first step, allow dpc_probe() to continue even if
>>>> firmware first mode is enabled. Also add appropriate checks to ensure
>>>> device registers are not modified outside EDR notification window in
>>>> firmware first mode. This is a preparatory patch for adding EDR support.
>>>>
>>>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>> Acked-by: Keith Busch <keith.busch@intel.com>
>>>> ---
>>>>    drivers/pci/pcie/dpc.c | 74 ++++++++++++++++++++++++++++++++++--------
>>>>    1 file changed, 61 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
>>>> index e06f42f58d3d..c583a90fa90d 100644
>>>> --- a/drivers/pci/pcie/dpc.c
>>>> +++ b/drivers/pci/pcie/dpc.c
>>>> @@ -22,6 +22,7 @@ struct dpc_dev {
>>>>    	u16			cap_pos;
>>>>    	bool			rp_extensions;
>>>>    	u8			rp_log_size;
>>>> +	bool			edr_enabled; /* EDR mode is supported */
>>> This needs a better name, or perhaps it should be removed completely.
>> Any suggestions ? native_mode or firmware_dpc ?
>>> EDR is not a mode that can be enabled or disabled.  The _OSC "EDR
>>> supported" bit tells the firmware whether the OS supports EDR
>>> notification, but there's no corresponding bit that tells the OS
>>> whether firmware will actually *send* those notifications.
>> I agree that EDR is not a mode. But with EDR support, DPC driver functions
>> can be triggered/executed in two contexts.
>>
>> 1. DPC is controlled by OS.
>> 2. Via EDR notification if DPC is controlled by firmware.
>>
>> Since we want to use same code for both scenarios, we need some method
>> to detect which context is currently in use.
> I haven't gotten as far as looking at the actual EDR support yet, but
> I wonder if this could be rearranged so that dpc.c contained two
> things: (1) the existing DPC driver that claims DPC capability via
> dpc_probe() and (2) a few functions exported for use by the EDR path.
> Then maybe the EDR path could live in pci-acpi.c (or maybe a new edr.c
> file) and would not depend on dpc_probe() actually *claiming* the
> capability.
In EDR code path, we depend on common pcie_do_recovery() function in
pcie/err.c for doing the error recovery, and this function has
dependency (related to callbacks) on service (DPC/AER) from
which error recovery is triggered. So if we plan to separate EDR code 
path from
DPC by exporting required functions, we might have to do similar thing 
in err.c.
There will be also be some code duplication if we choose this path. So, 
I am not
sure whether there is enough gain in taking this approach. Let me know 
your comments.


>
>>> Also, EDR is an ACPI concept, and dpc.c is a generic driver not
>>> specific to ACPI, so we should try to avoid polluting it with
>>> ACPI-isms.
>>>
>>>>    };
>>>>    static const char * const rp_pio_error_string[] = {
>>>> @@ -69,6 +70,14 @@ void pci_save_dpc_state(struct pci_dev *dev)
>>>>    	if (!dpc)
>>>>    		return;
>>>> +	/*
>>>> +	 * If DPC is controlled by firmware then save/restore tasks are also
>>>> +	 * controlled by firmware. So skip rest of the function if DPC is
>>>> +	 * controlled by firmware.
>>>> +	 */
>>>> +	if (dpc->edr_enabled)
>>>> +		return;
>>> I think this should be something like:
>>>
>>>     if (!host->native_dpc)
>>>       return;
>>>
>>> That's what the spec says: if firmware does not grant control of DPC
>>> to the OS via _OSC, the OS may not read/write the DPC capability.
>>>
>>> The usual situation would be that if firmware doesn't grant the OS
>>> permission to use DPC, we wouldn't use the dpc.c driver at all.
>>>
>>> But IIUC, the point of this EDR stuff is that we're adding a case
>>> where the OS doesn't have permission to use DPC, but we *do* want to
>>> use parts of dpc.c in limited cases while handling EDR notifications.
>> Yes, your assumption is correct.
>>> I think you should probably take the DPC-related _OSC stuff from the
>>> last patch ("PCI/ACPI: Enable EDR support") and move it before this
>>> patch, so it *only* negotiates DPC ownership, per the ECN.  That would
>>> probably result in dpc.c being used only if _OSC grants DPC ownership
>>> to the OS.
>> Patch titled ("PCI/ACPI: Enable EDR support") exposes EDR support to
>> firmware in _OSC negotiation. So you wanted me to move this patch to
>> the end of the series to make sure we don't expose EDR support until
>> we have necessary code changes in kernel.
> It actually does *two* things: (1) adds OSC_PCI_EXPRESS_DPC_CONTROL
> and native_dpc, and (2) adds OSC_PCI_EDR_SUPPORT and related code.  I
> think these should be split into two patches, and part 1 is what I
> think could be done early before adding EDR support.
ok. makes sense.
>
>>>> +	 * As per PCIe r5.0, sec 6.2.10, implementation note titled
>>>> +	 * "Determination of DPC Control", to avoid conflicts over whether
>>>> +	 * platform firmware or the operating system have control of DPC,
>>>> +	 * it is recommended that platform firmware and operating systems
>>>> +	 * always link the control of DPC to the control of Advanced Error
>>>> +	 * Reporting.
>>>> +	 *
>>>> +	 * So use AER FF mode check API pcie_aer_get_firmware_first() to decide
>>>> +	 * whether DPC is controlled by software or firmware.
>>> AFAICT, this is not what the spec says.  Firmware-first is not what
>>> tells us whether DPC is controlled by firmware or the OS.  For ACPI
>>> systems, _OSC tells us whether the OS is allowed control of DPC.
>> But current DPC/AER driver use FIRMWARE_FIRST bit (in
>> pcie_aer_get_firmware_first()) determine firmware/OS ownership.
> I think that's a problem and we should fix it.  It's arguably a little
> bit of feature creep to do that in this series, but it makes it very
> confusing when the specs talk about ownership negotiated via _OSC and
> the code talks about "firmware-first" and makes assumptions about
> connections between the two.
>
> Those things may very well *be* related in the firmware
> implementation, but assuming those connections in Linux makes the code
> hard to read and maintain.  This series adds quite a bit of
> ACPI-related stuff, and I think we should untangle confusions like
> this before we add more.
>
> Bjorn
>
-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

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

[Bjorn Helgaas]

On Sat, Jan 18, 2020 at 08:00:33PM -0800, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
> As per ACPI specification r6.3, sec 5.6.6, 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 also let firmware know the recovery status via
> _OST ACPI call. Related _OST status codes can be found in ACPI
> specification r6.3, sec 6.3.5.2.
> 
> Also, as per PCI firmware specification r3.2 Downstream Port Containment
> Related Enhancements ECN, sec 4.5.1, table 4-6, If DPC is controlled by
> firmware (firmware first mode), firmware is responsible for
> configuring the DPC and OS is responsible for error recovery. Also, OS
> is allowed to modify DPC registers only during the EDR notification
> window. So with EDR support, OS should provide DPC port services even in
> firmware first mode.
> 
> Cc: Keith Busch <keith.busch@intel.com>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Acked-by: Keith Busch <keith.busch@intel.com>
> Tested-by: Huong Nguyen <huong.nguyen@dell.com>
> Tested-by: Austin Bolen <Austin.Bolen@dell.com>
> ---
>  drivers/pci/pci-acpi.c   |  99 +++++++++++++++++++++++++++++++
>  drivers/pci/pcie/Kconfig |  10 ++++
>  drivers/pci/pcie/dpc.c   | 123 ++++++++++++++++++++++++++++++++++++++-
>  include/linux/pci-acpi.h |  13 +++++
>  4 files changed, 244 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 0c02d500158f..920928f0d55c 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -103,6 +103,105 @@ int acpi_get_rc_resources(struct device *dev, const char *hid, u16 segment,
>  }
>  #endif
>  
> +#if defined(CONFIG_PCIE_DPC)
> +
> +/*
> + * _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.
> + */
> +int acpi_enable_dpc_port(struct pci_dev *pdev, acpi_handle handle, bool enable)
> +{
> +	union acpi_object *obj, argv4, req;
> +	int status = 0;
> +
> +	req.type = ACPI_TYPE_INTEGER;
> +	req.integer.value = enable;
> +
> +	argv4.type = ACPI_TYPE_PACKAGE;
> +	argv4.package.count = 1;
> +	argv4.package.elements = &req;
> +
> +	/*
> +	 * Per the Downstream Port Containment Related Enhancements ECN to
> +	 * the PCI Firmware Specification r3.2, sec 4.6.12,
> +	 * EDR_PORT_ENABLE_DSM is optional.  Return success if it's not
> +	 * implemented.
> +	 */
> +	obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, 5,
> +				EDR_PORT_ENABLE_DSM, &argv4);
> +	if (!obj)
> +		return 0;
> +
> +	if (obj->type != ACPI_TYPE_INTEGER || obj->integer.value != enable)
> +		status = -EIO;
> +
> +	ACPI_FREE(obj);
> +
> +	return status;
> +}
> +
> +/*
> + * _DSM wrapper function to locate DPC port.
> + * @dpc   : DPC device structure
> + *
> + * returns pci_dev or NULL.
> + */
> +struct pci_dev *acpi_locate_dpc_port(struct pci_dev *pdev, acpi_handle handle)
> +{
> +	union acpi_object *obj;
> +	u16 port;
> +
> +	obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, 5,
> +				EDR_PORT_LOCATE_DSM, NULL);
> +	if (!obj)
> +		return pci_dev_get(pdev);
> +
> +	if (obj->type != ACPI_TYPE_INTEGER) {
> +		ACPI_FREE(obj);
> +		return NULL;
> +	}
> +
> +	/*
> +	 * 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);
> +
> +	return pci_get_domain_bus_and_slot(pci_domain_nr(pdev->bus),
> +					   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.
> + */
> +int acpi_send_edr_status(struct pci_dev *pdev, acpi_handle handle, u16 status)
> +{
> +	u32 ost_status;
> +
> +	pci_dbg(pdev, "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(handle,
> +				   ACPI_NOTIFY_DISCONNECT_RECOVER,
> +				   ost_status, NULL);
> +	if (ACPI_FAILURE(status))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +#endif /* CONFIG_PCIE_DPC */

I think we should try collecting all this stuff into a new pcie/edr.c
file.  It could contain all the above, plus edr_handle_event() and
maybe something like a "pci_acpi_add_edr_notifier()" that could be
called from pci_acpi_setup() to install the notify handler.

I think the ACPI EDR stuff in dpc.c kind of clutters it up, especially
for the non-ACPI systems that use dpc.c.

Obviously this would require some sort of interface exported from
dpc.c to do the guts of edr_handle_event(), i.e., reading
PCI_EXP_DPC_STATUS and calling dpc_process_error().

>  phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle)
>  {
>  	acpi_status status = AE_NOT_EXIST;
> diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
> index 6e3c04b46fb1..772b1f4cb19e 100644
> --- a/drivers/pci/pcie/Kconfig
> +++ b/drivers/pci/pcie/Kconfig
> @@ -140,3 +140,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"
> +	depends on PCIE_DPC && ACPI
> +	help
> +	  This option adds Error Disconnect Recover support as specified
> +	  in the Downstream Port Containment Related Enhancements ECN to
> +	  the PCI Firmware Specification r3.2.  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 9f58e91f8852..8a8ee374d9b1 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -13,6 +13,8 @@
>  #include <linux/interrupt.h>
>  #include <linux/init.h>
>  #include <linux/pci.h>
> +#include <linux/acpi.h>
> +#include <linux/pci-acpi.h>
>  
>  #include "portdrv.h"
>  #include "../pci.h"
> @@ -23,6 +25,11 @@ struct dpc_dev {
>  	bool			rp_extensions;
>  	u8			rp_log_size;
>  	bool			edr_enabled; /* EDR mode is supported */
> +	pci_ers_result_t	error_state;
> +	struct mutex		edr_lock;
> +#ifdef CONFIG_ACPI
> +	struct acpi_device	*adev;
> +#endif
>  };
>  
>  static const char * const rp_pio_error_string[] = {
> @@ -161,6 +168,9 @@ static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
>  	if (!pcie_wait_for_link(pdev, true))
>  		return PCI_ERS_RESULT_DISCONNECT;
>  
> +	/* Since the device recovery is done just update the error state */
> +	dpc->error_state = PCI_ERS_RESULT_RECOVERED;
> +
>  	return PCI_ERS_RESULT_RECOVERED;
>  }
>  
> @@ -305,14 +315,94 @@ 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
> +
> +static void edr_handle_event(acpi_handle handle, u32 event, void *data)
> +{
> +	struct dpc_dev *dpc = data;
> +	struct pci_dev *pdev = dpc->dev->port;
> +	u16 status;
> +
> +	pci_info(pdev, "ACPI event %#x received\n", event);
> +
> +	if (event != ACPI_NOTIFY_DISCONNECT_RECOVER)
> +		return;
> +
> +	/*
> +	 * Check if _DSM(0xD) is available, and if present locate the
> +	 * port which issued EDR event.
> +	 */
> +	pdev = acpi_locate_dpc_port(pdev, dpc->adev->handle);
> +	if (!pdev) {
> +		pdev = dpc->dev->port;
> +		pci_err(pdev, "No valid port found\n");

This message should include the bus/device/function that we looked for.

> +		return;
> +	}
> +
> +	dpc = to_dpc_dev(pdev);
> +	if (!dpc) {
> +		pci_err(pdev, "DPC port is NULL\n");
> +		goto done;
> +	}
> +
> +	mutex_lock(&dpc->edr_lock);
> +
> +	dpc->error_state = PCI_ERS_RESULT_DISCONNECT;
> +
> +	/*
> +	 * Check if the port supports DPC:
> +	 *
> +	 * If port supports DPC, then fall back to default error
> +	 * recovery.
> +	 */
> +	if (dpc->cap_pos) {
> +		/* Check if there is a valid DPC trigger */
> +		pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_STATUS,
> +				     &status);
> +		if ((status & PCI_EXP_DPC_STATUS_TRIGGER))
> +			dpc_process_error(dpc);
> +		else
> +			pci_err(pdev, "Invalid DPC trigger %#010x\n", status);
> +	}
> +
> +	/*
> +	 * 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 = EDR_OST_SUCCESS;
> +	else
> +		status = EDR_OST_FAILED;
> +
> +	/* Use ACPI handle of DPC event device for sending EDR status */
> +	dpc = data;
> +
> +	acpi_send_edr_status(pdev, dpc->adev->handle, status);
> +
> +	mutex_unlock(&dpc->edr_lock);
> +done:
> +	pci_dev_put(pdev);
> +}
> +#endif
> +
>  #define FLAG(x, y) (((x) & (y)) ? '+' : '-')
>  static int dpc_probe(struct pcie_device *dev)
>  {
>  	struct dpc_dev *dpc;
>  	struct pci_dev *pdev = dev->port;
>  	struct device *device = &dev->device;
> -	int status;
> +	int status = 0;
>  	u16 ctl, cap;
> +#ifdef CONFIG_ACPI
> +	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
> +#endif
>  
>  	dpc = devm_kzalloc(device, sizeof(*dpc), GFP_KERNEL);
>  	if (!dpc)
> @@ -321,6 +411,9 @@ static int dpc_probe(struct pcie_device *dev)
>  	dpc->cap_pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DPC);
>  	dpc->dev = dev;
>  
> +	dpc->error_state = PCI_ERS_RESULT_NONE;
> +	mutex_init(&dpc->edr_lock);
> +
>  	/*
>  	 * As per PCIe r5.0, sec 6.2.10, implementation note titled
>  	 * "Determination of DPC Control", to avoid conflicts over whether
> @@ -358,6 +451,33 @@ static int dpc_probe(struct pcie_device *dev)
>  		}
>  	}
>  
> +#ifdef CONFIG_ACPI
> +	if (pcie_aer_get_firmware_first(pdev) && adev) {

I'm not convinced about using pcie_aer_get_firmware_first() here yet.
AFAICT the spec doesn't actually say anything like this (I'm looking
at the ECN sec 4.5.1 description of "Error Disconnect Recover
Supported".

> +		acpi_status astatus;
> +
> +		dpc->adev = adev;
> +
> +		astatus = acpi_install_notify_handler(adev->handle,
> +						      ACPI_SYSTEM_NOTIFY,
> +						      edr_handle_event,
> +						      dpc);

I think there are a couple issues with the ECN here:

  - The ECN allows EDR notifications on host bridges (sec 4.5.1, table
    4-4), but it does not allow the "Locate" _DSM under host bridges
    (sec 4.6.13).

  - The ECN allows EDR notifications on root ports or switch ports
    that do not support DPC (sec 4.5.1), but it does not allow the
    "Locate" _DSM on those ports (sec 4.6.13).

> +		if (ACPI_FAILURE(astatus)) {
> +			pci_err(pdev,
> +				"Install ACPI_SYSTEM_NOTIFY handler failed\n");
> +			return -EBUSY;
> +		}
> +
> +		status = acpi_enable_dpc_port(pdev, adev->handle, true);
> +		if (status) {
> +			pci_warn(pdev, "Enable DPC port failed\n");
> +			acpi_remove_notify_handler(adev->handle,
> +						   ACPI_SYSTEM_NOTIFY,
> +						   edr_handle_event);
> +			return status;
> +		}
> +	}
> +#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);
>  
> @@ -409,6 +529,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/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index 62b7fdcc661c..f4e0d5d984b0 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -111,6 +111,19 @@ extern const guid_t pci_acpi_dsm_guid;
>  #define DEVICE_LABEL_DSM		0x07
>  #define RESET_DELAY_DSM			0x08
>  #define FUNCTION_DELAY_DSM		0x09
> +#ifdef CONFIG_PCIE_DPC
> +#define EDR_PORT_ENABLE_DSM		0x0C
> +#define EDR_PORT_LOCATE_DSM		0x0D
> +#define EDR_OST_SUCCESS			0x80
> +#define EDR_OST_FAILED			0x81
> +
> +int acpi_enable_dpc_port(struct pci_dev *pdev, acpi_handle handle,
> +			 bool enable);
> +struct pci_dev *acpi_locate_dpc_port(struct pci_dev *pdev,
> +				     acpi_handle handle);
> +int acpi_send_edr_status(struct pci_dev *pdev,
> +			 acpi_handle handle, u16 status);
> +#endif /* CONFIG_PCIE_DPC */
>  
>  #else	/* CONFIG_ACPI */
>  static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
> -- 
> 2.21.0
> 

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

[Kuppuswamy Sathyanarayanan]

On Fri, Jan 24, 2020 at 09:04:50AM -0600, Bjorn Helgaas wrote:
> On Sat, Jan 18, 2020 at 08:00:33PM -0800, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > 
> > As per ACPI specification r6.3, sec 5.6.6, 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 also let firmware know the recovery status via
> > _OST ACPI call. Related _OST status codes can be found in ACPI
> > specification r6.3, sec 6.3.5.2.
> > 
> > Also, as per PCI firmware specification r3.2 Downstream Port Containment
> > Related Enhancements ECN, sec 4.5.1, table 4-6, If DPC is controlled by
> > firmware (firmware first mode), firmware is responsible for
> > configuring the DPC and OS is responsible for error recovery. Also, OS
> > is allowed to modify DPC registers only during the EDR notification
> > window. So with EDR support, OS should provide DPC port services even in
> > firmware first mode.
> > 
> > Cc: Keith Busch <keith.busch@intel.com>
> > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > Acked-by: Keith Busch <keith.busch@intel.com>
> > Tested-by: Huong Nguyen <huong.nguyen@dell.com>
> > Tested-by: Austin Bolen <Austin.Bolen@dell.com>
> > ---
> >  drivers/pci/pci-acpi.c   |  99 +++++++++++++++++++++++++++++++
> >  drivers/pci/pcie/Kconfig |  10 ++++
> >  drivers/pci/pcie/dpc.c   | 123 ++++++++++++++++++++++++++++++++++++++-
> >  include/linux/pci-acpi.h |  13 +++++
> >  4 files changed, 244 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > index 0c02d500158f..920928f0d55c 100644
> > --- a/drivers/pci/pci-acpi.c
> > +++ b/drivers/pci/pci-acpi.c
> > @@ -103,6 +103,105 @@ int acpi_get_rc_resources(struct device *dev, const char *hid, u16 segment,
> >  }
> >  #endif
> >  
> > +#if defined(CONFIG_PCIE_DPC)
> > +
> > +/*
> > + * _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.
> > + */
> > +int acpi_enable_dpc_port(struct pci_dev *pdev, acpi_handle handle, bool enable)
> > +{
> > +	union acpi_object *obj, argv4, req;
> > +	int status = 0;
> > +
> > +	req.type = ACPI_TYPE_INTEGER;
> > +	req.integer.value = enable;
> > +
> > +	argv4.type = ACPI_TYPE_PACKAGE;
> > +	argv4.package.count = 1;
> > +	argv4.package.elements = &req;
> > +
> > +	/*
> > +	 * Per the Downstream Port Containment Related Enhancements ECN to
> > +	 * the PCI Firmware Specification r3.2, sec 4.6.12,
> > +	 * EDR_PORT_ENABLE_DSM is optional.  Return success if it's not
> > +	 * implemented.
> > +	 */
> > +	obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, 5,
> > +				EDR_PORT_ENABLE_DSM, &argv4);
> > +	if (!obj)
> > +		return 0;
> > +
> > +	if (obj->type != ACPI_TYPE_INTEGER || obj->integer.value != enable)
> > +		status = -EIO;
> > +
> > +	ACPI_FREE(obj);
> > +
> > +	return status;
> > +}
> > +
> > +/*
> > + * _DSM wrapper function to locate DPC port.
> > + * @dpc   : DPC device structure
> > + *
> > + * returns pci_dev or NULL.
> > + */
> > +struct pci_dev *acpi_locate_dpc_port(struct pci_dev *pdev, acpi_handle handle)
> > +{
> > +	union acpi_object *obj;
> > +	u16 port;
> > +
> > +	obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, 5,
> > +				EDR_PORT_LOCATE_DSM, NULL);
> > +	if (!obj)
> > +		return pci_dev_get(pdev);
> > +
> > +	if (obj->type != ACPI_TYPE_INTEGER) {
> > +		ACPI_FREE(obj);
> > +		return NULL;
> > +	}
> > +
> > +	/*
> > +	 * 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);
> > +
> > +	return pci_get_domain_bus_and_slot(pci_domain_nr(pdev->bus),
> > +					   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.
> > + */
> > +int acpi_send_edr_status(struct pci_dev *pdev, acpi_handle handle, u16 status)
> > +{
> > +	u32 ost_status;
> > +
> > +	pci_dbg(pdev, "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(handle,
> > +				   ACPI_NOTIFY_DISCONNECT_RECOVER,
> > +				   ost_status, NULL);
> > +	if (ACPI_FAILURE(status))
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +#endif /* CONFIG_PCIE_DPC */
> 
> I think we should try collecting all this stuff into a new pcie/edr.c
> file.  It could contain all the above, plus edr_handle_event() and
> maybe something like a "pci_acpi_add_edr_notifier()" that could be
> called from pci_acpi_setup() to install the notify handler.
> 
> I think the ACPI EDR stuff in dpc.c kind of clutters it up, especially
> for the non-ACPI systems that use dpc.c.
> 
> Obviously this would require some sort of interface exported from
> dpc.c to do the guts of edr_handle_event(), i.e., reading
> PCI_EXP_DPC_STATUS and calling dpc_process_error().
Ok. Let me move all EDR related code to edr.c. But, this might also 
require me to export a new interface similar to pcie_do_recovery
function. Since this function has dependency on port service
from which is its called, I might need to create a version without
this dependency.
> 
> >  phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle)
> >  {
> >  	acpi_status status = AE_NOT_EXIST;
> > diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
> > index 6e3c04b46fb1..772b1f4cb19e 100644
> > --- a/drivers/pci/pcie/Kconfig
> > +++ b/drivers/pci/pcie/Kconfig
> > @@ -140,3 +140,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"
> > +	depends on PCIE_DPC && ACPI
> > +	help
> > +	  This option adds Error Disconnect Recover support as specified
> > +	  in the Downstream Port Containment Related Enhancements ECN to
> > +	  the PCI Firmware Specification r3.2.  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 9f58e91f8852..8a8ee374d9b1 100644
> > --- a/drivers/pci/pcie/dpc.c
> > +++ b/drivers/pci/pcie/dpc.c
> > @@ -13,6 +13,8 @@
> >  #include <linux/interrupt.h>
> >  #include <linux/init.h>
> >  #include <linux/pci.h>
> > +#include <linux/acpi.h>
> > +#include <linux/pci-acpi.h>
> >  
> >  #include "portdrv.h"
> >  #include "../pci.h"
> > @@ -23,6 +25,11 @@ struct dpc_dev {
> >  	bool			rp_extensions;
> >  	u8			rp_log_size;
> >  	bool			edr_enabled; /* EDR mode is supported */
> > +	pci_ers_result_t	error_state;
> > +	struct mutex		edr_lock;
> > +#ifdef CONFIG_ACPI
> > +	struct acpi_device	*adev;
> > +#endif
> >  };
> >  
> >  static const char * const rp_pio_error_string[] = {
> > @@ -161,6 +168,9 @@ static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
> >  	if (!pcie_wait_for_link(pdev, true))
> >  		return PCI_ERS_RESULT_DISCONNECT;
> >  
> > +	/* Since the device recovery is done just update the error state */
> > +	dpc->error_state = PCI_ERS_RESULT_RECOVERED;
> > +
> >  	return PCI_ERS_RESULT_RECOVERED;
> >  }
> >  
> > @@ -305,14 +315,94 @@ 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
> > +
> > +static void edr_handle_event(acpi_handle handle, u32 event, void *data)
> > +{
> > +	struct dpc_dev *dpc = data;
> > +	struct pci_dev *pdev = dpc->dev->port;
> > +	u16 status;
> > +
> > +	pci_info(pdev, "ACPI event %#x received\n", event);
> > +
> > +	if (event != ACPI_NOTIFY_DISCONNECT_RECOVER)
> > +		return;
> > +
> > +	/*
> > +	 * Check if _DSM(0xD) is available, and if present locate the
> > +	 * port which issued EDR event.
> > +	 */
> > +	pdev = acpi_locate_dpc_port(pdev, dpc->adev->handle);
> > +	if (!pdev) {
> > +		pdev = dpc->dev->port;
> > +		pci_err(pdev, "No valid port found\n");
> 
i> This message should include the bus/device/function that we looked for.
Ok. I will add this in next version.
> 
> > +		return;
> > +	}
> > +
> > +	dpc = to_dpc_dev(pdev);
> > +	if (!dpc) {
> > +		pci_err(pdev, "DPC port is NULL\n");
> > +		goto done;
> > +	}
> > +
> > +	mutex_lock(&dpc->edr_lock);
> > +
> > +	dpc->error_state = PCI_ERS_RESULT_DISCONNECT;
> > +
> > +	/*
> > +	 * Check if the port supports DPC:
> > +	 *
> > +	 * If port supports DPC, then fall back to default error
> > +	 * recovery.
> > +	 */
> > +	if (dpc->cap_pos) {
> > +		/* Check if there is a valid DPC trigger */
> > +		pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_STATUS,
> > +				     &status);
> > +		if ((status & PCI_EXP_DPC_STATUS_TRIGGER))
> > +			dpc_process_error(dpc);
> > +		else
> > +			pci_err(pdev, "Invalid DPC trigger %#010x\n", status);
> > +	}
> > +
> > +	/*
> > +	 * 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 = EDR_OST_SUCCESS;
> > +	else
> > +		status = EDR_OST_FAILED;
> > +
> > +	/* Use ACPI handle of DPC event device for sending EDR status */
> > +	dpc = data;
> > +
> > +	acpi_send_edr_status(pdev, dpc->adev->handle, status);
> > +
> > +	mutex_unlock(&dpc->edr_lock);
> > +done:
> > +	pci_dev_put(pdev);
> > +}
> > +#endif
> > +
> >  #define FLAG(x, y) (((x) & (y)) ? '+' : '-')
> >  static int dpc_probe(struct pcie_device *dev)
> >  {
> >  	struct dpc_dev *dpc;
> >  	struct pci_dev *pdev = dev->port;
> >  	struct device *device = &dev->device;
> > -	int status;
> > +	int status = 0;
> >  	u16 ctl, cap;
> > +#ifdef CONFIG_ACPI
> > +	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
> > +#endif
> >  
> >  	dpc = devm_kzalloc(device, sizeof(*dpc), GFP_KERNEL);
> >  	if (!dpc)
> > @@ -321,6 +411,9 @@ static int dpc_probe(struct pcie_device *dev)
> >  	dpc->cap_pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DPC);
> >  	dpc->dev = dev;
> >  
> > +	dpc->error_state = PCI_ERS_RESULT_NONE;
> > +	mutex_init(&dpc->edr_lock);
> > +
> >  	/*
> >  	 * As per PCIe r5.0, sec 6.2.10, implementation note titled
> >  	 * "Determination of DPC Control", to avoid conflicts over whether
> > @@ -358,6 +451,33 @@ static int dpc_probe(struct pcie_device *dev)
> >  		}
> >  	}
> >  
> > +#ifdef CONFIG_ACPI
> > +	if (pcie_aer_get_firmware_first(pdev) && adev) {
> 
> I'm not convinced about using pcie_aer_get_firmware_first() here yet.
> AFAICT the spec doesn't actually say anything like this (I'm looking
> at the ECN sec 4.5.1 description of "Error Disconnect Recover
> Supported".
If we are moving all EDR related code to edr.c, we will not need this
change.
> 
> > +		acpi_status astatus;
> > +
> > +		dpc->adev = adev;
> > +
> > +		astatus = acpi_install_notify_handler(adev->handle,
> > +						      ACPI_SYSTEM_NOTIFY,
> > +						      edr_handle_event,
> > +						      dpc);
> 
> I think there are a couple issues with the ECN here:
> 
>   - The ECN allows EDR notifications on host bridges (sec 4.5.1, table
>     4-4), but it does not allow the "Locate" _DSM under host bridges
>     (sec 4.6.13).
> 
>   - The ECN allows EDR notifications on root ports or switch ports
>     that do not support DPC (sec 4.5.1), but it does not allow the
>     "Locate" _DSM on those ports (sec 4.6.13).

Can you please give more details on where its mentioned? Following is
the copy of "Locate" _DSM location related specification. All it says is,
this object can be placed under any object representing root port or
switch port. It does not seem to add any restrictions. Please let me  know
your comments.

Location:
This object can be placed under any object representing a DPC capable
PCI Express Root Port or Switch Downstream Port. If a port implements
this DSM, its child devices cannot instantiate this DSM function

> 
> > +		if (ACPI_FAILURE(astatus)) {
> > +			pci_err(pdev,
> > +				"Install ACPI_SYSTEM_NOTIFY handler failed\n");
> > +			return -EBUSY;
> > +		}
> > +
> > +		status = acpi_enable_dpc_port(pdev, adev->handle, true);
> > +		if (status) {
> > +			pci_warn(pdev, "Enable DPC port failed\n");
> > +			acpi_remove_notify_handler(adev->handle,
> > +						   ACPI_SYSTEM_NOTIFY,
> > +						   edr_handle_event);
> > +			return status;
> > +		}
> > +	}
> > +#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);
> >  
> > @@ -409,6 +529,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/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> > index 62b7fdcc661c..f4e0d5d984b0 100644
> > --- a/include/linux/pci-acpi.h
> > +++ b/include/linux/pci-acpi.h
> > @@ -111,6 +111,19 @@ extern const guid_t pci_acpi_dsm_guid;
> >  #define DEVICE_LABEL_DSM		0x07
> >  #define RESET_DELAY_DSM			0x08
> >  #define FUNCTION_DELAY_DSM		0x09
> > +#ifdef CONFIG_PCIE_DPC
> > +#define EDR_PORT_ENABLE_DSM		0x0C
> > +#define EDR_PORT_LOCATE_DSM		0x0D
> > +#define EDR_OST_SUCCESS			0x80
> > +#define EDR_OST_FAILED			0x81
> > +
> > +int acpi_enable_dpc_port(struct pci_dev *pdev, acpi_handle handle,
> > +			 bool enable);
> > +struct pci_dev *acpi_locate_dpc_port(struct pci_dev *pdev,
> > +				     acpi_handle handle);
> > +int acpi_send_edr_status(struct pci_dev *pdev,
> > +			 acpi_handle handle, u16 status);
> > +#endif /* CONFIG_PCIE_DPC */
> >  
> >  #else	/* CONFIG_ACPI */
> >  static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
> > -- 
> > 2.21.0
> > 

-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

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

[Bjorn Helgaas]

On Sat, Jan 25, 2020 at 03:25:00PM -0800, Kuppuswamy Sathyanarayanan wrote:
> On Fri, Jan 24, 2020 at 09:04:50AM -0600, Bjorn Helgaas wrote:
> > On Sat, Jan 18, 2020 at 08:00:33PM -0800, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > 
> > > As per ACPI specification r6.3, sec 5.6.6, 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 also let firmware know the recovery status via
> > > _OST ACPI call. Related _OST status codes can be found in ACPI
> > > specification r6.3, sec 6.3.5.2.
> > > 
> > > Also, as per PCI firmware specification r3.2 Downstream Port Containment
> > > Related Enhancements ECN, sec 4.5.1, table 4-6, If DPC is controlled by
> > > firmware (firmware first mode), firmware is responsible for
> > > configuring the DPC and OS is responsible for error recovery. Also, OS
> > > is allowed to modify DPC registers only during the EDR notification
> > > window. So with EDR support, OS should provide DPC port services even in
> > > firmware first mode.
> ...

> > > +		acpi_status astatus;
> > > +
> > > +		dpc->adev = adev;
> > > +
> > > +		astatus = acpi_install_notify_handler(adev->handle,
> > > +						      ACPI_SYSTEM_NOTIFY,
> > > +						      edr_handle_event,
> > > +						      dpc);
> > 
> > I think there are a couple issues with the ECN here:
> > 
> >   - The ECN allows EDR notifications on host bridges (sec 4.5.1, table
> >     4-4), but it does not allow the "Locate" _DSM under host bridges
> >     (sec 4.6.13).
> > 
> >   - The ECN allows EDR notifications on root ports or switch ports
> >     that do not support DPC (sec 4.5.1), but it does not allow the
> >     "Locate" _DSM on those ports (sec 4.6.13).
> 
> Can you please give more details on where its mentioned? Following is
> the copy of "Locate" _DSM location related specification. All it says is,
> this object can be placed under any object representing root port or
> switch port. It does not seem to add any restrictions. Please let me  know
> your comments.
> 
> Location:
> This object can be placed under any object representing a DPC capable
> PCI Express Root Port or Switch Downstream Port. If a port implements
> this DSM, its child devices cannot instantiate this DSM function

You quoted it: "This object [the 'Locate' _DSM] can be placed under
any object representing a *DPC capable* PCI Express Root Port or Switch
Downstream Port."

If the intention was to allow the Locate _DSM for *any* root ports or
switch downstream ports, the spec should not include the "DPC capable"
restriction.

Bjorn

Re: [PATCH v13 1/8] PCI/ERR: Update error status after reset_link()

[Kuppuswamy Sathyanarayanan]

Hi Bjorn,

On Sat, Jan 18, 2020 at 08:00:30PM -0800, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
> Commit bdb5ac85777d ("PCI/ERR: Handle fatal error recovery") uses
> reset_link() to recover from fatal errors. But during fatal error
> recovery, if the initial value of error status is
> PCI_ERS_RESULT_DISCONNECT or PCI_ERS_RESULT_NO_AER_DRIVER then
> even after successful recovery (using reset_link()) pcie_do_recovery()
> will report the recovery result as failure. So update the status of
> error after reset_link().

Since this patch has no dependency on EDR, can we merge it first?

> 
> Fixes: bdb5ac85777d ("PCI/ERR: Handle fatal error recovery")
> Cc: Ashok Raj <ashok.raj@intel.com>
> Cc: Keith Busch <keith.busch@intel.com>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Acked-by: Keith Busch <keith.busch@intel.com>
> ---
>  drivers/pci/pcie/err.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> index b0e6048a9208..71639055511e 100644
> --- a/drivers/pci/pcie/err.c
> +++ b/drivers/pci/pcie/err.c
> @@ -204,9 +204,11 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
>  	else
>  		pci_walk_bus(bus, report_normal_detected, &status);
>  
> -	if (state == pci_channel_io_frozen &&
> -	    reset_link(dev, service) != PCI_ERS_RESULT_RECOVERED)
> -		goto failed;
> +	if (state == pci_channel_io_frozen) {
> +		status = reset_link(dev, service);
> +		if (status != PCI_ERS_RESULT_RECOVERED)
> +			goto failed;
> +	}
>  
>  	if (status == PCI_ERS_RESULT_CAN_RECOVER) {
>  		status = PCI_ERS_RESULT_RECOVERED;
> -- 
> 2.21.0
> 

-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

Re: [PATCH v13 1/8] PCI/ERR: Update error status after reset_link()

[Bjorn Helgaas]

On Wed, Feb 05, 2020 at 10:28:00AM -0800, Kuppuswamy Sathyanarayanan wrote:
> Hi Bjorn,
> 
> On Sat, Jan 18, 2020 at 08:00:30PM -0800, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > 
> > Commit bdb5ac85777d ("PCI/ERR: Handle fatal error recovery") uses
> > reset_link() to recover from fatal errors. But during fatal error
> > recovery, if the initial value of error status is
> > PCI_ERS_RESULT_DISCONNECT or PCI_ERS_RESULT_NO_AER_DRIVER then
> > even after successful recovery (using reset_link()) pcie_do_recovery()
> > will report the recovery result as failure. So update the status of
> > error after reset_link().
> 
> Since this patch has no dependency on EDR, can we merge it first?

We *could*, but I don't think there's any benefit.  bdb5ac85777d
appeared in v4.20, so this wouldn't really be a candidate for v5.6.

I'm expecting (hoping, anyway) that we'll merge this whole series for
v5.7.  For minor bugfixes like this one, we should add stable tags so
they'll get backported.

> > Fixes: bdb5ac85777d ("PCI/ERR: Handle fatal error recovery")
> > Cc: Ashok Raj <ashok.raj@intel.com>
> > Cc: Keith Busch <keith.busch@intel.com>
> > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > Acked-by: Keith Busch <keith.busch@intel.com>
> > ---
> >  drivers/pci/pcie/err.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> > index b0e6048a9208..71639055511e 100644
> > --- a/drivers/pci/pcie/err.c
> > +++ b/drivers/pci/pcie/err.c
> > @@ -204,9 +204,11 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
> >  	else
> >  		pci_walk_bus(bus, report_normal_detected, &status);
> >  
> > -	if (state == pci_channel_io_frozen &&
> > -	    reset_link(dev, service) != PCI_ERS_RESULT_RECOVERED)
> > -		goto failed;
> > +	if (state == pci_channel_io_frozen) {
> > +		status = reset_link(dev, service);
> > +		if (status != PCI_ERS_RESULT_RECOVERED)
> > +			goto failed;
> > +	}
> >  
> >  	if (status == PCI_ERS_RESULT_CAN_RECOVER) {
> >  		status = PCI_ERS_RESULT_RECOVERED;
> > -- 
> > 2.21.0
> > 
> 
> -- 
> Sathyanarayanan Kuppuswamy
> Linux kernel developer