Hi Jay, I've tested the patches on my board, and they work well.
Thanks, Yicong On 2020/6/25 2:52, Jay Vosburgh wrote: > Jay Vosburgh <jay.vosbu...@canonical.com> wrote: > >> sathyanarayanan.kuppusw...@linux.intel.com wrote: >> >> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppusw...@linux.intel.com> >>> Fatal (DPC) error recovery is currently broken for non-hotplug >>> capable devices. With current implementation, after successful >>> fatal error recovery, non-hotplug capable device state won't be >>> restored properly. You can find related issues in following links. >>> >>> https://lkml.org/lkml/2020/5/27/290 >>> https://lore.kernel.org/linux-pci/12115.1588207324@famine/ >>> https://lkml.org/lkml/2020/3/28/328 >>> >>> Current fatal error 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_slot_reset() >>> function after resetting the link to restore the state of the >>> device/driver. >>> >>> So use PCI_ERS_RESULT_NEED_RESET as error status for successful >>> reset_link() operation and use PCI_ERS_RESULT_DISCONNECT for failure >>> case. PCI_ERS_RESULT_NEED_RESET error state will ensure slot_reset() >>> is called after reset link operation which will also fix the above >>> mentioned issue. >>> >>> [original patch is from jay.vosbu...@canonical.com] >>> [original patch link >>> https://lore.kernel.org/linux-pci/12115.1588207324@famine/] >>> Fixes: 6d2c89441571 ("PCI/ERR: Update error status after reset_link()") >>> Signed-off-by: Jay Vosburgh <jay.vosbu...@canonical.com> >>> Signed-off-by: Kuppuswamy Sathyanarayanan >>> <sathyanarayanan.kuppusw...@linux.intel.com> >> I've tested this patch set on one of our test machines, and it >> resolves the issue. I plan to test with other systems tomorrow. > I've done testing on two different systems that exhibit the > original issue and this patch set appears to behave as expected. > > Has anyone else (Yicong?) had an opportunity to test this? > > Can this be considered for acceptance, or is additional feedback > or review needed? > > -J > >>> --- >>> drivers/pci/pcie/err.c | 24 ++++++++++++++++++++++-- >>> 1 file changed, 22 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c >>> index 14bb8f54723e..5fe8561c7185 100644 >>> --- a/drivers/pci/pcie/err.c >>> +++ b/drivers/pci/pcie/err.c >>> @@ -165,8 +165,28 @@ 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) { >>> + /* >>> + * After resetting the link using reset_link() call, the >>> + * possible value of error status is either >>> + * PCI_ERS_RESULT_DISCONNECT (failure case) or >>> + * PCI_ERS_RESULT_NEED_RESET (success case). >>> + * So ignore the return value of report_error_detected() >>> + * call for fatal errors. Instead use >>> + * PCI_ERS_RESULT_NEED_RESET as initial status value. >>> + * >>> + * Ignoring the status return value of report_error_detected() >>> + * call will also help in case of EDR mode based error >>> + * recovery. In EDR mode AER and DPC Capabilities are owned by >>> + * firmware and hence report_error_detected() call will possibly >>> + * return PCI_ERS_RESULT_NO_AER_DRIVER. So if we don't ignore >>> + * the return value of report_error_detected() then >>> + * pcie_do_recovery() would report incorrect status after >>> + * successful recovery. Ignoring PCI_ERS_RESULT_NO_AER_DRIVER >>> + * in non EDR case should not have any functional impact. >>> + */ >>> + status = PCI_ERS_RESULT_NEED_RESET; >>> + if (reset_link(dev) != PCI_ERS_RESULT_RECOVERED) { >>> + status = PCI_ERS_RESULT_DISCONNECT; >>> pci_warn(dev, "link reset failed\n"); >>> goto failed; >>> } >>> -- >>> 2.17.1 > --- > -Jay Vosburgh, jay.vosbu...@canonical.com > . >
