Add Error Disconnect Recover (EDR) support

From: Kuppuswamy Sathyanarayanan <[email protected]>

reset_link member in struct pcie_port_service_driver was mainly added to let pcie_do_recovery() trigger the driver specific reset_link() on PCIe fatal errors. But after modifying the pcie_do_recovery() function to accept reset_link callback as function parameter, we no longer have need to use or set reset_link in struct pcie_port_service_driver. So remove it.

Quoted 1 line · On Tue, Mar 03, 2020 at 06:36:28PM -0800…

From: Kuppuswamy Sathyanarayanan <[email protected]>

This should be folded into
"PCI/ERR: Remove service dependency in pcie_do_recovery()"

From: Kuppuswamy Sathyanarayanan <[email protected]>

After pcie_do_recovery() refactor, instead of reset_link member in struct pcie_port_service_driver, we use reset_cb callback parameter in pcie_do_recovery() function to pass the service driver specific reset_link function. So modify the Documentation to reflect the latest changes.

Quoted 2 lines · On Tue, Mar 03, 2020 at 06:36:29PM -0800…

From: Kuppuswamy Sathyanarayanan <[email protected]>

This should be folded into the patch removing the method.

Quoted 4 lines

Quoted 2 lines

From: Kuppuswamy Sathyanarayanan <[email protected]>

This should be folded into the patch removing the method.

This is also folded in the mentioned patch.
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?h=review/edr&id=7a18dc6506f108db3dc40f5cd779bc15270c4183

Quoted 6 lines · Sathyanarayanan

Quoted 4 lines

Quoted 2 lines

From: Kuppuswamy Sathyanarayanan <[email protected]>

This should be folded into the patch removing the method.

This is also folded in the mentioned patch.
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?h=review/edr&id=7a18dc6506f108db3dc40f5cd779bc15270c4183

I can't find that series anywhere on the list. What did I miss?

Quoted 9 lines · Christoph Hellwig

Quoted 6 lines · Sathyanarayanan

Quoted 4 lines

Quoted 2 lines

From: Kuppuswamy Sathyanarayanan <[email protected]>

This should be folded into the patch removing the method.

This is also folded in the mentioned patch.
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?h=review/edr&id=7a18dc6506f108db3dc40f5cd779bc15270c4183

Quoted 1 line

I can't find that series anywhere on the list. What did I miss?

We've still been discussing other issues (access to AER registers, synchronization between EDR and hotplug, etc) in other parts of this thread. The git branch Sathy pointed to above is my local branch. I'll send it to the list before putting it into -next, but I wanted to make progress on some of these other issues first.

Quoted 8 lines · Bjorn Helgaas

Quoted 7 lines · Christoph Hellwig

Quoted 5 lines · Sathyanarayanan

Quoted 3 lines

Quoted 1 line

From: Kuppuswamy Sathyanarayanan <[email protected]>

This should be folded into the patch removing the method.

This is also folded in the mentioned patch.
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?h=review/edr&id=7a18dc6506f108db3dc40f5cd779bc15270c4183

Quoted 7 lines

Quoted 1 line

I can't find that series anywhere on the list. What did I miss?

We've still been discussing other issues (access to AER registers, synchronization between EDR and hotplug, etc) in other parts of this thread. The git branch Sathy pointed to above is my local branch. I'll send it to the list before putting it into -next, but I wanted to make progress on some of these other issues first.

A few nitpicks:

PCI/ERR: Update error status after reset_link():

- there are two "if (state == pci_channel_io_frozen)"
right after each other now, merging them would make the code a little easier to read.

PCI/DPC: Move DPC data into struct pci_dev:

- dpc_rp_extensions probable should be a "bool : 1"

PCI/ERR: Remove service dependency in pcie_do_recovery():

- as mentioned to Kuppuswamy the reset_cb is never NULL, and thus a lot of dead code in reset_link can be removed.

Also reset_link should be merged into pcie_do_recovery. That would also enable to call the argument reset_link, which might be a bit more descriptive than reset_cb.

PCI/DPC: Cache DPC capabilities in pci_init_capabilities():

- I think the pci_dpc_init could be cleaned up a bit to:

	...
	pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CAP, &cap);
	if (!(cap & PCI_EXP_DPC_CAP_RP_EXT))
		return;
	pdev->dpc_rp_extensions = true;
	pdev->dpc_rp_log_size = (cap & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8;
	...

From: Kuppuswamy Sathyanarayanan <[email protected]>

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

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

---
  drivers/pci/pci.h      |  2 ++
  drivers/pci/pcie/aer.c | 22 ++++++++++++++++++----
  2 files changed, 20 insertions(+), 4 deletions(-)

drivers/pci/pci.h +1 -0

index e57e78b619f8..c239e6dd2542 100644
@@ -655,6 +655,7 @@ extern const struct attribute_group aer_stats_attr_group;
  void pci_aer_clear_fatal_status(struct pci_dev *dev);
  void pci_aer_clear_device_status(struct pci_dev *dev);
  int pci_cleanup_aer_error_status_regs(struct pci_dev *dev);
+int pci_aer_raw_clear_status(struct pci_dev *dev);
  #else
  static inline void pci_no_aer(void) { }
  static inline void pci_aer_init(struct pci_dev *d) { }

drivers/pci/pci.h +0 -0

  #endif
 
  #ifdef CONFIG_ACPI

drivers/pci/pcie/aer.c +18 -4

index c0540c3761dc..41afefa562b7 100644
@@ -420,7 +420,16 @@ void pci_aer_clear_fatal_status(struct pci_dev *dev)
  		pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, status);
  }
 
-int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
+/**
+ * pci_aer_raw_clear_status - Clear AER error registers.
+ * @dev: the PCI device
+ *
+ * NOTE: Allows clearing error registers in both FF and
+ * non FF modes.
+ *
+ * Returns 0 on success, or negative on failure.
+ */
+int pci_aer_raw_clear_status(struct pci_dev *dev)
  {
  	int pos;
  	u32 status;

@@ -433,9 +442,6 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
  	if (!pos)
  		return -EIO;
 
-	if (pcie_aer_get_firmware_first(dev))
-		return -EIO;
-
  	port_type = pci_pcie_type(dev);
  	if (port_type == PCI_EXP_TYPE_ROOT_PORT) {
  		pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, &status);

@@ -451,6 +457,14 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
  	return 0;
  }
 
+int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
+{
+	if (pcie_aer_get_firmware_first(dev))
+		return -EIO;
+
+	return pci_aer_raw_clear_status(dev);
+}
+
  void pci_save_aer_state(struct pci_dev *dev)
  {
  	struct pci_cap_saved_state *save_state;

Quoted 29 lines · On 3/3/2020 6:36 PM, [email protected] wrote:

From: Kuppuswamy Sathyanarayanan <[email protected]>

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

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

---
  drivers/pci/pci.h      |  2 ++
  drivers/pci/pcie/aer.c | 22 ++++++++++++++++++----
  2 files changed, 20 insertions(+), 4 deletions(-)

drivers/pci/pci.h +1 -0

index e57e78b619f8..c239e6dd2542 100644
@@ -655,6 +655,7 @@ extern const struct attribute_group aer_stats_attr_group;
  void pci_aer_clear_fatal_status(struct pci_dev *dev);
  void pci_aer_clear_device_status(struct pci_dev *dev);
  int pci_cleanup_aer_error_status_regs(struct pci_dev *dev);
+int pci_aer_raw_clear_status(struct pci_dev *dev);
  #else
  static inline void pci_no_aer(void) { }
  static inline void pci_aer_init(struct pci_dev *d) { }

@@ -665,6 +666,7 @@ static inline int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)

drivers/pci/pci.h static inline int pci_cleanup_aer_error_status_regs(struct pci_dev *dev) · line 669

Quoted 4 lines

drivers/pci/pci.h +1 -0

  {
  	return -EINVAL;
  }
+int pci_aer_raw_clear_status(struct pci_dev *dev) { return -EINVAL; }

It's missing static specifier. It needs to be fixed. I can fix it in next version. Bjorn, if there is no need for next version, can you please make this change?

Quoted 30 lines · Sathyanarayanan

Quoted 29 lines · On 3/3/2020 6:36 PM, [email protected] wrote:

From: Kuppuswamy Sathyanarayanan <[email protected]>

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

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

---
   drivers/pci/pci.h      |  2 ++
   drivers/pci/pcie/aer.c | 22 ++++++++++++++++++----
   2 files changed, 20 insertions(+), 4 deletions(-)

drivers/pci/pci.h +1 -0

index e57e78b619f8..c239e6dd2542 100644
@@ -655,6 +655,7 @@ extern const struct attribute_group aer_stats_attr_group;
   void pci_aer_clear_fatal_status(struct pci_dev *dev);
   void pci_aer_clear_device_status(struct pci_dev *dev);
   int pci_cleanup_aer_error_status_regs(struct pci_dev *dev);
+int pci_aer_raw_clear_status(struct pci_dev *dev);
   #else
   static inline void pci_no_aer(void) { }
   static inline void pci_aer_init(struct pci_dev *d) { }

@@ -665,6 +666,7 @@ static inline int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)

Quoted 8 lines

drivers/pci/pci.h static inline int pci_cleanup_aer_error_status_regs(struct pci_dev *dev) · line 669

Quoted 4 lines

drivers/pci/pci.h +1 -0

   {
   	return -EINVAL;
   }
+int pci_aer_raw_clear_status(struct pci_dev *dev) { return -EINVAL; }

It's missing static specifier. It needs to be fixed. I can fix it in next version. Bjorn, if there is no need for next version, can you please make this change?

pci_aer_raw_clear_status() is defined in aer.c and called from aer.c and edr.c, so I do not think it can be static. Am I missing something?

I have a review/edr branch that I hope becomes what will be applied.

Bjorn

Quoted 40 lines · Bjorn Helgaas

Quoted 38 lines · Sathyanarayanan

Quoted 29 lines · On 3/3/2020 6:36 PM, [email protected] wrote:

From: Kuppuswamy Sathyanarayanan <[email protected]>

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

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

---
   drivers/pci/pci.h      |  2 ++
   drivers/pci/pcie/aer.c | 22 ++++++++++++++++++----
   2 files changed, 20 insertions(+), 4 deletions(-)

drivers/pci/pci.h +1 -0

index e57e78b619f8..c239e6dd2542 100644
@@ -655,6 +655,7 @@ extern const struct attribute_group aer_stats_attr_group;
   void pci_aer_clear_fatal_status(struct pci_dev *dev);
   void pci_aer_clear_device_status(struct pci_dev *dev);
   int pci_cleanup_aer_error_status_regs(struct pci_dev *dev);
+int pci_aer_raw_clear_status(struct pci_dev *dev);
   #else
   static inline void pci_no_aer(void) { }
   static inline void pci_aer_init(struct pci_dev *d) { }

drivers/pci/pci.h static inline int pci_cleanup_aer_error_status_regs(struct pci_dev *dev) · line 669

Quoted 4 lines

drivers/pci/pci.h +1 -0

@@ -665,6 +666,7 @@ static inline int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
   {
   	return -EINVAL;
   }
+int pci_aer_raw_clear_status(struct pci_dev *dev) { return -EINVAL; }

It's missing static specifier. It needs to be fixed. I can fix it in next version. Bjorn, if there is no need for next version, can you please make this change?

Quoted 3 lines

pci_aer_raw_clear_status() is defined in aer.c and called from aer.c and edr.c, so I do not think it can be static. Am I missing something?

For kernel configs that does not define CONFIG_PCIEAER, it will create redefinition error since pci.h can be included in many files.

From: Kuppuswamy Sathyanarayanan <[email protected]>

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

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

---
 drivers/pci/pci.h      |  2 ++
 drivers/pci/pcie/aer.c | 22 ++++++++++++++++++----
 2 files changed, 20 insertions(+), 4 deletions(-)

drivers/pci/pci.h +2 -0

index e57e78b619f8..c239e6dd2542 100644
@@ -655,6 +655,7 @@ extern const struct attribute_group aer_stats_attr_group;
 void pci_aer_clear_fatal_status(struct pci_dev *dev);
 void pci_aer_clear_device_status(struct pci_dev *dev);
 int pci_cleanup_aer_error_status_regs(struct pci_dev *dev);
+int pci_aer_raw_clear_status(struct pci_dev *dev);
 #else
 static inline void pci_no_aer(void) { }
 static inline void pci_aer_init(struct pci_dev *d) { }

@@ -665,6 +666,7 @@ static inline int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
 {
 	return -EINVAL;
 }
+int pci_aer_raw_clear_status(struct pci_dev *dev) { return -EINVAL; }
 #endif
 
 #ifdef CONFIG_ACPI

drivers/pci/pcie/aer.c +18 -4

index c0540c3761dc..41afefa562b7 100644
@@ -420,7 +420,16 @@ void pci_aer_clear_fatal_status(struct pci_dev *dev)
 		pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, status);
 }
 
-int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
+/**
+ * pci_aer_raw_clear_status - Clear AER error registers.
+ * @dev: the PCI device
+ *
+ * NOTE: Allows clearing error registers in both FF and
+ * non FF modes.
+ *
+ * Returns 0 on success, or negative on failure.
+ */
+int pci_aer_raw_clear_status(struct pci_dev *dev)
 {
 	int pos;
 	u32 status;

@@ -433,9 +442,6 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
 	if (!pos)
 		return -EIO;
 
-	if (pcie_aer_get_firmware_first(dev))
-		return -EIO;
-
 	port_type = pci_pcie_type(dev);
 	if (port_type == PCI_EXP_TYPE_ROOT_PORT) {
 		pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, &status);

@@ -451,6 +457,14 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
 	return 0;
 }
 
+int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
+{
+	if (pcie_aer_get_firmware_first(dev))
+		return -EIO;
+
+	return pci_aer_raw_clear_status(dev);
+}
+
 void pci_save_aer_state(struct pci_dev *dev)
 {
 	struct pci_cap_saved_state *save_state;

Signature · 1 line

2.25.1

[+cc Austin, tentative Linux patches on this git branch:
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/tree/drivers/pci/pcie?h=review/edr]

Quoted 9 lines · On Tue, Mar 03, 2020 at 06:36:32PM -0800…

From: Kuppuswamy Sathyanarayanan <[email protected]>

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

I see that this ECR was released as an ECN a few days ago:
https://members.pcisig.com/wg/PCI-SIG/document/14076
Regrettably the title in the PDF still says "ECR" (the rendered title *page* says "ENGINEERING CHANGE NOTIFICATION", but some metadata buried in the file says "ECR - SFI _OSC Support and DPC Updates".

Anyway, I think I see the note you refer to (now on page 12):

IMPLEMENTATION NOTE
DPC Event Handling

The flow chart below documents the behavior when firmware maintains control of AER and DPC and grants control of PCIe Hot-Plug to the operating system.

...

Capture and clear device AER status. OS may choose to offline devices3, either via SW (not load driver) or HW (power down device, disable Link5,6,7). Otherwise process _HPX, complete device enumeration, load drivers

This clearly suggests that the OS should clear device AER status. However, according to the intro text, firmware has retained control of AER, so what gives the OS the right to clear AER status?

The Downstream Port Containment Related Enhancements ECN (sec 4.5.1, table 4-6) contains an exception that allows the OS to read/write DPC registers during recovery. But

- that is for *DPC* registers, not for AER registers, and

- that exception only applies between OS receipt of the EDR notification and OS release of DPC by clearing the DPC Trigger Status bit.

The flowchart in the SFI ECN shows the OS releasing DPC before clearing AER status:

- Receive EDR notification

- Cleanup - Notify and unload child drivers below Port

- Bring Port out of DPC, clear port error status, assign bus numbers to child devices.

I assume this box includes clearing DPC error status and clearing Trigger Status? They seem to be out of order in the box.

- Evaluate _OST

- Capture and clear device AER status.

This seems suspect to me. Where does it say the OS is allowed to write AER status when firmware retains control of AER?

This patch series does things in this order:

- Receive EDR notification (edr_handle_event(), edr.c)

- Read, log, and clear DPC error regs (dpc_process_error(), dpc.c).

This also clears AER uncorrectable error status when the relevant HEST entries do not have the FIRMWARE_FIRST bit set. I think this is incorrect: the test should be based the _OSC negotiation for AER ownership, not on the HEST entries. But this problem pre-dates this patch series.

- Clear AER status (pci_aer_raw_clear_status(), aer.c).

This is at least inside the EDR recovery window, but again, I don't see where it says the OS is allowed to write the AER status.

- Attempt recovery (pcie_do_recovery(), err.c)

- Clear DPC Trigger Status (dpc_reset_link(), dpc.c)

- Evaluate _OST (acpi_send_edr_status(), edr.c)

What am I missing?

Quoted 76 lines

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

---
  drivers/pci/pci.h      |  2 ++
  drivers/pci/pcie/aer.c | 22 ++++++++++++++++++----
  2 files changed, 20 insertions(+), 4 deletions(-)

drivers/pci/pci.h +2 -0

index e57e78b619f8..c239e6dd2542 100644
@@ -655,6 +655,7 @@ extern const struct attribute_group aer_stats_attr_group;
  void pci_aer_clear_fatal_status(struct pci_dev *dev);
  void pci_aer_clear_device_status(struct pci_dev *dev);
  int pci_cleanup_aer_error_status_regs(struct pci_dev *dev);
+int pci_aer_raw_clear_status(struct pci_dev *dev);
  #else
  static inline void pci_no_aer(void) { }
  static inline void pci_aer_init(struct pci_dev *d) { }

@@ -665,6 +666,7 @@ static inline int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
  {
  	return -EINVAL;
  }
+int pci_aer_raw_clear_status(struct pci_dev *dev) { return -EINVAL; }
  #endif
 
  #ifdef CONFIG_ACPI

drivers/pci/pcie/aer.c +18 -4

index c0540c3761dc..41afefa562b7 100644
@@ -420,7 +420,16 @@ void pci_aer_clear_fatal_status(struct pci_dev *dev)
  		pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, status);
  }
 
-int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
+/**
+ * pci_aer_raw_clear_status - Clear AER error registers.
+ * @dev: the PCI device
+ *
+ * NOTE: Allows clearing error registers in both FF and
+ * non FF modes.
+ *
+ * Returns 0 on success, or negative on failure.
+ */
+int pci_aer_raw_clear_status(struct pci_dev *dev)
  {
  	int pos;
  	u32 status;

@@ -433,9 +442,6 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
  	if (!pos)
  		return -EIO;
 
-	if (pcie_aer_get_firmware_first(dev))
-		return -EIO;
-
  	port_type = pci_pcie_type(dev);
  	if (port_type == PCI_EXP_TYPE_ROOT_PORT) {
  		pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, &status);

@@ -451,6 +457,14 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
  	return 0;
  }
 
+int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
+{
+	if (pcie_aer_get_firmware_first(dev))
+		return -EIO;
+
+	return pci_aer_raw_clear_status(dev);
+}
+
  void pci_save_aer_state(struct pci_dev *dev)
  {
  	struct pci_cap_saved_state *save_state;

Signature · 1 line

2.25.1

[EXTERNAL EMAIL]

Hi Bjorn,

Quoted 19 lines · Bjorn Helgaas

[+cc Austin, tentative Linux patches on this git branch:
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/tree/drivers/pci/pcie?h=review/edr]

Quoted 9 lines · On Tue, Mar 03, 2020 at 06:36:32PM -0800…

From: Kuppuswamy Sathyanarayanan <[email protected]>

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

I see that this ECR was released as an ECN a few days ago:
https://members.pcisig.com/wg/PCI-SIG/document/14076
Regrettably the title in the PDF still says "ECR" (the rendered title *page* says "ENGINEERING CHANGE NOTIFICATION", but some metadata buried in the file says "ECR - SFI _OSC Support and DPC Updates".

Quoted 36 lines

Anyway, I think I see the note you refer to (now on page 12):

IMPLEMENTATION NOTE
DPC Event Handling

The flow chart below documents the behavior when firmware maintains control of AER and DPC and grants control of PCIe Hot-Plug to the operating system.

...

Capture and clear device AER status. OS may choose to offline devices3, either via SW (not load driver) or HW (power down device, disable Link5,6,7). Otherwise process _HPX, complete device enumeration, load drivers

This clearly suggests that the OS should clear device AER status. However, according to the intro text, firmware has retained control of AER, so what gives the OS the right to clear AER status?

The Downstream Port Containment Related Enhancements ECN (sec 4.5.1, table 4-6) contains an exception that allows the OS to read/write DPC registers during recovery. But

- that is for *DPC* registers, not for AER registers, and

- that exception only applies between OS receipt of the EDR notification and OS release of DPC by clearing the DPC Trigger Status bit.

The flowchart in the SFI ECN shows the OS releasing DPC before clearing AER status:

- Receive EDR notification

Quoted 7 lines

- Cleanup - Notify and unload child drivers below Port

- Bring Port out of DPC, clear port error status, assign bus numbers to child devices.

I assume this box includes clearing DPC error status and clearing Trigger Status? They seem to be out of order in the box.

Quoted 20 lines

- Evaluate _OST

- Capture and clear device AER status.

This seems suspect to me. Where does it say the OS is allowed to write AER status when firmware retains control of AER?

This patch series does things in this order:

- Receive EDR notification (edr_handle_event(), edr.c)

- Read, log, and clear DPC error regs (dpc_process_error(), dpc.c).

This also clears AER uncorrectable error status when the relevant HEST entries do not have the FIRMWARE_FIRST bit set. I think this is incorrect: the test should be based the _OSC negotiation for AER ownership, not on the HEST entries. But this problem pre-dates this patch series.

Quoted 4 lines

- Clear AER status (pci_aer_raw_clear_status(), aer.c).

This is at least inside the EDR recovery window, but again, I don't see where it says the OS is allowed to write the AER status.

Implementation note is the only reference we have regarding clearing the AER registers.

But since the spec says both DPC and AER needs to be always controlled together by the either OS or firmware, and when firmware relinquishes control over DPC registers in EDR notification window, we can assume that we also have control over AER registers.

But I agree that is not explicitly spelled out any where outside the implementation note.

Austin,

May be ECN (section 4.5.1, table 4-6) needs to be updated to add this clarification.

Quoted 86 lines

- Attempt recovery (pcie_do_recovery(), err.c)

- Clear DPC Trigger Status (dpc_reset_link(), dpc.c)

- Evaluate _OST (acpi_send_edr_status(), edr.c)

What am I missing?

Quoted 76 lines

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

---
   drivers/pci/pci.h      |  2 ++
   drivers/pci/pcie/aer.c | 22 ++++++++++++++++++----
   2 files changed, 20 insertions(+), 4 deletions(-)

drivers/pci/pci.h +2 -0

index e57e78b619f8..c239e6dd2542 100644
@@ -655,6 +655,7 @@ extern const struct attribute_group aer_stats_attr_group;
   void pci_aer_clear_fatal_status(struct pci_dev *dev);
   void pci_aer_clear_device_status(struct pci_dev *dev);
   int pci_cleanup_aer_error_status_regs(struct pci_dev *dev);
+int pci_aer_raw_clear_status(struct pci_dev *dev);
   #else
   static inline void pci_no_aer(void) { }
   static inline void pci_aer_init(struct pci_dev *d) { }

@@ -665,6 +666,7 @@ static inline int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
   {
   	return -EINVAL;
   }
+int pci_aer_raw_clear_status(struct pci_dev *dev) { return -EINVAL; }
   #endif
 
   #ifdef CONFIG_ACPI

drivers/pci/pcie/aer.c +18 -4

index c0540c3761dc..41afefa562b7 100644
@@ -420,7 +420,16 @@ void pci_aer_clear_fatal_status(struct pci_dev *dev)
   		pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, status);
   }
 
-int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
+/**
+ * pci_aer_raw_clear_status - Clear AER error registers.
+ * @dev: the PCI device
+ *
+ * NOTE: Allows clearing error registers in both FF and
+ * non FF modes.
+ *
+ * Returns 0 on success, or negative on failure.
+ */
+int pci_aer_raw_clear_status(struct pci_dev *dev)
   {
   	int pos;
   	u32 status;

@@ -433,9 +442,6 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
   	if (!pos)
   		return -EIO;
 
-	if (pcie_aer_get_firmware_first(dev))
-		return -EIO;
-
   	port_type = pci_pcie_type(dev);
   	if (port_type == PCI_EXP_TYPE_ROOT_PORT) {
   		pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, &status);

@@ -451,6 +457,14 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
   	return 0;
   }
 
+int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
+{
+	if (pcie_aer_get_firmware_first(dev))
+		return -EIO;
+
+	return pci_aer_raw_clear_status(dev);
+}
+
   void pci_save_aer_state(struct pci_dev *dev)
   {
   	struct pci_cap_saved_state *save_state;

Signature · 1 line

2.25.1

Quoted 15 lines · Sathyanarayanan

Quoted 14 lines · Bjorn Helgaas

[+cc Austin, tentative Linux patches on this git branch:
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/tree/drivers/pci/pcie?h=review/edr]

Quoted 9 lines · On Tue, Mar 03, 2020 at 06:36:32PM -0800…

From: Kuppuswamy Sathyanarayanan <[email protected]>

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

Quoted 5 lines

Quoted 5 lines

I see that this ECR was released as an ECN a few days ago:
https://members.pcisig.com/wg/PCI-SIG/document/14076
Regrettably the title in the PDF still says "ECR" (the rendered title *page* says "ENGINEERING CHANGE NOTIFICATION", but some metadata buried in the file says "ECR - SFI _OSC Support and DPC Updates".

I'll see if PCI-SIG can update the metadata and repost.

Quoted 35 lines

Quoted 35 lines

Anyway, I think I see the note you refer to (now on page 12):

IMPLEMENTATION NOTE
DPC Event Handling

The flow chart below documents the behavior when firmware maintains control of AER and DPC and grants control of PCIe Hot-Plug to the operating system.

...

Capture and clear device AER status. OS may choose to offline devices3, either via SW (not load driver) or HW (power down device, disable Link5,6,7). Otherwise process _HPX, complete device enumeration, load drivers

This clearly suggests that the OS should clear device AER status. However, according to the intro text, firmware has retained control of AER, so what gives the OS the right to clear AER status?

The Downstream Port Containment Related Enhancements ECN (sec 4.5.1, table 4-6) contains an exception that allows the OS to read/write DPC registers during recovery. But

- that is for *DPC* registers, not for AER registers, and

- that exception only applies between OS receipt of the EDR notification and OS release of DPC by clearing the DPC Trigger Status bit.

The flowchart in the SFI ECN shows the OS releasing DPC before clearing AER status:

- Receive EDR notification

Quoted 7 lines

Quoted 7 lines

- Cleanup - Notify and unload child drivers below Port

- Bring Port out of DPC, clear port error status, assign bus numbers to child devices.

I assume this box includes clearing DPC error status and clearing Trigger Status? They seem to be out of order in the box.

OS clears the DPC Trigger Status bit which will bring port below it out of containment. Then OS will clear the "port" error status bits (i.e., the AER and DPC status bits in the root port or downstream port that triggered containment). I don't think it would hurt to do this two steps in reverse order but don't think it is necessary. Note that error status bits for devices below the port in containment are cleared later after f/w has a chance to log them.

Quoted 30 lines

Quoted 21 lines

- Evaluate _OST

- Capture and clear device AER status.

This seems suspect to me. Where does it say the OS is allowed to write AER status when firmware retains control of AER?

This patch series does things in this order:

- Receive EDR notification (edr_handle_event(), edr.c)

- Read, log, and clear DPC error regs (dpc_process_error(), dpc.c).

This also clears AER uncorrectable error status when the relevant HEST entries do not have the FIRMWARE_FIRST bit set. I think this is incorrect: the test should be based the _OSC negotiation for AER ownership, not on the HEST entries. But this problem pre-dates this patch series.

Quoted 5 lines

- Clear AER status (pci_aer_raw_clear_status(), aer.c).

This is at least inside the EDR recovery window, but again, I don't see where it says the OS is allowed to write the AER status.

Implementation note is the only reference we have regarding clearing the AER registers.

Quoted 8 lines

But since the spec says both DPC and AER needs to be always controlled together by the either OS or firmware, and when firmware relinquishes control over DPC registers in EDR notification window, we can assume that we also have control over AER registers.

But I agree that is not explicitly spelled out any where outside the implementation note.

Quoted 4 lines

Austin,

May be ECN (section 4.5.1, table 4-6) needs to be updated to add this clarification.

Sure we can update to section 4.5.1, table 4-6 to indicate when OS can clear the AER status bits. It will just follow what's done in the implementation note so I think it's acceptable to follow implementation guidance for now.

Quoted 7 lines

Quoted 7 lines

- Attempt recovery (pcie_do_recovery(), err.c)

- Clear DPC Trigger Status (dpc_reset_link(), dpc.c)

- Evaluate _OST (acpi_send_edr_status(), edr.c)

What am I missing?

[EXTERNAL EMAIL]

Quoted 22 lines · On Tue, Mar 10, 2020 at 06:14:20PM +0000, [email protected] wrote:

Quoted 20 lines · Sathyanarayanan

Quoted 19 lines · Bjorn Helgaas

[+cc Austin, tentative Linux patches on this git branch:
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/tree/drivers/pci/pcie?h=review/edr]

Quoted 9 lines · On Tue, Mar 03, 2020 at 06:36:32PM -0800…

From: Kuppuswamy Sathyanarayanan <[email protected]>

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

I see that this ECR was released as an ECN a few days ago:
https://members.pcisig.com/wg/PCI-SIG/document/14076
Regrettably the title in the PDF still says "ECR" (the rendered title *page* says "ENGINEERING CHANGE NOTIFICATION", but some metadata buried in the file says "ECR - SFI _OSC Support and DPC Updates".

Quoted 1 line

I'll see if PCI-SIG can update the metadata and repost.

If that's possible, it would be nice to update the metadata for the "Downstream Port Containment related Enhancements" ECN as well. That one currently says "ECR - CardBus Header Proposal", which means that's what's in the window title bar and icons in the panel.

Quoted 44 lines

Quoted 36 lines

Quoted 36 lines

Anyway, I think I see the note you refer to (now on page 12):

IMPLEMENTATION NOTE
DPC Event Handling

The flow chart below documents the behavior when firmware maintains control of AER and DPC and grants control of PCIe Hot-Plug to the operating system.

...

Capture and clear device AER status. OS may choose to offline devices3, either via SW (not load driver) or HW (power down device, disable Link5,6,7). Otherwise process _HPX, complete device enumeration, load drivers

This clearly suggests that the OS should clear device AER status. However, according to the intro text, firmware has retained control of AER, so what gives the OS the right to clear AER status?

The Downstream Port Containment Related Enhancements ECN (sec 4.5.1, table 4-6) contains an exception that allows the OS to read/write DPC registers during recovery. But

- that is for *DPC* registers, not for AER registers, and

- that exception only applies between OS receipt of the EDR notification and OS release of DPC by clearing the DPC Trigger Status bit.

The flowchart in the SFI ECN shows the OS releasing DPC before clearing AER status:

- Receive EDR notification

Quoted 7 lines

Quoted 7 lines

- Cleanup - Notify and unload child drivers below Port

- Bring Port out of DPC, clear port error status, assign bus numbers to child devices.

I assume this box includes clearing DPC error status and clearing Trigger Status? They seem to be out of order in the box.

Quoted 7 lines

OS clears the DPC Trigger Status bit which will bring port below it out of containment. Then OS will clear the "port" error status bits (i.e., the AER and DPC status bits in the root port or downstream port that triggered containment). I don't think it would hurt to do this two steps in reverse order but don't think it is necessary. Note that error status bits for devices below the port in containment are cleared later after f/w has a chance to log them.

Maybe I'm misreading the DPC enhancements ECN. I think it says the OS can read/write DPC registers until it clears the DPC Trigger Status. If the OS clears Trigger Status first, my understanding is that we're now out of the EDR notification processing window and the OS is not permitted to write DPC registers.

If it's OK for the OS to clear Trigger Status before clearing DPC error status, what is the event that determines when the OS may no longer read/write the DPC registers?

Quoted 37 lines

Quoted 37 lines

Quoted 22 lines

- Evaluate _OST

- Capture and clear device AER status.

This seems suspect to me. Where does it say the OS is allowed to write AER status when firmware retains control of AER?

This patch series does things in this order:

- Receive EDR notification (edr_handle_event(), edr.c)

- Read, log, and clear DPC error regs (dpc_process_error(), dpc.c).

This also clears AER uncorrectable error status when the relevant HEST entries do not have the FIRMWARE_FIRST bit set. I think this is incorrect: the test should be based the _OSC negotiation for AER ownership, not on the HEST entries. But this problem pre-dates this patch series.

Quoted 5 lines

- Clear AER status (pci_aer_raw_clear_status(), aer.c).

This is at least inside the EDR recovery window, but again, I don't see where it says the OS is allowed to write the AER status.

Implementation note is the only reference we have regarding clearing the AER registers.

But since the spec says both DPC and AER needs to be always controlled together by the either OS or firmware, and when firmware relinquishes control over DPC registers in EDR notification window, we can assume that we also have control over AER registers.

Quoted 2 lines

Quoted 2 lines

But I agree that is not explicitly spelled out any where outside the implementation note.

This is all quite unsatisfying since implementation notes are not normative. I would far rather reference actual spec text.

Quoted 10 lines

Quoted 5 lines

Austin,

May be ECN (section 4.5.1, table 4-6) needs to be updated to add this clarification.

Sure we can update to section 4.5.1, table 4-6 to indicate when OS can clear the AER status bits. It will just follow what's done in the implementation note so I think it's acceptable to follow implementation guidance for now.

There are no events after the "clear device AER status" box. That seems to mean the OS can write the AER status registers at any time. But the whole implementation note assumes firmware maintains control of AER.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

- Attempt recovery (pcie_do_recovery(), err.c)

- Clear DPC Trigger Status (dpc_reset_link(), dpc.c)

- Evaluate _OST (acpi_send_edr_status(), edr.c)

What am I missing?

Quoted 23 lines · Bjorn Helgaas

[EXTERNAL EMAIL]

Quoted 20 lines · On Tue, Mar 10, 2020 at 06:14:20PM +0000, [email protected] wrote:

Quoted 19 lines · Sathyanarayanan

Quoted 18 lines · Bjorn Helgaas

[+cc Austin, tentative Linux patches on this git branch:
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/tree/drivers/pci/pcie?h=review/edr]

Quoted 9 lines · On Tue, Mar 03, 2020 at 06:36:32PM -0800…

From: Kuppuswamy Sathyanarayanan <[email protected]>

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

I see that this ECR was released as an ECN a few days ago:
https://members.pcisig.com/wg/PCI-SIG/document/14076
Regrettably the title in the PDF still says "ECR" (the rendered title *page* says "ENGINEERING CHANGE NOTIFICATION", but some metadata buried in the file says "ECR - SFI _OSC Support and DPC Updates".

Quoted 5 lines

Quoted 1 line

I'll see if PCI-SIG can update the metadata and repost.

If that's possible, it would be nice to update the metadata for the "Downstream Port Containment related Enhancements" ECN as well. That one currently says "ECR - CardBus Header Proposal", which means that's what's in the window title bar and icons in the panel.

Sure, I'll check.

Quoted 55 lines

Quoted 42 lines

Quoted 35 lines

Quoted 35 lines

Anyway, I think I see the note you refer to (now on page 12):

IMPLEMENTATION NOTE
DPC Event Handling

The flow chart below documents the behavior when firmware maintains control of AER and DPC and grants control of PCIe Hot-Plug to the operating system.

...

Capture and clear device AER status. OS may choose to offline devices3, either via SW (not load driver) or HW (power down device, disable Link5,6,7). Otherwise process _HPX, complete device enumeration, load drivers

This clearly suggests that the OS should clear device AER status. However, according to the intro text, firmware has retained control of AER, so what gives the OS the right to clear AER status?

The Downstream Port Containment Related Enhancements ECN (sec 4.5.1, table 4-6) contains an exception that allows the OS to read/write DPC registers during recovery. But

- that is for *DPC* registers, not for AER registers, and

- that exception only applies between OS receipt of the EDR notification and OS release of DPC by clearing the DPC Trigger Status bit.

The flowchart in the SFI ECN shows the OS releasing DPC before clearing AER status:

- Receive EDR notification

Quoted 7 lines

Quoted 7 lines

- Cleanup - Notify and unload child drivers below Port

- Bring Port out of DPC, clear port error status, assign bus numbers to child devices.

I assume this box includes clearing DPC error status and clearing Trigger Status? They seem to be out of order in the box.

Quoted 7 lines

OS clears the DPC Trigger Status bit which will bring port below it out of containment. Then OS will clear the "port" error status bits (i.e., the AER and DPC status bits in the root port or downstream port that triggered containment). I don't think it would hurt to do this two steps in reverse order but don't think it is necessary. Note that error status bits for devices below the port in containment are cleared later after f/w has a chance to log them.

Maybe I'm misreading the DPC enhancements ECN. I think it says the OS can read/write DPC registers until it clears the DPC Trigger Status. If the OS clears Trigger Status first, my understanding is that we're now out of the EDR notification processing window and the OS is not permitted to write DPC registers.

Quoted 3 lines

If it's OK for the OS to clear Trigger Status before clearing DPC error status, what is the event that determines when the OS may no longer read/write the DPC registers?

I think there are a few different registers to consider... DPC Control, DPC Status, various AER registers, and the RP PIO registers. At this point in the flow, the firmware has already had a chance to read all of them and so it really doesn't matter the order the OS does those two things. The firmware isn't going to get notified again until _OST so by then both operation will be done and system firmware will have no idea which order the OS did them in, nor will it care. But since the existing normative text specifies and order, I would just follow that.

Quoted 36 lines

Quoted 36 lines

Quoted 36 lines

Quoted 22 lines

- Evaluate _OST

- Capture and clear device AER status.

This seems suspect to me. Where does it say the OS is allowed to write AER status when firmware retains control of AER?

This patch series does things in this order:

- Receive EDR notification (edr_handle_event(), edr.c)

- Read, log, and clear DPC error regs (dpc_process_error(), dpc.c).

This also clears AER uncorrectable error status when the relevant HEST entries do not have the FIRMWARE_FIRST bit set. I think this is incorrect: the test should be based the _OSC negotiation for AER ownership, not on the HEST entries. But this problem pre-dates this patch series.

Quoted 5 lines

- Clear AER status (pci_aer_raw_clear_status(), aer.c).

This is at least inside the EDR recovery window, but again, I don't see where it says the OS is allowed to write the AER status.

Implementation note is the only reference we have regarding clearing the AER registers.

But since the spec says both DPC and AER needs to be always controlled together by the either OS or firmware, and when firmware relinquishes control over DPC registers in EDR notification window, we can assume that we also have control over AER registers.

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

But I agree that is not explicitly spelled out any where outside the implementation note.

This is all quite unsatisfying since implementation notes are not normative. I would far rather reference actual spec text.

Yes, the change I mention below would be to add normative text.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Austin,

May be ECN (section 4.5.1, table 4-6) needs to be updated to add this clarification.

Quoted 9 lines

Quoted 4 lines

Sure we can update to section 4.5.1, table 4-6 to indicate when OS can clear the AER status bits. It will just follow what's done in the implementation note so I think it's acceptable to follow implementation guidance for now.

There are no events after the "clear device AER status" box. That seems to mean the OS can write the AER status registers at any time. But the whole implementation note assumes firmware maintains control of AER.

In this model the OS doesn't own DPC or AER but the model allows OS to touch both DPC and AER registers at certain times. I would view ownership in this case as who is the primary owner and not who is the sole entity allowed to access the registers.

For the normative text describing when OS clears the AER bits following the informative flow chart, it could say that OS clears AER as soon as possible after OST returns and before OS processes _HPX and loading drivers. Open to other suggestions as well.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

- Attempt recovery (pcie_do_recovery(), err.c)

- Clear DPC Trigger Status (dpc_reset_link(), dpc.c)

- Evaluate _OST (acpi_send_edr_status(), edr.c)

What am I missing?

Please check the following spec reference (change to 4.5.1 Table 4-6)

the OS is permitted to read or write DPC Control and Status registers of a port while processing an Error Disconnect Recover notification from firmware on that port. Error Disconnect Recover notification processing begins with the
Error Disconnect Recover notify from Firmware, and *ends when the OS releases
DPC by clearing the DPC Trigger Status bit*.Firmware can read DPC Trigger Status bit to determine the ownership of DPC Control and Status registers. Firmware
is not permitted to write to DPC Control and Status registers if DPC Trigger Status is
set i.e. the link is in DPC state. *Outside of the Error Disconnect Recover notification
processing window, the OS is not permitted to modify DPC Control or Status registers*;
only firmware is allowed to.

Since the EDR processing window ends with clearing DPC Trigger status bit, OS needs to
clear DPC and AER registers before it ends.

Austin,

I think the order needs to be reversed in the implementation note.

Quoted 16 lines · Bjorn Helgaas

Quoted 15 lines · On Tue, Mar 10, 2020 at 06:14:20PM +0000, [email protected] wrote:

Quoted 14 lines · Sathyanarayanan

Quoted 13 lines · Bjorn Helgaas

[+cc Austin, tentative Linux patches on this git branch:
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/tree/drivers/pci/pcie?h=review/edr]

Quoted 9 lines · On Tue, Mar 03, 2020 at 06:36:32PM -0800…

From: Kuppuswamy Sathyanarayanan <[email protected]>

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

Quoted 8 lines

Quoted 8 lines

OS clears the DPC Trigger Status bit which will bring port below it out of containment. Then OS will clear the "port" error status bits (i.e., the AER and DPC status bits in the root port or downstream port that triggered containment). I don't think it would hurt to do this two steps in reverse order but don't think it is necessary.

Note that error status bits for devices below the port in containment are cleared later after f/w has a chance to log them.

Quoted 9 lines

Maybe I'm misreading the DPC enhancements ECN. I think it says the OS can read/write DPC registers until it clears the DPC Trigger Status. If the OS clears Trigger Status first, my understanding is that we're now out of the EDR notification processing window and the OS is not permitted to write DPC registers.

If it's OK for the OS to clear Trigger Status before clearing DPC error status, what is the event that determines when the OS may no longer read/write the DPC registers?

I think there are a few different registers to consider... DPC Control, DPC Status, various AER registers, and the RP PIO registers. At this point in the flow, the firmware has already had a chance to read all of them and so it really doesn't matter the order the OS does those two things. The firmware isn't going to get notified again until _OST so by then both operation will be done and system firmware will have no idea which order the OS did them in, nor will it care. But since the existing normative text specifies and order, I would just follow that.

Quoted 4 lines

There are no events after the "clear device AER status" box. That seems to mean the OS can write the AER status registers at any time. But the whole implementation note assumes firmware maintains control of AER.

In this model the OS doesn't own DPC or AER but the model allows OS to touch both DPC and AER registers at certain times. I would view ownership in this case as who is the primary owner and not who is the sole entity allowed to access the registers.

For the normative text describing when OS clears the AER bits following the informative flow chart, it could say that OS clears AER as soon as possible after OST returns and before OS processes _HPX and loading drivers. Open to other suggestions as well.

[EXTERNAL EMAIL]

Quoted 18 lines · On Tue, Mar 10, 2020 at 08:06:21PM +0000, [email protected] wrote:

Quoted 16 lines · Bjorn Helgaas

Quoted 15 lines · On Tue, Mar 10, 2020 at 06:14:20PM +0000, [email protected] wrote:

Quoted 14 lines · Sathyanarayanan

Quoted 13 lines · Bjorn Helgaas

[+cc Austin, tentative Linux patches on this git branch:
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/tree/drivers/pci/pcie?h=review/edr]

Quoted 9 lines · On Tue, Mar 03, 2020 at 06:36:32PM -0800…

From: Kuppuswamy Sathyanarayanan <[email protected]>

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

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

OS clears the DPC Trigger Status bit which will bring port below it out of containment. Then OS will clear the "port" error status bits (i.e., the AER and DPC status bits in the root port or downstream port that triggered containment). I don't think it would hurt to do this two steps in reverse order but don't think it is necessary.

Note that error status bits for devices below the port in containment are cleared later after f/w has a chance to log them.

Thanks for pointing out this wrinkle about devices below the port in containment. I think we might have an issue here with the current series because evaluating _OST is the last thing the EDR notify handler does. More below.

Quoted 10 lines

Quoted 9 lines

Maybe I'm misreading the DPC enhancements ECN. I think it says the OS can read/write DPC registers until it clears the DPC Trigger Status. If the OS clears Trigger Status first, my understanding is that we're now out of the EDR notification processing window and the OS is not permitted to write DPC registers.

If it's OK for the OS to clear Trigger Status before clearing DPC error status, what is the event that determines when the OS may no longer read/write the DPC registers?

Quoted 9 lines

I think there are a few different registers to consider... DPC Control, DPC Status, various AER registers, and the RP PIO registers. At this point in the flow, the firmware has already had a chance to read all of them and so it really doesn't matter the order the OS does those two things. The firmware isn't going to get notified again until _OST so by then both operation will be done and system firmware will have no idea which order the OS did them in, nor will it care. But since the existing normative text specifies and order, I would just follow that.

OK, this series clears DPC error status before clearing DPC Trigger Status, so I think we can keep that as-is.

Quoted 5 lines

Quoted 4 lines

There are no events after the "clear device AER status" box. That seems to mean the OS can write the AER status registers at any time. But the whole implementation note assumes firmware maintains control of AER.

Quoted 4 lines

In this model the OS doesn't own DPC or AER but the model allows OS to touch both DPC and AER registers at certain times. I would view ownership in this case as who is the primary owner and not who is the sole entity allowed to access the registers.

I'm not sure how to translate the idea of primary ownership into code.

Quoted 5 lines

For the normative text describing when OS clears the AER bits following the informative flow chart, it could say that OS clears AER as soon as possible after OST returns and before OS processes _HPX and loading drivers. Open to other suggestions as well.

I'm not sure what to do with "as soon as possible" either. That doesn't seem like something firmware and the OS can agree on.

For the port that triggered DPC containment, I think the easiest thing to understand and implement would be to allow AER access during the same EDR processing window where DPC access is allowed.

For child devices of that port, obviously it's impossible to access AER registers until DPC Trigger Status is cleared, and the flowchart says the OS shouldn't access them until after _OST.

I'm actually not sure we currently do *anything* with child device AER info in the EDR path. pcie_do_recovery() does walk the sub-hierarchy of child devices, but it only calls error handling callbacks in the child drivers; it doesn't do anything with the child AER registers itself. And of course, this happens before _OST, so it would be too early in any case. But maybe I'm missing something here.

BTW, if/when this is updated, I have another question: the _OSC DPC control bit currently allows the OS to write DPC Control during that window. I understand the OS writing the RW1C *Status* bits to clear them, but it seems like writing the DPC Control register is likely to cause issues. The same question would apply to the AER access we're talking about.

Bjorn

Quoted 67 lines · Bjorn Helgaas

Quoted 18 lines · On Tue, Mar 10, 2020 at 08:06:21PM +0000, [email protected] wrote:

Quoted 16 lines · Bjorn Helgaas

Quoted 15 lines · On Tue, Mar 10, 2020 at 06:14:20PM +0000, [email protected] wrote:

Quoted 14 lines · Sathyanarayanan

Quoted 13 lines · Bjorn Helgaas

[+cc Austin, tentative Linux patches on this git branch:
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/tree/drivers/pci/pcie?h=review/edr]

Quoted 9 lines · On Tue, Mar 03, 2020 at 06:36:32PM -0800…

From: Kuppuswamy Sathyanarayanan <[email protected]>

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

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

OS clears the DPC Trigger Status bit which will bring port below it out of containment. Then OS will clear the "port" error status bits (i.e., the AER and DPC status bits in the root port or downstream port that triggered containment). I don't think it would hurt to do this two steps in reverse order but don't think it is necessary.

Note that error status bits for devices below the port in containment are cleared later after f/w has a chance to log them.

Thanks for pointing out this wrinkle about devices below the port in containment. I think we might have an issue here with the current series because evaluating _OST is the last thing the EDR notify handler does. More below.

Quoted 10 lines

Quoted 9 lines

Maybe I'm misreading the DPC enhancements ECN. I think it says the OS can read/write DPC registers until it clears the DPC Trigger Status. If the OS clears Trigger Status first, my understanding is that we're now out of the EDR notification processing window and the OS is not permitted to write DPC registers.

If it's OK for the OS to clear Trigger Status before clearing DPC error status, what is the event that determines when the OS may no longer read/write the DPC registers?

Quoted 9 lines

I think there are a few different registers to consider... DPC Control, DPC Status, various AER registers, and the RP PIO registers. At this point in the flow, the firmware has already had a chance to read all of them and so it really doesn't matter the order the OS does those two things. The firmware isn't going to get notified again until _OST so by then both operation will be done and system firmware will have no idea which order the OS did them in, nor will it care. But since the existing normative text specifies and order, I would just follow that.

OK, this series clears DPC error status before clearing DPC Trigger Status, so I think we can keep that as-is.

Quoted 9 lines

Quoted 4 lines

There are no events after the "clear device AER status" box. That seems to mean the OS can write the AER status registers at any time. But the whole implementation note assumes firmware maintains control of AER.

In this model the OS doesn't own DPC or AER but the model allows OS to touch both DPC and AER registers at certain times. I would view ownership in this case as who is the primary owner and not who is the sole entity allowed to access the registers.

Quoted 2 lines

I'm not sure how to translate the idea of primary ownership into code.

I would just add text that said when it's ok for OS to touch these bits even when they don't own them similar to what's done for the DPC bits.

Quoted 5 lines

Quoted 4 lines

For the normative text describing when OS clears the AER bits following the informative flow chart, it could say that OS clears AER as soon as possible after OST returns and before OS processes _HPX and loading drivers. Open to other suggestions as well.

Quoted 2 lines

I'm not sure what to do with "as soon as possible" either. That doesn't seem like something firmware and the OS can agree on.

I can just state that it's done after OST returns but before _HPX or driver is loaded. Any time in that range is fine. I can't get super specific here because different OSes do different things. Even for a given OS they change over time. And I need something generic enough to support a wide variety of OS implementations.

Quoted 11 lines

For child devices of that port, obviously it's impossible to access AER registers until DPC Trigger Status is cleared, and the flowchart says the OS shouldn't access them until after _OST.

I'm actually not sure we currently do *anything* with child device AER info in the EDR path. pcie_do_recovery() does walk the sub-hierarchy of child devices, but it only calls error handling callbacks in the child drivers; it doesn't do anything with the child AER registers itself. And of course, this happens before _OST, so it would be too early in any case. But maybe I'm missing something here.

My understanding is that the OS read/clears AER in the case where OS has native control of AER. Feedback from OSVs is they wanted to continue to do that to keep the native OS controlled AER and FF mechanism similar. The other way we could have done it would be to have the firmware read/clear AER and report them to OS via APEI.

Quoted 6 lines

BTW, if/when this is updated, I have another question: the _OSC DPC control bit currently allows the OS to write DPC Control during that window. I understand the OS writing the RW1C *Status* bits to clear them, but it seems like writing the DPC Control register is likely to cause issues. The same question would apply to the AER access we're talking about.

We could specify which particular bits can and can't be touched. But it's hard to maintain as new bits are added. Probably better to add some guidance that OS should read/clear error status, DPC Trigger Status, etc. but shouldn't change masks/severity/control bits/etc.

[EXTERNAL EMAIL]

I'm probably missing your intent, but that sounds like "the OS can read/write AER bits whenever it wants, regardless of ownership."

That doesn't sound practical to me, and I don't think it's really similar to DPC, where it's pretty clear that the OS can touch DPC bits it doesn't own but only *during the EDR processing window*.

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

For the normative text describing when OS clears the AER bits following the informative flow chart, it could say that OS clears AER as soon as possible after OST returns and before OS processes _HPX and loading drivers. Open to other suggestions as well.

Quoted 8 lines

Quoted 2 lines

I'm not sure what to do with "as soon as possible" either. That doesn't seem like something firmware and the OS can agree on.

I can just state that it's done after OST returns but before _HPX or driver is loaded. Any time in that range is fine. I can't get super specific here because different OSes do different things. Even for a given OS they change over time. And I need something generic enough to support a wide variety of OS implementations.

Yeah. I don't know how to solve this.

Linux doesn't actually unload and reload drivers for the child devices (Sathy, correct me if I'm wrong here) even though DPC containment takes the link down and effectively unplugs and replugs the device. I would *like* to handle it like hotplug, but some higher-level software doesn't deal well with things like storage devices disappearing and reappearing.

Since Linux doesn't actually re-enumerate the child devices, it wouldn't evaluate _HPX again. It would probably be cleaner if it did, but it's all tied up with the whole unplug/replug problem.

Quoted 13 lines

Quoted 5 lines

For child devices of that port, obviously it's impossible to access AER registers until DPC Trigger Status is cleared, and the flowchart says the OS shouldn't access them until after _OST.

Quoted 7 lines

I'm actually not sure we currently do *anything* with child device AER info in the EDR path. pcie_do_recovery() does walk the sub-hierarchy of child devices, but it only calls error handling callbacks in the child drivers; it doesn't do anything with the child AER registers itself. And of course, this happens before _OST, so it would be too early in any case. But maybe I'm missing something here.

Quoted 5 lines

My understanding is that the OS read/clears AER in the case where OS has native control of AER. Feedback from OSVs is they wanted to continue to do that to keep the native OS controlled AER and FF mechanism similar. The other way we could have done it would be to have the firmware read/clear AER and report them to OS via APEI.

When Linux has native control of AER, it reads/clears AER status. The flowchart is for the case where firmware has AER control, so I guess Linux would not field AER interrupts and wouldn't expect to read/clear AER status. So I *guess* Linux would assume APEI? But that doesn't seem to be what the flowchart assumes.

Quoted 8 lines

Quoted 7 lines

BTW, if/when this is updated, I have another question: the _OSC DPC control bit currently allows the OS to write DPC Control during that window. I understand the OS writing the RW1C *Status* bits to clear them, but it seems like writing the DPC Control register is likely to cause issues. The same question would apply to the AER access we're talking about.

Quoted 5 lines

We could specify which particular bits can and can't be touched. But it's hard to maintain as new bits are added. Probably better to add some guidance that OS should read/clear error status, DPC Trigger Status, etc. but shouldn't change masks/severity/control bits/etc.

Yeah. I didn't mean at the level of individual bits; I was thinking more of status/log/etc vs control registers. But maybe even that is hard, I dunno.

Quoted 3 lines · Bjorn Helgaas

[EXTERNAL EMAIL]

<SNIP>

Quoted 4 lines

I'm probably missing your intent, but that sounds like "the OS can read/write AER bits whenever it wants, regardless of ownership."

Quoted 3 lines

That doesn't sound practical to me, and I don't think it's really similar to DPC, where it's pretty clear that the OS can touch DPC bits it doesn't own but only *during the EDR processing window*.

Yes, by treating AER bits like DPC bits I meant I'd define the specific time windows when OS can touch the AER status bits similar to how it's done for DPC in the current ECN.

Quoted 23 lines

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

For the normative text describing when OS clears the AER bits following the informative flow chart, it could say that OS clears AER as soon as possible after OST returns and before OS processes _HPX and loading drivers. Open to other suggestions as well.

Quoted 8 lines

Quoted 2 lines

I'm not sure what to do with "as soon as possible" either. That doesn't seem like something firmware and the OS can agree on.

I can just state that it's done after OST returns but before _HPX or driver is loaded. Any time in that range is fine. I can't get super specific here because different OSes do different things. Even for a given OS they change over time. And I need something generic enough to support a wide variety of OS implementations.

Yeah. I don't know how to solve this.

Linux doesn't actually unload and reload drivers for the child devices (Sathy, correct me if I'm wrong here) even though DPC containment takes the link down and effectively unplugs and replugs the device. I would *like* to handle it like hotplug, but some higher-level software doesn't deal well with things like storage devices disappearing and reappearing.

Quoted 3 lines

Since Linux doesn't actually re-enumerate the child devices, it wouldn't evaluate _HPX again. It would probably be cleaner if it did, but it's all tied up with the whole unplug/replug problem.

DPC resets everything below it and so to get it back up and running it would mean that all buses and resources need to be assigned, _HPX evaluated, and drivers reloaded. If those things don't happen then the whole hierarchy below the port that triggered DPC will be inaccessible.

For higher level software not handling storage device disappearing due to hot-plug, they will have the same problem with DPC since DPC holds the port in the disabled state (and hence will be inaccessible). And once DPC is released the devices will be unconfigured and so still inaccessible to upper-level software. A lot of upper-level storage software I've seen can already handle this gracefully.

Quoted 18 lines

Quoted 12 lines

Quoted 11 lines

For child devices of that port, obviously it's impossible to access AER registers until DPC Trigger Status is cleared, and the flowchart says the OS shouldn't access them until after _OST.

I'm actually not sure we currently do *anything* with child device AER info in the EDR path. pcie_do_recovery() does walk the sub-hierarchy of child devices, but it only calls error handling callbacks in the child drivers; it doesn't do anything with the child AER registers itself. And of course, this happens before _OST, so it would be too early in any case. But maybe I'm missing something here.

Quoted 5 lines

My understanding is that the OS read/clears AER in the case where OS has native control of AER. Feedback from OSVs is they wanted to continue to do that to keep the native OS controlled AER and FF mechanism similar. The other way we could have done it would be to have the firmware read/clear AER and report them to OS via APEI.

Quoted 5 lines

When Linux has native control of AER, it reads/clears AER status. The flowchart is for the case where firmware has AER control, so I guess Linux would not field AER interrupts and wouldn't expect to read/clear AER status. So I *guess* Linux would assume APEI? But that doesn't seem to be what the flowchart assumes.

Correct on the flowchart. The OSVs we talked with did not want to use APEI. They wanted to read and clear AER themselves and hence the flowchart is written that way.

Quoted 7 lines · On Wed, Mar 11, 2020 at 05:27:35PM +0000, [email protected] wrote:

Quoted 2 lines · Bjorn Helgaas

[EXTERNAL EMAIL]

<SNIP>

Quoted 3 lines

I'm probably missing your intent, but that sounds like "the OS can read/write AER bits whenever it wants, regardless of ownership."

Quoted 6 lines

Quoted 3 lines

That doesn't sound practical to me, and I don't think it's really similar to DPC, where it's pretty clear that the OS can touch DPC bits it doesn't own but only *during the EDR processing window*.

Yes, by treating AER bits like DPC bits I meant I'd define the specific time windows when OS can touch the AER status bits similar to how it's done for DPC in the current ECN.

Makes sense, thanks.

Quoted 20 lines

Quoted 20 lines

Quoted 11 lines

Quoted 6 lines

Quoted 4 lines

For the normative text describing when OS clears the AER bits following the informative flow chart, it could say that OS clears AER as soon as possible after OST returns and before OS processes _HPX and loading drivers. Open to other suggestions as well.

I'm not sure what to do with "as soon as possible" either. That doesn't seem like something firmware and the OS can agree on.

I can just state that it's done after OST returns but before _HPX or driver is loaded. Any time in that range is fine. I can't get super specific here because different OSes do different things. Even for a given OS they change over time. And I need something generic enough to support a wide variety of OS implementations.

Yeah. I don't know how to solve this.

Linux doesn't actually unload and reload drivers for the child devices (Sathy, correct me if I'm wrong here) even though DPC containment takes the link down and effectively unplugs and replugs the device. I would *like* to handle it like hotplug, but some higher-level software doesn't deal well with things like storage devices disappearing and reappearing.

Quoted 7 lines

Quoted 3 lines

Since Linux doesn't actually re-enumerate the child devices, it wouldn't evaluate _HPX again. It would probably be cleaner if it did, but it's all tied up with the whole unplug/replug problem.

DPC resets everything below it and so to get it back up and running it would mean that all buses and resources need to be assigned, _HPX evaluated, and drivers reloaded. If those things don't happen then the whole hierarchy below the port that triggered DPC will be inaccessible.

Hmm, I think I might be confusing this with another situation. Sathy, can you help me understand this? I don't have a way to actually exercise this EDR path. Is there some way the pciehp hotplug driver gets involved here?

Here's how this seems to work as far as I can tell:

- Linux does not have DPC or AER control

- Linux installs EDR notify handler

- Linux evaluates DPC Enable _DSM

- DPC containment event occurs

- Firmware fields DPC interrupt

- DPC event is not a surprise remove

- Firmware sends EDR notification

- Linux EDR notify handler evaluates Locate _DSM

- Linux reads and logs DPC and AER error information for port in containment mode. [If it was an RP PIO error, Linux clears RP PIO error status, which is an asymmetry with the non-RP PIO path.]

- Linux clears AER error status (pci_aer_raw_clear_status())

- Linux calls driver .error_detected() methods for all child devices of the port in containment mode (pcie_do_recovery()). These devices are inaccessible because the link is down.

- Linux clears DPC Trigger Status (dpc_reset_link() from

     pcie_do_recovery()).

- Linux calls driver .mmio_enabled() methods for all child devices.

This is where I get lost. These child devices are now accessible, but they've been reset, so I don't know how their config space got restored. Did pciehp enumerate them? Did we do something like pci_restore_state()? I don't see where either of these happens.

So they want to basically do native AER handling even though firmware owns AER? My head hurts.

Bjorn

[EXTERNAL EMAIL]

Hi,

Quoted 47 lines · Bjorn Helgaas

Quoted 7 lines · On Wed, Mar 11, 2020 at 05:27:35PM +0000, [email protected] wrote:

Quoted 2 lines · Bjorn Helgaas

[EXTERNAL EMAIL]

<SNIP>

Quoted 3 lines

I'm probably missing your intent, but that sounds like "the OS can read/write AER bits whenever it wants, regardless of ownership."

Quoted 6 lines

Quoted 3 lines

That doesn't sound practical to me, and I don't think it's really similar to DPC, where it's pretty clear that the OS can touch DPC bits it doesn't own but only *during the EDR processing window*.

Yes, by treating AER bits like DPC bits I meant I'd define the specific time windows when OS can touch the AER status bits similar to how it's done for DPC in the current ECN.

Makes sense, thanks.

Quoted 20 lines

Quoted 20 lines

Quoted 11 lines

Quoted 6 lines

Quoted 4 lines

For the normative text describing when OS clears the AER bits following the informative flow chart, it could say that OS clears AER as soon as possible after OST returns and before OS processes _HPX and loading drivers. Open to other suggestions as well.

I'm not sure what to do with "as soon as possible" either. That doesn't seem like something firmware and the OS can agree on.

I can just state that it's done after OST returns but before _HPX or driver is loaded. Any time in that range is fine. I can't get super specific here because different OSes do different things. Even for a given OS they change over time. And I need something generic enough to support a wide variety of OS implementations.

Yeah. I don't know how to solve this.

Linux doesn't actually unload and reload drivers for the child devices (Sathy, correct me if I'm wrong here) even though DPC containment takes the link down and effectively unplugs and replugs the device. I would *like* to handle it like hotplug, but some higher-level software doesn't deal well with things like storage devices disappearing and reappearing.

Quoted 7 lines

Quoted 3 lines

Since Linux doesn't actually re-enumerate the child devices, it wouldn't evaluate _HPX again. It would probably be cleaner if it did, but it's all tied up with the whole unplug/replug problem.

DPC resets everything below it and so to get it back up and running it would mean that all buses and resources need to be assigned, _HPX evaluated, and drivers reloaded. If those things don't happen then the whole hierarchy below the port that triggered DPC will be inaccessible.

Hmm, I think I might be confusing this with another situation. Sathy, can you help me understand this? I don't have a way to actually exercise this EDR path. Is there some way the pciehp hotplug driver gets involved here?

Quoted 32 lines

Here's how this seems to work as far as I can tell:

- Linux does not have DPC or AER control

- Linux installs EDR notify handler

- Linux evaluates DPC Enable _DSM

- DPC containment event occurs

- Firmware fields DPC interrupt

- DPC event is not a surprise remove

- Firmware sends EDR notification

- Linux EDR notify handler evaluates Locate _DSM

- Linux reads and logs DPC and AER error information for port in containment mode. [If it was an RP PIO error, Linux clears RP PIO error status, which is an asymmetry with the non-RP PIO path.]

- Linux clears AER error status (pci_aer_raw_clear_status())

- Linux calls driver .error_detected() methods for all child devices of the port in containment mode (pcie_do_recovery()). These devices are inaccessible because the link is down.

- Linux clears DPC Trigger Status (dpc_reset_link() from pcie_do_recovery()).

Quoted 6 lines

- Linux calls driver .mmio_enabled() methods for all child devices.

This is where I get lost. These child devices are now accessible, but they've been reset, so I don't know how their config space got restored. Did pciehp enumerate them? Did we do something like pci_restore_state()? I don't see where either of these happens.

AFAIK, AER error status registers  are sticky (RW1CS) and hence will be preserved during reset.

Quoted 3 lines

So they want to basically do native AER handling even though firmware owns AER? My head hurts.

No, Its meant only for clearing AER registers. In EDR path, since OS owns clearing DPC registers, they want to let OS own clearing AER registers as well. Also,  it would give OS a chance to decide whether we want to keep the device on based on error status and history of the device attached.

Quoted 2 lines

Bjorn

Quoted 48 lines · Kuppuswamy Sathyanarayanan

[EXTERNAL EMAIL]

Hi,

Quoted 43 lines · Bjorn Helgaas

Quoted 7 lines · On Wed, Mar 11, 2020 at 05:27:35PM +0000, [email protected] wrote:

Quoted 2 lines · Bjorn Helgaas

[EXTERNAL EMAIL]

<SNIP>

Quoted 3 lines

I'm probably missing your intent, but that sounds like "the OS can read/write AER bits whenever it wants, regardless of ownership."

Quoted 6 lines

Quoted 3 lines

That doesn't sound practical to me, and I don't think it's really similar to DPC, where it's pretty clear that the OS can touch DPC bits it doesn't own but only *during the EDR processing window*.

Yes, by treating AER bits like DPC bits I meant I'd define the specific time windows when OS can touch the AER status bits similar to how it's done for DPC in the current ECN.

Makes sense, thanks.

Quoted 27 lines

Quoted 20 lines

Quoted 11 lines

Quoted 6 lines

Quoted 4 lines

For the normative text describing when OS clears the AER bits following the informative flow chart, it could say that OS clears AER as soon as possible after OST returns and before OS processes _HPX and loading drivers. Open to other suggestions as well.

I'm not sure what to do with "as soon as possible" either. That doesn't seem like something firmware and the OS can agree on.

I can just state that it's done after OST returns but before _HPX or driver is loaded. Any time in that range is fine. I can't get super specific here because different OSes do different things. Even for a given OS they change over time. And I need something generic enough to support a wide variety of OS implementations.

Yeah. I don't know how to solve this.

Linux doesn't actually unload and reload drivers for the child devices (Sathy, correct me if I'm wrong here) even though DPC containment takes the link down and effectively unplugs and replugs the device. I would *like* to handle it like hotplug, but some higher-level software doesn't deal well with things like storage devices disappearing and reappearing.

Quoted 3 lines

Since Linux doesn't actually re-enumerate the child devices, it wouldn't evaluate _HPX again. It would probably be cleaner if it did, but it's all tied up with the whole unplug/replug problem.

DPC resets everything below it and so to get it back up and running it would mean that all buses and resources need to be assigned, _HPX evaluated, and drivers reloaded. If those things don't happen then the whole hierarchy below the port that triggered DPC will be inaccessible.

Quoted 4 lines

Quoted 4 lines

Hmm, I think I might be confusing this with another situation. Sathy, can you help me understand this? I don't have a way to actually exercise this EDR path. Is there some way the pciehp hotplug driver gets involved here?

If the port has hot-plug enabled then DPC trigger will cause the link to go down (disabled state) and will generate a DLLSC hot-plug interrupt. When DPC is released, the link will become active and generate another DLLSC hot-plug interrupt.

Quoted 32 lines

Quoted 32 lines

Here's how this seems to work as far as I can tell:

- Linux does not have DPC or AER control

- Linux installs EDR notify handler

- Linux evaluates DPC Enable _DSM

- DPC containment event occurs

- Firmware fields DPC interrupt

- DPC event is not a surprise remove

- Firmware sends EDR notification

- Linux EDR notify handler evaluates Locate _DSM

- Linux reads and logs DPC and AER error information for port in containment mode. [If it was an RP PIO error, Linux clears RP PIO error status, which is an asymmetry with the non-RP PIO path.]

- Linux clears AER error status (pci_aer_raw_clear_status())

- Linux calls driver .error_detected() methods for all child devices of the port in containment mode (pcie_do_recovery()). These devices are inaccessible because the link is down.

- Linux clears DPC Trigger Status (dpc_reset_link() from pcie_do_recovery()).

Quoted 8 lines

Quoted 6 lines

- Linux calls driver .mmio_enabled() methods for all child devices.

This is where I get lost. These child devices are now accessible, but they've been reset, so I don't know how their config space got restored. Did pciehp enumerate them? Did we do something like pci_restore_state()? I don't see where either of these happens.

AFAIK, AER error status registers  are sticky (RW1CS) and hence will be preserved during reset.

In our testing, the device directly connected to the port that was contained does get reprogrammed and the driver is reloaded. These are hot-plug slots and so might be due to DLLSC hot-plug interrupt when containment is released and link goes back to active state.

However, if a switch is connected to the port where DPC was triggered then we do not see the whole switch hierarchy being re-enumerated.

Also, DPC could be enabled on non-hot-plug slots so can't always rely on hot-plug to re-init devices in the recovery path.

Quoted 8 lines

Quoted 3 lines

So they want to basically do native AER handling even though firmware owns AER? My head hurts.

No, Its meant only for clearing AER registers. In EDR path, since OS owns clearing DPC registers, they want to let OS own clearing AER registers as well. Also,  it would give OS a chance to decide whether we want to keep the device on based on error status and history of the device attached.

Right. The way it was pitched to me was that the OSVs wanted to read/clear the error status bits so they could re-use the code that does that when OS natively owns AER/DPC.

Quoted 1 line

Quoted 1 line

Bjorn

Quoted 43 lines · On 3/11/20 2:53 PM, [email protected] wrote:

Quoted 42 lines · Kuppuswamy Sathyanarayanan

Quoted 41 lines · Bjorn Helgaas

Quoted 11 lines · On Wed, Mar 11, 2020 at 05:27:35PM +0000, [email protected] wrote:

On 3/11/2020 12:12 PM, Bjorn Helgaas wrote:
<SNIP>

Quoted 6 lines

I'm probably missing your intent, but that sounds like "the OS can read/write AER bits whenever it wants, regardless of ownership."

That doesn't sound practical to me, and I don't think it's really similar to DPC, where it's pretty clear that the OS can touch DPC bits it doesn't own but only *during the EDR processing window*.

Yes, by treating AER bits like DPC bits I meant I'd define the specific time windows when OS can touch the AER status bits similar to how it's done for DPC in the current ECN.

Makes sense, thanks.

Quoted 27 lines

Quoted 20 lines

Quoted 11 lines

Quoted 6 lines

Quoted 4 lines

For the normative text describing when OS clears the AER bits following the informative flow chart, it could say that OS clears AER as soon as possible after OST returns and before OS processes _HPX and loading drivers. Open to other suggestions as well.

I'm not sure what to do with "as soon as possible" either. That doesn't seem like something firmware and the OS can agree on.

I can just state that it's done after OST returns but before _HPX or driver is loaded. Any time in that range is fine. I can't get super specific here because different OSes do different things. Even for a given OS they change over time. And I need something generic enough to support a wide variety of OS implementations.

Yeah. I don't know how to solve this.

Linux doesn't actually unload and reload drivers for the child devices (Sathy, correct me if I'm wrong here) even though DPC containment takes the link down and effectively unplugs and replugs the device. I would *like* to handle it like hotplug, but some higher-level software doesn't deal well with things like storage devices disappearing and reappearing.

Quoted 3 lines

Since Linux doesn't actually re-enumerate the child devices, it wouldn't evaluate _HPX again. It would probably be cleaner if it did, but it's all tied up with the whole unplug/replug problem.

DPC resets everything below it and so to get it back up and running it would mean that all buses and resources need to be assigned, _HPX evaluated, and drivers reloaded. If those things don't happen then the whole hierarchy below the port that triggered DPC will be inaccessible.

Quoted 8 lines

Quoted 4 lines

Quoted 4 lines

Hmm, I think I might be confusing this with another situation. Sathy, can you help me understand this? I don't have a way to actually exercise this EDR path. Is there some way the pciehp hotplug driver gets involved here?

If the port has hot-plug enabled then DPC trigger will cause the link to go down (disabled state) and will generate a DLLSC hot-plug interrupt. When DPC is released, the link will become active and generate another DLLSC hot-plug interrupt.

Yes, device/driver enumeration and removal will triggered by DLLSC state change interrupt in pciehp driver.

Quoted 40 lines

Quoted 32 lines

Quoted 32 lines

Here's how this seems to work as far as I can tell:

- Linux does not have DPC or AER control

- Linux installs EDR notify handler

- Linux evaluates DPC Enable _DSM

- DPC containment event occurs

- Firmware fields DPC interrupt

- DPC event is not a surprise remove

- Firmware sends EDR notification

- Linux EDR notify handler evaluates Locate _DSM

- Linux reads and logs DPC and AER error information for port in containment mode. [If it was an RP PIO error, Linux clears RP PIO error status, which is an asymmetry with the non-RP PIO path.]

- Linux clears AER error status (pci_aer_raw_clear_status())

- Linux calls driver .error_detected() methods for all child devices of the port in containment mode (pcie_do_recovery()). These devices are inaccessible because the link is down.

- Linux clears DPC Trigger Status (dpc_reset_link() from pcie_do_recovery()).

Quoted 8 lines

Quoted 6 lines

- Linux calls driver .mmio_enabled() methods for all child devices.

This is where I get lost. These child devices are now accessible, but they've been reset, so I don't know how their config space got restored. Did pciehp enumerate them? Did we do something like pci_restore_state()? I don't see where either of these happens.

AFAIK, AER error status registers  are sticky (RW1CS) and hence will be preserved during reset.

Quoted 7 lines

In our testing, the device directly connected to the port that was contained does get reprogrammed and the driver is reloaded. These are hot-plug slots and so might be due to DLLSC hot-plug interrupt when containment is released and link goes back to active state.

However, if a switch is connected to the port where DPC was triggered then we do not see the whole switch hierarchy being re-enumerated.

Now that I have a hardware to verify this scenario, I will look into it. I suspect there is a transient state in link status which causes this disconnect issue. But I think this issue is not related to EDR support and hence should be reproducible in native handling as well.

Quoted 1 line

Quoted 2 lines

Also, DPC could be enabled on non-hot-plug slots so can't always rely on hot-plug to re-init devices in the recovery path.

If hotplug is not supported then there is support to enumerate devices via polling  or ACPI events. But a point to note here is, enumeration path is independent of error handler path, and hence there is no explicit trigger or event from error handler path to enumeration path to kick start the enumeration.

Is any synchronization needed here between the EDR path and the hotplug/enumeration path?

Quoted 2 lines · Bjorn Helgaas

Is any synchronization needed here between the EDR path and the hotplug/enumeration path?

If we want to follow the implementation note step by step (in sequence) then we need some synchronization between EDR path and enumeration path. But if it's OK to achieve the same end result by following steps out of sequence then we don't need to create any dependency between EDR and enumeration paths. Currently we follow the latter approach.

For example, consider the case in flow chart where after sending success _OST, firmware decides to stop the recovery of the device.

if we follow the flow chart as is then the steps should be,

1. clear the DPC status trigger
2. Send success code via _OST, and wait for return from _OST
3. if successful return then enumerate the child devices and reassign bus numbers.

In current approach the steps followed are,

1. Clear the DPC status trigger.
2. Send success code via _OST
2. In parallel, LINK UP event path will enumerate the child devices.
3. if firmware decides not to recover the device, then LINK DOWN event will eventually remove them again.

[EXTERNAL EMAIL]

Quoted 10 lines · Kuppuswamy Sathyanarayanan

Quoted 2 lines · Bjorn Helgaas

Is any synchronization needed here between the EDR path and the hotplug/enumeration path?

If we want to follow the implementation note step by step (in sequence) then we need some synchronization between EDR path and enumeration path. But if it's OK to achieve the same end result by following steps out of sequence then we don't need to create any dependency between EDR and enumeration paths. Currently we follow the latter approach.

What would the synchronization look like?

Ideally I think it would be better to follow the order in the flowchart if it's not too onerous. That will make the code easier to understand. The current situation with this dependency on pciehp and what it will do leaves a lot of things implicit.

What happens if CONFIG_PCIE_EDR=y but CONFIG_HOTPLUG_PCI_PCIE=n?

IIUC, when DPC triggers, pciehp is what fields the DLLSC interrupt and unbinds the drivers and removes the devices. If that doesn't happen, and Linux clears the DPC trigger to bring the link back up, will those drivers try to operate uninitialized devices?

Does EDR need a dependency on CONFIG_HOTPLUG_PCI_PCIE?

Quoted 11 lines

For example, consider the case in flow chart where after sending success _OST, firmware decides to stop the recovery of the device.

if we follow the flow chart as is then the steps should be,

1. clear the DPC status trigger
2. Send success code via _OST, and wait for return from _OST
3. if successful return then enumerate the child devices and reassign bus numbers.

Quoted 4 lines

In current approach the steps followed are,

1. Clear the DPC status trigger.
2. Send success code via _OST

Quoted 3 lines

2. In parallel, LINK UP event path will enumerate the child devices.
3. if firmware decides not to recover the device, then LINK DOWN event will eventually remove them again.

Quoted 19 lines · Bjorn Helgaas

[EXTERNAL EMAIL]

Quoted 9 lines · Kuppuswamy Sathyanarayanan

Quoted 2 lines · Bjorn Helgaas

Is any synchronization needed here between the EDR path and the hotplug/enumeration path?

If we want to follow the implementation note step by step (in sequence) then we need some synchronization between EDR path and enumeration path. But if it's OK to achieve the same end result by following steps out of sequence then we don't need to create any dependency between EDR and enumeration paths. Currently we follow the latter approach.

What would the synchronization look like?

Ideally I think it would be better to follow the order in the flowchart if it's not too onerous. That will make the code easier to understand. The current situation with this dependency on pciehp and what it will do leaves a lot of things implicit.

Quoted 8 lines

What happens if CONFIG_PCIE_EDR=y but CONFIG_HOTPLUG_PCI_PCIE=n?

IIUC, when DPC triggers, pciehp is what fields the DLLSC interrupt and unbinds the drivers and removes the devices. If that doesn't happen, and Linux clears the DPC trigger to bring the link back up, will those drivers try to operate uninitialized devices?

Does EDR need a dependency on CONFIG_HOTPLUG_PCI_PCIE?

From one of Sathya's other responses:

"If hotplug is not supported then there is support to enumerate devices via polling or ACPI events. But a point to note here is, enumeration path is independent of error handler path, and hence there is no explicit trigger or event from error handler path to enumeration path to kick start the enumeration."

The EDR standard doesn't have any dependency on hot-plug. It sounds like in the current implementation there's some manual intervention needed if hot-plug is not supported?

Ideally recovery would kick in automatically but requiring manual intervention is a good first step.

Quoted 10 lines

Quoted 10 lines

For example, consider the case in flow chart where after sending success _OST, firmware decides to stop the recovery of the device.

if we follow the flow chart as is then the steps should be,

1. clear the DPC status trigger
2. Send success code via _OST, and wait for return from _OST
3. if successful return then enumerate the child devices and reassign bus numbers.

Quoted 4 lines

Quoted 4 lines

In current approach the steps followed are,

1. Clear the DPC status trigger.
2. Send success code via _OST

Success in step 2 is assuming device trained and config space is accessible correct?

If device was removed or device config space is not accessible then failure status should be sent via _OST.

Quoted 4 lines

Quoted 4 lines

2. In parallel, LINK UP event path will enumerate the child devices.
3. if firmware decides not to recover the device, then LINK DOWN event will eventually remove them again.

Hi,

Quoted 34 lines · On 3/12/20 2:02 PM, [email protected] wrote:

Quoted 17 lines · Bjorn Helgaas

Quoted 9 lines · Kuppuswamy Sathyanarayanan

Quoted 2 lines · Bjorn Helgaas

Is any synchronization needed here between the EDR path and the hotplug/enumeration path?

If we want to follow the implementation note step by step (in sequence) then we need some synchronization between EDR path and enumeration path. But if it's OK to achieve the same end result by following steps out of sequence then we don't need to create any dependency between EDR and enumeration paths. Currently we follow the latter approach.

What would the synchronization look like?

Ideally I think it would be better to follow the order in the flowchart if it's not too onerous. That will make the code easier to understand. The current situation with this dependency on pciehp and what it will do leaves a lot of things implicit.

Quoted 8 lines

What happens if CONFIG_PCIE_EDR=y but CONFIG_HOTPLUG_PCI_PCIE=n?

IIUC, when DPC triggers, pciehp is what fields the DLLSC interrupt and unbinds the drivers and removes the devices. If that doesn't happen, and Linux clears the DPC trigger to bring the link back up, will those drivers try to operate uninitialized devices?

Does EDR need a dependency on CONFIG_HOTPLUG_PCI_PCIE?

From one of Sathya's other responses:

"If hotplug is not supported then there is support to enumerate devices via polling or ACPI events. But a point to note here is, enumeration path is independent of error handler path, and hence there is no explicit trigger or event from error handler path to enumeration path to kick start the enumeration."

Quoted 3 lines

The EDR standard doesn't have any dependency on hot-plug. It sounds like in the current implementation there's some manual intervention needed if hot-plug is not supported?

No, there is no need for manual intervention even in non hotplug cases.

For ACPI events case, we would rely on ACPI event to kick start the enumeration.  And for polling model, there is an independent polling thread which will kick start the enumeration.

Above both enumeration models are totally independent and has no dependency on error handler thread.

We will decide which model to use based on hardware capability and _OSC negotiation or kernel command line option.

Quoted 43 lines · Kuppuswamy Sathyanarayanan

Hi,

Quoted 34 lines · On 3/12/20 2:02 PM, [email protected] wrote:

Quoted 17 lines · Bjorn Helgaas

Quoted 9 lines · Kuppuswamy Sathyanarayanan

Quoted 2 lines · Bjorn Helgaas

Is any synchronization needed here between the EDR path and the hotplug/enumeration path?

If we want to follow the implementation note step by step (in sequence) then we need some synchronization between EDR path and enumeration path. But if it's OK to achieve the same end result by following steps out of sequence then we don't need to create any dependency between EDR and enumeration paths. Currently we follow the latter approach.

What would the synchronization look like?

Ideally I think it would be better to follow the order in the flowchart if it's not too onerous. That will make the code easier to understand. The current situation with this dependency on pciehp and what it will do leaves a lot of things implicit.

Quoted 8 lines

What happens if CONFIG_PCIE_EDR=y but CONFIG_HOTPLUG_PCI_PCIE=n?

IIUC, when DPC triggers, pciehp is what fields the DLLSC interrupt and unbinds the drivers and removes the devices. If that doesn't happen, and Linux clears the DPC trigger to bring the link back up, will those drivers try to operate uninitialized devices?

Does EDR need a dependency on CONFIG_HOTPLUG_PCI_PCIE?

From one of Sathya's other responses:

"If hotplug is not supported then there is support to enumerate devices via polling or ACPI events. But a point to note here is, enumeration path is independent of error handler path, and hence there is no explicit trigger or event from error handler path to enumeration path to kick start the enumeration."

Quoted 3 lines

The EDR standard doesn't have any dependency on hot-plug. It sounds like in the current implementation there's some manual intervention needed if hot-plug is not supported?

No, there is no need for manual intervention even in non hotplug cases.

Quoted 3 lines

For ACPI events case, we would rely on ACPI event to kick start the enumeration.  And for polling model, there is an independent polling thread which will kick start the enumeration.

I'm guessing the ACPI case works via hotplug_is_native(): if CONFIG_HOTPLUG_PCI_PCIE=n, pciehp_is_native() returns false, and acpiphp manages hotplug.

What if CONFIG_HOTPLUG_PCI_ACPI=n also?

Where is the polling thread?

Quoted 3 lines

Above both enumeration models are totally independent and has no dependency on error handler thread.

I see they're currently independent from the EDR thread, but it's not clear to me that there's no dependency. After all, both EDR and the hotplug paths are operating on the same devices at roughly the same time, so we should have some story about what keeps them from getting in each other's way.

Quoted 2 lines

We will decide which model to use based on hardware capability and _OSC negotiation or kernel command line option.

Quoted 47 lines · Bjorn Helgaas

Quoted 39 lines · Kuppuswamy Sathyanarayanan

Quoted 34 lines · On 3/12/20 2:02 PM, [email protected] wrote:

Quoted 17 lines · Bjorn Helgaas

Quoted 9 lines · Kuppuswamy Sathyanarayanan

Quoted 2 lines · Bjorn Helgaas

Is any synchronization needed here between the EDR path and the hotplug/enumeration path?

If we want to follow the implementation note step by step (in sequence) then we need some synchronization between EDR path and enumeration path. But if it's OK to achieve the same end result by following steps out of sequence then we don't need to create any dependency between EDR and enumeration paths. Currently we follow the latter approach.

What would the synchronization look like?

Ideally I think it would be better to follow the order in the flowchart if it's not too onerous. That will make the code easier to understand. The current situation with this dependency on pciehp and what it will do leaves a lot of things implicit.

Quoted 8 lines

What happens if CONFIG_PCIE_EDR=y but CONFIG_HOTPLUG_PCI_PCIE=n?

IIUC, when DPC triggers, pciehp is what fields the DLLSC interrupt and unbinds the drivers and removes the devices. If that doesn't happen, and Linux clears the DPC trigger to bring the link back up, will those drivers try to operate uninitialized devices?

Does EDR need a dependency on CONFIG_HOTPLUG_PCI_PCIE?

From one of Sathya's other responses:

"If hotplug is not supported then there is support to enumerate devices via polling or ACPI events. But a point to note here is, enumeration path is independent of error handler path, and hence there is no explicit trigger or event from error handler path to enumeration path to kick start the enumeration."

Quoted 3 lines

The EDR standard doesn't have any dependency on hot-plug. It sounds like in the current implementation there's some manual intervention needed if hot-plug is not supported?

Quoted 6 lines

No, there is no need for manual intervention even in non hotplug cases.

For ACPI events case, we would rely on ACPI event to kick start the enumeration.  And for polling model, there is an independent polling thread which will kick start the enumeration.

Quoted 5 lines

I'm guessing the ACPI case works via hotplug_is_native(): if CONFIG_HOTPLUG_PCI_PCIE=n, pciehp_is_native() returns false, and acpiphp manages hotplug.

What if CONFIG_HOTPLUG_PCI_ACPI=n also?

If none of the auto scans are enabled then we might need some manual trigger (rescan). But this would be needed in native DPC case as well.

Quoted 2 lines

Where is the polling thread?

drivers/pci/hotplug/pciehp_hpc.c

Quoted 9 lines · Kuppuswamy Sathyanarayanan

Quoted 2 lines · Bjorn Helgaas

Is any synchronization needed here between the EDR path and the hotplug/enumeration path?

If we want to follow the implementation note step by step (in sequence) then we need some synchronization between EDR path and enumeration path. But if it's OK to achieve the same end result by following steps out of sequence then we don't need to create any dependency between EDR and enumeration paths. Currently we follow the latter approach.

What would the synchronization look like?

Ideally I think it would be better to follow the order in the flowchart if it's not too onerous.

That will make the code easier to
understand. The current situation with this dependency on pciehp and what it will do leaves a lot of things implicit.

What happens if CONFIG_PCIE_EDR=y but CONFIG_HOTPLUG_PCI_PCIE=n?

IIUC, when DPC triggers, pciehp is what fields the DLLSC interrupt and unbinds the drivers and removes the devices.

If that doesn't happen,
and Linux clears the DPC trigger to bring the link back up, will those drivers try to operate uninitialized devices?

Does EDR need a dependency on CONFIG_HOTPLUG_PCI_PCIE?

Quoted 18 lines

For example, consider the case in flow chart where after sending success _OST, firmware decides to stop the recovery of the device.

if we follow the flow chart as is then the steps should be,

1. clear the DPC status trigger
2. Send success code via _OST, and wait for return from _OST
3. if successful return then enumerate the child devices and reassign bus numbers.

In current approach the steps followed are,

1. Clear the DPC status trigger.
2. Send success code via _OST
2. In parallel, LINK UP event path will enumerate the child devices.
3. if firmware decides not to recover the device, then LINK DOWN event will eventually remove them again.

Hi Bjorn,

Quoted 11 lines · Bjorn Helgaas

Quoted 9 lines · Kuppuswamy Sathyanarayanan

Quoted 2 lines · Bjorn Helgaas

Is any synchronization needed here between the EDR path and the hotplug/enumeration path?

If we want to follow the implementation note step by step (in sequence) then we need some synchronization between EDR path and enumeration path. But if it's OK to achieve the same end result by following steps out of sequence then we don't need to create any dependency between EDR and enumeration paths. Currently we follow the latter approach.

What would the synchronization look like?

we might need some way to disable the enumeration path till we get response from firmware.

In native hot plug case, I think we can do it in two ways.

1. Disable hotplug notification in slot ctl registers. (pcie_disable_notification())
2. Some how block hotplug driver from processing the new events (not sure how feasible its).

Following method 1 would be easy, But I am not sure whether its alright to disable them randomly. I think, unless we clear the status as well, we might get some issues due to stale notification history.

For ACPI event case, I am not sure whether we have some communication protocol in place to disable receiving ACPI events temporarily.

For polling model, we need to disable to the polling timer thread till we receive _OST response from firmware.

Quoted 3 lines

Ideally I think it would be better to follow the order in the flowchart if it's not too onerous.

None of the above changes will be pretty and I think it will not be simple as well.

Quoted 8 lines

That will make the code easier to
understand. The current situation with this dependency on pciehp and what it will do leaves a lot of things implicit.

What happens if CONFIG_PCIE_EDR=y but CONFIG_HOTPLUG_PCI_PCIE=n?

IIUC, when DPC triggers, pciehp is what fields the DLLSC interrupt and unbinds the drivers and removes the devices.

Quoted 3 lines

If that doesn't happen, and Linux clears the DPC trigger to bring the link back up, will those drivers try to operate uninitialized devices?

I don't think this will happen. In DPC reset_link before we bring up the device we wait for link to go down first using pcie_wait_for_link(pdev, false) function.

Quoted 49 lines · Kuppuswamy Sathyanarayanan

Hi Bjorn,

Quoted 11 lines · Bjorn Helgaas

Quoted 9 lines · Kuppuswamy Sathyanarayanan

Quoted 2 lines · Bjorn Helgaas

Is any synchronization needed here between the EDR path and the hotplug/enumeration path?

If we want to follow the implementation note step by step (in sequence) then we need some synchronization between EDR path and enumeration path. But if it's OK to achieve the same end result by following steps out of sequence then we don't need to create any dependency between EDR and enumeration paths. Currently we follow the latter approach.

What would the synchronization look like?

we might need some way to disable the enumeration path till we get response from firmware.

In native hot plug case, I think we can do it in two ways.

1. Disable hotplug notification in slot ctl registers. (pcie_disable_notification())
2. Some how block hotplug driver from processing the new events (not sure how feasible its).

Following method 1 would be easy, But I am not sure whether its alright to disable them randomly. I think, unless we clear the status as well, we might get some issues due to stale notification history.

For ACPI event case, I am not sure whether we have some communication protocol in place to disable receiving ACPI events temporarily.

For polling model, we need to disable to the polling timer thread till we receive _OST response from firmware.

Quoted 3 lines

Ideally I think it would be better to follow the order in the flowchart if it's not too onerous.

None of the above changes will be pretty and I think it will not be simple as well.

Quoted 8 lines

That will make the code easier to
understand. The current situation with this dependency on pciehp and what it will do leaves a lot of things implicit.

What happens if CONFIG_PCIE_EDR=y but CONFIG_HOTPLUG_PCI_PCIE=n?

IIUC, when DPC triggers, pciehp is what fields the DLLSC interrupt and unbinds the drivers and removes the devices.

Quoted 7 lines

Quoted 3 lines

If that doesn't happen, and Linux clears the DPC trigger to bring the link back up, will those drivers try to operate uninitialized devices?

I don't think this will happen. In DPC reset_link before we bring up the device we wait for link to go down first using pcie_wait_for_link(pdev, false) function.

I understand that, but these child devices were reset when DPC disabled the link. When the link comes back up, their BARs contain zeros.

If CONFIG_HOTPLUG_PCI_PCIE=y, the DLLSC interrupt will cause pciehp to unbind the driver. It seems like the unbind races with the EDR notify handler. '

pcie_do_recovery(), this seems fine -- we'll call dpc_reset_link(), which brings up the link, we won't call any driver callbacks because there's no driver, and another DLLSC interrupt will cause pciehp to re-enumerate, which will re-initialize the device, then rebind the driver.

If the EDR notify handler runs before pciehp unbinds the driver,

couldn't EDR bring up the link and call driver .mmio_enabled() before the device has been initialized?

If CONFIG_HOTPLUG_PCI_PCIE=n and CONFIG_HOTPLUG_PCI_ACPI=y, I could believe that the situations are similar to the above.

What if CONFIG_HOTPLUG_PCI_PCIE=n and CONFIG_HOTPLUG_PCI_ACPI=n? Then I assume there's nothing to unbind the driver, so pcie_do_recovery() will call the driver .mmio_enabled() and other recovery callbacks on a device that hasn't been initialized?

Quoted 56 lines · Bjorn Helgaas

Quoted 47 lines · Kuppuswamy Sathyanarayanan

Quoted 11 lines · Bjorn Helgaas

Quoted 9 lines · Kuppuswamy Sathyanarayanan

Quoted 2 lines · Bjorn Helgaas

Is any synchronization needed here between the EDR path and the hotplug/enumeration path?

If we want to follow the implementation note step by step (in sequence) then we need some synchronization between EDR path and enumeration path. But if it's OK to achieve the same end result by following steps out of sequence then we don't need to create any dependency between EDR and enumeration paths. Currently we follow the latter approach.

What would the synchronization look like?

we might need some way to disable the enumeration path till we get response from firmware.

In native hot plug case, I think we can do it in two ways.

1. Disable hotplug notification in slot ctl registers. (pcie_disable_notification())
2. Some how block hotplug driver from processing the new events (not sure how feasible its).

Following method 1 would be easy, But I am not sure whether its alright to disable them randomly. I think, unless we clear the status as well, we might get some issues due to stale notification history.

For ACPI event case, I am not sure whether we have some communication protocol in place to disable receiving ACPI events temporarily.

For polling model, we need to disable to the polling timer thread till we receive _OST response from firmware.

Quoted 3 lines

Ideally I think it would be better to follow the order in the flowchart if it's not too onerous.

None of the above changes will be pretty and I think it will not be simple as well.

Quoted 8 lines

That will make the code easier to
understand. The current situation with this dependency on pciehp and what it will do leaves a lot of things implicit.

What happens if CONFIG_PCIE_EDR=y but CONFIG_HOTPLUG_PCI_PCIE=n?

IIUC, when DPC triggers, pciehp is what fields the DLLSC interrupt and unbinds the drivers and removes the devices.

Quoted 7 lines

Quoted 3 lines

If that doesn't happen, and Linux clears the DPC trigger to bring the link back up, will those drivers try to operate uninitialized devices?

I don't think this will happen. In DPC reset_link before we bring up the device we wait for link to go down first using pcie_wait_for_link(pdev, false) function.

Quoted 7 lines

I understand that, but these child devices were reset when DPC disabled the link. When the link comes back up, their BARs contain zeros.

If CONFIG_HOTPLUG_PCI_PCIE=y, the DLLSC interrupt will cause pciehp to unbind the driver. It seems like the unbind races with the EDR notify handler.

Agree. But even if there is a race condition, after clearing DPC trigger status, if hotplug driver properly removes/re-enumerates the driver then the end result will still be same. There should be no functional impact.

Quoted 8 lines

If pciehp unbinds the driver before edr_handle_event() calls pcie_do_recovery(), this seems fine -- we'll call dpc_reset_link(), which brings up the link, we won't call any driver callbacks because there's no driver, and another DLLSC interrupt will cause pciehp to re-enumerate, which will re-initialize the device, then rebind the driver.

If the EDR notify handler runs before pciehp unbinds the driver,

In the above case, from the kernel perspective device is still accessible and IIUC, it will try to recover it in pcie_do_recovery() using one of the callbacks.

int (*mmio_enabled)(struct pci_dev *dev);
int (*slot_reset)(struct pci_dev *dev);
void (*resume)(struct pci_dev *dev);

One of these callbacks will do pci_restore_state() to restore the device, and IO will not attempted in these callbacks until the device is successfully recovered.

Quoted 2 lines

couldn't EDR bring up the link and call driver .mmio_enabled() before the device has been initialized?

Calling mmio_enabled in this case should not be a problem right?

Please check the following content from Documentation/PCI/pci-error-recovery.rst. IIUC (following content), IO will not be attempted until the device is successfully re-configured.

STEP 2: MMIO Enabled
--------------------
This callback is made if all drivers on a segment agree that they can try to recover and if no automatic link reset was performed by the HW. If the platform can't just re-enable IOs without a slot reset or a link reset, it will not call this callback, and instead will have gone directly to STEP 3 (Link Reset) or STEP 4 (Slot Reset)

Quoted 3 lines

If CONFIG_HOTPLUG_PCI_PCIE=n and CONFIG_HOTPLUG_PCI_ACPI=y, I could believe that the situations are similar to the above.

Quoted 4 lines

What if CONFIG_HOTPLUG_PCI_PCIE=n and CONFIG_HOTPLUG_PCI_ACPI=n? Then I assume there's nothing to unbind the driver, so pcie_do_recovery() will call the driver .mmio_enabled() and other recovery callbacks on a device that hasn't been initialized?

probably in .slot_reset() callback device config will be restored and it will make the device functional again.

Also since in above case hotplug is not supported, topology change will not be supported.

[+cc Russell, Sam, Oliver since we're talking about the error recovery flow. The code we're talking about is at [1]]

Quoted 77 lines · Sathyanarayanan

Quoted 52 lines · Bjorn Helgaas

Quoted 43 lines · Kuppuswamy Sathyanarayanan

Quoted 11 lines · Bjorn Helgaas

Quoted 9 lines · Kuppuswamy Sathyanarayanan

Quoted 2 lines · Bjorn Helgaas

Is any synchronization needed here between the EDR path and the hotplug/enumeration path?

If we want to follow the implementation note step by step (in sequence) then we need some synchronization between EDR path and enumeration path. But if it's OK to achieve the same end result by following steps out of sequence then we don't need to create any dependency between EDR and enumeration paths. Currently we follow the latter approach.

What would the synchronization look like?

we might need some way to disable the enumeration path till we get response from firmware.

In native hot plug case, I think we can do it in two ways.

1. Disable hotplug notification in slot ctl registers. (pcie_disable_notification())
2. Some how block hotplug driver from processing the new events (not sure how feasible its).

Following method 1 would be easy, But I am not sure whether its alright to disable them randomly. I think, unless we clear the status as well, we might get some issues due to stale notification history.

For ACPI event case, I am not sure whether we have some communication protocol in place to disable receiving ACPI events temporarily.

For polling model, we need to disable to the polling timer thread till we receive _OST response from firmware.

Quoted 2 lines

Ideally I think it would be better to follow the order in the flowchart if it's not too onerous.

None of the above changes will be pretty and I think it will not be simple as well.

Quoted 6 lines

That will make the code easier to
understand. The current situation with this dependency on pciehp and what it will do leaves a lot of things implicit.

What happens if CONFIG_PCIE_EDR=y but CONFIG_HOTPLUG_PCI_PCIE=n?

Quoted 8 lines

Quoted 5 lines

IIUC, when DPC triggers, pciehp is what fields the DLLSC interrupt and unbinds the drivers and removes the devices.
If that doesn't happen, and Linux clears the DPC trigger to bring the link back up, will those drivers try to operate uninitialized devices?

I don't think this will happen. In DPC reset_link before we bring up the device we wait for link to go down first using pcie_wait_for_link(pdev, false) function.

Quoted 7 lines

I understand that, but these child devices were reset when DPC disabled the link. When the link comes back up, their BARs contain zeros.

If CONFIG_HOTPLUG_PCI_PCIE=y, the DLLSC interrupt will cause pciehp to unbind the driver. It seems like the unbind races with the EDR notify handler.

Agree. But even if there is a race condition, after clearing DPC trigger status, if hotplug driver properly removes/re-enumerates the driver then the end result will still be same. There should be no functional impact.

Quoted 8 lines

If pciehp unbinds the driver before edr_handle_event() calls pcie_do_recovery(), this seems fine -- we'll call dpc_reset_link(), which brings up the link, we won't call any driver callbacks because there's no driver, and another DLLSC interrupt will cause pciehp to re-enumerate, which will re-initialize the device, then rebind the driver.

If the EDR notify handler runs before pciehp unbinds the driver,

In the above case, from the kernel perspective device is still accessible and IIUC, it will try to recover it in pcie_do_recovery() using one of the callbacks.

Quoted 7 lines

int (*mmio_enabled)(struct pci_dev *dev);
int (*slot_reset)(struct pci_dev *dev);
void (*resume)(struct pci_dev *dev);

One of these callbacks will do pci_restore_state() to restore the device, and IO will not attempted in these callbacks until the device is successfully recovered.

That might be what *should* happen, but I don't think it's what *does* happen.

I don't think we use .mmio_enabled() and .slot_reset() for EDR because Linux EDR currently depends on DPC, so we'll be using dpc_reset_link(), which normally returns PCI_ERS_RESULT_RECOVERED, so pcie_do_recovery() skips .mmio_enabled() and .slot_reset().

I looked at the first few .resume() implementations (FWIW, I used [2] to find them), and none of them calls pci_restore_state() before doing I/O to the device:

   ioat_pcie_error_resume()
   pci_resume() (hfi1)
   qib_pci_resume()
   cxl_pci_resume()
   genwqe_err_resume()
   ...

But I assume you've tested EDR with some driver that *does* call pci_restore_state()? Or maybe you have pciehp enabled,

and it always
wins the race and unbinds the driver before the EDR notification? It would be interesting to make pciehp *lose* the race and see if anything breaks.

pci-error-recovery.rst does not mention any requirement for the driver to call pci_restore_state(), and I think any state restoration like that should be the responsibility of the PCI core, not the driver.

Quoted 20 lines

Quoted 2 lines

couldn't EDR bring up the link and call driver .mmio_enabled() before the device has been initialized?

Calling mmio_enabled in this case should not be a problem right?

Please check the following content from Documentation/PCI/pci-error-recovery.rst. IIUC (following content), IO will not be attempted until the device is successfully re-configured.

STEP 2: MMIO Enabled
--------------------
This callback is made if all drivers on a segment agree that they can try to recover and if no automatic link reset was performed by the HW. If the platform can't just re-enable IOs without a slot reset or a link reset, it will not call this callback, and instead will have gone directly to STEP 3 (Link Reset) or STEP 4 (Slot Reset)

Quoted 3 lines

If CONFIG_HOTPLUG_PCI_PCIE=n and CONFIG_HOTPLUG_PCI_ACPI=y, I could believe that the situations are similar to the above.

Quoted 6 lines

Quoted 4 lines

What if CONFIG_HOTPLUG_PCI_PCIE=n and CONFIG_HOTPLUG_PCI_ACPI=n? Then I assume there's nothing to unbind the driver, so pcie_do_recovery() will call the driver .mmio_enabled() and other recovery callbacks on a device that hasn't been initialized?

probably in .slot_reset() callback device config will be restored and it will make the device functional again.

I don't think .mmio_enabled() is a problem because IIUC, the device should not have been reset before calling .mmio_enabled().

But I think .slot_reset() *is* a problem. I looked at several .slot_reset() implementations ([3]); some called pci_restore_state(), but many did not.

If no hotplug driver is enabled, I think the .slot_reset() callbacks that do not call pci_restore_state() are broken.

Quoted 3 lines

Also since in above case hotplug is not supported, topology change will not be supported.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=review/edr
[2] F='\.resume'; git grep -A 10 "struct pci_error_handlers" | grep "$F\s*=" | sed -e "s/.*$F\s*=\s*//" -e 's/,\s*$//'
[3] F='\.slot_reset'; git grep -A 10 "struct pci_error_handlers" | grep "$F\s*=" | sed -e "s/.*$F\s*=\s*//" -e 's/,\s*$//'

Quoted 88 lines · Bjorn Helgaas

[+cc Russell, Sam, Oliver since we're talking about the error recovery flow. The code we're talking about is at [1]]

Quoted 77 lines · Sathyanarayanan

Quoted 52 lines · Bjorn Helgaas

Quoted 43 lines · Kuppuswamy Sathyanarayanan

Quoted 11 lines · Bjorn Helgaas

Quoted 9 lines · Kuppuswamy Sathyanarayanan

Quoted 2 lines · Bjorn Helgaas

Is any synchronization needed here between the EDR path and the hotplug/enumeration path?

If we want to follow the implementation note step by step (in sequence) then we need some synchronization between EDR path and enumeration path. But if it's OK to achieve the same end result by following steps out of sequence then we don't need to create any dependency between EDR and enumeration paths. Currently we follow the latter approach.

What would the synchronization look like?

we might need some way to disable the enumeration path till we get response from firmware.

In native hot plug case, I think we can do it in two ways.

1. Disable hotplug notification in slot ctl registers. (pcie_disable_notification())
2. Some how block hotplug driver from processing the new events (not sure how feasible its).

Following method 1 would be easy, But I am not sure whether its alright to disable them randomly. I think, unless we clear the status as well, we might get some issues due to stale notification history.

For ACPI event case, I am not sure whether we have some communication protocol in place to disable receiving ACPI events temporarily.

For polling model, we need to disable to the polling timer thread till we receive _OST response from firmware.

Quoted 2 lines

Ideally I think it would be better to follow the order in the flowchart if it's not too onerous.

None of the above changes will be pretty and I think it will not be simple as well.

Quoted 6 lines

That will make the code easier to
understand. The current situation with this dependency on pciehp and what it will do leaves a lot of things implicit.

What happens if CONFIG_PCIE_EDR=y but CONFIG_HOTPLUG_PCI_PCIE=n?

Quoted 8 lines

Quoted 5 lines

IIUC, when DPC triggers, pciehp is what fields the DLLSC interrupt and unbinds the drivers and removes the devices.
If that doesn't happen, and Linux clears the DPC trigger to bring the link back up, will those drivers try to operate uninitialized devices?

I don't think this will happen. In DPC reset_link before we bring up the device we wait for link to go down first using pcie_wait_for_link(pdev, false) function.

Quoted 7 lines

I understand that, but these child devices were reset when DPC disabled the link. When the link comes back up, their BARs contain zeros.

If CONFIG_HOTPLUG_PCI_PCIE=y, the DLLSC interrupt will cause pciehp to unbind the driver. It seems like the unbind races with the EDR notify handler.

Agree. But even if there is a race condition, after clearing DPC trigger status, if hotplug driver properly removes/re-enumerates the driver then the end result will still be same. There should be no functional impact.

Quoted 8 lines

If pciehp unbinds the driver before edr_handle_event() calls pcie_do_recovery(), this seems fine -- we'll call dpc_reset_link(), which brings up the link, we won't call any driver callbacks because there's no driver, and another DLLSC interrupt will cause pciehp to re-enumerate, which will re-initialize the device, then rebind the driver.

If the EDR notify handler runs before pciehp unbinds the driver,

In the above case, from the kernel perspective device is still accessible and IIUC, it will try to recover it in pcie_do_recovery() using one of the callbacks.

Quoted 7 lines

int (*mmio_enabled)(struct pci_dev *dev);
int (*slot_reset)(struct pci_dev *dev);
void (*resume)(struct pci_dev *dev);

One of these callbacks will do pci_restore_state() to restore the device, and IO will not attempted in these callbacks until the device is successfully recovered.

Quoted 7 lines

That might be what *should* happen, but I don't think it's what *does* happen.

I don't think we use .mmio_enabled() and .slot_reset() for EDR because Linux EDR currently depends on DPC, so we'll be using dpc_reset_link(), which normally returns PCI_ERS_RESULT_RECOVERED, so pcie_do_recovery() skips .mmio_enabled() and .slot_reset().

After our discussion about non-hotplug cases, I am thinking that reset_link() callback should not return PCI_ERS_RESULT_RECOVERED in non hotplug cases. If successfully reset-ed, it should return PCI_ERS_RESULT_NEED_RESET. This will enable pcie_do_recovery() to proceed to .slot_reset() to successfully recover the device.

Any comments ?

Quoted 12 lines

I looked at the first few .resume() implementations (FWIW, I used [2] to find them), and none of them calls pci_restore_state() before doing I/O to the device:

   ioat_pcie_error_resume()
   pci_resume() (hfi1)
   qib_pci_resume()
   cxl_pci_resume()
   genwqe_err_resume()
   ...

Quoted 2 lines

But I assume you've tested EDR with some driver that *does* call pci_restore_state()? Or maybe you have pciehp enabled,

Yes. I have tested it only with hotplug enabled. Let me try to disable hotplug and verify the cases.

Quoted 29 lines

and it always
wins the race and unbinds the driver before the EDR notification? It would be interesting to make pciehp *lose* the race and see if anything breaks.

pci-error-recovery.rst does not mention any requirement for the driver to call pci_restore_state(), and I think any state restoration like that should be the responsibility of the PCI core, not the driver.

Quoted 20 lines

Quoted 2 lines

couldn't EDR bring up the link and call driver .mmio_enabled() before the device has been initialized?

Calling mmio_enabled in this case should not be a problem right?

Please check the following content from Documentation/PCI/pci-error-recovery.rst. IIUC (following content), IO will not be attempted until the device is successfully re-configured.

STEP 2: MMIO Enabled
--------------------
This callback is made if all drivers on a segment agree that they can try to recover and if no automatic link reset was performed by the HW. If the platform can't just re-enable IOs without a slot reset or a link reset, it will not call this callback, and instead will have gone directly to STEP 3 (Link Reset) or STEP 4 (Slot Reset)

Quoted 3 lines

If CONFIG_HOTPLUG_PCI_PCIE=n and CONFIG_HOTPLUG_PCI_ACPI=y, I could believe that the situations are similar to the above.

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

What if CONFIG_HOTPLUG_PCI_PCIE=n and CONFIG_HOTPLUG_PCI_ACPI=n? Then I assume there's nothing to unbind the driver, so pcie_do_recovery() will call the driver .mmio_enabled() and other recovery callbacks on a device that hasn't been initialized?

probably in .slot_reset() callback device config will be restored and it will make the device functional again.

I don't think .mmio_enabled() is a problem because IIUC, the device should not have been reset before calling .mmio_enabled().

In hotplug case, it is possible. since reset_link() is called before .mmio_enabled, the device might be in reset state by the time .mmio_enabled is called.

Quoted 1 line

Quoted 6 lines

But I think .slot_reset() *is* a problem. I looked at several .slot_reset() implementations ([3]); some called pci_restore_state(), but many did not.

If no hotplug driver is enabled, I think the .slot_reset() callbacks that do not call pci_restore_state() are broken.

Yes. Agree. May be the documentation needs to be explicit about it ?

I made a few of the updates Christoph suggested and updated the review/edr branch. Do you want to start with that as the basis for a v18 posting?

Quoted 70 lines · Bjorn Helgaas

Quoted 22 lines · On Wed, Mar 11, 2020 at 05:27:35PM +0000, [email protected] wrote:

On 3/11/2020 12:12 PM, Bjorn Helgaas wrote:
<SNIP>

Quoted 20 lines

Quoted 11 lines

Quoted 6 lines

Quoted 4 lines

For the normative text describing when OS clears the AER bits following the informative flow chart, it could say that OS clears AER as soon as possible after OST returns and before OS processes _HPX and loading drivers. Open to other suggestions as well.

I'm not sure what to do with "as soon as possible" either. That doesn't seem like something firmware and the OS can agree on.

I can just state that it's done after OST returns but before _HPX or driver is loaded. Any time in that range is fine. I can't get super specific here because different OSes do different things. Even for a given OS they change over time. And I need something generic enough to support a wide variety of OS implementations.

Yeah. I don't know how to solve this.

Linux doesn't actually unload and reload drivers for the child devices (Sathy, correct me if I'm wrong here) even though DPC containment takes the link down and effectively unplugs and replugs the device. I would *like* to handle it like hotplug, but some higher-level software doesn't deal well with things like storage devices disappearing and reappearing.

Quoted 8 lines

Quoted 3 lines

Since Linux doesn't actually re-enumerate the child devices, it wouldn't evaluate _HPX again. It would probably be cleaner if it did, but it's all tied up with the whole unplug/replug problem.

DPC resets everything below it and so to get it back up and running it would mean that all buses and resources need to be assigned, _HPX evaluated, and drivers reloaded. If those things don't happen then the whole hierarchy below the port that triggered DPC will be inaccessible.

Hmm, I think I might be confusing this with another situation. Sathy, can you help me understand this? I don't have a way to actually exercise this EDR path. Is there some way the pciehp hotplug driver gets involved here?

Here's how this seems to work as far as I can tell:

- Linux does not have DPC or AER control

- Linux installs EDR notify handler

- Linux evaluates DPC Enable _DSM

- DPC containment event occurs

- Firmware fields DPC interrupt

- DPC event is not a surprise remove

- Firmware sends EDR notification

- Linux EDR notify handler evaluates Locate _DSM

- Linux reads and logs DPC and AER error information for port in containment mode. [If it was an RP PIO error, Linux clears RP PIO error status, which is an asymmetry with the non-RP PIO path.]

- Linux clears AER error status (pci_aer_raw_clear_status())

- Linux calls driver .error_detected() methods for all child devices of the port in containment mode (pcie_do_recovery()). These devices are inaccessible because the link is down.

- Linux clears DPC Trigger Status (dpc_reset_link() from

     pcie_do_recovery()).

- Linux calls driver .mmio_enabled() methods for all child devices.

Quoted 4 lines

This is where I get lost. These child devices are now accessible, but they've been reset, so I don't know how their config space got restored. Did pciehp enumerate them? Did we do something like pci_restore_state()? I don't see where either of these happens.

AFAIK, AER error status registers  are sticky (RW1CS) and hence will be preserved during reset.

Quoted 2 lines

So they want to basically do native AER handling even though firmware owns AER? My head hurts.

No, it's meant only for clearing AER registers. In EDR path, since OS owns clearing DPC registers, they want to let OS own clearing AER registers as well. Also, it would give OS a chance to decide whether we want to keep the device on based on error status and history of the device attached.

Quoted 73 lines · Kuppuswamy Sathyanarayanan

Quoted 72 lines · Bjorn Helgaas

Quoted 22 lines · On Wed, Mar 11, 2020 at 05:27:35PM +0000, [email protected] wrote:

On 3/11/2020 12:12 PM, Bjorn Helgaas wrote:
<SNIP>

Quoted 20 lines

Quoted 11 lines

Quoted 6 lines

Quoted 4 lines

For the normative text describing when OS clears the AER bits following the informative flow chart, it could say that OS clears AER as soon as possible after OST returns and before OS processes _HPX and loading drivers. Open to other suggestions as well.

I'm not sure what to do with "as soon as possible" either. That doesn't seem like something firmware and the OS can agree on.

I can just state that it's done after OST returns but before _HPX or driver is loaded. Any time in that range is fine. I can't get super specific here because different OSes do different things. Even for a given OS they change over time. And I need something generic enough to support a wide variety of OS implementations.

Yeah. I don't know how to solve this.

Linux doesn't actually unload and reload drivers for the child devices (Sathy, correct me if I'm wrong here) even though DPC containment takes the link down and effectively unplugs and replugs the device. I would *like* to handle it like hotplug, but some higher-level software doesn't deal well with things like storage devices disappearing and reappearing.

Quoted 7 lines

Quoted 3 lines

Since Linux doesn't actually re-enumerate the child devices, it wouldn't evaluate _HPX again. It would probably be cleaner if it did, but it's all tied up with the whole unplug/replug problem.

DPC resets everything below it and so to get it back up and running it would mean that all buses and resources need to be assigned, _HPX evaluated, and drivers reloaded. If those things don't happen then the whole hierarchy below the port that triggered DPC will be inaccessible.

Hmm, I think I might be confusing this with another situation. Sathy, can you help me understand this? I don't have a way to actually exercise this EDR path. Is there some way the pciehp hotplug driver gets involved here?

Here's how this seems to work as far as I can tell:

- Linux does not have DPC or AER control

- Linux installs EDR notify handler

- Linux evaluates DPC Enable _DSM

- DPC containment event occurs

- Firmware fields DPC interrupt

- DPC event is not a surprise remove

- Firmware sends EDR notification

- Linux EDR notify handler evaluates Locate _DSM

- Linux reads and logs DPC and AER error information for port in containment mode. [If it was an RP PIO error, Linux clears RP PIO error status, which is an asymmetry with the non-RP PIO path.]

- Linux clears AER error status (pci_aer_raw_clear_status())

- Linux calls driver .error_detected() methods for all child devices of the port in containment mode (pcie_do_recovery()). These devices are inaccessible because the link is down.

- Linux clears DPC Trigger Status (dpc_reset_link() from pcie_do_recovery()).

- Linux calls driver .mmio_enabled() methods for all child devices.

This is where I get lost. These child devices are now accessible, but they've been reset, so I don't know how their config space got restored. Did pciehp enumerate them? Did we do something like pci_restore_state()? I don't see where either of these happens.

Quoted 2 lines

AFAIK, AER error status registers  are sticky (RW1CS) and hence will be preserved during reset.

I'm not concerned about the AER registers. I'm wondering about bus numbers & windows (for bridges), BAR settings, MSI programming, etc.:
all the normal stuff the driver expects. Or do we actually detach the driver, remove the device, hot-add the device, re-enumerate, and rebind the driver?

Quoted 8 lines

Quoted 3 lines

So they want to basically do native AER handling even though firmware owns AER? My head hurts.

No, it's meant only for clearing AER registers. In EDR path, since OS owns clearing DPC registers, they want to let OS own clearing AER registers as well. Also, it would give OS a chance to decide whether we want to keep the device on based on error status and history of the device attached.

It's obviously not meant "only for clearing AER registers" if the OS is going to decide things based on the error status. How is deciding things and clearing AER registers different from native AER handling?

This sort of makes a mockery of the idea of "AER ownership". But I guess the spec doesn't actually say anything that limits OS access to the AER capability, even if firmware retains "control". It does restrict *firmware* from modifying the AER cap if it grants control to the OS, but not the other way around.

Bjorn

Quoted 6 lines

I can just state that it's done after OST returns but before _HPX or driver is loaded. Any time in that range is fine. I can't get super specific here because different OSes do different things. Even for a given OS they change over time. And I need something generic enough to support a wide variety of OS implementations.

Yeah. I don't know how to solve this.

Linux doesn't actually unload and reload drivers for the child devices (Sathy, correct me if I'm wrong here) even though DPC containment takes the link down and effectively unplugs and replugs the device. I would *like* to handle it like hotplug, but some higher-level software doesn't deal well with things like storage devices disappearing and reappearing.

Since Linux doesn't actually re-enumerate the child devices, it wouldn't evaluate _HPX again. It would probably be cleaner if it did, but it's all tied up with the whole unplug/replug problem.

Quoted 12 lines

Quoted 5 lines

For child devices of that port, obviously it's impossible to access AER registers until DPC Trigger Status is cleared, and the flowchart says the OS shouldn't access them until after _OST.

Quoted 7 lines

I'm actually not sure we currently do *anything* with child device AER info in the EDR path. pcie_do_recovery() does walk the sub-hierarchy of child devices, but it only calls error handling callbacks in the child drivers; it doesn't do anything with the child AER registers itself. And of course, this happens before _OST, so it would be too early in any case. But maybe I'm missing something here.

Quoted 5 lines

My understanding is that the OS read/clears AER in the case where OS has native control of AER. Feedback from OSVs is they wanted to continue to do that to keep the native OS controlled AER and FF mechanism similar. The other way we could have done it would be to have the firmware read/clear AER and report them to OS via APEI.

When Linux has native control of AER, it reads/clears AER status. The flowchart is for the case where firmware has AER control, so I guess Linux would not field AER interrupts and wouldn't expect to read/clear AER status. So I *guess* Linux would assume APEI? But that doesn't seem to be what the flowchart assumes.

Quoted 7 lines

Quoted 7 lines

BTW, if/when this is updated, I have another question: the _OSC DPC control bit currently allows the OS to write DPC Control during that window. I understand the OS writing the RW1C *Status* bits to clear them, but it seems like writing the DPC Control register is likely to cause issues. The same question would apply to the AER access we're talking about.

Quoted 5 lines

We could specify which particular bits can and can't be touched. But it's hard to maintain as new bits are added. Probably better to add some guidance that OS should read/clear error status, DPC Trigger Status, etc. but shouldn't change masks/severity/control bits/etc.

Yeah. I didn't mean at the level of individual bits; I was thinking more of status/log/etc vs control registers. But maybe even that is hard, I dunno.

Quoted 4 lines · Bjorn Helgaas

If that's possible, it would be nice to update the metadata for the "Downstream Port Containment related Enhancements" ECN as well. That one currently says "ECR - CardBus Header Proposal", which means that's what's in the window title bar and icons in the panel.

Sure, I'll check.

From: Kuppuswamy Sathyanarayanan <[email protected]>

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

+void pci_acpi_add_edr_notifier(struct pci_dev *pdev)
+{
+	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+	acpi_status astatus;
+
+	if (!adev) {
+		pci_dbg(pdev, "No valid ACPI node, so skip EDR init\n");
+		return;
+	}
+
+	/*
+	 * Per the Downstream Port Containment Related Enhancements ECN to
+	 * the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-6, EDR support
+	 * can only be enabled if DPC is controlled by firmware.
+	 *
+	 * TODO: Remove dependency on ACPI FIRMWARE_FIRST bit to
+	 * determine ownership of DPC between firmware or OS.
+	 * Per the Downstream Port Containment Related Enhancements
+	 * ECN to the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-5,

[+cc Olof for pcie_ports=dpc-native question]

Quoted 21 lines · On Tue, Mar 03, 2020 at 06:36:34PM -0800…

From: Kuppuswamy Sathyanarayanan <[email protected]>

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

+void pci_acpi_add_edr_notifier(struct pci_dev *pdev)
+{
+	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+	acpi_status astatus;
+
+	if (!adev) {
+		pci_dbg(pdev, "No valid ACPI node, so skip EDR init\n");
+		return;
+	}
+
+	/*
+	 * Per the Downstream Port Containment Related Enhancements ECN to
+	 * the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-6, EDR support
+	 * can only be enabled if DPC is controlled by firmware.
+	 *
+	 * TODO: Remove dependency on ACPI FIRMWARE_FIRST bit to
+	 * determine ownership of DPC between firmware or OS.
+	 * Per the Downstream Port Containment Related Enhancements
+	 * ECN to the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-5,

drivers/pci/pcie/edr.c line 223

Quoted 10 lines

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

+	 * OS can use bit 7 of _OSC control field to negotiate control
+	 * over DPC Capability.
+	 */
+	if (!pcie_aer_get_firmware_first(pdev) || pcie_ports_dpc_native) {
+		pci_dbg(pdev, "OS handles AER/DPC, so skip EDR init\n");
+		return;
+	}
+
+	astatus = acpi_install_notify_handler(adev->handle, ACPI_SYSTEM_NOTIFY,
+					      edr_handle_event, pdev);

I think this is still problematic. You mentioned Alex's work [1,2]. We do need to revisit those patches, but I don't really want to defer *this* question of the EDR notify handler. Negotiating support of AER/DPC/EDR is already complicated, and I don't want to complicate it even more by merging something we already know is not quite right.

I don't understand your comment that "EDR can only be enabled if DPC is controlled by firmware." I don't see anything in table 4-6 to that effect. The only mention of EDR there is to say that the OS can access the DPC capability in the EDR processing window, i.e., after the OS receives the EDR notification and before it clears DPC Trigger Status.

EDR is a general ACPI feature that is not PCI-specific. For EDR on PCI devices, OS support is advertised via _OSC *Support* (table 4-4), which says:

Error Disconnect Recover Supported

The OS sets this bit to 1 if it supports Error Disconnect Recover notification on PCI Express Host Bridges, Root Ports and Switch Downstream Ports. Otherwise, the OS sets this bit to 0.

I think that means that if we set the "Error Disconnect Recover Supported" _OSC bit (OSC_PCI_EDR_SUPPORT), we must install a handler for EDR notifications. We set OSC_PCI_EDR_SUPPORT whenever CONFIG_PCIE_EDR=y, so I think we should install the notify handler here unconditionally (since this file is compiled only when CONFIG_PCIE_EDR=y).

I don't think we should even test pcie_ports_dpc_native here. If we told the platform we can handle EDR notifications, we should be prepared to get them, regardless of whether the user booted with "pcie_ports=dpc-native".

It's conceivable that pcie_ports_dpc_native should make us do something different in the notify handler after we *get* a notification, but I doubt we should even worry about that.

IIUC, pcie_ports_dpc_native exists because Linux DPC originally worked even if the OS didn't have control of AER. eed85ff4c0da7 ("PCI/DPC:
Enable DPC only if AER is available") meant that if Linux didn't have control of AER, DPC no longer worked. "pcie_ports=dpc-native" is basically a way to get that previous behavior of Linux DPC regardless of AER control.

I don't think that issue applies to EDR. There's no concept of an OS "enabling" or "being granted control of" EDR. The OS merely advertises that "yes, I'm prepared to handle EDR notifications". AFAICT, the ECR says nothing about EDR support being conditional on OS control of AER or DPC. The notify *handler* might need to do different things depending on whether we have AER or DPC control, but the handler itself should be registered regardless.

[1] https://lore.kernel.org/linux-pci/[email protected]/
[2] https://lore.kernel.org/linux-pci/[email protected]/

Quoted 40 lines · Bjorn Helgaas

Quoted 21 lines · On Tue, Mar 03, 2020 at 06:36:34PM -0800…

From: Kuppuswamy Sathyanarayanan <[email protected]>

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

+void pci_acpi_add_edr_notifier(struct pci_dev *pdev)
+{
+	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+	acpi_status astatus;
+
+	if (!adev) {
+		pci_dbg(pdev, "No valid ACPI node, so skip EDR init\n");
+		return;
+	}
+
+	/*
+	 * Per the Downstream Port Containment Related Enhancements ECN to
+	 * the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-6, EDR support
+	 * can only be enabled if DPC is controlled by firmware.
+	 *
+	 * TODO: Remove dependency on ACPI FIRMWARE_FIRST bit to
+	 * determine ownership of DPC between firmware or OS.
+	 * Per the Downstream Port Containment Related Enhancements
+	 * ECN to the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-5,

drivers/pci/pcie/edr.c line 223

Quoted 10 lines

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

+	 * OS can use bit 7 of _OSC control field to negotiate control
+	 * over DPC Capability.
+	 */
+	if (!pcie_aer_get_firmware_first(pdev) || pcie_ports_dpc_native) {
+		pci_dbg(pdev, "OS handles AER/DPC, so skip EDR init\n");
+		return;
+	}
+
+	astatus = acpi_install_notify_handler(adev->handle, ACPI_SYSTEM_NOTIFY,
+					      edr_handle_event, pdev);

I think this is still problematic. You mentioned Alex's work [1,2]. We do need to revisit those patches, but I don't really want to defer *this* question of the EDR notify handler. Negotiating support of AER/DPC/EDR is already complicated, and I don't want to complicate it even more by merging something we already know is not quite right.

Quoted 6 lines

I don't understand your comment that "EDR can only be enabled if DPC is controlled by firmware." I don't see anything in table 4-6 to that effect. The only mention of EDR there is to say that the OS can access the DPC capability in the EDR processing window, i.e., after the OS receives the EDR notification and before it clears DPC Trigger Status.

Please check the following spec reference (from table 4-6).

If control of this feature was requested and denied, firmware is responsible for initializing Downstream Port Containment Extended Capability Structures per firmware policy. Further, the OS is permitted to read or write DPC Control and Status registers of a port while processing an Error Disconnect Recover notification from firmware on that port.

It specifies firmware is expected to use EDR notification *only* when the control of DPC is requested and denied (which means firmware owns the DPC).

Although it does not explicitly state that we should install EDR notification handler only if firmware owns DPC, it mentions that EDR notification is only used if firmware owns DPC. So why should we install it if it's not going to be used when OS owns DPC.

Also check the following reference from section 2 of EDR ECN. It also clarifies EDR feature is only used when firmware owns DPC.

PCIe Base Specification suggests that Downstream Port Containment may be controlled either by the Firmware or the Operating System. It also suggests that the Firmware retain ownership of Downstream Port Containment if it also owns AER. When the Firmware owns Downstream Port Containment, *it is expected to use the new “Error Disconnect Recover” notification to alert OSPM of a Downstream Port Containment event*.

Quoted 11 lines

EDR is a general ACPI feature that is not PCI-specific. For EDR on PCI devices, OS support is advertised via _OSC *Support* (table 4-4), which says:

Error Disconnect Recover Supported

The OS sets this bit to 1 if it supports Error Disconnect Recover notification on PCI Express Host Bridges, Root Ports and Switch Downstream Ports. Otherwise, the OS sets this bit to 0.

Quoted 6 lines

I think that means that if we set the "Error Disconnect Recover Supported" _OSC bit (OSC_PCI_EDR_SUPPORT), we must install a handler for EDR notifications. We set OSC_PCI_EDR_SUPPORT whenever CONFIG_PCIE_EDR=y, so I think we should install the notify handler here unconditionally (since this file is compiled only when CONFIG_PCIE_EDR=y).

Although spec does not provide any restrictions on when to install EDR notification, it provides reference that notification is only used if firmware owns DPC. So when OS owns DPC, there is no need to install them at all.

Although installing them when OS owns DPC should not affect anything, it also opens up a additional way for firmware to mess up things. For example, consider a case when firmware gives OS control of DPC, but still sends EDR notification to OS. Although it's unrealistic, I am just giving an example.

Quoted 4 lines

I don't think we should even test pcie_ports_dpc_native here. If we told the platform we can handle EDR notifications, we should be prepared to get them, regardless of whether the user booted with "pcie_ports=dpc-native".

As per the command line parameter documentation, setting pcie_ports=dpc-native means, we will be using native PCIe service for DPC. So if DPC is handled by OS, as per my argument mentioned above (EDR is only useful if DPC handled by firmware), there is no use in installing EDR notification.

https://github.com/torvalds/linux/blob/master/Documentation/admin-guide/kernel-parameters.txt#L3642

dpc-native - Use native PCIe service for DPC only.

Quoted 22 lines

It's conceivable that pcie_ports_dpc_native should make us do something different in the notify handler after we *get* a notification, but I doubt we should even worry about that.

IIUC, pcie_ports_dpc_native exists because Linux DPC originally worked even if the OS didn't have control of AER. eed85ff4c0da7 ("PCI/DPC:
Enable DPC only if AER is available") meant that if Linux didn't have control of AER, DPC no longer worked. "pcie_ports=dpc-native" is basically a way to get that previous behavior of Linux DPC regardless of AER control.

I don't think that issue applies to EDR. There's no concept of an OS "enabling" or "being granted control of" EDR. The OS merely advertises that "yes, I'm prepared to handle EDR notifications". AFAICT, the ECR says nothing about EDR support being conditional on OS control of AER or DPC. The notify *handler* might need to do different things depending on whether we have AER or DPC control, but the handler itself should be registered regardless.

[1] https://lore.kernel.org/linux-pci/[email protected]/
[2] https://lore.kernel.org/linux-pci/[email protected]/

Signature · 2 lines

Sathyanarayanan Kuppuswamy
Linux Kernel Developer