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

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

[PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices

[sathyanarayanan.kuppuswamy]

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

If there are non-hotplug capable devices connected to a given
port, then during the fatal error recovery(triggered by DPC or
AER), after calling reset_link() function, we cannot rely on
hotplug handler to detach and re-enumerate the device drivers
in the affected bus. Instead, we will have to let the error
recovery handler call report_slot_reset() for all devices in
the bus to notify about the reset operation. Although this is
only required for non hot-plug capable devices, doing it for
hotplug capable devices should not affect the functionality.

Along with above issue, this fix also applicable to following
issue.

Commit 6d2c89441571 ("PCI/ERR: Update error status after
reset_link()") added support to store status of reset_link()
call. Although this fixed the error recovery issue observed if
the initial value of error status is PCI_ERS_RESULT_DISCONNECT
or PCI_ERS_RESULT_NO_AER_DRIVER, it also discarded 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 initiating 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.

[original patch is from jay.vosburgh@canonical.com]
[original patch link https://lore.kernel.org/linux-pci/18609.1588812972@famine/]
Fixes: 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")
Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/pci/pcie/err.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

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

Re: [PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices

[Jay Vosburgh]

sathyanarayanan.kuppuswamy@linux.intel.com wrote:

>From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>
>If there are non-hotplug capable devices connected to a given
>port, then during the fatal error recovery(triggered by DPC or
>AER), after calling reset_link() function, we cannot rely on
>hotplug handler to detach and re-enumerate the device drivers
>in the affected bus. Instead, we will have to let the error
>recovery handler call report_slot_reset() for all devices in
>the bus to notify about the reset operation. Although this is
>only required for non hot-plug capable devices, doing it for
>hotplug capable devices should not affect the functionality.

	Yicong,

	Does the patch below also resolve the issue for you, as with
your changed version of my original patch?

	-J

>Along with above issue, this fix also applicable to following
>issue.
>
>Commit 6d2c89441571 ("PCI/ERR: Update error status after
>reset_link()") added support to store status of reset_link()
>call. Although this fixed the error recovery issue observed if
>the initial value of error status is PCI_ERS_RESULT_DISCONNECT
>or PCI_ERS_RESULT_NO_AER_DRIVER, it also discarded 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 initiating 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.
>
>[original patch is from jay.vosburgh@canonical.com]
>[original patch link https://lore.kernel.org/linux-pci/18609.1588812972@famine/]
>Fixes: 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")
>Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
>Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>---
> drivers/pci/pcie/err.c | 19 +++++++++++++++----
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
>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) {
>-- 
>2.17.1
>

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

Re: [PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices

[Yicong Yang]

On 2020/5/13 3:20, Jay Vosburgh wrote:
> sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>
>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>
>> If there are non-hotplug capable devices connected to a given
>> port, then during the fatal error recovery(triggered by DPC or
>> AER), after calling reset_link() function, we cannot rely on
>> hotplug handler to detach and re-enumerate the device drivers
>> in the affected bus. Instead, we will have to let the error
>> recovery handler call report_slot_reset() for all devices in
>> the bus to notify about the reset operation. Although this is
>> only required for non hot-plug capable devices, doing it for
>> hotplug capable devices should not affect the functionality.
> 	Yicong,
>
> 	Does the patch below also resolve the issue for you, as with
> your changed version of my original patch?

Yes. It works.


>
> 	-J
>
>> Along with above issue, this fix also applicable to following
>> issue.
>>
>> Commit 6d2c89441571 ("PCI/ERR: Update error status after
>> reset_link()") added support to store status of reset_link()
>> call. Although this fixed the error recovery issue observed if
>> the initial value of error status is PCI_ERS_RESULT_DISCONNECT
>> or PCI_ERS_RESULT_NO_AER_DRIVER, it also discarded 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 initiating 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.
>>
>> [original patch is from jay.vosburgh@canonical.com]
>> [original patch link https://lore.kernel.org/linux-pci/18609.1588812972@famine/]
>> Fixes: 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")
>> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>> drivers/pci/pcie/err.c | 19 +++++++++++++++----
>> 1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> 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) {
>> -- 
>> 2.17.1
>>
> ---
> 	-Jay Vosburgh, jay.vosburgh@canonical.com
> .
>

Re: [PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices

[Bjorn Helgaas]

On Wed, May 06, 2020 at 08:32:59PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
> If there are non-hotplug capable devices connected to a given
> port, then during the fatal error recovery(triggered by DPC or
> AER), after calling reset_link() function, we cannot rely on
> hotplug handler to detach and re-enumerate the device drivers
> in the affected bus. Instead, we will have to let the error
> recovery handler call report_slot_reset() for all devices in
> the bus to notify about the reset operation. Although this is
> only required for non hot-plug capable devices, doing it for
> hotplug capable devices should not affect the functionality.

Apparently this fixes a bug.  Can you include a link to a problem
report?  The above is a description of a *solution*, but it's hard to
understand without starting with the problem itself.

The above talks about reset_link(), which is only used in the
io_frozen case.  In that case, I think the only thing this patch
changes is that when reset_link() fails, we'll return with
PCI_ERS_RESULT_DISCONNECT instead of whatever reset_link()
returned.

What this patch *does* change is that in the normal_detected case, we
previously never called reset_link() but now we will if an
.error_detected() callback requested it.

> Along with above issue, this fix also applicable to following
> issue.
> 
> Commit 6d2c89441571 ("PCI/ERR: Update error status after
> reset_link()") added support to store status of reset_link()
> call. Although this fixed the error recovery issue observed if
> the initial value of error status is PCI_ERS_RESULT_DISCONNECT
> or PCI_ERS_RESULT_NO_AER_DRIVER, it also discarded 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.

Sorry, I'm not following this explanation.  IIUC this fixes a bug when
the channel is frozen and .error_detected() returns NEED_RESET.

In that case, the current code does:

  * state == io_frozen
  * .error_detected() returns status == NEED_RESET
  * status = reset_link()
  * if status != RECOVERED, we return status
  * otherwise status == RECOVERED
  * we do not call report_slot_reset()
  * we return RECOVERED

It does seem like we *should* call the .slot_reset() callbacks, but
the only time we return recovery failure is if reset_link() failed.

I can certainly understand if drivers don't recover when we reset
their device but don't call their .slot_reset() callback.  Is that
what this fixes?

After the patch,

  * state == io_frozen
  * .error_detected() returns status == NEED_RESET
  * we set status = NEED_RESET (doesn't change anything in this case)
  * we call reset_link()
  * if reset_link() failed, we return DISCONNECT
  * otherwise continue with status == NEED_RESET
  * we *do* call report_slot_reset()
  * we return whatever .slot_reset() returned

I think the change to call .slot_reset() makes sense, but that's not
at all clear from the commit log.  Am I understanding the behavior
correctly?

> 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 initiating 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.

Use "pcie_do_recovery()" and "report_slot_reset()" (including the
parentheses) to follow convention.  Also fix other occurrences above.

> [original patch is from jay.vosburgh@canonical.com]
> [original patch link https://lore.kernel.org/linux-pci/18609.1588812972@famine/]
> Fixes: 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")
> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  drivers/pci/pcie/err.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> 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;

As far as I can tell, there is no caller of pcie_do_recovery() where
reset_link is NULL, so this call of pci_bus_error_reset() looks like
dead code.

We did not previously check whether reset_link was NULL, and this
patch changes nothing that could result in it being NULL.

> +		}
> +
> +		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) {
> -- 
> 2.17.1
> 

Re: [PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices

[Kuppuswamy, Sathyanarayanan]

Hi Bjorn,

On 5/13/20 3:44 PM, Bjorn Helgaas wrote:
> On Wed, May 06, 2020 at 08:32:59PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>
>> If there are non-hotplug capable devices connected to a given
>> port, then during the fatal error recovery(triggered by DPC or
>> AER), after calling reset_link() function, we cannot rely on
>> hotplug handler to detach and re-enumerate the device drivers
>> in the affected bus. Instead, we will have to let the error
>> recovery handler call report_slot_reset() for all devices in
>> the bus to notify about the reset operation. Although this is
>> only required for non hot-plug capable devices, doing it for
>> hotplug capable devices should not affect the functionality.
> 
> Apparently this fixes a bug.  Can you include a link to a problem
> report?  The above is a description of a *solution*, but it's hard to
> understand without starting with the problem itself.

Let me add the problem description to the commit log in next version.

Adding it here for discussion.

Current pcie_do_recovery() implementation has following three issues:

1. Fatal (DPC) error recovery is currently broken for non-hotplug
capable devices. Current DPC recovery implementation relies on hotplug
handler for detaching/re-enumerating the affected devices/drivers on
DLLSC state changes. So when dealing with non-hotplug capable devices,
recovery code does not restore the state of the affected devices
correctly. Correct implementation should call report_mmio_enabled() and
report_slot_reset() functions after reseting the link/device which is
currently missing.

2. For non-fatal errors if report_error_detected() or
report_mmio_enabled() functions requests PCI_ERS_RESULT_NEED_RESET then
current pcie_do_recovery() implementation does not do the requested
explicit device reset, instead just calls the report_slot_reset() on all
affected devices. Notifying about the reset via report_slot_reset()
without doing the actual device reset is incorrect.

3. For fatal errors, pcie_do_recovery() function currently completely
ignores the status result of report_error_detected() call. After calling
reset_link() function then status value gets updated to either
PCI_ERS_RESULT_DISCONNECT or PCI_ERS_RESULT_RECOVERED, and the initial
value returned from report_error_detected() is totally ignored. For
solving the issue # 1, we need to update the status only if reset_link()
call failed and returns PCI_ERS_RESULT_DISCONNECT. Otherwise recovery
handler should proceed to call report_mmio_enabled() and
report_slot_reset().


> 
> The above talks about reset_link(), which is only used in the
> io_frozen case.  In that case, I think the only thing this patch
> changes is that when reset_link() fails, we'll return with
> PCI_ERS_RESULT_DISCONNECT instead of whatever reset_link()
> returned.
Yes, if reset_link() is successful then we let pcie_do_recovery()
proceed to call report_slot_reset() with initial status value returned
from report_error_detected() function call.
> 
> What this patch *does* change is that in the normal_detected case, we
> previously never called reset_link() but now we will if an
> .error_detected() callback requested it.
yes, as highlighted in issue # 1 this patch adds support to call
reset_link() if report_error_detected() function requests it.
> 
>> Along with above issue, this fix also applicable to following
>> issue.
>>
>> Commit 6d2c89441571 ("PCI/ERR: Update error status after
>> reset_link()") added support to store status of reset_link()
>> call. Although this fixed the error recovery issue observed if
>> the initial value of error status is PCI_ERS_RESULT_DISCONNECT
>> or PCI_ERS_RESULT_NO_AER_DRIVER, it also discarded 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.
> 
> Sorry, I'm not following this explanation.  IIUC this fixes a bug when
> the channel is frozen and .error_detected() returns NEED_RESET.
> 
> In that case, the current code does:
> 
>    * state == io_frozen
>    * .error_detected() returns status == NEED_RESET
>    * status = reset_link()
>    * if status != RECOVERED, we return status
>    * otherwise status == RECOVERED
>    * we do not call report_slot_reset()
>    * we return RECOVERED
> 
> It does seem like we *should* call the .slot_reset() callbacks, but
> the only time we return recovery failure is if reset_link() failed.
> 
> I can certainly understand if drivers don't recover when we reset
> their device but don't call their .slot_reset() callback.  Is that
> what this fixes?
> 
> After the patch,
> 
>    * state == io_frozen
>    * .error_detected() returns status == NEED_RESET
>    * we set status = NEED_RESET (doesn't change anything in this case)
>    * we call reset_link()
>    * if reset_link() failed, we return DISCONNECT
>    * otherwise continue with status == NEED_RESET
>    * we *do* call report_slot_reset()
>    * we return whatever .slot_reset() returned
> 
> I think the change to call .slot_reset() makes sense, but that's not
> at all clear from the commit log.  Am I understanding the behavior
> correctly?
Yes, your understanding is correct. I hope above mentioned error
description clarifies it.
> 
>> 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 initiating 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.
> 
> Use "pcie_do_recovery()" and "report_slot_reset()" (including the
> parentheses) to follow convention.  Also fix other occurrences above.
will fix it in next version.
> 
>> [original patch is from jay.vosburgh@canonical.com]
>> [original patch link https://lore.kernel.org/linux-pci/18609.1588812972@famine/]
>> Fixes: 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")
>> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>>   drivers/pci/pcie/err.c | 19 +++++++++++++++----
>>   1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> 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;
> 
> As far as I can tell, there is no caller of pcie_do_recovery() where
> reset_link is NULL, so this call of pci_bus_error_reset() looks like
> dead code.
Yes, I will remove the if (reset_link) check.
> 
> We did not previously check whether reset_link was NULL, and this
> patch changes nothing that could result in it being NULL.
> 
>> +		}
>> +
>> +		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) {
>> -- 
>> 2.17.1
>>

Re: [PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices

[Yicong Yang]

On 2020/5/7 11:32, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>
> If there are non-hotplug capable devices connected to a given
> port, then during the fatal error recovery(triggered by DPC or
> AER), after calling reset_link() function, we cannot rely on
> hotplug handler to detach and re-enumerate the device drivers
> in the affected bus. Instead, we will have to let the error
> recovery handler call report_slot_reset() for all devices in
> the bus to notify about the reset operation. Although this is
> only required for non hot-plug capable devices, doing it for
> hotplug capable devices should not affect the functionality.
>
> Along with above issue, this fix also applicable to following
> issue.
>
> Commit 6d2c89441571 ("PCI/ERR: Update error status after
> reset_link()") added support to store status of reset_link()
> call. Although this fixed the error recovery issue observed if
> the initial value of error status is PCI_ERS_RESULT_DISCONNECT
> or PCI_ERS_RESULT_NO_AER_DRIVER, it also discarded 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 initiating 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.
>
> [original patch is from jay.vosburgh@canonical.com]
> [original patch link https://lore.kernel.org/linux-pci/18609.1588812972@famine/]
> Fixes: 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")
> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  drivers/pci/pcie/err.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> 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)

we'll call reset_link() only if link is frozen. so it may have problem here.

Thanks,
Yicong


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

Re: [PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices

[Kuppuswamy, Sathyanarayanan]

On 5/20/20 1:28 AM, Yicong Yang wrote:
> On 2020/5/7 11:32, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>
>> If there are non-hotplug capable devices connected to a given
>> port, then during the fatal error recovery(triggered by DPC or
>> AER), after calling reset_link() function, we cannot rely on
>> hotplug handler to detach and re-enumerate the device drivers
>> in the affected bus. Instead, we will have to let the error
>> recovery handler call report_slot_reset() for all devices in
>> the bus to notify about the reset operation. Although this is
>> only required for non hot-plug capable devices, doing it for
>> hotplug capable devices should not affect the functionality.
>>
>> Along with above issue, this fix also applicable to following
>> issue.
>>
>> Commit 6d2c89441571 ("PCI/ERR: Update error status after
>> reset_link()") added support to store status of reset_link()
>> call. Although this fixed the error recovery issue observed if
>> the initial value of error status is PCI_ERS_RESULT_DISCONNECT
>> or PCI_ERS_RESULT_NO_AER_DRIVER, it also discarded 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 initiating 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.
>>
>> [original patch is from jay.vosburgh@canonical.com]
>> [original patch link https://lore.kernel.org/linux-pci/18609.1588812972@famine/]
>> Fixes: 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")
>> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>>   drivers/pci/pcie/err.c | 19 +++++++++++++++----
>>   1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> 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)
> 
> we'll call reset_link() only if link is frozen. so it may have problem here.
you mean before this change right?
After this change, reset_link() will be called as long as status is
PCI_ERS_RESULT_NEED_RESET.
> 
> Thanks,
> Yicong
> 
> 
>> +				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) {
> 

Re: [PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices

[Yicong Yang]

On 2020/5/21 1:04, Kuppuswamy, Sathyanarayanan wrote:
>
>
> On 5/20/20 1:28 AM, Yicong Yang wrote:
>> On 2020/5/7 11:32, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>
>>> If there are non-hotplug capable devices connected to a given
>>> port, then during the fatal error recovery(triggered by DPC or
>>> AER), after calling reset_link() function, we cannot rely on
>>> hotplug handler to detach and re-enumerate the device drivers
>>> in the affected bus. Instead, we will have to let the error
>>> recovery handler call report_slot_reset() for all devices in
>>> the bus to notify about the reset operation. Although this is
>>> only required for non hot-plug capable devices, doing it for
>>> hotplug capable devices should not affect the functionality.
>>>
>>> Along with above issue, this fix also applicable to following
>>> issue.
>>>
>>> Commit 6d2c89441571 ("PCI/ERR: Update error status after
>>> reset_link()") added support to store status of reset_link()
>>> call. Although this fixed the error recovery issue observed if
>>> the initial value of error status is PCI_ERS_RESULT_DISCONNECT
>>> or PCI_ERS_RESULT_NO_AER_DRIVER, it also discarded 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 initiating 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.
>>>
>>> [original patch is from jay.vosburgh@canonical.com]
>>> [original patch link https://lore.kernel.org/linux-pci/18609.1588812972@famine/]
>>> Fixes: 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")
>>> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
>>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>> ---
>>>   drivers/pci/pcie/err.c | 19 +++++++++++++++----
>>>   1 file changed, 15 insertions(+), 4 deletions(-)
>>>
>>> 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)
>>
>> we'll call reset_link() only if link is frozen. so it may have problem here.
> you mean before this change right?
> After this change, reset_link() will be called as long as status is
> PCI_ERS_RESULT_NEED_RESET.

Yes. I think we should reset the link only if the io is blocked as before. There's
no reason to reset a normal link.

Furthermore, PCI_ERS_RESULT_NEED_RESET means device driver requires a slot reset rather
than a link reset, so it maybe improper to use it to judge whether a link reset is needed.
We decide whether to do a link reset only by the io state.

Thanks,
Yicong


>>
>> Thanks,
>> Yicong
>>
>>
>>> +                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) {
>>
> .
>

Re: [PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices

[Kuppuswamy, Sathyanarayanan]

On 5/21/20 3:58 AM, Yicong Yang wrote:
> On 2020/5/21 1:04, Kuppuswamy, Sathyanarayanan wrote:
>>
>>
>> On 5/20/20 1:28 AM, Yicong Yang wrote:
>>> On 2020/5/7 11:32, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>>>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>>
>>>> If there are non-hotplug capable devices connected to a given
>>>> port, then during the fatal error recovery(triggered by DPC or
>>>> AER), after calling reset_link() function, we cannot rely on
>>>> hotplug handler to detach and re-enumerate the device drivers
>>>> in the affected bus. Instead, we will have to let the error
>>>> recovery handler call report_slot_reset() for all devices in
>>>> the bus to notify about the reset operation. Although this is
>>>> only required for non hot-plug capable devices, doing it for
>>>> hotplug capable devices should not affect the functionality.
>>>>
>>>> Along with above issue, this fix also applicable to following
>>>> issue.
>>>>
>>>> Commit 6d2c89441571 ("PCI/ERR: Update error status after
>>>> reset_link()") added support to store status of reset_link()
>>>> call. Although this fixed the error recovery issue observed if
>>>> the initial value of error status is PCI_ERS_RESULT_DISCONNECT
>>>> or PCI_ERS_RESULT_NO_AER_DRIVER, it also discarded 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 initiating 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.
>>>>
>>>> [original patch is from jay.vosburgh@canonical.com]
>>>> [original patch link https://lore.kernel.org/linux-pci/18609.1588812972@famine/]
>>>> Fixes: 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")
>>>> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
>>>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>> ---
>>>>    drivers/pci/pcie/err.c | 19 +++++++++++++++----
>>>>    1 file changed, 15 insertions(+), 4 deletions(-)
>>>>
>>>> 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)
>>>
>>> we'll call reset_link() only if link is frozen. so it may have problem here.
>> you mean before this change right?
>> After this change, reset_link() will be called as long as status is
>> PCI_ERS_RESULT_NEED_RESET.
> 
> Yes. I think we should reset the link only if the io is blocked as before. There's
> no reason to reset a normal link.
Currently, only AER and DPC driver uses pcie_do_recovery() call. So the
possible reset_link options are dpc_reset_link() and aer_root_reset().

In dpc_reset_link() case, the link is already disabled and hence we
don't need to do another reset. In case of aer_root_reset() it
uses pci_bus_error_reset() to reset the slot.
> 
> Furthermore, PCI_ERS_RESULT_NEED_RESET means device driver requires a slot reset rather
> than a link reset, so it maybe improper to use it to judge whether a link reset is needed.
> We decide whether to do a link reset only by the io state.
> 
> Thanks,
> Yicong
> 
> 
>>>
>>> Thanks,
>>> Yicong
>>>
>>>
>>>> +                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) {
>>>
>> .
>>
> 

Re: [PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices

[Yicong Yang]

On 2020/5/22 3:31, Kuppuswamy, Sathyanarayanan wrote:
>
>
> On 5/21/20 3:58 AM, Yicong Yang wrote:
>> On 2020/5/21 1:04, Kuppuswamy, Sathyanarayanan wrote:
>>>
>>>
>>> On 5/20/20 1:28 AM, Yicong Yang wrote:
>>>> On 2020/5/7 11:32, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>>>>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>>>
>>>>> If there are non-hotplug capable devices connected to a given
>>>>> port, then during the fatal error recovery(triggered by DPC or
>>>>> AER), after calling reset_link() function, we cannot rely on
>>>>> hotplug handler to detach and re-enumerate the device drivers
>>>>> in the affected bus. Instead, we will have to let the error
>>>>> recovery handler call report_slot_reset() for all devices in
>>>>> the bus to notify about the reset operation. Although this is
>>>>> only required for non hot-plug capable devices, doing it for
>>>>> hotplug capable devices should not affect the functionality.
>>>>>
>>>>> Along with above issue, this fix also applicable to following
>>>>> issue.
>>>>>
>>>>> Commit 6d2c89441571 ("PCI/ERR: Update error status after
>>>>> reset_link()") added support to store status of reset_link()
>>>>> call. Although this fixed the error recovery issue observed if
>>>>> the initial value of error status is PCI_ERS_RESULT_DISCONNECT
>>>>> or PCI_ERS_RESULT_NO_AER_DRIVER, it also discarded 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 initiating 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.
>>>>>
>>>>> [original patch is from jay.vosburgh@canonical.com]
>>>>> [original patch link https://lore.kernel.org/linux-pci/18609.1588812972@famine/]
>>>>> Fixes: 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")
>>>>> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
>>>>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>>> ---
>>>>>    drivers/pci/pcie/err.c | 19 +++++++++++++++----
>>>>>    1 file changed, 15 insertions(+), 4 deletions(-)
>>>>>
>>>>> 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)
>>>>
>>>> we'll call reset_link() only if link is frozen. so it may have problem here.
>>> you mean before this change right?
>>> After this change, reset_link() will be called as long as status is
>>> PCI_ERS_RESULT_NEED_RESET.
>>
>> Yes. I think we should reset the link only if the io is blocked as before. There's
>> no reason to reset a normal link.
> Currently, only AER and DPC driver uses pcie_do_recovery() call. So the
> possible reset_link options are dpc_reset_link() and aer_root_reset().
>
> In dpc_reset_link() case, the link is already disabled and hence we
> don't need to do another reset. In case of aer_root_reset() it
> uses pci_bus_error_reset() to reset the slot.

Not exactly. In pci_bus_error_reset(), we call pci_slot_reset() only if it's
hotpluggable. But we always call pci_bus_reset() to perform a secondary bus
reset for the bridge. That's what I think is unnecessary for a normal link,
and that's what reset link indicates us to do. The slot reset is introduced
in the process only to solve side effects. (c4eed62a2143, PCI/ERR: Use slot reset if available)

PCI_ERS_RESULT_NEED_RESET indicates that the driver
wants a platform-dependent slot reset and its ->slot_reset() method to be called then.
I don't think it's same as slot reset mentioned above, which is only for hotpluggable
ones.

Previously, if link is normal and the driver reports PCI_ERS_RESULT_NEED_RESET,
we'll only call ->slot_reset() without slot reset in reset_link(). Maybe it's better
to perform just like before.

Thanks.


>>
>> Furthermore, PCI_ERS_RESULT_NEED_RESET means device driver requires a slot reset rather
>> than a link reset, so it maybe improper to use it to judge whether a link reset is needed.
>> We decide whether to do a link reset only by the io state.
>>
>> Thanks,
>> Yicong
>>
>>
>>>>
>>>> Thanks,
>>>> Yicong
>>>>
>>>>
>>>>> +                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) {
>>>>
>>> .
>>>
>>
> .
>

Re: [PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices

[Kuppuswamy, Sathyanarayanan]

Hi,

On 5/21/20 7:56 PM, Yicong Yang wrote:
> 
> 
> On 2020/5/22 3:31, Kuppuswamy, Sathyanarayanan wrote:
>>
>>
>> On 5/21/20 3:58 AM, Yicong Yang wrote:
>>> On 2020/5/21 1:04, Kuppuswamy, Sathyanarayanan wrote:
>>>>
>>>>
>>>> On 5/20/20 1:28 AM, Yicong Yang wrote:
>>>>> On 2020/5/7 11:32, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>>>>>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>>>>
>>>>>> If there are non-hotplug capable devices connected to a given
>>>>>> port, then during the fatal error recovery(triggered by DPC or
>>>>>> AER), after calling reset_link() function, we cannot rely on
>>>>>> hotplug handler to detach and re-enumerate the device drivers
>>>>>> in the affected bus. Instead, we will have to let the error
>>>>>> recovery handler call report_slot_reset() for all devices in
>>>>>> the bus to notify about the reset operation. Although this is
>>>>>> only required for non hot-plug capable devices, doing it for
>>>>>> hotplug capable devices should not affect the functionality.
>>>>>>
>>>>>> Along with above issue, this fix also applicable to following
>>>>>> issue.
>>>>>>
>>>>>> Commit 6d2c89441571 ("PCI/ERR: Update error status after
>>>>>> reset_link()") added support to store status of reset_link()
>>>>>> call. Although this fixed the error recovery issue observed if
>>>>>> the initial value of error status is PCI_ERS_RESULT_DISCONNECT
>>>>>> or PCI_ERS_RESULT_NO_AER_DRIVER, it also discarded 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 initiating 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.
>>>>>>
>>>>>> [original patch is from jay.vosburgh@canonical.com]
>>>>>> [original patch link https://lore.kernel.org/linux-pci/18609.1588812972@famine/]
>>>>>> Fixes: 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")
>>>>>> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
>>>>>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>>>> ---
>>>>>>     drivers/pci/pcie/err.c | 19 +++++++++++++++----
>>>>>>     1 file changed, 15 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> 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)
>>>>>
>>>>> we'll call reset_link() only if link is frozen. so it may have problem here.
>>>> you mean before this change right?
>>>> After this change, reset_link() will be called as long as status is
>>>> PCI_ERS_RESULT_NEED_RESET.
>>>
>>> Yes. I think we should reset the link only if the io is blocked as before. There's
>>> no reason to reset a normal link.
>> Currently, only AER and DPC driver uses pcie_do_recovery() call. So the
>> possible reset_link options are dpc_reset_link() and aer_root_reset().
>>
>> In dpc_reset_link() case, the link is already disabled and hence we
>> don't need to do another reset. In case of aer_root_reset() it
>> uses pci_bus_error_reset() to reset the slot.
> 
> Not exactly. In pci_bus_error_reset(), we call pci_slot_reset() only if it's
> hotpluggable. But we always call pci_bus_reset() to perform a secondary bus
> reset for the bridge. That's what I think is unnecessary for a normal link,
> and that's what reset link indicates us to do. The slot reset is introduced
> in the process only to solve side effects. (c4eed62a2143, PCI/ERR: Use slot reset if available)

IIUC, pci_bus_reset() will do slot reset if its supported (hot-plug
capable slots). If its not supported then it will attempt secondary
bus reset. So secondary bus reset will be attempted only if slot
reset is not supported.

Since reported_error_detected() requests us to do reset, we will have
to attempt some kind of reset before we call ->slot_reset() right?
What is the side effect in calling secondary bus reset?

> 
> PCI_ERS_RESULT_NEED_RESET indicates that the driver
> wants a platform-dependent slot reset and its ->slot_reset() method to be called then.
> I don't think it's same as slot reset mentioned above, which is only for hotpluggable
> ones.
What you think is the correct reset implementation ? Is it something
like this?

if (hotplug capable)
    try_slot_reset()
else
    do_nothing()
> 
> Previously, if link is normal and the driver reports PCI_ERS_RESULT_NEED_RESET,
> we'll only call ->slot_reset() without slot reset in reset_link(). Maybe it's better
> to perform just like before.
> 
> Thanks.
> 
> 
>>>
>>> Furthermore, PCI_ERS_RESULT_NEED_RESET means device driver requires a slot reset rather
>>> than a link reset, so it maybe improper to use it to judge whether a link reset is needed.
>>> We decide whether to do a link reset only by the io state.
>>>
>>> Thanks,
>>> Yicong
>>>
>>>
>>>>>
>>>>> Thanks,
>>>>> Yicong
>>>>>
>>>>>
>>>>>> +                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) {
>>>>>
>>>> .
>>>>
>>>
>> .
>>
> 

Re: [PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices

[Oliver O'Halloran]

On Wed, May 27, 2020 at 12:00 PM Kuppuswamy, Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>
> Hi,
>
> On 5/21/20 7:56 PM, Yicong Yang wrote:
> >
> >
> > On 2020/5/22 3:31, Kuppuswamy, Sathyanarayanan wrote:
> >>
> > Not exactly. In pci_bus_error_reset(), we call pci_slot_reset() only if it's
> > hotpluggable. But we always call pci_bus_reset() to perform a secondary bus
> > reset for the bridge. That's what I think is unnecessary for a normal link,
> > and that's what reset link indicates us to do. The slot reset is introduced
> > in the process only to solve side effects. (c4eed62a2143, PCI/ERR: Use slot reset if available)
>
> IIUC, pci_bus_reset() will do slot reset if its supported (hot-plug
> capable slots). If its not supported then it will attempt secondary
> bus reset. So secondary bus reset will be attempted only if slot
> reset is not supported.
>
> Since reported_error_detected() requests us to do reset, we will have
> to attempt some kind of reset before we call ->slot_reset() right?

Yes, the driver returns PCI_ERS_RESULT_NEED_RESET from
->error_detected() to indicate that it doesn't know how to recover
from the error. How that reset is performed doesn't really matter, but
it does need to happen.


> > PCI_ERS_RESULT_NEED_RESET indicates that the driver
> > wants a platform-dependent slot reset and its ->slot_reset() method to be called then.
> > I don't think it's same as slot reset mentioned above, which is only for hotpluggable
> > ones.
> What you think is the correct reset implementation ? Is it something
> like this?
>
> if (hotplug capable)
>     try_slot_reset()
> else
>     do_nothing()

Looks broken to me, but all the reset handling is a rat's nest so
maybe I'm missing something. In the case of a DPC trip the link is
disabled which has the side-effect of hot-resetting the downstream
device. Maybe it's fine?

As an aside, why do we have both ->slot_reset() and ->reset_done() in
the error handling callbacks? Seems like their roles are almost
identical.

Oliver

Re: [PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices

[Kuppuswamy, Sathyanarayanan]

Hi,

On 5/26/20 8:00 PM, Oliver O'Halloran wrote:
> On Wed, May 27, 2020 at 12:00 PM Kuppuswamy, Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>
>> Hi,
>>
>> On 5/21/20 7:56 PM, Yicong Yang wrote:
>>>
>>>
>>> On 2020/5/22 3:31, Kuppuswamy, Sathyanarayanan wrote:
>>>>
>>> Not exactly. In pci_bus_error_reset(), we call pci_slot_reset() only if it's
>>> hotpluggable. But we always call pci_bus_reset() to perform a secondary bus
>>> reset for the bridge. That's what I think is unnecessary for a normal link,
>>> and that's what reset link indicates us to do. The slot reset is introduced
>>> in the process only to solve side effects. (c4eed62a2143, PCI/ERR: Use slot reset if available)
>>
>> IIUC, pci_bus_reset() will do slot reset if its supported (hot-plug
>> capable slots). If its not supported then it will attempt secondary
>> bus reset. So secondary bus reset will be attempted only if slot
>> reset is not supported.
>>
>> Since reported_error_detected() requests us to do reset, we will have
>> to attempt some kind of reset before we call ->slot_reset() right?
> 
> Yes, the driver returns PCI_ERS_RESULT_NEED_RESET from
> ->error_detected() to indicate that it doesn't know how to recover
> from the error. How that reset is performed doesn't really matter, but
> it does need to happen.
> 
> 
>>> PCI_ERS_RESULT_NEED_RESET indicates that the driver
>>> wants a platform-dependent slot reset and its ->slot_reset() method to be called then.
>>> I don't think it's same as slot reset mentioned above, which is only for hotpluggable
>>> ones.
>> What you think is the correct reset implementation ? Is it something
>> like this?
>>
>> if (hotplug capable)
>>      try_slot_reset()
>> else
>>      do_nothing()
> 
> Looks broken to me, but all the reset handling is a rat's nest so
> maybe I'm missing something. In the case of a DPC trip the link is
> disabled which has the side-effect of hot-resetting the downstream
> device. Maybe it's fine?
Yes, in case of DPC (Fatal errors) link is already reset. So we
don't need any special handling. This reset logic is mainly for
non-fatal errors.
> 
> As an aside, why do we have both ->slot_reset() and ->reset_done() in
> the error handling callbacks? Seems like their roles are almost
> identical.
Not sure.I think reset_done() is final cleanup.
> 
> Oliver
> 

Re: [PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices

[Oliver O'Halloran]

On Wed, May 27, 2020 at 1:06 PM Kuppuswamy, Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>
> Yes, in case of DPC (Fatal errors) link is already reset. So we
> don't need any special handling. This reset logic is mainly for
> non-fatal errors.

Why? In our experience most fatal errors aren't all that fatal and can
be recovered by resetting the device. The base spec backs that up (see
gen5 base, sec 6.2) too saying the main point of distinction between
fatal and non-fatal errors is whether handling the error requires a
reset or not. For EEH we always try to recover the device and only
mark it as permanently failed once the devices goes over the max error
threshold (5 errors per hour, by default). Doing something similar for
(native) DPC would make sense IMO.

Re: [PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices

[Yicong Yang]

Hi,


On 2020/5/27 9:31, Kuppuswamy, Sathyanarayanan wrote:
> Hi,
>
> On 5/21/20 7:56 PM, Yicong Yang wrote:
>>
>>
>> On 2020/5/22 3:31, Kuppuswamy, Sathyanarayanan wrote:
>>>
>>>
>>> On 5/21/20 3:58 AM, Yicong Yang wrote:
>>>> On 2020/5/21 1:04, Kuppuswamy, Sathyanarayanan wrote:
>>>>>
>>>>>
>>>>> On 5/20/20 1:28 AM, Yicong Yang wrote:
>>>>>> On 2020/5/7 11:32, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>>>>>>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>>>>>
>>>>>>> If there are non-hotplug capable devices connected to a given
>>>>>>> port, then during the fatal error recovery(triggered by DPC or
>>>>>>> AER), after calling reset_link() function, we cannot rely on
>>>>>>> hotplug handler to detach and re-enumerate the device drivers
>>>>>>> in the affected bus. Instead, we will have to let the error
>>>>>>> recovery handler call report_slot_reset() for all devices in
>>>>>>> the bus to notify about the reset operation. Although this is
>>>>>>> only required for non hot-plug capable devices, doing it for
>>>>>>> hotplug capable devices should not affect the functionality.
>>>>>>>
>>>>>>> Along with above issue, this fix also applicable to following
>>>>>>> issue.
>>>>>>>
>>>>>>> Commit 6d2c89441571 ("PCI/ERR: Update error status after
>>>>>>> reset_link()") added support to store status of reset_link()
>>>>>>> call. Although this fixed the error recovery issue observed if
>>>>>>> the initial value of error status is PCI_ERS_RESULT_DISCONNECT
>>>>>>> or PCI_ERS_RESULT_NO_AER_DRIVER, it also discarded 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 initiating 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.
>>>>>>>
>>>>>>> [original patch is from jay.vosburgh@canonical.com]
>>>>>>> [original patch link https://lore.kernel.org/linux-pci/18609.1588812972@famine/]
>>>>>>> Fixes: 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")
>>>>>>> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
>>>>>>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>>>>> ---
>>>>>>>     drivers/pci/pcie/err.c | 19 +++++++++++++++----
>>>>>>>     1 file changed, 15 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> 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)
>>>>>>
>>>>>> we'll call reset_link() only if link is frozen. so it may have problem here.
>>>>> you mean before this change right?
>>>>> After this change, reset_link() will be called as long as status is
>>>>> PCI_ERS_RESULT_NEED_RESET.
>>>>
>>>> Yes. I think we should reset the link only if the io is blocked as before. There's
>>>> no reason to reset a normal link.
>>> Currently, only AER and DPC driver uses pcie_do_recovery() call. So the
>>> possible reset_link options are dpc_reset_link() and aer_root_reset().
>>>
>>> In dpc_reset_link() case, the link is already disabled and hence we
>>> don't need to do another reset. In case of aer_root_reset() it
>>> uses pci_bus_error_reset() to reset the slot.
>>
>> Not exactly. In pci_bus_error_reset(), we call pci_slot_reset() only if it's
>> hotpluggable. But we always call pci_bus_reset() to perform a secondary bus
>> reset for the bridge. That's what I think is unnecessary for a normal link,
>> and that's what reset link indicates us to do. The slot reset is introduced
>> in the process only to solve side effects. (c4eed62a2143, PCI/ERR: Use slot reset if available)
>
> IIUC, pci_bus_reset() will do slot reset if its supported (hot-plug
> capable slots). If its not supported then it will attempt secondary
> bus reset. So secondary bus reset will be attempted only if slot
> reset is not supported.
>
> Since reported_error_detected() requests us to do reset, we will have
> to attempt some kind of reset before we call ->slot_reset() right?
> What is the side effect in calling secondary bus reset?

I agree we should do a slot reset if driver required. The question is if we apply
the patch, think of a situation that the io is normal, the slot is not hotpluggable but
driver reports a reset, then:
-->aer_root_reset()
----->pci_bus_error_reset()
---------> pci_bridge_secondary_bus_reset()  // Is it necessary to reset if the link is not blocked?

Before commit (c4eed62a2143, PCI/ERR: Use slot reset if available), the reset_link() for aer is
-->aer_root_reset()
----->pci_bridge_secondary_bus_reset()

As mentioned by the commit c4eed62a2143 "The secondary bus reset may have link side effects that a hotplug capable
port may incorrectly react to. Use the slot specific reset for hotplug ports, fixing the undesirable link
down-up handling during error recovering." So I assume it use hotplug slot reset rather than secondary
bus reset to recover the link. If the link is normal, it's unnecessary to do so. so we should add a check
before reset the link in the patch:

+ if (status == PCI_ERS_RESULT_NEED_RESET &&
+      state == pci_channel_io_frozen) {

We should do slot reset if driver required, but it's different from the `slot reset` in pci_bus_error_reset().
Previously we don't do a slot reset and call ->slot_reset() directly, I don't know the certain reason.

Thanks,
Yicong
>
>>
>> PCI_ERS_RESULT_NEED_RESET indicates that the driver
>> wants a platform-dependent slot reset and its ->slot_reset() method to be called then.
>> I don't think it's same as slot reset mentioned above, which is only for hotpluggable
>> ones.
> What you think is the correct reset implementation ? Is it something
> like this?
>
> if (hotplug capable)
>    try_slot_reset()
> else
>    do_nothing()
>>
>> Previously, if link is normal and the driver reports PCI_ERS_RESULT_NEED_RESET,
>> we'll only call ->slot_reset() without slot reset in reset_link(). Maybe it's better
>> to perform just like before.
>>
>> Thanks.
>>
>>
>>>>
>>>> Furthermore, PCI_ERS_RESULT_NEED_RESET means device driver requires a slot reset rather
>>>> than a link reset, so it maybe improper to use it to judge whether a link reset is needed.
>>>> We decide whether to do a link reset only by the io state.
>>>>
>>>> Thanks,
>>>> Yicong
>>>>
>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Yicong
>>>>>>
>>>>>>
>>>>>>> +                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) {
>>>>>>
>>>>> .
>>>>>
>>>>
>>> .
>>>
>>
> .
>

Re: [PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices

[Kuppuswamy, Sathyanarayanan]

On 5/26/20 8:50 PM, Yicong Yang wrote:
> Hi,
> 
> 
> On 2020/5/27 9:31, Kuppuswamy, Sathyanarayanan wrote:
>> Hi,
>>
>> On 5/21/20 7:56 PM, Yicong Yang wrote:
>>>
>>>
>>> On 2020/5/22 3:31, Kuppuswamy, Sathyanarayanan wrote:
>>>>
>>>>
>>>> On 5/21/20 3:58 AM, Yicong Yang wrote:
>>>>> On 2020/5/21 1:04, Kuppuswamy, Sathyanarayanan wrote:
>>>>>>
>>>>>>
>>>>>> On 5/20/20 1:28 AM, Yicong Yang wrote:
>>>>>>> On 2020/5/7 11:32, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>>>>>>>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>>>>>>
>>>>>>>> If there are non-hotplug capable devices connected to a given
>>>>>>>> port, then during the fatal error recovery(triggered by DPC or
>>>>>>>> AER), after calling reset_link() function, we cannot rely on
>>>>>>>> hotplug handler to detach and re-enumerate the device drivers
>>>>>>>> in the affected bus. Instead, we will have to let the error
>>>>>>>> recovery handler call report_slot_reset() for all devices in
>>>>>>>> the bus to notify about the reset operation. Although this is
>>>>>>>> only required for non hot-plug capable devices, doing it for
>>>>>>>> hotplug capable devices should not affect the functionality.
>>>>>>>>
>>>>>>>> Along with above issue, this fix also applicable to following
>>>>>>>> issue.
>>>>>>>>
>>>>>>>> Commit 6d2c89441571 ("PCI/ERR: Update error status after
>>>>>>>> reset_link()") added support to store status of reset_link()
>>>>>>>> call. Although this fixed the error recovery issue observed if
>>>>>>>> the initial value of error status is PCI_ERS_RESULT_DISCONNECT
>>>>>>>> or PCI_ERS_RESULT_NO_AER_DRIVER, it also discarded 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 initiating 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.
>>>>>>>>
>>>>>>>> [original patch is from jay.vosburgh@canonical.com]
>>>>>>>> [original patch link https://lore.kernel.org/linux-pci/18609.1588812972@famine/]
>>>>>>>> Fixes: 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")
>>>>>>>> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
>>>>>>>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>>>>>> ---
>>>>>>>>      drivers/pci/pcie/err.c | 19 +++++++++++++++----
>>>>>>>>      1 file changed, 15 insertions(+), 4 deletions(-)
>>>>>>>>
>>>>>>>> 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)
>>>>>>>
>>>>>>> we'll call reset_link() only if link is frozen. so it may have problem here.
>>>>>> you mean before this change right?
>>>>>> After this change, reset_link() will be called as long as status is
>>>>>> PCI_ERS_RESULT_NEED_RESET.
>>>>>
>>>>> Yes. I think we should reset the link only if the io is blocked as before. There's
>>>>> no reason to reset a normal link.
>>>> Currently, only AER and DPC driver uses pcie_do_recovery() call. So the
>>>> possible reset_link options are dpc_reset_link() and aer_root_reset().
>>>>
>>>> In dpc_reset_link() case, the link is already disabled and hence we
>>>> don't need to do another reset. In case of aer_root_reset() it
>>>> uses pci_bus_error_reset() to reset the slot.
>>>
>>> Not exactly. In pci_bus_error_reset(), we call pci_slot_reset() only if it's
>>> hotpluggable. But we always call pci_bus_reset() to perform a secondary bus
>>> reset for the bridge. That's what I think is unnecessary for a normal link,
>>> and that's what reset link indicates us to do. The slot reset is introduced
>>> in the process only to solve side effects. (c4eed62a2143, PCI/ERR: Use slot reset if available)
>>
>> IIUC, pci_bus_reset() will do slot reset if its supported (hot-plug
>> capable slots). If its not supported then it will attempt secondary
>> bus reset. So secondary bus reset will be attempted only if slot
>> reset is not supported.
>>
>> Since reported_error_detected() requests us to do reset, we will have
>> to attempt some kind of reset before we call ->slot_reset() right?
>> What is the side effect in calling secondary bus reset?
> 
> I agree we should do a slot reset if driver required. The question is if we apply
> the patch, think of a situation that the io is normal, the slot is not hotpluggable but
> driver reports a reset, then:
> -->aer_root_reset()
> ----->pci_bus_error_reset()
> ---------> pci_bridge_secondary_bus_reset()  // Is it necessary to reset if the link is not blocked?
> 
> Before commit (c4eed62a2143, PCI/ERR: Use slot reset if available), the reset_link() for aer is
> -->aer_root_reset()
> ----->pci_bridge_secondary_bus_reset()
> 
> As mentioned by the commit c4eed62a2143 "The secondary bus reset may have link side effects that a hotplug capable
> port may incorrectly react to. Use the slot specific reset for hotplug ports, fixing the undesirable link
> down-up handling during error recovering." So I assume it use hotplug slot reset rather than secondary
> bus reset to recover the link. If the link is normal, it's unnecessary to do so. so we should add a check
> before reset the link in the patch:
> 
> + if (status == PCI_ERS_RESULT_NEED_RESET &&
> +      state == pci_channel_io_frozen) {

> 
> We should do slot reset if driver required, but it's different from the `slot reset` in pci_bus_error_reset().
> Previously we don't do a slot reset and call ->slot_reset() directly, I don't know the certain reason.
IIUC, your concern is whether it is correct to trigger reset for
pci_channel_io_normal case right ? Please correct me if my
assumption is incorrect.

If its true, then why would report_error_detected() will return
PCI_ERS_*_NEED_RESET for pci_channel_io_normal case ? If
report_error_detected() requests reset in pci_channel_io_normal
case then I think we should give preference to it.

Let me know your comments.
> 
> Thanks,
> Yicong
>>
>>>
>>> PCI_ERS_RESULT_NEED_RESET indicates that the driver
>>> wants a platform-dependent slot reset and its ->slot_reset() method to be called then.
>>> I don't think it's same as slot reset mentioned above, which is only for hotpluggable
>>> ones.
>> What you think is the correct reset implementation ? Is it something
>> like this?
>>
>> if (hotplug capable)
>>     try_slot_reset()
>> else
>>     do_nothing()
>>>
>>> Previously, if link is normal and the driver reports PCI_ERS_RESULT_NEED_RESET,
>>> we'll only call ->slot_reset() without slot reset in reset_link(). Maybe it's better
>>> to perform just like before.
>>>
>>> Thanks.
>>>
>>>
>>>>>
>>>>> Furthermore, PCI_ERS_RESULT_NEED_RESET means device driver requires a slot reset rather
>>>>> than a link reset, so it maybe improper to use it to judge whether a link reset is needed.
>>>>> We decide whether to do a link reset only by the io state.
>>>>>
>>>>> Thanks,
>>>>> Yicong
>>>>>
>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Yicong
>>>>>>>
>>>>>>>
>>>>>>>> +                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) {
>>>>>>>
>>>>>> .
>>>>>>
>>>>>
>>>> .
>>>>
>>>
>> .
>>
> 

Re: [PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices

[Yicong Yang]

On 2020/5/27 12:04, Kuppuswamy, Sathyanarayanan wrote:
>
>
> On 5/26/20 8:50 PM, Yicong Yang wrote:
>> Hi,
>>
>>
>> On 2020/5/27 9:31, Kuppuswamy, Sathyanarayanan wrote:
>>> Hi,
>>>
>>> On 5/21/20 7:56 PM, Yicong Yang wrote:
>>>>
>>>>
>>>> On 2020/5/22 3:31, Kuppuswamy, Sathyanarayanan wrote:
>>>>>
>>>>>
>>>>> On 5/21/20 3:58 AM, Yicong Yang wrote:
>>>>>> On 2020/5/21 1:04, Kuppuswamy, Sathyanarayanan wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 5/20/20 1:28 AM, Yicong Yang wrote:
>>>>>>>> On 2020/5/7 11:32, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>>>>>>>>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>>>>>>>
>>>>>>>>> If there are non-hotplug capable devices connected to a given
>>>>>>>>> port, then during the fatal error recovery(triggered by DPC or
>>>>>>>>> AER), after calling reset_link() function, we cannot rely on
>>>>>>>>> hotplug handler to detach and re-enumerate the device drivers
>>>>>>>>> in the affected bus. Instead, we will have to let the error
>>>>>>>>> recovery handler call report_slot_reset() for all devices in
>>>>>>>>> the bus to notify about the reset operation. Although this is
>>>>>>>>> only required for non hot-plug capable devices, doing it for
>>>>>>>>> hotplug capable devices should not affect the functionality.
>>>>>>>>>
>>>>>>>>> Along with above issue, this fix also applicable to following
>>>>>>>>> issue.
>>>>>>>>>
>>>>>>>>> Commit 6d2c89441571 ("PCI/ERR: Update error status after
>>>>>>>>> reset_link()") added support to store status of reset_link()
>>>>>>>>> call. Although this fixed the error recovery issue observed if
>>>>>>>>> the initial value of error status is PCI_ERS_RESULT_DISCONNECT
>>>>>>>>> or PCI_ERS_RESULT_NO_AER_DRIVER, it also discarded 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 initiating 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.
>>>>>>>>>
>>>>>>>>> [original patch is from jay.vosburgh@canonical.com]
>>>>>>>>> [original patch link https://lore.kernel.org/linux-pci/18609.1588812972@famine/]
>>>>>>>>> Fixes: 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")
>>>>>>>>> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
>>>>>>>>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>>>>>>> ---
>>>>>>>>>      drivers/pci/pcie/err.c | 19 +++++++++++++++----
>>>>>>>>>      1 file changed, 15 insertions(+), 4 deletions(-)
>>>>>>>>>
>>>>>>>>> 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)
>>>>>>>>
>>>>>>>> we'll call reset_link() only if link is frozen. so it may have problem here.
>>>>>>> you mean before this change right?
>>>>>>> After this change, reset_link() will be called as long as status is
>>>>>>> PCI_ERS_RESULT_NEED_RESET.
>>>>>>
>>>>>> Yes. I think we should reset the link only if the io is blocked as before. There's
>>>>>> no reason to reset a normal link.
>>>>> Currently, only AER and DPC driver uses pcie_do_recovery() call. So the
>>>>> possible reset_link options are dpc_reset_link() and aer_root_reset().
>>>>>
>>>>> In dpc_reset_link() case, the link is already disabled and hence we
>>>>> don't need to do another reset. In case of aer_root_reset() it
>>>>> uses pci_bus_error_reset() to reset the slot.
>>>>
>>>> Not exactly. In pci_bus_error_reset(), we call pci_slot_reset() only if it's
>>>> hotpluggable. But we always call pci_bus_reset() to perform a secondary bus
>>>> reset for the bridge. That's what I think is unnecessary for a normal link,
>>>> and that's what reset link indicates us to do. The slot reset is introduced
>>>> in the process only to solve side effects. (c4eed62a2143, PCI/ERR: Use slot reset if available)
>>>
>>> IIUC, pci_bus_reset() will do slot reset if its supported (hot-plug
>>> capable slots). If its not supported then it will attempt secondary
>>> bus reset. So secondary bus reset will be attempted only if slot
>>> reset is not supported.
>>>
>>> Since reported_error_detected() requests us to do reset, we will have
>>> to attempt some kind of reset before we call ->slot_reset() right?
>>> What is the side effect in calling secondary bus reset?
>>
>> I agree we should do a slot reset if driver required. The question is if we apply
>> the patch, think of a situation that the io is normal, the slot is not hotpluggable but
>> driver reports a reset, then:
>> -->aer_root_reset()
>> ----->pci_bus_error_reset()
>> ---------> pci_bridge_secondary_bus_reset()  // Is it necessary to reset if the link is not blocked?
>>
>> Before commit (c4eed62a2143, PCI/ERR: Use slot reset if available), the reset_link() for aer is
>> -->aer_root_reset()
>> ----->pci_bridge_secondary_bus_reset()
>>
>> As mentioned by the commit c4eed62a2143 "The secondary bus reset may have link side effects that a hotplug capable
>> port may incorrectly react to. Use the slot specific reset for hotplug ports, fixing the undesirable link
>> down-up handling during error recovering." So I assume it use hotplug slot reset rather than secondary
>> bus reset to recover the link. If the link is normal, it's unnecessary to do so. so we should add a check
>> before reset the link in the patch:
>>
>> + if (status == PCI_ERS_RESULT_NEED_RESET &&
>> +      state == pci_channel_io_frozen) {
>
>>
>> We should do slot reset if driver required, but it's different from the `slot reset` in pci_bus_error_reset().
>> Previously we don't do a slot reset and call ->slot_reset() directly, I don't know the certain reason.
> IIUC, your concern is whether it is correct to trigger reset for
> pci_channel_io_normal case right ? Please correct me if my
> assumption is incorrect.

right.

>
> If its true, then why would report_error_detected() will return
> PCI_ERS_*_NEED_RESET for pci_channel_io_normal case ? If
> report_error_detected() requests reset in pci_channel_io_normal
> case then I think we should give preference to it.

If we get PCI_ERS_*_NEED_RESET, we should do slot reset, no matter it's a
hotpluggable slot or not.  And we shouldn't do it here in reset_link(), that's
two separate things.  The `slot reset` done in aer_root_reset() is only for *link
reset*, as there may have some side effects to perform secondary bus reset directly
for hotpluggable slot, as mentioned in commit c4eed62a2143, so it use slot reset
to do the reset link things.

As for slot reset required by the driver, we should perform it later just before the
->slot_reset(). I noticed the TODO comments there and we should implement
it if it's necessary.

It lies in line 183, drivers/pcie/err.c:

    if (status == PCI_ERS_RESULT_NEED_RESET) {
        /*
         * TODO: Should call platform-specific
         * functions to reset slot before calling
         * drivers' slot_reset callbacks?
         */
        status = PCI_ERS_RESULT_RECOVERED;
        pci_dbg(dev, "broadcast slot_reset message\n");
        pci_walk_bus(bus, report_slot_reset, &status);
    }


>
> Let me know your comments.
>>
>> Thanks,
>> Yicong
>>>
>>>>
>>>> PCI_ERS_RESULT_NEED_RESET indicates that the driver
>>>> wants a platform-dependent slot reset and its ->slot_reset() method to be called then.
>>>> I don't think it's same as slot reset mentioned above, which is only for hotpluggable
>>>> ones.
>>> What you think is the correct reset implementation ? Is it something
>>> like this?
>>>
>>> if (hotplug capable)
>>>     try_slot_reset()
>>> else
>>>     do_nothing()
>>>>
>>>> Previously, if link is normal and the driver reports PCI_ERS_RESULT_NEED_RESET,
>>>> we'll only call ->slot_reset() without slot reset in reset_link(). Maybe it's better
>>>> to perform just like before.
>>>>
>>>> Thanks.
>>>>
>>>>
>>>>>>
>>>>>> Furthermore, PCI_ERS_RESULT_NEED_RESET means device driver requires a slot reset rather
>>>>>> than a link reset, so it maybe improper to use it to judge whether a link reset is needed.
>>>>>> We decide whether to do a link reset only by the io state.
>>>>>>
>>>>>> Thanks,
>>>>>> Yicong
>>>>>>
>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Yicong
>>>>>>>>
>>>>>>>>
>>>>>>>>> +                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) {
>>>>>>>>
>>>>>>> .
>>>>>>>
>>>>>>
>>>>> .
>>>>>
>>>>
>>> .
>>>
>>
> .
>

Re: [PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices

[Kuppuswamy, Sathyanarayanan]

On 5/26/20 11:41 PM, Yicong Yang wrote:
>>> We should do slot reset if driver required, but it's different from the `slot reset` in pci_bus_error_reset().
>>> Previously we don't do a slot reset and call ->slot_reset() directly, I don't know the certain reason.
>> IIUC, your concern is whether it is correct to trigger reset for
>> pci_channel_io_normal case right ? Please correct me if my
>> assumption is incorrect.
> right.
> 
>> If its true, then why would report_error_detected() will return
>> PCI_ERS_*_NEED_RESET for pci_channel_io_normal case ? If
>> report_error_detected() requests reset in pci_channel_io_normal
>> case then I think we should give preference to it.
> If we get PCI_ERS_*_NEED_RESET, we should do slot reset, no matter it's a
> hotpluggable slot or not.

pci_slot_reset() function itself has dependency on hotplug ops. So
what kind of slot reset is needed for non-hotplug case?

static int pci_slot_reset(struct pci_slot *slot, int probe)
{
	int rc;

	if (!slot || !pci_slot_resetable(slot))
		return -ENOTTY;

	if (!probe)
		pci_slot_lock(slot);

	might_sleep();

	rc = pci_reset_hotplug_slot(slot->hotplug, probe);

	if (!probe)
		pci_slot_unlock(slot);

	return rc;
}

static int pci_reset_hotplug_slot(struct hotplug_slot *hotplug, int probe)
{
	int rc = -ENOTTY;

	if (!hotplug || !try_module_get(hotplug->owner))
		return rc;

	if (hotplug->ops->reset_slot)
		rc = hotplug->ops->reset_slot(hotplug, probe);

	module_put(hotplug->owner);

	return rc;
}
   And we shouldn't do it here in reset_link(), that's
> two separate things.  The `slot reset` done in aer_root_reset() is only for *link
> reset*, as there may have some side effects to perform secondary bus reset directly
> for hotpluggable slot, as mentioned in commit c4eed62a2143, so it use slot reset
> to do the reset link things.
> 
> As for slot reset required by the driver, we should perform it later just before the
> ->slot_reset(). I noticed the TODO comments there and we should implement
> it if it's necessary.
I agree.
> 
> It lies in line 183, drivers/pcie/err.c:
> 
>      if (status == PCI_ERS_RESULT_NEED_RESET) {
>          /*
>           * TODO: Should call platform-specific
>           * functions to reset slot before calling
>           * drivers' slot_reset callbacks?
>           */
>          status = PCI_ERS_RESULT_RECOVERED;
>          pci_dbg(dev, "broadcast slot_reset message\n");
>          pci_walk_bus(bus, report_slot_reset, &status);
>      }
> 
>