* [PATCH v16 0/9] Address error and recovery for AER and DPC
@ 2018-05-11 10:43 Oza Pawandeep
2018-05-11 10:43 ` [PATCH v16 1/9] PCI: Unify wait for link active into generic PCI Oza Pawandeep
` (9 more replies)
0 siblings, 10 replies; 29+ messages in thread
From: Oza Pawandeep @ 2018-05-11 10:43 UTC (permalink / raw)
To: Bjorn Helgaas, Philippe Ombredanne, Thomas Gleixner,
Greg Kroah-Hartman, Kate Stewart, linux-pci, linux-kernel,
Dongdong Liu, Keith Busch, Wei Zhang, Sinan Kaya, Timur Tabi
Cc: Oza Pawandeep
This patch set brings in error handling support for DPC
The current implementation of AER and error message broadcasting to the
EP driver is tightly coupled and limited to AER service driver.
It is important to factor out broadcasting and other link handling
callbacks. So that not only when AER gets triggered, but also when DPC get
triggered (for e.g. ERR_FATAL), callbacks are handled appropriately.
The goal of the patch-set is:
DPC should handle the error handling and recovery similar to AER, because
finally both are attempting recovery in some or the other way,
and for that error handling and recovery framework has to be loosely
coupled.
It achieves uniformity and transparency to the error handling agents such
as AER, DPC, with respect to recovery and error handling.
So, this patch-set tries to unify lot of things between error agents and
make them behave in a well defined way. (be it error (FATAL, NON_FATAL)
handling or recovery).
The FATAL error handling is handled with remove/reset_link/re-enumerate
sequence while the NON_FATAL follows the default path.
Documentation/PCI/pci-error-recovery.txt talks more on that.
Changes since v15:
Bjorn's comments addressed
> minor comments fixed
> made FATAL sequence aligned to existing one, as far as clearing status are concerned.
> pcie_do_fatal_recovery and pcie_do_nonfatal_recovery functions made to modularize
> pcie_do_fatal_recovery now takes service as an argument
Changes since v14:
Bjorn's comments addressed
> simplified the patch set, and moved AER_FATAL handling in the beginning.
> rebase the code to 4.17-rc1.
Changes since v13:
Bjorn's comments addressed
> handke FATAL errors with remove devices followed by re-enumeration.
> changes in AER and DPC along with required Documentation.
Changes since v12:
Bjorn's and Keith's Comments addressed.
> Made DPC and AER error handling identical <aligned err.c>
> hanldled cases for hotplug enabled system differently.
Changes since v11:
Bjorn's comments addressed.
> rename pcie-err.c to err.c
> removed EXPORT_SYMBOL
> made generic find_serivce function in port driver.
> removed mutex patch as no need to have mutex in pcie_do_recovery
> brough in DPC_FATAL in aer.h
> so now all the error codes (AER and DPC) are unified in aer.h
Changes since v10:
Christoph Hellwig's, David Laight's and Randy Dunlap's
comments addressed.
> renamed pci_do_recovery to pcie_do_recovery
> removed inner braces in conditional statements.
> restrctured the code in pci_wait_for_link
> EXPORT_SYMBOL_GPL
Changes since v9:
Sinan's comments addressed.
> bool active = true; unnecessary variable removed.
Changes since v8:
Fixed Kbuild errors.
Changes since v7:
Rebased the code on pci master
> https://kernel.googlesource.com/pub/scm/linux/kernel/git/helgaas/pci
Changes since v6:
Sinan's and Stefan's comments implemented.
> reordered patch 6 and 7
> cleaned up
Changes since v5:
Sinan's and Keith's comments incorporated.
> made separate patch for mutex
> unified error repotting codes into driver/pci/pci.h
> got rid of wait link active/inactive and
made generic function in driver/pci/pci.c
Changes since v4:
Bjorn's comments incorporated.
> Renamed only do_recovery.
> moved the things more locally to drivers/pci/pci.h
Changes since v3:
Bjorn's comments incorporated.
> Made separate patch renaming generic pci_err.c
> Introduce pci_err.h to contain all the error types and recovery
> removed all the dependencies on pci.h
Changes since v2:
Based on feedback from Keith:
"
When DPC is triggered due to receipt of an uncorrectable error Message,
the Requester ID from the Message is recorded in the DPC Error
Source ID register and that Message is discarded and not forwarded Upstream.
"
Removed the patch where AER checks if DPC service is active
Changes since v1:
Kbuild errors fixed:
> pci_find_dpc_dev made static
> ras_event.h updated
> pci_find_aer_service call with CONFIG check
> pci_find_dpc_service call with CONFIG check
Oza Pawandeep (9):
PCI: Unify wait for link active into generic PCI
pci-error-recovery: Add AER_FATAL handling
PCI/AER: Handle ERRR_FATAL with removal and re-enumeration of devices
PCI/AER: Rename error recovery to generic PCI naming
PCI/AER: Factor out error reporting from AER
PCI/PORTDRV: Implement generic find service
PCI/PORTDRV: Implement generic find device
PCI/DPC: Unify and plumb error handling into DPC
PCI/DPC: Disable ERR_NONFATAL and enable ERR_FATAL for DPC
Documentation/PCI/pci-error-recovery.txt | 35 ++-
drivers/pci/hotplug/pciehp_hpc.c | 20 +-
drivers/pci/pci.c | 29 +++
drivers/pci/pci.h | 4 +
drivers/pci/pcie/Makefile | 2 +-
drivers/pci/pcie/aer/aerdrv.c | 2 +
drivers/pci/pcie/aer/aerdrv.h | 30 ---
drivers/pci/pcie/aer/aerdrv_core.c | 317 +-------------------------
drivers/pci/pcie/dpc.c | 58 +++--
drivers/pci/pcie/err.c | 374 +++++++++++++++++++++++++++++++
drivers/pci/pcie/portdrv.h | 4 +
drivers/pci/pcie/portdrv_core.c | 67 ++++++
include/linux/aer.h | 1 +
include/uapi/linux/pci_regs.h | 1 +
14 files changed, 540 insertions(+), 404 deletions(-)
create mode 100644 drivers/pci/pcie/err.c
--
2.7.4
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v16 1/9] PCI: Unify wait for link active into generic PCI
2018-05-11 10:43 [PATCH v16 0/9] Address error and recovery for AER and DPC Oza Pawandeep
@ 2018-05-11 10:43 ` Oza Pawandeep
2018-05-11 10:43 ` [PATCH v16 2/9] pci-error-recovery: Add AER_FATAL handling Oza Pawandeep
` (8 subsequent siblings)
9 siblings, 0 replies; 29+ messages in thread
From: Oza Pawandeep @ 2018-05-11 10:43 UTC (permalink / raw)
To: Bjorn Helgaas, Philippe Ombredanne, Thomas Gleixner,
Greg Kroah-Hartman, Kate Stewart, linux-pci, linux-kernel,
Dongdong Liu, Keith Busch, Wei Zhang, Sinan Kaya, Timur Tabi
Cc: Oza Pawandeep
Clients such as HP, DPC are using pcie_wait_link_active(), which waits
till the link becomes active or inactive.
Made generic function and moved it to drivers/pci/pci.c
Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
Reviewed-by: Keith Busch <keith.busch@intel.com>
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 18a42f8..e0c2b8e 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -231,25 +231,11 @@ bool pciehp_check_link_active(struct controller *ctrl)
return ret;
}
-static void __pcie_wait_link_active(struct controller *ctrl, bool active)
-{
- int timeout = 1000;
-
- if (pciehp_check_link_active(ctrl) == active)
- return;
- while (timeout > 0) {
- msleep(10);
- timeout -= 10;
- if (pciehp_check_link_active(ctrl) == active)
- return;
- }
- ctrl_dbg(ctrl, "Data Link Layer Link Active not %s in 1000 msec\n",
- active ? "set" : "cleared");
-}
-
static void pcie_wait_link_active(struct controller *ctrl)
{
- __pcie_wait_link_active(ctrl, true);
+ struct pci_dev *pdev = ctrl_dev(ctrl);
+
+ pcie_wait_for_link(pdev, true);
}
static bool pci_bus_check_dev(struct pci_bus *bus, int devfn)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e597655..adfc553 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4138,6 +4138,35 @@ static int pci_pm_reset(struct pci_dev *dev, int probe)
return pci_dev_wait(dev, "PM D3->D0", PCIE_RESET_READY_POLL_MS);
}
+/**
+ * pcie_wait_for_link - Wait for link till it's active?/inactive?
+ * @pdev: Bridge device
+ * @active: waiting for active or inactive ?
+ *
+ * Use this to wait till link becomes active or inactive.
+ */
+bool pcie_wait_for_link(struct pci_dev *pdev, bool active)
+{
+ int timeout = 1000;
+ bool ret;
+ u16 lnk_status;
+
+ for (;;) {
+ pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
+ ret = !!(lnk_status & PCI_EXP_LNKSTA_DLLLA);
+ if (ret == active)
+ return true;
+ if (timeout <= 0)
+ break;
+ msleep(10);
+ timeout -= 10;
+ }
+
+ pci_info(pdev, "Data Link Layer Link Active not %s in 1000 msec\n",
+ active ? "set" : "cleared");
+
+ return false;
+}
void pci_reset_secondary_bus(struct pci_dev *dev)
{
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 023f7cf..cec9d8c 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -353,6 +353,7 @@ static inline resource_size_t pci_resource_alignment(struct pci_dev *dev,
void pci_enable_acs(struct pci_dev *dev);
+bool pcie_wait_for_link(struct pci_dev *pdev, bool active);
#ifdef CONFIG_PCIEASPM
void pcie_aspm_init_link_state(struct pci_dev *pdev);
void pcie_aspm_exit_link_state(struct pci_dev *pdev);
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 8c57d60..80ec384 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -68,19 +68,9 @@ static int dpc_wait_rp_inactive(struct dpc_dev *dpc)
static void dpc_wait_link_inactive(struct dpc_dev *dpc)
{
- unsigned long timeout = jiffies + HZ;
struct pci_dev *pdev = dpc->dev->port;
- struct device *dev = &dpc->dev->device;
- u16 lnk_status;
- pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
- while (lnk_status & PCI_EXP_LNKSTA_DLLLA &&
- !time_after(jiffies, timeout)) {
- msleep(10);
- pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
- }
- if (lnk_status & PCI_EXP_LNKSTA_DLLLA)
- dev_warn(dev, "Link state not disabled for DPC event\n");
+ pcie_wait_for_link(pdev, false);
}
static void dpc_work(struct work_struct *work)
--
2.7.4
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v16 2/9] pci-error-recovery: Add AER_FATAL handling
2018-05-11 10:43 [PATCH v16 0/9] Address error and recovery for AER and DPC Oza Pawandeep
2018-05-11 10:43 ` [PATCH v16 1/9] PCI: Unify wait for link active into generic PCI Oza Pawandeep
@ 2018-05-11 10:43 ` Oza Pawandeep
2018-05-11 10:43 ` [PATCH v16 3/9] PCI/AER: Handle ERR_FATAL with removal and re-enumeration of devices Oza Pawandeep
` (7 subsequent siblings)
9 siblings, 0 replies; 29+ messages in thread
From: Oza Pawandeep @ 2018-05-11 10:43 UTC (permalink / raw)
To: Bjorn Helgaas, Philippe Ombredanne, Thomas Gleixner,
Greg Kroah-Hartman, Kate Stewart, linux-pci, linux-kernel,
Dongdong Liu, Keith Busch, Wei Zhang, Sinan Kaya, Timur Tabi
Cc: Oza Pawandeep
It adds description on AER_FATAL error handling.
Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
diff --git a/Documentation/PCI/pci-error-recovery.txt b/Documentation/PCI/pci-error-recovery.txt
index 0b6bb3e..688b691 100644
--- a/Documentation/PCI/pci-error-recovery.txt
+++ b/Documentation/PCI/pci-error-recovery.txt
@@ -110,7 +110,7 @@ The actual steps taken by a platform to recover from a PCI error
event will be platform-dependent, but will follow the general
sequence described below.
-STEP 0: Error Event
+STEP 0: Error Event: ERR_NONFATAL
-------------------
A PCI bus error is detected by the PCI hardware. On powerpc, the slot
is isolated, in that all I/O is blocked: all reads return 0xffffffff,
@@ -228,13 +228,7 @@ proceeds to either STEP3 (Link Reset) or to STEP 5 (Resume Operations).
If any driver returned PCI_ERS_RESULT_NEED_RESET, then the platform
proceeds to STEP 4 (Slot Reset)
-STEP 3: Link Reset
-------------------
-The platform resets the link. This is a PCI-Express specific step
-and is done whenever a fatal error has been detected that can be
-"solved" by resetting the link.
-
-STEP 4: Slot Reset
+STEP 3: Slot Reset
------------------
In response to a return value of PCI_ERS_RESULT_NEED_RESET, the
@@ -320,7 +314,7 @@ Failure).
>>> However, it probably should.
-STEP 5: Resume Operations
+STEP 4: Resume Operations
-------------------------
The platform will call the resume() callback on all affected device
drivers if all drivers on the segment have returned
@@ -332,7 +326,7 @@ a result code.
At this point, if a new error happens, the platform will restart
a new error recovery sequence.
-STEP 6: Permanent Failure
+STEP 5: Permanent Failure
-------------------------
A "permanent failure" has occurred, and the platform cannot recover
the device. The platform will call error_detected() with a
@@ -355,6 +349,27 @@ errors. See the discussion in powerpc/eeh-pci-error-recovery.txt
for additional detail on real-life experience of the causes of
software errors.
+STEP 0: Error Event: ERR_FATAL
+-------------------
+PCI bus error is detected by the PCI hardware. On powerpc, the slot is
+isolated, in that all I/O is blocked: all reads return 0xffffffff, all
+writes are ignored.
+
+STEP 1: Remove devices
+--------------------
+Platform removes the devices depending on the error agent, it could be
+this port for all subordinates or upstream component (likely downstream
+port)
+
+STEP 2: Reset link
+--------------------
+The platform resets the link. This is a PCI-Express specific step and is
+done whenever a fatal error has been detected that can be "solved" by
+resetting the link.
+
+STEP 3: Re-enumerate the devices
+--------------------
+Initiates the re-enumeration.
Conclusion; General Remarks
---------------------------
--
2.7.4
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v16 3/9] PCI/AER: Handle ERR_FATAL with removal and re-enumeration of devices
2018-05-11 10:43 [PATCH v16 0/9] Address error and recovery for AER and DPC Oza Pawandeep
2018-05-11 10:43 ` [PATCH v16 1/9] PCI: Unify wait for link active into generic PCI Oza Pawandeep
2018-05-11 10:43 ` [PATCH v16 2/9] pci-error-recovery: Add AER_FATAL handling Oza Pawandeep
@ 2018-05-11 10:43 ` Oza Pawandeep
2018-05-15 23:59 ` Bjorn Helgaas
2018-05-11 10:43 ` [PATCH v16 4/9] PCI/AER: Rename error recovery to generic PCI naming Oza Pawandeep
` (6 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Oza Pawandeep @ 2018-05-11 10:43 UTC (permalink / raw)
To: Bjorn Helgaas, Philippe Ombredanne, Thomas Gleixner,
Greg Kroah-Hartman, Kate Stewart, linux-pci, linux-kernel,
Dongdong Liu, Keith Busch, Wei Zhang, Sinan Kaya, Timur Tabi
Cc: Oza Pawandeep
This patch alters the behavior of handling of ERR_FATAL, where removal
of devices is initiated, followed by reset link, followed by
re-enumeration.
So the errors are handled in a different way as follows:
ERR_NONFATAL => call driver recovery entry points
ERR_FATAL => remove and re-enumerate
please refer to Documentation/PCI/pci-error-recovery.txt for more details.
Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
Reviewed-by: Keith Busch <keith.busch@intel.com>
diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
index 0ea5acc..649dd1f 100644
--- a/drivers/pci/pcie/aer/aerdrv_core.c
+++ b/drivers/pci/pcie/aer/aerdrv_core.c
@@ -20,6 +20,7 @@
#include <linux/slab.h>
#include <linux/kfifo.h>
#include "aerdrv.h"
+#include "../../pci.h"
#define PCI_EXP_AER_FLAGS (PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVCTL_NFERE | \
PCI_EXP_DEVCTL_FERE | PCI_EXP_DEVCTL_URRE)
@@ -475,35 +476,84 @@ static pci_ers_result_t reset_link(struct pci_dev *dev)
}
/**
- * do_recovery - handle nonfatal/fatal error recovery process
+ * do_fatal_recovery - handle fatal error recovery process
+ * @dev: pointer to a pci_dev data structure of agent detecting an error
+ *
+ * Invoked when an error is fatal. Once being invoked, removes the devices
+ * benetah this AER agent, followed by reset link e.g. secondary bus reset
+ * followed by re-enumeration of devices.
+ */
+
+static void do_fatal_recovery(struct pci_dev *dev)
+{
+ struct pci_dev *udev;
+ struct pci_bus *parent;
+ struct pci_dev *pdev, *temp;
+ pci_ers_result_t result = PCI_ERS_RESULT_RECOVERED;
+ struct aer_broadcast_data result_data;
+
+ if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
+ udev = dev;
+ else
+ udev = dev->bus->self;
+
+ parent = udev->subordinate;
+ pci_lock_rescan_remove();
+ list_for_each_entry_safe_reverse(pdev, temp, &parent->devices,
+ bus_list) {
+ pci_dev_get(pdev);
+ pci_dev_set_disconnected(pdev, NULL);
+ if (pci_has_subordinate(pdev))
+ pci_walk_bus(pdev->subordinate,
+ pci_dev_set_disconnected, NULL);
+ pci_stop_and_remove_bus_device(pdev);
+ pci_dev_put(pdev);
+ }
+
+ result = reset_link(udev);
+
+ if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
+ /*
+ * If the error is reported by a bridge, we think this error
+ * is related to the downstream link of the bridge, so we
+ * do error recovery on all subordinates of the bridge instead
+ * of the bridge and clear the error status of the bridge.
+ */
+ pci_walk_bus(dev->subordinate, report_resume, &result_data);
+ pci_cleanup_aer_uncorrect_error_status(dev);
+ }
+
+ if (result == PCI_ERS_RESULT_RECOVERED) {
+ if (pcie_wait_for_link(udev, true))
+ pci_rescan_bus(udev->bus);
+ } else {
+ pci_uevent_ers(dev, PCI_ERS_RESULT_DISCONNECT);
+ pci_info(dev, "AER: Device recovery failed\n");
+ }
+
+ pci_unlock_rescan_remove();
+}
+
+/**
+ * do_nonfatal_recovery - handle nonfatal error recovery process
* @dev: pointer to a pci_dev data structure of agent detecting an error
- * @severity: error severity type
*
* Invoked when an error is nonfatal/fatal. Once being invoked, broadcast
* error detected message to all downstream drivers within a hierarchy in
* question and return the returned code.
*/
-static void do_recovery(struct pci_dev *dev, int severity)
+static void do_nonfatal_recovery(struct pci_dev *dev)
{
- pci_ers_result_t status, result = PCI_ERS_RESULT_RECOVERED;
+ pci_ers_result_t status;
enum pci_channel_state state;
- if (severity == AER_FATAL)
- state = pci_channel_io_frozen;
- else
- state = pci_channel_io_normal;
+ state = pci_channel_io_normal;
status = broadcast_error_message(dev,
state,
"error_detected",
report_error_detected);
- if (severity == AER_FATAL) {
- result = reset_link(dev);
- if (result != PCI_ERS_RESULT_RECOVERED)
- goto failed;
- }
-
if (status == PCI_ERS_RESULT_CAN_RECOVER)
status = broadcast_error_message(dev,
state,
@@ -562,8 +612,10 @@ static void handle_error_source(struct pcie_device *aerdev,
if (pos)
pci_write_config_dword(dev, pos + PCI_ERR_COR_STATUS,
info->status);
- } else
- do_recovery(dev, info->severity);
+ } else if (info->severity == AER_NONFATAL)
+ do_nonfatal_recovery(dev);
+ else if (info->severity == AER_FATAL)
+ do_fatal_recovery(dev);
}
#ifdef CONFIG_ACPI_APEI_PCIEAER
@@ -627,8 +679,10 @@ static void aer_recover_work_func(struct work_struct *work)
continue;
}
cper_print_aer(pdev, entry.severity, entry.regs);
- if (entry.severity != AER_CORRECTABLE)
- do_recovery(pdev, entry.severity);
+ if (entry.severity == AER_NONFATAL)
+ do_nonfatal_recovery(pdev);
+ else if (entry.severity == AER_FATAL)
+ do_fatal_recovery(pdev);
pci_dev_put(pdev);
}
}
--
2.7.4
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v16 4/9] PCI/AER: Rename error recovery to generic PCI naming
2018-05-11 10:43 [PATCH v16 0/9] Address error and recovery for AER and DPC Oza Pawandeep
` (2 preceding siblings ...)
2018-05-11 10:43 ` [PATCH v16 3/9] PCI/AER: Handle ERR_FATAL with removal and re-enumeration of devices Oza Pawandeep
@ 2018-05-11 10:43 ` Oza Pawandeep
2018-05-11 10:43 ` [PATCH v16 5/9] PCI/AER: Factor out error reporting from AER Oza Pawandeep
` (5 subsequent siblings)
9 siblings, 0 replies; 29+ messages in thread
From: Oza Pawandeep @ 2018-05-11 10:43 UTC (permalink / raw)
To: Bjorn Helgaas, Philippe Ombredanne, Thomas Gleixner,
Greg Kroah-Hartman, Kate Stewart, linux-pci, linux-kernel,
Dongdong Liu, Keith Busch, Wei Zhang, Sinan Kaya, Timur Tabi
Cc: Oza Pawandeep
This patch renames error recovery to generic name with pcie prefix
Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
Reviewed-by: Keith Busch <keith.busch@intel.com>
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index cec9d8c..5e8857a 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -353,6 +353,10 @@ static inline resource_size_t pci_resource_alignment(struct pci_dev *dev,
void pci_enable_acs(struct pci_dev *dev);
+/* PCI error reporting and recovery */
+void pcie_do_fatal_recovery(struct pci_dev *dev);
+void pcie_do_nonfatal_recovery(struct pci_dev *dev);
+
bool pcie_wait_for_link(struct pci_dev *pdev, bool active);
#ifdef CONFIG_PCIEASPM
void pcie_aspm_init_link_state(struct pci_dev *pdev);
diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
index 649dd1f..4444aa4 100644
--- a/drivers/pci/pcie/aer/aerdrv_core.c
+++ b/drivers/pci/pcie/aer/aerdrv_core.c
@@ -476,7 +476,7 @@ static pci_ers_result_t reset_link(struct pci_dev *dev)
}
/**
- * do_fatal_recovery - handle fatal error recovery process
+ * pcie_do_fatal_recovery - handle fatal error recovery process
* @dev: pointer to a pci_dev data structure of agent detecting an error
*
* Invoked when an error is fatal. Once being invoked, removes the devices
@@ -484,7 +484,7 @@ static pci_ers_result_t reset_link(struct pci_dev *dev)
* followed by re-enumeration of devices.
*/
-static void do_fatal_recovery(struct pci_dev *dev)
+void pcie_do_fatal_recovery(struct pci_dev *dev)
{
struct pci_dev *udev;
struct pci_bus *parent;
@@ -535,14 +535,14 @@ static void do_fatal_recovery(struct pci_dev *dev)
}
/**
- * do_nonfatal_recovery - handle nonfatal error recovery process
+ * pcie_do_nonfatal_recovery - handle nonfatal error recovery process
* @dev: pointer to a pci_dev data structure of agent detecting an error
*
* Invoked when an error is nonfatal/fatal. Once being invoked, broadcast
* error detected message to all downstream drivers within a hierarchy in
* question and return the returned code.
*/
-static void do_nonfatal_recovery(struct pci_dev *dev)
+void pcie_do_nonfatal_recovery(struct pci_dev *dev)
{
pci_ers_result_t status;
enum pci_channel_state state;
@@ -613,9 +613,9 @@ static void handle_error_source(struct pcie_device *aerdev,
pci_write_config_dword(dev, pos + PCI_ERR_COR_STATUS,
info->status);
} else if (info->severity == AER_NONFATAL)
- do_nonfatal_recovery(dev);
+ pcie_do_nonfatal_recovery(dev);
else if (info->severity == AER_FATAL)
- do_fatal_recovery(dev);
+ pcie_do_fatal_recovery(dev);
}
#ifdef CONFIG_ACPI_APEI_PCIEAER
@@ -680,9 +680,9 @@ static void aer_recover_work_func(struct work_struct *work)
}
cper_print_aer(pdev, entry.severity, entry.regs);
if (entry.severity == AER_NONFATAL)
- do_nonfatal_recovery(pdev);
+ pcie_do_nonfatal_recovery(pdev);
else if (entry.severity == AER_FATAL)
- do_fatal_recovery(pdev);
+ pcie_do_fatal_recovery(pdev);
pci_dev_put(pdev);
}
}
--
2.7.4
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v16 5/9] PCI/AER: Factor out error reporting from AER
2018-05-11 10:43 [PATCH v16 0/9] Address error and recovery for AER and DPC Oza Pawandeep
` (3 preceding siblings ...)
2018-05-11 10:43 ` [PATCH v16 4/9] PCI/AER: Rename error recovery to generic PCI naming Oza Pawandeep
@ 2018-05-11 10:43 ` Oza Pawandeep
2018-05-11 12:58 ` Lukas Wunner
2018-05-16 0:06 ` Bjorn Helgaas
2018-05-11 10:43 ` [PATCH v16 6/9] PCI/PORTDRV: Implement generic find service Oza Pawandeep
` (4 subsequent siblings)
9 siblings, 2 replies; 29+ messages in thread
From: Oza Pawandeep @ 2018-05-11 10:43 UTC (permalink / raw)
To: Bjorn Helgaas, Philippe Ombredanne, Thomas Gleixner,
Greg Kroah-Hartman, Kate Stewart, linux-pci, linux-kernel,
Dongdong Liu, Keith Busch, Wei Zhang, Sinan Kaya, Timur Tabi
Cc: Oza Pawandeep
This patch factors out error reporting callbacks, which are currently
tightly coupled with AER.
DPC should be able to register callbacks and attempt recovery when DPC
trigger event occurs.
Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
index 800e1d4..03f4e0b 100644
--- a/drivers/pci/pcie/Makefile
+++ b/drivers/pci/pcie/Makefile
@@ -2,7 +2,7 @@
#
# Makefile for PCI Express features and port driver
-pcieportdrv-y := portdrv_core.o portdrv_pci.o
+pcieportdrv-y := portdrv_core.o portdrv_pci.o err.o
obj-$(CONFIG_PCIEPORTBUS) += pcieportdrv.o
diff --git a/drivers/pci/pcie/aer/aerdrv.h b/drivers/pci/pcie/aer/aerdrv.h
index 08b4584..b4c9506 100644
--- a/drivers/pci/pcie/aer/aerdrv.h
+++ b/drivers/pci/pcie/aer/aerdrv.h
@@ -76,36 +76,6 @@ struct aer_rpc {
*/
};
-struct aer_broadcast_data {
- enum pci_channel_state state;
- enum pci_ers_result result;
-};
-
-static inline pci_ers_result_t merge_result(enum pci_ers_result orig,
- enum pci_ers_result new)
-{
- if (new == PCI_ERS_RESULT_NO_AER_DRIVER)
- return PCI_ERS_RESULT_NO_AER_DRIVER;
-
- if (new == PCI_ERS_RESULT_NONE)
- return orig;
-
- switch (orig) {
- case PCI_ERS_RESULT_CAN_RECOVER:
- case PCI_ERS_RESULT_RECOVERED:
- orig = new;
- break;
- case PCI_ERS_RESULT_DISCONNECT:
- if (new == PCI_ERS_RESULT_NEED_RESET)
- orig = PCI_ERS_RESULT_NEED_RESET;
- break;
- default:
- break;
- }
-
- return orig;
-}
-
extern struct bus_type pcie_port_bus_type;
void aer_isr(struct work_struct *work);
void aer_print_error(struct pci_dev *dev, struct aer_err_info *info);
diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
index 4444aa4..4fa1ee4 100644
--- a/drivers/pci/pcie/aer/aerdrv_core.c
+++ b/drivers/pci/pcie/aer/aerdrv_core.c
@@ -228,191 +228,6 @@ static bool find_source_device(struct pci_dev *parent,
return true;
}
-static int report_error_detected(struct pci_dev *dev, void *data)
-{
- pci_ers_result_t vote;
- const struct pci_error_handlers *err_handler;
- struct aer_broadcast_data *result_data;
- result_data = (struct aer_broadcast_data *) data;
-
- device_lock(&dev->dev);
- dev->error_state = result_data->state;
-
- if (!dev->driver ||
- !dev->driver->err_handler ||
- !dev->driver->err_handler->error_detected) {
- if (result_data->state == pci_channel_io_frozen &&
- dev->hdr_type != PCI_HEADER_TYPE_BRIDGE) {
- /*
- * In case of fatal recovery, if one of down-
- * stream device has no driver. We might be
- * unable to recover because a later insmod
- * of a driver for this device is unaware of
- * its hw state.
- */
- pci_printk(KERN_DEBUG, dev, "device has %s\n",
- dev->driver ?
- "no AER-aware driver" : "no driver");
- }
-
- /*
- * If there's any device in the subtree that does not
- * have an error_detected callback, returning
- * PCI_ERS_RESULT_NO_AER_DRIVER prevents calling of
- * the subsequent mmio_enabled/slot_reset/resume
- * callbacks of "any" device in the subtree. All the
- * devices in the subtree are left in the error state
- * without recovery.
- */
-
- if (dev->hdr_type != PCI_HEADER_TYPE_BRIDGE)
- vote = PCI_ERS_RESULT_NO_AER_DRIVER;
- else
- vote = PCI_ERS_RESULT_NONE;
- } else {
- err_handler = dev->driver->err_handler;
- vote = err_handler->error_detected(dev, result_data->state);
- pci_uevent_ers(dev, PCI_ERS_RESULT_NONE);
- }
-
- result_data->result = merge_result(result_data->result, vote);
- device_unlock(&dev->dev);
- return 0;
-}
-
-static int report_mmio_enabled(struct pci_dev *dev, void *data)
-{
- pci_ers_result_t vote;
- const struct pci_error_handlers *err_handler;
- struct aer_broadcast_data *result_data;
- result_data = (struct aer_broadcast_data *) data;
-
- device_lock(&dev->dev);
- if (!dev->driver ||
- !dev->driver->err_handler ||
- !dev->driver->err_handler->mmio_enabled)
- goto out;
-
- err_handler = dev->driver->err_handler;
- vote = err_handler->mmio_enabled(dev);
- result_data->result = merge_result(result_data->result, vote);
-out:
- device_unlock(&dev->dev);
- return 0;
-}
-
-static int report_slot_reset(struct pci_dev *dev, void *data)
-{
- pci_ers_result_t vote;
- const struct pci_error_handlers *err_handler;
- struct aer_broadcast_data *result_data;
- result_data = (struct aer_broadcast_data *) data;
-
- device_lock(&dev->dev);
- if (!dev->driver ||
- !dev->driver->err_handler ||
- !dev->driver->err_handler->slot_reset)
- goto out;
-
- err_handler = dev->driver->err_handler;
- vote = err_handler->slot_reset(dev);
- result_data->result = merge_result(result_data->result, vote);
-out:
- device_unlock(&dev->dev);
- return 0;
-}
-
-static int report_resume(struct pci_dev *dev, void *data)
-{
- const struct pci_error_handlers *err_handler;
-
- device_lock(&dev->dev);
- dev->error_state = pci_channel_io_normal;
-
- if (!dev->driver ||
- !dev->driver->err_handler ||
- !dev->driver->err_handler->resume)
- goto out;
-
- err_handler = dev->driver->err_handler;
- err_handler->resume(dev);
- pci_uevent_ers(dev, PCI_ERS_RESULT_RECOVERED);
-out:
- device_unlock(&dev->dev);
- return 0;
-}
-
-/**
- * broadcast_error_message - handle message broadcast to downstream drivers
- * @dev: pointer to from where in a hierarchy message is broadcasted down
- * @state: error state
- * @error_mesg: message to print
- * @cb: callback to be broadcasted
- *
- * Invoked during error recovery process. Once being invoked, the content
- * of error severity will be broadcasted to all downstream drivers in a
- * hierarchy in question.
- */
-static pci_ers_result_t broadcast_error_message(struct pci_dev *dev,
- enum pci_channel_state state,
- char *error_mesg,
- int (*cb)(struct pci_dev *, void *))
-{
- struct aer_broadcast_data result_data;
-
- pci_printk(KERN_DEBUG, dev, "broadcast %s message\n", error_mesg);
- result_data.state = state;
- if (cb == report_error_detected)
- result_data.result = PCI_ERS_RESULT_CAN_RECOVER;
- else
- result_data.result = PCI_ERS_RESULT_RECOVERED;
-
- if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
- /*
- * If the error is reported by a bridge, we think this error
- * is related to the downstream link of the bridge, so we
- * do error recovery on all subordinates of the bridge instead
- * of the bridge and clear the error status of the bridge.
- */
- if (cb == report_error_detected)
- dev->error_state = state;
- pci_walk_bus(dev->subordinate, cb, &result_data);
- if (cb == report_resume) {
- pci_cleanup_aer_uncorrect_error_status(dev);
- dev->error_state = pci_channel_io_normal;
- }
- } else {
- /*
- * If the error is reported by an end point, we think this
- * error is related to the upstream link of the end point.
- */
- if (state == pci_channel_io_normal)
- /*
- * the error is non fatal so the bus is ok, just invoke
- * the callback for the function that logged the error.
- */
- cb(dev, &result_data);
- else
- pci_walk_bus(dev->bus, cb, &result_data);
- }
-
- return result_data.result;
-}
-
-/**
- * default_reset_link - default reset function
- * @dev: pointer to pci_dev data structure
- *
- * Invoked when performing link reset on a Downstream Port or a
- * Root Port with no aer driver.
- */
-static pci_ers_result_t default_reset_link(struct pci_dev *dev)
-{
- pci_reset_bridge_secondary_bus(dev);
- pci_printk(KERN_DEBUG, dev, "downstream link has been reset\n");
- return PCI_ERS_RESULT_RECOVERED;
-}
-
static int find_aer_service_iter(struct device *device, void *data)
{
struct pcie_port_service_driver *service_driver, **drv;
@@ -430,7 +245,7 @@ static int find_aer_service_iter(struct device *device, void *data)
return 0;
}
-static struct pcie_port_service_driver *find_aer_service(struct pci_dev *dev)
+struct pcie_port_service_driver *find_aer_service(struct pci_dev *dev)
{
struct pcie_port_service_driver *drv = NULL;
@@ -439,156 +254,6 @@ static struct pcie_port_service_driver *find_aer_service(struct pci_dev *dev)
return drv;
}
-static pci_ers_result_t reset_link(struct pci_dev *dev)
-{
- struct pci_dev *udev;
- pci_ers_result_t status;
- struct pcie_port_service_driver *driver;
-
- if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
- /* Reset this port for all subordinates */
- udev = dev;
- } else {
- /* Reset the upstream component (likely downstream port) */
- udev = dev->bus->self;
- }
-
- /* Use the aer driver of the component firstly */
- driver = find_aer_service(udev);
-
- if (driver && driver->reset_link) {
- status = driver->reset_link(udev);
- } else if (udev->has_secondary_link) {
- status = default_reset_link(udev);
- } else {
- pci_printk(KERN_DEBUG, dev, "no link-reset support at upstream device %s\n",
- pci_name(udev));
- return PCI_ERS_RESULT_DISCONNECT;
- }
-
- if (status != PCI_ERS_RESULT_RECOVERED) {
- pci_printk(KERN_DEBUG, dev, "link reset at upstream device %s failed\n",
- pci_name(udev));
- return PCI_ERS_RESULT_DISCONNECT;
- }
-
- return status;
-}
-
-/**
- * pcie_do_fatal_recovery - handle fatal error recovery process
- * @dev: pointer to a pci_dev data structure of agent detecting an error
- *
- * Invoked when an error is fatal. Once being invoked, removes the devices
- * benetah this AER agent, followed by reset link e.g. secondary bus reset
- * followed by re-enumeration of devices.
- */
-
-void pcie_do_fatal_recovery(struct pci_dev *dev)
-{
- struct pci_dev *udev;
- struct pci_bus *parent;
- struct pci_dev *pdev, *temp;
- pci_ers_result_t result = PCI_ERS_RESULT_RECOVERED;
- struct aer_broadcast_data result_data;
-
- if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
- udev = dev;
- else
- udev = dev->bus->self;
-
- parent = udev->subordinate;
- pci_lock_rescan_remove();
- list_for_each_entry_safe_reverse(pdev, temp, &parent->devices,
- bus_list) {
- pci_dev_get(pdev);
- pci_dev_set_disconnected(pdev, NULL);
- if (pci_has_subordinate(pdev))
- pci_walk_bus(pdev->subordinate,
- pci_dev_set_disconnected, NULL);
- pci_stop_and_remove_bus_device(pdev);
- pci_dev_put(pdev);
- }
-
- result = reset_link(udev);
-
- if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
- /*
- * If the error is reported by a bridge, we think this error
- * is related to the downstream link of the bridge, so we
- * do error recovery on all subordinates of the bridge instead
- * of the bridge and clear the error status of the bridge.
- */
- pci_walk_bus(dev->subordinate, report_resume, &result_data);
- pci_cleanup_aer_uncorrect_error_status(dev);
- }
-
- if (result == PCI_ERS_RESULT_RECOVERED) {
- if (pcie_wait_for_link(udev, true))
- pci_rescan_bus(udev->bus);
- } else {
- pci_uevent_ers(dev, PCI_ERS_RESULT_DISCONNECT);
- pci_info(dev, "AER: Device recovery failed\n");
- }
-
- pci_unlock_rescan_remove();
-}
-
-/**
- * pcie_do_nonfatal_recovery - handle nonfatal error recovery process
- * @dev: pointer to a pci_dev data structure of agent detecting an error
- *
- * Invoked when an error is nonfatal/fatal. Once being invoked, broadcast
- * error detected message to all downstream drivers within a hierarchy in
- * question and return the returned code.
- */
-void pcie_do_nonfatal_recovery(struct pci_dev *dev)
-{
- pci_ers_result_t status;
- enum pci_channel_state state;
-
- state = pci_channel_io_normal;
-
- status = broadcast_error_message(dev,
- state,
- "error_detected",
- report_error_detected);
-
- if (status == PCI_ERS_RESULT_CAN_RECOVER)
- status = broadcast_error_message(dev,
- state,
- "mmio_enabled",
- report_mmio_enabled);
-
- if (status == PCI_ERS_RESULT_NEED_RESET) {
- /*
- * TODO: Should call platform-specific
- * functions to reset slot before calling
- * drivers' slot_reset callbacks?
- */
- status = broadcast_error_message(dev,
- state,
- "slot_reset",
- report_slot_reset);
- }
-
- if (status != PCI_ERS_RESULT_RECOVERED)
- goto failed;
-
- broadcast_error_message(dev,
- state,
- "resume",
- report_resume);
-
- pci_info(dev, "AER: Device recovery successful\n");
- return;
-
-failed:
- pci_uevent_ers(dev, PCI_ERS_RESULT_DISCONNECT);
- /* TODO: Should kernel panic here? */
- pci_info(dev, "AER: Device recovery failed\n");
-}
-
/**
* handle_error_source - handle logging error into an event log
* @aerdev: pointer to pcie_device data structure of the root port
diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
new file mode 100644
index 0000000..c4ded88
--- /dev/null
+++ b/drivers/pci/pcie/err.c
@@ -0,0 +1,400 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * This file implements the error recovery as a core part of PCIe error
+ * reporting. When a PCIe error is delivered, an error message will be
+ * collected and printed to console, then, an error recovery procedure
+ * will be executed by following the PCI error recovery rules.
+ *
+ * Copyright (C) 2006 Intel Corp.
+ * Tom Long Nguyen (tom.l.nguyen@intel.com)
+ * Zhang Yanmin (yanmin.zhang@intel.com)
+ *
+ */
+
+#include <linux/pci.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/aer.h>
+#include "portdrv.h"
+#include "../pci.h"
+
+struct aer_broadcast_data {
+ enum pci_channel_state state;
+ enum pci_ers_result result;
+};
+
+static pci_ers_result_t merge_result(enum pci_ers_result orig,
+ enum pci_ers_result new)
+{
+ if (new == PCI_ERS_RESULT_NO_AER_DRIVER)
+ return PCI_ERS_RESULT_NO_AER_DRIVER;
+
+ if (new == PCI_ERS_RESULT_NONE)
+ return orig;
+
+ switch (orig) {
+ case PCI_ERS_RESULT_CAN_RECOVER:
+ case PCI_ERS_RESULT_RECOVERED:
+ orig = new;
+ break;
+ case PCI_ERS_RESULT_DISCONNECT:
+ if (new == PCI_ERS_RESULT_NEED_RESET)
+ orig = PCI_ERS_RESULT_NEED_RESET;
+ break;
+ default:
+ break;
+ }
+
+ return orig;
+}
+
+static int report_error_detected(struct pci_dev *dev, void *data)
+{
+ pci_ers_result_t vote;
+ const struct pci_error_handlers *err_handler;
+ struct aer_broadcast_data *result_data;
+
+ result_data = (struct aer_broadcast_data *) data;
+
+ device_lock(&dev->dev);
+ dev->error_state = result_data->state;
+
+ if (!dev->driver ||
+ !dev->driver->err_handler ||
+ !dev->driver->err_handler->error_detected) {
+ if (result_data->state == pci_channel_io_frozen &&
+ dev->hdr_type != PCI_HEADER_TYPE_BRIDGE) {
+ /*
+ * In case of fatal recovery, if one of down-
+ * stream device has no driver. We might be
+ * unable to recover because a later insmod
+ * of a driver for this device is unaware of
+ * its hw state.
+ */
+ pci_printk(KERN_DEBUG, dev, "device has %s\n",
+ dev->driver ?
+ "no AER-aware driver" : "no driver");
+ }
+
+ /*
+ * If there's any device in the subtree that does not
+ * have an error_detected callback, returning
+ * PCI_ERS_RESULT_NO_AER_DRIVER prevents calling of
+ * the subsequent mmio_enabled/slot_reset/resume
+ * callbacks of "any" device in the subtree. All the
+ * devices in the subtree are left in the error state
+ * without recovery.
+ */
+
+ if (dev->hdr_type != PCI_HEADER_TYPE_BRIDGE)
+ vote = PCI_ERS_RESULT_NO_AER_DRIVER;
+ else
+ vote = PCI_ERS_RESULT_NONE;
+ } else {
+ err_handler = dev->driver->err_handler;
+ vote = err_handler->error_detected(dev, result_data->state);
+#if defined(CONFIG_PCIEAER)
+ pci_uevent_ers(dev, PCI_ERS_RESULT_NONE);
+#endif
+ }
+
+ result_data->result = merge_result(result_data->result, vote);
+ device_unlock(&dev->dev);
+ return 0;
+}
+
+static int report_mmio_enabled(struct pci_dev *dev, void *data)
+{
+ pci_ers_result_t vote;
+ const struct pci_error_handlers *err_handler;
+ struct aer_broadcast_data *result_data;
+
+ result_data = (struct aer_broadcast_data *) data;
+
+ device_lock(&dev->dev);
+ if (!dev->driver ||
+ !dev->driver->err_handler ||
+ !dev->driver->err_handler->mmio_enabled)
+ goto out;
+
+ err_handler = dev->driver->err_handler;
+ vote = err_handler->mmio_enabled(dev);
+ result_data->result = merge_result(result_data->result, vote);
+out:
+ device_unlock(&dev->dev);
+ return 0;
+}
+
+static int report_slot_reset(struct pci_dev *dev, void *data)
+{
+ pci_ers_result_t vote;
+ const struct pci_error_handlers *err_handler;
+ struct aer_broadcast_data *result_data;
+
+ result_data = (struct aer_broadcast_data *) data;
+
+ device_lock(&dev->dev);
+ if (!dev->driver ||
+ !dev->driver->err_handler ||
+ !dev->driver->err_handler->slot_reset)
+ goto out;
+
+ err_handler = dev->driver->err_handler;
+ vote = err_handler->slot_reset(dev);
+ result_data->result = merge_result(result_data->result, vote);
+out:
+ device_unlock(&dev->dev);
+ return 0;
+}
+
+static int report_resume(struct pci_dev *dev, void *data)
+{
+ const struct pci_error_handlers *err_handler;
+
+ device_lock(&dev->dev);
+ dev->error_state = pci_channel_io_normal;
+
+ if (!dev->driver ||
+ !dev->driver->err_handler ||
+ !dev->driver->err_handler->resume)
+ goto out;
+
+ err_handler = dev->driver->err_handler;
+ err_handler->resume(dev);
+#if defined(CONFIG_PCIEAER)
+ pci_uevent_ers(dev, PCI_ERS_RESULT_RECOVERED);
+#endif
+out:
+ device_unlock(&dev->dev);
+ return 0;
+}
+
+/**
+ * default_reset_link - default reset function
+ * @dev: pointer to pci_dev data structure
+ *
+ * Invoked when performing link reset on a Downstream Port or a
+ * Root Port with no aer driver.
+ */
+static pci_ers_result_t default_reset_link(struct pci_dev *dev)
+{
+ pci_reset_bridge_secondary_bus(dev);
+ pci_printk(KERN_DEBUG, dev, "downstream link has been reset\n");
+ return PCI_ERS_RESULT_RECOVERED;
+}
+
+static pci_ers_result_t reset_link(struct pci_dev *dev)
+{
+ struct pci_dev *udev;
+ pci_ers_result_t status;
+ struct pcie_port_service_driver *driver;
+
+ if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
+ /* Reset this port for all subordinates */
+ udev = dev;
+ } else {
+ /* Reset the upstream component (likely downstream port) */
+ udev = dev->bus->self;
+ }
+
+#if IS_ENABLED(CONFIG_PCIEAER)
+ /* Use the aer driver of the component firstly */
+ driver = find_aer_service(udev);
+#endif
+
+ if (driver && driver->reset_link) {
+ status = driver->reset_link(udev);
+ } else if (udev->has_secondary_link) {
+ status = default_reset_link(udev);
+ } else {
+ pci_printk(KERN_DEBUG, dev, "no link-reset support at upstream device %s\n",
+ pci_name(udev));
+ return PCI_ERS_RESULT_DISCONNECT;
+ }
+
+ if (status != PCI_ERS_RESULT_RECOVERED) {
+ pci_printk(KERN_DEBUG, dev, "link reset at upstream device %s failed\n",
+ pci_name(udev));
+ return PCI_ERS_RESULT_DISCONNECT;
+ }
+
+ return status;
+}
+
+/**
+ * broadcast_error_message - handle message broadcast to downstream drivers
+ * @dev: pointer to from where in a hierarchy message is broadcasted down
+ * @state: error state
+ * @error_mesg: message to print
+ * @cb: callback to be broadcasted
+ *
+ * Invoked during error recovery process. Once being invoked, the content
+ * of error severity will be broadcasted to all downstream drivers in a
+ * hierarchy in question.
+ */
+static pci_ers_result_t broadcast_error_message(struct pci_dev *dev,
+ enum pci_channel_state state,
+ char *error_mesg,
+ int (*cb)(struct pci_dev *, void *))
+{
+ struct aer_broadcast_data result_data;
+
+ pci_printk(KERN_DEBUG, dev, "broadcast %s message\n", error_mesg);
+ result_data.state = state;
+ if (cb == report_error_detected)
+ result_data.result = PCI_ERS_RESULT_CAN_RECOVER;
+ else
+ result_data.result = PCI_ERS_RESULT_RECOVERED;
+
+ if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
+ /*
+ * If the error is reported by a bridge, we think this error
+ * is related to the downstream link of the bridge, so we
+ * do error recovery on all subordinates of the bridge instead
+ * of the bridge and clear the error status of the bridge.
+ */
+ if (cb == report_error_detected)
+ dev->error_state = state;
+ pci_walk_bus(dev->subordinate, cb, &result_data);
+ if (cb == report_resume) {
+ pci_cleanup_aer_uncorrect_error_status(dev);
+ dev->error_state = pci_channel_io_normal;
+ }
+ } else {
+ /*
+ * If the error is reported by an end point, we think this
+ * error is related to the upstream link of the end point.
+ */
+ if (state == pci_channel_io_normal)
+ /*
+ * the error is non fatal so the bus is ok, just invoke
+ * the callback for the function that logged the error.
+ */
+ cb(dev, &result_data);
+ else
+ pci_walk_bus(dev->bus, cb, &result_data);
+ }
+
+ return result_data.result;
+}
+
+/**
+ * pcie_do_fatal_recovery - handle fatal error recovery process
+ * @dev: pointer to a pci_dev data structure of agent detecting an error
+ *
+ * Invoked when an error is fatal. Once being invoked, removes the devices
+ * benetah this AER agent, followed by reset link e.g. secondary bus reset
+ * followed by re-enumeration of devices.
+ */
+
+void pcie_do_fatal_recovery(struct pci_dev *dev)
+{
+ struct pci_dev *udev;
+ struct pci_bus *parent;
+ struct pci_dev *pdev, *temp;
+ pci_ers_result_t result = PCI_ERS_RESULT_RECOVERED;
+ struct aer_broadcast_data result_data;
+
+ if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
+ udev = dev;
+ else
+ udev = dev->bus->self;
+
+ parent = udev->subordinate;
+ pci_lock_rescan_remove();
+ list_for_each_entry_safe_reverse(pdev, temp, &parent->devices,
+ bus_list) {
+ pci_dev_get(pdev);
+ pci_dev_set_disconnected(pdev, NULL);
+ if (pci_has_subordinate(pdev))
+ pci_walk_bus(pdev->subordinate,
+ pci_dev_set_disconnected, NULL);
+ pci_stop_and_remove_bus_device(pdev);
+ pci_dev_put(pdev);
+ }
+
+ result = reset_link(udev);
+
+ if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
+ /*
+ * If the error is reported by a bridge, we think this error
+ * is related to the downstream link of the bridge, so we
+ * do error recovery on all subordinates of the bridge instead
+ * of the bridge and clear the error status of the bridge.
+ */
+ pci_walk_bus(dev->subordinate, report_resume, &result_data);
+ pci_cleanup_aer_uncorrect_error_status(dev);
+ }
+
+ if (result == PCI_ERS_RESULT_RECOVERED) {
+ if (pcie_wait_for_link(udev, true))
+ pci_rescan_bus(udev->bus);
+ pci_info(dev, "Device recovery successful\n");
+ } else {
+#if defined(CONFIG_PCIEAER)
+ pci_uevent_ers(dev, PCI_ERS_RESULT_DISCONNECT);
+#endif
+ pci_info(dev, "Device recovery failed\n");
+ }
+
+ pci_unlock_rescan_remove();
+}
+
+/**
+ * pcie_do_nonfatal_recovery - handle nonfatal error recovery process
+ * @dev: pointer to a pci_dev data structure of agent detecting an error
+ *
+ * Invoked when an error is nonfatal/fatal. Once being invoked, broadcast
+ * error detected message to all downstream drivers within a hierarchy in
+ * question and return the returned code.
+ */
+void pcie_do_nonfatal_recovery(struct pci_dev *dev)
+{
+ pci_ers_result_t status;
+ enum pci_channel_state state;
+
+ state = pci_channel_io_normal;
+
+ status = broadcast_error_message(dev,
+ state,
+ "error_detected",
+ report_error_detected);
+
+ if (status == PCI_ERS_RESULT_CAN_RECOVER)
+ status = broadcast_error_message(dev,
+ state,
+ "mmio_enabled",
+ report_mmio_enabled);
+
+ if (status == PCI_ERS_RESULT_NEED_RESET) {
+ /*
+ * TODO: Should call platform-specific
+ * functions to reset slot before calling
+ * drivers' slot_reset callbacks?
+ */
+ status = broadcast_error_message(dev,
+ state,
+ "slot_reset",
+ report_slot_reset);
+ }
+
+ if (status != PCI_ERS_RESULT_RECOVERED)
+ goto failed;
+
+ broadcast_error_message(dev,
+ state,
+ "resume",
+ report_resume);
+
+ pci_info(dev, "AER: Device recovery successful\n");
+ return;
+
+failed:
+#if defined(CONFIG_PCIEAER)
+ pci_uevent_ers(dev, PCI_ERS_RESULT_DISCONNECT);
+#endif
+ /* TODO: Should kernel panic here? */
+ pci_info(dev, "AER: Device recovery failed\n");
+}
diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h
index d0c6783..47c9824 100644
--- a/drivers/pci/pcie/portdrv.h
+++ b/drivers/pci/pcie/portdrv.h
@@ -112,4 +112,5 @@ static inline bool pcie_pme_no_msi(void) { return false; }
static inline void pcie_pme_interrupt_enable(struct pci_dev *dev, bool en) {}
#endif /* !CONFIG_PCIE_PME */
+struct pcie_port_service_driver *find_aer_service(struct pci_dev *dev);
#endif /* _PORTDRV_H_ */
--
2.7.4
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v16 6/9] PCI/PORTDRV: Implement generic find service
2018-05-11 10:43 [PATCH v16 0/9] Address error and recovery for AER and DPC Oza Pawandeep
` (4 preceding siblings ...)
2018-05-11 10:43 ` [PATCH v16 5/9] PCI/AER: Factor out error reporting from AER Oza Pawandeep
@ 2018-05-11 10:43 ` Oza Pawandeep
2018-05-11 10:43 ` [PATCH v16 7/9] PCI/PORTDRV: Implement generic find device Oza Pawandeep
` (3 subsequent siblings)
9 siblings, 0 replies; 29+ messages in thread
From: Oza Pawandeep @ 2018-05-11 10:43 UTC (permalink / raw)
To: Bjorn Helgaas, Philippe Ombredanne, Thomas Gleixner,
Greg Kroah-Hartman, Kate Stewart, linux-pci, linux-kernel,
Dongdong Liu, Keith Busch, Wei Zhang, Sinan Kaya, Timur Tabi
Cc: Oza Pawandeep
This patch implements generic pcie_port_find_service() routine.
Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
Reviewed-by: Keith Busch <keith.busch@intel.com>
diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
index 4fa1ee4..fdfc474 100644
--- a/drivers/pci/pcie/aer/aerdrv_core.c
+++ b/drivers/pci/pcie/aer/aerdrv_core.c
@@ -228,32 +228,6 @@ static bool find_source_device(struct pci_dev *parent,
return true;
}
-static int find_aer_service_iter(struct device *device, void *data)
-{
- struct pcie_port_service_driver *service_driver, **drv;
-
- drv = (struct pcie_port_service_driver **) data;
-
- if (device->bus == &pcie_port_bus_type && device->driver) {
- service_driver = to_service_driver(device->driver);
- if (service_driver->service == PCIE_PORT_SERVICE_AER) {
- *drv = service_driver;
- return 1;
- }
- }
-
- return 0;
-}
-
-struct pcie_port_service_driver *find_aer_service(struct pci_dev *dev)
-{
- struct pcie_port_service_driver *drv = NULL;
-
- device_for_each_child(&dev->dev, &drv, find_aer_service_iter);
-
- return drv;
-}
-
/**
* handle_error_source - handle logging error into an event log
* @aerdev: pointer to pcie_device data structure of the root port
diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index c4ded88..33a16b1 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -199,10 +199,8 @@ static pci_ers_result_t reset_link(struct pci_dev *dev)
udev = dev->bus->self;
}
-#if IS_ENABLED(CONFIG_PCIEAER)
/* Use the aer driver of the component firstly */
- driver = find_aer_service(udev);
-#endif
+ driver = pcie_port_find_service(udev, PCIE_PORT_SERVICE_AER);
if (driver && driver->reset_link) {
status = driver->reset_link(udev);
diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h
index 47c9824..ba6c963 100644
--- a/drivers/pci/pcie/portdrv.h
+++ b/drivers/pci/pcie/portdrv.h
@@ -112,5 +112,6 @@ static inline bool pcie_pme_no_msi(void) { return false; }
static inline void pcie_pme_interrupt_enable(struct pci_dev *dev, bool en) {}
#endif /* !CONFIG_PCIE_PME */
-struct pcie_port_service_driver *find_aer_service(struct pci_dev *dev);
+struct pcie_port_service_driver *pcie_port_find_service(struct pci_dev *dev,
+ u32 service);
#endif /* _PORTDRV_H_ */
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index c9c0663..d843055 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -18,6 +18,10 @@
#include "../pci.h"
#include "portdrv.h"
+struct portdrv_service_data {
+ struct pcie_port_service_driver *drv;
+ u32 service;
+};
/**
* release_pcie_device - free PCI Express port service device structure
@@ -398,6 +402,46 @@ static int remove_iter(struct device *dev, void *data)
return 0;
}
+static int find_service_iter(struct device *device, void *data)
+{
+ struct pcie_port_service_driver *service_driver;
+ struct portdrv_service_data *pdrvs;
+ u32 service;
+
+ pdrvs = (struct portdrv_service_data *) data;
+ service = pdrvs->service;
+
+ if (device->bus == &pcie_port_bus_type && device->driver) {
+ service_driver = to_service_driver(device->driver);
+ if (service_driver->service == service) {
+ pdrvs->drv = service_driver;
+ return 1;
+ }
+ }
+
+ return 0;
+}
+/**
+ * pcie_port_find_service - find the service driver
+ * @dev: PCI Express port the service devices associated with
+ * @service: Service to find
+ *
+ * Find PCI Express port service driver associated with given service
+ */
+struct pcie_port_service_driver *pcie_port_find_service(struct pci_dev *dev,
+ u32 service)
+{
+ struct pcie_port_service_driver *drv;
+ struct portdrv_service_data pdrvs;
+
+ pdrvs.drv = NULL;
+ pdrvs.service = service;
+ device_for_each_child(&dev->dev, &pdrvs, find_service_iter);
+
+ drv = pdrvs.drv;
+ return drv;
+}
+
/**
* pcie_port_device_remove - unregister PCI Express port service devices
* @dev: PCI Express port the service devices to unregister are associated with
--
2.7.4
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v16 7/9] PCI/PORTDRV: Implement generic find device
2018-05-11 10:43 [PATCH v16 0/9] Address error and recovery for AER and DPC Oza Pawandeep
` (5 preceding siblings ...)
2018-05-11 10:43 ` [PATCH v16 6/9] PCI/PORTDRV: Implement generic find service Oza Pawandeep
@ 2018-05-11 10:43 ` Oza Pawandeep
2018-05-11 10:43 ` [PATCH v16 8/9] PCI/DPC: Unify and plumb error handling into DPC Oza Pawandeep
` (2 subsequent siblings)
9 siblings, 0 replies; 29+ messages in thread
From: Oza Pawandeep @ 2018-05-11 10:43 UTC (permalink / raw)
To: Bjorn Helgaas, Philippe Ombredanne, Thomas Gleixner,
Greg Kroah-Hartman, Kate Stewart, linux-pci, linux-kernel,
Dongdong Liu, Keith Busch, Wei Zhang, Sinan Kaya, Timur Tabi
Cc: Oza Pawandeep
This patch implements generic pcie_port_find_device() routine.
Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
Reviewed-by: Keith Busch <keith.busch@intel.com>
diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h
index ba6c963..896608a 100644
--- a/drivers/pci/pcie/portdrv.h
+++ b/drivers/pci/pcie/portdrv.h
@@ -114,4 +114,6 @@ static inline void pcie_pme_interrupt_enable(struct pci_dev *dev, bool en) {}
struct pcie_port_service_driver *pcie_port_find_service(struct pci_dev *dev,
u32 service);
+struct device *pcie_port_find_device(struct pci_dev *dev,
+ u32 service);
#endif /* _PORTDRV_H_ */
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index d843055..bc2c337 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -20,6 +20,7 @@
#include "portdrv.h"
struct portdrv_service_data {
struct pcie_port_service_driver *drv;
+ struct device *dev;
u32 service;
};
@@ -415,6 +416,7 @@ static int find_service_iter(struct device *device, void *data)
service_driver = to_service_driver(device->driver);
if (service_driver->service == service) {
pdrvs->drv = service_driver;
+ pdrvs->dev = device;
return 1;
}
}
@@ -443,6 +445,27 @@ struct pcie_port_service_driver *pcie_port_find_service(struct pci_dev *dev,
}
/**
+ * pcie_port_find_device - find the struct device
+ * @dev: PCI Express port the service devices associated with
+ * @service: For the service to find
+ *
+ * Find PCI Express port service driver associated with given service
+ */
+struct device *pcie_port_find_device(struct pci_dev *dev,
+ u32 service)
+{
+ struct device *device;
+ struct portdrv_service_data pdrvs;
+
+ pdrvs.dev = NULL;
+ pdrvs.service = service;
+ device_for_each_child(&dev->dev, &pdrvs, find_service_iter);
+
+ device = pdrvs.dev;
+ return device;
+}
+
+/**
* pcie_port_device_remove - unregister PCI Express port service devices
* @dev: PCI Express port the service devices to unregister are associated with
*
--
2.7.4
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v16 8/9] PCI/DPC: Unify and plumb error handling into DPC
2018-05-11 10:43 [PATCH v16 0/9] Address error and recovery for AER and DPC Oza Pawandeep
` (6 preceding siblings ...)
2018-05-11 10:43 ` [PATCH v16 7/9] PCI/PORTDRV: Implement generic find device Oza Pawandeep
@ 2018-05-11 10:43 ` Oza Pawandeep
2018-05-11 11:52 ` poza
2018-05-11 10:43 ` [PATCH v16 9/9] PCI/DPC: Disable ERR_NONFATAL and enable ERR_FATAL for DPC Oza Pawandeep
2018-05-16 0:09 ` [PATCH v16 0/9] Address error and recovery for AER and DPC Bjorn Helgaas
9 siblings, 1 reply; 29+ messages in thread
From: Oza Pawandeep @ 2018-05-11 10:43 UTC (permalink / raw)
To: Bjorn Helgaas, Philippe Ombredanne, Thomas Gleixner,
Greg Kroah-Hartman, Kate Stewart, linux-pci, linux-kernel,
Dongdong Liu, Keith Busch, Wei Zhang, Sinan Kaya, Timur Tabi
Cc: Oza Pawandeep
DPC driver implements link_reset callback, and calls
pci_do_fatal_recovery().
Which follows standard path of ERR_FATAL recovery.
Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 5e8857a..6af7595 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -354,7 +354,7 @@ static inline resource_size_t pci_resource_alignment(struct pci_dev *dev,
void pci_enable_acs(struct pci_dev *dev);
/* PCI error reporting and recovery */
-void pcie_do_fatal_recovery(struct pci_dev *dev);
+void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service);
void pcie_do_nonfatal_recovery(struct pci_dev *dev);
bool pcie_wait_for_link(struct pci_dev *pdev, bool active);
diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
index fdfc474..36e622d 100644
--- a/drivers/pci/pcie/aer/aerdrv_core.c
+++ b/drivers/pci/pcie/aer/aerdrv_core.c
@@ -254,7 +254,7 @@ static void handle_error_source(struct pcie_device *aerdev,
} else if (info->severity == AER_NONFATAL)
pcie_do_nonfatal_recovery(dev);
else if (info->severity == AER_FATAL)
- pcie_do_fatal_recovery(dev);
+ pcie_do_fatal_recovery(dev, PCIE_PORT_SERVICE_AER);
}
#ifdef CONFIG_ACPI_APEI_PCIEAER
@@ -321,7 +321,7 @@ static void aer_recover_work_func(struct work_struct *work)
if (entry.severity == AER_NONFATAL)
pcie_do_nonfatal_recovery(pdev);
else if (entry.severity == AER_FATAL)
- pcie_do_fatal_recovery(pdev);
+ pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_AER);
pci_dev_put(pdev);
}
}
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 80ec384..5680c13 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -73,29 +73,31 @@ static void dpc_wait_link_inactive(struct dpc_dev *dpc)
pcie_wait_for_link(pdev, false);
}
-static void dpc_work(struct work_struct *work)
+static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
{
- struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
- struct pci_dev *dev, *temp, *pdev = dpc->dev->port;
- struct pci_bus *parent = pdev->subordinate;
- u16 cap = dpc->cap_pos, ctl;
-
- pci_lock_rescan_remove();
- list_for_each_entry_safe_reverse(dev, temp, &parent->devices,
- bus_list) {
- pci_dev_get(dev);
- pci_dev_set_disconnected(dev, NULL);
- if (pci_has_subordinate(dev))
- pci_walk_bus(dev->subordinate,
- pci_dev_set_disconnected, NULL);
- pci_stop_and_remove_bus_device(dev);
- pci_dev_put(dev);
- }
- pci_unlock_rescan_remove();
-
+ struct dpc_dev *dpc;
+ struct pcie_device *pciedev;
+ struct device *devdpc;
+ u16 cap, ctl;
+
+ /*
+ * DPC disables the Link automatically in hardware, so it has
+ * already been reset by the time we get here.
+ */
+
+ devdpc = pcie_port_find_device(pdev, PCIE_PORT_SERVICE_DPC);
+ pciedev = to_pcie_device(devdpc);
+ dpc = get_service_data(pciedev);
+ cap = dpc->cap_pos;
+
+ /*
+ * Waiting until the link is inactive, then clearing DPC
+ * trigger status to allow the port to leave DPC.
+ */
dpc_wait_link_inactive(dpc);
+
if (dpc->rp_extensions && dpc_wait_rp_inactive(dpc))
- return;
+ return PCI_ERS_RESULT_DISCONNECT;
if (dpc->rp_extensions && dpc->rp_pio_status) {
pci_write_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS,
dpc->rp_pio_status);
@@ -108,6 +110,17 @@ static void dpc_work(struct work_struct *work)
pci_read_config_word(pdev, cap + PCI_EXP_DPC_CTL, &ctl);
pci_write_config_word(pdev, cap + PCI_EXP_DPC_CTL,
ctl | PCI_EXP_DPC_CTL_INT_EN);
+
+ return PCI_ERS_RESULT_RECOVERED;
+}
+
+static void dpc_work(struct work_struct *work)
+{
+ struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
+ struct pci_dev *pdev = dpc->dev->port;
+
+ /* From DPC point of view error is always FATAL. */
+ pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_DPC);
}
static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
@@ -288,6 +301,7 @@ static struct pcie_port_service_driver dpcdriver = {
.service = PCIE_PORT_SERVICE_DPC,
.probe = dpc_probe,
.remove = dpc_remove,
+ .reset_link = dpc_reset_link,
};
static int __init dpc_service_init(void)
diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index 33a16b1..29ff148 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -185,7 +185,7 @@ static pci_ers_result_t default_reset_link(struct pci_dev *dev)
return PCI_ERS_RESULT_RECOVERED;
}
-static pci_ers_result_t reset_link(struct pci_dev *dev)
+static pci_ers_result_t reset_link(struct pci_dev *dev, u32 service)
{
struct pci_dev *udev;
pci_ers_result_t status;
@@ -200,7 +200,7 @@ static pci_ers_result_t reset_link(struct pci_dev *dev)
}
/* Use the aer driver of the component firstly */
- driver = pcie_port_find_service(udev, PCIE_PORT_SERVICE_AER);
+ driver = pcie_port_find_service(udev, service);
if (driver && driver->reset_link) {
status = driver->reset_link(udev);
@@ -287,7 +287,7 @@ static pci_ers_result_t broadcast_error_message(struct pci_dev *dev,
* followed by re-enumeration of devices.
*/
-void pcie_do_fatal_recovery(struct pci_dev *dev)
+void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
{
struct pci_dev *udev;
struct pci_bus *parent;
@@ -313,7 +313,7 @@ void pcie_do_fatal_recovery(struct pci_dev *dev)
pci_dev_put(pdev);
}
- result = reset_link(udev);
+ result = reset_link(udev, service);
if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
/*
diff --git a/include/linux/aer.h b/include/linux/aer.h
index 8f87bbe..0c506fe 100644
--- a/include/linux/aer.h
+++ b/include/linux/aer.h
@@ -14,6 +14,7 @@
#define AER_NONFATAL 0
#define AER_FATAL 1
#define AER_CORRECTABLE 2
+#define DPC_FATAL 4
struct pci_dev;
--
2.7.4
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v16 9/9] PCI/DPC: Disable ERR_NONFATAL and enable ERR_FATAL for DPC
2018-05-11 10:43 [PATCH v16 0/9] Address error and recovery for AER and DPC Oza Pawandeep
` (7 preceding siblings ...)
2018-05-11 10:43 ` [PATCH v16 8/9] PCI/DPC: Unify and plumb error handling into DPC Oza Pawandeep
@ 2018-05-11 10:43 ` Oza Pawandeep
2018-05-16 0:09 ` [PATCH v16 0/9] Address error and recovery for AER and DPC Bjorn Helgaas
9 siblings, 0 replies; 29+ messages in thread
From: Oza Pawandeep @ 2018-05-11 10:43 UTC (permalink / raw)
To: Bjorn Helgaas, Philippe Ombredanne, Thomas Gleixner,
Greg Kroah-Hartman, Kate Stewart, linux-pci, linux-kernel,
Dongdong Liu, Keith Busch, Wei Zhang, Sinan Kaya, Timur Tabi
Cc: Oza Pawandeep
This patch disables ERR_NONFATAL trigger for DPC, so now DPC
handles only ERR_FATAL.
Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 5680c13..358b4324 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -273,7 +273,7 @@ static int dpc_probe(struct pcie_device *dev)
}
}
- ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_NONFATAL | PCI_EXP_DPC_CTL_INT_EN;
+ ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
pci_write_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, ctl);
dev_info(device, "DPC error containment capabilities: Int Msg #%d, RPExt%c PoisonedTLP%c SwTrigger%c RP PIO Log %d, DL_ActiveErr%c\n",
@@ -291,7 +291,7 @@ static void dpc_remove(struct pcie_device *dev)
u16 ctl;
pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, &ctl);
- ctl &= ~(PCI_EXP_DPC_CTL_EN_NONFATAL | PCI_EXP_DPC_CTL_INT_EN);
+ ctl &= ~(PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN);
pci_write_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, ctl);
}
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 103ba79..5182e0d 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -981,6 +981,7 @@
#define PCI_EXP_DPC_CAP_DL_ACTIVE 0x1000 /* ERR_COR signal on DL_Active supported */
#define PCI_EXP_DPC_CTL 6 /* DPC control */
+#define PCI_EXP_DPC_CTL_EN_FATAL 0x0001 /* Enable trigger on ERR_FATAL message */
#define PCI_EXP_DPC_CTL_EN_NONFATAL 0x0002 /* Enable trigger on ERR_NONFATAL message */
#define PCI_EXP_DPC_CTL_INT_EN 0x0008 /* DPC Interrupt Enable */
--
2.7.4
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v16 8/9] PCI/DPC: Unify and plumb error handling into DPC
2018-05-11 10:43 ` [PATCH v16 8/9] PCI/DPC: Unify and plumb error handling into DPC Oza Pawandeep
@ 2018-05-11 11:52 ` poza
2018-05-15 23:56 ` Bjorn Helgaas
0 siblings, 1 reply; 29+ messages in thread
From: poza @ 2018-05-11 11:52 UTC (permalink / raw)
To: Bjorn Helgaas, Philippe Ombredanne, Thomas Gleixner,
Greg Kroah-Hartman, Kate Stewart, linux-pci, linux-kernel,
Dongdong Liu, Keith Busch, Wei Zhang, Sinan Kaya, Timur Tabi
On 2018-05-11 16:13, Oza Pawandeep wrote:
> DPC driver implements link_reset callback, and calls
> pci_do_fatal_recovery().
>
> Which follows standard path of ERR_FATAL recovery.
>
> Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
>
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 5e8857a..6af7595 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -354,7 +354,7 @@ static inline resource_size_t
> pci_resource_alignment(struct pci_dev *dev,
> void pci_enable_acs(struct pci_dev *dev);
>
> /* PCI error reporting and recovery */
> -void pcie_do_fatal_recovery(struct pci_dev *dev);
> +void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service);
> void pcie_do_nonfatal_recovery(struct pci_dev *dev);
>
> bool pcie_wait_for_link(struct pci_dev *pdev, bool active);
> diff --git a/drivers/pci/pcie/aer/aerdrv_core.c
> b/drivers/pci/pcie/aer/aerdrv_core.c
> index fdfc474..36e622d 100644
> --- a/drivers/pci/pcie/aer/aerdrv_core.c
> +++ b/drivers/pci/pcie/aer/aerdrv_core.c
> @@ -254,7 +254,7 @@ static void handle_error_source(struct pcie_device
> *aerdev,
> } else if (info->severity == AER_NONFATAL)
> pcie_do_nonfatal_recovery(dev);
> else if (info->severity == AER_FATAL)
> - pcie_do_fatal_recovery(dev);
> + pcie_do_fatal_recovery(dev, PCIE_PORT_SERVICE_AER);
> }
>
> #ifdef CONFIG_ACPI_APEI_PCIEAER
> @@ -321,7 +321,7 @@ static void aer_recover_work_func(struct
> work_struct *work)
> if (entry.severity == AER_NONFATAL)
> pcie_do_nonfatal_recovery(pdev);
> else if (entry.severity == AER_FATAL)
> - pcie_do_fatal_recovery(pdev);
> + pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_AER);
> pci_dev_put(pdev);
> }
> }
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index 80ec384..5680c13 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -73,29 +73,31 @@ static void dpc_wait_link_inactive(struct dpc_dev
> *dpc)
> pcie_wait_for_link(pdev, false);
> }
>
> -static void dpc_work(struct work_struct *work)
> +static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
> {
> - struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
> - struct pci_dev *dev, *temp, *pdev = dpc->dev->port;
> - struct pci_bus *parent = pdev->subordinate;
> - u16 cap = dpc->cap_pos, ctl;
> -
> - pci_lock_rescan_remove();
> - list_for_each_entry_safe_reverse(dev, temp, &parent->devices,
> - bus_list) {
> - pci_dev_get(dev);
> - pci_dev_set_disconnected(dev, NULL);
> - if (pci_has_subordinate(dev))
> - pci_walk_bus(dev->subordinate,
> - pci_dev_set_disconnected, NULL);
> - pci_stop_and_remove_bus_device(dev);
> - pci_dev_put(dev);
> - }
> - pci_unlock_rescan_remove();
> -
> + struct dpc_dev *dpc;
> + struct pcie_device *pciedev;
> + struct device *devdpc;
> + u16 cap, ctl;
> +
> + /*
> + * DPC disables the Link automatically in hardware, so it has
> + * already been reset by the time we get here.
> + */
> +
> + devdpc = pcie_port_find_device(pdev, PCIE_PORT_SERVICE_DPC);
> + pciedev = to_pcie_device(devdpc);
> + dpc = get_service_data(pciedev);
> + cap = dpc->cap_pos;
> +
> + /*
> + * Waiting until the link is inactive, then clearing DPC
> + * trigger status to allow the port to leave DPC.
> + */
> dpc_wait_link_inactive(dpc);
> +
> if (dpc->rp_extensions && dpc_wait_rp_inactive(dpc))
> - return;
> + return PCI_ERS_RESULT_DISCONNECT;
> if (dpc->rp_extensions && dpc->rp_pio_status) {
> pci_write_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS,
> dpc->rp_pio_status);
> @@ -108,6 +110,17 @@ static void dpc_work(struct work_struct *work)
> pci_read_config_word(pdev, cap + PCI_EXP_DPC_CTL, &ctl);
> pci_write_config_word(pdev, cap + PCI_EXP_DPC_CTL,
> ctl | PCI_EXP_DPC_CTL_INT_EN);
> +
> + return PCI_ERS_RESULT_RECOVERED;
> +}
> +
> +static void dpc_work(struct work_struct *work)
> +{
> + struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
> + struct pci_dev *pdev = dpc->dev->port;
> +
> + /* From DPC point of view error is always FATAL. */
> + pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_DPC);
> }
>
> static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
> @@ -288,6 +301,7 @@ static struct pcie_port_service_driver dpcdriver =
> {
> .service = PCIE_PORT_SERVICE_DPC,
> .probe = dpc_probe,
> .remove = dpc_remove,
> + .reset_link = dpc_reset_link,
> };
>
> static int __init dpc_service_init(void)
> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> index 33a16b1..29ff148 100644
> --- a/drivers/pci/pcie/err.c
> +++ b/drivers/pci/pcie/err.c
> @@ -185,7 +185,7 @@ static pci_ers_result_t default_reset_link(struct
> pci_dev *dev)
> return PCI_ERS_RESULT_RECOVERED;
> }
>
> -static pci_ers_result_t reset_link(struct pci_dev *dev)
> +static pci_ers_result_t reset_link(struct pci_dev *dev, u32 service)
> {
> struct pci_dev *udev;
> pci_ers_result_t status;
> @@ -200,7 +200,7 @@ static pci_ers_result_t reset_link(struct pci_dev
> *dev)
> }
>
> /* Use the aer driver of the component firstly */
> - driver = pcie_port_find_service(udev, PCIE_PORT_SERVICE_AER);
> + driver = pcie_port_find_service(udev, service);
>
> if (driver && driver->reset_link) {
> status = driver->reset_link(udev);
> @@ -287,7 +287,7 @@ static pci_ers_result_t
> broadcast_error_message(struct pci_dev *dev,
> * followed by re-enumeration of devices.
> */
>
> -void pcie_do_fatal_recovery(struct pci_dev *dev)
> +void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
> {
> struct pci_dev *udev;
> struct pci_bus *parent;
> @@ -313,7 +313,7 @@ void pcie_do_fatal_recovery(struct pci_dev *dev)
> pci_dev_put(pdev);
> }
>
> - result = reset_link(udev);
> + result = reset_link(udev, service);
>
> if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
> /*
> diff --git a/include/linux/aer.h b/include/linux/aer.h
> index 8f87bbe..0c506fe 100644
> --- a/include/linux/aer.h
> +++ b/include/linux/aer.h
> @@ -14,6 +14,7 @@
> #define AER_NONFATAL 0
> #define AER_FATAL 1
> #define AER_CORRECTABLE 2
> +#define DPC_FATAL 4
>
> struct pci_dev;
Hi Bjorn,
I have addressed all the comments, and I hope we are heading towards
closure.
I just figure that pcie_do_fatal_recovery (which is getting executed
for DPC as well)
if (result == PCI_ERS_RESULT_RECOVERED) {
if (pcie_wait_for_link(udev, true))
pci_rescan_bus(udev->bus);
pci_info(dev, "Device recovery successful\n");
}
I have to correct it to
if (service==AER && result == PCI_ERS_RESULT_RECOVERED) {
if (pcie_wait_for_link(udev, true))
pci_rescan_bus(udev->bus);
pci_info(dev, "Device recovery successful\n");
}
rest of the things look okay to me.
If you have any more comments,
I can fix them with this one and hopefully v17 will be the final one.
PS: I am going though the code more, and we can have some follow up
patches (probably some cleanup)
for e.g. pcie_portdrv_slot_reset() checks
if (dev->error_state == pci_channel_io_frozen) {
dev->state_saved = true;
pci_restore_state(dev);
pcie_portdrv_restore_config(dev);
pci_enable_pcie_error_reporting(dev);
}
but now since the ERR_FATAL path does not call pcie_portdrv_slot_reset
the check is meaning less.
besides driver's shut_down callbacks might want to handle
pci_channel_io_frozen, but that something left to be discussed later.
So, I am not touching dev->error_state anywhere as of now in ERR_FATAL
case.
Regards,
Oza.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v16 5/9] PCI/AER: Factor out error reporting from AER
2018-05-11 10:43 ` [PATCH v16 5/9] PCI/AER: Factor out error reporting from AER Oza Pawandeep
@ 2018-05-11 12:58 ` Lukas Wunner
2018-05-11 15:34 ` poza
2018-05-16 0:06 ` Bjorn Helgaas
1 sibling, 1 reply; 29+ messages in thread
From: Lukas Wunner @ 2018-05-11 12:58 UTC (permalink / raw)
To: Oza Pawandeep
Cc: Bjorn Helgaas, Philippe Ombredanne, Thomas Gleixner,
Greg Kroah-Hartman, Kate Stewart, linux-pci, linux-kernel,
Dongdong Liu, Keith Busch, Wei Zhang, Sinan Kaya, Timur Tabi
On Fri, May 11, 2018 at 06:43:24AM -0400, Oza Pawandeep wrote:
> +void pcie_do_fatal_recovery(struct pci_dev *dev)
> +{
> + struct pci_dev *udev;
> + struct pci_bus *parent;
> + struct pci_dev *pdev, *temp;
> + pci_ers_result_t result = PCI_ERS_RESULT_RECOVERED;
> + struct aer_broadcast_data result_data;
> +
> + if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
> + udev = dev;
> + else
> + udev = dev->bus->self;
> +
> + parent = udev->subordinate;
> + pci_lock_rescan_remove();
> + list_for_each_entry_safe_reverse(pdev, temp, &parent->devices,
> + bus_list) {
> + pci_dev_get(pdev);
> + pci_dev_set_disconnected(pdev, NULL);
> + if (pci_has_subordinate(pdev))
> + pci_walk_bus(pdev->subordinate,
> + pci_dev_set_disconnected, NULL);
> + pci_stop_and_remove_bus_device(pdev);
> + pci_dev_put(pdev);
> + }
Any reason not to simply call
pci_walk_bus(udev->subordinate, pci_dev_set_disconnected, NULL);
before the list_for_each_entry_safe_reverse() iteration, instead of
calling it for each device on the subordinate bus and for each
device's children? Should be semantically identical, saves 3 LoC
and saves wasted cycles of acquiring pci_bus_sem over and over again
for each device on the subordinate bus.
Thanks,
Lukas
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v16 5/9] PCI/AER: Factor out error reporting from AER
2018-05-11 12:58 ` Lukas Wunner
@ 2018-05-11 15:34 ` poza
2018-05-11 15:54 ` Lukas Wunner
0 siblings, 1 reply; 29+ messages in thread
From: poza @ 2018-05-11 15:34 UTC (permalink / raw)
To: Lukas Wunner
Cc: Bjorn Helgaas, Philippe Ombredanne, Thomas Gleixner,
Greg Kroah-Hartman, Kate Stewart, linux-pci, linux-kernel,
Dongdong Liu, Keith Busch, Wei Zhang, Sinan Kaya, Timur Tabi
On 2018-05-11 18:28, Lukas Wunner wrote:
> On Fri, May 11, 2018 at 06:43:24AM -0400, Oza Pawandeep wrote:
>> +void pcie_do_fatal_recovery(struct pci_dev *dev)
>> +{
>> + struct pci_dev *udev;
>> + struct pci_bus *parent;
>> + struct pci_dev *pdev, *temp;
>> + pci_ers_result_t result = PCI_ERS_RESULT_RECOVERED;
>> + struct aer_broadcast_data result_data;
>> +
>> + if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
>> + udev = dev;
>> + else
>> + udev = dev->bus->self;
>> +
>> + parent = udev->subordinate;
>> + pci_lock_rescan_remove();
>> + list_for_each_entry_safe_reverse(pdev, temp, &parent->devices,
>> + bus_list) {
>> + pci_dev_get(pdev);
>> + pci_dev_set_disconnected(pdev, NULL);
>> + if (pci_has_subordinate(pdev))
>> + pci_walk_bus(pdev->subordinate,
>> + pci_dev_set_disconnected, NULL);
>> + pci_stop_and_remove_bus_device(pdev);
>> + pci_dev_put(pdev);
>> + }
>
> Any reason not to simply call
>
> pci_walk_bus(udev->subordinate, pci_dev_set_disconnected, NULL);
>
> before the list_for_each_entry_safe_reverse() iteration, instead of
> calling it for each device on the subordinate bus and for each
> device's children? Should be semantically identical, saves 3 LoC
> and saves wasted cycles of acquiring pci_bus_sem over and over again
> for each device on the subordinate bus.
>
> Thanks,
>
> Lukas
Well this is borrowed code from DPC driver, hence I thought to keep the
same.
but to me it looks like its taking care of PCIe switch where is goes
through all the subordinates, and which could turn out to be more
swicthes down the line, and son on...
it goes all the way down to the tree
Am I missing something here ?
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v16 5/9] PCI/AER: Factor out error reporting from AER
2018-05-11 15:34 ` poza
@ 2018-05-11 15:54 ` Lukas Wunner
2018-05-11 16:11 ` poza
0 siblings, 1 reply; 29+ messages in thread
From: Lukas Wunner @ 2018-05-11 15:54 UTC (permalink / raw)
To: poza
Cc: Bjorn Helgaas, Philippe Ombredanne, Thomas Gleixner,
Greg Kroah-Hartman, Kate Stewart, linux-pci, linux-kernel,
Dongdong Liu, Keith Busch, Wei Zhang, Sinan Kaya, Timur Tabi
On Fri, May 11, 2018 at 09:04:36PM +0530, poza@codeaurora.org wrote:
> On 2018-05-11 18:28, Lukas Wunner wrote:
> >On Fri, May 11, 2018 at 06:43:24AM -0400, Oza Pawandeep wrote:
> >>+void pcie_do_fatal_recovery(struct pci_dev *dev)
> >>+{
> >>+ struct pci_dev *udev;
> >>+ struct pci_bus *parent;
> >>+ struct pci_dev *pdev, *temp;
> >>+ pci_ers_result_t result = PCI_ERS_RESULT_RECOVERED;
> >>+ struct aer_broadcast_data result_data;
> >>+
> >>+ if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
> >>+ udev = dev;
> >>+ else
> >>+ udev = dev->bus->self;
> >>+
> >>+ parent = udev->subordinate;
> >>+ pci_lock_rescan_remove();
> >>+ list_for_each_entry_safe_reverse(pdev, temp, &parent->devices,
> >>+ bus_list) {
> >>+ pci_dev_get(pdev);
> >>+ pci_dev_set_disconnected(pdev, NULL);
> >>+ if (pci_has_subordinate(pdev))
> >>+ pci_walk_bus(pdev->subordinate,
> >>+ pci_dev_set_disconnected, NULL);
> >>+ pci_stop_and_remove_bus_device(pdev);
> >>+ pci_dev_put(pdev);
> >>+ }
> >
> >Any reason not to simply call
> >
> > pci_walk_bus(udev->subordinate, pci_dev_set_disconnected, NULL);
> >
> >before the list_for_each_entry_safe_reverse() iteration, instead of
> >calling it for each device on the subordinate bus and for each
> >device's children? Should be semantically identical, saves 3 LoC
> >and saves wasted cycles of acquiring pci_bus_sem over and over again
> >for each device on the subordinate bus.
>
> Well this is borrowed code from DPC driver, hence I thought to keep the
> same.
> but to me it looks like its taking care of PCIe switch where is goes through
> all the subordinates, and which could turn out to be more swicthes down the
> line, and son on...
> it goes all the way down to the tree
... which is precisely what the one line I suggested above does.
You don't need to respin for this alone as far as I'm concerned,
but please post a follow-up refactoring patch. I have a patch
in the pipeline which makes the same change in pciehp, hence this
caught my eye.
Thanks,
Lukas
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v16 5/9] PCI/AER: Factor out error reporting from AER
2018-05-11 15:54 ` Lukas Wunner
@ 2018-05-11 16:11 ` poza
0 siblings, 0 replies; 29+ messages in thread
From: poza @ 2018-05-11 16:11 UTC (permalink / raw)
To: Lukas Wunner
Cc: Bjorn Helgaas, Philippe Ombredanne, Thomas Gleixner,
Greg Kroah-Hartman, Kate Stewart, linux-pci, linux-kernel,
Dongdong Liu, Keith Busch, Wei Zhang, Sinan Kaya, Timur Tabi
On 2018-05-11 21:24, Lukas Wunner wrote:
> On Fri, May 11, 2018 at 09:04:36PM +0530, poza@codeaurora.org wrote:
>> On 2018-05-11 18:28, Lukas Wunner wrote:
>> >On Fri, May 11, 2018 at 06:43:24AM -0400, Oza Pawandeep wrote:
>> >>+void pcie_do_fatal_recovery(struct pci_dev *dev)
>> >>+{
>> >>+ struct pci_dev *udev;
>> >>+ struct pci_bus *parent;
>> >>+ struct pci_dev *pdev, *temp;
>> >>+ pci_ers_result_t result = PCI_ERS_RESULT_RECOVERED;
>> >>+ struct aer_broadcast_data result_data;
>> >>+
>> >>+ if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
>> >>+ udev = dev;
>> >>+ else
>> >>+ udev = dev->bus->self;
>> >>+
>> >>+ parent = udev->subordinate;
>> >>+ pci_lock_rescan_remove();
>> >>+ list_for_each_entry_safe_reverse(pdev, temp, &parent->devices,
>> >>+ bus_list) {
>> >>+ pci_dev_get(pdev);
>> >>+ pci_dev_set_disconnected(pdev, NULL);
>> >>+ if (pci_has_subordinate(pdev))
>> >>+ pci_walk_bus(pdev->subordinate,
>> >>+ pci_dev_set_disconnected, NULL);
>> >>+ pci_stop_and_remove_bus_device(pdev);
>> >>+ pci_dev_put(pdev);
>> >>+ }
>> >
>> >Any reason not to simply call
>> >
>> > pci_walk_bus(udev->subordinate, pci_dev_set_disconnected, NULL);
>> >
>> >before the list_for_each_entry_safe_reverse() iteration, instead of
>> >calling it for each device on the subordinate bus and for each
>> >device's children? Should be semantically identical, saves 3 LoC
>> >and saves wasted cycles of acquiring pci_bus_sem over and over again
>> >for each device on the subordinate bus.
>>
>> Well this is borrowed code from DPC driver, hence I thought to keep
>> the
>> same.
>> but to me it looks like its taking care of PCIe switch where is goes
>> through
>> all the subordinates, and which could turn out to be more swicthes
>> down the
>> line, and son on...
>> it goes all the way down to the tree
>
> ... which is precisely what the one line I suggested above does.
>
> You don't need to respin for this alone as far as I'm concerned,
> but please post a follow-up refactoring patch. I have a patch
> in the pipeline which makes the same change in pciehp, hence this
> caught my eye.
>
> Thanks,
>
> Lukas
Thanks Lukas, I will keep this in my pipeline as an optimization.
appreciate your input.
Regards,
Oza.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v16 8/9] PCI/DPC: Unify and plumb error handling into DPC
2018-05-11 11:52 ` poza
@ 2018-05-15 23:56 ` Bjorn Helgaas
2018-05-16 8:16 ` poza
0 siblings, 1 reply; 29+ messages in thread
From: Bjorn Helgaas @ 2018-05-15 23:56 UTC (permalink / raw)
To: poza
Cc: Bjorn Helgaas, Philippe Ombredanne, Thomas Gleixner,
Greg Kroah-Hartman, Kate Stewart, linux-pci, linux-kernel,
Dongdong Liu, Keith Busch, Wei Zhang, Sinan Kaya, Timur Tabi
On Fri, May 11, 2018 at 05:22:08PM +0530, poza@codeaurora.org wrote:
> On 2018-05-11 16:13, Oza Pawandeep wrote:
> > DPC driver implements link_reset callback, and calls
> > pci_do_fatal_recovery().
> >
> > Which follows standard path of ERR_FATAL recovery.
> >
> > Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
> >
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > index 5e8857a..6af7595 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -354,7 +354,7 @@ static inline resource_size_t
> > pci_resource_alignment(struct pci_dev *dev,
> > void pci_enable_acs(struct pci_dev *dev);
> >
> > /* PCI error reporting and recovery */
> > -void pcie_do_fatal_recovery(struct pci_dev *dev);
> > +void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service);
> > void pcie_do_nonfatal_recovery(struct pci_dev *dev);
> >
> > bool pcie_wait_for_link(struct pci_dev *pdev, bool active);
> > diff --git a/drivers/pci/pcie/aer/aerdrv_core.c
> > b/drivers/pci/pcie/aer/aerdrv_core.c
> > index fdfc474..36e622d 100644
> > --- a/drivers/pci/pcie/aer/aerdrv_core.c
> > +++ b/drivers/pci/pcie/aer/aerdrv_core.c
> > @@ -254,7 +254,7 @@ static void handle_error_source(struct pcie_device
> > *aerdev,
> > } else if (info->severity == AER_NONFATAL)
> > pcie_do_nonfatal_recovery(dev);
> > else if (info->severity == AER_FATAL)
> > - pcie_do_fatal_recovery(dev);
> > + pcie_do_fatal_recovery(dev, PCIE_PORT_SERVICE_AER);
> > }
> >
> > #ifdef CONFIG_ACPI_APEI_PCIEAER
> > @@ -321,7 +321,7 @@ static void aer_recover_work_func(struct work_struct
> > *work)
> > if (entry.severity == AER_NONFATAL)
> > pcie_do_nonfatal_recovery(pdev);
> > else if (entry.severity == AER_FATAL)
> > - pcie_do_fatal_recovery(pdev);
> > + pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_AER);
> > pci_dev_put(pdev);
> > }
> > }
> > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > index 80ec384..5680c13 100644
> > --- a/drivers/pci/pcie/dpc.c
> > +++ b/drivers/pci/pcie/dpc.c
> > @@ -73,29 +73,31 @@ static void dpc_wait_link_inactive(struct dpc_dev
> > *dpc)
> > pcie_wait_for_link(pdev, false);
> > }
> >
> > -static void dpc_work(struct work_struct *work)
> > +static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
> > {
> > - struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
> > - struct pci_dev *dev, *temp, *pdev = dpc->dev->port;
> > - struct pci_bus *parent = pdev->subordinate;
> > - u16 cap = dpc->cap_pos, ctl;
> > -
> > - pci_lock_rescan_remove();
> > - list_for_each_entry_safe_reverse(dev, temp, &parent->devices,
> > - bus_list) {
> > - pci_dev_get(dev);
> > - pci_dev_set_disconnected(dev, NULL);
> > - if (pci_has_subordinate(dev))
> > - pci_walk_bus(dev->subordinate,
> > - pci_dev_set_disconnected, NULL);
> > - pci_stop_and_remove_bus_device(dev);
> > - pci_dev_put(dev);
> > - }
> > - pci_unlock_rescan_remove();
> > -
> > + struct dpc_dev *dpc;
> > + struct pcie_device *pciedev;
> > + struct device *devdpc;
> > + u16 cap, ctl;
> > +
> > + /*
> > + * DPC disables the Link automatically in hardware, so it has
> > + * already been reset by the time we get here.
> > + */
> > +
> > + devdpc = pcie_port_find_device(pdev, PCIE_PORT_SERVICE_DPC);
> > + pciedev = to_pcie_device(devdpc);
> > + dpc = get_service_data(pciedev);
> > + cap = dpc->cap_pos;
> > +
> > + /*
> > + * Waiting until the link is inactive, then clearing DPC
> > + * trigger status to allow the port to leave DPC.
> > + */
> > dpc_wait_link_inactive(dpc);
> > +
> > if (dpc->rp_extensions && dpc_wait_rp_inactive(dpc))
> > - return;
> > + return PCI_ERS_RESULT_DISCONNECT;
> > if (dpc->rp_extensions && dpc->rp_pio_status) {
> > pci_write_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS,
> > dpc->rp_pio_status);
> > @@ -108,6 +110,17 @@ static void dpc_work(struct work_struct *work)
> > pci_read_config_word(pdev, cap + PCI_EXP_DPC_CTL, &ctl);
> > pci_write_config_word(pdev, cap + PCI_EXP_DPC_CTL,
> > ctl | PCI_EXP_DPC_CTL_INT_EN);
> > +
> > + return PCI_ERS_RESULT_RECOVERED;
> > +}
> > +
> > +static void dpc_work(struct work_struct *work)
> > +{
> > + struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
> > + struct pci_dev *pdev = dpc->dev->port;
> > +
> > + /* From DPC point of view error is always FATAL. */
> > + pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_DPC);
> > }
> >
> > static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
> > @@ -288,6 +301,7 @@ static struct pcie_port_service_driver dpcdriver = {
> > .service = PCIE_PORT_SERVICE_DPC,
> > .probe = dpc_probe,
> > .remove = dpc_remove,
> > + .reset_link = dpc_reset_link,
> > };
> >
> > static int __init dpc_service_init(void)
> > diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> > index 33a16b1..29ff148 100644
> > --- a/drivers/pci/pcie/err.c
> > +++ b/drivers/pci/pcie/err.c
> > @@ -185,7 +185,7 @@ static pci_ers_result_t default_reset_link(struct
> > pci_dev *dev)
> > return PCI_ERS_RESULT_RECOVERED;
> > }
> >
> > -static pci_ers_result_t reset_link(struct pci_dev *dev)
> > +static pci_ers_result_t reset_link(struct pci_dev *dev, u32 service)
> > {
> > struct pci_dev *udev;
> > pci_ers_result_t status;
> > @@ -200,7 +200,7 @@ static pci_ers_result_t reset_link(struct pci_dev
> > *dev)
> > }
> >
> > /* Use the aer driver of the component firstly */
> > - driver = pcie_port_find_service(udev, PCIE_PORT_SERVICE_AER);
> > + driver = pcie_port_find_service(udev, service);
> >
> > if (driver && driver->reset_link) {
> > status = driver->reset_link(udev);
> > @@ -287,7 +287,7 @@ static pci_ers_result_t
> > broadcast_error_message(struct pci_dev *dev,
> > * followed by re-enumeration of devices.
> > */
> >
> > -void pcie_do_fatal_recovery(struct pci_dev *dev)
> > +void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
> > {
> > struct pci_dev *udev;
> > struct pci_bus *parent;
> > @@ -313,7 +313,7 @@ void pcie_do_fatal_recovery(struct pci_dev *dev)
> > pci_dev_put(pdev);
> > }
> >
> > - result = reset_link(udev);
> > + result = reset_link(udev, service);
> >
> > if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
> > /*
> > diff --git a/include/linux/aer.h b/include/linux/aer.h
> > index 8f87bbe..0c506fe 100644
> > --- a/include/linux/aer.h
> > +++ b/include/linux/aer.h
> > @@ -14,6 +14,7 @@
> > #define AER_NONFATAL 0
> > #define AER_FATAL 1
> > #define AER_CORRECTABLE 2
> > +#define DPC_FATAL 4
I think DPC_FATAL can be 3, since these values are not used as a bit mask.
> > struct pci_dev;
>
>
> Hi Bjorn,
>
>
> I have addressed all the comments, and I hope we are heading towards
> closure.
>
> I just figure that pcie_do_fatal_recovery (which is getting executed for
> DPC as well)
>
> if (result == PCI_ERS_RESULT_RECOVERED) {
> if (pcie_wait_for_link(udev, true))
> pci_rescan_bus(udev->bus);
> pci_info(dev, "Device recovery successful\n");
> }
>
> I have to correct it to
>
> if (service==AER && result == PCI_ERS_RESULT_RECOVERED) {
> if (pcie_wait_for_link(udev, true))
> pci_rescan_bus(udev->bus);
> pci_info(dev, "Device recovery successful\n");
> }
This patch is mostly a restructuring of DPC and doesn't really change its
behavior. DPC didn't previously call pcie_wait_for_link() or
pci_rescan_bus(), so I agree that adding the "service==AER" test will
help preserve the existing DPC behavior.
However, the rescan should happen with DPC *somewhere* and we should
clarify where that is. Maybe for now we only need a comment about where
that happens. Ideally, we could eventually converge this so the same
mechanism is used for AER and DPC, so we wouldn't need a test like
"service=AER".
> rest of the things look okay to me.
> If you have any more comments,
> I can fix them with this one and hopefully v17 will be the final one.
>
>
> PS: I am going though the code more, and we can have some follow up patches
> (probably some cleanup)
> for e.g. pcie_portdrv_slot_reset() checks
> if (dev->error_state == pci_channel_io_frozen) {
> dev->state_saved = true;
> pci_restore_state(dev);
> pcie_portdrv_restore_config(dev);
> pci_enable_pcie_error_reporting(dev);
> }
>
>
> but now since the ERR_FATAL path does not call pcie_portdrv_slot_reset the
> check is meaning less.
>
> besides driver's shut_down callbacks might want to handle
> pci_channel_io_frozen, but that something left to be discussed later.
> So, I am not touching dev->error_state anywhere as of now in ERR_FATAL case.
>
> Regards,
> Oza.
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v16 3/9] PCI/AER: Handle ERR_FATAL with removal and re-enumeration of devices
2018-05-11 10:43 ` [PATCH v16 3/9] PCI/AER: Handle ERR_FATAL with removal and re-enumeration of devices Oza Pawandeep
@ 2018-05-15 23:59 ` Bjorn Helgaas
2018-05-16 5:49 ` poza
0 siblings, 1 reply; 29+ messages in thread
From: Bjorn Helgaas @ 2018-05-15 23:59 UTC (permalink / raw)
To: Oza Pawandeep
Cc: Bjorn Helgaas, Philippe Ombredanne, Thomas Gleixner,
Greg Kroah-Hartman, Kate Stewart, linux-pci, linux-kernel,
Dongdong Liu, Keith Busch, Wei Zhang, Sinan Kaya, Timur Tabi
On Fri, May 11, 2018 at 06:43:22AM -0400, Oza Pawandeep wrote:
> This patch alters the behavior of handling of ERR_FATAL, where removal
> of devices is initiated, followed by reset link, followed by
> re-enumeration.
>
> So the errors are handled in a different way as follows:
> ERR_NONFATAL => call driver recovery entry points
> ERR_FATAL => remove and re-enumerate
>
> please refer to Documentation/PCI/pci-error-recovery.txt for more details.
>
> Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
> Reviewed-by: Keith Busch <keith.busch@intel.com>
>
> diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
> index 0ea5acc..649dd1f 100644
> --- a/drivers/pci/pcie/aer/aerdrv_core.c
> +++ b/drivers/pci/pcie/aer/aerdrv_core.c
> @@ -20,6 +20,7 @@
> #include <linux/slab.h>
> #include <linux/kfifo.h>
> #include "aerdrv.h"
> +#include "../../pci.h"
>
> #define PCI_EXP_AER_FLAGS (PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVCTL_NFERE | \
> PCI_EXP_DEVCTL_FERE | PCI_EXP_DEVCTL_URRE)
> @@ -475,35 +476,84 @@ static pci_ers_result_t reset_link(struct pci_dev *dev)
> }
>
> /**
> - * do_recovery - handle nonfatal/fatal error recovery process
> + * do_fatal_recovery - handle fatal error recovery process
> + * @dev: pointer to a pci_dev data structure of agent detecting an error
> + *
> + * Invoked when an error is fatal. Once being invoked, removes the devices
> + * benetah this AER agent, followed by reset link e.g. secondary bus reset
> + * followed by re-enumeration of devices.
> + */
> +
> +static void do_fatal_recovery(struct pci_dev *dev)
> +{
> + struct pci_dev *udev;
> + struct pci_bus *parent;
> + struct pci_dev *pdev, *temp;
> + pci_ers_result_t result = PCI_ERS_RESULT_RECOVERED;
> + struct aer_broadcast_data result_data;
> +
> + if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
> + udev = dev;
> + else
> + udev = dev->bus->self;
> +
> + parent = udev->subordinate;
> + pci_lock_rescan_remove();
> + list_for_each_entry_safe_reverse(pdev, temp, &parent->devices,
> + bus_list) {
> + pci_dev_get(pdev);
> + pci_dev_set_disconnected(pdev, NULL);
> + if (pci_has_subordinate(pdev))
> + pci_walk_bus(pdev->subordinate,
> + pci_dev_set_disconnected, NULL);
> + pci_stop_and_remove_bus_device(pdev);
> + pci_dev_put(pdev);
> + }
> +
> + result = reset_link(udev);
I don't like the fact that for DPC, the link reset happens before we call
the driver .remove() methods, while for AER, the reset happens *after* the
.remove() methods. That means the .remove() methods may work differently
for AER vs. DPC, e.g., they may be able to access the device if AER is
handling the error, but they would not be able to access it if DPC is
handling it.
I don't know how to fix this, and I think we can keep this patch as it is
for now, but I think we should fix it eventually.
> + if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
> + /*
> + * If the error is reported by a bridge, we think this error
> + * is related to the downstream link of the bridge, so we
> + * do error recovery on all subordinates of the bridge instead
> + * of the bridge and clear the error status of the bridge.
> + */
> + pci_walk_bus(dev->subordinate, report_resume, &result_data);
> + pci_cleanup_aer_uncorrect_error_status(dev);
> + }
> +
> + if (result == PCI_ERS_RESULT_RECOVERED) {
> + if (pcie_wait_for_link(udev, true))
> + pci_rescan_bus(udev->bus);
> + } else {
> + pci_uevent_ers(dev, PCI_ERS_RESULT_DISCONNECT);
> + pci_info(dev, "AER: Device recovery failed\n");
> + }
> +
> + pci_unlock_rescan_remove();
> +}
> +
> +/**
> + * do_nonfatal_recovery - handle nonfatal error recovery process
> * @dev: pointer to a pci_dev data structure of agent detecting an error
> - * @severity: error severity type
> *
> * Invoked when an error is nonfatal/fatal. Once being invoked, broadcast
> * error detected message to all downstream drivers within a hierarchy in
> * question and return the returned code.
> */
> -static void do_recovery(struct pci_dev *dev, int severity)
> +static void do_nonfatal_recovery(struct pci_dev *dev)
> {
> - pci_ers_result_t status, result = PCI_ERS_RESULT_RECOVERED;
> + pci_ers_result_t status;
> enum pci_channel_state state;
>
> - if (severity == AER_FATAL)
> - state = pci_channel_io_frozen;
> - else
> - state = pci_channel_io_normal;
> + state = pci_channel_io_normal;
>
> status = broadcast_error_message(dev,
> state,
> "error_detected",
> report_error_detected);
>
> - if (severity == AER_FATAL) {
> - result = reset_link(dev);
> - if (result != PCI_ERS_RESULT_RECOVERED)
> - goto failed;
> - }
> -
> if (status == PCI_ERS_RESULT_CAN_RECOVER)
> status = broadcast_error_message(dev,
> state,
> @@ -562,8 +612,10 @@ static void handle_error_source(struct pcie_device *aerdev,
> if (pos)
> pci_write_config_dword(dev, pos + PCI_ERR_COR_STATUS,
> info->status);
> - } else
> - do_recovery(dev, info->severity);
> + } else if (info->severity == AER_NONFATAL)
> + do_nonfatal_recovery(dev);
> + else if (info->severity == AER_FATAL)
> + do_fatal_recovery(dev);
> }
>
> #ifdef CONFIG_ACPI_APEI_PCIEAER
> @@ -627,8 +679,10 @@ static void aer_recover_work_func(struct work_struct *work)
> continue;
> }
> cper_print_aer(pdev, entry.severity, entry.regs);
> - if (entry.severity != AER_CORRECTABLE)
> - do_recovery(pdev, entry.severity);
> + if (entry.severity == AER_NONFATAL)
> + do_nonfatal_recovery(pdev);
> + else if (entry.severity == AER_FATAL)
> + do_fatal_recovery(pdev);
> pci_dev_put(pdev);
> }
> }
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v16 5/9] PCI/AER: Factor out error reporting from AER
2018-05-11 10:43 ` [PATCH v16 5/9] PCI/AER: Factor out error reporting from AER Oza Pawandeep
2018-05-11 12:58 ` Lukas Wunner
@ 2018-05-16 0:06 ` Bjorn Helgaas
1 sibling, 0 replies; 29+ messages in thread
From: Bjorn Helgaas @ 2018-05-16 0:06 UTC (permalink / raw)
To: Oza Pawandeep
Cc: Bjorn Helgaas, Philippe Ombredanne, Thomas Gleixner,
Greg Kroah-Hartman, Kate Stewart, linux-pci, linux-kernel,
Dongdong Liu, Keith Busch, Wei Zhang, Sinan Kaya, Timur Tabi
On Fri, May 11, 2018 at 06:43:24AM -0400, Oza Pawandeep wrote:
> This patch factors out error reporting callbacks, which are currently
> tightly coupled with AER.
>
> DPC should be able to register callbacks and attempt recovery when DPC
> trigger event occurs.
>
> Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
> +static int report_error_detected(struct pci_dev *dev, void *data)
> +{
> + pci_ers_result_t vote;
> + const struct pci_error_handlers *err_handler;
> + struct aer_broadcast_data *result_data;
> +
> + result_data = (struct aer_broadcast_data *) data;
> +
> + device_lock(&dev->dev);
> + dev->error_state = result_data->state;
> +
> + if (!dev->driver ||
> + !dev->driver->err_handler ||
> + !dev->driver->err_handler->error_detected) {
> + if (result_data->state == pci_channel_io_frozen &&
> + dev->hdr_type != PCI_HEADER_TYPE_BRIDGE) {
> + /*
> + * In case of fatal recovery, if one of down-
> + * stream device has no driver. We might be
> + * unable to recover because a later insmod
> + * of a driver for this device is unaware of
> + * its hw state.
> + */
> + pci_printk(KERN_DEBUG, dev, "device has %s\n",
> + dev->driver ?
> + "no AER-aware driver" : "no driver");
> + }
> +
> + /*
> + * If there's any device in the subtree that does not
> + * have an error_detected callback, returning
> + * PCI_ERS_RESULT_NO_AER_DRIVER prevents calling of
> + * the subsequent mmio_enabled/slot_reset/resume
> + * callbacks of "any" device in the subtree. All the
> + * devices in the subtree are left in the error state
> + * without recovery.
> + */
> +
> + if (dev->hdr_type != PCI_HEADER_TYPE_BRIDGE)
> + vote = PCI_ERS_RESULT_NO_AER_DRIVER;
> + else
> + vote = PCI_ERS_RESULT_NONE;
> + } else {
> + err_handler = dev->driver->err_handler;
> + vote = err_handler->error_detected(dev, result_data->state);
> +#if defined(CONFIG_PCIEAER)
> + pci_uevent_ers(dev, PCI_ERS_RESULT_NONE);
> +#endif
> +static int report_slot_reset(struct pci_dev *dev, void *data)
> +{
> + pci_ers_result_t vote;
> + const struct pci_error_handlers *err_handler;
> + struct aer_broadcast_data *result_data;
> +
> + result_data = (struct aer_broadcast_data *) data;
> +
> + device_lock(&dev->dev);
> + if (!dev->driver ||
> + !dev->driver->err_handler ||
> + !dev->driver->err_handler->slot_reset)
> + goto out;
> +
> + err_handler = dev->driver->err_handler;
> + vote = err_handler->slot_reset(dev);
> + result_data->result = merge_result(result_data->result, vote);
> +out:
> + device_unlock(&dev->dev);
> + return 0;
> +}
> +
> +static int report_resume(struct pci_dev *dev, void *data)
> +{
> + const struct pci_error_handlers *err_handler;
> +
> + device_lock(&dev->dev);
> + dev->error_state = pci_channel_io_normal;
> +
> + if (!dev->driver ||
> + !dev->driver->err_handler ||
> + !dev->driver->err_handler->resume)
> + goto out;
> +
> + err_handler = dev->driver->err_handler;
> + err_handler->resume(dev);
> +#if defined(CONFIG_PCIEAER)
> + pci_uevent_ers(dev, PCI_ERS_RESULT_RECOVERED);
> +#endif
> +void pcie_do_fatal_recovery(struct pci_dev *dev)
> +{
> + struct pci_dev *udev;
> + struct pci_bus *parent;
> + struct pci_dev *pdev, *temp;
> + pci_ers_result_t result = PCI_ERS_RESULT_RECOVERED;
> + struct aer_broadcast_data result_data;
> +
> + if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
> + udev = dev;
> + else
> + udev = dev->bus->self;
> +
> + parent = udev->subordinate;
> + pci_lock_rescan_remove();
> + list_for_each_entry_safe_reverse(pdev, temp, &parent->devices,
> + bus_list) {
> + pci_dev_get(pdev);
> + pci_dev_set_disconnected(pdev, NULL);
> + if (pci_has_subordinate(pdev))
> + pci_walk_bus(pdev->subordinate,
> + pci_dev_set_disconnected, NULL);
> + pci_stop_and_remove_bus_device(pdev);
> + pci_dev_put(pdev);
> + }
> +
> + result = reset_link(udev);
> +
> + if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
> + /*
> + * If the error is reported by a bridge, we think this error
> + * is related to the downstream link of the bridge, so we
> + * do error recovery on all subordinates of the bridge instead
> + * of the bridge and clear the error status of the bridge.
> + */
> + pci_walk_bus(dev->subordinate, report_resume, &result_data);
> + pci_cleanup_aer_uncorrect_error_status(dev);
> + }
> +
> + if (result == PCI_ERS_RESULT_RECOVERED) {
> + if (pcie_wait_for_link(udev, true))
> + pci_rescan_bus(udev->bus);
> + pci_info(dev, "Device recovery successful\n");
> + } else {
> +#if defined(CONFIG_PCIEAER)
> + pci_uevent_ers(dev, PCI_ERS_RESULT_DISCONNECT);
> +#endif
I don't think this is the optimal resolution for this problem.
It is true that we only call this function if either
CONFIG_PCIEAER=y or
CONFIG_PCIE_DPC=y
and furthermore that CONFIG_PCIE_DPC depends on CONFIG_PCIEAER, so in
either case, pci_uevent_ers() is present, since it is conditional on
#if defined(CONFIG_PCIEAER) || defined(CONFIG_EEH)
But the #ifdef here seems unnecessarily complicated. I think it would be
better to change the #ifdef around the definition of pci_uevent_ers().
Then we wouldn't need the several #ifdefs in this file.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v16 0/9] Address error and recovery for AER and DPC
2018-05-11 10:43 [PATCH v16 0/9] Address error and recovery for AER and DPC Oza Pawandeep
` (8 preceding siblings ...)
2018-05-11 10:43 ` [PATCH v16 9/9] PCI/DPC: Disable ERR_NONFATAL and enable ERR_FATAL for DPC Oza Pawandeep
@ 2018-05-16 0:09 ` Bjorn Helgaas
9 siblings, 0 replies; 29+ messages in thread
From: Bjorn Helgaas @ 2018-05-16 0:09 UTC (permalink / raw)
To: Oza Pawandeep
Cc: Bjorn Helgaas, Philippe Ombredanne, Thomas Gleixner,
Greg Kroah-Hartman, Kate Stewart, linux-pci, linux-kernel,
Dongdong Liu, Keith Busch, Wei Zhang, Sinan Kaya, Timur Tabi
On Fri, May 11, 2018 at 06:43:19AM -0400, Oza Pawandeep wrote:
> This patch set brings in error handling support for DPC
>
> The current implementation of AER and error message broadcasting to the
> EP driver is tightly coupled and limited to AER service driver.
> It is important to factor out broadcasting and other link handling
> callbacks. So that not only when AER gets triggered, but also when DPC get
> triggered (for e.g. ERR_FATAL), callbacks are handled appropriately.
>
> The goal of the patch-set is:
> DPC should handle the error handling and recovery similar to AER, because
> finally both are attempting recovery in some or the other way,
> and for that error handling and recovery framework has to be loosely
> coupled.
>
> It achieves uniformity and transparency to the error handling agents such
> as AER, DPC, with respect to recovery and error handling.
>
> So, this patch-set tries to unify lot of things between error agents and
> make them behave in a well defined way. (be it error (FATAL, NON_FATAL)
> handling or recovery).
>
> The FATAL error handling is handled with remove/reset_link/re-enumerate
> sequence while the NON_FATAL follows the default path.
> Documentation/PCI/pci-error-recovery.txt talks more on that.
>
> Changes since v15:
> Bjorn's comments addressed
> > minor comments fixed
> > made FATAL sequence aligned to existing one, as far as clearing status are concerned.
> > pcie_do_fatal_recovery and pcie_do_nonfatal_recovery functions made to modularize
> > pcie_do_fatal_recovery now takes service as an argument
I made a few tweaks and pushed the result to pci/oza-v16. The code diffs
are below (I also edited some of the changelogs). If you post a v17,
please start from that branch so you keep the tweaks.
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 6ace47099fc5..ffb956457b4a 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -1535,7 +1535,7 @@ static int pci_uevent(struct device *dev, struct kobj_uevent_env *env)
return 0;
}
-#if defined(CONFIG_PCIEAER) || defined(CONFIG_EEH)
+#if defined(CONFIG_PCIEPORTBUS) || defined(CONFIG_EEH)
/**
* pci_uevent_ers - emit a uevent during recovery path of PCI device
* @pdev: PCI device undergoing error recovery
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index adfc55347535..764bf64a097d 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4139,9 +4139,9 @@ static int pci_pm_reset(struct pci_dev *dev, int probe)
return pci_dev_wait(dev, "PM D3->D0", PCIE_RESET_READY_POLL_MS);
}
/**
- * pcie_wait_for_link - Wait for link till it's active?/inactive?
+ * pcie_wait_for_link - Wait until link is active or inactive
* @pdev: Bridge device
- * @active: waiting for active or inactive ?
+ * @active: waiting for active or inactive?
*
* Use this to wait till link becomes active or inactive.
*/
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 358b4324b9a2..6064041409d2 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -84,15 +84,14 @@ static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
* DPC disables the Link automatically in hardware, so it has
* already been reset by the time we get here.
*/
-
devdpc = pcie_port_find_device(pdev, PCIE_PORT_SERVICE_DPC);
pciedev = to_pcie_device(devdpc);
dpc = get_service_data(pciedev);
cap = dpc->cap_pos;
/*
- * Waiting until the link is inactive, then clearing DPC
- * trigger status to allow the port to leave DPC.
+ * Wait until the Link is inactive, then clear DPC Trigger Status
+ * to allow the Port to leave DPC.
*/
dpc_wait_link_inactive(dpc);
@@ -119,7 +118,7 @@ static void dpc_work(struct work_struct *work)
struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
struct pci_dev *pdev = dpc->dev->port;
- /* From DPC point of view error is always FATAL. */
+ /* We configure DPC so it only triggers on ERR_FATAL */
pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_DPC);
}
diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index 29ff1488e516..d4d869f68acf 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -8,7 +8,6 @@
* Copyright (C) 2006 Intel Corp.
* Tom Long Nguyen (tom.l.nguyen@intel.com)
* Zhang Yanmin (yanmin.zhang@intel.com)
- *
*/
#include <linux/pci.h>
@@ -95,9 +94,7 @@ static int report_error_detected(struct pci_dev *dev, void *data)
} else {
err_handler = dev->driver->err_handler;
vote = err_handler->error_detected(dev, result_data->state);
-#if defined(CONFIG_PCIEAER)
pci_uevent_ers(dev, PCI_ERS_RESULT_NONE);
-#endif
}
result_data->result = merge_result(result_data->result, vote);
@@ -163,9 +160,7 @@ static int report_resume(struct pci_dev *dev, void *data)
err_handler = dev->driver->err_handler;
err_handler->resume(dev);
-#if defined(CONFIG_PCIEAER)
pci_uevent_ers(dev, PCI_ERS_RESULT_RECOVERED);
-#endif
out:
device_unlock(&dev->dev);
return 0;
@@ -189,7 +184,7 @@ static pci_ers_result_t reset_link(struct pci_dev *dev, u32 service)
{
struct pci_dev *udev;
pci_ers_result_t status;
- struct pcie_port_service_driver *driver;
+ struct pcie_port_service_driver *driver = NULL;
if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
/* Reset this port for all subordinates */
@@ -283,16 +278,15 @@ static pci_ers_result_t broadcast_error_message(struct pci_dev *dev,
* @dev: pointer to a pci_dev data structure of agent detecting an error
*
* Invoked when an error is fatal. Once being invoked, removes the devices
- * benetah this AER agent, followed by reset link e.g. secondary bus reset
+ * beneath this AER agent, followed by reset link e.g. secondary bus reset
* followed by re-enumeration of devices.
*/
-
void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
{
struct pci_dev *udev;
struct pci_bus *parent;
struct pci_dev *pdev, *temp;
- pci_ers_result_t result = PCI_ERS_RESULT_RECOVERED;
+ pci_ers_result_t result;
struct aer_broadcast_data result_data;
if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
@@ -303,7 +297,7 @@ void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
parent = udev->subordinate;
pci_lock_rescan_remove();
list_for_each_entry_safe_reverse(pdev, temp, &parent->devices,
- bus_list) {
+ bus_list) {
pci_dev_get(pdev);
pci_dev_set_disconnected(pdev, NULL);
if (pci_has_subordinate(pdev))
@@ -329,12 +323,10 @@ void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
if (result == PCI_ERS_RESULT_RECOVERED) {
if (pcie_wait_for_link(udev, true))
pci_rescan_bus(udev->bus);
- pci_info(dev, "Device recovery successful\n");
+ pci_info(dev, "Device recovery from fatal error successful\n");
} else {
-#if defined(CONFIG_PCIEAER)
pci_uevent_ers(dev, PCI_ERS_RESULT_DISCONNECT);
-#endif
- pci_info(dev, "Device recovery failed\n");
+ pci_info(dev, "Device recovery from fatal error failed\n");
}
pci_unlock_rescan_remove();
@@ -390,9 +382,8 @@ void pcie_do_nonfatal_recovery(struct pci_dev *dev)
return;
failed:
-#if defined(CONFIG_PCIEAER)
pci_uevent_ers(dev, PCI_ERS_RESULT_DISCONNECT);
-#endif
+
/* TODO: Should kernel panic here? */
pci_info(dev, "AER: Device recovery failed\n");
}
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index bc2c3375d363..a5b3b3ae6ab0 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -425,7 +425,7 @@ static int find_service_iter(struct device *device, void *data)
}
/**
* pcie_port_find_service - find the service driver
- * @dev: PCI Express port the service devices associated with
+ * @dev: PCI Express port the service is associated with
* @service: Service to find
*
* Find PCI Express port service driver associated with given service
@@ -446,10 +446,10 @@ struct pcie_port_service_driver *pcie_port_find_service(struct pci_dev *dev,
/**
* pcie_port_find_device - find the struct device
- * @dev: PCI Express port the service devices associated with
+ * @dev: PCI Express port the service is associated with
* @service: For the service to find
*
- * Find PCI Express port service driver associated with given service
+ * Find the struct device associated with given service on a pci_dev
*/
struct device *pcie_port_find_device(struct pci_dev *dev,
u32 service)
diff --git a/include/linux/aer.h b/include/linux/aer.h
index 0c506fe9eff5..514bffa11dbb 100644
--- a/include/linux/aer.h
+++ b/include/linux/aer.h
@@ -14,7 +14,7 @@
#define AER_NONFATAL 0
#define AER_FATAL 1
#define AER_CORRECTABLE 2
-#define DPC_FATAL 4
+#define DPC_FATAL 3
struct pci_dev;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 73178a2fcee0..4f721f757363 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2284,7 +2284,7 @@ static inline bool pci_is_thunderbolt_attached(struct pci_dev *pdev)
return false;
}
-#if defined(CONFIG_PCIEAER) || defined(CONFIG_EEH)
+#if defined(CONFIG_PCIEPORTBUS) || defined(CONFIG_EEH)
void pci_uevent_ers(struct pci_dev *pdev, enum pci_ers_result err_type);
#endif
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v16 3/9] PCI/AER: Handle ERR_FATAL with removal and re-enumeration of devices
2018-05-15 23:59 ` Bjorn Helgaas
@ 2018-05-16 5:49 ` poza
0 siblings, 0 replies; 29+ messages in thread
From: poza @ 2018-05-16 5:49 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Bjorn Helgaas, Philippe Ombredanne, Thomas Gleixner,
Greg Kroah-Hartman, Kate Stewart, linux-pci, linux-kernel,
Dongdong Liu, Keith Busch, Wei Zhang, Sinan Kaya, Timur Tabi,
linux-pci-owner
On 2018-05-16 05:29, Bjorn Helgaas wrote:
> On Fri, May 11, 2018 at 06:43:22AM -0400, Oza Pawandeep wrote:
>> This patch alters the behavior of handling of ERR_FATAL, where removal
>> of devices is initiated, followed by reset link, followed by
>> re-enumeration.
>>
>> So the errors are handled in a different way as follows:
>> ERR_NONFATAL => call driver recovery entry points
>> ERR_FATAL => remove and re-enumerate
>>
>> please refer to Documentation/PCI/pci-error-recovery.txt for more
>> details.
>>
>> Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
>> Reviewed-by: Keith Busch <keith.busch@intel.com>
>>
>> diff --git a/drivers/pci/pcie/aer/aerdrv_core.c
>> b/drivers/pci/pcie/aer/aerdrv_core.c
>> index 0ea5acc..649dd1f 100644
>> --- a/drivers/pci/pcie/aer/aerdrv_core.c
>> +++ b/drivers/pci/pcie/aer/aerdrv_core.c
>> @@ -20,6 +20,7 @@
>> #include <linux/slab.h>
>> #include <linux/kfifo.h>
>> #include "aerdrv.h"
>> +#include "../../pci.h"
>>
>> #define PCI_EXP_AER_FLAGS (PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVCTL_NFERE
>> | \
>> PCI_EXP_DEVCTL_FERE | PCI_EXP_DEVCTL_URRE)
>> @@ -475,35 +476,84 @@ static pci_ers_result_t reset_link(struct
>> pci_dev *dev)
>> }
>>
>> /**
>> - * do_recovery - handle nonfatal/fatal error recovery process
>> + * do_fatal_recovery - handle fatal error recovery process
>> + * @dev: pointer to a pci_dev data structure of agent detecting an
>> error
>> + *
>> + * Invoked when an error is fatal. Once being invoked, removes the
>> devices
>> + * benetah this AER agent, followed by reset link e.g. secondary bus
>> reset
>> + * followed by re-enumeration of devices.
>> + */
>> +
>> +static void do_fatal_recovery(struct pci_dev *dev)
>> +{
>> + struct pci_dev *udev;
>> + struct pci_bus *parent;
>> + struct pci_dev *pdev, *temp;
>> + pci_ers_result_t result = PCI_ERS_RESULT_RECOVERED;
>> + struct aer_broadcast_data result_data;
>> +
>> + if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
>> + udev = dev;
>> + else
>> + udev = dev->bus->self;
>> +
>> + parent = udev->subordinate;
>> + pci_lock_rescan_remove();
>> + list_for_each_entry_safe_reverse(pdev, temp, &parent->devices,
>> + bus_list) {
>> + pci_dev_get(pdev);
>> + pci_dev_set_disconnected(pdev, NULL);
>> + if (pci_has_subordinate(pdev))
>> + pci_walk_bus(pdev->subordinate,
>> + pci_dev_set_disconnected, NULL);
>> + pci_stop_and_remove_bus_device(pdev);
>> + pci_dev_put(pdev);
>> + }
>> +
>> + result = reset_link(udev);
>
> I don't like the fact that for DPC, the link reset happens before we
> call
> the driver .remove() methods, while for AER, the reset happens *after*
> the
> .remove() methods. That means the .remove() methods may work
> differently
> for AER vs. DPC, e.g., they may be able to access the device if AER is
> handling the error, but they would not be able to access it if DPC is
> handling it.
>
> I don't know how to fix this, and I think we can keep this patch as it
> is
> for now, but I think we should fix it eventually.
point noted, will see to this eventually.
>
>> + if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
>> + /*
>> + * If the error is reported by a bridge, we think this error
>> + * is related to the downstream link of the bridge, so we
>> + * do error recovery on all subordinates of the bridge instead
>> + * of the bridge and clear the error status of the bridge.
>> + */
>> + pci_walk_bus(dev->subordinate, report_resume, &result_data);
>> + pci_cleanup_aer_uncorrect_error_status(dev);
>> + }
>> +
>> + if (result == PCI_ERS_RESULT_RECOVERED) {
>> + if (pcie_wait_for_link(udev, true))
>> + pci_rescan_bus(udev->bus);
>> + } else {
>> + pci_uevent_ers(dev, PCI_ERS_RESULT_DISCONNECT);
>> + pci_info(dev, "AER: Device recovery failed\n");
>> + }
>> +
>> + pci_unlock_rescan_remove();
>> +}
>> +
>> +/**
>> + * do_nonfatal_recovery - handle nonfatal error recovery process
>> * @dev: pointer to a pci_dev data structure of agent detecting an
>> error
>> - * @severity: error severity type
>> *
>> * Invoked when an error is nonfatal/fatal. Once being invoked,
>> broadcast
>> * error detected message to all downstream drivers within a
>> hierarchy in
>> * question and return the returned code.
>> */
>> -static void do_recovery(struct pci_dev *dev, int severity)
>> +static void do_nonfatal_recovery(struct pci_dev *dev)
>> {
>> - pci_ers_result_t status, result = PCI_ERS_RESULT_RECOVERED;
>> + pci_ers_result_t status;
>> enum pci_channel_state state;
>>
>> - if (severity == AER_FATAL)
>> - state = pci_channel_io_frozen;
>> - else
>> - state = pci_channel_io_normal;
>> + state = pci_channel_io_normal;
>>
>> status = broadcast_error_message(dev,
>> state,
>> "error_detected",
>> report_error_detected);
>>
>> - if (severity == AER_FATAL) {
>> - result = reset_link(dev);
>> - if (result != PCI_ERS_RESULT_RECOVERED)
>> - goto failed;
>> - }
>> -
>> if (status == PCI_ERS_RESULT_CAN_RECOVER)
>> status = broadcast_error_message(dev,
>> state,
>> @@ -562,8 +612,10 @@ static void handle_error_source(struct
>> pcie_device *aerdev,
>> if (pos)
>> pci_write_config_dword(dev, pos + PCI_ERR_COR_STATUS,
>> info->status);
>> - } else
>> - do_recovery(dev, info->severity);
>> + } else if (info->severity == AER_NONFATAL)
>> + do_nonfatal_recovery(dev);
>> + else if (info->severity == AER_FATAL)
>> + do_fatal_recovery(dev);
>> }
>>
>> #ifdef CONFIG_ACPI_APEI_PCIEAER
>> @@ -627,8 +679,10 @@ static void aer_recover_work_func(struct
>> work_struct *work)
>> continue;
>> }
>> cper_print_aer(pdev, entry.severity, entry.regs);
>> - if (entry.severity != AER_CORRECTABLE)
>> - do_recovery(pdev, entry.severity);
>> + if (entry.severity == AER_NONFATAL)
>> + do_nonfatal_recovery(pdev);
>> + else if (entry.severity == AER_FATAL)
>> + do_fatal_recovery(pdev);
>> pci_dev_put(pdev);
>> }
>> }
>> --
>> 2.7.4
>>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v16 8/9] PCI/DPC: Unify and plumb error handling into DPC
2018-05-15 23:56 ` Bjorn Helgaas
@ 2018-05-16 8:16 ` poza
2018-05-16 10:52 ` Bjorn Helgaas
0 siblings, 1 reply; 29+ messages in thread
From: poza @ 2018-05-16 8:16 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Bjorn Helgaas, Philippe Ombredanne, Thomas Gleixner,
Greg Kroah-Hartman, Kate Stewart, linux-pci, linux-kernel,
Dongdong Liu, Keith Busch, Wei Zhang, Sinan Kaya, Timur Tabi
On 2018-05-16 05:26, Bjorn Helgaas wrote:
> On Fri, May 11, 2018 at 05:22:08PM +0530, poza@codeaurora.org wrote:
>> On 2018-05-11 16:13, Oza Pawandeep wrote:
>> > DPC driver implements link_reset callback, and calls
>> > pci_do_fatal_recovery().
>> >
>> > Which follows standard path of ERR_FATAL recovery.
>> >
>> > Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
>> >
>> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> > index 5e8857a..6af7595 100644
>> > --- a/drivers/pci/pci.h
>> > +++ b/drivers/pci/pci.h
>> > @@ -354,7 +354,7 @@ static inline resource_size_t
>> > pci_resource_alignment(struct pci_dev *dev,
>> > void pci_enable_acs(struct pci_dev *dev);
>> >
>> > /* PCI error reporting and recovery */
>> > -void pcie_do_fatal_recovery(struct pci_dev *dev);
>> > +void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service);
>> > void pcie_do_nonfatal_recovery(struct pci_dev *dev);
>> >
>> > bool pcie_wait_for_link(struct pci_dev *pdev, bool active);
>> > diff --git a/drivers/pci/pcie/aer/aerdrv_core.c
>> > b/drivers/pci/pcie/aer/aerdrv_core.c
>> > index fdfc474..36e622d 100644
>> > --- a/drivers/pci/pcie/aer/aerdrv_core.c
>> > +++ b/drivers/pci/pcie/aer/aerdrv_core.c
>> > @@ -254,7 +254,7 @@ static void handle_error_source(struct pcie_device
>> > *aerdev,
>> > } else if (info->severity == AER_NONFATAL)
>> > pcie_do_nonfatal_recovery(dev);
>> > else if (info->severity == AER_FATAL)
>> > - pcie_do_fatal_recovery(dev);
>> > + pcie_do_fatal_recovery(dev, PCIE_PORT_SERVICE_AER);
>> > }
>> >
>> > #ifdef CONFIG_ACPI_APEI_PCIEAER
>> > @@ -321,7 +321,7 @@ static void aer_recover_work_func(struct work_struct
>> > *work)
>> > if (entry.severity == AER_NONFATAL)
>> > pcie_do_nonfatal_recovery(pdev);
>> > else if (entry.severity == AER_FATAL)
>> > - pcie_do_fatal_recovery(pdev);
>> > + pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_AER);
>> > pci_dev_put(pdev);
>> > }
>> > }
>> > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
>> > index 80ec384..5680c13 100644
>> > --- a/drivers/pci/pcie/dpc.c
>> > +++ b/drivers/pci/pcie/dpc.c
>> > @@ -73,29 +73,31 @@ static void dpc_wait_link_inactive(struct dpc_dev
>> > *dpc)
>> > pcie_wait_for_link(pdev, false);
>> > }
>> >
>> > -static void dpc_work(struct work_struct *work)
>> > +static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
>> > {
>> > - struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
>> > - struct pci_dev *dev, *temp, *pdev = dpc->dev->port;
>> > - struct pci_bus *parent = pdev->subordinate;
>> > - u16 cap = dpc->cap_pos, ctl;
>> > -
>> > - pci_lock_rescan_remove();
>> > - list_for_each_entry_safe_reverse(dev, temp, &parent->devices,
>> > - bus_list) {
>> > - pci_dev_get(dev);
>> > - pci_dev_set_disconnected(dev, NULL);
>> > - if (pci_has_subordinate(dev))
>> > - pci_walk_bus(dev->subordinate,
>> > - pci_dev_set_disconnected, NULL);
>> > - pci_stop_and_remove_bus_device(dev);
>> > - pci_dev_put(dev);
>> > - }
>> > - pci_unlock_rescan_remove();
>> > -
>> > + struct dpc_dev *dpc;
>> > + struct pcie_device *pciedev;
>> > + struct device *devdpc;
>> > + u16 cap, ctl;
>> > +
>> > + /*
>> > + * DPC disables the Link automatically in hardware, so it has
>> > + * already been reset by the time we get here.
>> > + */
>> > +
>> > + devdpc = pcie_port_find_device(pdev, PCIE_PORT_SERVICE_DPC);
>> > + pciedev = to_pcie_device(devdpc);
>> > + dpc = get_service_data(pciedev);
>> > + cap = dpc->cap_pos;
>> > +
>> > + /*
>> > + * Waiting until the link is inactive, then clearing DPC
>> > + * trigger status to allow the port to leave DPC.
>> > + */
>> > dpc_wait_link_inactive(dpc);
>> > +
>> > if (dpc->rp_extensions && dpc_wait_rp_inactive(dpc))
>> > - return;
>> > + return PCI_ERS_RESULT_DISCONNECT;
>> > if (dpc->rp_extensions && dpc->rp_pio_status) {
>> > pci_write_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS,
>> > dpc->rp_pio_status);
>> > @@ -108,6 +110,17 @@ static void dpc_work(struct work_struct *work)
>> > pci_read_config_word(pdev, cap + PCI_EXP_DPC_CTL, &ctl);
>> > pci_write_config_word(pdev, cap + PCI_EXP_DPC_CTL,
>> > ctl | PCI_EXP_DPC_CTL_INT_EN);
>> > +
>> > + return PCI_ERS_RESULT_RECOVERED;
>> > +}
>> > +
>> > +static void dpc_work(struct work_struct *work)
>> > +{
>> > + struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
>> > + struct pci_dev *pdev = dpc->dev->port;
>> > +
>> > + /* From DPC point of view error is always FATAL. */
>> > + pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_DPC);
>> > }
>> >
>> > static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
>> > @@ -288,6 +301,7 @@ static struct pcie_port_service_driver dpcdriver = {
>> > .service = PCIE_PORT_SERVICE_DPC,
>> > .probe = dpc_probe,
>> > .remove = dpc_remove,
>> > + .reset_link = dpc_reset_link,
>> > };
>> >
>> > static int __init dpc_service_init(void)
>> > diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>> > index 33a16b1..29ff148 100644
>> > --- a/drivers/pci/pcie/err.c
>> > +++ b/drivers/pci/pcie/err.c
>> > @@ -185,7 +185,7 @@ static pci_ers_result_t default_reset_link(struct
>> > pci_dev *dev)
>> > return PCI_ERS_RESULT_RECOVERED;
>> > }
>> >
>> > -static pci_ers_result_t reset_link(struct pci_dev *dev)
>> > +static pci_ers_result_t reset_link(struct pci_dev *dev, u32 service)
>> > {
>> > struct pci_dev *udev;
>> > pci_ers_result_t status;
>> > @@ -200,7 +200,7 @@ static pci_ers_result_t reset_link(struct pci_dev
>> > *dev)
>> > }
>> >
>> > /* Use the aer driver of the component firstly */
>> > - driver = pcie_port_find_service(udev, PCIE_PORT_SERVICE_AER);
>> > + driver = pcie_port_find_service(udev, service);
>> >
>> > if (driver && driver->reset_link) {
>> > status = driver->reset_link(udev);
>> > @@ -287,7 +287,7 @@ static pci_ers_result_t
>> > broadcast_error_message(struct pci_dev *dev,
>> > * followed by re-enumeration of devices.
>> > */
>> >
>> > -void pcie_do_fatal_recovery(struct pci_dev *dev)
>> > +void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
>> > {
>> > struct pci_dev *udev;
>> > struct pci_bus *parent;
>> > @@ -313,7 +313,7 @@ void pcie_do_fatal_recovery(struct pci_dev *dev)
>> > pci_dev_put(pdev);
>> > }
>> >
>> > - result = reset_link(udev);
>> > + result = reset_link(udev, service);
>> >
>> > if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
>> > /*
>> > diff --git a/include/linux/aer.h b/include/linux/aer.h
>> > index 8f87bbe..0c506fe 100644
>> > --- a/include/linux/aer.h
>> > +++ b/include/linux/aer.h
>> > @@ -14,6 +14,7 @@
>> > #define AER_NONFATAL 0
>> > #define AER_FATAL 1
>> > #define AER_CORRECTABLE 2
>> > +#define DPC_FATAL 4
>
> I think DPC_FATAL can be 3, since these values are not used as a bit
> mask.
>
>> > struct pci_dev;
>>
>>
>> Hi Bjorn,
>>
>>
>> I have addressed all the comments, and I hope we are heading towards
>> closure.
>>
>> I just figure that pcie_do_fatal_recovery (which is getting executed
>> for
>> DPC as well)
>>
>> if (result == PCI_ERS_RESULT_RECOVERED) {
>> if (pcie_wait_for_link(udev, true))
>> pci_rescan_bus(udev->bus);
>> pci_info(dev, "Device recovery successful\n");
>> }
>>
>> I have to correct it to
>>
>> if (service==AER && result == PCI_ERS_RESULT_RECOVERED) {
>> if (pcie_wait_for_link(udev, true))
>> pci_rescan_bus(udev->bus);
>> pci_info(dev, "Device recovery successful\n");
>> }
>
> This patch is mostly a restructuring of DPC and doesn't really change
> its
> behavior. DPC didn't previously call pcie_wait_for_link() or
> pci_rescan_bus(), so I agree that adding the "service==AER" test will
> help preserve the existing DPC behavior.
>
> However, the rescan should happen with DPC *somewhere* and we should
> clarify where that is. Maybe for now we only need a comment about
> where
> that happens. Ideally, we could eventually converge this so the same
> mechanism is used for AER and DPC, so we wouldn't need a test like
> "service=AER".
>
>> rest of the things look okay to me.
>> If you have any more comments,
>> I can fix them with this one and hopefully v17 will be the final one.
I am sorry I pasted the wrong snippet.
following needs to be fixed in v17.
from:
if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
/*
* If the error is reported by a bridge, we think this
error
* is related to the downstream link of the bridge, so
we
* do error recovery on all subordinates of the bridge
instead
* of the bridge and clear the error status of the
bridge.
*/
pci_walk_bus(dev->subordinate, report_resume,
&result_data);
pci_cleanup_aer_uncorrect_error_status(dev);
}
to
if (service==AER && dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
/*
* If the error is reported by a bridge, we think this
error
* is related to the downstream link of the bridge, so
we
* do error recovery on all subordinates of the bridge
instead
* of the bridge and clear the error status of the
bridge.
*/
pci_walk_bus(dev->subordinate, report_resume,
&result_data);
pci_cleanup_aer_uncorrect_error_status(dev);
}
this is only needed in case of AER.
by the way I can not find pci/oza-v16 branch when I cloned
git clone
https://kernel.googlesource.com/pub/scm/linux/kernel/git/helgaas/pci
Regards,
Oza.
>>
>>
>> PS: I am going though the code more, and we can have some follow up
>> patches
>> (probably some cleanup)
>> for e.g. pcie_portdrv_slot_reset() checks
>> if (dev->error_state == pci_channel_io_frozen) {
>> dev->state_saved = true;
>> pci_restore_state(dev);
>> pcie_portdrv_restore_config(dev);
>> pci_enable_pcie_error_reporting(dev);
>> }
>>
>>
>> but now since the ERR_FATAL path does not call pcie_portdrv_slot_reset
>> the
>> check is meaning less.
>>
>> besides driver's shut_down callbacks might want to handle
>> pci_channel_io_frozen, but that something left to be discussed later.
>> So, I am not touching dev->error_state anywhere as of now in ERR_FATAL
>> case.
>>
>> Regards,
>> Oza.
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v16 8/9] PCI/DPC: Unify and plumb error handling into DPC
2018-05-16 8:16 ` poza
@ 2018-05-16 10:52 ` Bjorn Helgaas
2018-05-16 12:15 ` poza
2018-05-16 12:51 ` poza
0 siblings, 2 replies; 29+ messages in thread
From: Bjorn Helgaas @ 2018-05-16 10:52 UTC (permalink / raw)
To: poza
Cc: Bjorn Helgaas, Philippe Ombredanne, Thomas Gleixner,
Greg Kroah-Hartman, Kate Stewart, linux-pci, linux-kernel,
Dongdong Liu, Keith Busch, Wei Zhang, Sinan Kaya, Timur Tabi
On Wed, May 16, 2018 at 01:46:25PM +0530, poza@codeaurora.org wrote:
> On 2018-05-16 05:26, Bjorn Helgaas wrote:
> > On Fri, May 11, 2018 at 05:22:08PM +0530, poza@codeaurora.org wrote:
> > > On 2018-05-11 16:13, Oza Pawandeep wrote:
> > > > DPC driver implements link_reset callback, and calls
> > > > pci_do_fatal_recovery().
> > > >
> > > > Which follows standard path of ERR_FATAL recovery.
> > > >
> > > > Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
> > > >
> > > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > > > index 5e8857a..6af7595 100644
> > > > --- a/drivers/pci/pci.h
> > > > +++ b/drivers/pci/pci.h
> > > > @@ -354,7 +354,7 @@ static inline resource_size_t
> > > > pci_resource_alignment(struct pci_dev *dev,
> > > > void pci_enable_acs(struct pci_dev *dev);
> > > >
> > > > /* PCI error reporting and recovery */
> > > > -void pcie_do_fatal_recovery(struct pci_dev *dev);
> > > > +void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service);
> > > > void pcie_do_nonfatal_recovery(struct pci_dev *dev);
> > > >
> > > > bool pcie_wait_for_link(struct pci_dev *pdev, bool active);
> > > > diff --git a/drivers/pci/pcie/aer/aerdrv_core.c
> > > > b/drivers/pci/pcie/aer/aerdrv_core.c
> > > > index fdfc474..36e622d 100644
> > > > --- a/drivers/pci/pcie/aer/aerdrv_core.c
> > > > +++ b/drivers/pci/pcie/aer/aerdrv_core.c
> > > > @@ -254,7 +254,7 @@ static void handle_error_source(struct pcie_device
> > > > *aerdev,
> > > > } else if (info->severity == AER_NONFATAL)
> > > > pcie_do_nonfatal_recovery(dev);
> > > > else if (info->severity == AER_FATAL)
> > > > - pcie_do_fatal_recovery(dev);
> > > > + pcie_do_fatal_recovery(dev, PCIE_PORT_SERVICE_AER);
> > > > }
> > > >
> > > > #ifdef CONFIG_ACPI_APEI_PCIEAER
> > > > @@ -321,7 +321,7 @@ static void aer_recover_work_func(struct work_struct
> > > > *work)
> > > > if (entry.severity == AER_NONFATAL)
> > > > pcie_do_nonfatal_recovery(pdev);
> > > > else if (entry.severity == AER_FATAL)
> > > > - pcie_do_fatal_recovery(pdev);
> > > > + pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_AER);
> > > > pci_dev_put(pdev);
> > > > }
> > > > }
> > > > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > > > index 80ec384..5680c13 100644
> > > > --- a/drivers/pci/pcie/dpc.c
> > > > +++ b/drivers/pci/pcie/dpc.c
> > > > @@ -73,29 +73,31 @@ static void dpc_wait_link_inactive(struct dpc_dev
> > > > *dpc)
> > > > pcie_wait_for_link(pdev, false);
> > > > }
> > > >
> > > > -static void dpc_work(struct work_struct *work)
> > > > +static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
> > > > {
> > > > - struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
> > > > - struct pci_dev *dev, *temp, *pdev = dpc->dev->port;
> > > > - struct pci_bus *parent = pdev->subordinate;
> > > > - u16 cap = dpc->cap_pos, ctl;
> > > > -
> > > > - pci_lock_rescan_remove();
> > > > - list_for_each_entry_safe_reverse(dev, temp, &parent->devices,
> > > > - bus_list) {
> > > > - pci_dev_get(dev);
> > > > - pci_dev_set_disconnected(dev, NULL);
> > > > - if (pci_has_subordinate(dev))
> > > > - pci_walk_bus(dev->subordinate,
> > > > - pci_dev_set_disconnected, NULL);
> > > > - pci_stop_and_remove_bus_device(dev);
> > > > - pci_dev_put(dev);
> > > > - }
> > > > - pci_unlock_rescan_remove();
> > > > -
> > > > + struct dpc_dev *dpc;
> > > > + struct pcie_device *pciedev;
> > > > + struct device *devdpc;
> > > > + u16 cap, ctl;
> > > > +
> > > > + /*
> > > > + * DPC disables the Link automatically in hardware, so it has
> > > > + * already been reset by the time we get here.
> > > > + */
> > > > +
> > > > + devdpc = pcie_port_find_device(pdev, PCIE_PORT_SERVICE_DPC);
> > > > + pciedev = to_pcie_device(devdpc);
> > > > + dpc = get_service_data(pciedev);
> > > > + cap = dpc->cap_pos;
> > > > +
> > > > + /*
> > > > + * Waiting until the link is inactive, then clearing DPC
> > > > + * trigger status to allow the port to leave DPC.
> > > > + */
> > > > dpc_wait_link_inactive(dpc);
> > > > +
> > > > if (dpc->rp_extensions && dpc_wait_rp_inactive(dpc))
> > > > - return;
> > > > + return PCI_ERS_RESULT_DISCONNECT;
> > > > if (dpc->rp_extensions && dpc->rp_pio_status) {
> > > > pci_write_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS,
> > > > dpc->rp_pio_status);
> > > > @@ -108,6 +110,17 @@ static void dpc_work(struct work_struct *work)
> > > > pci_read_config_word(pdev, cap + PCI_EXP_DPC_CTL, &ctl);
> > > > pci_write_config_word(pdev, cap + PCI_EXP_DPC_CTL,
> > > > ctl | PCI_EXP_DPC_CTL_INT_EN);
> > > > +
> > > > + return PCI_ERS_RESULT_RECOVERED;
> > > > +}
> > > > +
> > > > +static void dpc_work(struct work_struct *work)
> > > > +{
> > > > + struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
> > > > + struct pci_dev *pdev = dpc->dev->port;
> > > > +
> > > > + /* From DPC point of view error is always FATAL. */
> > > > + pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_DPC);
> > > > }
> > > >
> > > > static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
> > > > @@ -288,6 +301,7 @@ static struct pcie_port_service_driver dpcdriver = {
> > > > .service = PCIE_PORT_SERVICE_DPC,
> > > > .probe = dpc_probe,
> > > > .remove = dpc_remove,
> > > > + .reset_link = dpc_reset_link,
> > > > };
> > > >
> > > > static int __init dpc_service_init(void)
> > > > diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> > > > index 33a16b1..29ff148 100644
> > > > --- a/drivers/pci/pcie/err.c
> > > > +++ b/drivers/pci/pcie/err.c
> > > > @@ -185,7 +185,7 @@ static pci_ers_result_t default_reset_link(struct
> > > > pci_dev *dev)
> > > > return PCI_ERS_RESULT_RECOVERED;
> > > > }
> > > >
> > > > -static pci_ers_result_t reset_link(struct pci_dev *dev)
> > > > +static pci_ers_result_t reset_link(struct pci_dev *dev, u32 service)
> > > > {
> > > > struct pci_dev *udev;
> > > > pci_ers_result_t status;
> > > > @@ -200,7 +200,7 @@ static pci_ers_result_t reset_link(struct pci_dev
> > > > *dev)
> > > > }
> > > >
> > > > /* Use the aer driver of the component firstly */
> > > > - driver = pcie_port_find_service(udev, PCIE_PORT_SERVICE_AER);
> > > > + driver = pcie_port_find_service(udev, service);
> > > >
> > > > if (driver && driver->reset_link) {
> > > > status = driver->reset_link(udev);
> > > > @@ -287,7 +287,7 @@ static pci_ers_result_t
> > > > broadcast_error_message(struct pci_dev *dev,
> > > > * followed by re-enumeration of devices.
> > > > */
> > > >
> > > > -void pcie_do_fatal_recovery(struct pci_dev *dev)
> > > > +void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
> > > > {
> > > > struct pci_dev *udev;
> > > > struct pci_bus *parent;
> > > > @@ -313,7 +313,7 @@ void pcie_do_fatal_recovery(struct pci_dev *dev)
> > > > pci_dev_put(pdev);
> > > > }
> > > >
> > > > - result = reset_link(udev);
> > > > + result = reset_link(udev, service);
> > > >
> > > > if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
> > > > /*
> > > > diff --git a/include/linux/aer.h b/include/linux/aer.h
> > > > index 8f87bbe..0c506fe 100644
> > > > --- a/include/linux/aer.h
> > > > +++ b/include/linux/aer.h
> > > > @@ -14,6 +14,7 @@
> > > > #define AER_NONFATAL 0
> > > > #define AER_FATAL 1
> > > > #define AER_CORRECTABLE 2
> > > > +#define DPC_FATAL 4
> >
> > I think DPC_FATAL can be 3, since these values are not used as a bit
> > mask.
> >
> > > > struct pci_dev;
> > >
> > >
> > > Hi Bjorn,
> > >
> > >
> > > I have addressed all the comments, and I hope we are heading towards
> > > closure.
> > >
> > > I just figure that pcie_do_fatal_recovery (which is getting
> > > executed for
> > > DPC as well)
> > >
> > > if (result == PCI_ERS_RESULT_RECOVERED) {
> > > if (pcie_wait_for_link(udev, true))
> > > pci_rescan_bus(udev->bus);
> > > pci_info(dev, "Device recovery successful\n");
> > > }
> > >
> > > I have to correct it to
> > >
> > > if (service==AER && result == PCI_ERS_RESULT_RECOVERED) {
> > > if (pcie_wait_for_link(udev, true))
> > > pci_rescan_bus(udev->bus);
> > > pci_info(dev, "Device recovery successful\n");
> > > }
> >
> > This patch is mostly a restructuring of DPC and doesn't really change
> > its
> > behavior. DPC didn't previously call pcie_wait_for_link() or
> > pci_rescan_bus(), so I agree that adding the "service==AER" test will
> > help preserve the existing DPC behavior.
> >
> > However, the rescan should happen with DPC *somewhere* and we should
> > clarify where that is. Maybe for now we only need a comment about where
> > that happens. Ideally, we could eventually converge this so the same
> > mechanism is used for AER and DPC, so we wouldn't need a test like
> > "service=AER".
Please still add a comment here about why the rescan behavior is
different.
> I am sorry I pasted the wrong snippet.
> following needs to be fixed in v17.
> from:
> if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
> /*
> * If the error is reported by a bridge, we think this error
> * is related to the downstream link of the bridge, so we
> * do error recovery on all subordinates of the bridge
> instead
> * of the bridge and clear the error status of the bridge.
> */
> pci_walk_bus(dev->subordinate, report_resume, &result_data);
> pci_cleanup_aer_uncorrect_error_status(dev);
> }
>
>
> to
>
> if (service==AER && dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
> /*
> * If the error is reported by a bridge, we think this error
> * is related to the downstream link of the bridge, so we
> * do error recovery on all subordinates of the bridge
> instead
> * of the bridge and clear the error status of the bridge.
> */
> pci_walk_bus(dev->subordinate, report_resume, &result_data);
> pci_cleanup_aer_uncorrect_error_status(dev);
> }
>
> this is only needed in case of AER.
Oh, I missed this before. It makes sense to clear the AER status
here, but why do we need to call report_resume()? We just called all
the driver .remove() methods and detached the drivers from the
devices. So I don't think report_resume() will do anything
("dev->driver" should be NULL) except set the dev->error_state to
pci_channel_io_normal. We probably don't need that because we didn't
change error_state in this fatal error path.
> by the way I can not find pci/oza-v16 branch when I cloned
> git clone
> https://kernel.googlesource.com/pub/scm/linux/kernel/git/helgaas/pci
Sorry, I must have forgotten to push it. It's there now at
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/oza-v16
I don't know anything about the googlesource mirror, so I don't know
when it will get there.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v16 8/9] PCI/DPC: Unify and plumb error handling into DPC
2018-05-16 10:52 ` Bjorn Helgaas
@ 2018-05-16 12:15 ` poza
2018-05-16 13:04 ` Bjorn Helgaas
2018-05-16 12:51 ` poza
1 sibling, 1 reply; 29+ messages in thread
From: poza @ 2018-05-16 12:15 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Bjorn Helgaas, Philippe Ombredanne, Thomas Gleixner,
Greg Kroah-Hartman, Kate Stewart, linux-pci, linux-kernel,
Dongdong Liu, Keith Busch, Wei Zhang, Sinan Kaya, Timur Tabi
On 2018-05-16 16:22, Bjorn Helgaas wrote:
> On Wed, May 16, 2018 at 01:46:25PM +0530, poza@codeaurora.org wrote:
>> On 2018-05-16 05:26, Bjorn Helgaas wrote:
>> > On Fri, May 11, 2018 at 05:22:08PM +0530, poza@codeaurora.org wrote:
>> > > On 2018-05-11 16:13, Oza Pawandeep wrote:
>> > > > DPC driver implements link_reset callback, and calls
>> > > > pci_do_fatal_recovery().
>> > > >
>> > > > Which follows standard path of ERR_FATAL recovery.
>> > > >
>> > > > Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
>> > > >
>> > > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> > > > index 5e8857a..6af7595 100644
>> > > > --- a/drivers/pci/pci.h
>> > > > +++ b/drivers/pci/pci.h
>> > > > @@ -354,7 +354,7 @@ static inline resource_size_t
>> > > > pci_resource_alignment(struct pci_dev *dev,
>> > > > void pci_enable_acs(struct pci_dev *dev);
>> > > >
>> > > > /* PCI error reporting and recovery */
>> > > > -void pcie_do_fatal_recovery(struct pci_dev *dev);
>> > > > +void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service);
>> > > > void pcie_do_nonfatal_recovery(struct pci_dev *dev);
>> > > >
>> > > > bool pcie_wait_for_link(struct pci_dev *pdev, bool active);
>> > > > diff --git a/drivers/pci/pcie/aer/aerdrv_core.c
>> > > > b/drivers/pci/pcie/aer/aerdrv_core.c
>> > > > index fdfc474..36e622d 100644
>> > > > --- a/drivers/pci/pcie/aer/aerdrv_core.c
>> > > > +++ b/drivers/pci/pcie/aer/aerdrv_core.c
>> > > > @@ -254,7 +254,7 @@ static void handle_error_source(struct pcie_device
>> > > > *aerdev,
>> > > > } else if (info->severity == AER_NONFATAL)
>> > > > pcie_do_nonfatal_recovery(dev);
>> > > > else if (info->severity == AER_FATAL)
>> > > > - pcie_do_fatal_recovery(dev);
>> > > > + pcie_do_fatal_recovery(dev, PCIE_PORT_SERVICE_AER);
>> > > > }
>> > > >
>> > > > #ifdef CONFIG_ACPI_APEI_PCIEAER
>> > > > @@ -321,7 +321,7 @@ static void aer_recover_work_func(struct work_struct
>> > > > *work)
>> > > > if (entry.severity == AER_NONFATAL)
>> > > > pcie_do_nonfatal_recovery(pdev);
>> > > > else if (entry.severity == AER_FATAL)
>> > > > - pcie_do_fatal_recovery(pdev);
>> > > > + pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_AER);
>> > > > pci_dev_put(pdev);
>> > > > }
>> > > > }
>> > > > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
>> > > > index 80ec384..5680c13 100644
>> > > > --- a/drivers/pci/pcie/dpc.c
>> > > > +++ b/drivers/pci/pcie/dpc.c
>> > > > @@ -73,29 +73,31 @@ static void dpc_wait_link_inactive(struct dpc_dev
>> > > > *dpc)
>> > > > pcie_wait_for_link(pdev, false);
>> > > > }
>> > > >
>> > > > -static void dpc_work(struct work_struct *work)
>> > > > +static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
>> > > > {
>> > > > - struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
>> > > > - struct pci_dev *dev, *temp, *pdev = dpc->dev->port;
>> > > > - struct pci_bus *parent = pdev->subordinate;
>> > > > - u16 cap = dpc->cap_pos, ctl;
>> > > > -
>> > > > - pci_lock_rescan_remove();
>> > > > - list_for_each_entry_safe_reverse(dev, temp, &parent->devices,
>> > > > - bus_list) {
>> > > > - pci_dev_get(dev);
>> > > > - pci_dev_set_disconnected(dev, NULL);
>> > > > - if (pci_has_subordinate(dev))
>> > > > - pci_walk_bus(dev->subordinate,
>> > > > - pci_dev_set_disconnected, NULL);
>> > > > - pci_stop_and_remove_bus_device(dev);
>> > > > - pci_dev_put(dev);
>> > > > - }
>> > > > - pci_unlock_rescan_remove();
>> > > > -
>> > > > + struct dpc_dev *dpc;
>> > > > + struct pcie_device *pciedev;
>> > > > + struct device *devdpc;
>> > > > + u16 cap, ctl;
>> > > > +
>> > > > + /*
>> > > > + * DPC disables the Link automatically in hardware, so it has
>> > > > + * already been reset by the time we get here.
>> > > > + */
>> > > > +
>> > > > + devdpc = pcie_port_find_device(pdev, PCIE_PORT_SERVICE_DPC);
>> > > > + pciedev = to_pcie_device(devdpc);
>> > > > + dpc = get_service_data(pciedev);
>> > > > + cap = dpc->cap_pos;
>> > > > +
>> > > > + /*
>> > > > + * Waiting until the link is inactive, then clearing DPC
>> > > > + * trigger status to allow the port to leave DPC.
>> > > > + */
>> > > > dpc_wait_link_inactive(dpc);
>> > > > +
>> > > > if (dpc->rp_extensions && dpc_wait_rp_inactive(dpc))
>> > > > - return;
>> > > > + return PCI_ERS_RESULT_DISCONNECT;
>> > > > if (dpc->rp_extensions && dpc->rp_pio_status) {
>> > > > pci_write_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS,
>> > > > dpc->rp_pio_status);
>> > > > @@ -108,6 +110,17 @@ static void dpc_work(struct work_struct *work)
>> > > > pci_read_config_word(pdev, cap + PCI_EXP_DPC_CTL, &ctl);
>> > > > pci_write_config_word(pdev, cap + PCI_EXP_DPC_CTL,
>> > > > ctl | PCI_EXP_DPC_CTL_INT_EN);
>> > > > +
>> > > > + return PCI_ERS_RESULT_RECOVERED;
>> > > > +}
>> > > > +
>> > > > +static void dpc_work(struct work_struct *work)
>> > > > +{
>> > > > + struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
>> > > > + struct pci_dev *pdev = dpc->dev->port;
>> > > > +
>> > > > + /* From DPC point of view error is always FATAL. */
>> > > > + pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_DPC);
>> > > > }
>> > > >
>> > > > static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
>> > > > @@ -288,6 +301,7 @@ static struct pcie_port_service_driver dpcdriver = {
>> > > > .service = PCIE_PORT_SERVICE_DPC,
>> > > > .probe = dpc_probe,
>> > > > .remove = dpc_remove,
>> > > > + .reset_link = dpc_reset_link,
>> > > > };
>> > > >
>> > > > static int __init dpc_service_init(void)
>> > > > diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>> > > > index 33a16b1..29ff148 100644
>> > > > --- a/drivers/pci/pcie/err.c
>> > > > +++ b/drivers/pci/pcie/err.c
>> > > > @@ -185,7 +185,7 @@ static pci_ers_result_t default_reset_link(struct
>> > > > pci_dev *dev)
>> > > > return PCI_ERS_RESULT_RECOVERED;
>> > > > }
>> > > >
>> > > > -static pci_ers_result_t reset_link(struct pci_dev *dev)
>> > > > +static pci_ers_result_t reset_link(struct pci_dev *dev, u32 service)
>> > > > {
>> > > > struct pci_dev *udev;
>> > > > pci_ers_result_t status;
>> > > > @@ -200,7 +200,7 @@ static pci_ers_result_t reset_link(struct pci_dev
>> > > > *dev)
>> > > > }
>> > > >
>> > > > /* Use the aer driver of the component firstly */
>> > > > - driver = pcie_port_find_service(udev, PCIE_PORT_SERVICE_AER);
>> > > > + driver = pcie_port_find_service(udev, service);
>> > > >
>> > > > if (driver && driver->reset_link) {
>> > > > status = driver->reset_link(udev);
>> > > > @@ -287,7 +287,7 @@ static pci_ers_result_t
>> > > > broadcast_error_message(struct pci_dev *dev,
>> > > > * followed by re-enumeration of devices.
>> > > > */
>> > > >
>> > > > -void pcie_do_fatal_recovery(struct pci_dev *dev)
>> > > > +void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
>> > > > {
>> > > > struct pci_dev *udev;
>> > > > struct pci_bus *parent;
>> > > > @@ -313,7 +313,7 @@ void pcie_do_fatal_recovery(struct pci_dev *dev)
>> > > > pci_dev_put(pdev);
>> > > > }
>> > > >
>> > > > - result = reset_link(udev);
>> > > > + result = reset_link(udev, service);
>> > > >
>> > > > if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
>> > > > /*
>> > > > diff --git a/include/linux/aer.h b/include/linux/aer.h
>> > > > index 8f87bbe..0c506fe 100644
>> > > > --- a/include/linux/aer.h
>> > > > +++ b/include/linux/aer.h
>> > > > @@ -14,6 +14,7 @@
>> > > > #define AER_NONFATAL 0
>> > > > #define AER_FATAL 1
>> > > > #define AER_CORRECTABLE 2
>> > > > +#define DPC_FATAL 4
>> >
>> > I think DPC_FATAL can be 3, since these values are not used as a bit
>> > mask.
>> >
>> > > > struct pci_dev;
>> > >
>> > >
>> > > Hi Bjorn,
>> > >
>> > >
>> > > I have addressed all the comments, and I hope we are heading towards
>> > > closure.
>> > >
>> > > I just figure that pcie_do_fatal_recovery (which is getting
>> > > executed for
>> > > DPC as well)
>> > >
>> > > if (result == PCI_ERS_RESULT_RECOVERED) {
>> > > if (pcie_wait_for_link(udev, true))
>> > > pci_rescan_bus(udev->bus);
>> > > pci_info(dev, "Device recovery successful\n");
>> > > }
>> > >
>> > > I have to correct it to
>> > >
>> > > if (service==AER && result == PCI_ERS_RESULT_RECOVERED) {
>> > > if (pcie_wait_for_link(udev, true))
>> > > pci_rescan_bus(udev->bus);
>> > > pci_info(dev, "Device recovery successful\n");
>> > > }
>> >
>> > This patch is mostly a restructuring of DPC and doesn't really change
>> > its
>> > behavior. DPC didn't previously call pcie_wait_for_link() or
>> > pci_rescan_bus(), so I agree that adding the "service==AER" test will
>> > help preserve the existing DPC behavior.
>> >
>> > However, the rescan should happen with DPC *somewhere* and we should
>> > clarify where that is. Maybe for now we only need a comment about where
>> > that happens. Ideally, we could eventually converge this so the same
>> > mechanism is used for AER and DPC, so we wouldn't need a test like
>> > "service=AER".
>
> Please still add a comment here about why the rescan behavior is
> different.
>
>> I am sorry I pasted the wrong snippet.
>> following needs to be fixed in v17.
>> from:
>> if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
>> /*
>> * If the error is reported by a bridge, we think this
>> error
>> * is related to the downstream link of the bridge, so
>> we
>> * do error recovery on all subordinates of the bridge
>> instead
>> * of the bridge and clear the error status of the
>> bridge.
>> */
>> pci_walk_bus(dev->subordinate, report_resume,
>> &result_data);
>> pci_cleanup_aer_uncorrect_error_status(dev);
>> }
>>
>>
>> to
>>
>> if (service==AER && dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
>> /*
>> * If the error is reported by a bridge, we think this
>> error
>> * is related to the downstream link of the bridge, so
>> we
>> * do error recovery on all subordinates of the bridge
>> instead
>> * of the bridge and clear the error status of the
>> bridge.
>> */
>> pci_walk_bus(dev->subordinate, report_resume,
>> &result_data);
>> pci_cleanup_aer_uncorrect_error_status(dev);
>> }
>>
>> this is only needed in case of AER.
>
> Oh, I missed this before. It makes sense to clear the AER status
> here, but why do we need to call report_resume()? We just called all
> the driver .remove() methods and detached the drivers from the
> devices. So I don't think report_resume() will do anything
> ("dev->driver" should be NULL) except set the dev->error_state to
> pci_channel_io_normal. We probably don't need that because we didn't
> change error_state in this fatal error path.
>
if you remember, the path ends up calling
aer_error_resume
the existing code ends up calling aer_error_resume as follows.
do_recovery(pci_dev)
broadcast_error_message(..., error_detected, ...)
if (AER_FATAL)
reset_link(pci_dev)
udev = BRIDGE ? pci_dev : pci_dev->bus->self
driver->reset_link(udev)
aer_root_reset(udev)
if (CAN_RECOVER)
broadcast_error_message(..., mmio_enabled, ...)
if (NEED_RESET)
broadcast_error_message(..., slot_reset, ...)
broadcast_error_message(dev, ..., report_resume, ...)
if (BRIDGE)
report_resume
driver->resume
pcie_portdrv_err_resume
device_for_each_child(..., resume_iter)
resume_iter
driver->error_resume
aer_error_resume
pci_cleanup_aer_uncorrect_error_status(pci_dev) # only if
BRIDGE
pci_write_config_dword(PCI_ERR_UNCOR_STATUS)
hence I think we have to call it in order to clear the root port
PCI_ERR_UNCOR_STATUS and PCI_EXP_DEVSTA.
makes sense ?
Regards,
Oza.
>> by the way I can not find pci/oza-v16 branch when I cloned
>> git clone
>> https://kernel.googlesource.com/pub/scm/linux/kernel/git/helgaas/pci
>
> Sorry, I must have forgotten to push it. It's there now at
>
>
> https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/oza-v16
>
> I don't know anything about the googlesource mirror, so I don't know
> when it will get there.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v16 8/9] PCI/DPC: Unify and plumb error handling into DPC
2018-05-16 10:52 ` Bjorn Helgaas
2018-05-16 12:15 ` poza
@ 2018-05-16 12:51 ` poza
2018-05-16 13:09 ` Bjorn Helgaas
1 sibling, 1 reply; 29+ messages in thread
From: poza @ 2018-05-16 12:51 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Bjorn Helgaas, Philippe Ombredanne, Thomas Gleixner,
Greg Kroah-Hartman, Kate Stewart, linux-pci, linux-kernel,
Dongdong Liu, Keith Busch, Wei Zhang, Sinan Kaya, Timur Tabi
On 2018-05-16 16:22, Bjorn Helgaas wrote:
> On Wed, May 16, 2018 at 01:46:25PM +0530, poza@codeaurora.org wrote:
>> On 2018-05-16 05:26, Bjorn Helgaas wrote:
>> > On Fri, May 11, 2018 at 05:22:08PM +0530, poza@codeaurora.org wrote:
>> > > On 2018-05-11 16:13, Oza Pawandeep wrote:
>> > > > DPC driver implements link_reset callback, and calls
>> > > > pci_do_fatal_recovery().
>> > > >
>> > > > Which follows standard path of ERR_FATAL recovery.
>> > > >
>> > > > Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
>> > > >
>> > > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> > > > index 5e8857a..6af7595 100644
>> > > > --- a/drivers/pci/pci.h
>> > > > +++ b/drivers/pci/pci.h
>> > > > @@ -354,7 +354,7 @@ static inline resource_size_t
>> > > > pci_resource_alignment(struct pci_dev *dev,
>> > > > void pci_enable_acs(struct pci_dev *dev);
>> > > >
>> > > > /* PCI error reporting and recovery */
>> > > > -void pcie_do_fatal_recovery(struct pci_dev *dev);
>> > > > +void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service);
>> > > > void pcie_do_nonfatal_recovery(struct pci_dev *dev);
>> > > >
>> > > > bool pcie_wait_for_link(struct pci_dev *pdev, bool active);
>> > > > diff --git a/drivers/pci/pcie/aer/aerdrv_core.c
>> > > > b/drivers/pci/pcie/aer/aerdrv_core.c
>> > > > index fdfc474..36e622d 100644
>> > > > --- a/drivers/pci/pcie/aer/aerdrv_core.c
>> > > > +++ b/drivers/pci/pcie/aer/aerdrv_core.c
>> > > > @@ -254,7 +254,7 @@ static void handle_error_source(struct pcie_device
>> > > > *aerdev,
>> > > > } else if (info->severity == AER_NONFATAL)
>> > > > pcie_do_nonfatal_recovery(dev);
>> > > > else if (info->severity == AER_FATAL)
>> > > > - pcie_do_fatal_recovery(dev);
>> > > > + pcie_do_fatal_recovery(dev, PCIE_PORT_SERVICE_AER);
>> > > > }
>> > > >
>> > > > #ifdef CONFIG_ACPI_APEI_PCIEAER
>> > > > @@ -321,7 +321,7 @@ static void aer_recover_work_func(struct work_struct
>> > > > *work)
>> > > > if (entry.severity == AER_NONFATAL)
>> > > > pcie_do_nonfatal_recovery(pdev);
>> > > > else if (entry.severity == AER_FATAL)
>> > > > - pcie_do_fatal_recovery(pdev);
>> > > > + pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_AER);
>> > > > pci_dev_put(pdev);
>> > > > }
>> > > > }
>> > > > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
>> > > > index 80ec384..5680c13 100644
>> > > > --- a/drivers/pci/pcie/dpc.c
>> > > > +++ b/drivers/pci/pcie/dpc.c
>> > > > @@ -73,29 +73,31 @@ static void dpc_wait_link_inactive(struct dpc_dev
>> > > > *dpc)
>> > > > pcie_wait_for_link(pdev, false);
>> > > > }
>> > > >
>> > > > -static void dpc_work(struct work_struct *work)
>> > > > +static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
>> > > > {
>> > > > - struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
>> > > > - struct pci_dev *dev, *temp, *pdev = dpc->dev->port;
>> > > > - struct pci_bus *parent = pdev->subordinate;
>> > > > - u16 cap = dpc->cap_pos, ctl;
>> > > > -
>> > > > - pci_lock_rescan_remove();
>> > > > - list_for_each_entry_safe_reverse(dev, temp, &parent->devices,
>> > > > - bus_list) {
>> > > > - pci_dev_get(dev);
>> > > > - pci_dev_set_disconnected(dev, NULL);
>> > > > - if (pci_has_subordinate(dev))
>> > > > - pci_walk_bus(dev->subordinate,
>> > > > - pci_dev_set_disconnected, NULL);
>> > > > - pci_stop_and_remove_bus_device(dev);
>> > > > - pci_dev_put(dev);
>> > > > - }
>> > > > - pci_unlock_rescan_remove();
>> > > > -
>> > > > + struct dpc_dev *dpc;
>> > > > + struct pcie_device *pciedev;
>> > > > + struct device *devdpc;
>> > > > + u16 cap, ctl;
>> > > > +
>> > > > + /*
>> > > > + * DPC disables the Link automatically in hardware, so it has
>> > > > + * already been reset by the time we get here.
>> > > > + */
>> > > > +
>> > > > + devdpc = pcie_port_find_device(pdev, PCIE_PORT_SERVICE_DPC);
>> > > > + pciedev = to_pcie_device(devdpc);
>> > > > + dpc = get_service_data(pciedev);
>> > > > + cap = dpc->cap_pos;
>> > > > +
>> > > > + /*
>> > > > + * Waiting until the link is inactive, then clearing DPC
>> > > > + * trigger status to allow the port to leave DPC.
>> > > > + */
>> > > > dpc_wait_link_inactive(dpc);
>> > > > +
>> > > > if (dpc->rp_extensions && dpc_wait_rp_inactive(dpc))
>> > > > - return;
>> > > > + return PCI_ERS_RESULT_DISCONNECT;
>> > > > if (dpc->rp_extensions && dpc->rp_pio_status) {
>> > > > pci_write_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS,
>> > > > dpc->rp_pio_status);
>> > > > @@ -108,6 +110,17 @@ static void dpc_work(struct work_struct *work)
>> > > > pci_read_config_word(pdev, cap + PCI_EXP_DPC_CTL, &ctl);
>> > > > pci_write_config_word(pdev, cap + PCI_EXP_DPC_CTL,
>> > > > ctl | PCI_EXP_DPC_CTL_INT_EN);
>> > > > +
>> > > > + return PCI_ERS_RESULT_RECOVERED;
>> > > > +}
>> > > > +
>> > > > +static void dpc_work(struct work_struct *work)
>> > > > +{
>> > > > + struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
>> > > > + struct pci_dev *pdev = dpc->dev->port;
>> > > > +
>> > > > + /* From DPC point of view error is always FATAL. */
>> > > > + pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_DPC);
>> > > > }
>> > > >
>> > > > static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
>> > > > @@ -288,6 +301,7 @@ static struct pcie_port_service_driver dpcdriver = {
>> > > > .service = PCIE_PORT_SERVICE_DPC,
>> > > > .probe = dpc_probe,
>> > > > .remove = dpc_remove,
>> > > > + .reset_link = dpc_reset_link,
>> > > > };
>> > > >
>> > > > static int __init dpc_service_init(void)
>> > > > diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>> > > > index 33a16b1..29ff148 100644
>> > > > --- a/drivers/pci/pcie/err.c
>> > > > +++ b/drivers/pci/pcie/err.c
>> > > > @@ -185,7 +185,7 @@ static pci_ers_result_t default_reset_link(struct
>> > > > pci_dev *dev)
>> > > > return PCI_ERS_RESULT_RECOVERED;
>> > > > }
>> > > >
>> > > > -static pci_ers_result_t reset_link(struct pci_dev *dev)
>> > > > +static pci_ers_result_t reset_link(struct pci_dev *dev, u32 service)
>> > > > {
>> > > > struct pci_dev *udev;
>> > > > pci_ers_result_t status;
>> > > > @@ -200,7 +200,7 @@ static pci_ers_result_t reset_link(struct pci_dev
>> > > > *dev)
>> > > > }
>> > > >
>> > > > /* Use the aer driver of the component firstly */
>> > > > - driver = pcie_port_find_service(udev, PCIE_PORT_SERVICE_AER);
>> > > > + driver = pcie_port_find_service(udev, service);
>> > > >
>> > > > if (driver && driver->reset_link) {
>> > > > status = driver->reset_link(udev);
>> > > > @@ -287,7 +287,7 @@ static pci_ers_result_t
>> > > > broadcast_error_message(struct pci_dev *dev,
>> > > > * followed by re-enumeration of devices.
>> > > > */
>> > > >
>> > > > -void pcie_do_fatal_recovery(struct pci_dev *dev)
>> > > > +void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
>> > > > {
>> > > > struct pci_dev *udev;
>> > > > struct pci_bus *parent;
>> > > > @@ -313,7 +313,7 @@ void pcie_do_fatal_recovery(struct pci_dev *dev)
>> > > > pci_dev_put(pdev);
>> > > > }
>> > > >
>> > > > - result = reset_link(udev);
>> > > > + result = reset_link(udev, service);
>> > > >
>> > > > if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
>> > > > /*
>> > > > diff --git a/include/linux/aer.h b/include/linux/aer.h
>> > > > index 8f87bbe..0c506fe 100644
>> > > > --- a/include/linux/aer.h
>> > > > +++ b/include/linux/aer.h
>> > > > @@ -14,6 +14,7 @@
>> > > > #define AER_NONFATAL 0
>> > > > #define AER_FATAL 1
>> > > > #define AER_CORRECTABLE 2
>> > > > +#define DPC_FATAL 4
>> >
>> > I think DPC_FATAL can be 3, since these values are not used as a bit
>> > mask.
>> >
>> > > > struct pci_dev;
>> > >
>> > >
>> > > Hi Bjorn,
>> > >
>> > >
>> > > I have addressed all the comments, and I hope we are heading towards
>> > > closure.
>> > >
>> > > I just figure that pcie_do_fatal_recovery (which is getting
>> > > executed for
>> > > DPC as well)
>> > >
>> > > if (result == PCI_ERS_RESULT_RECOVERED) {
>> > > if (pcie_wait_for_link(udev, true))
>> > > pci_rescan_bus(udev->bus);
>> > > pci_info(dev, "Device recovery successful\n");
>> > > }
>> > >
>> > > I have to correct it to
>> > >
>> > > if (service==AER && result == PCI_ERS_RESULT_RECOVERED) {
>> > > if (pcie_wait_for_link(udev, true))
>> > > pci_rescan_bus(udev->bus);
>> > > pci_info(dev, "Device recovery successful\n");
>> > > }
>> >
>> > This patch is mostly a restructuring of DPC and doesn't really change
>> > its
>> > behavior. DPC didn't previously call pcie_wait_for_link() or
>> > pci_rescan_bus(), so I agree that adding the "service==AER" test will
>> > help preserve the existing DPC behavior.
>> >
>> > However, the rescan should happen with DPC *somewhere* and we should
>> > clarify where that is. Maybe for now we only need a comment about where
>> > that happens. Ideally, we could eventually converge this so the same
>> > mechanism is used for AER and DPC, so we wouldn't need a test like
>> > "service=AER".
>
> Please still add a comment here about why the rescan behavior is
> different.
I am not sure if I understand this comment. AER and DPC will follow the
same path.
if (result == PCI_ERS_RESULT_RECOVERED) {
if (pcie_wait_for_link(udev, true))
pci_rescan_bus(udev->bus);
pci_info(dev, "Device recovery successful\n");
}
so the above code will execute for both AER and DPC.
and before re-enumerating both will wait for link to become active.
Regards,
Oza.
>
>> I am sorry I pasted the wrong snippet.
>> following needs to be fixed in v17.
>> from:
>> if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
>> /*
>> * If the error is reported by a bridge, we think this
>> error
>> * is related to the downstream link of the bridge, so
>> we
>> * do error recovery on all subordinates of the bridge
>> instead
>> * of the bridge and clear the error status of the
>> bridge.
>> */
>> pci_walk_bus(dev->subordinate, report_resume,
>> &result_data);
>> pci_cleanup_aer_uncorrect_error_status(dev);
>> }
>>
>>
>> to
>>
>> if (service==AER && dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
>> /*
>> * If the error is reported by a bridge, we think this
>> error
>> * is related to the downstream link of the bridge, so
>> we
>> * do error recovery on all subordinates of the bridge
>> instead
>> * of the bridge and clear the error status of the
>> bridge.
>> */
>> pci_walk_bus(dev->subordinate, report_resume,
>> &result_data);
>> pci_cleanup_aer_uncorrect_error_status(dev);
>> }
>>
>> this is only needed in case of AER.
>
> Oh, I missed this before. It makes sense to clear the AER status
> here, but why do we need to call report_resume()? We just called all
> the driver .remove() methods and detached the drivers from the
> devices. So I don't think report_resume() will do anything
> ("dev->driver" should be NULL) except set the dev->error_state to
> pci_channel_io_normal. We probably don't need that because we didn't
> change error_state in this fatal error path.
>
>> by the way I can not find pci/oza-v16 branch when I cloned
>> git clone
>> https://kernel.googlesource.com/pub/scm/linux/kernel/git/helgaas/pci
>
> Sorry, I must have forgotten to push it. It's there now at
>
>
> https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/oza-v16
>
> I don't know anything about the googlesource mirror, so I don't know
> when it will get there.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v16 8/9] PCI/DPC: Unify and plumb error handling into DPC
2018-05-16 12:15 ` poza
@ 2018-05-16 13:04 ` Bjorn Helgaas
2018-05-16 13:58 ` poza
2018-05-16 14:58 ` poza
0 siblings, 2 replies; 29+ messages in thread
From: Bjorn Helgaas @ 2018-05-16 13:04 UTC (permalink / raw)
To: poza
Cc: Bjorn Helgaas, Philippe Ombredanne, Thomas Gleixner,
Greg Kroah-Hartman, Kate Stewart, linux-pci, linux-kernel,
Dongdong Liu, Keith Busch, Wei Zhang, Sinan Kaya, Timur Tabi
On Wed, May 16, 2018 at 05:45:58PM +0530, poza@codeaurora.org wrote:
> On 2018-05-16 16:22, Bjorn Helgaas wrote:
> > On Wed, May 16, 2018 at 01:46:25PM +0530, poza@codeaurora.org wrote:
> > > On 2018-05-16 05:26, Bjorn Helgaas wrote:
> > > > On Fri, May 11, 2018 at 05:22:08PM +0530, poza@codeaurora.org wrote:
> > > > > On 2018-05-11 16:13, Oza Pawandeep wrote:
> > > > > > DPC driver implements link_reset callback, and calls
> > > > > > pci_do_fatal_recovery().
> > > > > >
> > > > > > Which follows standard path of ERR_FATAL recovery.
> > > > > >
> > > > > > Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
> > > > > >
> > > > > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > > > > > index 5e8857a..6af7595 100644
> > > > > > --- a/drivers/pci/pci.h
> > > > > > +++ b/drivers/pci/pci.h
> > > > > > @@ -354,7 +354,7 @@ static inline resource_size_t
> > > > > > pci_resource_alignment(struct pci_dev *dev,
> > > > > > void pci_enable_acs(struct pci_dev *dev);
> > > > > >
> > > > > > /* PCI error reporting and recovery */
> > > > > > -void pcie_do_fatal_recovery(struct pci_dev *dev);
> > > > > > +void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service);
> > > > > > void pcie_do_nonfatal_recovery(struct pci_dev *dev);
> > > > > >
> > > > > > bool pcie_wait_for_link(struct pci_dev *pdev, bool active);
> > > > > > diff --git a/drivers/pci/pcie/aer/aerdrv_core.c
> > > > > > b/drivers/pci/pcie/aer/aerdrv_core.c
> > > > > > index fdfc474..36e622d 100644
> > > > > > --- a/drivers/pci/pcie/aer/aerdrv_core.c
> > > > > > +++ b/drivers/pci/pcie/aer/aerdrv_core.c
> > > > > > @@ -254,7 +254,7 @@ static void handle_error_source(struct pcie_device
> > > > > > *aerdev,
> > > > > > } else if (info->severity == AER_NONFATAL)
> > > > > > pcie_do_nonfatal_recovery(dev);
> > > > > > else if (info->severity == AER_FATAL)
> > > > > > - pcie_do_fatal_recovery(dev);
> > > > > > + pcie_do_fatal_recovery(dev, PCIE_PORT_SERVICE_AER);
> > > > > > }
> > > > > >
> > > > > > #ifdef CONFIG_ACPI_APEI_PCIEAER
> > > > > > @@ -321,7 +321,7 @@ static void aer_recover_work_func(struct work_struct
> > > > > > *work)
> > > > > > if (entry.severity == AER_NONFATAL)
> > > > > > pcie_do_nonfatal_recovery(pdev);
> > > > > > else if (entry.severity == AER_FATAL)
> > > > > > - pcie_do_fatal_recovery(pdev);
> > > > > > + pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_AER);
> > > > > > pci_dev_put(pdev);
> > > > > > }
> > > > > > }
> > > > > > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > > > > > index 80ec384..5680c13 100644
> > > > > > --- a/drivers/pci/pcie/dpc.c
> > > > > > +++ b/drivers/pci/pcie/dpc.c
> > > > > > @@ -73,29 +73,31 @@ static void dpc_wait_link_inactive(struct dpc_dev
> > > > > > *dpc)
> > > > > > pcie_wait_for_link(pdev, false);
> > > > > > }
> > > > > >
> > > > > > -static void dpc_work(struct work_struct *work)
> > > > > > +static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
> > > > > > {
> > > > > > - struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
> > > > > > - struct pci_dev *dev, *temp, *pdev = dpc->dev->port;
> > > > > > - struct pci_bus *parent = pdev->subordinate;
> > > > > > - u16 cap = dpc->cap_pos, ctl;
> > > > > > -
> > > > > > - pci_lock_rescan_remove();
> > > > > > - list_for_each_entry_safe_reverse(dev, temp, &parent->devices,
> > > > > > - bus_list) {
> > > > > > - pci_dev_get(dev);
> > > > > > - pci_dev_set_disconnected(dev, NULL);
> > > > > > - if (pci_has_subordinate(dev))
> > > > > > - pci_walk_bus(dev->subordinate,
> > > > > > - pci_dev_set_disconnected, NULL);
> > > > > > - pci_stop_and_remove_bus_device(dev);
> > > > > > - pci_dev_put(dev);
> > > > > > - }
> > > > > > - pci_unlock_rescan_remove();
> > > > > > -
> > > > > > + struct dpc_dev *dpc;
> > > > > > + struct pcie_device *pciedev;
> > > > > > + struct device *devdpc;
> > > > > > + u16 cap, ctl;
> > > > > > +
> > > > > > + /*
> > > > > > + * DPC disables the Link automatically in hardware, so it has
> > > > > > + * already been reset by the time we get here.
> > > > > > + */
> > > > > > +
> > > > > > + devdpc = pcie_port_find_device(pdev, PCIE_PORT_SERVICE_DPC);
> > > > > > + pciedev = to_pcie_device(devdpc);
> > > > > > + dpc = get_service_data(pciedev);
> > > > > > + cap = dpc->cap_pos;
> > > > > > +
> > > > > > + /*
> > > > > > + * Waiting until the link is inactive, then clearing DPC
> > > > > > + * trigger status to allow the port to leave DPC.
> > > > > > + */
> > > > > > dpc_wait_link_inactive(dpc);
> > > > > > +
> > > > > > if (dpc->rp_extensions && dpc_wait_rp_inactive(dpc))
> > > > > > - return;
> > > > > > + return PCI_ERS_RESULT_DISCONNECT;
> > > > > > if (dpc->rp_extensions && dpc->rp_pio_status) {
> > > > > > pci_write_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS,
> > > > > > dpc->rp_pio_status);
> > > > > > @@ -108,6 +110,17 @@ static void dpc_work(struct work_struct *work)
> > > > > > pci_read_config_word(pdev, cap + PCI_EXP_DPC_CTL, &ctl);
> > > > > > pci_write_config_word(pdev, cap + PCI_EXP_DPC_CTL,
> > > > > > ctl | PCI_EXP_DPC_CTL_INT_EN);
> > > > > > +
> > > > > > + return PCI_ERS_RESULT_RECOVERED;
> > > > > > +}
> > > > > > +
> > > > > > +static void dpc_work(struct work_struct *work)
> > > > > > +{
> > > > > > + struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
> > > > > > + struct pci_dev *pdev = dpc->dev->port;
> > > > > > +
> > > > > > + /* From DPC point of view error is always FATAL. */
> > > > > > + pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_DPC);
> > > > > > }
> > > > > >
> > > > > > static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
> > > > > > @@ -288,6 +301,7 @@ static struct pcie_port_service_driver dpcdriver = {
> > > > > > .service = PCIE_PORT_SERVICE_DPC,
> > > > > > .probe = dpc_probe,
> > > > > > .remove = dpc_remove,
> > > > > > + .reset_link = dpc_reset_link,
> > > > > > };
> > > > > >
> > > > > > static int __init dpc_service_init(void)
> > > > > > diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> > > > > > index 33a16b1..29ff148 100644
> > > > > > --- a/drivers/pci/pcie/err.c
> > > > > > +++ b/drivers/pci/pcie/err.c
> > > > > > @@ -185,7 +185,7 @@ static pci_ers_result_t default_reset_link(struct
> > > > > > pci_dev *dev)
> > > > > > return PCI_ERS_RESULT_RECOVERED;
> > > > > > }
> > > > > >
> > > > > > -static pci_ers_result_t reset_link(struct pci_dev *dev)
> > > > > > +static pci_ers_result_t reset_link(struct pci_dev *dev, u32 service)
> > > > > > {
> > > > > > struct pci_dev *udev;
> > > > > > pci_ers_result_t status;
> > > > > > @@ -200,7 +200,7 @@ static pci_ers_result_t reset_link(struct pci_dev
> > > > > > *dev)
> > > > > > }
> > > > > >
> > > > > > /* Use the aer driver of the component firstly */
> > > > > > - driver = pcie_port_find_service(udev, PCIE_PORT_SERVICE_AER);
> > > > > > + driver = pcie_port_find_service(udev, service);
> > > > > >
> > > > > > if (driver && driver->reset_link) {
> > > > > > status = driver->reset_link(udev);
> > > > > > @@ -287,7 +287,7 @@ static pci_ers_result_t
> > > > > > broadcast_error_message(struct pci_dev *dev,
> > > > > > * followed by re-enumeration of devices.
> > > > > > */
> > > > > >
> > > > > > -void pcie_do_fatal_recovery(struct pci_dev *dev)
> > > > > > +void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
> > > > > > {
> > > > > > struct pci_dev *udev;
> > > > > > struct pci_bus *parent;
> > > > > > @@ -313,7 +313,7 @@ void pcie_do_fatal_recovery(struct pci_dev *dev)
> > > > > > pci_dev_put(pdev);
> > > > > > }
> > > > > >
> > > > > > - result = reset_link(udev);
> > > > > > + result = reset_link(udev, service);
> > > > > >
> > > > > > if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
> > > > > > /*
> > > > > > diff --git a/include/linux/aer.h b/include/linux/aer.h
> > > > > > index 8f87bbe..0c506fe 100644
> > > > > > --- a/include/linux/aer.h
> > > > > > +++ b/include/linux/aer.h
> > > > > > @@ -14,6 +14,7 @@
> > > > > > #define AER_NONFATAL 0
> > > > > > #define AER_FATAL 1
> > > > > > #define AER_CORRECTABLE 2
> > > > > > +#define DPC_FATAL 4
> > > >
> > > > I think DPC_FATAL can be 3, since these values are not used as a bit
> > > > mask.
> > > >
> > > > > > struct pci_dev;
> > > > >
> > > > >
> > > > > Hi Bjorn,
> > > > >
> > > > >
> > > > > I have addressed all the comments, and I hope we are heading towards
> > > > > closure.
> > > > >
> > > > > I just figure that pcie_do_fatal_recovery (which is getting
> > > > > executed for
> > > > > DPC as well)
> > > > >
> > > > > if (result == PCI_ERS_RESULT_RECOVERED) {
> > > > > if (pcie_wait_for_link(udev, true))
> > > > > pci_rescan_bus(udev->bus);
> > > > > pci_info(dev, "Device recovery successful\n");
> > > > > }
> > > > >
> > > > > I have to correct it to
> > > > >
> > > > > if (service==AER && result == PCI_ERS_RESULT_RECOVERED) {
> > > > > if (pcie_wait_for_link(udev, true))
> > > > > pci_rescan_bus(udev->bus);
> > > > > pci_info(dev, "Device recovery successful\n");
> > > > > }
> > > >
> > > > This patch is mostly a restructuring of DPC and doesn't really change
> > > > its
> > > > behavior. DPC didn't previously call pcie_wait_for_link() or
> > > > pci_rescan_bus(), so I agree that adding the "service==AER" test will
> > > > help preserve the existing DPC behavior.
> > > >
> > > > However, the rescan should happen with DPC *somewhere* and we should
> > > > clarify where that is. Maybe for now we only need a comment about where
> > > > that happens. Ideally, we could eventually converge this so the same
> > > > mechanism is used for AER and DPC, so we wouldn't need a test like
> > > > "service=AER".
> >
> > Please still add a comment here about why the rescan behavior is
> > different.
> >
> > > I am sorry I pasted the wrong snippet.
> > > following needs to be fixed in v17.
> > > from:
> > > if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
> > > /*
> > > * If the error is reported by a bridge, we think
> > > this error
> > > * is related to the downstream link of the bridge,
> > > so we
> > > * do error recovery on all subordinates of the bridge
> > > instead
> > > * of the bridge and clear the error status of the
> > > bridge.
> > > */
> > > pci_walk_bus(dev->subordinate, report_resume,
> > > &result_data);
> > > pci_cleanup_aer_uncorrect_error_status(dev);
> > > }
> > >
> > >
> > > to
> > >
> > > if (service==AER && dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
> > > /*
> > > * If the error is reported by a bridge, we think
> > > this error
> > > * is related to the downstream link of the bridge,
> > > so we
> > > * do error recovery on all subordinates of the bridge
> > > instead
> > > * of the bridge and clear the error status of the
> > > bridge.
> > > */
> > > pci_walk_bus(dev->subordinate, report_resume,
> > > &result_data);
> > > pci_cleanup_aer_uncorrect_error_status(dev);
> > > }
> > >
> > > this is only needed in case of AER.
> >
> > Oh, I missed this before. It makes sense to clear the AER status
> > here, but why do we need to call report_resume()? We just called all
> > the driver .remove() methods and detached the drivers from the
> > devices. So I don't think report_resume() will do anything
> > ("dev->driver" should be NULL) except set the dev->error_state to
> > pci_channel_io_normal. We probably don't need that because we didn't
> > change error_state in this fatal error path.
>
> if you remember, the path ends up calling
> aer_error_resume
>
> the existing code ends up calling aer_error_resume as follows.
>
> do_recovery(pci_dev)
> broadcast_error_message(..., error_detected, ...)
> if (AER_FATAL)
> reset_link(pci_dev)
> udev = BRIDGE ? pci_dev : pci_dev->bus->self
> driver->reset_link(udev)
> aer_root_reset(udev)
> if (CAN_RECOVER)
> broadcast_error_message(..., mmio_enabled, ...)
> if (NEED_RESET)
> broadcast_error_message(..., slot_reset, ...)
> broadcast_error_message(dev, ..., report_resume, ...)
> if (BRIDGE)
> report_resume
> driver->resume
> pcie_portdrv_err_resume
> device_for_each_child(..., resume_iter)
> resume_iter
> driver->error_resume
> aer_error_resume
> pci_cleanup_aer_uncorrect_error_status(pci_dev) # only if
> BRIDGE
> pci_write_config_dword(PCI_ERR_UNCOR_STATUS)
>
> hence I think we have to call it in order to clear the root port
> PCI_ERR_UNCOR_STATUS and PCI_EXP_DEVSTA.
> makes sense ?
I know I sent you the call graph above, so you would think I might
understand it, but you would be mistaken :) This still doesn't make
sense to me.
I think your point is that we need to call aer_error_resume(). That
is the aerdriver.error_resume() method. The AER driver only binds to
root ports.
This path:
pcie_do_fatal_recovery
pci_walk_bus(dev->subordinate, report_resume, &result_data)
calls report_resume() for every device on the dev->subordinate bus
(and for anything below those devices). There are no root ports on
that dev->subordinate bus, because root ports are always on a root
bus, never on a subordinate bus.
So I don't see how report_resume() can ever get to aer_error_resume().
Can you instrument that path and verify that it actually does get
there somehow?
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v16 8/9] PCI/DPC: Unify and plumb error handling into DPC
2018-05-16 12:51 ` poza
@ 2018-05-16 13:09 ` Bjorn Helgaas
0 siblings, 0 replies; 29+ messages in thread
From: Bjorn Helgaas @ 2018-05-16 13:09 UTC (permalink / raw)
To: poza
Cc: Bjorn Helgaas, Philippe Ombredanne, Thomas Gleixner,
Greg Kroah-Hartman, Kate Stewart, linux-pci, linux-kernel,
Dongdong Liu, Keith Busch, Wei Zhang, Sinan Kaya, Timur Tabi
On Wed, May 16, 2018 at 06:21:09PM +0530, poza@codeaurora.org wrote:
> On 2018-05-16 16:22, Bjorn Helgaas wrote:
> > On Wed, May 16, 2018 at 01:46:25PM +0530, poza@codeaurora.org wrote:
> > > On 2018-05-16 05:26, Bjorn Helgaas wrote:
> > > > On Fri, May 11, 2018 at 05:22:08PM +0530, poza@codeaurora.org wrote:
> > > > > On 2018-05-11 16:13, Oza Pawandeep wrote:
> > > > > > DPC driver implements link_reset callback, and calls
> > > > > > pci_do_fatal_recovery().
> > > > > >
> > > > > > Which follows standard path of ERR_FATAL recovery.
> > > > > >
> > > > > > Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
> > > > > >
> > > > > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > > > > > index 5e8857a..6af7595 100644
> > > > > > --- a/drivers/pci/pci.h
> > > > > > +++ b/drivers/pci/pci.h
> > > > > > @@ -354,7 +354,7 @@ static inline resource_size_t
> > > > > > pci_resource_alignment(struct pci_dev *dev,
> > > > > > void pci_enable_acs(struct pci_dev *dev);
> > > > > >
> > > > > > /* PCI error reporting and recovery */
> > > > > > -void pcie_do_fatal_recovery(struct pci_dev *dev);
> > > > > > +void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service);
> > > > > > void pcie_do_nonfatal_recovery(struct pci_dev *dev);
> > > > > >
> > > > > > bool pcie_wait_for_link(struct pci_dev *pdev, bool active);
> > > > > > diff --git a/drivers/pci/pcie/aer/aerdrv_core.c
> > > > > > b/drivers/pci/pcie/aer/aerdrv_core.c
> > > > > > index fdfc474..36e622d 100644
> > > > > > --- a/drivers/pci/pcie/aer/aerdrv_core.c
> > > > > > +++ b/drivers/pci/pcie/aer/aerdrv_core.c
> > > > > > @@ -254,7 +254,7 @@ static void handle_error_source(struct pcie_device
> > > > > > *aerdev,
> > > > > > } else if (info->severity == AER_NONFATAL)
> > > > > > pcie_do_nonfatal_recovery(dev);
> > > > > > else if (info->severity == AER_FATAL)
> > > > > > - pcie_do_fatal_recovery(dev);
> > > > > > + pcie_do_fatal_recovery(dev, PCIE_PORT_SERVICE_AER);
> > > > > > }
> > > > > >
> > > > > > #ifdef CONFIG_ACPI_APEI_PCIEAER
> > > > > > @@ -321,7 +321,7 @@ static void aer_recover_work_func(struct work_struct
> > > > > > *work)
> > > > > > if (entry.severity == AER_NONFATAL)
> > > > > > pcie_do_nonfatal_recovery(pdev);
> > > > > > else if (entry.severity == AER_FATAL)
> > > > > > - pcie_do_fatal_recovery(pdev);
> > > > > > + pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_AER);
> > > > > > pci_dev_put(pdev);
> > > > > > }
> > > > > > }
> > > > > > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > > > > > index 80ec384..5680c13 100644
> > > > > > --- a/drivers/pci/pcie/dpc.c
> > > > > > +++ b/drivers/pci/pcie/dpc.c
> > > > > > @@ -73,29 +73,31 @@ static void dpc_wait_link_inactive(struct dpc_dev
> > > > > > *dpc)
> > > > > > pcie_wait_for_link(pdev, false);
> > > > > > }
> > > > > >
> > > > > > -static void dpc_work(struct work_struct *work)
> > > > > > +static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
> > > > > > {
> > > > > > - struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
> > > > > > - struct pci_dev *dev, *temp, *pdev = dpc->dev->port;
> > > > > > - struct pci_bus *parent = pdev->subordinate;
> > > > > > - u16 cap = dpc->cap_pos, ctl;
> > > > > > -
> > > > > > - pci_lock_rescan_remove();
> > > > > > - list_for_each_entry_safe_reverse(dev, temp, &parent->devices,
> > > > > > - bus_list) {
> > > > > > - pci_dev_get(dev);
> > > > > > - pci_dev_set_disconnected(dev, NULL);
> > > > > > - if (pci_has_subordinate(dev))
> > > > > > - pci_walk_bus(dev->subordinate,
> > > > > > - pci_dev_set_disconnected, NULL);
> > > > > > - pci_stop_and_remove_bus_device(dev);
> > > > > > - pci_dev_put(dev);
> > > > > > - }
> > > > > > - pci_unlock_rescan_remove();
> > > > > > -
> > > > > > + struct dpc_dev *dpc;
> > > > > > + struct pcie_device *pciedev;
> > > > > > + struct device *devdpc;
> > > > > > + u16 cap, ctl;
> > > > > > +
> > > > > > + /*
> > > > > > + * DPC disables the Link automatically in hardware, so it has
> > > > > > + * already been reset by the time we get here.
> > > > > > + */
> > > > > > +
> > > > > > + devdpc = pcie_port_find_device(pdev, PCIE_PORT_SERVICE_DPC);
> > > > > > + pciedev = to_pcie_device(devdpc);
> > > > > > + dpc = get_service_data(pciedev);
> > > > > > + cap = dpc->cap_pos;
> > > > > > +
> > > > > > + /*
> > > > > > + * Waiting until the link is inactive, then clearing DPC
> > > > > > + * trigger status to allow the port to leave DPC.
> > > > > > + */
> > > > > > dpc_wait_link_inactive(dpc);
> > > > > > +
> > > > > > if (dpc->rp_extensions && dpc_wait_rp_inactive(dpc))
> > > > > > - return;
> > > > > > + return PCI_ERS_RESULT_DISCONNECT;
> > > > > > if (dpc->rp_extensions && dpc->rp_pio_status) {
> > > > > > pci_write_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS,
> > > > > > dpc->rp_pio_status);
> > > > > > @@ -108,6 +110,17 @@ static void dpc_work(struct work_struct *work)
> > > > > > pci_read_config_word(pdev, cap + PCI_EXP_DPC_CTL, &ctl);
> > > > > > pci_write_config_word(pdev, cap + PCI_EXP_DPC_CTL,
> > > > > > ctl | PCI_EXP_DPC_CTL_INT_EN);
> > > > > > +
> > > > > > + return PCI_ERS_RESULT_RECOVERED;
> > > > > > +}
> > > > > > +
> > > > > > +static void dpc_work(struct work_struct *work)
> > > > > > +{
> > > > > > + struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
> > > > > > + struct pci_dev *pdev = dpc->dev->port;
> > > > > > +
> > > > > > + /* From DPC point of view error is always FATAL. */
> > > > > > + pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_DPC);
> > > > > > }
> > > > > >
> > > > > > static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
> > > > > > @@ -288,6 +301,7 @@ static struct pcie_port_service_driver dpcdriver = {
> > > > > > .service = PCIE_PORT_SERVICE_DPC,
> > > > > > .probe = dpc_probe,
> > > > > > .remove = dpc_remove,
> > > > > > + .reset_link = dpc_reset_link,
> > > > > > };
> > > > > >
> > > > > > static int __init dpc_service_init(void)
> > > > > > diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> > > > > > index 33a16b1..29ff148 100644
> > > > > > --- a/drivers/pci/pcie/err.c
> > > > > > +++ b/drivers/pci/pcie/err.c
> > > > > > @@ -185,7 +185,7 @@ static pci_ers_result_t default_reset_link(struct
> > > > > > pci_dev *dev)
> > > > > > return PCI_ERS_RESULT_RECOVERED;
> > > > > > }
> > > > > >
> > > > > > -static pci_ers_result_t reset_link(struct pci_dev *dev)
> > > > > > +static pci_ers_result_t reset_link(struct pci_dev *dev, u32 service)
> > > > > > {
> > > > > > struct pci_dev *udev;
> > > > > > pci_ers_result_t status;
> > > > > > @@ -200,7 +200,7 @@ static pci_ers_result_t reset_link(struct pci_dev
> > > > > > *dev)
> > > > > > }
> > > > > >
> > > > > > /* Use the aer driver of the component firstly */
> > > > > > - driver = pcie_port_find_service(udev, PCIE_PORT_SERVICE_AER);
> > > > > > + driver = pcie_port_find_service(udev, service);
> > > > > >
> > > > > > if (driver && driver->reset_link) {
> > > > > > status = driver->reset_link(udev);
> > > > > > @@ -287,7 +287,7 @@ static pci_ers_result_t
> > > > > > broadcast_error_message(struct pci_dev *dev,
> > > > > > * followed by re-enumeration of devices.
> > > > > > */
> > > > > >
> > > > > > -void pcie_do_fatal_recovery(struct pci_dev *dev)
> > > > > > +void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
> > > > > > {
> > > > > > struct pci_dev *udev;
> > > > > > struct pci_bus *parent;
> > > > > > @@ -313,7 +313,7 @@ void pcie_do_fatal_recovery(struct pci_dev *dev)
> > > > > > pci_dev_put(pdev);
> > > > > > }
> > > > > >
> > > > > > - result = reset_link(udev);
> > > > > > + result = reset_link(udev, service);
> > > > > >
> > > > > > if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
> > > > > > /*
> > > > > > diff --git a/include/linux/aer.h b/include/linux/aer.h
> > > > > > index 8f87bbe..0c506fe 100644
> > > > > > --- a/include/linux/aer.h
> > > > > > +++ b/include/linux/aer.h
> > > > > > @@ -14,6 +14,7 @@
> > > > > > #define AER_NONFATAL 0
> > > > > > #define AER_FATAL 1
> > > > > > #define AER_CORRECTABLE 2
> > > > > > +#define DPC_FATAL 4
> > > >
> > > > I think DPC_FATAL can be 3, since these values are not used as a bit
> > > > mask.
> > > >
> > > > > > struct pci_dev;
> > > > >
> > > > >
> > > > > Hi Bjorn,
> > > > >
> > > > >
> > > > > I have addressed all the comments, and I hope we are heading towards
> > > > > closure.
> > > > >
> > > > > I just figure that pcie_do_fatal_recovery (which is getting
> > > > > executed for
> > > > > DPC as well)
> > > > >
> > > > > if (result == PCI_ERS_RESULT_RECOVERED) {
> > > > > if (pcie_wait_for_link(udev, true))
> > > > > pci_rescan_bus(udev->bus);
> > > > > pci_info(dev, "Device recovery successful\n");
> > > > > }
> > > > >
> > > > > I have to correct it to
> > > > >
> > > > > if (service==AER && result == PCI_ERS_RESULT_RECOVERED) {
> > > > > if (pcie_wait_for_link(udev, true))
> > > > > pci_rescan_bus(udev->bus);
> > > > > pci_info(dev, "Device recovery successful\n");
> > > > > }
> > > >
> > > > This patch is mostly a restructuring of DPC and doesn't really change
> > > > its
> > > > behavior. DPC didn't previously call pcie_wait_for_link() or
> > > > pci_rescan_bus(), so I agree that adding the "service==AER" test will
> > > > help preserve the existing DPC behavior.
> > > >
> > > > However, the rescan should happen with DPC *somewhere* and we should
> > > > clarify where that is. Maybe for now we only need a comment about where
> > > > that happens. Ideally, we could eventually converge this so the same
> > > > mechanism is used for AER and DPC, so we wouldn't need a test like
> > > > "service=AER".
> >
> > Please still add a comment here about why the rescan behavior is
> > different.
>
> I am not sure if I understand this comment. AER and DPC will follow the same
> path.
> if (result == PCI_ERS_RESULT_RECOVERED) {
> if (pcie_wait_for_link(udev, true))
> pci_rescan_bus(udev->bus);
> pci_info(dev, "Device recovery successful\n");
> }
>
> so the above code will execute for both AER and DPC.
> and before re-enumerating both will wait for link to become active.
OK, good. I still had your comment about adding the "service==AER"
test here in my mind. You had already corrected that by saying you
intended that test for a different path, but I hadn't internalized it
yet.
If AER and DPC work the same here, that's great. That's what we want,
so no clarifying comment should be needed.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v16 8/9] PCI/DPC: Unify and plumb error handling into DPC
2018-05-16 13:04 ` Bjorn Helgaas
@ 2018-05-16 13:58 ` poza
2018-05-16 14:58 ` poza
1 sibling, 0 replies; 29+ messages in thread
From: poza @ 2018-05-16 13:58 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Bjorn Helgaas, Philippe Ombredanne, Thomas Gleixner,
Greg Kroah-Hartman, Kate Stewart, linux-pci, linux-kernel,
Dongdong Liu, Keith Busch, Wei Zhang, Sinan Kaya, Timur Tabi,
linux-pci-owner
On 2018-05-16 18:34, Bjorn Helgaas wrote:
> On Wed, May 16, 2018 at 05:45:58PM +0530, poza@codeaurora.org wrote:
>> On 2018-05-16 16:22, Bjorn Helgaas wrote:
>> > On Wed, May 16, 2018 at 01:46:25PM +0530, poza@codeaurora.org wrote:
>> > > On 2018-05-16 05:26, Bjorn Helgaas wrote:
>> > > > On Fri, May 11, 2018 at 05:22:08PM +0530, poza@codeaurora.org wrote:
>> > > > > On 2018-05-11 16:13, Oza Pawandeep wrote:
>> > > > > > DPC driver implements link_reset callback, and calls
>> > > > > > pci_do_fatal_recovery().
>> > > > > >
>> > > > > > Which follows standard path of ERR_FATAL recovery.
>> > > > > >
>> > > > > > Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
>> > > > > >
>> > > > > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> > > > > > index 5e8857a..6af7595 100644
>> > > > > > --- a/drivers/pci/pci.h
>> > > > > > +++ b/drivers/pci/pci.h
>> > > > > > @@ -354,7 +354,7 @@ static inline resource_size_t
>> > > > > > pci_resource_alignment(struct pci_dev *dev,
>> > > > > > void pci_enable_acs(struct pci_dev *dev);
>> > > > > >
>> > > > > > /* PCI error reporting and recovery */
>> > > > > > -void pcie_do_fatal_recovery(struct pci_dev *dev);
>> > > > > > +void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service);
>> > > > > > void pcie_do_nonfatal_recovery(struct pci_dev *dev);
>> > > > > >
>> > > > > > bool pcie_wait_for_link(struct pci_dev *pdev, bool active);
>> > > > > > diff --git a/drivers/pci/pcie/aer/aerdrv_core.c
>> > > > > > b/drivers/pci/pcie/aer/aerdrv_core.c
>> > > > > > index fdfc474..36e622d 100644
>> > > > > > --- a/drivers/pci/pcie/aer/aerdrv_core.c
>> > > > > > +++ b/drivers/pci/pcie/aer/aerdrv_core.c
>> > > > > > @@ -254,7 +254,7 @@ static void handle_error_source(struct pcie_device
>> > > > > > *aerdev,
>> > > > > > } else if (info->severity == AER_NONFATAL)
>> > > > > > pcie_do_nonfatal_recovery(dev);
>> > > > > > else if (info->severity == AER_FATAL)
>> > > > > > - pcie_do_fatal_recovery(dev);
>> > > > > > + pcie_do_fatal_recovery(dev, PCIE_PORT_SERVICE_AER);
>> > > > > > }
>> > > > > >
>> > > > > > #ifdef CONFIG_ACPI_APEI_PCIEAER
>> > > > > > @@ -321,7 +321,7 @@ static void aer_recover_work_func(struct work_struct
>> > > > > > *work)
>> > > > > > if (entry.severity == AER_NONFATAL)
>> > > > > > pcie_do_nonfatal_recovery(pdev);
>> > > > > > else if (entry.severity == AER_FATAL)
>> > > > > > - pcie_do_fatal_recovery(pdev);
>> > > > > > + pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_AER);
>> > > > > > pci_dev_put(pdev);
>> > > > > > }
>> > > > > > }
>> > > > > > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
>> > > > > > index 80ec384..5680c13 100644
>> > > > > > --- a/drivers/pci/pcie/dpc.c
>> > > > > > +++ b/drivers/pci/pcie/dpc.c
>> > > > > > @@ -73,29 +73,31 @@ static void dpc_wait_link_inactive(struct dpc_dev
>> > > > > > *dpc)
>> > > > > > pcie_wait_for_link(pdev, false);
>> > > > > > }
>> > > > > >
>> > > > > > -static void dpc_work(struct work_struct *work)
>> > > > > > +static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
>> > > > > > {
>> > > > > > - struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
>> > > > > > - struct pci_dev *dev, *temp, *pdev = dpc->dev->port;
>> > > > > > - struct pci_bus *parent = pdev->subordinate;
>> > > > > > - u16 cap = dpc->cap_pos, ctl;
>> > > > > > -
>> > > > > > - pci_lock_rescan_remove();
>> > > > > > - list_for_each_entry_safe_reverse(dev, temp, &parent->devices,
>> > > > > > - bus_list) {
>> > > > > > - pci_dev_get(dev);
>> > > > > > - pci_dev_set_disconnected(dev, NULL);
>> > > > > > - if (pci_has_subordinate(dev))
>> > > > > > - pci_walk_bus(dev->subordinate,
>> > > > > > - pci_dev_set_disconnected, NULL);
>> > > > > > - pci_stop_and_remove_bus_device(dev);
>> > > > > > - pci_dev_put(dev);
>> > > > > > - }
>> > > > > > - pci_unlock_rescan_remove();
>> > > > > > -
>> > > > > > + struct dpc_dev *dpc;
>> > > > > > + struct pcie_device *pciedev;
>> > > > > > + struct device *devdpc;
>> > > > > > + u16 cap, ctl;
>> > > > > > +
>> > > > > > + /*
>> > > > > > + * DPC disables the Link automatically in hardware, so it has
>> > > > > > + * already been reset by the time we get here.
>> > > > > > + */
>> > > > > > +
>> > > > > > + devdpc = pcie_port_find_device(pdev, PCIE_PORT_SERVICE_DPC);
>> > > > > > + pciedev = to_pcie_device(devdpc);
>> > > > > > + dpc = get_service_data(pciedev);
>> > > > > > + cap = dpc->cap_pos;
>> > > > > > +
>> > > > > > + /*
>> > > > > > + * Waiting until the link is inactive, then clearing DPC
>> > > > > > + * trigger status to allow the port to leave DPC.
>> > > > > > + */
>> > > > > > dpc_wait_link_inactive(dpc);
>> > > > > > +
>> > > > > > if (dpc->rp_extensions && dpc_wait_rp_inactive(dpc))
>> > > > > > - return;
>> > > > > > + return PCI_ERS_RESULT_DISCONNECT;
>> > > > > > if (dpc->rp_extensions && dpc->rp_pio_status) {
>> > > > > > pci_write_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS,
>> > > > > > dpc->rp_pio_status);
>> > > > > > @@ -108,6 +110,17 @@ static void dpc_work(struct work_struct *work)
>> > > > > > pci_read_config_word(pdev, cap + PCI_EXP_DPC_CTL, &ctl);
>> > > > > > pci_write_config_word(pdev, cap + PCI_EXP_DPC_CTL,
>> > > > > > ctl | PCI_EXP_DPC_CTL_INT_EN);
>> > > > > > +
>> > > > > > + return PCI_ERS_RESULT_RECOVERED;
>> > > > > > +}
>> > > > > > +
>> > > > > > +static void dpc_work(struct work_struct *work)
>> > > > > > +{
>> > > > > > + struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
>> > > > > > + struct pci_dev *pdev = dpc->dev->port;
>> > > > > > +
>> > > > > > + /* From DPC point of view error is always FATAL. */
>> > > > > > + pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_DPC);
>> > > > > > }
>> > > > > >
>> > > > > > static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
>> > > > > > @@ -288,6 +301,7 @@ static struct pcie_port_service_driver dpcdriver = {
>> > > > > > .service = PCIE_PORT_SERVICE_DPC,
>> > > > > > .probe = dpc_probe,
>> > > > > > .remove = dpc_remove,
>> > > > > > + .reset_link = dpc_reset_link,
>> > > > > > };
>> > > > > >
>> > > > > > static int __init dpc_service_init(void)
>> > > > > > diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>> > > > > > index 33a16b1..29ff148 100644
>> > > > > > --- a/drivers/pci/pcie/err.c
>> > > > > > +++ b/drivers/pci/pcie/err.c
>> > > > > > @@ -185,7 +185,7 @@ static pci_ers_result_t default_reset_link(struct
>> > > > > > pci_dev *dev)
>> > > > > > return PCI_ERS_RESULT_RECOVERED;
>> > > > > > }
>> > > > > >
>> > > > > > -static pci_ers_result_t reset_link(struct pci_dev *dev)
>> > > > > > +static pci_ers_result_t reset_link(struct pci_dev *dev, u32 service)
>> > > > > > {
>> > > > > > struct pci_dev *udev;
>> > > > > > pci_ers_result_t status;
>> > > > > > @@ -200,7 +200,7 @@ static pci_ers_result_t reset_link(struct pci_dev
>> > > > > > *dev)
>> > > > > > }
>> > > > > >
>> > > > > > /* Use the aer driver of the component firstly */
>> > > > > > - driver = pcie_port_find_service(udev, PCIE_PORT_SERVICE_AER);
>> > > > > > + driver = pcie_port_find_service(udev, service);
>> > > > > >
>> > > > > > if (driver && driver->reset_link) {
>> > > > > > status = driver->reset_link(udev);
>> > > > > > @@ -287,7 +287,7 @@ static pci_ers_result_t
>> > > > > > broadcast_error_message(struct pci_dev *dev,
>> > > > > > * followed by re-enumeration of devices.
>> > > > > > */
>> > > > > >
>> > > > > > -void pcie_do_fatal_recovery(struct pci_dev *dev)
>> > > > > > +void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
>> > > > > > {
>> > > > > > struct pci_dev *udev;
>> > > > > > struct pci_bus *parent;
>> > > > > > @@ -313,7 +313,7 @@ void pcie_do_fatal_recovery(struct pci_dev *dev)
>> > > > > > pci_dev_put(pdev);
>> > > > > > }
>> > > > > >
>> > > > > > - result = reset_link(udev);
>> > > > > > + result = reset_link(udev, service);
>> > > > > >
>> > > > > > if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
>> > > > > > /*
>> > > > > > diff --git a/include/linux/aer.h b/include/linux/aer.h
>> > > > > > index 8f87bbe..0c506fe 100644
>> > > > > > --- a/include/linux/aer.h
>> > > > > > +++ b/include/linux/aer.h
>> > > > > > @@ -14,6 +14,7 @@
>> > > > > > #define AER_NONFATAL 0
>> > > > > > #define AER_FATAL 1
>> > > > > > #define AER_CORRECTABLE 2
>> > > > > > +#define DPC_FATAL 4
>> > > >
>> > > > I think DPC_FATAL can be 3, since these values are not used as a bit
>> > > > mask.
>> > > >
>> > > > > > struct pci_dev;
>> > > > >
>> > > > >
>> > > > > Hi Bjorn,
>> > > > >
>> > > > >
>> > > > > I have addressed all the comments, and I hope we are heading towards
>> > > > > closure.
>> > > > >
>> > > > > I just figure that pcie_do_fatal_recovery (which is getting
>> > > > > executed for
>> > > > > DPC as well)
>> > > > >
>> > > > > if (result == PCI_ERS_RESULT_RECOVERED) {
>> > > > > if (pcie_wait_for_link(udev, true))
>> > > > > pci_rescan_bus(udev->bus);
>> > > > > pci_info(dev, "Device recovery successful\n");
>> > > > > }
>> > > > >
>> > > > > I have to correct it to
>> > > > >
>> > > > > if (service==AER && result == PCI_ERS_RESULT_RECOVERED) {
>> > > > > if (pcie_wait_for_link(udev, true))
>> > > > > pci_rescan_bus(udev->bus);
>> > > > > pci_info(dev, "Device recovery successful\n");
>> > > > > }
>> > > >
>> > > > This patch is mostly a restructuring of DPC and doesn't really change
>> > > > its
>> > > > behavior. DPC didn't previously call pcie_wait_for_link() or
>> > > > pci_rescan_bus(), so I agree that adding the "service==AER" test will
>> > > > help preserve the existing DPC behavior.
>> > > >
>> > > > However, the rescan should happen with DPC *somewhere* and we should
>> > > > clarify where that is. Maybe for now we only need a comment about where
>> > > > that happens. Ideally, we could eventually converge this so the same
>> > > > mechanism is used for AER and DPC, so we wouldn't need a test like
>> > > > "service=AER".
>> >
>> > Please still add a comment here about why the rescan behavior is
>> > different.
>> >
>> > > I am sorry I pasted the wrong snippet.
>> > > following needs to be fixed in v17.
>> > > from:
>> > > if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
>> > > /*
>> > > * If the error is reported by a bridge, we think
>> > > this error
>> > > * is related to the downstream link of the bridge,
>> > > so we
>> > > * do error recovery on all subordinates of the bridge
>> > > instead
>> > > * of the bridge and clear the error status of the
>> > > bridge.
>> > > */
>> > > pci_walk_bus(dev->subordinate, report_resume,
>> > > &result_data);
>> > > pci_cleanup_aer_uncorrect_error_status(dev);
>> > > }
>> > >
>> > >
>> > > to
>> > >
>> > > if (service==AER && dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
>> > > /*
>> > > * If the error is reported by a bridge, we think
>> > > this error
>> > > * is related to the downstream link of the bridge,
>> > > so we
>> > > * do error recovery on all subordinates of the bridge
>> > > instead
>> > > * of the bridge and clear the error status of the
>> > > bridge.
>> > > */
>> > > pci_walk_bus(dev->subordinate, report_resume,
>> > > &result_data);
>> > > pci_cleanup_aer_uncorrect_error_status(dev);
>> > > }
>> > >
>> > > this is only needed in case of AER.
>> >
>> > Oh, I missed this before. It makes sense to clear the AER status
>> > here, but why do we need to call report_resume()? We just called all
>> > the driver .remove() methods and detached the drivers from the
>> > devices. So I don't think report_resume() will do anything
>> > ("dev->driver" should be NULL) except set the dev->error_state to
>> > pci_channel_io_normal. We probably don't need that because we didn't
>> > change error_state in this fatal error path.
>>
>> if you remember, the path ends up calling
>> aer_error_resume
>>
>> the existing code ends up calling aer_error_resume as follows.
>>
>> do_recovery(pci_dev)
>> broadcast_error_message(..., error_detected, ...)
>> if (AER_FATAL)
>> reset_link(pci_dev)
>> udev = BRIDGE ? pci_dev : pci_dev->bus->self
>> driver->reset_link(udev)
>> aer_root_reset(udev)
>> if (CAN_RECOVER)
>> broadcast_error_message(..., mmio_enabled, ...)
>> if (NEED_RESET)
>> broadcast_error_message(..., slot_reset, ...)
>> broadcast_error_message(dev, ..., report_resume, ...)
>> if (BRIDGE)
>> report_resume
>> driver->resume
>> pcie_portdrv_err_resume
>> device_for_each_child(..., resume_iter)
>> resume_iter
>> driver->error_resume
>> aer_error_resume
>> pci_cleanup_aer_uncorrect_error_status(pci_dev) # only
>> if
>> BRIDGE
>> pci_write_config_dword(PCI_ERR_UNCOR_STATUS)
>>
>> hence I think we have to call it in order to clear the root port
>> PCI_ERR_UNCOR_STATUS and PCI_EXP_DEVSTA.
>> makes sense ?
>
> I know I sent you the call graph above, so you would think I might
> understand it, but you would be mistaken :) This still doesn't make
> sense to me.
>
> I think your point is that we need to call aer_error_resume(). That
> is the aerdriver.error_resume() method. The AER driver only binds to
> root ports.
>
> This path:
>
> pcie_do_fatal_recovery
> pci_walk_bus(dev->subordinate, report_resume, &result_data)
>
> calls report_resume() for every device on the dev->subordinate bus
> (and for anything below those devices). There are no root ports on
> that dev->subordinate bus, because root ports are always on a root
> bus, never on a subordinate bus.
>
> So I don't see how report_resume() can ever get to aer_error_resume().
> Can you instrument that path and verify that it actually does get
> there somehow?
Then I being to wonder that aer_error_resume() gets ever called from
anywhere ?
because I was testing this with EP poit of view.
another thing is aer_error_resume() does clear the things for RP, so it
makes sense to call it in BRIDGE case.
anyway let me go ahead and call this code to see what gets called.
Regards,
Oza.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v16 8/9] PCI/DPC: Unify and plumb error handling into DPC
2018-05-16 13:04 ` Bjorn Helgaas
2018-05-16 13:58 ` poza
@ 2018-05-16 14:58 ` poza
2018-05-16 20:02 ` Bjorn Helgaas
1 sibling, 1 reply; 29+ messages in thread
From: poza @ 2018-05-16 14:58 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Bjorn Helgaas, Philippe Ombredanne, Thomas Gleixner,
Greg Kroah-Hartman, Kate Stewart, linux-pci, linux-kernel,
Dongdong Liu, Keith Busch, Wei Zhang, Sinan Kaya, Timur Tabi,
linux-pci-owner
On 2018-05-16 18:34, Bjorn Helgaas wrote:
> On Wed, May 16, 2018 at 05:45:58PM +0530, poza@codeaurora.org wrote:
>> On 2018-05-16 16:22, Bjorn Helgaas wrote:
>> > On Wed, May 16, 2018 at 01:46:25PM +0530, poza@codeaurora.org wrote:
>> > > On 2018-05-16 05:26, Bjorn Helgaas wrote:
>> > > > On Fri, May 11, 2018 at 05:22:08PM +0530, poza@codeaurora.org wrote:
>> > > > > On 2018-05-11 16:13, Oza Pawandeep wrote:
>> > > > > > DPC driver implements link_reset callback, and calls
>> > > > > > pci_do_fatal_recovery().
>> > > > > >
>> > > > > > Which follows standard path of ERR_FATAL recovery.
>> > > > > >
>> > > > > > Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
>> > > > > >
>> > > > > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> > > > > > index 5e8857a..6af7595 100644
>> > > > > > --- a/drivers/pci/pci.h
>> > > > > > +++ b/drivers/pci/pci.h
>> > > > > > @@ -354,7 +354,7 @@ static inline resource_size_t
>> > > > > > pci_resource_alignment(struct pci_dev *dev,
>> > > > > > void pci_enable_acs(struct pci_dev *dev);
>> > > > > >
>> > > > > > /* PCI error reporting and recovery */
>> > > > > > -void pcie_do_fatal_recovery(struct pci_dev *dev);
>> > > > > > +void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service);
>> > > > > > void pcie_do_nonfatal_recovery(struct pci_dev *dev);
>> > > > > >
>> > > > > > bool pcie_wait_for_link(struct pci_dev *pdev, bool active);
>> > > > > > diff --git a/drivers/pci/pcie/aer/aerdrv_core.c
>> > > > > > b/drivers/pci/pcie/aer/aerdrv_core.c
>> > > > > > index fdfc474..36e622d 100644
>> > > > > > --- a/drivers/pci/pcie/aer/aerdrv_core.c
>> > > > > > +++ b/drivers/pci/pcie/aer/aerdrv_core.c
>> > > > > > @@ -254,7 +254,7 @@ static void handle_error_source(struct pcie_device
>> > > > > > *aerdev,
>> > > > > > } else if (info->severity == AER_NONFATAL)
>> > > > > > pcie_do_nonfatal_recovery(dev);
>> > > > > > else if (info->severity == AER_FATAL)
>> > > > > > - pcie_do_fatal_recovery(dev);
>> > > > > > + pcie_do_fatal_recovery(dev, PCIE_PORT_SERVICE_AER);
>> > > > > > }
>> > > > > >
>> > > > > > #ifdef CONFIG_ACPI_APEI_PCIEAER
>> > > > > > @@ -321,7 +321,7 @@ static void aer_recover_work_func(struct work_struct
>> > > > > > *work)
>> > > > > > if (entry.severity == AER_NONFATAL)
>> > > > > > pcie_do_nonfatal_recovery(pdev);
>> > > > > > else if (entry.severity == AER_FATAL)
>> > > > > > - pcie_do_fatal_recovery(pdev);
>> > > > > > + pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_AER);
>> > > > > > pci_dev_put(pdev);
>> > > > > > }
>> > > > > > }
>> > > > > > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
>> > > > > > index 80ec384..5680c13 100644
>> > > > > > --- a/drivers/pci/pcie/dpc.c
>> > > > > > +++ b/drivers/pci/pcie/dpc.c
>> > > > > > @@ -73,29 +73,31 @@ static void dpc_wait_link_inactive(struct dpc_dev
>> > > > > > *dpc)
>> > > > > > pcie_wait_for_link(pdev, false);
>> > > > > > }
>> > > > > >
>> > > > > > -static void dpc_work(struct work_struct *work)
>> > > > > > +static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
>> > > > > > {
>> > > > > > - struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
>> > > > > > - struct pci_dev *dev, *temp, *pdev = dpc->dev->port;
>> > > > > > - struct pci_bus *parent = pdev->subordinate;
>> > > > > > - u16 cap = dpc->cap_pos, ctl;
>> > > > > > -
>> > > > > > - pci_lock_rescan_remove();
>> > > > > > - list_for_each_entry_safe_reverse(dev, temp, &parent->devices,
>> > > > > > - bus_list) {
>> > > > > > - pci_dev_get(dev);
>> > > > > > - pci_dev_set_disconnected(dev, NULL);
>> > > > > > - if (pci_has_subordinate(dev))
>> > > > > > - pci_walk_bus(dev->subordinate,
>> > > > > > - pci_dev_set_disconnected, NULL);
>> > > > > > - pci_stop_and_remove_bus_device(dev);
>> > > > > > - pci_dev_put(dev);
>> > > > > > - }
>> > > > > > - pci_unlock_rescan_remove();
>> > > > > > -
>> > > > > > + struct dpc_dev *dpc;
>> > > > > > + struct pcie_device *pciedev;
>> > > > > > + struct device *devdpc;
>> > > > > > + u16 cap, ctl;
>> > > > > > +
>> > > > > > + /*
>> > > > > > + * DPC disables the Link automatically in hardware, so it has
>> > > > > > + * already been reset by the time we get here.
>> > > > > > + */
>> > > > > > +
>> > > > > > + devdpc = pcie_port_find_device(pdev, PCIE_PORT_SERVICE_DPC);
>> > > > > > + pciedev = to_pcie_device(devdpc);
>> > > > > > + dpc = get_service_data(pciedev);
>> > > > > > + cap = dpc->cap_pos;
>> > > > > > +
>> > > > > > + /*
>> > > > > > + * Waiting until the link is inactive, then clearing DPC
>> > > > > > + * trigger status to allow the port to leave DPC.
>> > > > > > + */
>> > > > > > dpc_wait_link_inactive(dpc);
>> > > > > > +
>> > > > > > if (dpc->rp_extensions && dpc_wait_rp_inactive(dpc))
>> > > > > > - return;
>> > > > > > + return PCI_ERS_RESULT_DISCONNECT;
>> > > > > > if (dpc->rp_extensions && dpc->rp_pio_status) {
>> > > > > > pci_write_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS,
>> > > > > > dpc->rp_pio_status);
>> > > > > > @@ -108,6 +110,17 @@ static void dpc_work(struct work_struct *work)
>> > > > > > pci_read_config_word(pdev, cap + PCI_EXP_DPC_CTL, &ctl);
>> > > > > > pci_write_config_word(pdev, cap + PCI_EXP_DPC_CTL,
>> > > > > > ctl | PCI_EXP_DPC_CTL_INT_EN);
>> > > > > > +
>> > > > > > + return PCI_ERS_RESULT_RECOVERED;
>> > > > > > +}
>> > > > > > +
>> > > > > > +static void dpc_work(struct work_struct *work)
>> > > > > > +{
>> > > > > > + struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
>> > > > > > + struct pci_dev *pdev = dpc->dev->port;
>> > > > > > +
>> > > > > > + /* From DPC point of view error is always FATAL. */
>> > > > > > + pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_DPC);
>> > > > > > }
>> > > > > >
>> > > > > > static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
>> > > > > > @@ -288,6 +301,7 @@ static struct pcie_port_service_driver dpcdriver = {
>> > > > > > .service = PCIE_PORT_SERVICE_DPC,
>> > > > > > .probe = dpc_probe,
>> > > > > > .remove = dpc_remove,
>> > > > > > + .reset_link = dpc_reset_link,
>> > > > > > };
>> > > > > >
>> > > > > > static int __init dpc_service_init(void)
>> > > > > > diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>> > > > > > index 33a16b1..29ff148 100644
>> > > > > > --- a/drivers/pci/pcie/err.c
>> > > > > > +++ b/drivers/pci/pcie/err.c
>> > > > > > @@ -185,7 +185,7 @@ static pci_ers_result_t default_reset_link(struct
>> > > > > > pci_dev *dev)
>> > > > > > return PCI_ERS_RESULT_RECOVERED;
>> > > > > > }
>> > > > > >
>> > > > > > -static pci_ers_result_t reset_link(struct pci_dev *dev)
>> > > > > > +static pci_ers_result_t reset_link(struct pci_dev *dev, u32 service)
>> > > > > > {
>> > > > > > struct pci_dev *udev;
>> > > > > > pci_ers_result_t status;
>> > > > > > @@ -200,7 +200,7 @@ static pci_ers_result_t reset_link(struct pci_dev
>> > > > > > *dev)
>> > > > > > }
>> > > > > >
>> > > > > > /* Use the aer driver of the component firstly */
>> > > > > > - driver = pcie_port_find_service(udev, PCIE_PORT_SERVICE_AER);
>> > > > > > + driver = pcie_port_find_service(udev, service);
>> > > > > >
>> > > > > > if (driver && driver->reset_link) {
>> > > > > > status = driver->reset_link(udev);
>> > > > > > @@ -287,7 +287,7 @@ static pci_ers_result_t
>> > > > > > broadcast_error_message(struct pci_dev *dev,
>> > > > > > * followed by re-enumeration of devices.
>> > > > > > */
>> > > > > >
>> > > > > > -void pcie_do_fatal_recovery(struct pci_dev *dev)
>> > > > > > +void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
>> > > > > > {
>> > > > > > struct pci_dev *udev;
>> > > > > > struct pci_bus *parent;
>> > > > > > @@ -313,7 +313,7 @@ void pcie_do_fatal_recovery(struct pci_dev *dev)
>> > > > > > pci_dev_put(pdev);
>> > > > > > }
>> > > > > >
>> > > > > > - result = reset_link(udev);
>> > > > > > + result = reset_link(udev, service);
>> > > > > >
>> > > > > > if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
>> > > > > > /*
>> > > > > > diff --git a/include/linux/aer.h b/include/linux/aer.h
>> > > > > > index 8f87bbe..0c506fe 100644
>> > > > > > --- a/include/linux/aer.h
>> > > > > > +++ b/include/linux/aer.h
>> > > > > > @@ -14,6 +14,7 @@
>> > > > > > #define AER_NONFATAL 0
>> > > > > > #define AER_FATAL 1
>> > > > > > #define AER_CORRECTABLE 2
>> > > > > > +#define DPC_FATAL 4
>> > > >
>> > > > I think DPC_FATAL can be 3, since these values are not used as a bit
>> > > > mask.
>> > > >
>> > > > > > struct pci_dev;
>> > > > >
>> > > > >
>> > > > > Hi Bjorn,
>> > > > >
>> > > > >
>> > > > > I have addressed all the comments, and I hope we are heading towards
>> > > > > closure.
>> > > > >
>> > > > > I just figure that pcie_do_fatal_recovery (which is getting
>> > > > > executed for
>> > > > > DPC as well)
>> > > > >
>> > > > > if (result == PCI_ERS_RESULT_RECOVERED) {
>> > > > > if (pcie_wait_for_link(udev, true))
>> > > > > pci_rescan_bus(udev->bus);
>> > > > > pci_info(dev, "Device recovery successful\n");
>> > > > > }
>> > > > >
>> > > > > I have to correct it to
>> > > > >
>> > > > > if (service==AER && result == PCI_ERS_RESULT_RECOVERED) {
>> > > > > if (pcie_wait_for_link(udev, true))
>> > > > > pci_rescan_bus(udev->bus);
>> > > > > pci_info(dev, "Device recovery successful\n");
>> > > > > }
>> > > >
>> > > > This patch is mostly a restructuring of DPC and doesn't really change
>> > > > its
>> > > > behavior. DPC didn't previously call pcie_wait_for_link() or
>> > > > pci_rescan_bus(), so I agree that adding the "service==AER" test will
>> > > > help preserve the existing DPC behavior.
>> > > >
>> > > > However, the rescan should happen with DPC *somewhere* and we should
>> > > > clarify where that is. Maybe for now we only need a comment about where
>> > > > that happens. Ideally, we could eventually converge this so the same
>> > > > mechanism is used for AER and DPC, so we wouldn't need a test like
>> > > > "service=AER".
>> >
>> > Please still add a comment here about why the rescan behavior is
>> > different.
>> >
>> > > I am sorry I pasted the wrong snippet.
>> > > following needs to be fixed in v17.
>> > > from:
>> > > if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
>> > > /*
>> > > * If the error is reported by a bridge, we think
>> > > this error
>> > > * is related to the downstream link of the bridge,
>> > > so we
>> > > * do error recovery on all subordinates of the bridge
>> > > instead
>> > > * of the bridge and clear the error status of the
>> > > bridge.
>> > > */
>> > > pci_walk_bus(dev->subordinate, report_resume,
>> > > &result_data);
>> > > pci_cleanup_aer_uncorrect_error_status(dev);
>> > > }
>> > >
>> > >
>> > > to
>> > >
>> > > if (service==AER && dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
>> > > /*
>> > > * If the error is reported by a bridge, we think
>> > > this error
>> > > * is related to the downstream link of the bridge,
>> > > so we
>> > > * do error recovery on all subordinates of the bridge
>> > > instead
>> > > * of the bridge and clear the error status of the
>> > > bridge.
>> > > */
>> > > pci_walk_bus(dev->subordinate, report_resume,
>> > > &result_data);
>> > > pci_cleanup_aer_uncorrect_error_status(dev);
>> > > }
>> > >
>> > > this is only needed in case of AER.
>> >
>> > Oh, I missed this before. It makes sense to clear the AER status
>> > here, but why do we need to call report_resume()? We just called all
>> > the driver .remove() methods and detached the drivers from the
>> > devices. So I don't think report_resume() will do anything
>> > ("dev->driver" should be NULL) except set the dev->error_state to
>> > pci_channel_io_normal. We probably don't need that because we didn't
>> > change error_state in this fatal error path.
>>
>> if you remember, the path ends up calling
>> aer_error_resume
>>
>> the existing code ends up calling aer_error_resume as follows.
>>
>> do_recovery(pci_dev)
>> broadcast_error_message(..., error_detected, ...)
>> if (AER_FATAL)
>> reset_link(pci_dev)
>> udev = BRIDGE ? pci_dev : pci_dev->bus->self
>> driver->reset_link(udev)
>> aer_root_reset(udev)
>> if (CAN_RECOVER)
>> broadcast_error_message(..., mmio_enabled, ...)
>> if (NEED_RESET)
>> broadcast_error_message(..., slot_reset, ...)
>> broadcast_error_message(dev, ..., report_resume, ...)
>> if (BRIDGE)
>> report_resume
>> driver->resume
>> pcie_portdrv_err_resume
>> device_for_each_child(..., resume_iter)
>> resume_iter
>> driver->error_resume
>> aer_error_resume
>> pci_cleanup_aer_uncorrect_error_status(pci_dev) # only
>> if
>> BRIDGE
>> pci_write_config_dword(PCI_ERR_UNCOR_STATUS)
>>
>> hence I think we have to call it in order to clear the root port
>> PCI_ERR_UNCOR_STATUS and PCI_EXP_DEVSTA.
>> makes sense ?
>
> I know I sent you the call graph above, so you would think I might
> understand it, but you would be mistaken :) This still doesn't make
> sense to me.
>
> I think your point is that we need to call aer_error_resume(). That
> is the aerdriver.error_resume() method. The AER driver only binds to
> root ports.
>
> This path:
>
> pcie_do_fatal_recovery
> pci_walk_bus(dev->subordinate, report_resume, &result_data)
>
> calls report_resume() for every device on the dev->subordinate bus
> (and for anything below those devices). There are no root ports on
> that dev->subordinate bus, because root ports are always on a root
> bus, never on a subordinate bus.
>
> So I don't see how report_resume() can ever get to aer_error_resume().
> Can you instrument that path and verify that it actually does get
> there somehow?
you are right....the call
pci_walk_bus(dev->subordinate, report_resume, &result_data);
does not call aer_error_resume()
but
pci_walk_bus(udev->bus, report_resume, &result_data);
does call aer_error_resume()
now if you look at the comment of the function:
/**
* aer_error_resume - clean up corresponding error status bits
* @dev: pointer to Root Port's pci_dev data structure
*
* Invoked by Port Bus driver during nonfatal recovery.
*/
it is invoked during nonfatal recovery.
but the code handles both fatal and nonfatal clearing of error bits.
if (dev->error_state == pci_channel_io_normal)
status &= ~mask; /* Clear corresponding nonfatal bits */
else
status &= mask; /* Clear corresponding fatal bits */
pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, status);
so the question is, should we not call aer_error_resume during fatal
recovery ?
so that it clears the root port status, if of course the error is
triggered by AER running agent (RP, Switch)
Regards,
Oza.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v16 8/9] PCI/DPC: Unify and plumb error handling into DPC
2018-05-16 14:58 ` poza
@ 2018-05-16 20:02 ` Bjorn Helgaas
0 siblings, 0 replies; 29+ messages in thread
From: Bjorn Helgaas @ 2018-05-16 20:02 UTC (permalink / raw)
To: poza
Cc: Bjorn Helgaas, Philippe Ombredanne, Thomas Gleixner,
Greg Kroah-Hartman, Kate Stewart, linux-pci, linux-kernel,
Dongdong Liu, Keith Busch, Wei Zhang, Sinan Kaya, Timur Tabi,
linux-pci-owner
On Wed, May 16, 2018 at 08:28:39PM +0530, poza@codeaurora.org wrote:
> On 2018-05-16 18:34, Bjorn Helgaas wrote:
> > On Wed, May 16, 2018 at 05:45:58PM +0530, poza@codeaurora.org wrote:
> > > On 2018-05-16 16:22, Bjorn Helgaas wrote:
> > > > On Wed, May 16, 2018 at 01:46:25PM +0530, poza@codeaurora.org wrote:
> > > > > I am sorry I pasted the wrong snippet.
> > > > > following needs to be fixed in v17.
> > > > > from:
> > > > > if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
> > > > > /*
> > > > > * If the error is reported by a bridge, we think
> > > > > this error
> > > > > * is related to the downstream link of the bridge,
> > > > > so we
> > > > > * do error recovery on all subordinates of the bridge
> > > > > instead
> > > > > * of the bridge and clear the error status of the
> > > > > bridge.
> > > > > */
> > > > > pci_walk_bus(dev->subordinate, report_resume,
> > > > > &result_data);
> > > > > pci_cleanup_aer_uncorrect_error_status(dev);
> > > > > }
> > > > >
> > > > >
> > > > > to
> > > > >
> > > > > if (service==AER && dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
> > > > > /*
> > > > > * If the error is reported by a bridge, we think
> > > > > this error
> > > > > * is related to the downstream link of the bridge,
> > > > > so we
> > > > > * do error recovery on all subordinates of the bridge
> > > > > instead
> > > > > * of the bridge and clear the error status of the
> > > > > bridge.
> > > > > */
> > > > > pci_walk_bus(dev->subordinate, report_resume,
> > > > > &result_data);
> > > > > pci_cleanup_aer_uncorrect_error_status(dev);
> > > > > }
> > > > >
> > > > > this is only needed in case of AER.
> > > >
> > > > Oh, I missed this before. It makes sense to clear the AER status
> > > > here, but why do we need to call report_resume()? We just called all
> > > > the driver .remove() methods and detached the drivers from the
> > > > devices. So I don't think report_resume() will do anything
> > > > ("dev->driver" should be NULL) except set the dev->error_state to
> > > > pci_channel_io_normal. We probably don't need that because we didn't
> > > > change error_state in this fatal error path.
> > >
> > > if you remember, the path ends up calling
> > > aer_error_resume
> > >
> > > the existing code ends up calling aer_error_resume as follows.
> > >
> > > do_recovery(pci_dev)
> > > broadcast_error_message(..., error_detected, ...)
> > > if (AER_FATAL)
> > > reset_link(pci_dev)
> > > udev = BRIDGE ? pci_dev : pci_dev->bus->self
> > > driver->reset_link(udev)
> > > aer_root_reset(udev)
> > > if (CAN_RECOVER)
> > > broadcast_error_message(..., mmio_enabled, ...)
> > > if (NEED_RESET)
> > > broadcast_error_message(..., slot_reset, ...)
> > > broadcast_error_message(dev, ..., report_resume, ...)
> > > if (BRIDGE)
> > > report_resume
> > > driver->resume
> > > pcie_portdrv_err_resume
> > > device_for_each_child(..., resume_iter)
> > > resume_iter
> > > driver->error_resume
> > > aer_error_resume
> > > pci_cleanup_aer_uncorrect_error_status(pci_dev) # only
> > > if
> > > BRIDGE
> > > pci_write_config_dword(PCI_ERR_UNCOR_STATUS)
> > >
> > > hence I think we have to call it in order to clear the root port
> > > PCI_ERR_UNCOR_STATUS and PCI_EXP_DEVSTA.
> > > makes sense ?
> >
> > I know I sent you the call graph above, so you would think I might
> > understand it, but you would be mistaken :) This still doesn't make
> > sense to me.
> >
> > I think your point is that we need to call aer_error_resume(). That
> > is the aerdriver.error_resume() method. The AER driver only binds to
> > root ports.
> >
> > This path:
> >
> > pcie_do_fatal_recovery
> > pci_walk_bus(dev->subordinate, report_resume, &result_data)
> >
> > calls report_resume() for every device on the dev->subordinate bus
> > (and for anything below those devices). There are no root ports on
> > that dev->subordinate bus, because root ports are always on a root
> > bus, never on a subordinate bus.
> >
> > So I don't see how report_resume() can ever get to aer_error_resume().
> > Can you instrument that path and verify that it actually does get
> > there somehow?
>
> you are right....the call
> pci_walk_bus(dev->subordinate, report_resume, &result_data);
> does not call aer_error_resume()
>
> but
> pci_walk_bus(udev->bus, report_resume, &result_data);
> does call aer_error_resume()
>
> now if you look at the comment of the function:
> /**
> * aer_error_resume - clean up corresponding error status bits
> * @dev: pointer to Root Port's pci_dev data structure
> *
> * Invoked by Port Bus driver during nonfatal recovery.
> */
>
> it is invoked during nonfatal recovery.
> but the code handles both fatal and nonfatal clearing of error bits.
>
> if (dev->error_state == pci_channel_io_normal)
> status &= ~mask; /* Clear corresponding nonfatal bits */
> else
> status &= mask; /* Clear corresponding fatal bits */
> pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, status);
>
>
> so the question is, should we not call aer_error_resume during fatal
> recovery ?
> so that it clears the root port status, if of course the error is triggered
> by AER running agent (RP, Switch)
I'm sure we *should* clear AER status bits somewhere during ERR_FATAL
recovery.
As far as I can tell, the current code (before your patches) never
calls aer_error_resume(). That might be a bug, but even if it is,
it's something that should be fixed separately from *this* series.
I think in this series, you should probably adjust the patch that adds
do_fatal_recovery() so it doesn't call pci_walk_bus(..., report_resume).
I don't think that does anything useful anyway, and that patch already
changes AER so it doesn't call the pci_error_handlers callbacks
(except .resume()). I think it would be cleaner to remove the
ERR_FATAL use of .resume() at the same time you remove the others.
^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2018-05-16 20:02 UTC | newest]
Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-11 10:43 [PATCH v16 0/9] Address error and recovery for AER and DPC Oza Pawandeep
2018-05-11 10:43 ` [PATCH v16 1/9] PCI: Unify wait for link active into generic PCI Oza Pawandeep
2018-05-11 10:43 ` [PATCH v16 2/9] pci-error-recovery: Add AER_FATAL handling Oza Pawandeep
2018-05-11 10:43 ` [PATCH v16 3/9] PCI/AER: Handle ERR_FATAL with removal and re-enumeration of devices Oza Pawandeep
2018-05-15 23:59 ` Bjorn Helgaas
2018-05-16 5:49 ` poza
2018-05-11 10:43 ` [PATCH v16 4/9] PCI/AER: Rename error recovery to generic PCI naming Oza Pawandeep
2018-05-11 10:43 ` [PATCH v16 5/9] PCI/AER: Factor out error reporting from AER Oza Pawandeep
2018-05-11 12:58 ` Lukas Wunner
2018-05-11 15:34 ` poza
2018-05-11 15:54 ` Lukas Wunner
2018-05-11 16:11 ` poza
2018-05-16 0:06 ` Bjorn Helgaas
2018-05-11 10:43 ` [PATCH v16 6/9] PCI/PORTDRV: Implement generic find service Oza Pawandeep
2018-05-11 10:43 ` [PATCH v16 7/9] PCI/PORTDRV: Implement generic find device Oza Pawandeep
2018-05-11 10:43 ` [PATCH v16 8/9] PCI/DPC: Unify and plumb error handling into DPC Oza Pawandeep
2018-05-11 11:52 ` poza
2018-05-15 23:56 ` Bjorn Helgaas
2018-05-16 8:16 ` poza
2018-05-16 10:52 ` Bjorn Helgaas
2018-05-16 12:15 ` poza
2018-05-16 13:04 ` Bjorn Helgaas
2018-05-16 13:58 ` poza
2018-05-16 14:58 ` poza
2018-05-16 20:02 ` Bjorn Helgaas
2018-05-16 12:51 ` poza
2018-05-16 13:09 ` Bjorn Helgaas
2018-05-11 10:43 ` [PATCH v16 9/9] PCI/DPC: Disable ERR_NONFATAL and enable ERR_FATAL for DPC Oza Pawandeep
2018-05-16 0:09 ` [PATCH v16 0/9] Address error and recovery for AER and DPC Bjorn Helgaas
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).