Add Error Disconnect Recover (EDR) support

From: Kuppuswamy Sathyanarayanan <[email protected]>

If hotplug is supported, during DPC event, hotplug driver would remove the affected devices and detach the drivers on DLLSC link down event and will re-enumerate it once the DPC recovery is handled and link comes back online (on DLLSC LINK up event). Hence we don't depend on .mmio_enabled or .slot_reset callbacks in error recovery handler to restore the device.

But if hotplug is not supported/enabled, then we need to let the error recovery handler attempt
the recovery of the devices using slot reset.

So if hotplug is not supported, then instead of returning PCI_ERS_RESULT_RECOVERED, return PCI_ERS_RESULT_NEED_RESET.

Also modify the way error recovery handler processes the recovery value.

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

---
 drivers/pci/pcie/dpc.c | 8 ++++++++
 drivers/pci/pcie/err.c | 5 +++--
 2 files changed, 11 insertions(+), 2 deletions(-)

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

index e06f42f58d3d..0e356ed0d73f 100644
@@ -13,6 +13,7 @@
 #include <linux/interrupt.h>
 #include <linux/init.h>
 #include <linux/pci.h>
+#include <linux/pci_hotplug.h>
 
 #include "portdrv.h"
 #include "../pci.h"

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

 	return PCI_ERS_RESULT_RECOVERED;
 }
 

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

index 1ac57e9e1e71..6e52591a4722 100644
@@ -178,7 +178,8 @@ static pci_ers_result_t reset_link(struct pci_dev *dev, u32 service)
 		return PCI_ERS_RESULT_DISCONNECT;
 	}
 
-	if (status != PCI_ERS_RESULT_RECOVERED) {
+	if ((status != PCI_ERS_RESULT_RECOVERED) &&
+	    (status != PCI_ERS_RESULT_NEED_RESET)) {
 		pci_printk(KERN_DEBUG, dev, "link reset at upstream device %s failed\n",
 			pci_name(dev));
 		return PCI_ERS_RESULT_DISCONNECT;

@@ -206,7 +207,7 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
 	if (state == pci_channel_io_frozen) {
 		pci_walk_bus(bus, report_frozen_detected, &status);
 		status = reset_link(dev, service);
-		if (status != PCI_ERS_RESULT_RECOVERED)
+		if (status == PCI_ERS_RESULT_DISCONNECT)
 			goto failed;
 	} else {
 		pci_walk_bus(bus, report_normal_detected, &status);

Signature · 1 line

2.17.1

I don't understand why hotplug is relevant here. This path (dpc_reset_link()) is only used for downstream ports that support DPC. DPC has already disabled the link, which resets everything below the port, regardless of whether the port supports hotplug.

I do see that PCI_ERS_RESULT_NEED_RESET seems to promise a lot more than it actually*does*. The doc (pci-error-recovery.rst) says .error_detected() can return PCI_ERS_RESULT_NEED_RESET to*request* a slot reset. But if that happens, pcie_do_recovery() doesn't do a reset at all. It calls the driver's .slot_reset() method, which tells the driver "we've reset your device; please re-initialize the hardware."

I guess this abuses PCI_ERS_RESULT_NEED_RESET by taking advantage of that implementation deficiency in pcie_do_recovery(): we know the downstream devices have already been reset via DPC, and returning PCI_ERS_RESULT_NEED_RESET means we'll call .slot_reset() to tell the driver about that reset.

I can see how this achieves the desired result, but if/when we fix pcie_do_recovery() to actually*do* the reset promised by PCI_ERS_RESULT_NEED_RESET, we will be doing*two* resets: the first via DPC and a second via whatever slot reset mechanism pcie_do_recovery() would use.

So I guess the real issue (as you allude to in the commit log) is that we rely on hotplug to unbind/rebind the driver, and without hotplug we need to at least tell the driver the device was reset.

I'll try to expand the comment here so it reminds me what's going on
when we have to look at this again:) Let me know if I'm on the right
track.

Quoted 19 lines · Bjorn Helgaas

I don't understand why hotplug is relevant here. This path (dpc_reset_link()) is only used for downstream ports that support DPC. DPC has already disabled the link, which resets everything below the port, regardless of whether the port supports hotplug.

I do see that PCI_ERS_RESULT_NEED_RESET seems to promise a lot more than it actually *does*. The doc (pci-error-recovery.rst) says .error_detected() can return PCI_ERS_RESULT_NEED_RESET to *request* a slot reset. But if that happens, pcie_do_recovery() doesn't do a reset at all. It calls the driver's .slot_reset() method, which tells the driver "we've reset your device; please re-initialize the hardware."

I guess this abuses PCI_ERS_RESULT_NEED_RESET by taking advantage of that implementation deficiency in pcie_do_recovery(): we know the downstream devices have already been reset via DPC, and returning PCI_ERS_RESULT_NEED_RESET means we'll call .slot_reset() to tell the driver about that reset.

Quoted 5 lines

I can see how this achieves the desired result, but if/when we fix pcie_do_recovery() to actually *do* the reset promised by PCI_ERS_RESULT_NEED_RESET, we will be doing *two* resets: the first via DPC and a second via whatever slot reset mechanism pcie_do_recovery() would use.

When we fix this issue, if we make sure the reset logic is implemented before we call .reset_link callback we should be able to avoid resetting the device twice. Before we call DPC .reset_link callback, the device link will not up and hence we should not able to reset it.

Quoted 3 lines

So I guess the real issue (as you allude to in the commit log) is that we rely on hotplug to unbind/rebind the driver, and without hotplug we need to at least tell the driver the device was reset.

Agree

Quoted 3 lines

I'll try to expand the comment here so it reminds me what's going on
when we have to look at this again:) Let me know if I'm on the right
track.

Yes, your understanding is correct.

Quoted 42 lines · Sathyanarayanan

Quoted 19 lines · Bjorn Helgaas

I don't understand why hotplug is relevant here. This path (dpc_reset_link()) is only used for downstream ports that support DPC. DPC has already disabled the link, which resets everything below the port, regardless of whether the port supports hotplug.

I do see that PCI_ERS_RESULT_NEED_RESET seems to promise a lot more than it actually *does*. The doc (pci-error-recovery.rst) says .error_detected() can return PCI_ERS_RESULT_NEED_RESET to *request* a slot reset. But if that happens, pcie_do_recovery() doesn't do a reset at all. It calls the driver's .slot_reset() method, which tells the driver "we've reset your device; please re-initialize the hardware."

I guess this abuses PCI_ERS_RESULT_NEED_RESET by taking advantage of that implementation deficiency in pcie_do_recovery(): we know the downstream devices have already been reset via DPC, and returning PCI_ERS_RESULT_NEED_RESET means we'll call .slot_reset() to tell the driver about that reset.

Quoted 5 lines

I can see how this achieves the desired result, but if/when we fix pcie_do_recovery() to actually *do* the reset promised by PCI_ERS_RESULT_NEED_RESET, we will be doing *two* resets: the first via DPC and a second via whatever slot reset mechanism pcie_do_recovery() would use.

When we fix this issue, if we make sure the reset logic is implemented before we call .reset_link callback we should be able to avoid resetting the device twice. Before we call DPC .reset_link callback, the device link will not up and hence we should not able to reset it.

Quoted 3 lines

So I guess the real issue (as you allude to in the commit log) is that we rely on hotplug to unbind/rebind the driver, and without hotplug we need to at least tell the driver the device was reset.

Agree

Quoted 3 lines

I'll try to expand the comment here so it reminds me what's going on
when we have to look at this again:) Let me know if I'm on the right
track.

Quoted 1 line

Yes, your understanding is correct.

OK, thanks. I'm still uncomfortable with this issue, so I think I'm going to apply this series but omit this patch. Here's why:

1) The fact that resets may cause hotplug events isn't specific to DPC, so I don't think dpc_reset_link() is the right place. For instance, aer_root_reset() ultimately does a secondary bus reset.

pci_slot_reset() -> pciehp_reset_slot() path goes to some trouble to ignore the resulting hotplug event, but the pci_bus_reset() path does not.

2) I'm not convinced that "hotplug_is_native()" is the correct test. Even if we're using ACPI hotplug (acpiphp), that will detach the drivers and remove the devices, won't it?

I considered something like the patch below, which partly addresses my first concern, but not the second. Even the first one is awfully messy because of the different ways the aer_root_reset() path can work.

PCI/ERR: Skip driver callbacks if reset causes hotplug remove/add

drivers/pci/pcie/err.c +12 -0

index 1ac57e9e1e71..000551a06013 100644
@@ -208,6 +208,18 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
  		status = reset_link(dev, service);
  		if (status != PCI_ERS_RESULT_RECOVERED)
  			goto failed;
+
+		/*
+		 * If pdev supports hotplug, a link reset causes a hotplug
+		 * remove event.  If we have a hotplug driver, it will
+		 * detach all drivers of downstream devices and remove the
+		 * devices, so we can't call any driver error recovery
+		 * callbacks.  Bringing the link back up causes a hotplug
+		 * add event, and the devices should be re-enumerated and
+		 * the drivers re-attached.
+		 */
+		if (hotplug_is_native(pdev))
+			goto succeeded;
  	} else {
  		pci_walk_bus(bus, report_normal_detected, &status);
  	}

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

  		pci_dbg(dev, "broadcast slot_reset message\n");
  		pci_walk_bus(bus, report_slot_reset, &status);
  	}

@@ -235,6 +251,7 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
  	pci_dbg(dev, "broadcast resume message\n");
  	pci_walk_bus(bus, report_resume, &status);
 
+succeeded:
  	pci_aer_clear_device_status(dev);
  	pci_cleanup_aer_uncorrect_error_status(dev);
  	pci_info(dev, "device recovery successful\n");

Hi Bjorn,

Quoted 43 lines · Bjorn Helgaas

Quoted 42 lines · Sathyanarayanan

Quoted 19 lines · Bjorn Helgaas

I don't understand why hotplug is relevant here. This path (dpc_reset_link()) is only used for downstream ports that support DPC. DPC has already disabled the link, which resets everything below the port, regardless of whether the port supports hotplug.

I do see that PCI_ERS_RESULT_NEED_RESET seems to promise a lot more than it actually *does*. The doc (pci-error-recovery.rst) says .error_detected() can return PCI_ERS_RESULT_NEED_RESET to *request* a slot reset. But if that happens, pcie_do_recovery() doesn't do a reset at all. It calls the driver's .slot_reset() method, which tells the driver "we've reset your device; please re-initialize the hardware."

I guess this abuses PCI_ERS_RESULT_NEED_RESET by taking advantage of that implementation deficiency in pcie_do_recovery(): we know the downstream devices have already been reset via DPC, and returning PCI_ERS_RESULT_NEED_RESET means we'll call .slot_reset() to tell the driver about that reset.

Quoted 5 lines

I can see how this achieves the desired result, but if/when we fix pcie_do_recovery() to actually *do* the reset promised by PCI_ERS_RESULT_NEED_RESET, we will be doing *two* resets: the first via DPC and a second via whatever slot reset mechanism pcie_do_recovery() would use.

When we fix this issue, if we make sure the reset logic is implemented before we call .reset_link callback we should be able to avoid resetting the device twice. Before we call DPC .reset_link callback, the device link will not up and hence we should not able to reset it.

Quoted 3 lines

So I guess the real issue (as you allude to in the commit log) is that we rely on hotplug to unbind/rebind the driver, and without hotplug we need to at least tell the driver the device was reset.

Agree

Quoted 3 lines

I'll try to expand the comment here so it reminds me what's going on
when we have to look at this again:) Let me know if I'm on the right
track.

Quoted 8 lines

Quoted 1 line

Yes, your understanding is correct.

OK, thanks. I'm still uncomfortable with this issue, so I think I'm going to apply this series but omit this patch. Here's why:

1) The fact that resets may cause hotplug events isn't specific to DPC, so I don't think dpc_reset_link() is the right place. For instance, aer_root_reset() ultimately does a secondary bus reset.

Agree. Reset is common for pci_channel_io_frozen errors. I did not look into aer_root_reset() implementation. So if state is pci_channel_io_frozen then we can assume the slot has been reseted.
The

Quoted 4 lines

pci_slot_reset() -> pciehp_reset_slot() path goes to some trouble to ignore the resulting hotplug event, but the pci_bus_reset() path does not.

Quoted 3 lines

2) I'm not convinced that "hotplug_is_native()" is the correct test. Even if we're using ACPI hotplug (acpiphp), that will detach the drivers and remove the devices, won't it?

Yes, agreed. It does not handle ACPI hotplug case. In case of ACPI hotplug, native_pcie_hotplug = 0. May be we need a new helper function. hotplug_is_enabled() ?

Quoted 32 lines

I considered something like the patch below, which partly addresses my first concern, but not the second. Even the first one is awfully messy because of the different ways the aer_root_reset() path can work.

PCI/ERR: Skip driver callbacks if reset causes hotplug remove/add

drivers/pci/pcie/err.c +12 -0

index 1ac57e9e1e71..000551a06013 100644
@@ -208,6 +208,18 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
  		status = reset_link(dev, service);
  		if (status != PCI_ERS_RESULT_RECOVERED)
  			goto failed;
+
+		/*
+		 * If pdev supports hotplug, a link reset causes a hotplug
+		 * remove event.  If we have a hotplug driver, it will
+		 * detach all drivers of downstream devices and remove the
+		 * devices, so we can't call any driver error recovery
+		 * callbacks.  Bringing the link back up causes a hotplug
+		 * add event, and the devices should be re-enumerated and
+		 * the drivers re-attached.
+		 */
+		if (hotplug_is_native(pdev))
+			goto succeeded;
  	} else {
  		pci_walk_bus(bus, report_normal_detected, &status);
  	}

drivers/pci/pcie/err.c void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state, · line 243

Quoted 8 lines

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

@@ -224,7 +236,11 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
  		 * functions to reset slot before calling
  		 * drivers' slot_reset callbacks?
  		 */
+		pci_warn(pdev, "driver requested reset, but that's not implemented\n");
  		status = PCI_ERS_RESULT_RECOVERED;
+	}
+
+	if (status == PCI_ERS_RESULT_RECOVERED) {

Moving it outside status == PCI_ERS_NEED_RESET check will let it execute in non frozen error as well. IIUC, we should not call it on all error types. Let me know your comments.

Quoted 11 lines

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

  		pci_dbg(dev, "broadcast slot_reset message\n");
  		pci_walk_bus(bus, report_slot_reset, &status);
  	}

@@ -235,6 +251,7 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
  	pci_dbg(dev, "broadcast resume message\n");
  	pci_walk_bus(bus, report_resume, &status);
+succeeded:
  	pci_aer_clear_device_status(dev);
  	pci_cleanup_aer_uncorrect_error_status(dev);
  	pci_info(dev, "device recovery successful\n");
 

Quoted 15 lines

Quoted 6 lines

OK, thanks. I'm still uncomfortable with this issue, so I think I'm going to apply this series but omit this patch. Here's why:

1) The fact that resets may cause hotplug events isn't specific to DPC, so I don't think dpc_reset_link() is the right place. For instance, aer_root_reset() ultimately does a secondary bus reset.

Agree. Reset is common for pci_channel_io_frozen errors. I did not look into aer_root_reset() implementation. So if state is pci_channel_io_frozen then we can assume the slot has been reseted.
The

Quoted 4 lines

pci_slot_reset() -> pciehp_reset_slot() path goes to some trouble to ignore the resulting hotplug event, but the pci_bus_reset() path does not.

Quoted 6 lines

Quoted 3 lines

2) I'm not convinced that "hotplug_is_native()" is the correct test. Even if we're using ACPI hotplug (acpiphp), that will detach the drivers and remove the devices, won't it?

Yes, agreed. It does not handle ACPI hotplug case. In case of ACPI hotplug, native_pcie_hotplug = 0. May be we need a new helper function. hotplug_is_enabled() ?

I'm not proposing the patch below to be applied. I only included it as an idea of where the hotplug testing should be.

I'm proposing to merge the pci/edr branch as-is, without these two patches:

PCI: move {pciehp,shpchp}_is_native() definitions to pci.c PCI/DPC: Fix DPC recovery issue in non hotplug case

accepting that we still have some issues in the non-hotplug case that we can fix later.

From: Kuppuswamy Sathyanarayanan <[email protected]>

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

+void pcie_do_recovery(struct pci_dev *dev,
+		      enum pci_channel_state state,
+		      pci_ers_result_t (*reset_link)(struct pci_dev *pdev))
  {
  	pci_ers_result_t status = PCI_ERS_RESULT_CAN_RECOVER;
  	struct pci_bus *bus;

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

+		if ((status != PCI_ERS_RESULT_RECOVERED) &&
+		    (status != PCI_ERS_RESULT_NEED_RESET)) {
+			pci_dbg(dev, "link reset at upstream device failed\n");
  			goto failed;
+		}
  	} else {
  		pci_walk_bus(bus, report_normal_detected, &status);
  	}

drivers/pci/pcie/portdrv.h +0 -5

index 1e673619b101..64b5e081cdb2 100644
@@ -92,9 +92,6 @@ struct pcie_port_service_driver {
  	/* Device driver may resume normal operations */
  	void (*error_resume)(struct pci_dev *dev);
 
-	/* Link Reset Capability - AER service driver specific */
-	pci_ers_result_t (*reset_link)(struct pci_dev *dev);
-
  	int port_type;  /* Type of the port this driver can handle */
  	u32 service;    /* Port service this device represents */
 

@@ -161,7 +158,5 @@ static inline int pcie_aer_get_firmware_first(struct pci_dev *pci_dev)
  }
  #endif
 
-struct pcie_port_service_driver *pcie_port_find_service(struct pci_dev *dev,
-							u32 service);
  struct device *pcie_port_find_device(struct pci_dev *dev, u32 service);
  #endif /* _PORTDRV_H_ */

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

index 5075cb9e850c..50a9522ab07d 100644
@@ -458,27 +458,6 @@ static int find_service_iter(struct device *device, void *data)
  	return 0;
  }
 
-/**
- * pcie_port_find_service - find the service driver
- * @dev: PCI Express port the service is associated with
- * @service: Service to find
- *
- * Find PCI Express port service driver associated with given service
- */
-struct pcie_port_service_driver *pcie_port_find_service(struct pci_dev *dev,
-							u32 service)
-{
-	struct pcie_port_service_driver *drv;
-	struct portdrv_service_data pdrvs;
-
-	pdrvs.drv = NULL;
-	pdrvs.service = service;
-	device_for_each_child(&dev->dev, &pdrvs, find_service_iter);
-
-	drv = pdrvs.drv;
-	return drv;
-}
-
  /**
   * pcie_port_find_device - find the struct device
   * @dev: PCI Express port the service is associated with

Quoted 10 lines · On 3/23/20 5:26 PM, [email protected] wrote:

From: Kuppuswamy Sathyanarayanan <[email protected]>

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

+void pcie_do_recovery(struct pci_dev *dev,
+		      enum pci_channel_state state,
+		      pci_ers_result_t (*reset_link)(struct pci_dev *pdev))
  {
  	pci_ers_result_t status = PCI_ERS_RESULT_CAN_RECOVER;
  	struct pci_bus *bus;

drivers/pci/pcie/err.c void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state, · line 210

Quoted 5 lines

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

@@ -206,9 +165,12 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
  	pci_dbg(dev, "broadcast error_detected message\n");
  	if (state == pci_channel_io_frozen) {
  		pci_walk_bus(bus, report_frozen_detected, &status);
-		status = reset_link(dev, service);
-		if 		if (reset_link)

			status = reset_link(dev);(status == PCI_ERS_RESULT_DISCONNECT

drivers/pci/pcie/err.c void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state, · line 168

Quoted 1 line

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

+		status = reset_link(dev);

Above line needs to be replaced as below. Since there is a possibility reset_link can NULL (eventhough currently its not true).

		if (reset_link)
			status = reset_link(dev);

Shall I submit another version to add above fix on top of our pci/edr branch ?

Quoted 17 lines · Sathyanarayanan

Quoted 10 lines · On 3/23/20 5:26 PM, [email protected] wrote:

From: Kuppuswamy Sathyanarayanan <[email protected]>

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

+void pcie_do_recovery(struct pci_dev *dev,
+		      enum pci_channel_state state,
+		      pci_ers_result_t (*reset_link)(struct pci_dev *pdev))
   {
   	pci_ers_result_t status = PCI_ERS_RESULT_CAN_RECOVER;
   	struct pci_bus *bus;

drivers/pci/pcie/err.c void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state, · line 210

Quoted 5 lines

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

@@ -206,9 +165,12 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
   	pci_dbg(dev, "broadcast error_detected message\n");
   	if (state == pci_channel_io_frozen) {
   		pci_walk_bus(bus, report_frozen_detected, &status);
-		status = reset_link(dev, service);
-		if 		if (reset_link)

			status = reset_link(dev);(status == PCI_ERS_RESULT_DISCONNECT

Quoted 8 lines

drivers/pci/pcie/err.c void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state, · line 168

Quoted 1 line

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

+		status = reset_link(dev);

Above line needs to be replaced as below. Since there is a possibility reset_link can NULL (eventhough currently its not true).

		if (reset_link)
			status = reset_link(dev);

Shall I submit another version to add above fix on top of our pci/edr branch ?

No, I can squash that in if needed.

But I don't actually think we *do* need it. All the callers supply a valid reset_link function pointer, and if somebody changes or adds a new one that doesn't, I'd rather take the null pointer exception and find out about it than silently ignore it.

Bjorn

Quoted 29 lines · Bjorn Helgaas

Quoted 17 lines · Sathyanarayanan

Quoted 10 lines · On 3/23/20 5:26 PM, [email protected] wrote:

From: Kuppuswamy Sathyanarayanan <[email protected]>

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

+void pcie_do_recovery(struct pci_dev *dev,
+		      enum pci_channel_state state,
+		      pci_ers_result_t (*reset_link)(struct pci_dev *pdev))
   {
   	pci_ers_result_t status = PCI_ERS_RESULT_CAN_RECOVER;
   	struct pci_bus *bus;

drivers/pci/pcie/err.c void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state, · line 210

Quoted 5 lines

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

@@ -206,9 +165,12 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
   	pci_dbg(dev, "broadcast error_detected message\n");
   	if (state == pci_channel_io_frozen) {
   		pci_walk_bus(bus, report_frozen_detected, &status);
-		status = reset_link(dev, service);
-		if 		if (reset_link)

			status = reset_link(dev);(status == PCI_ERS_RESULT_DISCONNECT

Quoted 8 lines

drivers/pci/pcie/err.c void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state, · line 168

Quoted 1 line

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

+		status = reset_link(dev);

Above line needs to be replaced as below. Since there is a possibility reset_link can NULL (eventhough currently its not true).

		if (reset_link)
			status = reset_link(dev);

Shall I submit another version to add above fix on top of our pci/edr branch ?

No, I can squash that in if needed.

Quoted 4 lines

But I don't actually think we *do* need it. All the callers supply a valid reset_link function pointer, and if somebody changes or adds a new one that doesn't, I'd rather take the null pointer exception and find out about it than silently ignore it.

But the documentation says "If reset_link is not NULL, recovery function will use it to reset the link." It considers NULL as a possible case. So I think its better to allow that case with a pci_warn() message.

From: Kuppuswamy Sathyanarayanan <[email protected]>

Error Disconnect Recover (EDR) is a feature that allows ACPI firmware to notify OSPM that a device has been disconnected due to an error condition (ACPI v6.3, sec 5.6.6). OSPM advertises its support for EDR on PCI devices via _OSC (see [1], sec 4.5.1, table 4-4). The OSPM EDR notify handler should invalidate software state associated with disconnected devices and may attempt to recover them. OSPM communicates the status of recovery to the firmware via _OST (sec 6.3.5.2).

For PCIe, firmware may use Downstream Port Containment (DPC) to support EDR. Per [1], sec 4.5.1, table 4-6, even if firmware has retained control of DPC, OSPM may read/write DPC control and status registers during the EDR notification processing window, i.e., from the time it receives an EDR notification until it clears the DPC Trigger Status.

Note that per [1], sec 4.5.1 and 4.5.2.4,

1. If the OS supports EDR, it should advertise that to firmware by setting OSC_PCI_EDR_SUPPORT in _OSC Support.

2. If the OS sets OSC_PCI_EXPRESS_DPC_CONTROL in _OSC Control to request control of the DPC capability, it must also set OSC_PCI_EDR_SUPPORT in _OSC Support.

Add an EDR notify handler to attempt recovery.

[1] Downstream Port Containment Related Enhancements ECN, Jan 28, 2019, affecting PCI Firmware Specification, Rev. 3.2
https://members.pcisig.com/wg/PCI-SIG/document/12888

Link https://lore.kernel.org/r/9ae1d3285beeb81bbf85571a89b8f3d4451eae8f.1583286655.git.sathyanarayanan.kuppuswamy@linux.intel.com open here

Link https://lore.kernel.org/r/246aa05acca8f0a7e6d20a65ab05af0027f60118.1583286655.git.sathyanarayanan.kuppuswamy@linux.intel.com open here

[bhelgaas: squash add/enable patches into one]

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

Signed-off-by Bjorn Helgaas <[email protected]>

Cc Len Brown <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

drivers/pci/pcie/edr.c new file +6 -0

+static int acpi_enable_dpc(struct pci_dev *pdev)
+{
+	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+	union acpi_object *obj, argv4, req;
+	int status;
+

drivers/pci/pcie/edr.c new file +20 -0

+        if (!status)
+                return 0;
+
+	status = 0;
+	req.type = ACPI_TYPE_INTEGER;
+	req.integer.value = 1;
+
+	argv4.type = ACPI_TYPE_PACKAGE;
+	argv4.package.count = 1;
+	argv4.package.elements = &req;
+
+	/*
+	 * Per Downstream Port Containment Related Enhancements ECN to PCI
+	 * Firmware Specification r3.2, sec 4.6.12, EDR_PORT_DPC_ENABLE_DSM is
+	 * optional.  Return success if it's not implemented.
+	 */
+	obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
+				EDR_PORT_DPC_ENABLE_DSM, &argv4);
+	if (!obj)
+		return 0;

This is really ugly. What's the story on this firmware? It sounds defective to me.

Or is everybody that uses _DSM supposed to check before evaluating it?

E.g.,

   if (!acpi_check_dsm(...))
     return -EINVAL;

   obj = acpi_evaluate_dsm(...);

If everybody is supposed to do this, it seems like the check part should be moved into acpi_evaluate_dsm().

Hi Bjorn,

Quoted 2 lines · Bjorn Helgaas

This is really ugly. What's the story on this firmware? It sounds defective to me.

I think there is no defined standard for this. I have checked few _DSM implementations. Some of them return default value and some don't. But atleast in the test hardware I use, we need this check.

Quoted 1 line

Or is everybody that uses _DSM supposed to check before evaluating it?

I think its safer to do this check.

Quoted 9 lines

E.g.,

   if (!acpi_check_dsm(...))
     return -EINVAL;

   obj = acpi_evaluate_dsm(...);

If everybody is supposed to do this, it seems like the check part should be moved into acpi_evaluate_dsm().

From: Kuppuswamy Sathyanarayanan <[email protected]>

Error Disconnect Recover (EDR) is a feature that allows ACPI firmware to notify OSPM that a device has been disconnected due to an error condition (ACPI v6.3, sec 5.6.6). OSPM advertises its support for EDR on PCI devices via _OSC (see [1], sec 4.5.1, table 4-4). The OSPM EDR notify handler should invalidate software state associated with disconnected devices and may attempt to recover them. OSPM communicates the status of recovery to the firmware via _OST (sec 6.3.5.2).

For PCIe, firmware may use Downstream Port Containment (DPC) to support EDR. Per [1], sec 4.5.1, table 4-6, even if firmware has retained control of DPC, OSPM may read/write DPC control and status registers during the EDR notification processing window, i.e., from the time it receives an EDR notification until it clears the DPC Trigger Status.

Note that per [1], sec 4.5.1 and 4.5.2.4,

1. If the OS supports EDR, it should advertise that to firmware by setting OSC_PCI_EDR_SUPPORT in _OSC Support.

2. If the OS sets OSC_PCI_EXPRESS_DPC_CONTROL in _OSC Control to request control of the DPC capability, it must also set OSC_PCI_EDR_SUPPORT in _OSC Support.

Add an EDR notify handler to attempt recovery.

[1] Downstream Port Containment Related Enhancements ECN, Jan 28, 2019, affecting PCI Firmware Specification, Rev. 3.2
https://members.pcisig.com/wg/PCI-SIG/document/12888

drivers/pci/pcie/edr.c new file +24 -0

+static int acpi_enable_dpc(struct pci_dev *pdev)
+{
+	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+	union acpi_object *obj, argv4, req;
+	int status;
+
+	/*
+	 * Some firmware implementations will return default values for
+	 * unsupported _DSM calls. So checking acpi_evaluate_dsm() return
+	 * value for NULL condition is not a complete method for finding
+	 * whether given _DSM function is supported or not. So use
+	 * explicit func 0 call to find whether given _DSM function is
+	 * supported or not.
+	 */
+        status = acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
+				1ULL << EDR_PORT_DPC_ENABLE_DSM);
+        if (!status)
+                return 0;
+
+	status = 0;
+	req.type = ACPI_TYPE_INTEGER;
+	req.integer.value = 1;
+
+	argv4.type = ACPI_TYPE_PACKAGE;

drivers/pci/pcie/edr.c new file +17 -0

+	if (!obj)
+		return 0;
+
+	if (obj->type != ACPI_TYPE_INTEGER) {
+		pci_err(pdev, FW_BUG "Enable DPC _DSM returned non integer\n");
+		status = -EIO;
+	}
+
+	if (obj->integer.value != 1) {
+		pci_err(pdev, "Enable DPC _DSM failed to enable DPC\n");
+		status = -EIO;
+	}
+
+	ACPI_FREE(obj);
+
+	return status;
+}

Quoted 54 lines · On Mon, Mar 23, 2020 at 05:26:07PM -0700…

From: Kuppuswamy Sathyanarayanan <[email protected]>

Error Disconnect Recover (EDR) is a feature that allows ACPI firmware to notify OSPM that a device has been disconnected due to an error condition (ACPI v6.3, sec 5.6.6). OSPM advertises its support for EDR on PCI devices via _OSC (see [1], sec 4.5.1, table 4-4). The OSPM EDR notify handler should invalidate software state associated with disconnected devices and may attempt to recover them. OSPM communicates the status of recovery to the firmware via _OST (sec 6.3.5.2).

For PCIe, firmware may use Downstream Port Containment (DPC) to support EDR. Per [1], sec 4.5.1, table 4-6, even if firmware has retained control of DPC, OSPM may read/write DPC control and status registers during the EDR notification processing window, i.e., from the time it receives an EDR notification until it clears the DPC Trigger Status.

Note that per [1], sec 4.5.1 and 4.5.2.4,

1. If the OS supports EDR, it should advertise that to firmware by setting OSC_PCI_EDR_SUPPORT in _OSC Support.

2. If the OS sets OSC_PCI_EXPRESS_DPC_CONTROL in _OSC Control to request control of the DPC capability, it must also set OSC_PCI_EDR_SUPPORT in _OSC Support.

Add an EDR notify handler to attempt recovery.

[1] Downstream Port Containment Related Enhancements ECN, Jan 28, 2019, affecting PCI Firmware Specification, Rev. 3.2
https://members.pcisig.com/wg/PCI-SIG/document/12888

drivers/pci/pcie/edr.c new file +24 -0

+static int acpi_enable_dpc(struct pci_dev *pdev)
+{
+	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+	union acpi_object *obj, argv4, req;
+	int status;
+
+	/*
+	 * Some firmware implementations will return default values for
+	 * unsupported _DSM calls. So checking acpi_evaluate_dsm() return
+	 * value for NULL condition is not a complete method for finding
+	 * whether given _DSM function is supported or not. So use
+	 * explicit func 0 call to find whether given _DSM function is
+	 * supported or not.
+	 */
+        status = acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
+				1ULL << EDR_PORT_DPC_ENABLE_DSM);
+        if (!status)
+                return 0;
+
+	status = 0;
+	req.type = ACPI_TYPE_INTEGER;
+	req.integer.value = 1;
+
+	argv4.type = ACPI_TYPE_PACKAGE;

drivers/pci/pcie/edr.c line 61

Quoted 10 lines

drivers/pci/pcie/edr.c new file +10 -0

+	argv4.package.count = 1;
+	argv4.package.elements = &req;
+
+	/*
+	 * Per Downstream Port Containment Related Enhancements ECN to PCI
+	 * Firmware Specification r3.2, sec 4.6.12, EDR_PORT_DPC_ENABLE_DSM is
+	 * optional.  Return success if it's not implemented.
+	 */
+	obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
+				EDR_PORT_DPC_ENABLE_DSM, &argv4);

This has been upstream for a while, just a follow-up question: this _DSM function was defined by the ECN with Rev 5. The ECN was incorporated into the PCI Firmware spec r3.3 with slightly different behavior as Rev 6.

The main differences are:

ECN
- Rev 5
- Arg3 is an Integer
- Return is 0 (DPC disabled) or 1 (DPC enabled)

r3.3 spec
- Rev 6
- Arg3 is a Package of one Integer
- Return is 0 (DPC disabled, Hot-Plug Surprise may be set), 1 (DPC enabled, Hot-Plug Surprise may be cleared), or 2 (failure)

So the question is whether this actually implements Rev 5 or Rev 6? It looks like this builds a *package* for Arg3 (which would correspond to Rev 6), but we're evaluating Rev 5, which specified an Integer.

The meaning of the Arg3 values is basically the same, so I don't see an issue there, but it looks like if a platform implemented Rev 5 according to the ECN to take a bare Integer, this might not work correctly.

Quoted 18 lines

drivers/pci/pcie/edr.c new file +17 -0

+	if (!obj)
+		return 0;
+
+	if (obj->type != ACPI_TYPE_INTEGER) {
+		pci_err(pdev, FW_BUG "Enable DPC _DSM returned non integer\n");
+		status = -EIO;
+	}
+
+	if (obj->integer.value != 1) {
+		pci_err(pdev, "Enable DPC _DSM failed to enable DPC\n");
+		status = -EIO;
+	}
+
+	ACPI_FREE(obj);
+
+	return status;
+}

From: Kuppuswamy Sathyanarayanan <[email protected]>

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 v17 + Bjorns changes:
* This version is based on Bjorn's review/edr branch.
* Moved {pciehp,shpchp}_is_native() function definitions to pci.c and removed it's CONFIG_ACPI dependency.
* Modified dpc_reset_link() function to return PCI_ERS_RESULT_NEED_RESET when hotplug is not supported or enabled in kernel.
* Modified reset_link() function to handle PCI_ERS_RESULT_NEED_RESET as valid return value.
* Moved the implementation of reset_link() function to pcie_do_recovery() and renamed function callback parameter from reset_cb to reset_link.
* Moved the order of pci_acpi_add_edr_notifier() and pci_acpi_remove_edr_notifier() calls in pci_acpi_setup() and pci_acpi_cleanup() above wakeup capable support checks.
* Used acpi_check_dsm() to check whether given _DSM is supported or not in edr.c.

Changes since v16:
* Removed reset_link from pcie_port_service_driver.
* Removed pcie_port_find_service().
* Added pci_dpc_init() in pci_init_capabilities().

Changes since v15:
* Splitted Patch # 3 in previous set into multiple patches.
* Refactored EDR driver use pci_dev instead of dpc_dev.
* Added some debug logs to EDR driver.
* Used pci_aer_raw_clear_status() for clearing AER errors in EDR path.
* Addressed other comments from Bjorn.
* Rebased patches on top of Bjorns "PCI/DPC: Move data to struct pci_dev" patch.

Changes since v14:
* Rebased on top of v5.6-rc1

Changes since v13:
* Moved all EDR related code to edr.c
* Addressed Bjorns comments.

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

Bjorn Helgaas (1):
PCI/DPC: Move DPC data into struct pci_dev

Kuppuswamy Sathyanarayanan (10):
PCI/ERR: Update error status after reset_link()
PCI: move {pciehp,shpchp}_is_native() definitions to pci.c PCI/DPC: Fix DPC recovery issue in non hotplug case PCI/ERR: Remove service dependency in pcie_do_recovery() PCI/ERR: Return status of pcie_do_recovery()
PCI/DPC: Cache DPC capabilities in pci_init_capabilities() PCI/AER: Add pci_aer_raw_clear_status() to unconditionally clear Error
Status
PCI/DPC: Expose dpc_process_error(), dpc_reset_link() for use by EDR PCI/DPC: Add Error Disconnect Recover (EDR) support PCI/AER: Rationalize error status register clearing

 Documentation/PCI/pcieaer-howto.rst       |  23 +-
 drivers/acpi/pci_root.c                   |  15 ++
 drivers/net/ethernet/intel/ice/ice_main.c |   4 +-
 drivers/ntb/hw/idt/ntb_hw_idt.c           |   4 +-
 drivers/pci/pci-acpi.c                    |  40 +---
 drivers/pci/pci.c                         |  40 +++-
 drivers/pci/pci.h                         |  13 +-
 drivers/pci/pcie/Kconfig                  |  10 +
 drivers/pci/pcie/Makefile                 |   1 +
 drivers/pci/pcie/aer.c                    |  40 ++--
 drivers/pci/pcie/dpc.c                    | 145 ++++++-------
 drivers/pci/pcie/edr.c                    | 251 ++++++++++++++++++++++
 drivers/pci/pcie/err.c                    |  67 ++----
 drivers/pci/pcie/portdrv.h                |   5 -
 drivers/pci/pcie/portdrv_core.c           |  21 --
 drivers/pci/probe.c                       |   2 +
 drivers/scsi/lpfc/lpfc_attr.c             |   4 +-
 include/linux/acpi.h                      |   6 +-
 include/linux/aer.h                       |   9 +-
 include/linux/pci-acpi.h                  |   8 +
 include/linux/pci.h                       |   6 +
 include/linux/pci_hotplug.h               |   7 +-
 22 files changed, 471 insertions(+), 250 deletions(-)
 create mode 100644 drivers/pci/pcie/edr.c

Signature · 1 line

2.17.1

Applied to pci/edr for v5.7, except these two:

PCI: move {pciehp,shpchp}_is_native() definitions to pci.c PCI/DPC: Fix DPC recovery issue in non hotplug case