Re: [PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices
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);
}
Re: [PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices
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.kuppusw...@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan
>
>
> 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.vosbu...@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
> Signed-off-by: Kuppuswamy Sathyanarayanan
>
> ---
> 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 i
Re: [PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices
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.kuppusw...@linux.intel.com wrote:
From: Kuppuswamy Sathyanarayanan
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.vosbu...@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
Signed-off-by: Kuppuswamy Sathyanarayanan
---
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,
Re: [PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices
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.kuppusw...@linux.intel.com wrote:
>>> From: Kuppuswamy Sathyanarayanan
>>>
>>>
>>> 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.vosbu...@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
>>> Signed-off-by: Kuppuswamy Sathyanarayanan
>>>
>>> ---
>>> 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
> res
Re: [PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices
On Wed, May 27, 2020 at 1:06 PM Kuppuswamy, Sathyanarayanan 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
Hi, On 5/26/20 8:00 PM, Oliver O'Halloran wrote: On Wed, May 27, 2020 at 12:00 PM Kuppuswamy, Sathyanarayanan 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
On Wed, May 27, 2020 at 12:00 PM Kuppuswamy, Sathyanarayanan 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
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.kuppusw...@linux.intel.com wrote:
From: Kuppuswamy Sathyanarayanan
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.vosbu...@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
Signed-off-by: Kuppuswamy Sathyanarayanan
---
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()
Re: [PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices
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.kuppusw...@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan
>
>
> 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.vosbu...@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
> Signed-off-by: Kuppuswamy Sathyanarayanan
>
> ---
>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
>> re
Re: [PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices
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.kuppusw...@linux.intel.com wrote:
From: Kuppuswamy Sathyanarayanan
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.vosbu...@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
Signed-off-by: Kuppuswamy Sathyanarayanan
---
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
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.kuppusw...@linux.intel.com wrote:
>>> From: Kuppuswamy Sathyanarayanan
>>>
>>>
>>> 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.vosbu...@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
>>> Signed-off-by: Kuppuswamy Sathyanarayanan
>>>
>>> ---
>>> 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
On 5/20/20 1:28 AM, Yicong Yang wrote:
On 2020/5/7 11:32, sathyanarayanan.kuppusw...@linux.intel.com wrote:
From: Kuppuswamy Sathyanarayanan
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.vosbu...@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
Signed-off-by: Kuppuswamy Sathyanarayanan
---
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
On 2020/5/7 11:32, sathyanarayanan.kuppusw...@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan
>
> 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.vosbu...@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
> Signed-off-by: Kuppuswamy Sathyanarayanan
>
> ---
> 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
Hi Bjorn,
On 5/13/20 3:44 PM, Bjorn Helgaas wrote:
On Wed, May 06, 2020 at 08:32:59PM -0700,
sathyanarayanan.kuppusw...@linux.intel.com wrote:
From: Kuppuswamy Sathyanarayanan
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
Re: [PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices
On Wed, May 06, 2020 at 08:32:59PM -0700,
sathyanarayanan.kuppusw...@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan
>
> 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.vosbu...@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
> Signed-off-by: Kuppuswamy Sathyanarayanan
>
> ---
> 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;
Re: [PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices
On 2020/5/13 3:20, Jay Vosburgh wrote:
> sathyanarayanan.kuppusw...@linux.intel.com wrote:
>
>> From: Kuppuswamy Sathyanarayanan
>>
>> 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.vosbu...@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
>> Signed-off-by: Kuppuswamy Sathyanarayanan
>>
>> ---
>> 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.vosbu...@canonical.com
> .
>
Re: [PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices
sathyanarayanan.kuppusw...@linux.intel.com wrote:
>From: Kuppuswamy Sathyanarayanan
>
>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.vosbu...@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
>Signed-off-by: Kuppuswamy Sathyanarayanan
>
>---
> 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.vosbu...@canonical.com
[PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices
From: Kuppuswamy Sathyanarayanan
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.vosbu...@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
Signed-off-by: Kuppuswamy Sathyanarayanan
---
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
