Re: [PATCH] PCI/ERR: Resolve regression in pcie_do_recovery

[Jay Vosburgh <jay.vosburgh@canonical.com>]

Bjorn Helgaas <helgaas@kernel.org> wrote:

>On Wed, May 06, 2020 at 11:08:35AM -0700, Jay Vosburgh wrote:
>> Jay Vosburgh <jay.vosburgh@canonical.com> wrote:
>> 
>> >"Kuppuswamy, Sathyanarayanan" wrote:
>> >
>> >>Hi Jay,
>> >>
>> >>On 4/29/20 6:15 PM, Kuppuswamy, Sathyanarayanan wrote:
>> >>>
>> >>>
>> >>> On 4/29/20 5:42 PM, Jay Vosburgh wrote:
[...]
>> >>> I think this issue is related to the issue discussed in following
>> >>> thread (DPC non-hotplug support).
>> >>>
>> >>> https://lkml.org/lkml/2020/3/28/328
>> >>>
>> >>> If my assumption is correct, you are dealing with devices which are
>> >>> not hotplug capable. If the devices are hotplug capable then you don't
>> >>> need to proceed to report_slot_reset(), since hotplug handler will
>> >>> remove/re-enumerate the devices correctly.
>> >
>> >	Correct, this particular device (a network card) is in a
>> >non-hotplug slot.
>> >
>> >>Can you check whether following fix works for you?
>> >
>> >	Yes, it does.
>> >
>> >	I fixed up the whitespace and made a minor change to add braces
>> >in what look like the correct places around the "if (reset_link)" block;
>> >the patch I tested with is below.  I'll also install this on another
>> >machine with hotplug capable slots to test there as well.
>> 
>> 	We've tested the below patch on a couple of different machines
>> and devices (network card, NVMe device) and it appears to solve the
>> recovery issue in our testing.
>> 
>> 	Is there anything further we need to do, or can this be
>> considered for inclusion upstream at this time?
>
>Can somebody please post a clean version of what we should merge?
>There was the initial patch plus a follow-up fix, so it's not clear
>where we ended up.
>
>Bjorn

	Below is the patch we tested, from Sathyanarayanan's test patch
(slightly edited to clarify ambiguous "if else" nesting), along with an
edited version of the commit log from my original patch.  I have not
seen a Signed-off-by from Sathyanarayanan, so I didn't include one here.

	One question I have is that, after the patch is applied, the
"status" filled in by pci_walk_bus(... report_frozen_detected ...) is
discarded regardless of its value.  Is that correct behavior in all
cases?  The original issue I was trying to solve was that the status set
by report_frozen_detected was thrown away starting with 6d2c89441571
("PCI/ERR: Update error status after reset_link()"), causing a
_NEED_RESET to be lost.  With the below patch, all cases of
pci_channel_io_frozen will call reset_link unconditionally.

	-J

Subject: [PATCH] PCI/ERR: Resolve regression in pcie_do_recovery

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
 
	Commit 6d2c89441571 ("PCI/ERR: Update error status after
reset_link()"), introduced a regression, as pcie_do_recovery will
discard the status result from report_frozen_detected.  This can cause a
failure to recover if _NEED_RESET is returned by report_frozen_detected
and report_slot_reset is not invoked.

	Such an event can be induced for testing purposes by reducing
the Max_Payload_Size of a PCIe bridge to less than that of a device
downstream from the bridge, and then initating I/O through the device,
resulting in oversize transactions.  In the presence of DPC, this
results in a containment event and attempted reset and recovery via
pcie_do_recovery.  After 6d2c89441571 report_slot_reset is not invoked,
and the device does not recover.

Fixes: 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")


diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index 14bb8f54723e..db80e1ecb2dc 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -165,13 +165,24 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
 	pci_dbg(dev, "broadcast error_detected message\n");
 	if (state == pci_channel_io_frozen) {
 		pci_walk_bus(bus, report_frozen_detected, &status);
-		status = reset_link(dev);
-		if (status != PCI_ERS_RESULT_RECOVERED) {
+		status = PCI_ERS_RESULT_NEED_RESET;
+	} else {
+		pci_walk_bus(bus, report_normal_detected, &status);
+	}
+
+	if (status == PCI_ERS_RESULT_NEED_RESET) {
+		if (reset_link) {
+			if (reset_link(dev) != PCI_ERS_RESULT_RECOVERED)
+				status = PCI_ERS_RESULT_DISCONNECT;
+		} else {
+			if (pci_bus_error_reset(dev))
+				status = PCI_ERS_RESULT_DISCONNECT;
+		}
+
+		if (status == PCI_ERS_RESULT_DISCONNECT) {
 			pci_warn(dev, "link reset failed\n");
 			goto failed;
 		}
-	} else {
-		pci_walk_bus(bus, report_normal_detected, &status);
 	}
 
 	if (status == PCI_ERS_RESULT_CAN_RECOVER) {


---
	-Jay Vosburgh, jay.vosburgh@canonical.com