Add Error Disconnect Recover (EDR) support

From: Kuppuswamy Sathyanarayanan <[email protected]>

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 <[email protected]>

Cc Keith Busch <[email protected]>

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

Acked-by Keith Busch <[email protected]>

---
 drivers/pci/pcie/err.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

drivers/pci/pcie/err.c +5 -3

index b0e6048a9208..71639055511e 100644
@@ -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;

Signature · 1 line

2.21.0

Hi Bjorn,

Quoted 9 lines · On Sat, Jan 18, 2020 at 08:00:30PM -0800…

From: Kuppuswamy Sathyanarayanan <[email protected]>

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?

Quoted 31 lines

Fixes bdb5ac85777d ("PCI/ERR: Handle fatal error recovery")

Cc Ashok Raj <[email protected]>

Cc Keith Busch <[email protected]>

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

Acked-by Keith Busch <[email protected]>

---
 drivers/pci/pcie/err.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

drivers/pci/pcie/err.c +5 -3

index b0e6048a9208..71639055511e 100644
@@ -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;

Signature · 1 line

2.21.0

Signature · 2 lines

Sathyanarayanan Kuppuswamy
Linux kernel developer

From: Kuppuswamy Sathyanarayanan <[email protected]>

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 <[email protected]>

Acked-by Keith Busch <[email protected]>

---
 drivers/pci/pcie/dpc.c | 74 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 61 insertions(+), 13 deletions(-)

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

index e06f42f58d3d..c583a90fa90d 100644

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

 };
 
 static const char * const rp_pio_error_string[] = {

drivers/pci/pcie/dpc.c +9 -3

 	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;
+

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

+	 * 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;
+		}
 	}
 

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

 	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);

Signature · 1 line

2.21.0

Quoted 6 lines · On Sat, Jan 18, 2020 at 08:00:31PM -0800…

From: Kuppuswamy Sathyanarayanan <[email protected]>

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.

Quoted 5 lines

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.

Quoted 20 lines

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 <[email protected]>

Acked-by Keith Busch <[email protected]>

---
  drivers/pci/pcie/dpc.c | 74 ++++++++++++++++++++++++++++++++++--------
  1 file changed, 61 insertions(+), 13 deletions(-)

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

Quoted 9 lines

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

index e06f42f58d3d..c583a90fa90d 100644
@@ -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.

Quoted 4 lines

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

  };
 
  static const char * const rp_pio_error_string[] = {

drivers/pci/pcie/dpc.c void pci_save_dpc_state(struct pci_dev *dev) · line 79

Quoted 10 lines

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

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

Quoted 29 lines

drivers/pci/pcie/dpc.c +8 -3

  	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;
 

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

Quoted 13 lines

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

  	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.

Quoted 66 lines

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

+	 * 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);

Signature · 1 line

2.21.0

Hi Bjorn,

Quoted 29 lines · Bjorn Helgaas

Quoted 6 lines · On Sat, Jan 18, 2020 at 08:00:31PM -0800…

From: Kuppuswamy Sathyanarayanan <[email protected]>

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.

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 19 lines

Quoted 19 lines

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 <[email protected]>

Acked-by Keith Busch <[email protected]>

---
  drivers/pci/pcie/dpc.c | 74 ++++++++++++++++++++++++++++++++++--------
  1 file changed, 61 insertions(+), 13 deletions(-)

Quoted 10 lines

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

Quoted 9 lines

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

index e06f42f58d3d..c583a90fa90d 100644
@@ -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 ?

Quoted 4 lines

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.

Quoted 24 lines

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.

drivers/pci/pcie/dpc.c void pci_save_dpc_state(struct pci_dev *dev) · line 78

Quoted 12 lines

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

  };
  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.

Quoted 6 lines

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.

Quoted 6 lines

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.

Quoted 5 lines

Quoted 5 lines

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

+	 * 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

Quoted 8 lines

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

Quoted 4 lines

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

+	 * 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.

Quoted 32 lines · Kuppuswamy Sathyanarayanan

Hi Bjorn,

Quoted 29 lines · Bjorn Helgaas

Quoted 6 lines · On Sat, Jan 18, 2020 at 08:00:31PM -0800…

From: Kuppuswamy Sathyanarayanan <[email protected]>

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.

Quoted 5 lines

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.

Quoted 8 lines

Quoted 3 lines

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.

Quoted 34 lines

Quoted 19 lines

Quoted 19 lines

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 <[email protected]>

Acked-by Keith Busch <[email protected]>

---
   drivers/pci/pcie/dpc.c | 74 ++++++++++++++++++++++++++++++++++--------
   1 file changed, 61 insertions(+), 13 deletions(-)

Quoted 10 lines

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

Quoted 9 lines

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

index e06f42f58d3d..c583a90fa90d 100644
@@ -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 ?

Quoted 4 lines

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.

Quoted 8 lines

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.

Quoted 37 lines

Quoted 25 lines

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.

drivers/pci/pcie/dpc.c void pci_save_dpc_state(struct pci_dev *dev) · line 78

Quoted 12 lines

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

   };
   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.

Quoted 6 lines

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.

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 13 lines

Quoted 13 lines

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

Quoted 10 lines

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

+	 * 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.

Quoted 2 lines

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

From: Kuppuswamy Sathyanarayanan <[email protected]>

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 <[email protected]>

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

Acked-by Keith Busch <[email protected]>

Tested-by Huong Nguyen <[email protected]>

Tested-by Austin Bolen <[email protected]>

---
 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(-)

drivers/pci/pci-acpi.c +88 -0

index 0c02d500158f..920928f0d55c 100644
@@ -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;

drivers/pci/pci-acpi.c +0 -0

 phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle)
 {
 	acpi_status status = AE_NOT_EXIST;

drivers/pci/pcie/Kconfig +10 -0

index 6e3c04b46fb1..772b1f4cb19e 100644
@@ -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.

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

index 9f58e91f8852..8a8ee374d9b1 100644
@@ -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)

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

+		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

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

+		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)

include/linux/pci-acpi.h +13 -0

index 62b7fdcc661c..f4e0d5d984b0 100644
@@ -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) { }

Signature · 1 line

2.21.0

Quoted 126 lines · On Sat, Jan 18, 2020 at 08:00:33PM -0800…

From: Kuppuswamy Sathyanarayanan <[email protected]>

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 <[email protected]>

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

Acked-by Keith Busch <[email protected]>

Tested-by Huong Nguyen <[email protected]>

Tested-by Austin Bolen <[email protected]>

---
 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(-)

drivers/pci/pci-acpi.c +88 -0

index 0c02d500158f..920928f0d55c 100644
@@ -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;

drivers/pci/pci-acpi.c int acpi_get_rc_resources(struct device *dev, const char *hid, u16 segment, · line 203

Quoted 10 lines

drivers/pci/pci-acpi.c +10 -0

+
+	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().

Quoted 81 lines

drivers/pci/pci-acpi.c +0 -0

 phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle)
 {
 	acpi_status status = AE_NOT_EXIST;

drivers/pci/pcie/Kconfig +10 -0

index 6e3c04b46fb1..772b1f4cb19e 100644
@@ -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.

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

index 9f58e91f8852..8a8ee374d9b1 100644
@@ -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;
+

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

Quoted 8 lines

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

+	/*
+	 * 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");

Quoted 79 lines

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

+		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)
 		}
 	}
 

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

Quoted 2 lines

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

+#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".

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

Quoted 9 lines

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

+		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).

Quoted 55 lines

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

+		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)

include/linux/pci-acpi.h +13 -0

index 62b7fdcc661c..f4e0d5d984b0 100644
@@ -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) { }

Signature · 1 line

2.21.0

Quoted 18 lines · Bjorn Helgaas

Quoted 17 lines · On Sat, Jan 18, 2020 at 08:00:33PM -0800…

From: Kuppuswamy Sathyanarayanan <[email protected]>

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.

...

Quoted 11 lines

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

Quoted 8 lines

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

+		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:

Quoted 7 lines

- 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