| Message ID | f4bbacd3af453285271c8fc733652969e11b84f8.1588821160.git.sathyanarayanan.kuppuswamy@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [v1,1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices | expand |
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
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 > . >
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 >
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 >>
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) {
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) { >
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) { >> > . >
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) { >>> >> . >> >
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) { >>>> >>> . >>> >> > . >
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) { >>>>> >>>> . >>>> >>> >> . >> >
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
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 >
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.
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) { >>>>>> >>>>> . >>>>> >>>> >>> . >>> >> > . >
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) { >>>>>>> >>>>>> . >>>>>> >>>>> >>>> . >>>> >>> >> . >> >
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) { >>>>>>>> >>>>>>> . >>>>>>> >>>>>> >>>>> . >>>>> >>>> >>> . >>> >> > . >
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); > } > >
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) {