Address error and recovery for AER and DPC
Subsystems (4)
v10 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.
DPC should enumerate the devices after recovering the link, which is achieved by implementing error_resume callback.
Changes since v9:
Sinan's comments addressed.
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 (7):
PCI/AER: Rename error recovery to generic pci naming PCI/AER: factor out error reporting from AER PCI/ERR: add mutex to synchronize recovery PCI/DPC: Unify and plumb error handling into DPC PCI/AER: Unify aer error defines at single space PCI/DPC: Enumerate the devices after DPC trigger event
PCI: Unify wait for link active into generic pci
drivers/acpi/apei/ghes.c | 1 + drivers/pci/hotplug/pciehp_hpc.c | 21 +- drivers/pci/pci.c | 39 +++- drivers/pci/pci.h | 11 + drivers/pci/pcie/Makefile | 2 +- drivers/pci/pcie/aer/aerdrv.h | 30 --- drivers/pci/pcie/aer/aerdrv_core.c | 293 +------------------------- drivers/pci/pcie/aer/aerdrv_errprint.c | 1 + drivers/pci/pcie/pcie-dpc.c | 115 ++++++++++- drivers/pci/pcie/pcie-err.c | 366 +++++++++++++++++++++++++++++++++ drivers/pci/pcie/portdrv.h | 2 + include/linux/aer.h | 4 - include/linux/pci.h | 1 + 13 files changed, 534 insertions(+), 352 deletions(-) create mode 100644 drivers/pci/pcie/pcie-err.c
Signature · 2 lines
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc., a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
PATCH v10 1/7 PCI/AER: Rename error recovery to generic pci naming
This patch renames error recovery to generic name with pci prefix
drivers/pci/pcie/aer/aerdrv_core.c +4 -4
@@ -478,7 +478,7 @@ static pci_ers_result_t reset_link(struct pci_dev *dev) }/** -* do_recovery - handle nonfatal/fatal error recovery process +* pci_do_recovery - handle nonfatal/fatal error recovery process * @dev: pointer to a pci_dev data structure of agent detecting an error * @severity: error severity type *
@@ -486,7 +486,7 @@ static pci_ers_result_t reset_link(struct pci_dev *dev)* 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 pci_do_recovery (struct pci_dev *dev,int severity) {pci_ers_result_t status, result =PCI_ERS_RESULT_RECOVERED ;enum pci_channel_state state;
@@ -566,7 +566,7 @@ static void handle_error_source(struct pcie_device *aerdev,pci_write_config_dword (dev, pos +PCI_ERR_COR_STATUS , info->status); }else -do_recovery (dev, info->severity); +pci_do_recovery (dev, info->severity); }#ifdef CONFIG_ACPI_APEI_PCIEAER
@@ -631,7 +631,7 @@ static void aer_recover_work_func(struct work_struct *work) }cper_print_aer (pdev, entry.severity, entry.regs);if (entry.severity !=AER_CORRECTABLE ) -do_recovery (pdev, entry.severity); +pci_do_recovery (pdev, entry.severity);pci_dev_put (pdev); } }
Signature · 2 lines
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc., a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH v10 1/7] PCI/AER: Rename error recovery to generic pci naming
Quoted 18 lines · Oza Pawandeep
This patch renames error recovery to generic name with pci prefix
Signed-off-by Oza Pawandeep <[email protected]>drivers/pci/pcie/aer/aerdrv_core.c +1
-1@@ -478,7 +478,7 @@ static pci_ers_result_t reset_link(struct pci_dev *dev) }/** -* do_recovery - handle nonfatal/fatal error recovery process +* pci_do_recovery - handle nonfatal/fatal error recovery process * @dev: pointer to a pci_dev data structure of agent detecting an error * @severity: error severity type *
drivers/pci/pcie/aer/aerdrv_core.c static pci_ers_result_t reset_link(struct pci_dev *dev) · line 489
Quoted 5 lines
drivers/pci/pcie/aer/aerdrv_core.c +1
-1@@ -486,7 +486,7 @@ static pci_ers_result_t reset_link(struct pci_dev *dev)* 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 pci_do_recovery (struct pci_dev *dev,int severity)
Maybe pcie_do_recovery?
Otherwise looks fine:
Re: [PATCH v10 1/7] PCI/AER: Rename error recovery to generic pci naming
Quoted 22 lines · Christoph Hellwig
Quoted 21 lines · Oza Pawandeep
This patch renames error recovery to generic name with pci prefix
Signed-off-by Oza Pawandeep <[email protected]>drivers/pci/pcie/aer/aerdrv_core.c +1
-1@@ -478,7 +478,7 @@ static pci_ers_result_t reset_link(struct pci_dev *dev) }/** -* do_recovery - handle nonfatal/fatal error recovery process +* pci_do_recovery - handle nonfatal/fatal error recovery process * @dev: pointer to a pci_dev data structure of agent detecting an error
* @severity: error severity type
*drivers/pci/pcie/aer/aerdrv_core.c +0
-0@@ -486,7 +486,7 @@ static pci_ers_result_t reset_link(struct pci_dev *dev)
Quoted 9 lines
drivers/pci/pcie/aer/aerdrv_core.c static pci_ers_result_t reset_link(struct pci_dev *dev) · line 489
Quoted 6 lines
drivers/pci/pcie/aer/aerdrv_core.c +0
-0* error detected message to all downstream drivers within a hierarchy in
* question and return the returned code.
*/drivers/pci/pcie/aer/aerdrv_core.c +1
-1-static void do_recovery (struct pci_dev *dev,int severity) +static void pci_do_recovery (struct pci_dev *dev,int severity)Maybe
pcie_do_recovery?
sure will take care.
Thanks.
Quoted 3 lines
Otherwise looks fine:
Reviewed-by Christoph Hellwig <[email protected]>
PATCH v10 2/7 PCI/AER: factor out error reporting from AER
In reply to Oza PawandeepThis patch factors out error reporting callbacks, which are currently tightly coupled with AER.
DPC should be able to register callbacks and attmept recovery when DPC trigger event occurs.
drivers/pci/pci.h +3 -0
@@ -342,6 +342,9 @@ 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 pci_do_recovery (struct pci_dev *dev,int severity); +#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/Makefile +1 -1
@@ -6,7 +6,7 @@ # Build PCI Express ASPM if needed obj-$(CONFIG_PCIEASPM) += aspm.o -pcieportdrv-y := portdrv_core.o portdrv_pci.o portdrv_bus.o +pcieportdrv-y := portdrv_core.o portdrv_pci.o portdrv_bus.o pcie-err.o pcieportdrv-$(CONFIG_ACPI) += portdrv_acpi.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 +3 -287
314 lines not on this page · raw message
drivers/pci/pcie/pcie-err.c new file +334 -0
334 lines not on this page · raw message
drivers/pci/pcie/portdrv.h +1 -0
@@ -79,4 +79,5 @@ static inline void pcie_port_platform_notify(struct pci_dev *port, int *mask)static inline void pcie_port_platform_notify (struct pci_dev *port,int *mask){}#endif /* !CONFIG_ACPI */ +struct pcie_port_service_driver *pci_find_aer_service (struct pci_dev *dev);#endif /* _PORTDRV_H_ */
Signature · 2 lines
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc., a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH v10 2/7] PCI/AER: factor out error reporting from AER
drivers/pci/pcie/aer/aerdrv_core.c static struct pcie_port_service_driver *find_aer_service(struct pci_dev *dev) · line 259
Quoted 5 lines
drivers/pci/pcie/aer/aerdrv_core.c +1
-4-pci_uevent_ers (dev,PCI_ERS_RESULT_DISCONNECT ); -/* TODO: Should kernel panic here? */ -pci_info (dev,"AER: Device recovery failed\n" ); -} +EXPORT_SYMBOL (pci_find_aer_service);
EXPORT_SYMBPOL_GPL please.
RE: [PATCH v10 2/7] PCI/AER: factor out error reporting from AER
From: Christoph Hellwig
Quoted 7 lines
drivers/pci/pcie/aer/aerdrv_core.c static struct pcie_port_service_driver *find_aer_service(struct pci_dev *dev) · line 259
Quoted 5 lines
drivers/pci/pcie/aer/aerdrv_core.c +1
-4-pci_uevent_ers (dev,PCI_ERS_RESULT_DISCONNECT ); -/* TODO: Should kernel panic here? */ -pci_info (dev,"AER: Device recovery failed\n" ); -} +EXPORT_SYMBOL (pci_find_aer_service);
EXPORT_SYMBPOL_GPLplease.
Is this a function that a 'normal' PCIe device driver would need to call? If so it is a PITA requiring all drivers that want AER to be GPL.
David
PATCH v10 3/7 PCI/ERR: add mutex to synchronize recovery
In reply to Oza PawandeepThis patch protects pci_do_recovery with mutex.
drivers/pci/pcie/pcie-err.c +6 -0
@@ -20,6 +20,8 @@#include <linux/pcieport_if.h> #include "portdrv.h" +static DEFINE_MUTEX (pci_err_recovery_lock); +struct aer_broadcast_data {enum pci_channel_state state;enum pci_ers_result result;
@@ -283,6 +285,8 @@ void pci_do_recovery(struct pci_dev *dev, int severity)pci_ers_result_t status, result =PCI_ERS_RESULT_RECOVERED ;enum pci_channel_state state; +mutex_lock (&pci_err_recovery_lock); +if (severity ==AER_FATAL ) state = pci_channel_io_frozen;else
@@ -326,9 +330,11 @@ void pci_do_recovery(struct pci_dev *dev, int severity) report_resume);dev_info (&dev->dev,"Device recovery successful\n" ); +mutex_unlock (&pci_err_recovery_lock);return ; failed:/* TODO: Should kernel panic here? */ dev_info (&dev->dev,"Device recovery failed\n" ); +mutex_unlock (&pci_err_recovery_lock); }
Signature · 2 lines
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc., a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH v10 3/7] PCI/ERR: add mutex to synchronize recovery
Quoted 1 line · Oza Pawandeep
This patch protects
pci_do_recoverywith mutex.
Do we really want a global mutex for this and not one per root complex at very least?
Re: [PATCH v10 3/7] PCI/ERR: add mutex to synchronize recovery
Quoted 5 lines · Christoph Hellwig
Quoted 1 line · Oza Pawandeep
This patch protects
pci_do_recoverywith mutex.Do we really want a global mutex for this and not one per root complex at very least?
This protects pcie_do_recovery, which is called by port service drivers. mainly aer and dpc as of now. so the association is not root complex agents instead service drivers who attempts to do link recovery.
so I think we are good here.
PATCH v10 4/7 PCI/DPC: Unify and plumb error handling into DPC
In reply to Oza PawandeepCurrent DPC driver does not do recovery, e.g. calling end-point's driver's callbacks, which sanitize the sw.
DPC driver implements link_reset callback, and calls pci_do_recovery.
drivers/pci/pci.h +2 -0
@@ -343,6 +343,8 @@ 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 */ +#define DPC_FATAL 4 +void pci_do_recovery (struct pci_dev *dev,int severity);#ifdef CONFIG_PCIEASPM
drivers/pci/pcie/pcie-dpc.c +86 -7
137 lines not on this page · raw message
drivers/pci/pcie/pcie-err.c +21 -6
@@ -19,6 +19,7 @@#include <linux/aer.h> #include <linux/pcieport_if.h> #include "portdrv.h" +#include "./../pci.h" static DEFINE_MUTEX (pci_err_recovery_lock);
@@ -181,7 +182,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,int severity) {struct pci_dev *udev;pci_ers_result_t status;
@@ -195,9 +196,17 @@ static pci_ers_result_t reset_link(struct pci_dev *dev) udev = dev->bus->self; } + +/* Use the service driver of the component firstly */ +#if IS_ENABLED (CONFIG_PCIE_DPC ) +if (severity ==DPC_FATAL ) + driver =pci_find_dpc_service (udev); +#endif #if IS_ENABLED (CONFIG_PCIEAER ) -/* Use the aer driver of the component firstly */ - driver =pci_find_aer_service (udev); +if ((severity ==AER_FATAL ) || + (severity ==AER_NONFATAL ) || + (severity ==AER_CORRECTABLE )) + driver =pci_find_aer_service (udev);#endif if (driver && driver->reset_link) {
@@ -287,7 +296,8 @@ void pci_do_recovery(struct pci_dev *dev, int severity)mutex_lock (&pci_err_recovery_lock); -if (severity ==AER_FATAL ) +if ((severity ==AER_FATAL ) || + (severity ==DPC_FATAL )) state = pci_channel_io_frozen;else state = pci_channel_io_normal;
@@ -297,10 +307,14 @@ void pci_do_recovery(struct pci_dev *dev, int severity)"error_detected" , report_error_detected); -if (severity ==AER_FATAL ) { - result =reset_link (dev); +if ((severity ==AER_FATAL ) || + (severity ==DPC_FATAL )) { + result =reset_link (dev, severity);if (result !=PCI_ERS_RESULT_RECOVERED ) goto failed; +else if (severity ==DPC_FATAL ) + goto resume; + }if (status ==PCI_ERS_RESULT_CAN_RECOVER )
@@ -324,6 +338,7 @@ void pci_do_recovery(struct pci_dev *dev, int severity)if (status !=PCI_ERS_RESULT_RECOVERED ) goto failed; +resume: broadcast_error_message(dev, state, "resume",
drivers/pci/pcie/portdrv.h +1 -0
@@ -80,4 +80,5 @@ static inline void pcie_port_platform_notify(struct pci_dev *port, int *mask){}#endif /* !CONFIG_ACPI */ struct pcie_port_service_driver *pci_find_aer_service (struct pci_dev *dev); +struct pcie_port_service_driver *pci_find_dpc_service (struct pci_dev *dev);#endif /* _PORTDRV_H_ */
Signature · 2 lines
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc., a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH v10 4/7] PCI/DPC: Unify and plumb error handling into DPC
Quoted 35 lines · Oza Pawandeep
Current DPC driver does not do recovery, e.g. calling end-point's driver's callbacks, which sanitize the sw.
DPC driver implements
link_resetcallback, and callspci_do_recovery.Signed-off-by Oza Pawandeep <[email protected]>drivers/pci/pci.h +2
-0@@ -343,6 +343,8 @@ 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 */ +#define DPC_FATAL 4 +void pci_do_recovery (struct pci_dev *dev,int severity);#ifdef CONFIG_PCIEASPM drivers/pci/pcie/pcie-dpc.c +1
-0@@ -13,6 +13,7 @@#include <linux/pcieport_if.h> #include "../pci.h" #include "aer/aerdrv.h" +#include "portdrv.h" struct dpc_dev {struct pcie_device *dev;@@ -45,6 +46,60 @@ struct dpc_dev { "Memory Request Completion Timeout", /* Bit Position 18 */ };
drivers/pci/pcie/pcie-dpc.c struct dpc_dev { · line 57
Quoted 10 lines
drivers/pci/pcie/pcie-dpc.c +9
-0+static int find_dpc_dev_iter (struct device *device,void *data) +{ +struct pcie_port_service_driver *service_driver; +struct device **dev; + + dev = (struct device **) data; + +if (device->bus == &pcie_port_bus_type && device->driver) { + service_driver =to_service_driver (device->driver);
Please move the initial assignment to the declaration line to clean this up a bit:
struct device **dev = (struct device **)data;
Same thing happens a couple more times in this patch.
drivers/pci/pcie/pcie-dpc.c struct dpc_dev { · line 101
Quoted 1 line
drivers/pci/pcie/pcie-dpc.c +1
-0+EXPORT_SYMBOL (pci_find_dpc_service);
EXPORT_SYMBOL_GPL for PCI layer internals again - and ditto for probably about everything in this series.
drivers/pci/pcie/pcie-dpc.c struct dpc_dev {
Quoted 8 lines
drivers/pci/pcie/pcie-dpc.c +4
-2#if IS_ENABLED (CONFIG_PCIEAER ) -/* Use the aer driver of the component firstly */ - driver =pci_find_aer_service (udev); +if ((severity ==AER_FATAL ) || + (severity ==AER_NONFATAL ) || + (severity ==AER_CORRECTABLE )) + driver =pci_find_aer_service (udev);#endif
No need for the inner braces here.
drivers/pci/pcie/pcie-dpc.c
Quoted 6 lines
drivers/pci/pcie/pcie-dpc.c +2
-1-if (severity ==AER_FATAL ) +if ((severity ==AER_FATAL ) || + (severity ==DPC_FATAL )) state = pci_channel_io_frozen;else state = pci_channel_io_normal;
Same here and a few more times below.
Re: [PATCH v10 4/7] PCI/DPC: Unify and plumb error handling into DPC
Quoted 51 lines · Christoph Hellwig
Quoted 36 lines · Oza Pawandeep
Current DPC driver does not do recovery, e.g. calling end-point's driver's callbacks, which sanitize the sw.
DPC driver implements
link_resetcallback, and callspci_do_recovery.Signed-off-by Oza Pawandeep <[email protected]>drivers/pci/pci.h +2
-0@@ -343,6 +343,8 @@ 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 */ +#define DPC_FATAL 4 +void pci_do_recovery (struct pci_dev *dev,int severity);#ifdef
CONFIG_PCIEASPMdrivers/pci/pcie/pcie-dpc.c +1
-0@@ -13,6 +13,7 @@#include <linux/pcieport_if.h> #include "../pci.h" #include "aer/aerdrv.h" +#include "portdrv.h" struct dpc_dev {struct pcie_device *dev;@@ -45,6 +46,60 @@ struct dpc_dev { "Memory Request Completion Timeout", /* Bit Position 18 */ };drivers/pci/pcie/pcie-dpc.c struct dpc_dev { · line 57
Quoted 10 lines
drivers/pci/pcie/pcie-dpc.c +9
-0+static int find_dpc_dev_iter (struct device *device,void *data) +{ +struct pcie_port_service_driver *service_driver; +struct device **dev; + + dev = (struct device **) data; + +if (device->bus == &pcie_port_bus_type && device->driver) { + service_driver =to_service_driver (device->driver);Please move the initial assignment to the declaration line to clean this up a bit:
Quoted 8 lines
struct device **dev = (struct device **)data;Same thing happens a couple more times in this patch.
drivers/pci/pcie/pcie-dpc.c struct dpc_dev { · line 101
Quoted 1 line
drivers/pci/pcie/pcie-dpc.c +1
-0+EXPORT_SYMBOL (pci_find_dpc_service);
EXPORT_SYMBOL_GPLfor PCI layer internals again - and ditto for probably about everything in this series.
will take care of all these comments.
Quoted 12 lines
drivers/pci/pcie/pcie-dpc.c struct dpc_dev {
Quoted 9 lines
drivers/pci/pcie/pcie-dpc.c +4
-2#if IS_ENABLED (CONFIG_PCIEAER ) -/* Use the aer driver of the component firstly */ - driver =pci_find_aer_service (udev); +if ((severity ==AER_FATAL ) || + (severity ==AER_NONFATAL ) || + (severity ==AER_CORRECTABLE )) + driver =pci_find_aer_service (udev);#endif No need for the inner braces here.
Quoted 8 lines
drivers/pci/pcie/pcie-dpc.c
Quoted 6 lines
drivers/pci/pcie/pcie-dpc.c +2
-1-if (severity ==AER_FATAL ) +if ((severity ==AER_FATAL ) || + (severity ==DPC_FATAL )) state = pci_channel_io_frozen;else state = pci_channel_io_normal;Same here and a few more times below.
will take care
Thanks,
Oza.
PATCH v10 5/7 PCI/AER: Unify aer error defines at single space
In reply to Oza PawandeepThis patch moves AER error defines to drivers/pci/pci.h.
So that it unifies the error repoting codes at single place along with dpc
drivers/acpi/apei/ghes.c +1 -0
@@ -56,6 +56,7 @@#include <ras/ras_event.h> #include "apei-internal.h" +#include "../../pci/pci.h" #define GHES_PFX "GHES: "
drivers/pci/pci.h +5 -1
@@ -343,7 +343,11 @@ 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 */ -#define DPC_FATAL 4 +#define AER_NONFATAL 0 +#define AER_FATAL 1 +#define AER_CORRECTABLE 2 + +#define DPC_FATAL 4void pci_do_recovery (struct pci_dev *dev,int severity);
drivers/pci/pcie/aer/aerdrv_errprint.c +1 -0
@@ -19,6 +19,7 @@#include <linux/cper.h> #include "aerdrv.h" +#include "../../pci.h" #include <ras/ras_event.h> #define AER_AGENT_RECEIVER 0
include/linux/aer.h +0 -4
@@ -11,10 +11,6 @@#include <linux/errno.h> #include <linux/types.h> -#define AER_NONFATAL 0 -#define AER_FATAL 1 -#define AER_CORRECTABLE 2 -struct pci_dev ;struct aer_header_log_regs {
include/ras/ras_event.h +1 -0
@@ -13,6 +13,7 @@#include <linux/aer.h> #include <linux/cper.h> #include <linux/mm.h> +#include "../../../drivers/pci/pci.h" /* * MCE Extended Error Log trace event
Signature · 2 lines
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc., a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH v10 5/7] PCI/AER: Unify aer error defines at single space
Quoted 68 lines · Oza Pawandeep
This patch moves AER error defines to
drivers/pci/pci.h.
So that it unifies the error repoting codes at single place along with dpcSigned-off-by Oza Pawandeep <[email protected]>drivers/acpi/apei/ghes.c +1
-0@@ -56,6 +56,7 @@#include <ras/ras_event.h> #include "apei-internal.h" +#include "../../pci/pci.h" #define GHES_PFX "GHES: "drivers/pci/pci.h +5
-1@@ -343,7 +343,11 @@ 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 */ -#define DPC_FATAL 4 +#define AER_NONFATAL 0 +#define AER_FATAL 1 +#define AER_CORRECTABLE 2 + +#define DPC_FATAL 4void pci_do_recovery (struct pci_dev *dev,int severity);drivers/pci/pcie/aer/aerdrv_errprint.c +1
-0@@ -19,6 +19,7 @@#include <linux/cper.h> #include "aerdrv.h" +#include "../../pci.h" #include <ras/ras_event.h> #define AER_AGENT_RECEIVER 0include/linux/aer.h +0
-4@@ -11,10 +11,6 @@#include <linux/errno.h> #include <linux/types.h> -#define AER_NONFATAL 0 -#define AER_FATAL 1 -#define AER_CORRECTABLE 2 -struct pci_dev ;struct aer_header_log_regs {include/ras/ras_event.h +0
-0
include/ras/ras_event.h line 19
Quoted 7 lines
include/ras/ras_event.h +1
-0@@ -13,6 +13,7 @@#include <linux/aer.h> #include <linux/cper.h> #include <linux/mm.h> +#include "../../../drivers/pci/pci.h" /* * MCE Extended Error Log trace event
Hi Bjorn,
Based on your earlier comment I have moved AER and DPC error defined in driver/pci/pci.h but I am not entirely comfortable with moving them there because
file such asdrivers/acpi/apei/ghes.cinclude/ras/ras_event.h
they are outside of the scope of driver/pci/pci.h and they have to include the files #include "../../pci/pci.h" and #include "../../../drivers/pci/pci.h" respectively.
hence my suggestion is to move error defines in include/linux/pci.h
let me know your view.
Regards,
Oza.
PATCH v10 6/7 PCI: Unify wait for link active into generic pci
In reply to Oza PawandeepClients such as pciehp, 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
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) +static bool pcie_wait_link_active (struct controller *ctrl) { -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" ); -} +struct pci_dev *pdev =ctrl_dev (ctrl); -static void pcie_wait_link_active (struct controller *ctrl) -{ -__pcie_wait_link_active (ctrl, true); +return pci_wait_for_link (pdev, true); }static bool pci_bus_check_dev (struct pci_bus *bus,int devfn)
drivers/pci/pci.c +34 -0
@@ -4176,6 +4176,40 @@ static int pci_pm_reset(struct pci_dev *dev, int probe)return 0 ; } +/** +* pci__wait_for_link - Wait for link till its active/inactive +* @pdev: Bridge device +* @active: waiting for active or inactive ? +* +* Use this to wait till link becomes active or inactive. +*/ +bool pci_wait_for_link (struct pci_dev *pdev,bool active) +{ +int timeout =1000 ; +bool ret; +u16 lnk_status; + +pcie_capability_read_word (pdev,PCI_EXP_LNKSTA , &lnk_status); + ret = !!(lnk_status &PCI_EXP_LNKSTA_DLLLA ); + +while ((ret != active) && (timeout >0 )) { +msleep (10 ); + timeout -=10 ; +pcie_capability_read_word (pdev,PCI_EXP_LNKSTA , &lnk_status); + ret = !!(lnk_status &PCI_EXP_LNKSTA_DLLLA ); + } + +if (ret == active) +return true; + +dev_printk (KERN_DEBUG , &pdev->dev, +"Data Link Layer Link Active not %s in 1000 msec\n" , + active ?"set" :"cleared" ); + +return false; +} +EXPORT_SYMBOL (pci_wait_for_link); +void pci_reset_secondary_bus (struct pci_dev *dev) {u16 ctrl;
drivers/pci/pcie/pcie-dpc.c +1 -11
@@ -122,19 +122,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" ); +pci_wait_for_link (pdev, false); }/**
include/linux/pci.h +1 -0
@@ -1195,6 +1195,7 @@ int pci_add_ext_cap_save_buffer(struct pci_dev *dev,int pci_request_selected_regions (struct pci_dev *,int ,const char *);int pci_request_selected_regions_exclusive (struct pci_dev *,int ,const char *);void pci_release_selected_regions (struct pci_dev *,int ); +bool pci_wait_for_link (struct pci_dev *pdev,bool active);/* drivers/pci/bus.c */ struct pci_bus *pci_bus_get (struct pci_bus *bus);
Signature · 2 lines
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc., a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH v10 6/7] PCI: Unify wait for link active into generic pci
Quoted 3 lines
+/**
+ *pci__wait_for_link- Wait for link till its active/inactive
typo - just wants a single underscore.
Review
Quoted 2 lines
(quoted diff) +2
-0+ pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status); + ret = !!(lnk_status & PCI_EXP_LNKSTA_DLLLA);
no need for the !! when assigning to a boolean.
Review
Quoted 2 lines
(quoted diff) +2
-0+ + while ((ret != active) && (timeout > 0)) {
No need for either pair of inner braces.
drivers/pci/pci.c static int pci_pm_reset(struct pci_dev *dev, int probe) · line 4199
Quoted 4 lines
drivers/pci/pci.c +4
-0+msleep (10 ); + timeout -=10 ; +pcie_capability_read_word (pdev,PCI_EXP_LNKSTA , &lnk_status); + ret = !!(lnk_status &PCI_EXP_LNKSTA_DLLLA );
Same as above.
drivers/pci/pci.c static int pci_pm_reset(struct pci_dev *dev, int probe) · line 4203
Quoted 4 lines
drivers/pci/pci.c +4
-0+ } + +if (ret == active) +return true;
Seems like the structure is a bit odd. Why not:
for (;;) {
pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
if ((lnk_status & PCI_EXP_LNKSTA_DLLLA) == active)
return true;
if (timeout <= 0)
break;
timeout -= 10;
}Re: [PATCH v10 6/7] PCI: Unify wait for link active into generic pci
Quoted 10 lines · Christoph Hellwig
Quoted 3 lines
+/**
+ *pci__wait_for_link- Wait for link till its active/inactivetypo - just wants a single underscore.
Review
Quoted 2 lines
(quoted diff) +2
-0+ pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status); + ret = !!(lnk_status & PCI_EXP_LNKSTA_DLLLA);no need for the !! when assigning to a boolean.
bool is 8 bit, while
#define PCI_EXP_LNKSTA_DLLLA 0x2000 /* Data Link Layer Link Active */
so I think we need it. otherwise it will treat lnk_status & PCI_EXP_LNKSTA_DLLLA as 0 even if 13th bit is set.
Quoted 20 lines
Review
Quoted 3 lines
(quoted diff) +2
-0+ + while ((ret != active) && (timeout > 0)) {No need for either pair of inner braces.
drivers/pci/pci.c static int pci_pm_reset(struct pci_dev *dev, int probe) · line 4199
Quoted 4 lines
drivers/pci/pci.c +4
-0+msleep (10 ); + timeout -=10 ; +pcie_capability_read_word (pdev,PCI_EXP_LNKSTA , &lnk_status); + ret = !!(lnk_status &PCI_EXP_LNKSTA_DLLLA );Same as above.
drivers/pci/pci.c static int pci_pm_reset(struct pci_dev *dev, int probe) · line 4203
Quoted 4 lines
drivers/pci/pci.c +4
-0+ } + +if (ret == active) +return true;Seems like the structure is a bit odd. Why not:
Quoted 8 lines
for (;;) {
pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
if ((lnk_status & PCI_EXP_LNKSTA_DLLLA) == active)
return true;
if (timeout <= 0)
break;
timeout -= 10;
}Thanks for suggestion, will do this way. thanks.
Regards,
Oza.
PATCH v10 7/7 PCI/DPC: Enumerate the devices after DPC trigger event
In reply to Oza PawandeepImplement error_resume callback in DPC so, after DPC trigger event enumerates the devices beneath.
drivers/pci/pcie/pcie-dpc.c +18 -0
@@ -128,6 +128,23 @@ static void dpc_wait_link_inactive(struct dpc_dev *dpc) }/** +* dpc_error_resume - enumerate the devices beneath +* @dev: pointer to Root Port's pci_dev data structure +* +* Invoked by Port Bus driver during nonfatal recovery. +*/ +static void dpc_error_resume (struct pci_dev *pdev) +{ +bool active = true; + +if (pci_wait_for_link (pdev, active)) { +pci_lock_rescan_remove (); +pci_rescan_bus (pdev->bus); +pci_unlock_rescan_remove (); + } +} + +/** * dpc_reset_link - reset link DPC routine * @dev: pointer to Root Port's pci_dev data structure *
@@ -365,6 +382,7 @@ static void dpc_remove(struct pcie_device *dev) .service = PCIE_PORT_SERVICE_DPC, .probe = dpc_probe, .remove = dpc_remove, + .error_resume = dpc_error_resume, .reset_link = dpc_reset_link, };
drivers/pci/pcie/pcie-err.c +22 -5
@@ -236,6 +236,7 @@ static pci_ers_result_t reset_link(struct pci_dev *dev, int severity)* @state: error state * @error_mesg: message to print * @cb: callback to be broadcasted +* @severity: error severity * * Invoked during error recovery process. Once being invoked, the content * of error severity will be broadcasted to all downstream drivers in a
@@ -244,7 +245,8 @@ static pci_ers_result_t reset_link(struct pci_dev *dev, int severity)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 *)) +int (*cb)(struct pci_dev *,void *), +int severity) {struct aer_broadcast_data result_data;
@@ -256,6 +258,17 @@ static pci_ers_result_t broadcast_error_message(struct pci_dev *dev, result_data.result =PCI_ERS_RESULT_RECOVERED ;if (dev->hdr_type ==PCI_HEADER_TYPE_BRIDGE ) { +/* If DPC is triggered, call resume error handler +* because, at this point we can safely assume that +* link recovery has happened, this is only handled if +* callback is resume, as this function can be called +* with multiple callbacks. +*/ +if ((severity ==DPC_FATAL ) && + (cb == report_resume)) { +cb (dev,NULL ); +return PCI_ERS_RESULT_RECOVERED ; + }/* * If the error is reported by a bridge, we think this error * is related to the downstream link of the bridge, so we
@@ -305,7 +318,8 @@ void pci_do_recovery(struct pci_dev *dev, int severity) status =broadcast_error_message (dev, state,"error_detected" , - report_error_detected); + report_error_detected, + severity);if ((severity ==AER_FATAL ) || (severity ==DPC_FATAL )) {
@@ -321,7 +335,8 @@ void pci_do_recovery(struct pci_dev *dev, int severity) status =broadcast_error_message (dev, state,"mmio_enabled" , - report_mmio_enabled); + report_mmio_enabled, + severity);if (status ==PCI_ERS_RESULT_NEED_RESET ) {/*
@@ -332,7 +347,8 @@ void pci_do_recovery(struct pci_dev *dev, int severity) status =broadcast_error_message (dev, state,"slot_reset" , - report_slot_reset); + report_slot_reset, + severity); }if (status !=PCI_ERS_RESULT_RECOVERED )
@@ -342,7 +358,8 @@ void pci_do_recovery(struct pci_dev *dev, int severity)broadcast_error_message (dev, state,"resume" , - report_resume); + report_resume, + severity);dev_info (&dev->dev,"Device recovery successful\n" );mutex_unlock (&pci_err_recovery_lock);
Signature · 2 lines
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc., a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.