Address error and recovery for AER and DPC
Subsystems (4)
v17 cover Address error and recovery for AER and DPC
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 v16:
Bjorn's comments addressed
> remove call pci_walk_bus(dev->subordinate, report_resume, &result_data) > pci_cleanup_aer_uncorrect_error_status(dev); happens only if service is AER > aer_error_resume does not handle ERR_FATAL clearing anymore
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
Signature · 1 line
2.7.4
PATCH v17 1/9 PCI: Add generic pcie_wait_for_link() interface
Clients such as hotplug and Downstream Port Containment (DPC) both need to wait until a link becomes active or inactive.
Add a generic pcie_wait_link_active() interface and use it instead of duplicating the code.
drivers/pci/hotplug/pciehp_hpc.c +3 -17
@@ -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)
drivers/pci/pci.c +29 -0
@@ -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 until link is active or 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) {
drivers/pci/pci.h +1 -0
@@ -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);
drivers/pci/pcie/dpc.c +1 -11
@@ -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)
Signature · 1 line
2.7.4
PATCH v17 2/9 PCI/AER: Handle ERR_FATAL with removal and re-enumeration of devices
In reply to Oza PawandeepPCIe ERR_FATAL errors mean the Link is unreliable. Components on the Link may need to be reset to return to reliable operation (PCIe r4.0, sec 6.2.2). We previously handled these errors much differently depending on whether the platform supports Downstream Port Containment (DPC) (PCIe r4.0, sec 6.2.10) or not.
The AER driver has historically logged the error details, called driver-supplied pci_error_handlers callbacks, and reset the Link. This reset downstream devices, but did not remove them from the PCI subsystem, re-enumerate them, or call their driver .remove() or .probe() methods.
DPC is different because the hardware automatically disables the Link when it detects ERR_FATAL, which resets downstream devices. There's no opportunity for pci_error_handlers callbacks before resetting the Link. The DPC driver removes affected devices (which calls their .remove() methods), brings the Link back up, and re-enumerates (which calls driver .probe() methods).
Align AER ERR_FATAL handling with DPC by resetting the Link in software, skipping the driver pci_error_handlers callbacks, removing the devices from the PCI subsystem, and re-enumerating. The idea is that drivers and devices should see the same behavior for ERR_FATAL events, regardless of whether they're handled by AER or DPC.
Here are the basic ERR_FATAL recovery steps, showing the previous AER behavior, the AER behavior after this patch, and the DPC behavior:
AER AER DPC
previous new behavior
-------- --- --------
Log error yes yes yes (minimal)
drv.error_detected() yes no no
Reset Link yes yes yes
drv.mmio_enabled() yes no no
drv.slot_reset() yes no no
drv.resume() yes no no
Remove PCI devices no yes yes
(calls drv.remove())
Re-enumerate no yes yes
(calls drv.probe())N.B. With DPC, the Link reset happens before the driver .remove() calls, while with AER, the reset happens *after* the .remove() calls.
Documentation/PCI/pci-error-recovery.txt +25 -10
@@ -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 ---------------------------
drivers/pci/pcie/aer/aerdrv.c +1 -4
@@ -353,10 +353,7 @@ static void aer_error_resume(struct pci_dev *dev) pos = dev->aer_cap;pci_read_config_dword (dev, pos +PCI_ERR_UNCOR_STATUS , &status);pci_read_config_dword (dev, pos +PCI_ERR_UNCOR_SEVER , &mask); -if (dev->error_state == pci_channel_io_normal) - status &= ~mask;/* Clear corresponding nonfatal bits */ -else - status &= mask;/* Clear corresponding fatal bits */ + status &= ~mask;/* Clear corresponding nonfatal bits */ pci_write_config_dword (dev, pos +PCI_ERR_UNCOR_STATUS , status); }
drivers/pci/pcie/aer/aerdrv_core.c +71 -19
128 lines not on this page · raw message
Signature · 1 line
2.7.4
PATCH v17 3/9 PCI/AER: Rename error recovery interfaces to generic PCI naming
In reply to Oza PawandeepRename error recovery interfaces with "pcie_" prefix so they can be made non-static.
drivers/pci/pcie/aer/aerdrv_core.c +8 -8
@@ -476,14 +476,14 @@ 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 * beneath 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) +static void pcie_do_fatal_recovery (struct pci_dev *dev) {struct pci_dev *udev;struct pci_bus *parent;
@@ -533,14 +533,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. 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) +static void pcie_do_nonfatal_recovery (struct pci_dev *dev) {pci_ers_result_t status;enum pci_channel_state state;
@@ -611,9 +611,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
@@ -678,9 +678,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); } }
Signature · 1 line
2.7.4
PATCH v17 4/9 PCI/AER: Factor out error reporting to drivers/pci/pcie/err.c
In reply to Oza PawandeepMove the error reporting callbacks from aerdrv_core.c to err.c, where they can be used by DPC in addition to AER.
As part of aerdrv_core.c, these callbacks were built under CONFIG_PCIEAER. Moving them to the new err.c means they will now be built under CONFIG_PCIEPORTBUS, so adjust the definition of pci_uevent_ers() to match.
[bhelgaas: in reset_link(), initialize "driver" even if CONFIG_PCIEAER is unset, update pci_uevent_ers() #ifdef wrapper]
drivers/pci/pci-driver.c +1 -1
@@ -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
drivers/pci/pci.h +4 -0
@@ -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);
drivers/pci/pcie/Makefile +1 -1
@@ -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
drivers/pci/pcie/aer/aerdrv.h +0 -30
@@ -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);
drivers/pci/pcie/aer/aerdrv_core.c +1 -334
353 lines not on this page · raw message
drivers/pci/pcie/err.c new file +390 -0
390 lines not on this page · raw message
drivers/pci/pcie/portdrv.h +1 -0
@@ -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_ */
include/linux/pci.h +1 -1
@@ -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
Signature · 1 line
2.7.4
PATCH v17 5/9 PCI/portdrv: Add generic pcie_port_find_service()
In reply to Oza PawandeepAdd generic pcie_port_find_service() routine.
drivers/pci/pcie/aer/aerdrv_core.c +0 -26
@@ -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
drivers/pci/pcie/err.c +1 -3
@@ -194,10 +194,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);
drivers/pci/pcie/portdrv.h +2 -1
@@ -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_ */
drivers/pci/pcie/portdrv_core.c +44 -0
56 lines not on this page · raw message
Signature · 1 line
2.7.4
PATCH v17 6/9 PCI/portdrv: Add generic pcie_port_find_device()
In reply to Oza PawandeepAdd generic pcie_port_find_device() routine.
drivers/pci/pcie/portdrv.h +2 -0
@@ -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_ */
drivers/pci/pcie/portdrv_core.c +23 -0
@@ -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 is associated with +* @service: For the service to find +* +* Find the struct device associated with given service on a pci_dev +*/ +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 *
Signature · 1 line
2.7.4
PATCH v17 7/9 PCI/DPC: Disable ERR_NONFATAL handling by DPC
In reply to Oza PawandeepPCIe ERR_NONFATAL errors mean a particular transaction is unreliable but the Link is otherwise fully functional (PCIe r4.0, sec 6.2.2).
The AER driver handles these by logging the error details and calling driver-supplied pci_error_handlers callbacks. It does not reset downstream devices, does not remove them from the PCI subsystem, does not re-enumerate them, and does not call their driver .remove() or .probe() methods.
But DPC driver previously enabled DPC on ERR_NONFATAL, so if the hardware supports DPC, these errors caused a Link reset (performed automatically by the hardware), followed by the DPC driver removing affected devices (which calls their .remove() methods), bringing the Link back up, and re-enumerating (which calls driver .probe() methods).
Disable ERR_NONFATAL DPC triggering so these errors will only be handled by AER. This means drivers won't have to deal with different usage of their pci_error_handlers callbacks and .probe() and .remove() methods based on whether the platform has DPC support.
drivers/pci/pcie/dpc.c +2 -2
@@ -260,7 +260,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",
@@ -278,7 +278,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); }
include/uapi/linux/pci_regs.h +1 -0
@@ -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 */
Signature · 1 line
2.7.4
PATCH v17 8/9 PCI/AER: Pass service type to pcie_do_fatal_recovery()
In reply to Oza PawandeepPass the service type to pcie_do_fatal_recovery() instead of assuming AER. We will make DPC also use pcie_do_fatal_recovery(), and it needs to do things a little differently for AER and DPC.
drivers/pci/pci.h +1 -1
@@ -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);
drivers/pci/pcie/aer/aerdrv_core.c +2 -2
@@ -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); } }
drivers/pci/pcie/err.c +6 -5
@@ -180,7 +180,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;
@@ -195,7 +195,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);
@@ -281,7 +281,7 @@ static pci_ers_result_t broadcast_error_message(struct pci_dev *dev,* 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) +void pcie_do_fatal_recovery (struct pci_dev *dev,u32 service) {struct pci_dev *udev;struct pci_bus *parent;
@@ -307,9 +307,10 @@ 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 ) { +if ((service ==PCIE_PORT_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
Signature · 1 line
2.7.4
PATCH v17 9/9 PCI/DPC: Use the generic pcie_do_fatal_recovery() path
In reply to Oza PawandeepOur goal is to handle ERR_FATAL errors similarly, whether they are reported via AER or via DPC. A previous commit changed AER so it handles ERR_FATAL by calling driver .remove() methods and resetting the Link. DPC already does that (although the Link reset is done automatically by hardware and happens before we call the driver .remove() methods).
Restructure the DPC code so it calls the same pcie_do_fatal_recovery() interface used by AER. This makes it clearer that we want to use the same path.
Implement the .reset_link() method used by pcie_do_fatal_recovery(). For DPC, the actual reset is done automatically by hardware, so we really only have to wait for the Link to be inactive, then release the Port from DPC.
drivers/pci/pcie/dpc.c +33 -20
@@ -73,29 +73,30 @@ 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; + +/* +* Wait until the Link is inactive, then clear 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 +109,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; + +/* We configure DPC so it only triggers on ERR_FATAL */ +pcie_do_fatal_recovery (pdev,PCIE_PORT_SERVICE_DPC ); }static void dpc_process_rp_pio_error (struct dpc_dev *dpc)
@@ -288,6 +300,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 )
include/linux/aer.h +1 -0
@@ -14,6 +14,7 @@#define AER_NONFATAL 0#define AER_FATAL 1#define AER_CORRECTABLE 2 +#define DPC_FATAL 3struct pci_dev ;
Signature · 1 line
2.7.4
[+cc Russell, Sam, Bryant, linuxppc-dev, Sebastian, linux-s390]
Sorry, I should have pulled in these new CC's earlier because ppc and s390 both have PCI error handling similar to what Oza is changing here.
The basic issue is that the new PCIe DPC (Downstream Port Containment, see PCIe r4.0, sec 6.2.10) feature doesn't fit very well in the framework of the pci_error_handlers callbacks.
When DPC is enabled, a Downstream Port (either a Root Port or a Switch Downstream Port) that receives an ERR_FATAL message automatically disables its Link. IIUC, this is also intended for use in hot-unplug scenarios.
When the DPC hardware takes the Link down, it resets all the downstream devices, and there's not much point in calling the pci_error_handlers callbacks because the devices are unreachable. Even after the Link comes back up, we can't be certain the same device is there because of the hotplug possibility.
The software side of DPC recovery basically consists of detaching the drivers of the downstream devices (calling their .remove() methods), bringing the link back up, re-enumerating the downstream devices, and re-attaching the drivers (calling their .probe() methods).
The existing AER code also responds to ERR_FATAL messages, but it does call the pci_error_handlers callbacks and also resets the link.
This is a bit of a mess because things look a lot different to the driver depending on whether the platform supports AER or DPC.
Since we can't change the way DPC works, the idea of this series is basically to make AER handle ERR_FATAL more like DPC does, i.e., by resetting the link, detaching, and re-attaching the drivers.
This series is currently on my pci/aer branch
(https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/aer)
and is headed for v4.18 unless somebody raises major objections.
Quoted 17 lines · 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.
Quoted 7 lines
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_FATALfollows the default path.Documentation/PCI/pci-error-recovery.txttalks more on that.
I applied this series with a trivial change to remove an unused variable to pci/aer for v4.18, thanks!
Quoted 111 lines
Changes since v16:
Bjorn's comments addressed
> remove callpci_walk_bus(dev->subordinate, report_resume, &result_data)>pci_cleanup_aer_uncorrect_error_status(dev); happens only if service is AER >aer_error_resumedoes not handleERR_FATALclearing anymore
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_recoveryandpcie_do_nonfatal_recoveryfunctions made to modularize >pcie_do_fatal_recoverynow takes service as an argument
Changes since v14:
Bjorn's comments addressed
> simplified the patch set, and movedAER_FATALhandling 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 <alignederr.c>
> hanldled cases for hotplug enabled system differently.
Changes since v11:
Bjorn's comments addressed.
> renamepcie-err.ctoerr.c
> removedEXPORT_SYMBOL
> made genericfind_serivcefunction in port driver.
> removed mutex patch as no need to have mutex inpcie_do_recovery
> brough inDPC_FATALinaer.h
> so now all the error codes (AER and DPC) are unified inaer.h
Changes since v10:
Christoph Hellwig's, David Laight's and Randy Dunlap's
comments addressed.
> renamedpci_do_recoverytopcie_do_recovery
> removed inner braces in conditional statements.
> restrctured the code inpci_wait_for_link
>EXPORT_SYMBOL_GPL
Changes since v9:
Sinan's comments addressed.
> bool active = true; unnecessary variable removed.
Changes since v8:
FixedKbuilderrors.
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 intodriver/pci/pci.h
> got rid of wait link active/inactive and
made generic function indriver/pci/pci.c
Changes since v4:
Bjorn's comments incorporated.
> Renamed onlydo_recovery.
> moved the things more locally todrivers/pci/pci.h
Changes since v3:
Bjorn's comments incorporated.
> Made separate patch renaming genericpci_err.c
> Introducepci_err.hto contain all the error types and recovery
> removed all the dependencies onpci.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:
Kbuilderrors fixed:
>pci_find_dpc_devmade static
>ras_event.hupdated
>pci_find_aer_servicecall with CONFIG check
>pci_find_dpc_servicecall with CONFIG checkOza Pawandeep (9):
PCI: Unify wait for link active into generic PCI
pci-error-recovery: AddAER_FATALhandling
PCI/AER: HandleERRR_FATALwith 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: DisableERR_NONFATALand enableERR_FATALfor DPCDocumentation/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.cSignature · 1 line
2.7.4