From: sathyanarayanan.kuppuswamy@linux.intel.com
To: bhelgaas@google.com
Cc: linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
ashok.raj@intel.com, sathyanarayanan.kuppuswamy@linux.intel.com,
Jay Vosburgh <jay.vosburgh@canonical.com>
Subject: [PATCH v3 1/1] PCI/ERR: Fix reset logic in pcie_do_recovery() call
Date: Fri, 24 Jul 2020 12:07:55 -0700 [thread overview]
Message-ID: <cbba08a5e9ca62778c8937f44eda2192a2045da7.1595617529.git.sathyanarayanan.kuppuswamy@linux.intel.com> (raw)
From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Current pcie_do_recovery() implementation has following two issues:
1. Fatal (DPC) error recovery is currently broken for non-hotplug
capable devices. Current fatal error recovery implementation relies
on PCIe hotplug (pciehp) handler for detaching and re-enumerating
the affected devices/drivers. pciehp handler listens for DLLSC state
changes and handles device/driver detachment on DLLSC_LINK_DOWN event
and re-enumeration on DLLSC_LINK_UP event. So when dealing with
non-hotplug capable devices, recovery code does not restore the state
of the affected devices correctly. Correct implementation should
restore the device state and call report_slot_reset() function after
resetting the link to restore the state of the device/driver.
You can find fatal non-hotplug related issues reported in following links:
https://lore.kernel.org/linux-pci/20200527083130.4137-1-Zhiqiang.Hou@nxp.com/
https://lore.kernel.org/linux-pci/12115.1588207324@famine/
https://lore.kernel.org/linux-pci/0e6f89cd6b9e4a72293cc90fafe93487d7c2d295.1585000084.git.sathyanarayanan.kuppuswamy@linux.intel.com/
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.
To fix above issues, use PCI_ERS_RESULT_NEED_RESET as error state after
successful reset_link() operation. This will ensure ->slot_reset() be
called after reset_link() operation for fatal errors. Also call
pci_bus_reset() to do slot/bus reset() before triggering device specific
->slot_reset() callback. Also, using pci_bus_reset() will restore the state
of the devices after performing the reset operation.
Even though using pci_bus_reset() will do redundant reset operation after
->reset_link() for fatal errors, it should should affect the functional
behavior.
[original patch is from jay.vosburgh@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.vosburgh@canonical.com>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
Changes since v2:
* Changed the subject of patch to "PCI/ERR: Fix reset logic in
pcie_do_recovery() call". v2 patch link is,
https://lore.kernel.org/linux-pci/ce417fbf81a8a46a89535f44b9224ee9fbb55a29.1591307288.git.sathyanarayanan.kuppuswamy@linux.intel.com/
* Squashed "PCI/ERR: Add reset support for non fatal errors" patch.
drivers/pci/pcie/err.c | 41 +++++++++++++++++++++++++++++++++++++----
1 file changed, 37 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index 14bb8f54723e..b5eb6ba65be1 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -165,8 +165,29 @@ 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);
+ /*
+ * 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.
+ *
+ * In EDR mode, since AER and DPC Capabilities are owned by
+ * firmware, reported_error_detected() will return error
+ * status PCI_ERS_RESULT_NO_AER_DRIVER. Continuing
+ * pcie_do_recovery() with error status as
+ * PCI_ERS_RESULT_NO_AER_DRIVER will report recovery failure
+ * irrespective of recovery status. But successful reset_link()
+ * call usually recovers all fatal errors. So ignoring the
+ * status result of report_error_detected() also helps EDR based
+ * error recovery.
+ */
status = reset_link(dev);
- if (status != PCI_ERS_RESULT_RECOVERED) {
+ if (status == PCI_ERS_RESULT_RECOVERED) {
+ status = PCI_ERS_RESULT_NEED_RESET;
+ } else {
+ status = PCI_ERS_RESULT_DISCONNECT;
pci_warn(dev, "link reset failed\n");
goto failed;
}
@@ -182,10 +203,22 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
if (status == PCI_ERS_RESULT_NEED_RESET) {
/*
- * TODO: Should call platform-specific
- * functions to reset slot before calling
- * drivers' slot_reset callbacks?
+ * TODO: Optimize the call to pci_reset_bus()
+ *
+ * There are two components to pci_reset_bus().
+ *
+ * 1. Do platform specific slot/bus reset.
+ * 2. Save/Restore all devices in the bus.
+ *
+ * For hotplug capable devices and fatal errors,
+ * device is already in reset state due to link
+ * reset. So repeating platform specific slot/bus
+ * reset via pci_reset_bus() call is redundant. So
+ * can optimize this logic and conditionally call
+ * pci_reset_bus().
*/
+ pci_reset_bus(dev);
+
status = PCI_ERS_RESULT_RECOVERED;
pci_dbg(dev, "broadcast slot_reset message\n");
pci_walk_bus(bus, report_slot_reset, &status);
--
2.17.1
next reply other threads:[~2020-07-24 19:08 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-24 19:07 sathyanarayanan.kuppuswamy [this message]
2020-09-01 5:41 ` [PATCH v3 1/1] PCI/ERR: Fix reset logic in pcie_do_recovery() call Kuppuswamy, Sathyanarayanan
2020-09-22 18:52 ` Bjorn Helgaas
[not found] <3cad1a07-509b-ef51-f635-71def2ff8293@linux.intel.com>
2020-09-22 23:33 ` Bjorn Helgaas
2020-09-22 23:44 ` Kuppuswamy, Sathyanarayanan
2020-09-24 1:15 ` Sinan Kaya
2020-09-24 2:04 ` Kuppuswamy, Sathyanarayanan
2020-09-24 2:16 ` Sinan Kaya
2020-09-24 2:51 ` Kuppuswamy, Sathyanarayanan
2020-09-24 3:13 ` Sinan Kaya
2020-09-24 4:06 ` Kuppuswamy, Sathyanarayanan
2020-09-24 20:52 ` Sinan Kaya
2020-09-25 5:11 ` Kuppuswamy, Sathyanarayanan
2020-09-25 16:55 ` Sinan Kaya
2020-09-25 17:11 ` Kuppuswamy, Sathyanarayanan
2020-09-25 17:47 ` Sinan Kaya
2020-09-25 18:16 ` Kuppuswamy, Sathyanarayanan
2020-09-25 18:30 ` Sinan Kaya
2020-09-28 2:43 ` Kuppuswamy, Sathyanarayanan
2020-09-28 8:41 ` Ethan Zhao
2020-09-28 11:17 ` Sinan Kaya
2020-09-28 12:16 ` Sinan Kaya
2020-09-28 17:15 ` Kuppuswamy, Sathyanarayanan
2020-09-28 18:02 ` Sinan Kaya
2020-09-28 18:25 ` Sinan Kaya
2020-09-28 18:32 ` Kuppuswamy, Sathyanarayanan
2020-10-12 5:13 ` Kuppuswamy, Sathyanarayanan
2020-10-12 14:51 ` Sinan Kaya
2020-09-28 12:11 ` Sinan Kaya
2020-09-25 23:50 ` Sinan Kaya
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cbba08a5e9ca62778c8937f44eda2192a2045da7.1595617529.git.sathyanarayanan.kuppuswamy@linux.intel.com \
--to=sathyanarayanan.kuppuswamy@linux.intel.com \
--cc=20200714230803.GA92891@bjorn-Precision-5520 \
--cc=ashok.raj@intel.com \
--cc=bhelgaas@google.com \
--cc=jay.vosburgh@canonical.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).