[PATCH] PCI/ERR: Resolve regression in pcie_do_recovery

[PATCH] PCI/ERR: Resolve regression in pcie_do_recovery

[Jay Vosburgh]

	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.

	Inspection shows a similar path is plausible for a return of
_CAN_RECOVER and the invocation of report_mmio_enabled.

	Resolve this by preserving the result of report_frozen_detected if
reset_link does not return _DISCONNECT.

Fixes: 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")
Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>

---
 drivers/pci/pcie/err.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index 14bb8f54723e..e4274562f3a0 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -164,10 +164,17 @@ 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_ers_result_t status2;
+
 		pci_walk_bus(bus, report_frozen_detected, &status);
-		status = reset_link(dev);
-		if (status != PCI_ERS_RESULT_RECOVERED) {
+		/* preserve status from report_frozen_detected to
+		 * insure report_mmio_enabled or report_slot_reset are
+		 * invoked even if reset_link returns _RECOVERED.
+		 */
+		status2 = reset_link(dev);
+		if (status2 != PCI_ERS_RESULT_RECOVERED) {
 			pci_warn(dev, "link reset failed\n");
+			status = status2;
 			goto failed;
 		}
 	} else {
-- 
2.7.4

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

[Kuppuswamy, Sathyanarayanan]

On 4/29/20 5:42 PM, Jay Vosburgh wrote:
> 	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.

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.

> 
> 	Inspection shows a similar path is plausible for a return of
> _CAN_RECOVER and the invocation of report_mmio_enabled.
> 
> 	Resolve this by preserving the result of report_frozen_detected if
> reset_link does not return _DISCONNECT.
> 
> Fixes: 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")
> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
> 
> ---
>   drivers/pci/pcie/err.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> index 14bb8f54723e..e4274562f3a0 100644
> --- a/drivers/pci/pcie/err.c
> +++ b/drivers/pci/pcie/err.c
> @@ -164,10 +164,17 @@ 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_ers_result_t status2;
> +
>   		pci_walk_bus(bus, report_frozen_detected, &status);
> -		status = reset_link(dev);
> -		if (status != PCI_ERS_RESULT_RECOVERED) {
> +		/* preserve status from report_frozen_detected to
> +		 * insure report_mmio_enabled or report_slot_reset are
> +		 * invoked even if reset_link returns _RECOVERED.
> +		 */
> +		status2 = reset_link(dev);
> +		if (status2 != PCI_ERS_RESULT_RECOVERED) {
>   			pci_warn(dev, "link reset failed\n");
> +			status = status2;
>   			goto failed;
>   		}
>   	} else {
> 

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

[Kuppuswamy, Sathyanarayanan]

Hi Jay,

On 4/29/20 6:15 PM, Kuppuswamy, Sathyanarayanan wrote:
> 
> 
> On 4/29/20 5:42 PM, Jay Vosburgh wrote:
>>     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.
> 
> 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.

Can you check whether following fix works for you?

This includes support for bus_reset in recovery function itself.

index 14bb8f54723e..c9eaab68ab7a 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -165,13 +165,23 @@ 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) {
         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) {


> 
>>
>>     Inspection shows a similar path is plausible for a return of
>> _CAN_RECOVER and the invocation of report_mmio_enabled.
>>
>>     Resolve this by preserving the result of report_frozen_detected if
>> reset_link does not return _DISCONNECT.
>>
>> Fixes: 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")
>> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
>>
>> ---
>>   drivers/pci/pcie/err.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>> index 14bb8f54723e..e4274562f3a0 100644
>> --- a/drivers/pci/pcie/err.c
>> +++ b/drivers/pci/pcie/err.c
>> @@ -164,10 +164,17 @@ 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_ers_result_t status2;
>> +
>>           pci_walk_bus(bus, report_frozen_detected, &status);
>> -        status = reset_link(dev);
>> -        if (status != PCI_ERS_RESULT_RECOVERED) {
>> +        /* preserve status from report_frozen_detected to
>> +         * insure report_mmio_enabled or report_slot_reset are
>> +         * invoked even if reset_link returns _RECOVERED.
>> +         */
>> +        status2 = reset_link(dev);
>> +        if (status2 != PCI_ERS_RESULT_RECOVERED) {
>>               pci_warn(dev, "link reset failed\n");
>> +            status = status2;
>>               goto failed;
>>           }
>>       } else {
>>

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

[Jay Vosburgh]

"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:
>>>     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.
>>
>> 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.

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


	-J

>This includes support for bus_reset in recovery function itself.
>
>index 14bb8f54723e..c9eaab68ab7a 100644
>--- a/drivers/pci/pcie/err.c
>+++ b/drivers/pci/pcie/err.c
>@@ -165,13 +165,23 @@ 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) {
>        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) {
>
>
>>
>>>
>>>     Inspection shows a similar path is plausible for a return of
>>> _CAN_RECOVER and the invocation of report_mmio_enabled.
>>>
>>>     Resolve this by preserving the result of report_frozen_detected if
>>> reset_link does not return _DISCONNECT.
>>>
>>> Fixes: 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")
>>> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
>>>
>>> ---
>>>   drivers/pci/pcie/err.c | 11 +++++++++--
>>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>>> index 14bb8f54723e..e4274562f3a0 100644
>>> --- a/drivers/pci/pcie/err.c
>>> +++ b/drivers/pci/pcie/err.c
>>> @@ -164,10 +164,17 @@ 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_ers_result_t status2;
>>> +
>>>           pci_walk_bus(bus, report_frozen_detected, &status);
>>> -        status = reset_link(dev);
>>> -        if (status != PCI_ERS_RESULT_RECOVERED) {
>>> +        /* preserve status from report_frozen_detected to
>>> +         * insure report_mmio_enabled or report_slot_reset are
>>> +         * invoked even if reset_link returns _RECOVERED.
>>> +         */
>>> +        status2 = reset_link(dev);
>>> +        if (status2 != PCI_ERS_RESULT_RECOVERED) {
>>>               pci_warn(dev, "link reset failed\n");
>>> +            status = status2;
>>>               goto failed;
>>>           }
>>>       } else {
>>>

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

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

[Jay Vosburgh]

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:
>>>>     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.
>>>
>>> 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?

	-J

>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) {
>
>
>	-J
>
>>This includes support for bus_reset in recovery function itself.
>>
>>index 14bb8f54723e..c9eaab68ab7a 100644
>>--- a/drivers/pci/pcie/err.c
>>+++ b/drivers/pci/pcie/err.c
>>@@ -165,13 +165,23 @@ 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) {
>>        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) {
>>
>>
>>>
>>>>
>>>>     Inspection shows a similar path is plausible for a return of
>>>> _CAN_RECOVER and the invocation of report_mmio_enabled.
>>>>
>>>>     Resolve this by preserving the result of report_frozen_detected if
>>>> reset_link does not return _DISCONNECT.
>>>>
>>>> Fixes: 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")
>>>> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
>>>>
>>>> ---
>>>>   drivers/pci/pcie/err.c | 11 +++++++++--
>>>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>>>> index 14bb8f54723e..e4274562f3a0 100644
>>>> --- a/drivers/pci/pcie/err.c
>>>> +++ b/drivers/pci/pcie/err.c
>>>> @@ -164,10 +164,17 @@ 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_ers_result_t status2;
>>>> +
>>>>           pci_walk_bus(bus, report_frozen_detected, &status);
>>>> -        status = reset_link(dev);
>>>> -        if (status != PCI_ERS_RESULT_RECOVERED) {
>>>> +        /* preserve status from report_frozen_detected to
>>>> +         * insure report_mmio_enabled or report_slot_reset are
>>>> +         * invoked even if reset_link returns _RECOVERED.
>>>> +         */
>>>> +        status2 = reset_link(dev);
>>>> +        if (status2 != PCI_ERS_RESULT_RECOVERED) {
>>>>               pci_warn(dev, "link reset failed\n");
>>>> +            status = status2;
>>>>               goto failed;
>>>>           }
>>>>       } else {
>>>>
>
>---
>	-Jay Vosburgh, jay.vosburgh@canonical.com

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

[Yicong Yang]

Hi Jay, Kuppuswamy

On 2020/5/1 4:41, Jay Vosburgh 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:
>>>>     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.
>>> 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.
>
> 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;
> +		}
> +

The PCI_ERS_RESULT_NEED_RESET may indicate that the driver requires a *slot* reset.
With this patch, seems later slot reset broadcast may not be performed.

    if (status == PCI_ERS_RESULT_NEED_RESET) {
        status = PCI_ERS_RESULT_RECOVERED;
        pci_dbg(dev, "broadcast slot_reset message\n");
        pci_walk_bus(bus, report_slot_reset, &status);
    }

One minor question, currently the callers of pcie_do_recovery() will always pass a
reset_link pointer, so is the condition necessary?

Yicong

> +		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) {
>
>
> 	-J
>
>> This includes support for bus_reset in recovery function itself.
>>
>> index 14bb8f54723e..c9eaab68ab7a 100644
>> --- a/drivers/pci/pcie/err.c
>> +++ b/drivers/pci/pcie/err.c
>> @@ -165,13 +165,23 @@ 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) {
>>        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) {
>>
>>
>>>>     Inspection shows a similar path is plausible for a return of
>>>> _CAN_RECOVER and the invocation of report_mmio_enabled.
>>>>
>>>>     Resolve this by preserving the result of report_frozen_detected if
>>>> reset_link does not return _DISCONNECT.
>>>>
>>>> Fixes: 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")
>>>> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
>>>>
>>>> ---
>>>>   drivers/pci/pcie/err.c | 11 +++++++++--
>>>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>>>> index 14bb8f54723e..e4274562f3a0 100644
>>>> --- a/drivers/pci/pcie/err.c
>>>> +++ b/drivers/pci/pcie/err.c
>>>> @@ -164,10 +164,17 @@ 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_ers_result_t status2;
>>>> +
>>>>           pci_walk_bus(bus, report_frozen_detected, &status);
>>>> -        status = reset_link(dev);
>>>> -        if (status != PCI_ERS_RESULT_RECOVERED) {
>>>> +        /* preserve status from report_frozen_detected to
>>>> +         * insure report_mmio_enabled or report_slot_reset are
>>>> +         * invoked even if reset_link returns _RECOVERED.
>>>> +         */
>>>> +        status2 = reset_link(dev);
>>>> +        if (status2 != PCI_ERS_RESULT_RECOVERED) {
>>>>               pci_warn(dev, "link reset failed\n");
>>>> +            status = status2;
>>>>               goto failed;
>>>>           }
>>>>       } else {
>>>>
> ---
> 	-Jay Vosburgh, jay.vosburgh@canonical.com
> .
>

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

[Yicong Yang]

[ +cc dongdong as we met the issue ]

Hi,

The regression happened with our intel 82599 network adaptor. A DPC event
happened on the device, recovery returns successful but the device is
not enabled. As we only reset link, but don't call driver specific
handler(mmio enable/slot reset).

If we met an error with IO blocked, the logic should be:
1. try to reset link
2. broadcast report_frozen_detected on the bus
3. do what drivers suggest(mmio enable/slot reset)

Currently only step 1 is performed, and returns directly. I tried the
patch below and it solves the issues. The differences from Jay's one
is that it merge the return value to the status and it match the logic
I mentioned above.

Regards,
Yicong

diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index 14bb8f5..6f8870c 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -164,12 +164,12 @@ 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) {
                        pci_warn(dev, "link reset failed\n");
                        goto failed;
                }
+               pci_walk_bus(bus, report_frozen_detected, &status);
        } else {
                pci_walk_bus(bus, report_normal_detected, &status);
        }

On 2020/4/30 8:42, Jay Vosburgh wrote:
> 	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.
>
> 	Inspection shows a similar path is plausible for a return of
> _CAN_RECOVER and the invocation of report_mmio_enabled.
>
> 	Resolve this by preserving the result of report_frozen_detected if
> reset_link does not return _DISCONNECT.
>
> Fixes: 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")
> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
>
> ---
>  drivers/pci/pcie/err.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> index 14bb8f54723e..e4274562f3a0 100644
> --- a/drivers/pci/pcie/err.c
> +++ b/drivers/pci/pcie/err.c
> @@ -164,10 +164,17 @@ 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_ers_result_t status2;
> +
>  		pci_walk_bus(bus, report_frozen_detected, &status);
> -		status = reset_link(dev);
> -		if (status != PCI_ERS_RESULT_RECOVERED) {
> +		/* preserve status from report_frozen_detected to
> +		 * insure report_mmio_enabled or report_slot_reset are
> +		 * invoked even if reset_link returns _RECOVERED.
> +		 */
> +		status2 = reset_link(dev);
> +		if (status2 != PCI_ERS_RESULT_RECOVERED) {
>  			pci_warn(dev, "link reset failed\n");
> +			status = status2;
>  			goto failed;
>  		}
>  	} else {

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

[Kuppuswamy, Sathyanarayanan]

On 5/8/20 11:35 PM, Yicong Yang wrote:
> Hi Jay, Kuppuswamy
> 
> On 2020/5/1 4:41, Jay Vosburgh 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:
>>>>>      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.
>>>> 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.
>>
>> 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;
>> +		}
>> +
> 
> The PCI_ERS_RESULT_NEED_RESET may indicate that the driver requires a *slot* reset.
> With this patch, seems later slot reset broadcast may not be performed.
Slot reset wont be performed only if reset_link or pci_bus_error_reset
returns error. Otherwise, we will still call pci_slot_reset later.
> 
>      if (status == PCI_ERS_RESULT_NEED_RESET) {
>          status = PCI_ERS_RESULT_RECOVERED;
>          pci_dbg(dev, "broadcast slot_reset message\n");
>          pci_walk_bus(bus, report_slot_reset, &status);
>      }
> 
> One minor question, currently the callers of pcie_do_recovery() will always pass a
> reset_link pointer, so is the condition necessary?
Yes, currently we don't need it. I added it to cover future use cases.
But we can remove it if not needed.
> 
> Yicong
> 
>> +		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) {
>>
>>pci_bus_error_reset
>> 	-J
>>
>>> This includes support for bus_reset in recovery function itself.
>>>
>>> index 14bb8f54723e..c9eaab68ab7a 100644
>>> --- a/drivers/pci/pcie/err.c
>>> +++ b/drivers/pci/pcie/err.c
>>> @@ -165,13 +165,23 @@ 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) {
>>>         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) {
>>>
>>>
>>>>>      Inspection shows a similar path is plausible for a return of
>>>>> _CAN_RECOVER and the invocation of report_mmio_enabled.
>>>>>
>>>>>      Resolve this by preserving the result of report_frozen_detected if
>>>>> reset_link does not return _DISCONNECT.
>>>>>
>>>>> Fixes: 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")
>>>>> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
>>>>>
>>>>> ---
>>>>>    drivers/pci/pcie/err.c | 11 +++++++++--
>>>>>    1 file changed, 9 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>>>>> index 14bb8f54723e..e4274562f3a0 100644
>>>>> --- a/drivers/pci/pcie/err.c
>>>>> +++ b/drivers/pci/pcie/err.c
>>>>> @@ -164,10 +164,17 @@ 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_ers_result_t status2;
>>>>> +
>>>>>            pci_walk_bus(bus, report_frozen_detected, &status);
>>>>> -        status = reset_link(dev);
>>>>> -        if (status != PCI_ERS_RESULT_RECOVERED) {
>>>>> +        /* preserve status from report_frozen_detected to
>>>>> +         * insure report_mmio_enabled or report_slot_reset are
>>>>> +         * invoked even if reset_link returns _RECOVERED.
>>>>> +         */
>>>>> +        status2 = reset_link(dev);
>>>>> +        if (status2 != PCI_ERS_RESULT_RECOVERED) {
>>>>>                pci_warn(dev, "link reset failed\n");
>>>>> +            status = status2;
>>>>>                goto failed;
>>>>>            }
>>>>>        } else {
>>>>>
>> ---
>> 	-Jay Vosburgh, jay.vosburgh@canonical.com
>> .
>>
> 

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

[Jay Vosburgh]

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