PCI: Mask and unmask hotplug interrupts during reset

We need to figure out how to gracefully return inside hotplug driver if link down happened and there is an error pending. Fatal error needs to be serviced by AER/DPC drivers. Hotplug driver is observing link events while AER/DPC drivers are performing link recovery today and are causing confusion for the hotplug statemachine.

1. check if there is a fatal error pending in the device_status register of the PCI Express capability on the root port.
2. bail out from hotplug routine if this is the case.
3. otherwise, existing behavior.

If fatal error is pending and a fatal error service such as DPC or AER is running, it is the responsibility of the fatal error service to recover the link.

drivers/pci/hotplug/pciehp_hpc.c +0 -3

@@ -612,10 +612,15 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
 	 * and cause the wrong event to queue.
 	 */
 	if (events & PCI_EXP_SLTSTA_DLLSC) {
-		ctrl_info(ctrl, "Slot(%s): Link %s\n", slot_name(slot),
-			  link ? "Up" : "Down");
-		pciehp_queue_interrupt_event(slot, link ? INT_LINK_UP :

Quoted 14 lines · Sinan Kaya

We need to figure out how to gracefully return inside hotplug driver if link down happened and there is an error pending. Fatal error needs to be serviced by AER/DPC drivers. Hotplug driver is observing link events while AER/DPC drivers are performing link recovery today and are causing confusion for the hotplug statemachine.

1. check if there is a fatal error pending in the device_status register of the PCI Express capability on the root port.
2. bail out from hotplug routine if this is the case.
3. otherwise, existing behavior.

If fatal error is pending and a fatal error service such as DPC or AER is running, it is the responsibility of the fatal error service to recover the link.

[snip]

Quoted 11 lines

drivers/pci/hotplug/pciehp_hpc.c +0 -4

@@ -612,10 +612,15 @@ static irqreturn_t pciehp_isr(int irq, void
 *dev_id)
 	 * and cause the wrong event to queue.
 	 */
 	if (events & PCI_EXP_SLTSTA_DLLSC) {
-		ctrl_info(ctrl, "Slot(%s): Link %s\n", slot_name(slot),
-			  link ? "Up" : "Down");
-		pciehp_queue_interrupt_event(slot, link ? INT_LINK_UP :
-					     INT_LINK_DOWN);

drivers/pci/hotplug/pciehp_hpc.c static irqreturn_t pciehp_isr(int irq, void · line 625

Quoted 10 lines

drivers/pci/hotplug/pciehp_hpc.c +9 -0

+		if (pci_fatal_error_pending(pdev, PCI_ERR_UNC_SURPDN))
+			ctrl_info(ctrl, "Slot(%s): Ignoring Link %s event due to detected
 Fatal Error\n",
+				  slot_name(slot), link ? "Up" : "Down");
+		else {
+			ctrl_info(ctrl, "Slot(%s): Link %s\n", slot_name(slot),
+				  link ? "Up" : "Down");
+			pciehp_queue_interrupt_event(slot, link ? INT_LINK_UP :
+						     INT_LINK_DOWN);
+		}

The above is still based on the event handling in v4.18, so the patch doesn't apply to what's currently queued on Bjorn's pci/hotplug branch.

That said, a problem I see with the approach you're suggesting is that recovery from the fatal error may fail in pcie_do_fatal_recovery(). The devices below the hotplug port will then have been removed but internally in pciehp the slot will remain in ON_STATE and slot power will remain enabled. That feels wrong.

After re-reading all the e-mails we've exchanged since early July, the approach Oza suggested in this e-mail seems the most sensible to me:
https://lkml.kernel.org/r/[email protected]

He suggested skipping removal of devices in pcie_do_fatal_recovery() for hotplug ports.

The rationale is that when a PCIe port raises a fatal error, that port will normally not have the ability to remove its children from the hierarchy unless it's a hotplug port. Thus, if the port is a hotplug port, pcie_do_fatal_recovery() should let pciehp handle removal of the devices. Only if it's not a hotplug port should pcie_do_fatal_recovery() remove the devices. My understanding is that after the error has been cleared, the link should automatically come back up, is that correct? pciehp will then bring up the slot on its own.

Thanks,

Lukas

Quoted 7 lines · Lukas Wunner

After re-reading all the e-mails we've exchanged since early July, the approach Oza suggested in this e-mail seems the most sensible to me:
https://lkml.kernel.org/r/[email protected]

He suggested skipping removal of devices in pcie_do_fatal_recovery() for hotplug ports.

Quoted 9 lines

The rationale is that when a PCIe port raises a fatal error, that port will normally not have the ability to remove its children from the hierarchy unless it's a hotplug port. Thus, if the port is a hotplug port, pcie_do_fatal_recovery() should let pciehp handle removal of the devices. Only if it's not a hotplug port should pcie_do_fatal_recovery() remove the devices. My understanding is that after the error has been cleared, the link should automatically come back up, is that correct? pciehp will then bring up the slot on its own.

I understand your slot power concerns. I don't have an answer at this moment.

Here is the issue with mixing pciehp link handler and fatal error recovery paths.

1. fatal error happens
2. Pciehp ISR executes and turns off slot power
3. Fatal error recovery executes either secondary bus reset or dpc status trigger via reset_link() function to recover the link.
4. Link doesn't recover because power is off.

Quoted 3 lines

ere is the issue with mixing pciehp link handler and fatal error recovery paths.

With the new code on the pci/hotplug branch, the expected behavior is:

4. Having brought the slot down after the DLLSC event, pciehp_handle_presence_or_link_change() notices that the slot is occupied ("Slot(...): Card present") and immediately tries to bring the slot up again:
Slot power is re-enabled, green LED is set to blinking. pciehp_check_link_status() then waits 1 second for the link to go up. If the link does not come up within 1 second, an error message is logged but the procedure does not abort yet. We then wait for 100 ms and poll for 1 second whether the vendor ID of slot 0 function 0 becomes readable. Only if the link has not come up at this point is the procedure aborted.

So the question is:

a. Is DPC/AER able to recover from the error if slot power is disabled?
Or do we need to synchronize with pciehp to keep slot power enabled so that the error can be handled?

b. How fast is DPC/AER able to recover from a fatal error?
If it's more than 2.1 seconds, pciehp will not bring the slot up automatically and the user is required to bring it up manually via sysfs. If we know that DPC/AER need a specific amount of time that is longer than 2.1 seconds, we can amend board_added() to poll until recovery from the fatal error (or use a waitqueue which is woken on recovery), with a sufficient timeout.

.
Does this work for you?

Yes, slot power needs to be kept on.

pciehp shouldn't attempt recovery.

If link goes down due to a DPC event, it should be recovered by DPC status trigger. Injecting a cold reset in the middle can cause a HW lockup as it is an undefined behavior.

Similarly, If link goes down due to an AER secondary bus reset issue, it should be recovered by HW. Injecting a cold reset in the middle of a secondary bus reset can cause a HW lockup as it is an undefined behavior.

Maybe, this helps:

1. HP ISR observes link down interrupt.
2. HP ISR checks that there is a fatal error pending, it doesn't touch the link.
3. HP ISR waits until link recovery happens.
4. HP ISR calls the read vendor id function.

DPC link recovery is very quick (100ms at most). Secondary bus reset recovery should be contained within 1 seconds for most cases but spec allows a device to extend vendor id read as much as it wants via CRS response. We poll up to an additional 60 seconds in read vendor id function.

Quoted 11 lines · Sinan Kaya

Yes, slot power needs to be kept on.

pciehp shouldn't attempt recovery.

If link goes down due to a DPC event, it should be recovered by DPC status trigger. Injecting a cold reset in the middle can cause a HW lockup as it is an undefined behavior.

Similarly, If link goes down due to an AER secondary bus reset issue, it should be recovered by HW. Injecting a cold reset in the middle of a secondary bus reset can cause a HW lockup as it is an undefined behavior.

Thanks a lot for the explanation, understood now.

Quoted 8 lines

Maybe, this helps:

1. HP ISR observes link down interrupt.
2. HP ISR checks that there is a fatal error pending, it doesn't touch the link.
3. HP ISR waits until link recovery happens.
4. HP ISR calls the read vendor id function.

Quoted 5 lines

DPC link recovery is very quick (100ms at most). Secondary bus reset recovery should be contained within 1 seconds for most cases but spec allows a device to extend vendor id read as much as it wants via CRS response. We poll up to an additional 60 seconds in read vendor id function.

Yes, that proposal makes a lot of sense to me. This should also work regardless whether pciehp or DPC/AER react first to the Link Down. Could you rebase your patch on the current pci/hotplug branch and insert the procedure you've outlined above at the top of pciehp_handle_presence_or_link_change() in pciehp_ctrl.c, or put it in a helper that's called at the top of that function.

Your patch "[PATCH v6 1/1] PCI: pciehp: Ignore link events when there is a fatal error pending" only checks once for a pending fatal error, it should poll until either the fatal error is gone or a timeout is hit. If the fatal error is gone and the link is up, you can just return from pciehp_handle_presence_or_link_change(). Else (in the timeout case) fall back to the normal handling of a Link Down, i.e. let it bring down the slot.

Please add a code comment in pciehp_handle_presence_or_link_change() along the lines of

	/* If a fatal error is pending, wait for AER or DPC to handle it. */

The information in your e-mail that a cold reset would incorrectly interfere with error recovery is a crucial piece of information that should be included at least in the commit message. (I was unaware of that.)

If you have any further questions on pciehp, ask away.

Thanks!

Lukas