* [PATCH v15 0/5] Add Error Disconnect Recover (EDR) support
@ 2020-02-13 18:20 sathyanarayanan.kuppuswamy
2020-02-13 18:20 ` [PATCH v15 1/5] PCI/ERR: Update error status after reset_link() sathyanarayanan.kuppuswamy
` (4 more replies)
0 siblings, 5 replies; 18+ messages in thread
From: sathyanarayanan.kuppuswamy @ 2020-02-13 18:20 UTC (permalink / raw)
To: bhelgaas; +Cc: linux-pci, linux-kernel, ashok.raj, sathyanarayanan.kuppuswamy
From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
This patchset adds support for following features:
1. Error Disconnect Recover (EDR) support.
2. _OSC based negotiation support for DPC.
You can find EDR spec in the following link.
https://members.pcisig.com/wg/PCI-SIG/document/12614
Changes since v14:
* Rebased on top of v5.6-rc1
Changes since v13:
* Moved all EDR related code to edr.c
* Addressed Bjorns comments.
Changes since v12:
* Addressed Bjorns comments.
* Added check for CONFIG_PCIE_EDR before requesting DPC control from firmware.
* Removed ff_check parameter from AER APIs.
* Used macros for _OST return status values in DPC driver.
Changes since v11:
* Allowed error recovery to proceed after successful reset_link().
* Used correct ACPI handle for sending EDR status.
* Rebased on top of v5.5-rc5
Changes since v10:
* Added "edr_enabled" member to dpc priv structure, which is used to cache EDR
enabling status based on status of pcie_ports_dpc_native and FF mode.
* Changed type of _DSM argument from Integer to Package in acpi_enable_dpc_port()
function to fix ACPI related boot warnings.
* Rebased on top of v5.5-rc3
Changes since v9:
* Removed caching of pcie_aer_get_firmware_first() in dpc driver.
* Added proper spec reference in git log for patch 5 & 7.
* Added new function parameter "ff_check" to pci_cleanup_aer_uncorrect_error_status(),
pci_aer_clear_fatal_status() and pci_cleanup_aer_error_status_regs() functions.
* Rebased on top of v5.4-rc5
Changes since v8:
* Rebased on top of v5.4-rc1
Changes since v7:
* Updated DSM version number to match the spec.
Changes since v6:
* Modified the order of patches to enable EDR only after all necessary support is added in kernel.
* Addressed Bjorn comments.
Changes since v5:
* Addressed Keith's comments.
* Added additional check for FF mode in pci_aer_init().
* Updated commit history of "PCI/DPC: Add support for DPC recovery on NON_FATAL errors" patch.
Changes since v4:
* Rebased on top of v5.3-rc1
* Fixed lock/unlock issue in edr_handle_event().
* Merged "Update error status after reset_link()" patch into this patchset.
Changes since v3:
* Moved EDR related ACPI functions/definitions to pci-acpi.c
* Modified commit history in few patches to include spec reference.
* Added support to handle DPC triggered by NON_FATAL errors.
* Added edr_lock to protect PCI device receiving duplicate EDR notifications.
* Addressed Bjorn comments.
Changes since v2:
* Split EDR support patch into multiple patches.
* Addressed Bjorn comments.
Changes since v1:
* Rebased on top of v5.1-rc1
Kuppuswamy Sathyanarayanan (5):
PCI/ERR: Update error status after reset_link()
PCI/DPC: Remove pcie_device reference from dpc_dev structure
PCI/EDR: Export AER, DPC and error recovery functions
PCI/DPC: Add Error Disconnect Recover (EDR) support
PCI/ACPI: Enable EDR support
drivers/acpi/pci_root.c | 16 +++
drivers/pci/pci-acpi.c | 3 +
drivers/pci/pci.h | 8 ++
drivers/pci/pcie/Kconfig | 10 ++
drivers/pci/pcie/Makefile | 1 +
drivers/pci/pcie/aer.c | 39 ++++--
drivers/pci/pcie/dpc.c | 92 ++++++++------
drivers/pci/pcie/dpc.h | 20 +++
drivers/pci/pcie/edr.c | 259 ++++++++++++++++++++++++++++++++++++++
drivers/pci/pcie/err.c | 36 ++++--
drivers/pci/probe.c | 1 +
include/linux/acpi.h | 6 +-
include/linux/pci-acpi.h | 8 ++
include/linux/pci.h | 1 +
14 files changed, 442 insertions(+), 58 deletions(-)
create mode 100644 drivers/pci/pcie/dpc.h
create mode 100644 drivers/pci/pcie/edr.c
--
2.21.0
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v15 1/5] PCI/ERR: Update error status after reset_link()
2020-02-13 18:20 [PATCH v15 0/5] Add Error Disconnect Recover (EDR) support sathyanarayanan.kuppuswamy
@ 2020-02-13 18:20 ` sathyanarayanan.kuppuswamy
2020-02-26 13:41 ` Bjorn Helgaas
2020-02-13 18:20 ` [PATCH v15 2/5] PCI/DPC: Remove pcie_device reference from dpc_dev structure sathyanarayanan.kuppuswamy
` (3 subsequent siblings)
4 siblings, 1 reply; 18+ messages in thread
From: sathyanarayanan.kuppuswamy @ 2020-02-13 18:20 UTC (permalink / raw)
To: bhelgaas
Cc: linux-pci, linux-kernel, ashok.raj, sathyanarayanan.kuppuswamy,
Keith Busch
From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Commit bdb5ac85777d ("PCI/ERR: Handle fatal error recovery") uses
reset_link() to recover from fatal errors. But during fatal error
recovery, if the initial value of error status is
PCI_ERS_RESULT_DISCONNECT or PCI_ERS_RESULT_NO_AER_DRIVER then
even after successful recovery (using reset_link()) pcie_do_recovery()
will report the recovery result as failure. So update the status of
error after reset_link().
Fixes: bdb5ac85777d ("PCI/ERR: Handle fatal error recovery")
Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Keith Busch <keith.busch@intel.com>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Acked-by: Keith Busch <keith.busch@intel.com>
---
drivers/pci/pcie/err.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index 01dfc8bb7ca0..eefefe03857a 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -208,9 +208,11 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
else
pci_walk_bus(bus, report_normal_detected, &status);
- if (state == pci_channel_io_frozen &&
- reset_link(dev, service) != PCI_ERS_RESULT_RECOVERED)
- goto failed;
+ if (state == pci_channel_io_frozen) {
+ status = reset_link(dev, service);
+ if (status != PCI_ERS_RESULT_RECOVERED)
+ goto failed;
+ }
if (status == PCI_ERS_RESULT_CAN_RECOVER) {
status = PCI_ERS_RESULT_RECOVERED;
--
2.21.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v15 2/5] PCI/DPC: Remove pcie_device reference from dpc_dev structure
2020-02-13 18:20 [PATCH v15 0/5] Add Error Disconnect Recover (EDR) support sathyanarayanan.kuppuswamy
2020-02-13 18:20 ` [PATCH v15 1/5] PCI/ERR: Update error status after reset_link() sathyanarayanan.kuppuswamy
@ 2020-02-13 18:20 ` sathyanarayanan.kuppuswamy
2020-02-26 13:43 ` Bjorn Helgaas
2020-02-13 18:20 ` [PATCH v15 3/5] PCI/EDR: Export AER, DPC and error recovery functions sathyanarayanan.kuppuswamy
` (2 subsequent siblings)
4 siblings, 1 reply; 18+ messages in thread
From: sathyanarayanan.kuppuswamy @ 2020-02-13 18:20 UTC (permalink / raw)
To: bhelgaas; +Cc: linux-pci, linux-kernel, ashok.raj, sathyanarayanan.kuppuswamy
From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Currently the only use of pcie_device member in dpc_dev structure is to
get the associated pci_dev reference. Since none of the users of
dpc_dev need reference to pcie_device, just remove it and replace it
with associated pci_dev pointer reference.
Removing pcie_device reference will help if we have need to call DPC
driver functions outside PCIe port drivers.
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
drivers/pci/pcie/dpc.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index e06f42f58d3d..99fca8400956 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -18,7 +18,7 @@
#include "../pci.h"
struct dpc_dev {
- struct pcie_device *dev;
+ struct pci_dev *pdev;
u16 cap_pos;
bool rp_extensions;
u8 rp_log_size;
@@ -101,7 +101,7 @@ void pci_restore_dpc_state(struct pci_dev *dev)
static int dpc_wait_rp_inactive(struct dpc_dev *dpc)
{
unsigned long timeout = jiffies + HZ;
- struct pci_dev *pdev = dpc->dev->port;
+ struct pci_dev *pdev = dpc->pdev;
u16 cap = dpc->cap_pos, status;
pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
@@ -149,7 +149,7 @@ static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
{
- struct pci_dev *pdev = dpc->dev->port;
+ struct pci_dev *pdev = dpc->pdev;
u16 cap = dpc->cap_pos, dpc_status, first_error;
u32 status, mask, sev, syserr, exc, dw0, dw1, dw2, dw3, log, prefix;
int i;
@@ -228,7 +228,7 @@ static irqreturn_t dpc_handler(int irq, void *context)
{
struct aer_err_info info;
struct dpc_dev *dpc = context;
- struct pci_dev *pdev = dpc->dev->port;
+ struct pci_dev *pdev = dpc->pdev;
u16 cap = dpc->cap_pos, status, source, reason, ext_reason;
pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
@@ -267,7 +267,7 @@ static irqreturn_t dpc_handler(int irq, void *context)
static irqreturn_t dpc_irq(int irq, void *context)
{
struct dpc_dev *dpc = (struct dpc_dev *)context;
- struct pci_dev *pdev = dpc->dev->port;
+ struct pci_dev *pdev = dpc->pdev;
u16 cap = dpc->cap_pos, status;
pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
@@ -299,7 +299,7 @@ static int dpc_probe(struct pcie_device *dev)
return -ENOMEM;
dpc->cap_pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DPC);
- dpc->dev = dev;
+ dpc->pdev = pdev;
set_service_data(dev, dpc);
status = devm_request_threaded_irq(device, dev->irq, dpc_irq,
--
2.21.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v15 3/5] PCI/EDR: Export AER, DPC and error recovery functions
2020-02-13 18:20 [PATCH v15 0/5] Add Error Disconnect Recover (EDR) support sathyanarayanan.kuppuswamy
2020-02-13 18:20 ` [PATCH v15 1/5] PCI/ERR: Update error status after reset_link() sathyanarayanan.kuppuswamy
2020-02-13 18:20 ` [PATCH v15 2/5] PCI/DPC: Remove pcie_device reference from dpc_dev structure sathyanarayanan.kuppuswamy
@ 2020-02-13 18:20 ` sathyanarayanan.kuppuswamy
2020-02-26 1:02 ` Bjorn Helgaas
2020-02-13 18:20 ` [PATCH v15 4/5] PCI/DPC: Add Error Disconnect Recover (EDR) support sathyanarayanan.kuppuswamy
2020-02-13 18:20 ` [PATCH v15 5/5] PCI/ACPI: Enable EDR support sathyanarayanan.kuppuswamy
4 siblings, 1 reply; 18+ messages in thread
From: sathyanarayanan.kuppuswamy @ 2020-02-13 18:20 UTC (permalink / raw)
To: bhelgaas; +Cc: linux-pci, linux-kernel, ashok.raj, sathyanarayanan.kuppuswamy
From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
This is a preparatory patch for adding EDR support.
As per the Downstream Port Containment Related Enhancements ECN to the
PCI Firmware Specification r3.2, sec 4.5.1, table 4-6, If DPC is
controlled by firmware, firmware is responsible for initializing
Downstream Port Containment Extended Capability Structures per firmware
policy. Further, the OS is permitted to read or write DPC Control and
Status registers of a port while processing an Error Disconnect Recover
notification from firmware on that port. Error Disconnect Recover
notification processing begins with the Error Disconnect Recover notify
from Firmware, and ends when the OS releases DPC by clearing the DPC
Trigger Status bit.Firmware can read DPC Trigger Status bit to determine
the ownership of DPC Control and Status registers. Firmware is not
permitted to write to DPC Control and Status registers if DPC Trigger
Status is set i.e. the link is in DPC state. Outside of the Error
Disconnect Recover notification processing window, the OS is not
permitted to modify DPC Control or Status registers; only firmware
is allowed to.
To add EDR support we need to re-use some of the existing DPC,
AER and pCIE error recovery functions. So add necessary interfaces.
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
drivers/pci/pci.h | 8 ++++
drivers/pci/pcie/aer.c | 39 ++++++++++++++------
drivers/pci/pcie/dpc.c | 84 +++++++++++++++++++++++++-----------------
drivers/pci/pcie/dpc.h | 20 ++++++++++
drivers/pci/pcie/err.c | 30 ++++++++++++---
5 files changed, 131 insertions(+), 50 deletions(-)
create mode 100644 drivers/pci/pcie/dpc.h
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 6394e7746fb5..136f27cf3871 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -443,6 +443,9 @@ struct aer_err_info {
int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info);
void aer_print_error(struct pci_dev *dev, struct aer_err_info *info);
+int pci_aer_clear_err_uncor_status(struct pci_dev *dev);
+void pci_aer_clear_err_fatal_status(struct pci_dev *dev);
+int pci_aer_clear_err_status_regs(struct pci_dev *dev);
#endif /* CONFIG_PCIEAER */
#ifdef CONFIG_PCIE_DPC
@@ -549,6 +552,11 @@ static inline int pci_dev_specific_disable_acs_redir(struct pci_dev *dev)
/* PCI error reporting and recovery */
void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
u32 service);
+pci_ers_result_t pcie_do_recovery_common(struct pci_dev *dev,
+ enum pci_channel_state state,
+ u32 service,
+ pci_ers_result_t (*reset_cb)(void *cb_data),
+ void *cb_data);
bool pcie_wait_for_link(struct pci_dev *pdev, bool active);
#ifdef CONFIG_PCIEASPM
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 4a818b07a1af..399836aa07f4 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -376,7 +376,7 @@ void pci_aer_clear_device_status(struct pci_dev *dev)
pcie_capability_write_word(dev, PCI_EXP_DEVSTA, sta);
}
-int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
+int pci_aer_clear_err_uncor_status(struct pci_dev *dev)
{
int pos;
u32 status, sev;
@@ -385,9 +385,6 @@ int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
if (!pos)
return -EIO;
- if (pcie_aer_get_firmware_first(dev))
- return -EIO;
-
/* Clear status bits for ERR_NONFATAL errors only */
pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, &status);
pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, &sev);
@@ -397,9 +394,17 @@ int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
return 0;
}
+
+int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
+{
+ if (pcie_aer_get_firmware_first(dev))
+ return -EIO;
+
+ return pci_aer_clear_err_uncor_status(dev);
+}
EXPORT_SYMBOL_GPL(pci_cleanup_aer_uncorrect_error_status);
-void pci_aer_clear_fatal_status(struct pci_dev *dev)
+void pci_aer_clear_err_fatal_status(struct pci_dev *dev)
{
int pos;
u32 status, sev;
@@ -408,9 +413,6 @@ void pci_aer_clear_fatal_status(struct pci_dev *dev)
if (!pos)
return;
- if (pcie_aer_get_firmware_first(dev))
- return;
-
/* Clear status bits for ERR_FATAL errors only */
pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, &status);
pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, &sev);
@@ -419,7 +421,15 @@ void pci_aer_clear_fatal_status(struct pci_dev *dev)
pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, status);
}
-int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
+void pci_aer_clear_fatal_status(struct pci_dev *dev)
+{
+ if (pcie_aer_get_firmware_first(dev))
+ return;
+
+ return pci_aer_clear_err_fatal_status(dev);
+}
+
+int pci_aer_clear_err_status_regs(struct pci_dev *dev)
{
int pos;
u32 status;
@@ -432,9 +442,6 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
if (!pos)
return -EIO;
- if (pcie_aer_get_firmware_first(dev))
- return -EIO;
-
port_type = pci_pcie_type(dev);
if (port_type == PCI_EXP_TYPE_ROOT_PORT) {
pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, &status);
@@ -450,6 +457,14 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
return 0;
}
+int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
+{
+ if (pcie_aer_get_firmware_first(dev))
+ return -EIO;
+
+ return pci_aer_clear_err_status_regs(dev);
+}
+
void pci_save_aer_state(struct pci_dev *dev)
{
struct pci_cap_saved_state *save_state;
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 99fca8400956..acae12dbf9ff 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -15,15 +15,9 @@
#include <linux/pci.h>
#include "portdrv.h"
+#include "dpc.h"
#include "../pci.h"
-struct dpc_dev {
- struct pci_dev *pdev;
- u16 cap_pos;
- bool rp_extensions;
- u8 rp_log_size;
-};
-
static const char * const rp_pio_error_string[] = {
"Configuration Request received UR Completion", /* Bit Position 0 */
"Configuration Request received CA Completion", /* Bit Position 1 */
@@ -117,36 +111,44 @@ static int dpc_wait_rp_inactive(struct dpc_dev *dpc)
return 0;
}
-static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
+pci_ers_result_t dpc_reset_link_common(struct dpc_dev *dpc)
{
- struct dpc_dev *dpc;
u16 cap;
- /*
- * DPC disables the Link automatically in hardware, so it has
- * already been reset by the time we get here.
- */
- dpc = to_dpc_dev(pdev);
cap = dpc->cap_pos;
/*
* Wait until the Link is inactive, then clear DPC Trigger Status
* to allow the Port to leave DPC.
*/
- pcie_wait_for_link(pdev, false);
+ pcie_wait_for_link(dpc->pdev, false);
if (dpc->rp_extensions && dpc_wait_rp_inactive(dpc))
return PCI_ERS_RESULT_DISCONNECT;
- pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
+ pci_write_config_word(dpc->pdev, cap + PCI_EXP_DPC_STATUS,
PCI_EXP_DPC_STATUS_TRIGGER);
- if (!pcie_wait_for_link(pdev, true))
+ if (!pcie_wait_for_link(dpc->pdev, true))
return PCI_ERS_RESULT_DISCONNECT;
return PCI_ERS_RESULT_RECOVERED;
}
+static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
+{
+ struct dpc_dev *dpc;
+
+ /*
+ * DPC disables the Link automatically in hardware, so it has
+ * already been reset by the time we get here.
+ */
+ dpc = to_dpc_dev(pdev);
+
+ return dpc_reset_link_common(dpc);
+
+}
+
static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
{
struct pci_dev *pdev = dpc->pdev;
@@ -224,10 +226,9 @@ static int dpc_get_aer_uncorrect_severity(struct pci_dev *dev,
return 1;
}
-static irqreturn_t dpc_handler(int irq, void *context)
+void dpc_process_error(struct dpc_dev *dpc)
{
struct aer_err_info info;
- struct dpc_dev *dpc = context;
struct pci_dev *pdev = dpc->pdev;
u16 cap = dpc->cap_pos, status, source, reason, ext_reason;
@@ -257,6 +258,14 @@ static irqreturn_t dpc_handler(int irq, void *context)
pci_cleanup_aer_uncorrect_error_status(pdev);
pci_aer_clear_fatal_status(pdev);
}
+}
+
+static irqreturn_t dpc_handler(int irq, void *context)
+{
+ struct dpc_dev *dpc = context;
+ struct pci_dev *pdev = dpc->pdev;
+
+ dpc_process_error(dpc);
/* We configure DPC so it only triggers on ERR_FATAL */
pcie_do_recovery(pdev, pci_channel_io_frozen, PCIE_PORT_SERVICE_DPC);
@@ -282,6 +291,25 @@ static irqreturn_t dpc_irq(int irq, void *context)
return IRQ_HANDLED;
}
+void dpc_dev_init(struct pci_dev *pdev, struct dpc_dev *dpc)
+{
+ dpc->cap_pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DPC);
+ dpc->pdev = pdev;
+ pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CAP, &dpc->cap);
+ pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, &dpc->ctl);
+
+ dpc->rp_extensions = (dpc->cap & PCI_EXP_DPC_CAP_RP_EXT);
+ if (dpc->rp_extensions) {
+ dpc->rp_log_size =
+ (dpc->cap & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8;
+ if (dpc->rp_log_size < 4 || dpc->rp_log_size > 9) {
+ pci_err(pdev, "RP PIO log size %u is invalid\n",
+ dpc->rp_log_size);
+ dpc->rp_log_size = 0;
+ }
+ }
+}
+
#define FLAG(x, y) (((x) & (y)) ? '+' : '-')
static int dpc_probe(struct pcie_device *dev)
{
@@ -298,8 +326,8 @@ static int dpc_probe(struct pcie_device *dev)
if (!dpc)
return -ENOMEM;
- dpc->cap_pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DPC);
- dpc->pdev = pdev;
+ dpc_dev_init(pdev, dpc);
+
set_service_data(dev, dpc);
status = devm_request_threaded_irq(device, dev->irq, dpc_irq,
@@ -311,18 +339,8 @@ static int dpc_probe(struct pcie_device *dev)
return status;
}
- pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CAP, &cap);
- pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, &ctl);
-
- dpc->rp_extensions = (cap & PCI_EXP_DPC_CAP_RP_EXT);
- if (dpc->rp_extensions) {
- dpc->rp_log_size = (cap & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8;
- if (dpc->rp_log_size < 4 || dpc->rp_log_size > 9) {
- pci_err(pdev, "RP PIO log size %u is invalid\n",
- dpc->rp_log_size);
- dpc->rp_log_size = 0;
- }
- }
+ ctl = dpc->ctl;
+ cap = dpc->cap;
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);
diff --git a/drivers/pci/pcie/dpc.h b/drivers/pci/pcie/dpc.h
new file mode 100644
index 000000000000..2d82bc917fcb
--- /dev/null
+++ b/drivers/pci/pcie/dpc.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef DRIVERS_PCIE_DPC_H
+#define DRIVERS_PCIE_DPC_H
+
+#include <linux/pci.h>
+
+struct dpc_dev {
+ struct pci_dev *pdev;
+ u16 cap_pos;
+ bool rp_extensions;
+ u8 rp_log_size;
+ u16 ctl;
+ u16 cap;
+};
+
+pci_ers_result_t dpc_reset_link_common(struct dpc_dev *dpc);
+void dpc_process_error(struct dpc_dev *dpc);
+void dpc_dev_init(struct pci_dev *pdev, struct dpc_dev *dpc);
+
+#endif /* DRIVERS_PCIE_DPC_H */
diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index eefefe03857a..e7b9dfae9035 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -162,11 +162,18 @@ static pci_ers_result_t default_reset_link(struct pci_dev *dev)
return rc ? PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_RECOVERED;
}
-static pci_ers_result_t reset_link(struct pci_dev *dev, u32 service)
+static pci_ers_result_t reset_link(struct pci_dev *dev, u32 service,
+ pci_ers_result_t (*reset_cb)(void *cb_data),
+ void *cb_data)
{
pci_ers_result_t status;
struct pcie_port_service_driver *driver = NULL;
+ if (reset_cb) {
+ status = reset_cb(cb_data);
+ goto done;
+ }
+
driver = pcie_port_find_service(dev, service);
if (driver && driver->reset_link) {
status = driver->reset_link(dev);
@@ -178,6 +185,7 @@ static pci_ers_result_t reset_link(struct pci_dev *dev, u32 service)
return PCI_ERS_RESULT_DISCONNECT;
}
+done:
if (status != PCI_ERS_RESULT_RECOVERED) {
pci_printk(KERN_DEBUG, dev, "link reset at upstream device %s failed\n",
pci_name(dev));
@@ -187,8 +195,11 @@ static pci_ers_result_t reset_link(struct pci_dev *dev, u32 service)
return status;
}
-void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
- u32 service)
+pci_ers_result_t pcie_do_recovery_common(struct pci_dev *dev,
+ enum pci_channel_state state,
+ u32 service,
+ pci_ers_result_t (*reset_cb)(void *cb_data),
+ void *cb_data)
{
pci_ers_result_t status = PCI_ERS_RESULT_CAN_RECOVER;
struct pci_bus *bus;
@@ -209,7 +220,7 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
pci_walk_bus(bus, report_normal_detected, &status);
if (state == pci_channel_io_frozen) {
- status = reset_link(dev, service);
+ status = reset_link(dev, service, reset_cb, cb_data);
if (status != PCI_ERS_RESULT_RECOVERED)
goto failed;
}
@@ -240,11 +251,20 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
pci_aer_clear_device_status(dev);
pci_cleanup_aer_uncorrect_error_status(dev);
pci_info(dev, "device recovery successful\n");
- return;
+
+ return status;
failed:
pci_uevent_ers(dev, PCI_ERS_RESULT_DISCONNECT);
/* TODO: Should kernel panic here? */
pci_info(dev, "device recovery failed\n");
+
+ return status;
+}
+
+void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
+ u32 service)
+{
+ pcie_do_recovery_common(dev, state, service, NULL, NULL);
}
--
2.21.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v15 4/5] PCI/DPC: Add Error Disconnect Recover (EDR) support
2020-02-13 18:20 [PATCH v15 0/5] Add Error Disconnect Recover (EDR) support sathyanarayanan.kuppuswamy
` (2 preceding siblings ...)
2020-02-13 18:20 ` [PATCH v15 3/5] PCI/EDR: Export AER, DPC and error recovery functions sathyanarayanan.kuppuswamy
@ 2020-02-13 18:20 ` sathyanarayanan.kuppuswamy
2020-02-26 1:02 ` Bjorn Helgaas
2020-02-13 18:20 ` [PATCH v15 5/5] PCI/ACPI: Enable EDR support sathyanarayanan.kuppuswamy
4 siblings, 1 reply; 18+ messages in thread
From: sathyanarayanan.kuppuswamy @ 2020-02-13 18:20 UTC (permalink / raw)
To: bhelgaas; +Cc: linux-pci, linux-kernel, ashok.raj, sathyanarayanan.kuppuswamy
From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
As per ACPI specification r6.3, sec 5.6.6, when firmware owns Downstream
Port Containment (DPC), its expected to use the "Error Disconnect
Recover" (EDR) notification to alert OSPM of a DPC event and if OS
supports EDR, its expected to handle the software state invalidation and
port recovery in OS, and also let firmware know the recovery status via
_OST ACPI call. Related _OST status codes can be found in ACPI
specification r6.3, sec 6.3.5.2.
Also, as per PCI firmware specification r3.2 Downstream Port Containment
Related Enhancements ECN, sec 4.5.1, table 4-6, If DPC is controlled by
firmware (firmware first mode), firmware is responsible for
configuring the DPC and OS is responsible for error recovery. Also, OS
is allowed to modify DPC registers only during the EDR notification
window.
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
drivers/pci/pci-acpi.c | 3 +
drivers/pci/pcie/Kconfig | 10 ++
drivers/pci/pcie/Makefile | 1 +
drivers/pci/pcie/edr.c | 257 ++++++++++++++++++++++++++++++++++++++
include/linux/pci-acpi.h | 8 ++
5 files changed, 279 insertions(+)
create mode 100644 drivers/pci/pcie/edr.c
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 0c02d500158f..6af5d6a04990 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1258,6 +1258,7 @@ static void pci_acpi_setup(struct device *dev)
acpi_pci_wakeup(pci_dev, false);
acpi_device_power_add_dependent(adev, dev);
+ pci_acpi_add_edr_notifier(pci_dev);
}
static void pci_acpi_cleanup(struct device *dev)
@@ -1276,6 +1277,8 @@ static void pci_acpi_cleanup(struct device *dev)
device_set_wakeup_capable(dev, false);
}
+
+ pci_acpi_remove_edr_notifier(pci_dev);
}
static bool pci_acpi_bus_match(struct device *dev)
diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
index 6e3c04b46fb1..772b1f4cb19e 100644
--- a/drivers/pci/pcie/Kconfig
+++ b/drivers/pci/pcie/Kconfig
@@ -140,3 +140,13 @@ config PCIE_BW
This enables PCI Express Bandwidth Change Notification. If
you know link width or rate changes occur only to correct
unreliable links, you may answer Y.
+
+config PCIE_EDR
+ bool "PCI Express Error Disconnect Recover support"
+ depends on PCIE_DPC && ACPI
+ help
+ This option adds Error Disconnect Recover support as specified
+ in the Downstream Port Containment Related Enhancements ECN to
+ the PCI Firmware Specification r3.2. Enable this if you want to
+ support hybrid DPC model which uses both firmware and OS to
+ implement DPC.
diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
index efb9d2e71e9e..68da9280ff11 100644
--- a/drivers/pci/pcie/Makefile
+++ b/drivers/pci/pcie/Makefile
@@ -13,3 +13,4 @@ obj-$(CONFIG_PCIE_PME) += pme.o
obj-$(CONFIG_PCIE_DPC) += dpc.o
obj-$(CONFIG_PCIE_PTM) += ptm.o
obj-$(CONFIG_PCIE_BW) += bw_notification.o
+obj-$(CONFIG_PCIE_EDR) += edr.o
diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c
new file mode 100644
index 000000000000..b3e9103585a1
--- /dev/null
+++ b/drivers/pci/pcie/edr.c
@@ -0,0 +1,257 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PCI DPC Error Disconnect Recover support driver
+ * Author: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
+ *
+ * Copyright (C) 2020 Intel Corp.
+ */
+
+#define dev_fmt(fmt) "EDR: " fmt
+
+#include <linux/pci.h>
+#include <linux/pci-acpi.h>
+
+#include "portdrv.h"
+#include "dpc.h"
+#include "../pci.h"
+
+#define EDR_PORT_ENABLE_DSM 0x0C
+#define EDR_PORT_LOCATE_DSM 0x0D
+#define EDR_OST_SUCCESS 0x80
+#define EDR_OST_FAILED 0x81
+
+/*
+ * _DSM wrapper function to enable/disable DPC port.
+ * @pdev : PCI device structure.
+ * @enable: status of DPC port (0 or 1).
+ *
+ * returns 0 on success or errno on failure.
+ */
+static int acpi_enable_dpc_port(struct pci_dev *pdev, bool enable)
+{
+ struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+ union acpi_object *obj, argv4, req;
+ int status = 0;
+
+ req.type = ACPI_TYPE_INTEGER;
+ req.integer.value = enable;
+
+ argv4.type = ACPI_TYPE_PACKAGE;
+ argv4.package.count = 1;
+ argv4.package.elements = &req;
+
+ /*
+ * Per the Downstream Port Containment Related Enhancements ECN to
+ * the PCI Firmware Specification r3.2, sec 4.6.12,
+ * EDR_PORT_ENABLE_DSM is optional. Return success if it's not
+ * implemented.
+ */
+ obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
+ EDR_PORT_ENABLE_DSM, &argv4);
+ if (!obj)
+ return 0;
+
+ if (obj->type != ACPI_TYPE_INTEGER || obj->integer.value != enable)
+ status = -EIO;
+
+ ACPI_FREE(obj);
+
+ return status;
+}
+
+/*
+ * _DSM wrapper function to locate DPC port.
+ * @pdev : Device which received EDR event.
+ *
+ * returns pci_dev or NULL.
+ */
+static struct pci_dev *acpi_locate_dpc_port(struct pci_dev *pdev)
+{
+ struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+ union acpi_object *obj;
+ u16 port;
+
+ obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
+ EDR_PORT_LOCATE_DSM, NULL);
+ if (!obj)
+ return pci_dev_get(pdev);
+
+ if (obj->type != ACPI_TYPE_INTEGER) {
+ ACPI_FREE(obj);
+ return NULL;
+ }
+
+ /*
+ * Firmware returns DPC port BDF details in following format:
+ * 15:8 = bus
+ * 7:3 = device
+ * 2:0 = function
+ */
+ port = obj->integer.value;
+
+ ACPI_FREE(obj);
+
+ return pci_get_domain_bus_and_slot(pci_domain_nr(pdev->bus),
+ PCI_BUS_NUM(port), port & 0xff);
+}
+
+/*
+ * _OST wrapper function to let firmware know the status of EDR event.
+ * @pdev : Device used to send _OST.
+ * @edev : Device which experienced EDR event.
+ * @status: Status of EDR event.
+ */
+static int acpi_send_edr_status(struct pci_dev *pdev, struct pci_dev *edev,
+ u16 status)
+{
+ struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+ u32 ost_status;
+
+ pci_dbg(pdev, "Sending EDR status :%#x\n", status);
+
+ ost_status = PCI_DEVID(edev->bus->number, edev->devfn);
+ ost_status = (ost_status << 16) | status;
+
+ status = acpi_evaluate_ost(adev->handle,
+ ACPI_NOTIFY_DISCONNECT_RECOVER,
+ ost_status, NULL);
+ if (ACPI_FAILURE(status))
+ return -EINVAL;
+
+ return 0;
+}
+
+pci_ers_result_t edr_dpc_reset_link(void *cb_data)
+{
+ return dpc_reset_link_common(cb_data);
+}
+
+static void edr_handle_event(acpi_handle handle, u32 event, void *data)
+{
+ struct dpc_dev *dpc = data, ndpc;
+ struct pci_dev *pdev = dpc->pdev;
+ pci_ers_result_t estate = PCI_ERS_RESULT_DISCONNECT;
+ u16 status;
+
+ pci_info(pdev, "ACPI event %#x received\n", event);
+
+ if (event != ACPI_NOTIFY_DISCONNECT_RECOVER)
+ return;
+
+ /*
+ * Check if _DSM(0xD) is available, and if present locate the
+ * port which issued EDR event.
+ */
+ pdev = acpi_locate_dpc_port(pdev);
+ if (!pdev) {
+ pci_err(dpc->pdev, "No valid port found\n");
+ return;
+ }
+
+ if (pdev != dpc->pdev) {
+ pci_warn(pdev, "Initializing dpc again\n");
+ dpc_dev_init(pdev, &ndpc);
+ dpc= &ndpc;
+ }
+
+ /*
+ * If port does not support DPC, just send the OST:
+ */
+ if (!dpc->cap_pos)
+ goto send_ost;
+
+ /* Check if there is a valid DPC trigger */
+ pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_STATUS, &status);
+ if (!(status & PCI_EXP_DPC_STATUS_TRIGGER)) {
+ pci_err(pdev, "Invalid DPC trigger %#010x\n", status);
+ goto send_ost;
+ }
+
+ dpc_process_error(dpc);
+
+ /* Clear AER registers */
+ pci_aer_clear_err_uncor_status(pdev);
+ pci_aer_clear_err_fatal_status(pdev);
+ pci_aer_clear_err_status_regs(pdev);
+
+ /*
+ * Irrespective of whether the DPC event is triggered by
+ * ERR_FATAL or ERR_NONFATAL, since the link is already down,
+ * use the FATAL error recovery path for both cases.
+ */
+ estate = pcie_do_recovery_common(pdev, pci_channel_io_frozen, -1,
+ edr_dpc_reset_link, dpc);
+send_ost:
+
+ /* Use ACPI handle of DPC event device for sending EDR status */
+ dpc = data;
+
+ /*
+ * If recovery is successful, send _OST(0xF, BDF << 16 | 0x80)
+ * to firmware. If not successful, send _OST(0xF, BDF << 16 | 0x81).
+ */
+ if (estate == PCI_ERS_RESULT_RECOVERED)
+ acpi_send_edr_status(dpc->pdev, pdev, EDR_OST_SUCCESS);
+ else
+ acpi_send_edr_status(dpc->pdev, pdev, EDR_OST_FAILED);
+
+ pci_dev_put(pdev);
+}
+
+int pci_acpi_add_edr_notifier(struct pci_dev *pdev)
+{
+ struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+ struct dpc_dev *dpc;
+ acpi_status astatus;
+ int status;
+
+ /*
+ * Per the Downstream Port Containment Related Enhancements ECN to
+ * the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-6, EDR support
+ * can only be enabled if DPC is controlled by firmware.
+ *
+ * TODO: Remove dependency on ACPI FIRMWARE_FIRST bit to
+ * determine ownership of DPC between firmware or OS.
+ */
+ if (!pcie_aer_get_firmware_first(pdev) || pcie_ports_dpc_native)
+ return -ENODEV;
+
+ if (!adev)
+ return 0;
+
+ dpc = devm_kzalloc(&pdev->dev, sizeof(*dpc), GFP_KERNEL);
+ if (!dpc)
+ return -ENOMEM;
+
+ dpc_dev_init(pdev, dpc);
+
+ astatus = acpi_install_notify_handler(adev->handle,
+ ACPI_SYSTEM_NOTIFY,
+ edr_handle_event, dpc);
+ if (ACPI_FAILURE(astatus)) {
+ pci_err(pdev, "Install ACPI_SYSTEM_NOTIFY handler failed\n");
+ return -EBUSY;
+ }
+
+ status = acpi_enable_dpc_port(pdev, true);
+ if (status) {
+ pci_warn(pdev, "Enable DPC port failed\n");
+ acpi_remove_notify_handler(adev->handle,
+ ACPI_SYSTEM_NOTIFY,
+ edr_handle_event);
+ return status;
+ }
+
+ return 0;
+}
+
+void pci_acpi_remove_edr_notifier(struct pci_dev *pdev)
+{
+ struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+
+ if (!adev)
+ return;
+
+ acpi_remove_notify_handler(adev->handle, ACPI_SYSTEM_NOTIFY,
+ edr_handle_event);
+}
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 62b7fdcc661c..a430e5fc50f3 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -112,6 +112,14 @@ extern const guid_t pci_acpi_dsm_guid;
#define RESET_DELAY_DSM 0x08
#define FUNCTION_DELAY_DSM 0x09
+#ifdef CONFIG_PCIE_EDR
+int pci_acpi_add_edr_notifier(struct pci_dev *pdev);
+void pci_acpi_remove_edr_notifier(struct pci_dev *pdev);
+#else
+static inline int pci_acpi_add_edr_notifier(struct pci_dev *pdev) { return 0; }
+static inline void pci_acpi_remove_edr_notifier(struct pci_dev *pdev) { }
+#endif /* CONFIG_PCIE_EDR */
+
#else /* CONFIG_ACPI */
static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
--
2.21.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v15 5/5] PCI/ACPI: Enable EDR support
2020-02-13 18:20 [PATCH v15 0/5] Add Error Disconnect Recover (EDR) support sathyanarayanan.kuppuswamy
` (3 preceding siblings ...)
2020-02-13 18:20 ` [PATCH v15 4/5] PCI/DPC: Add Error Disconnect Recover (EDR) support sathyanarayanan.kuppuswamy
@ 2020-02-13 18:20 ` sathyanarayanan.kuppuswamy
4 siblings, 0 replies; 18+ messages in thread
From: sathyanarayanan.kuppuswamy @ 2020-02-13 18:20 UTC (permalink / raw)
To: bhelgaas
Cc: linux-pci, linux-kernel, ashok.raj, sathyanarayanan.kuppuswamy,
Rafael J. Wysocki, Len Brown, Keith Busch, Huong Nguyen,
Austin Bolen
From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
As per PCI firmware specification r3.2 Downstream Port Containment
Related Enhancements ECN, sec 4.5.1, OS must implement following steps
to enable/use EDR feature.
1. OS can use bit 7 of _OSC Control Field to negotiate control over
Downstream Port Containment (DPC) configuration of PCIe port. After _OSC
negotiation, firmware will Set this bit to grant OS control over PCIe
DPC configuration and Clear it if this feature was requested and denied,
or was not requested.
2. Also, if OS supports EDR, it should expose its support to BIOS by
setting bit 7 of _OSC Support Field. And if OS sets bit 7 of _OSC
Control Field it must also expose support for EDR by setting bit 7 of
_OSC Support Field.
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Acked-by: Keith Busch <keith.busch@intel.com>
Tested-by: Huong Nguyen <huong.nguyen@dell.com>
Tested-by: Austin Bolen <Austin.Bolen@dell.com>
---
drivers/acpi/pci_root.c | 16 ++++++++++++++++
drivers/pci/pcie/edr.c | 4 +++-
drivers/pci/probe.c | 1 +
include/linux/acpi.h | 6 ++++--
include/linux/pci.h | 1 +
5 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index d1e666ef3fcc..ad1be5941a00 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -131,6 +131,7 @@ static struct pci_osc_bit_struct pci_osc_support_bit[] = {
{ OSC_PCI_CLOCK_PM_SUPPORT, "ClockPM" },
{ OSC_PCI_SEGMENT_GROUPS_SUPPORT, "Segments" },
{ OSC_PCI_MSI_SUPPORT, "MSI" },
+ { OSC_PCI_EDR_SUPPORT, "EDR" },
{ OSC_PCI_HPX_TYPE_3_SUPPORT, "HPX-Type3" },
};
@@ -141,6 +142,7 @@ static struct pci_osc_bit_struct pci_osc_control_bit[] = {
{ OSC_PCI_EXPRESS_AER_CONTROL, "AER" },
{ OSC_PCI_EXPRESS_CAPABILITY_CONTROL, "PCIeCapability" },
{ OSC_PCI_EXPRESS_LTR_CONTROL, "LTR" },
+ { OSC_PCI_EXPRESS_DPC_CONTROL, "DPC" },
};
static void decode_osc_bits(struct acpi_pci_root *root, char *msg, u32 word,
@@ -440,6 +442,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
support |= OSC_PCI_ASPM_SUPPORT | OSC_PCI_CLOCK_PM_SUPPORT;
if (pci_msi_enabled())
support |= OSC_PCI_MSI_SUPPORT;
+ if (IS_ENABLED(CONFIG_PCIE_EDR))
+ support |= OSC_PCI_EDR_SUPPORT;
decode_osc_support(root, "OS supports", support);
status = acpi_pci_osc_support(root, support);
@@ -487,6 +491,16 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
control |= OSC_PCI_EXPRESS_AER_CONTROL;
}
+ /*
+ * Per the Downstream Port Containment Related Enhancements ECN to
+ * the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-5,
+ * OSC_PCI_EXPRESS_DPC_CONTROL indicates the OS supports both DPC
+ * and EDR. So use CONFIG_PCIE_EDR for requesting DPC control which
+ * will only be turned on if both EDR and DPC is enabled.
+ */
+ if (IS_ENABLED(CONFIG_PCIE_EDR))
+ control |= OSC_PCI_EXPRESS_DPC_CONTROL;
+
requested = control;
status = acpi_pci_osc_control_set(handle, &control,
OSC_PCI_EXPRESS_CAPABILITY_CONTROL);
@@ -916,6 +930,8 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
host_bridge->native_pme = 0;
if (!(root->osc_control_set & OSC_PCI_EXPRESS_LTR_CONTROL))
host_bridge->native_ltr = 0;
+ if (!(root->osc_control_set & OSC_PCI_EXPRESS_DPC_CONTROL))
+ host_bridge->native_dpc = 0;
/*
* Evaluate the "PCI Boot Configuration" _DSM Function. If it
diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c
index b3e9103585a1..e7dfe401db5c 100644
--- a/drivers/pci/pcie/edr.c
+++ b/drivers/pci/pcie/edr.c
@@ -201,6 +201,7 @@ static void edr_handle_event(acpi_handle handle, u32 event, void *data)
int pci_acpi_add_edr_notifier(struct pci_dev *pdev)
{
struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+ struct pci_host_bridge *host = pci_find_host_bridge(pdev->bus);
struct dpc_dev *dpc;
acpi_status astatus;
int status;
@@ -213,7 +214,8 @@ int pci_acpi_add_edr_notifier(struct pci_dev *pdev)
* TODO: Remove dependency on ACPI FIRMWARE_FIRST bit to
* determine ownership of DPC between firmware or OS.
*/
- if (!pcie_aer_get_firmware_first(pdev) || pcie_ports_dpc_native)
+ if (!pcie_aer_get_firmware_first(pdev) || pcie_ports_dpc_native ||
+ (host->native_dpc))
return -ENODEV;
if (!adev)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 512cb4312ddd..c9a9c5b42e72 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -598,6 +598,7 @@ static void pci_init_host_bridge(struct pci_host_bridge *bridge)
bridge->native_shpc_hotplug = 1;
bridge->native_pme = 1;
bridge->native_ltr = 1;
+ bridge->native_dpc = 1;
}
struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 0f24d701fbdc..b7d3caf6f205 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -530,8 +530,9 @@ extern bool osc_pc_lpi_support_confirmed;
#define OSC_PCI_CLOCK_PM_SUPPORT 0x00000004
#define OSC_PCI_SEGMENT_GROUPS_SUPPORT 0x00000008
#define OSC_PCI_MSI_SUPPORT 0x00000010
+#define OSC_PCI_EDR_SUPPORT 0x00000080
#define OSC_PCI_HPX_TYPE_3_SUPPORT 0x00000100
-#define OSC_PCI_SUPPORT_MASKS 0x0000011f
+#define OSC_PCI_SUPPORT_MASKS 0x0000019f
/* PCI Host Bridge _OSC: Capabilities DWORD 3: Control Field */
#define OSC_PCI_EXPRESS_NATIVE_HP_CONTROL 0x00000001
@@ -540,7 +541,8 @@ extern bool osc_pc_lpi_support_confirmed;
#define OSC_PCI_EXPRESS_AER_CONTROL 0x00000008
#define OSC_PCI_EXPRESS_CAPABILITY_CONTROL 0x00000010
#define OSC_PCI_EXPRESS_LTR_CONTROL 0x00000020
-#define OSC_PCI_CONTROL_MASKS 0x0000003f
+#define OSC_PCI_EXPRESS_DPC_CONTROL 0x00000080
+#define OSC_PCI_CONTROL_MASKS 0x000000bf
#define ACPI_GSB_ACCESS_ATTRIB_QUICK 0x00000002
#define ACPI_GSB_ACCESS_ATTRIB_SEND_RCV 0x00000004
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 3840a541a9de..9a0d602627d8 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -510,6 +510,7 @@ struct pci_host_bridge {
unsigned int native_shpc_hotplug:1; /* OS may use SHPC hotplug */
unsigned int native_pme:1; /* OS may use PCIe PME */
unsigned int native_ltr:1; /* OS may use PCIe LTR */
+ unsigned int native_dpc:1; /* OS may use PCIe DPC */
unsigned int preserve_config:1; /* Preserve FW resource setup */
/* Resource alignment requirements */
--
2.21.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v15 4/5] PCI/DPC: Add Error Disconnect Recover (EDR) support
2020-02-13 18:20 ` [PATCH v15 4/5] PCI/DPC: Add Error Disconnect Recover (EDR) support sathyanarayanan.kuppuswamy
@ 2020-02-26 1:02 ` Bjorn Helgaas
2020-02-26 18:42 ` Kuppuswamy Sathyanarayanan
0 siblings, 1 reply; 18+ messages in thread
From: Bjorn Helgaas @ 2020-02-26 1:02 UTC (permalink / raw)
To: sathyanarayanan.kuppuswamy; +Cc: linux-pci, linux-kernel, ashok.raj
On Thu, Feb 13, 2020 at 10:20:16AM -0800, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>
> As per ACPI specification r6.3, sec 5.6.6, when firmware owns Downstream
> Port Containment (DPC), its expected to use the "Error Disconnect
> Recover" (EDR) notification to alert OSPM of a DPC event and if OS
> supports EDR, its expected to handle the software state invalidation and
> port recovery in OS, and also let firmware know the recovery status via
> _OST ACPI call. Related _OST status codes can be found in ACPI
> specification r6.3, sec 6.3.5.2.
>
> Also, as per PCI firmware specification r3.2 Downstream Port Containment
> Related Enhancements ECN, sec 4.5.1, table 4-6, If DPC is controlled by
> firmware (firmware first mode), firmware is responsible for
> configuring the DPC and OS is responsible for error recovery. Also, OS
> is allowed to modify DPC registers only during the EDR notification
> window.
>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
> drivers/pci/pci-acpi.c | 3 +
> drivers/pci/pcie/Kconfig | 10 ++
> drivers/pci/pcie/Makefile | 1 +
> drivers/pci/pcie/edr.c | 257 ++++++++++++++++++++++++++++++++++++++
> include/linux/pci-acpi.h | 8 ++
> 5 files changed, 279 insertions(+)
> create mode 100644 drivers/pci/pcie/edr.c
>
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 0c02d500158f..6af5d6a04990 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -1258,6 +1258,7 @@ static void pci_acpi_setup(struct device *dev)
>
> acpi_pci_wakeup(pci_dev, false);
> acpi_device_power_add_dependent(adev, dev);
> + pci_acpi_add_edr_notifier(pci_dev);
> }
>
> static void pci_acpi_cleanup(struct device *dev)
> @@ -1276,6 +1277,8 @@ static void pci_acpi_cleanup(struct device *dev)
>
> device_set_wakeup_capable(dev, false);
> }
> +
> + pci_acpi_remove_edr_notifier(pci_dev);
> }
>
> static bool pci_acpi_bus_match(struct device *dev)
> diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
> index 6e3c04b46fb1..772b1f4cb19e 100644
> --- a/drivers/pci/pcie/Kconfig
> +++ b/drivers/pci/pcie/Kconfig
> @@ -140,3 +140,13 @@ config PCIE_BW
> This enables PCI Express Bandwidth Change Notification. If
> you know link width or rate changes occur only to correct
> unreliable links, you may answer Y.
> +
> +config PCIE_EDR
> + bool "PCI Express Error Disconnect Recover support"
> + depends on PCIE_DPC && ACPI
> + help
> + This option adds Error Disconnect Recover support as specified
> + in the Downstream Port Containment Related Enhancements ECN to
> + the PCI Firmware Specification r3.2. Enable this if you want to
> + support hybrid DPC model which uses both firmware and OS to
> + implement DPC.
> diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
> index efb9d2e71e9e..68da9280ff11 100644
> --- a/drivers/pci/pcie/Makefile
> +++ b/drivers/pci/pcie/Makefile
> @@ -13,3 +13,4 @@ obj-$(CONFIG_PCIE_PME) += pme.o
> obj-$(CONFIG_PCIE_DPC) += dpc.o
> obj-$(CONFIG_PCIE_PTM) += ptm.o
> obj-$(CONFIG_PCIE_BW) += bw_notification.o
> +obj-$(CONFIG_PCIE_EDR) += edr.o
> diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c
> new file mode 100644
> index 000000000000..b3e9103585a1
> --- /dev/null
> +++ b/drivers/pci/pcie/edr.c
> @@ -0,0 +1,257 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PCI DPC Error Disconnect Recover support driver
> + * Author: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> + *
> + * Copyright (C) 2020 Intel Corp.
> + */
> +
> +#define dev_fmt(fmt) "EDR: " fmt
> +
> +#include <linux/pci.h>
> +#include <linux/pci-acpi.h>
> +
> +#include "portdrv.h"
> +#include "dpc.h"
> +#include "../pci.h"
> +
> +#define EDR_PORT_ENABLE_DSM 0x0C
> +#define EDR_PORT_LOCATE_DSM 0x0D
> +#define EDR_OST_SUCCESS 0x80
> +#define EDR_OST_FAILED 0x81
> +
> +/*
> + * _DSM wrapper function to enable/disable DPC port.
> + * @pdev : PCI device structure.
> + * @enable: status of DPC port (0 or 1).
Either line up these colons or join them with the parameter names
(also below).
> + * returns 0 on success or errno on failure.
> + */
> +static int acpi_enable_dpc_port(struct pci_dev *pdev, bool enable)
We only call this with "true", so the "enable" parameter is pointless.
> +{
> + struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
> + union acpi_object *obj, argv4, req;
> + int status = 0;
> +
> + req.type = ACPI_TYPE_INTEGER;
> + req.integer.value = enable;
> +
> + argv4.type = ACPI_TYPE_PACKAGE;
> + argv4.package.count = 1;
> + argv4.package.elements = &req;
> +
> + /*
> + * Per the Downstream Port Containment Related Enhancements ECN to
> + * the PCI Firmware Specification r3.2, sec 4.6.12,
> + * EDR_PORT_ENABLE_DSM is optional. Return success if it's not
> + * implemented.
> + */
> + obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
> + EDR_PORT_ENABLE_DSM, &argv4);
> + if (!obj)
> + return 0;
> +
> + if (obj->type != ACPI_TYPE_INTEGER || obj->integer.value != enable)
> + status = -EIO;
> +
> + ACPI_FREE(obj);
> +
> + return status;
> +}
> +
> +/*
> + * _DSM wrapper function to locate DPC port.
> + * @pdev : Device which received EDR event.
> + *
> + * returns pci_dev or NULL.
> + */
> +static struct pci_dev *acpi_locate_dpc_port(struct pci_dev *pdev)
> +{
> + struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
> + union acpi_object *obj;
> + u16 port;
> +
> + obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
> + EDR_PORT_LOCATE_DSM, NULL);
> + if (!obj)
> + return pci_dev_get(pdev);
> +
> + if (obj->type != ACPI_TYPE_INTEGER) {
If this happens, it's a firmware defect, isn't it? I think maybe we
should warn here. I know we warn below ("No valid port found"), but
that doesn't give any clue about the fact that firmware screwed up.
> + ACPI_FREE(obj);
> + return NULL;
> + }
> +
> + /*
> + * Firmware returns DPC port BDF details in following format:
> + * 15:8 = bus
> + * 7:3 = device
> + * 2:0 = function
> + */
> + port = obj->integer.value;
> +
> + ACPI_FREE(obj);
> +
> + return pci_get_domain_bus_and_slot(pci_domain_nr(pdev->bus),
> + PCI_BUS_NUM(port), port & 0xff);
> +}
> +
> +/*
> + * _OST wrapper function to let firmware know the status of EDR event.
> + * @pdev : Device used to send _OST.
> + * @edev : Device which experienced EDR event.
> + * @status: Status of EDR event.
> + */
> +static int acpi_send_edr_status(struct pci_dev *pdev, struct pci_dev *edev,
> + u16 status)
> +{
> + struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
> + u32 ost_status;
> +
> + pci_dbg(pdev, "Sending EDR status :%#x\n", status);
> +
> + ost_status = PCI_DEVID(edev->bus->number, edev->devfn);
> + ost_status = (ost_status << 16) | status;
> +
> + status = acpi_evaluate_ost(adev->handle,
> + ACPI_NOTIFY_DISCONNECT_RECOVER,
> + ost_status, NULL);
> + if (ACPI_FAILURE(status))
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +pci_ers_result_t edr_dpc_reset_link(void *cb_data)
> +{
> + return dpc_reset_link_common(cb_data);
> +}
> +
> +static void edr_handle_event(acpi_handle handle, u32 event, void *data)
> +{
> + struct dpc_dev *dpc = data, ndpc;
There's actually very little use of struct dpc_dev in this file. I
bet with a little elbow grease, we could remove it completely and just
use the pci_dev * or maybe an opaque pointer.
> + struct pci_dev *pdev = dpc->pdev;
> + pci_ers_result_t estate = PCI_ERS_RESULT_DISCONNECT;
> + u16 status;
> +
> + pci_info(pdev, "ACPI event %#x received\n", event);
> +
> + if (event != ACPI_NOTIFY_DISCONNECT_RECOVER)
> + return;
> +
> + /*
> + * Check if _DSM(0xD) is available, and if present locate the
> + * port which issued EDR event.
> + */
> + pdev = acpi_locate_dpc_port(pdev);
This function name should include "get" since it's part of the
pci_dev_get()/pci_dev_put() sequence.
> + if (!pdev) {
> + pci_err(dpc->pdev, "No valid port found\n");
> + return;
> + }
> +
> + if (pdev != dpc->pdev) {
> + pci_warn(pdev, "Initializing dpc again\n");
> + dpc_dev_init(pdev, &ndpc);
This seems... I'm not sure what. I guess it's really just reading
the DPC capability for use by dpc_process_error(), so maybe it's OK.
But it's a little strange to read.
Is this something we should be warning about? I think the ECR says
it's legitimate to return a child device, doesn't it?
> + dpc= &ndpc;
Space before "=".
> + }
> +
> + /*
> + * If port does not support DPC, just send the OST:
> + */
> + if (!dpc->cap_pos)
> + goto send_ost;
> +
> + /* Check if there is a valid DPC trigger */
> + pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_STATUS, &status);
> + if (!(status & PCI_EXP_DPC_STATUS_TRIGGER)) {
> + pci_err(pdev, "Invalid DPC trigger %#010x\n", status);
> + goto send_ost;
> + }
> +
> + dpc_process_error(dpc);
> +
> + /* Clear AER registers */
> + pci_aer_clear_err_uncor_status(pdev);
> + pci_aer_clear_err_fatal_status(pdev);
> + pci_aer_clear_err_status_regs(pdev);
> +
> + /*
> + * Irrespective of whether the DPC event is triggered by
> + * ERR_FATAL or ERR_NONFATAL, since the link is already down,
> + * use the FATAL error recovery path for both cases.
> + */
> + estate = pcie_do_recovery_common(pdev, pci_channel_io_frozen, -1,
> + edr_dpc_reset_link, dpc);
> +send_ost:
> +
> + /* Use ACPI handle of DPC event device for sending EDR status */
> + dpc = data;
I think it'd be clearer to reserve "dpc" for the device that received
the notification and to which we send the status, and use a different
"e_dpc" or something for the dpc_process_error() and related code.
> + /*
> + * If recovery is successful, send _OST(0xF, BDF << 16 | 0x80)
> + * to firmware. If not successful, send _OST(0xF, BDF << 16 | 0x81).
> + */
> + if (estate == PCI_ERS_RESULT_RECOVERED)
> + acpi_send_edr_status(dpc->pdev, pdev, EDR_OST_SUCCESS);
> + else
> + acpi_send_edr_status(dpc->pdev, pdev, EDR_OST_FAILED);
> +
> + pci_dev_put(pdev);
> +}
> +
> +int pci_acpi_add_edr_notifier(struct pci_dev *pdev)
> +{
> + struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
> + struct dpc_dev *dpc;
> + acpi_status astatus;
> + int status;
> +
> + /*
> + * Per the Downstream Port Containment Related Enhancements ECN to
> + * the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-6, EDR support
> + * can only be enabled if DPC is controlled by firmware.
> + *
> + * TODO: Remove dependency on ACPI FIRMWARE_FIRST bit to
> + * determine ownership of DPC between firmware or OS.
Extend the comment to say how we *should* determine ownership.
> + */
> + if (!pcie_aer_get_firmware_first(pdev) || pcie_ports_dpc_native)
> + return -ENODEV;
> +
> + if (!adev)
> + return 0;
> +
> + dpc = devm_kzalloc(&pdev->dev, sizeof(*dpc), GFP_KERNEL);
This kzalloc should be in dpc.c, not here.
And I don't see a corresponding free.
> + if (!dpc)
> + return -ENOMEM;
> +
> + dpc_dev_init(pdev, dpc);
> +
> + astatus = acpi_install_notify_handler(adev->handle,
> + ACPI_SYSTEM_NOTIFY,
> + edr_handle_event, dpc);
> + if (ACPI_FAILURE(astatus)) {
> + pci_err(pdev, "Install ACPI_SYSTEM_NOTIFY handler failed\n");
> + return -EBUSY;
> + }
> +
> + status = acpi_enable_dpc_port(pdev, true);
> + if (status) {
> + pci_warn(pdev, "Enable DPC port failed\n");
> + acpi_remove_notify_handler(adev->handle,
> + ACPI_SYSTEM_NOTIFY,
> + edr_handle_event);
> + return status;
> + }
> +
> + return 0;
> +}
> +
> +void pci_acpi_remove_edr_notifier(struct pci_dev *pdev)
> +{
> + struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
> +
> + if (!adev)
> + return;
> +
> + acpi_remove_notify_handler(adev->handle, ACPI_SYSTEM_NOTIFY,
> + edr_handle_event);
> +}
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index 62b7fdcc661c..a430e5fc50f3 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -112,6 +112,14 @@ extern const guid_t pci_acpi_dsm_guid;
> #define RESET_DELAY_DSM 0x08
> #define FUNCTION_DELAY_DSM 0x09
>
> +#ifdef CONFIG_PCIE_EDR
> +int pci_acpi_add_edr_notifier(struct pci_dev *pdev);
> +void pci_acpi_remove_edr_notifier(struct pci_dev *pdev);
> +#else
> +static inline int pci_acpi_add_edr_notifier(struct pci_dev *pdev) { return 0; }
> +static inline void pci_acpi_remove_edr_notifier(struct pci_dev *pdev) { }
> +#endif /* CONFIG_PCIE_EDR */
> +
> #else /* CONFIG_ACPI */
> static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
> static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
> --
> 2.21.0
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v15 3/5] PCI/EDR: Export AER, DPC and error recovery functions
2020-02-13 18:20 ` [PATCH v15 3/5] PCI/EDR: Export AER, DPC and error recovery functions sathyanarayanan.kuppuswamy
@ 2020-02-26 1:02 ` Bjorn Helgaas
2020-02-26 19:30 ` Kuppuswamy Sathyanarayanan
0 siblings, 1 reply; 18+ messages in thread
From: Bjorn Helgaas @ 2020-02-26 1:02 UTC (permalink / raw)
To: sathyanarayanan.kuppuswamy; +Cc: linux-pci, linux-kernel, ashok.raj
On Thu, Feb 13, 2020 at 10:20:15AM -0800, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>
> This is a preparatory patch for adding EDR support.
>
> As per the Downstream Port Containment Related Enhancements ECN to the
> PCI Firmware Specification r3.2, sec 4.5.1, table 4-6, If DPC is
> controlled by firmware, firmware is responsible for initializing
> Downstream Port Containment Extended Capability Structures per firmware
> policy. Further, the OS is permitted to read or write DPC Control and
> Status registers of a port while processing an Error Disconnect Recover
> notification from firmware on that port. Error Disconnect Recover
> notification processing begins with the Error Disconnect Recover notify
> from Firmware, and ends when the OS releases DPC by clearing the DPC
> Trigger Status bit.Firmware can read DPC Trigger Status bit to determine
> the ownership of DPC Control and Status registers. Firmware is not
> permitted to write to DPC Control and Status registers if DPC Trigger
> Status is set i.e. the link is in DPC state. Outside of the Error
> Disconnect Recover notification processing window, the OS is not
> permitted to modify DPC Control or Status registers; only firmware
> is allowed to.
>
> To add EDR support we need to re-use some of the existing DPC,
> AER and pCIE error recovery functions. So add necessary interfaces.
>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
> drivers/pci/pci.h | 8 ++++
> drivers/pci/pcie/aer.c | 39 ++++++++++++++------
> drivers/pci/pcie/dpc.c | 84 +++++++++++++++++++++++++-----------------
> drivers/pci/pcie/dpc.h | 20 ++++++++++
> drivers/pci/pcie/err.c | 30 ++++++++++++---
> 5 files changed, 131 insertions(+), 50 deletions(-)
> create mode 100644 drivers/pci/pcie/dpc.h
>
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 6394e7746fb5..136f27cf3871 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -443,6 +443,9 @@ struct aer_err_info {
>
> int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info);
> void aer_print_error(struct pci_dev *dev, struct aer_err_info *info);
> +int pci_aer_clear_err_uncor_status(struct pci_dev *dev);
> +void pci_aer_clear_err_fatal_status(struct pci_dev *dev);
> +int pci_aer_clear_err_status_regs(struct pci_dev *dev);
> #endif /* CONFIG_PCIEAER */
>
> #ifdef CONFIG_PCIE_DPC
> @@ -549,6 +552,11 @@ static inline int pci_dev_specific_disable_acs_redir(struct pci_dev *dev)
> /* PCI error reporting and recovery */
> void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
> u32 service);
> +pci_ers_result_t pcie_do_recovery_common(struct pci_dev *dev,
> + enum pci_channel_state state,
> + u32 service,
> + pci_ers_result_t (*reset_cb)(void *cb_data),
> + void *cb_data);
>
> bool pcie_wait_for_link(struct pci_dev *pdev, bool active);
> #ifdef CONFIG_PCIEASPM
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index 4a818b07a1af..399836aa07f4 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -376,7 +376,7 @@ void pci_aer_clear_device_status(struct pci_dev *dev)
> pcie_capability_write_word(dev, PCI_EXP_DEVSTA, sta);
> }
>
> -int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
> +int pci_aer_clear_err_uncor_status(struct pci_dev *dev)
> {
> int pos;
> u32 status, sev;
> @@ -385,9 +385,6 @@ int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
> if (!pos)
> return -EIO;
>
> - if (pcie_aer_get_firmware_first(dev))
> - return -EIO;
> -
> /* Clear status bits for ERR_NONFATAL errors only */
> pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, &status);
> pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, &sev);
> @@ -397,9 +394,17 @@ int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
>
> return 0;
> }
> +
> +int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
> +{
> + if (pcie_aer_get_firmware_first(dev))
> + return -EIO;
> +
> + return pci_aer_clear_err_uncor_status(dev);
> +}
> EXPORT_SYMBOL_GPL(pci_cleanup_aer_uncorrect_error_status);
>
> -void pci_aer_clear_fatal_status(struct pci_dev *dev)
> +void pci_aer_clear_err_fatal_status(struct pci_dev *dev)
> {
> int pos;
> u32 status, sev;
> @@ -408,9 +413,6 @@ void pci_aer_clear_fatal_status(struct pci_dev *dev)
> if (!pos)
> return;
>
> - if (pcie_aer_get_firmware_first(dev))
> - return;
> -
> /* Clear status bits for ERR_FATAL errors only */
> pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, &status);
> pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, &sev);
> @@ -419,7 +421,15 @@ void pci_aer_clear_fatal_status(struct pci_dev *dev)
> pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, status);
> }
>
> -int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
> +void pci_aer_clear_fatal_status(struct pci_dev *dev)
> +{
> + if (pcie_aer_get_firmware_first(dev))
> + return;
> +
> + return pci_aer_clear_err_fatal_status(dev);
> +}
> +
> +int pci_aer_clear_err_status_regs(struct pci_dev *dev)
> {
> int pos;
> u32 status;
> @@ -432,9 +442,6 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
> if (!pos)
> return -EIO;
>
> - if (pcie_aer_get_firmware_first(dev))
> - return -EIO;
> -
> port_type = pci_pcie_type(dev);
> if (port_type == PCI_EXP_TYPE_ROOT_PORT) {
> pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, &status);
> @@ -450,6 +457,14 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
> return 0;
> }
>
> +int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
> +{
> + if (pcie_aer_get_firmware_first(dev))
> + return -EIO;
> +
> + return pci_aer_clear_err_status_regs(dev);
> +}
We started with these:
pci_cleanup_aer_uncorrect_error_status()
pci_aer_clear_fatal_status()
pci_cleanup_aer_error_status_regs()
This was already a mess. They do basically similar things, but the
function names are a complete jumble. Let's start with preliminary
patches to name them consistently, e.g.,
pci_aer_clear_nonfatal_status()
pci_aer_clear_fatal_status()
pci_aer_clear_status()
Now, for EDR you eventually add this in edr_handle_event():
edr_handle_event()
{
...
pci_aer_clear_err_uncor_status(pdev);
pci_aer_clear_err_fatal_status(pdev);
pci_aer_clear_err_status_regs(pdev);
I see that this path needs to clear the status even in the
firmware-first case, so you do need some change for that. But
pci_aer_clear_err_status_regs() does exactly the same thing as
pci_aer_clear_err_uncor_status() and pci_aer_clear_err_fatal_status()
plus a little more (clearing PCI_ERR_ROOT_STATUS), so it should be
sufficient to just call pci_aer_clear_err_status_regs().
So I don't think you need to make wrappers for
pci_aer_clear_nonfatal_status() and pci_aer_clear_fatal_status() at
all since they're not needed by the EDR path.
You *do* need a wrapper for pci_aer_clear_status(), but instead of
just adding random words ("err", "regs") to the name, please name it
something like pci_aer_raw_clear_status() as a hint that it skips
some sort of check.
I would split this into a separate patch. This patch contains a bunch
of things that aren't really related except that they're needed for
EDR. I think it will be more readable if each patch just does one
piece of it.
> void pci_save_aer_state(struct pci_dev *dev)
> {
> struct pci_cap_saved_state *save_state;
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index 99fca8400956..acae12dbf9ff 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -15,15 +15,9 @@
> #include <linux/pci.h>
>
> #include "portdrv.h"
> +#include "dpc.h"
> #include "../pci.h"
>
> -struct dpc_dev {
> - struct pci_dev *pdev;
> - u16 cap_pos;
> - bool rp_extensions;
> - u8 rp_log_size;
> -};
Adding dpc.h shouldn't be in this patch because there's no need for a
separate dpc.h file yet -- it's only included this one place. I'm not
positive a dpc.h is needed at all -- see comments on patch [4/5].
> static const char * const rp_pio_error_string[] = {
> "Configuration Request received UR Completion", /* Bit Position 0 */
> "Configuration Request received CA Completion", /* Bit Position 1 */
> @@ -117,36 +111,44 @@ static int dpc_wait_rp_inactive(struct dpc_dev *dpc)
> return 0;
> }
>
> -static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
> +pci_ers_result_t dpc_reset_link_common(struct dpc_dev *dpc)
> {
I don't see why you need to split this into dpc_reset_link_common()
and dpc_reset_link(). IIUC, you split it so the DPC driver path can
supply a pci_dev * via
dpc_handler
pcie_do_recovery
pcie_do_recovery_common(..., NULL, NULL)
reset_link(..., NULL, NULL)
driver->reset_link(pdev)
dpc_reset_link(pdev)
dpc = to_dpc_dev(pdev)
dpc_reset_link_common(dpc)
while the EDR path can supply a dpc_dev * via
edr_handle_event
pcie_do_recovery_common(..., edr_dpc_reset_link, dpc)
reset_link(..., edr_dpc_reset_link, dpc)
edr_dpc_reset_link(dpc)
dpc_reset_link_common(dpc)
But it looks like you *could* make these paths look like:
dpc_handler
pcie_do_recovery
pcie_do_recovery_common(..., NULL, NULL)
reset_link(..., NULL, NULL)
driver->reset_link(pdev)
dpc_reset_link(pdev)
edr_handle_event
pcie_do_recovery_common(..., dpc_reset_link, pdev)
reset_link(..., dpc_reset_link, pdev)
dpc_reset_link(pdev)
and you wouldn't need edr_dpc_reset_link() at all and you wouldn't
have to split dpc_reset_link_common() out.
> - struct dpc_dev *dpc;
> u16 cap;
>
> - /*
> - * DPC disables the Link automatically in hardware, so it has
> - * already been reset by the time we get here.
> - */
> - dpc = to_dpc_dev(pdev);
> cap = dpc->cap_pos;
>
> /*
> * Wait until the Link is inactive, then clear DPC Trigger Status
> * to allow the Port to leave DPC.
> */
> - pcie_wait_for_link(pdev, false);
> + pcie_wait_for_link(dpc->pdev, false);
I'm hoping you don't need to split this out at all, but if you do,
adding
struct pci_dev *pdev = dpc->pdev;
at the top will avoid these needless diffs.
> if (dpc->rp_extensions && dpc_wait_rp_inactive(dpc))
> return PCI_ERS_RESULT_DISCONNECT;
>
> - pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
> + pci_write_config_word(dpc->pdev, cap + PCI_EXP_DPC_STATUS,
> PCI_EXP_DPC_STATUS_TRIGGER);
>
> - if (!pcie_wait_for_link(pdev, true))
> + if (!pcie_wait_for_link(dpc->pdev, true))
> return PCI_ERS_RESULT_DISCONNECT;
>
> return PCI_ERS_RESULT_RECOVERED;
> }
>
> +static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
> +{
> + struct dpc_dev *dpc;
> +
> + /*
> + * DPC disables the Link automatically in hardware, so it has
> + * already been reset by the time we get here.
> + */
> + dpc = to_dpc_dev(pdev);
> +
> + return dpc_reset_link_common(dpc);
> +
> +}
> +
> static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
> {
> struct pci_dev *pdev = dpc->pdev;
> @@ -224,10 +226,9 @@ static int dpc_get_aer_uncorrect_severity(struct pci_dev *dev,
> return 1;
> }
>
> -static irqreturn_t dpc_handler(int irq, void *context)
> +void dpc_process_error(struct dpc_dev *dpc)
> {
> struct aer_err_info info;
> - struct dpc_dev *dpc = context;
> struct pci_dev *pdev = dpc->pdev;
> u16 cap = dpc->cap_pos, status, source, reason, ext_reason;
>
> @@ -257,6 +258,14 @@ static irqreturn_t dpc_handler(int irq, void *context)
> pci_cleanup_aer_uncorrect_error_status(pdev);
> pci_aer_clear_fatal_status(pdev);
> }
> +}
> +
> +static irqreturn_t dpc_handler(int irq, void *context)
> +{
> + struct dpc_dev *dpc = context;
> + struct pci_dev *pdev = dpc->pdev;
> +
> + dpc_process_error(dpc);
>
> /* We configure DPC so it only triggers on ERR_FATAL */
> pcie_do_recovery(pdev, pci_channel_io_frozen, PCIE_PORT_SERVICE_DPC);
> @@ -282,6 +291,25 @@ static irqreturn_t dpc_irq(int irq, void *context)
> return IRQ_HANDLED;
> }
>
> +void dpc_dev_init(struct pci_dev *pdev, struct dpc_dev *dpc)
> +{
Can you include the kzalloc() here so we don't have to do the alloc in
pci_acpi_add_edr_notifier()?
I think there's also a leak there: pci_acpi_add_edr_notifier()
kzallocs a struct dpc_dev, but I don't see a corresponding kfree().
> + dpc->cap_pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DPC);
I think we need to check cap_pos for non-zero here. It's arguably
safe today because portdrv only calls dpc_probe() when
PCIE_PORT_SERVICE_DPC is set and we only set that if there's a DPC
capability. But that's a long ways away from here and it's
convoluted, so it's pretty hard to verify that it's safe.
When we add EDR into the picture and call this from
pci_acpi_add_edr_notifier() and edr_handle_event(), I'm pretty sure
it's not safe at all because we have no idea whether the device has a
DPC capability.
Factoring out dpc_dev_init() and the kzalloc() would be a simple
cleanup-type patch all by itself, so it could be separated out for
ease of reviewing.
> + dpc->pdev = pdev;
> + pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CAP, &dpc->cap);
> + pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, &dpc->ctl);
> +
> + dpc->rp_extensions = (dpc->cap & PCI_EXP_DPC_CAP_RP_EXT);
> + if (dpc->rp_extensions) {
> + dpc->rp_log_size =
> + (dpc->cap & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8;
> + if (dpc->rp_log_size < 4 || dpc->rp_log_size > 9) {
> + pci_err(pdev, "RP PIO log size %u is invalid\n",
> + dpc->rp_log_size);
> + dpc->rp_log_size = 0;
> + }
> + }
> +}
> +
> #define FLAG(x, y) (((x) & (y)) ? '+' : '-')
> static int dpc_probe(struct pcie_device *dev)
> {
> @@ -298,8 +326,8 @@ static int dpc_probe(struct pcie_device *dev)
> if (!dpc)
> return -ENOMEM;
>
> - dpc->cap_pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DPC);
> - dpc->pdev = pdev;
> + dpc_dev_init(pdev, dpc);
> +
> set_service_data(dev, dpc);
>
> status = devm_request_threaded_irq(device, dev->irq, dpc_irq,
> @@ -311,18 +339,8 @@ static int dpc_probe(struct pcie_device *dev)
> return status;
> }
>
> - pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CAP, &cap);
> - pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, &ctl);
> -
> - dpc->rp_extensions = (cap & PCI_EXP_DPC_CAP_RP_EXT);
> - if (dpc->rp_extensions) {
> - dpc->rp_log_size = (cap & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8;
> - if (dpc->rp_log_size < 4 || dpc->rp_log_size > 9) {
> - pci_err(pdev, "RP PIO log size %u is invalid\n",
> - dpc->rp_log_size);
> - dpc->rp_log_size = 0;
> - }
> - }
> + ctl = dpc->ctl;
> + cap = dpc->cap;
>
> 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);
> diff --git a/drivers/pci/pcie/dpc.h b/drivers/pci/pcie/dpc.h
> new file mode 100644
> index 000000000000..2d82bc917fcb
> --- /dev/null
> +++ b/drivers/pci/pcie/dpc.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef DRIVERS_PCIE_DPC_H
> +#define DRIVERS_PCIE_DPC_H
> +
> +#include <linux/pci.h>
> +
> +struct dpc_dev {
> + struct pci_dev *pdev;
> + u16 cap_pos;
> + bool rp_extensions;
> + u8 rp_log_size;
> + u16 ctl;
> + u16 cap;
> +};
> +
> +pci_ers_result_t dpc_reset_link_common(struct dpc_dev *dpc);
> +void dpc_process_error(struct dpc_dev *dpc);
> +void dpc_dev_init(struct pci_dev *pdev, struct dpc_dev *dpc);
> +
> +#endif /* DRIVERS_PCIE_DPC_H */
> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> index eefefe03857a..e7b9dfae9035 100644
> --- a/drivers/pci/pcie/err.c
> +++ b/drivers/pci/pcie/err.c
> @@ -162,11 +162,18 @@ static pci_ers_result_t default_reset_link(struct pci_dev *dev)
> return rc ? PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_RECOVERED;
> }
>
> -static pci_ers_result_t reset_link(struct pci_dev *dev, u32 service)
> +static pci_ers_result_t reset_link(struct pci_dev *dev, u32 service,
> + pci_ers_result_t (*reset_cb)(void *cb_data),
> + void *cb_data)
This rejiggering of reset_link() and pcie_do_recovery() is unrelated
to the rest (except that it's needed for EDR, of course), so maybe
could be a separate patch.
We could also consider changing the interface of pcie_do_recovery() to
just add reset_cb and cb_data, and change the existing callers to pass
NULL, NULL. Then we wouldn't need pcie_do_recovery_common(). I'm not
sure which way would be better.
> {
> pci_ers_result_t status;
> struct pcie_port_service_driver *driver = NULL;
>
> + if (reset_cb) {
> + status = reset_cb(cb_data);
> + goto done;
> + }
> +
> driver = pcie_port_find_service(dev, service);
> if (driver && driver->reset_link) {
> status = driver->reset_link(dev);
> @@ -178,6 +185,7 @@ static pci_ers_result_t reset_link(struct pci_dev *dev, u32 service)
> return PCI_ERS_RESULT_DISCONNECT;
> }
>
> +done:
> if (status != PCI_ERS_RESULT_RECOVERED) {
> pci_printk(KERN_DEBUG, dev, "link reset at upstream device %s failed\n",
> pci_name(dev));
> @@ -187,8 +195,11 @@ static pci_ers_result_t reset_link(struct pci_dev *dev, u32 service)
> return status;
> }
>
> -void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
> - u32 service)
> +pci_ers_result_t pcie_do_recovery_common(struct pci_dev *dev,
> + enum pci_channel_state state,
> + u32 service,
> + pci_ers_result_t (*reset_cb)(void *cb_data),
> + void *cb_data)
> {
> pci_ers_result_t status = PCI_ERS_RESULT_CAN_RECOVER;
> struct pci_bus *bus;
> @@ -209,7 +220,7 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
> pci_walk_bus(bus, report_normal_detected, &status);
>
> if (state == pci_channel_io_frozen) {
> - status = reset_link(dev, service);
> + status = reset_link(dev, service, reset_cb, cb_data);
> if (status != PCI_ERS_RESULT_RECOVERED)
> goto failed;
> }
> @@ -240,11 +251,20 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
> pci_aer_clear_device_status(dev);
> pci_cleanup_aer_uncorrect_error_status(dev);
> pci_info(dev, "device recovery successful\n");
> - return;
> +
> + return status;
>
> failed:
> pci_uevent_ers(dev, PCI_ERS_RESULT_DISCONNECT);
>
> /* TODO: Should kernel panic here? */
> pci_info(dev, "device recovery failed\n");
> +
> + return status;
> +}
> +
> +void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
> + u32 service)
> +{
> + pcie_do_recovery_common(dev, state, service, NULL, NULL);
> }
> --
> 2.21.0
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v15 1/5] PCI/ERR: Update error status after reset_link()
2020-02-13 18:20 ` [PATCH v15 1/5] PCI/ERR: Update error status after reset_link() sathyanarayanan.kuppuswamy
@ 2020-02-26 13:41 ` Bjorn Helgaas
0 siblings, 0 replies; 18+ messages in thread
From: Bjorn Helgaas @ 2020-02-26 13:41 UTC (permalink / raw)
To: sathyanarayanan.kuppuswamy
Cc: linux-pci, linux-kernel, ashok.raj, Keith Busch
On Thu, Feb 13, 2020 at 10:20:13AM -0800, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>
> Commit bdb5ac85777d ("PCI/ERR: Handle fatal error recovery") uses
> reset_link() to recover from fatal errors. But during fatal error
> recovery, if the initial value of error status is
> PCI_ERS_RESULT_DISCONNECT or PCI_ERS_RESULT_NO_AER_DRIVER then
> even after successful recovery (using reset_link()) pcie_do_recovery()
> will report the recovery result as failure. So update the status of
> error after reset_link().
>
> Fixes: bdb5ac85777d ("PCI/ERR: Handle fatal error recovery")
> Cc: Ashok Raj <ashok.raj@intel.com>
> Cc: Keith Busch <keith.busch@intel.com>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Acked-by: Keith Busch <keith.busch@intel.com>
I raised the possibility of a stable tag for this. If you'd like
that, please add the tag and some justification per
Documentation/process/stable-kernel-rules.rst.
A kernel.org bugzilla pointer to show the user-visible effect of this,
e.g., "lspci -vv" and a dmesg log showing an error that should be
recoverable but isn't, would be a good start. That would actually be
useful even if you don't want a stable tag.
> ---
> drivers/pci/pcie/err.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> index 01dfc8bb7ca0..eefefe03857a 100644
> --- a/drivers/pci/pcie/err.c
> +++ b/drivers/pci/pcie/err.c
> @@ -208,9 +208,11 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
> else
> pci_walk_bus(bus, report_normal_detected, &status);
>
> - if (state == pci_channel_io_frozen &&
> - reset_link(dev, service) != PCI_ERS_RESULT_RECOVERED)
> - goto failed;
> + if (state == pci_channel_io_frozen) {
> + status = reset_link(dev, service);
> + if (status != PCI_ERS_RESULT_RECOVERED)
> + goto failed;
> + }
>
> if (status == PCI_ERS_RESULT_CAN_RECOVER) {
> status = PCI_ERS_RESULT_RECOVERED;
> --
> 2.21.0
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v15 2/5] PCI/DPC: Remove pcie_device reference from dpc_dev structure
2020-02-13 18:20 ` [PATCH v15 2/5] PCI/DPC: Remove pcie_device reference from dpc_dev structure sathyanarayanan.kuppuswamy
@ 2020-02-26 13:43 ` Bjorn Helgaas
0 siblings, 0 replies; 18+ messages in thread
From: Bjorn Helgaas @ 2020-02-26 13:43 UTC (permalink / raw)
To: sathyanarayanan.kuppuswamy; +Cc: linux-pci, linux-kernel, ashok.raj
On Thu, Feb 13, 2020 at 10:20:14AM -0800, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>
> Currently the only use of pcie_device member in dpc_dev structure is to
> get the associated pci_dev reference. Since none of the users of
> dpc_dev need reference to pcie_device, just remove it and replace it
> with associated pci_dev pointer reference.
>
> Removing pcie_device reference will help if we have need to call DPC
> driver functions outside PCIe port drivers.
I like this a lot, thanks for doing this.
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
> drivers/pci/pcie/dpc.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index e06f42f58d3d..99fca8400956 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -18,7 +18,7 @@
> #include "../pci.h"
>
> struct dpc_dev {
> - struct pcie_device *dev;
> + struct pci_dev *pdev;
> u16 cap_pos;
> bool rp_extensions;
> u8 rp_log_size;
> @@ -101,7 +101,7 @@ void pci_restore_dpc_state(struct pci_dev *dev)
> static int dpc_wait_rp_inactive(struct dpc_dev *dpc)
> {
> unsigned long timeout = jiffies + HZ;
> - struct pci_dev *pdev = dpc->dev->port;
> + struct pci_dev *pdev = dpc->pdev;
> u16 cap = dpc->cap_pos, status;
>
> pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
> @@ -149,7 +149,7 @@ static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
>
> static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
> {
> - struct pci_dev *pdev = dpc->dev->port;
> + struct pci_dev *pdev = dpc->pdev;
> u16 cap = dpc->cap_pos, dpc_status, first_error;
> u32 status, mask, sev, syserr, exc, dw0, dw1, dw2, dw3, log, prefix;
> int i;
> @@ -228,7 +228,7 @@ static irqreturn_t dpc_handler(int irq, void *context)
> {
> struct aer_err_info info;
> struct dpc_dev *dpc = context;
> - struct pci_dev *pdev = dpc->dev->port;
> + struct pci_dev *pdev = dpc->pdev;
> u16 cap = dpc->cap_pos, status, source, reason, ext_reason;
>
> pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
> @@ -267,7 +267,7 @@ static irqreturn_t dpc_handler(int irq, void *context)
> static irqreturn_t dpc_irq(int irq, void *context)
> {
> struct dpc_dev *dpc = (struct dpc_dev *)context;
> - struct pci_dev *pdev = dpc->dev->port;
> + struct pci_dev *pdev = dpc->pdev;
> u16 cap = dpc->cap_pos, status;
>
> pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
> @@ -299,7 +299,7 @@ static int dpc_probe(struct pcie_device *dev)
> return -ENOMEM;
>
> dpc->cap_pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DPC);
> - dpc->dev = dev;
> + dpc->pdev = pdev;
> set_service_data(dev, dpc);
>
> status = devm_request_threaded_irq(device, dev->irq, dpc_irq,
> --
> 2.21.0
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v15 4/5] PCI/DPC: Add Error Disconnect Recover (EDR) support
2020-02-26 1:02 ` Bjorn Helgaas
@ 2020-02-26 18:42 ` Kuppuswamy Sathyanarayanan
2020-02-26 21:32 ` Bjorn Helgaas
0 siblings, 1 reply; 18+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2020-02-26 18:42 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-pci, linux-kernel, ashok.raj
HI Bjorn,
Thanks for the review.
On 2/25/20 5:02 PM, Bjorn Helgaas wrote:
> On Thu, Feb 13, 2020 at 10:20:16AM -0800, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>
>> As per ACPI specification r6.3, sec 5.6.6, when firmware owns Downstream
>> Port Containment (DPC), its expected to use the "Error Disconnect
>> Recover" (EDR) notification to alert OSPM of a DPC event and if OS
>> supports EDR, its expected to handle the software state invalidation and
>> port recovery in OS, and also let firmware know the recovery status via
>> _OST ACPI call. Related _OST status codes can be found in ACPI
>> specification r6.3, sec 6.3.5.2.
>>
>> Also, as per PCI firmware specification r3.2 Downstream Port Containment
>> Related Enhancements ECN, sec 4.5.1, table 4-6, If DPC is controlled by
>> firmware (firmware first mode), firmware is responsible for
>> configuring the DPC and OS is responsible for error recovery. Also, OS
>> is allowed to modify DPC registers only during the EDR notification
>> window.
>>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>> drivers/pci/pci-acpi.c | 3 +
>> drivers/pci/pcie/Kconfig | 10 ++
>> drivers/pci/pcie/Makefile | 1 +
>> drivers/pci/pcie/edr.c | 257 ++++++++++++++++++++++++++++++++++++++
>> include/linux/pci-acpi.h | 8 ++
>> 5 files changed, 279 insertions(+)
>> create mode 100644 drivers/pci/pcie/edr.c
>>
>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>> index 0c02d500158f..6af5d6a04990 100644
>> --- a/drivers/pci/pci-acpi.c
>> +++ b/drivers/pci/pci-acpi.c
>> @@ -1258,6 +1258,7 @@ static void pci_acpi_setup(struct device *dev)
>>
>> acpi_pci_wakeup(pci_dev, false);
>> acpi_device_power_add_dependent(adev, dev);
>> + pci_acpi_add_edr_notifier(pci_dev);
>> }
>>
>> static void pci_acpi_cleanup(struct device *dev)
>> @@ -1276,6 +1277,8 @@ static void pci_acpi_cleanup(struct device *dev)
>>
>> device_set_wakeup_capable(dev, false);
>> }
>> +
>> + pci_acpi_remove_edr_notifier(pci_dev);
>> }
>>
>> static bool pci_acpi_bus_match(struct device *dev)
>> diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
>> index 6e3c04b46fb1..772b1f4cb19e 100644
>> --- a/drivers/pci/pcie/Kconfig
>> +++ b/drivers/pci/pcie/Kconfig
>> @@ -140,3 +140,13 @@ config PCIE_BW
>> This enables PCI Express Bandwidth Change Notification. If
>> you know link width or rate changes occur only to correct
>> unreliable links, you may answer Y.
>> +
>> +config PCIE_EDR
>> + bool "PCI Express Error Disconnect Recover support"
>> + depends on PCIE_DPC && ACPI
>> + help
>> + This option adds Error Disconnect Recover support as specified
>> + in the Downstream Port Containment Related Enhancements ECN to
>> + the PCI Firmware Specification r3.2. Enable this if you want to
>> + support hybrid DPC model which uses both firmware and OS to
>> + implement DPC.
>> diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
>> index efb9d2e71e9e..68da9280ff11 100644
>> --- a/drivers/pci/pcie/Makefile
>> +++ b/drivers/pci/pcie/Makefile
>> @@ -13,3 +13,4 @@ obj-$(CONFIG_PCIE_PME) += pme.o
>> obj-$(CONFIG_PCIE_DPC) += dpc.o
>> obj-$(CONFIG_PCIE_PTM) += ptm.o
>> obj-$(CONFIG_PCIE_BW) += bw_notification.o
>> +obj-$(CONFIG_PCIE_EDR) += edr.o
>> diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c
>> new file mode 100644
>> index 000000000000..b3e9103585a1
>> --- /dev/null
>> +++ b/drivers/pci/pcie/edr.c
>> @@ -0,0 +1,257 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * PCI DPC Error Disconnect Recover support driver
>> + * Author: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> + *
>> + * Copyright (C) 2020 Intel Corp.
>> + */
>> +
>> +#define dev_fmt(fmt) "EDR: " fmt
>> +
>> +#include <linux/pci.h>
>> +#include <linux/pci-acpi.h>
>> +
>> +#include "portdrv.h"
>> +#include "dpc.h"
>> +#include "../pci.h"
>> +
>> +#define EDR_PORT_ENABLE_DSM 0x0C
>> +#define EDR_PORT_LOCATE_DSM 0x0D
>> +#define EDR_OST_SUCCESS 0x80
>> +#define EDR_OST_FAILED 0x81
>> +
>> +/*
>> + * _DSM wrapper function to enable/disable DPC port.
>> + * @pdev : PCI device structure.
>> + * @enable: status of DPC port (0 or 1).
> Either line up these colons or join them with the parameter names
> (also below).
ok. will do it.
>
>> + * returns 0 on success or errno on failure.
>> + */
>> +static int acpi_enable_dpc_port(struct pci_dev *pdev, bool enable)
> We only call this with "true", so the "enable" parameter is pointless.
Makes sense. I will remove it. We can add it if needed in future.
>
>> +{
>> + struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
>> + union acpi_object *obj, argv4, req;
>> + int status = 0;
>> +
>> + req.type = ACPI_TYPE_INTEGER;
>> + req.integer.value = enable;
>> +
>> + argv4.type = ACPI_TYPE_PACKAGE;
>> + argv4.package.count = 1;
>> + argv4.package.elements = &req;
>> +
>> + /*
>> + * Per the Downstream Port Containment Related Enhancements ECN to
>> + * the PCI Firmware Specification r3.2, sec 4.6.12,
>> + * EDR_PORT_ENABLE_DSM is optional. Return success if it's not
>> + * implemented.
>> + */
>> + obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
>> + EDR_PORT_ENABLE_DSM, &argv4);
>> + if (!obj)
>> + return 0;
>> +
>> + if (obj->type != ACPI_TYPE_INTEGER || obj->integer.value != enable)
>> + status = -EIO;
>> +
>> + ACPI_FREE(obj);
>> +
>> + return status;
>> +}
>> +
>> +/*
>> + * _DSM wrapper function to locate DPC port.
>> + * @pdev : Device which received EDR event.
>> + *
>> + * returns pci_dev or NULL.
>> + */
>> +static struct pci_dev *acpi_locate_dpc_port(struct pci_dev *pdev)
>> +{
>> + struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
>> + union acpi_object *obj;
>> + u16 port;
>> +
>> + obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
>> + EDR_PORT_LOCATE_DSM, NULL);
>> + if (!obj)
>> + return pci_dev_get(pdev);
>> +
>> + if (obj->type != ACPI_TYPE_INTEGER) {
> If this happens, it's a firmware defect, isn't it? I think maybe we
> should warn here. I know we warn below ("No valid port found"), but
> that doesn't give any clue about the fact that firmware screwed up.
I will add it in next version.
>
>> + ACPI_FREE(obj);
>> + return NULL;
>> + }
>> +
>> + /*
>> + * Firmware returns DPC port BDF details in following format:
>> + * 15:8 = bus
>> + * 7:3 = device
>> + * 2:0 = function
>> + */
>> + port = obj->integer.value;
>> +
>> + ACPI_FREE(obj);
>> +
>> + return pci_get_domain_bus_and_slot(pci_domain_nr(pdev->bus),
>> + PCI_BUS_NUM(port), port & 0xff);
>> +}
>> +
>> +/*
>> + * _OST wrapper function to let firmware know the status of EDR event.
>> + * @pdev : Device used to send _OST.
>> + * @edev : Device which experienced EDR event.
>> + * @status: Status of EDR event.
>> + */
>> +static int acpi_send_edr_status(struct pci_dev *pdev, struct pci_dev *edev,
>> + u16 status)
>> +{
>> + struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
>> + u32 ost_status;
>> +
>> + pci_dbg(pdev, "Sending EDR status :%#x\n", status);
>> +
>> + ost_status = PCI_DEVID(edev->bus->number, edev->devfn);
>> + ost_status = (ost_status << 16) | status;
>> +
>> + status = acpi_evaluate_ost(adev->handle,
>> + ACPI_NOTIFY_DISCONNECT_RECOVER,
>> + ost_status, NULL);
>> + if (ACPI_FAILURE(status))
>> + return -EINVAL;
>> +
>> + return 0;
>> +}
>> +
>> +pci_ers_result_t edr_dpc_reset_link(void *cb_data)
>> +{
>> + return dpc_reset_link_common(cb_data);
>> +}
>> +
>> +static void edr_handle_event(acpi_handle handle, u32 event, void *data)
>> +{
>> + struct dpc_dev *dpc = data, ndpc;
> There's actually very little use of struct dpc_dev in this file. I
> bet with a little elbow grease, we could remove it completely and just
> use the pci_dev * or maybe an opaque pointer.
Yes, we could remove it. But it might need some more changes to
dpc driver functions. I can think of two ways,
1. Re-factor the DPC driver not to use dpc_dev structure and just use
pci_dev in their functions implementation. But this might lead to
re-reading following dpc_dev structure members every time we
use it in dpc driver functions.
(Currently in dpc driver probe they cache the following device parameters )
9 u16 cap_pos;
10 bool rp_extensions;
11 u8 rp_log_size;
12 u16 ctl;
13 u16 cap;
2. We can create a new variant of dpc_process_err() which depends on
pci_dev
structure and move the dpc_dev initialization to it. Downside is, we
should do this
initialization every time we get DPC event (which should be rare).
void dpc_process_error(struct pci_dev *pdev)
{
struct dpc_dev dpc;
dpc_dev_init(pdev, &dpc);
....
}
Let me know your comments.
>
>> + struct pci_dev *pdev = dpc->pdev;
>> + pci_ers_result_t estate = PCI_ERS_RESULT_DISCONNECT;
>> + u16 status;
>> +
>> + pci_info(pdev, "ACPI event %#x received\n", event);
>> +
>> + if (event != ACPI_NOTIFY_DISCONNECT_RECOVER)
>> + return;
>> +
>> + /*
>> + * Check if _DSM(0xD) is available, and if present locate the
>> + * port which issued EDR event.
>> + */
>> + pdev = acpi_locate_dpc_port(pdev);
> This function name should include "get" since it's part of the
> pci_dev_get()/pci_dev_put() sequence.
How about acpi_dpc_port_get(pdev) ?
>
>> + if (!pdev) {
>> + pci_err(dpc->pdev, "No valid port found\n");
>> + return;
>> + }
>> +
>> + if (pdev != dpc->pdev) {
>> + pci_warn(pdev, "Initializing dpc again\n");
>> + dpc_dev_init(pdev, &ndpc);
> This seems... I'm not sure what. I guess it's really just reading
> the DPC capability for use by dpc_process_error(), so maybe it's OK.
> But it's a little strange to read.
>
> Is this something we should be warning about?
No this is a valid case. it will only happen if we have a non-acpi based
switch
attached to root port.
> I think the ECR says
> it's legitimate to return a child device, doesn't it?
>
>> + dpc= &ndpc;
> Space before "=".
will fix it in next version.
>
>> + }
>> +
>> + /*
>> + * If port does not support DPC, just send the OST:
>> + */
>> + if (!dpc->cap_pos)
>> + goto send_ost;
>> +
>> + /* Check if there is a valid DPC trigger */
>> + pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_STATUS, &status);
>> + if (!(status & PCI_EXP_DPC_STATUS_TRIGGER)) {
>> + pci_err(pdev, "Invalid DPC trigger %#010x\n", status);
>> + goto send_ost;
>> + }
>> +
>> + dpc_process_error(dpc);
>> +
>> + /* Clear AER registers */
>> + pci_aer_clear_err_uncor_status(pdev);
>> + pci_aer_clear_err_fatal_status(pdev);
>> + pci_aer_clear_err_status_regs(pdev);
>> +
>> + /*
>> + * Irrespective of whether the DPC event is triggered by
>> + * ERR_FATAL or ERR_NONFATAL, since the link is already down,
>> + * use the FATAL error recovery path for both cases.
>> + */
>> + estate = pcie_do_recovery_common(pdev, pci_channel_io_frozen, -1,
>> + edr_dpc_reset_link, dpc);
>> +send_ost:
>> +
>> + /* Use ACPI handle of DPC event device for sending EDR status */
>> + dpc = data;
> I think it'd be clearer to reserve "dpc" for the device that received
> the notification and to which we send the status, and use a different
> "e_dpc" or something for the dpc_process_error() and related code.
ok. will make this change in next version.
>
>> + /*
>> + * If recovery is successful, send _OST(0xF, BDF << 16 | 0x80)
>> + * to firmware. If not successful, send _OST(0xF, BDF << 16 | 0x81).
>> + */
>> + if (estate == PCI_ERS_RESULT_RECOVERED)
>> + acpi_send_edr_status(dpc->pdev, pdev, EDR_OST_SUCCESS);
>> + else
>> + acpi_send_edr_status(dpc->pdev, pdev, EDR_OST_FAILED);
>> +
>> + pci_dev_put(pdev);
>> +}
>> +
>> +int pci_acpi_add_edr_notifier(struct pci_dev *pdev)
>> +{
>> + struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
>> + struct dpc_dev *dpc;
>> + acpi_status astatus;
>> + int status;
>> +
>> + /*
>> + * Per the Downstream Port Containment Related Enhancements ECN to
>> + * the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-6, EDR support
>> + * can only be enabled if DPC is controlled by firmware.
>> + *
>> + * TODO: Remove dependency on ACPI FIRMWARE_FIRST bit to
>> + * determine ownership of DPC between firmware or OS.
> Extend the comment to say how we *should* determine ownership.
Yes, ownership should be based on _OSC negotiation. I will add necessary
comments here.
>
>> + */
>> + if (!pcie_aer_get_firmware_first(pdev) || pcie_ports_dpc_native)
>> + return -ENODEV;
>> +
>> + if (!adev)
>> + return 0;
>> +
>> + dpc = devm_kzalloc(&pdev->dev, sizeof(*dpc), GFP_KERNEL);
> This kzalloc should be in dpc.c, not here.
>
> And I don't see a corresponding free.
It will be freed when removing the pdev right ? Do you want to free it
explicitly in this driver ?
>
>> + if (!dpc)
>> + return -ENOMEM;
>> +
>> + dpc_dev_init(pdev, dpc);
>> +
>> + astatus = acpi_install_notify_handler(adev->handle,
>> + ACPI_SYSTEM_NOTIFY,
>> + edr_handle_event, dpc);
>> + if (ACPI_FAILURE(astatus)) {
>> + pci_err(pdev, "Install ACPI_SYSTEM_NOTIFY handler failed\n");
>> + return -EBUSY;
>> + }
>> +
>> + status = acpi_enable_dpc_port(pdev, true);
>> + if (status) {
>> + pci_warn(pdev, "Enable DPC port failed\n");
>> + acpi_remove_notify_handler(adev->handle,
>> + ACPI_SYSTEM_NOTIFY,
>> + edr_handle_event);
>> + return status;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +void pci_acpi_remove_edr_notifier(struct pci_dev *pdev)
>> +{
>> + struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
>> +
>> + if (!adev)
>> + return;
>> +
>> + acpi_remove_notify_handler(adev->handle, ACPI_SYSTEM_NOTIFY,
>> + edr_handle_event);
>> +}
>> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
>> index 62b7fdcc661c..a430e5fc50f3 100644
>> --- a/include/linux/pci-acpi.h
>> +++ b/include/linux/pci-acpi.h
>> @@ -112,6 +112,14 @@ extern const guid_t pci_acpi_dsm_guid;
>> #define RESET_DELAY_DSM 0x08
>> #define FUNCTION_DELAY_DSM 0x09
>>
>> +#ifdef CONFIG_PCIE_EDR
>> +int pci_acpi_add_edr_notifier(struct pci_dev *pdev);
>> +void pci_acpi_remove_edr_notifier(struct pci_dev *pdev);
>> +#else
>> +static inline int pci_acpi_add_edr_notifier(struct pci_dev *pdev) { return 0; }
>> +static inline void pci_acpi_remove_edr_notifier(struct pci_dev *pdev) { }
>> +#endif /* CONFIG_PCIE_EDR */
>> +
>> #else /* CONFIG_ACPI */
>> static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
>> static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
>> --
>> 2.21.0
>>
--
Sathyanarayanan Kuppuswamy
Linux kernel developer
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v15 3/5] PCI/EDR: Export AER, DPC and error recovery functions
2020-02-26 1:02 ` Bjorn Helgaas
@ 2020-02-26 19:30 ` Kuppuswamy Sathyanarayanan
2020-02-26 21:13 ` Bjorn Helgaas
0 siblings, 1 reply; 18+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2020-02-26 19:30 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-pci, linux-kernel, ashok.raj
Hi Bjorn,
On 2/25/20 5:02 PM, Bjorn Helgaas wrote:
> On Thu, Feb 13, 2020 at 10:20:15AM -0800, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>
>> This is a preparatory patch for adding EDR support.
>>
>> As per the Downstream Port Containment Related Enhancements ECN to the
>> PCI Firmware Specification r3.2, sec 4.5.1, table 4-6, If DPC is
>> controlled by firmware, firmware is responsible for initializing
>> Downstream Port Containment Extended Capability Structures per firmware
>> policy. Further, the OS is permitted to read or write DPC Control and
>> Status registers of a port while processing an Error Disconnect Recover
>> notification from firmware on that port. Error Disconnect Recover
>> notification processing begins with the Error Disconnect Recover notify
>> from Firmware, and ends when the OS releases DPC by clearing the DPC
>> Trigger Status bit.Firmware can read DPC Trigger Status bit to determine
>> the ownership of DPC Control and Status registers. Firmware is not
>> permitted to write to DPC Control and Status registers if DPC Trigger
>> Status is set i.e. the link is in DPC state. Outside of the Error
>> Disconnect Recover notification processing window, the OS is not
>> permitted to modify DPC Control or Status registers; only firmware
>> is allowed to.
>>
>> To add EDR support we need to re-use some of the existing DPC,
>> AER and pCIE error recovery functions. So add necessary interfaces.
>>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>> drivers/pci/pci.h | 8 ++++
>> drivers/pci/pcie/aer.c | 39 ++++++++++++++------
>> drivers/pci/pcie/dpc.c | 84 +++++++++++++++++++++++++-----------------
>> drivers/pci/pcie/dpc.h | 20 ++++++++++
>> drivers/pci/pcie/err.c | 30 ++++++++++++---
>> 5 files changed, 131 insertions(+), 50 deletions(-)
>> create mode 100644 drivers/pci/pcie/dpc.h
>>
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index 6394e7746fb5..136f27cf3871 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -443,6 +443,9 @@ struct aer_err_info {
>>
>> int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info);
>> void aer_print_error(struct pci_dev *dev, struct aer_err_info *info);
>> +int pci_aer_clear_err_uncor_status(struct pci_dev *dev);
>> +void pci_aer_clear_err_fatal_status(struct pci_dev *dev);
>> +int pci_aer_clear_err_status_regs(struct pci_dev *dev);
>> #endif /* CONFIG_PCIEAER */
>>
>> #ifdef CONFIG_PCIE_DPC
>> @@ -549,6 +552,11 @@ static inline int pci_dev_specific_disable_acs_redir(struct pci_dev *dev)
>> /* PCI error reporting and recovery */
>> void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
>> u32 service);
>> +pci_ers_result_t pcie_do_recovery_common(struct pci_dev *dev,
>> + enum pci_channel_state state,
>> + u32 service,
>> + pci_ers_result_t (*reset_cb)(void *cb_data),
>> + void *cb_data);
>>
>> bool pcie_wait_for_link(struct pci_dev *pdev, bool active);
>> #ifdef CONFIG_PCIEASPM
>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>> index 4a818b07a1af..399836aa07f4 100644
>> --- a/drivers/pci/pcie/aer.c
>> +++ b/drivers/pci/pcie/aer.c
>> @@ -376,7 +376,7 @@ void pci_aer_clear_device_status(struct pci_dev *dev)
>> pcie_capability_write_word(dev, PCI_EXP_DEVSTA, sta);
>> }
>>
>> -int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
>> +int pci_aer_clear_err_uncor_status(struct pci_dev *dev)
>> {
>> int pos;
>> u32 status, sev;
>> @@ -385,9 +385,6 @@ int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
>> if (!pos)
>> return -EIO;
>>
>> - if (pcie_aer_get_firmware_first(dev))
>> - return -EIO;
>> -
>> /* Clear status bits for ERR_NONFATAL errors only */
>> pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, &status);
>> pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, &sev);
>> @@ -397,9 +394,17 @@ int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
>>
>> return 0;
>> }
>> +
>> +int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
>> +{
>> + if (pcie_aer_get_firmware_first(dev))
>> + return -EIO;
>> +
>> + return pci_aer_clear_err_uncor_status(dev);
>> +}
>> EXPORT_SYMBOL_GPL(pci_cleanup_aer_uncorrect_error_status);
>>
>> -void pci_aer_clear_fatal_status(struct pci_dev *dev)
>> +void pci_aer_clear_err_fatal_status(struct pci_dev *dev)
>> {
>> int pos;
>> u32 status, sev;
>> @@ -408,9 +413,6 @@ void pci_aer_clear_fatal_status(struct pci_dev *dev)
>> if (!pos)
>> return;
>>
>> - if (pcie_aer_get_firmware_first(dev))
>> - return;
>> -
>> /* Clear status bits for ERR_FATAL errors only */
>> pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, &status);
>> pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, &sev);
>> @@ -419,7 +421,15 @@ void pci_aer_clear_fatal_status(struct pci_dev *dev)
>> pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, status);
>> }
>>
>> -int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
>> +void pci_aer_clear_fatal_status(struct pci_dev *dev)
>> +{
>> + if (pcie_aer_get_firmware_first(dev))
>> + return;
>> +
>> + return pci_aer_clear_err_fatal_status(dev);
>> +}
>> +
>> +int pci_aer_clear_err_status_regs(struct pci_dev *dev)
>> {
>> int pos;
>> u32 status;
>> @@ -432,9 +442,6 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
>> if (!pos)
>> return -EIO;
>>
>> - if (pcie_aer_get_firmware_first(dev))
>> - return -EIO;
>> -
>> port_type = pci_pcie_type(dev);
>> if (port_type == PCI_EXP_TYPE_ROOT_PORT) {
>> pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, &status);
>> @@ -450,6 +457,14 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
>> return 0;
>> }
>>
>> +int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
>> +{
>> + if (pcie_aer_get_firmware_first(dev))
>> + return -EIO;
>> +
>> + return pci_aer_clear_err_status_regs(dev);
>> +}
> We started with these:
>
> pci_cleanup_aer_uncorrect_error_status()
> pci_aer_clear_fatal_status()
> pci_cleanup_aer_error_status_regs()
>
> This was already a mess. They do basically similar things, but the
> function names are a complete jumble. Let's start with preliminary
> patches to name them consistently, e.g.,
>
> pci_aer_clear_nonfatal_status()
> pci_aer_clear_fatal_status()
> pci_aer_clear_status()
>
> Now, for EDR you eventually add this in edr_handle_event():
>
> edr_handle_event()
> {
> ...
> pci_aer_clear_err_uncor_status(pdev);
> pci_aer_clear_err_fatal_status(pdev);
> pci_aer_clear_err_status_regs(pdev);
>
> I see that this path needs to clear the status even in the
> firmware-first case, so you do need some change for that. But
> pci_aer_clear_err_status_regs() does exactly the same thing as
> pci_aer_clear_err_uncor_status() and pci_aer_clear_err_fatal_status()
> plus a little more (clearing PCI_ERR_ROOT_STATUS), so it should be
> sufficient to just call pci_aer_clear_err_status_regs().
>
> So I don't think you need to make wrappers for
> pci_aer_clear_nonfatal_status() and pci_aer_clear_fatal_status() at
> all since they're not needed by the EDR path.
>
> You *do* need a wrapper for pci_aer_clear_status(), but instead of
> just adding random words ("err", "regs") to the name, please name it
> something like pci_aer_raw_clear_status() as a hint that it skips
> some sort of check.
>
> I would split this into a separate patch. This patch contains a bunch
> of things that aren't really related except that they're needed for
> EDR. I think it will be more readable if each patch just does one
> piece of it.
Ok. I will split it into multiple patches. I just grouped them together
as a preparatory patch for adding EDR support.
>
>> void pci_save_aer_state(struct pci_dev *dev)
>> {
>> struct pci_cap_saved_state *save_state;
>> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
>> index 99fca8400956..acae12dbf9ff 100644
>> --- a/drivers/pci/pcie/dpc.c
>> +++ b/drivers/pci/pcie/dpc.c
>> @@ -15,15 +15,9 @@
>> #include <linux/pci.h>
>>
>> #include "portdrv.h"
>> +#include "dpc.h"
>> #include "../pci.h"
>>
>> -struct dpc_dev {
>> - struct pci_dev *pdev;
>> - u16 cap_pos;
>> - bool rp_extensions;
>> - u8 rp_log_size;
>> -};
> Adding dpc.h shouldn't be in this patch because there's no need for a
> separate dpc.h file yet -- it's only included this one place. I'm not
> positive a dpc.h is needed at all -- see comments on patch [4/5].
Yes, if we use pdev in dpc functions used by EDR code, we should
not need it.
>
>> static const char * const rp_pio_error_string[] = {
>> "Configuration Request received UR Completion", /* Bit Position 0 */
>> "Configuration Request received CA Completion", /* Bit Position 1 */
>> @@ -117,36 +111,44 @@ static int dpc_wait_rp_inactive(struct dpc_dev *dpc)
>> return 0;
>> }
>>
>> -static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
>> +pci_ers_result_t dpc_reset_link_common(struct dpc_dev *dpc)
>> {
> I don't see why you need to split this into dpc_reset_link_common()
> and dpc_reset_link(). IIUC, you split it so the DPC driver path can
> supply a pci_dev * via
>
> dpc_handler
> pcie_do_recovery
> pcie_do_recovery_common(..., NULL, NULL)
> reset_link(..., NULL, NULL)
> driver->reset_link(pdev)
> dpc_reset_link(pdev)
> dpc = to_dpc_dev(pdev)
I have split it mainly because of dpc_dev dependency. If we use
dpc_reset_link(pdev) directly it will try to find related dpc_dev using
to_dpc_dev() function. But this will not work in FF mode where DPC
is handled by firmware and hence we will not have DPC pcie_port
service driver enumerated for this device.
> dpc_reset_link_common(dpc)
>
> while the EDR path can supply a dpc_dev * via
>
> edr_handle_event
> pcie_do_recovery_common(..., edr_dpc_reset_link, dpc)
> reset_link(..., edr_dpc_reset_link, dpc)
> edr_dpc_reset_link(dpc)
> dpc_reset_link_common(dpc)
>
> But it looks like you *could* make these paths look like:
>
> dpc_handler
> pcie_do_recovery
> pcie_do_recovery_common(..., NULL, NULL)
> reset_link(..., NULL, NULL)
> driver->reset_link(pdev)
> dpc_reset_link(pdev)
>
> edr_handle_event
> pcie_do_recovery_common(..., dpc_reset_link, pdev)
> reset_link(..., dpc_reset_link, pdev)
> dpc_reset_link(pdev)
>
> and you wouldn't need edr_dpc_reset_link() at all and you wouldn't
> have to split dpc_reset_link_common() out.
>
>> - struct dpc_dev *dpc;
>> u16 cap;
>>
>> - /*
>> - * DPC disables the Link automatically in hardware, so it has
>> - * already been reset by the time we get here.
>> - */
>> - dpc = to_dpc_dev(pdev);
>> cap = dpc->cap_pos;
>>
>> /*
>> * Wait until the Link is inactive, then clear DPC Trigger Status
>> * to allow the Port to leave DPC.
>> */
>> - pcie_wait_for_link(pdev, false);
>> + pcie_wait_for_link(dpc->pdev, false);
> I'm hoping you don't need to split this out at all, but if you do,
> adding
>
> struct pci_dev *pdev = dpc->pdev;
>
> at the top will avoid these needless diffs.
>
>> if (dpc->rp_extensions && dpc_wait_rp_inactive(dpc))
>> return PCI_ERS_RESULT_DISCONNECT;
>>
>> - pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
>> + pci_write_config_word(dpc->pdev, cap + PCI_EXP_DPC_STATUS,
>> PCI_EXP_DPC_STATUS_TRIGGER);
>>
>> - if (!pcie_wait_for_link(pdev, true))
>> + if (!pcie_wait_for_link(dpc->pdev, true))
>> return PCI_ERS_RESULT_DISCONNECT;
>>
>> return PCI_ERS_RESULT_RECOVERED;
>> }
>>
>> +static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
>> +{
>> + struct dpc_dev *dpc;
>> +
>> + /*
>> + * DPC disables the Link automatically in hardware, so it has
>> + * already been reset by the time we get here.
>> + */
>> + dpc = to_dpc_dev(pdev);
>> +
>> + return dpc_reset_link_common(dpc);
>> +
>> +}
>> +
>> static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
>> {
>> struct pci_dev *pdev = dpc->pdev;
>> @@ -224,10 +226,9 @@ static int dpc_get_aer_uncorrect_severity(struct pci_dev *dev,
>> return 1;
>> }
>>
>> -static irqreturn_t dpc_handler(int irq, void *context)
>> +void dpc_process_error(struct dpc_dev *dpc)
>> {
>> struct aer_err_info info;
>> - struct dpc_dev *dpc = context;
>> struct pci_dev *pdev = dpc->pdev;
>> u16 cap = dpc->cap_pos, status, source, reason, ext_reason;
>>
>> @@ -257,6 +258,14 @@ static irqreturn_t dpc_handler(int irq, void *context)
>> pci_cleanup_aer_uncorrect_error_status(pdev);
>> pci_aer_clear_fatal_status(pdev);
>> }
>> +}
>> +
>> +static irqreturn_t dpc_handler(int irq, void *context)
>> +{
>> + struct dpc_dev *dpc = context;
>> + struct pci_dev *pdev = dpc->pdev;
>> +
>> + dpc_process_error(dpc);
>>
>> /* We configure DPC so it only triggers on ERR_FATAL */
>> pcie_do_recovery(pdev, pci_channel_io_frozen, PCIE_PORT_SERVICE_DPC);
>> @@ -282,6 +291,25 @@ static irqreturn_t dpc_irq(int irq, void *context)
>> return IRQ_HANDLED;
>> }
>>
>> +void dpc_dev_init(struct pci_dev *pdev, struct dpc_dev *dpc)
>> +{
> Can you include the kzalloc() here so we don't have to do the alloc in
> pci_acpi_add_edr_notifier()?
Currently dpc driver allocates dpc_dev structure using pcie_port->device
reference in its devm_alloc* calls. But if we allocate dpc_dev inside
this function we should use pci_dev->dev reference for it. Is it OK to us
pci_dev->dev reference for DPC driver allocations ?
>
> I think there's also a leak there: pci_acpi_add_edr_notifier()
> kzallocs a struct dpc_dev, but I don't see a corresponding kfree().
since we are using devm_allocs, It should be freed when removing
the PCI device right?
>
>> + dpc->cap_pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DPC);
> I think we need to check cap_pos for non-zero here. It's arguably
> safe today because portdrv only calls dpc_probe() when
> PCIE_PORT_SERVICE_DPC is set and we only set that if there's a DPC
> capability. But that's a long ways away from here and it's
> convoluted, so it's pretty hard to verify that it's safe.
ok. makes sense.
>
> When we add EDR into the picture and call this from
> pci_acpi_add_edr_notifier() and edr_handle_event(), I'm pretty sure
> it's not safe at all because we have no idea whether the device has a
> DPC capability.
>
> Factoring out dpc_dev_init() and the kzalloc() would be a simple
> cleanup-type patch all by itself, so it could be separated out for
> ease of reviewing.
>
>> + dpc->pdev = pdev;
>> + pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CAP, &dpc->cap);
>> + pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, &dpc->ctl);
>> +
>> + dpc->rp_extensions = (dpc->cap & PCI_EXP_DPC_CAP_RP_EXT);
>> + if (dpc->rp_extensions) {
>> + dpc->rp_log_size =
>> + (dpc->cap & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8;
>> + if (dpc->rp_log_size < 4 || dpc->rp_log_size > 9) {
>> + pci_err(pdev, "RP PIO log size %u is invalid\n",
>> + dpc->rp_log_size);
>> + dpc->rp_log_size = 0;
>> + }
>> + }
>> +}
>> +
>> #define FLAG(x, y) (((x) & (y)) ? '+' : '-')
>> static int dpc_probe(struct pcie_device *dev)
>> {
>> @@ -298,8 +326,8 @@ static int dpc_probe(struct pcie_device *dev)
>> if (!dpc)
>> return -ENOMEM;
>>
>> - dpc->cap_pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DPC);
>> - dpc->pdev = pdev;
>> + dpc_dev_init(pdev, dpc);
>> +
>> set_service_data(dev, dpc);
>>
>> status = devm_request_threaded_irq(device, dev->irq, dpc_irq,
>> @@ -311,18 +339,8 @@ static int dpc_probe(struct pcie_device *dev)
>> return status;
>> }
>>
>> - pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CAP, &cap);
>> - pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, &ctl);
>> -
>> - dpc->rp_extensions = (cap & PCI_EXP_DPC_CAP_RP_EXT);
>> - if (dpc->rp_extensions) {
>> - dpc->rp_log_size = (cap & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8;
>> - if (dpc->rp_log_size < 4 || dpc->rp_log_size > 9) {
>> - pci_err(pdev, "RP PIO log size %u is invalid\n",
>> - dpc->rp_log_size);
>> - dpc->rp_log_size = 0;
>> - }
>> - }
>> + ctl = dpc->ctl;
>> + cap = dpc->cap;
>>
>> 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);
>> diff --git a/drivers/pci/pcie/dpc.h b/drivers/pci/pcie/dpc.h
>> new file mode 100644
>> index 000000000000..2d82bc917fcb
>> --- /dev/null
>> +++ b/drivers/pci/pcie/dpc.h
>> @@ -0,0 +1,20 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef DRIVERS_PCIE_DPC_H
>> +#define DRIVERS_PCIE_DPC_H
>> +
>> +#include <linux/pci.h>
>> +
>> +struct dpc_dev {
>> + struct pci_dev *pdev;
>> + u16 cap_pos;
>> + bool rp_extensions;
>> + u8 rp_log_size;
>> + u16 ctl;
>> + u16 cap;
>> +};
>> +
>> +pci_ers_result_t dpc_reset_link_common(struct dpc_dev *dpc);
>> +void dpc_process_error(struct dpc_dev *dpc);
>> +void dpc_dev_init(struct pci_dev *pdev, struct dpc_dev *dpc);
>> +
>> +#endif /* DRIVERS_PCIE_DPC_H */
>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>> index eefefe03857a..e7b9dfae9035 100644
>> --- a/drivers/pci/pcie/err.c
>> +++ b/drivers/pci/pcie/err.c
>> @@ -162,11 +162,18 @@ static pci_ers_result_t default_reset_link(struct pci_dev *dev)
>> return rc ? PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_RECOVERED;
>> }
>>
>> -static pci_ers_result_t reset_link(struct pci_dev *dev, u32 service)
>> +static pci_ers_result_t reset_link(struct pci_dev *dev, u32 service,
>> + pci_ers_result_t (*reset_cb)(void *cb_data),
>> + void *cb_data)
> This rejiggering of reset_link() and pcie_do_recovery() is unrelated
> to the rest (except that it's needed for EDR, of course), so maybe
> could be a separate patch.
ok
>
> We could also consider changing the interface of pcie_do_recovery() to
> just add reset_cb and cb_data, and change the existing callers to pass
> NULL, NULL. Then we wouldn't need pcie_do_recovery_common(). I'm not
> sure which way would be better.
I will change the pcie_do_recovery. Currently only aer and dpc drivers
uses it.
>
>> {
>> pci_ers_result_t status;
>> struct pcie_port_service_driver *driver = NULL;
>>
>> + if (reset_cb) {
>> + status = reset_cb(cb_data);
>> + goto done;
>> + }
>> +
>> driver = pcie_port_find_service(dev, service);
>> if (driver && driver->reset_link) {
>> status = driver->reset_link(dev);
>> @@ -178,6 +185,7 @@ static pci_ers_result_t reset_link(struct pci_dev *dev, u32 service)
>> return PCI_ERS_RESULT_DISCONNECT;
>> }
>>
>> +done:
>> if (status != PCI_ERS_RESULT_RECOVERED) {
>> pci_printk(KERN_DEBUG, dev, "link reset at upstream device %s failed\n",
>> pci_name(dev));
>> @@ -187,8 +195,11 @@ static pci_ers_result_t reset_link(struct pci_dev *dev, u32 service)
>> return status;
>> }
>>
>> -void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
>> - u32 service)
>> +pci_ers_result_t pcie_do_recovery_common(struct pci_dev *dev,
>> + enum pci_channel_state state,
>> + u32 service,
>> + pci_ers_result_t (*reset_cb)(void *cb_data),
>> + void *cb_data)
>> {
>> pci_ers_result_t status = PCI_ERS_RESULT_CAN_RECOVER;
>> struct pci_bus *bus;
>> @@ -209,7 +220,7 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
>> pci_walk_bus(bus, report_normal_detected, &status);
>>
>> if (state == pci_channel_io_frozen) {
>> - status = reset_link(dev, service);
>> + status = reset_link(dev, service, reset_cb, cb_data);
>> if (status != PCI_ERS_RESULT_RECOVERED)
>> goto failed;
>> }
>> @@ -240,11 +251,20 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
>> pci_aer_clear_device_status(dev);
>> pci_cleanup_aer_uncorrect_error_status(dev);
>> pci_info(dev, "device recovery successful\n");
>> - return;
>> +
>> + return status;
>>
>> failed:
>> pci_uevent_ers(dev, PCI_ERS_RESULT_DISCONNECT);
>>
>> /* TODO: Should kernel panic here? */
>> pci_info(dev, "device recovery failed\n");
>> +
>> + return status;
>> +}
>> +
>> +void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
>> + u32 service)
>> +{
>> + pcie_do_recovery_common(dev, state, service, NULL, NULL);
>> }
>> --
>> 2.21.0
>>
--
Sathyanarayanan Kuppuswamy
Linux kernel developer
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v15 3/5] PCI/EDR: Export AER, DPC and error recovery functions
2020-02-26 19:30 ` Kuppuswamy Sathyanarayanan
@ 2020-02-26 21:13 ` Bjorn Helgaas
2020-02-26 21:26 ` Kuppuswamy Sathyanarayanan
0 siblings, 1 reply; 18+ messages in thread
From: Bjorn Helgaas @ 2020-02-26 21:13 UTC (permalink / raw)
To: Kuppuswamy Sathyanarayanan; +Cc: linux-pci, linux-kernel, ashok.raj
On Wed, Feb 26, 2020 at 11:30:43AM -0800, Kuppuswamy Sathyanarayanan wrote:
> On 2/25/20 5:02 PM, Bjorn Helgaas wrote:
> > On Thu, Feb 13, 2020 at 10:20:15AM -0800, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > ...
> > > +int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
> > > +{
> > > + if (pcie_aer_get_firmware_first(dev))
> > > + return -EIO;
> > > +
> > > + return pci_aer_clear_err_status_regs(dev);
> > > +}
> > We started with these:
> >
> > pci_cleanup_aer_uncorrect_error_status()
> > pci_aer_clear_fatal_status()
> > pci_cleanup_aer_error_status_regs()
> >
> > This was already a mess. They do basically similar things, but the
> > function names are a complete jumble. Let's start with preliminary
> > patches to name them consistently, e.g.,
> >
> > pci_aer_clear_nonfatal_status()
> > pci_aer_clear_fatal_status()
> > pci_aer_clear_status()
> >
> > Now, for EDR you eventually add this in edr_handle_event():
> >
> > edr_handle_event()
> > {
> > ...
> > pci_aer_clear_err_uncor_status(pdev);
> > pci_aer_clear_err_fatal_status(pdev);
> > pci_aer_clear_err_status_regs(pdev);
> >
> > I see that this path needs to clear the status even in the
> > firmware-first case, so you do need some change for that. But
> > pci_aer_clear_err_status_regs() does exactly the same thing as
> > pci_aer_clear_err_uncor_status() and pci_aer_clear_err_fatal_status()
> > plus a little more (clearing PCI_ERR_ROOT_STATUS), so it should be
> > sufficient to just call pci_aer_clear_err_status_regs().
> >
> > So I don't think you need to make wrappers for
> > pci_aer_clear_nonfatal_status() and pci_aer_clear_fatal_status() at
> > all since they're not needed by the EDR path.
> >
> > You *do* need a wrapper for pci_aer_clear_status(), but instead of
> > just adding random words ("err", "regs") to the name, please name it
> > something like pci_aer_raw_clear_status() as a hint that it skips
> > some sort of check.
What do you think about the above?
> > I would split this into a separate patch. This patch contains a bunch
> > of things that aren't really related except that they're needed for
> > EDR. I think it will be more readable if each patch just does one
> > piece of it.
>
> Ok. I will split it into multiple patches. I just grouped them together
> as a preparatory patch for adding EDR support.
Sounds good.
> > > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > > index 99fca8400956..acae12dbf9ff 100644
> > > --- a/drivers/pci/pcie/dpc.c
> > > +++ b/drivers/pci/pcie/dpc.c
> > > @@ -15,15 +15,9 @@
> > > #include <linux/pci.h>
> > > #include "portdrv.h"
> > > +#include "dpc.h"
> > > #include "../pci.h"
> > > -struct dpc_dev {
> > > - struct pci_dev *pdev;
> > > - u16 cap_pos;
> > > - bool rp_extensions;
> > > - u8 rp_log_size;
> > > -};
> > Adding dpc.h shouldn't be in this patch because there's no need for a
> > separate dpc.h file yet -- it's only included this one place. I'm not
> > positive a dpc.h is needed at all -- see comments on patch [4/5].
> Yes, if we use pdev in dpc functions used by EDR code, we should
> not need it.
I think struct dpc_dev might be more trouble than it's worth. I
attached a possible patch at the end that folds the 25 bits of
DPC-related info into the struct pci_dev and gets rid of struct
dpc_dev completely. I compiled it but haven't tested it.
> > > -static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
> > > +pci_ers_result_t dpc_reset_link_common(struct dpc_dev *dpc)
> > > {
> > I don't see why you need to split this into dpc_reset_link_common()
> > and dpc_reset_link(). IIUC, you split it so the DPC driver path can
> > supply a pci_dev * via
> >
> > dpc_handler
> > pcie_do_recovery
> > pcie_do_recovery_common(..., NULL, NULL)
> > reset_link(..., NULL, NULL)
> > driver->reset_link(pdev)
> > dpc_reset_link(pdev)
> > dpc = to_dpc_dev(pdev)
>
> I have split it mainly because of dpc_dev dependency. If we use
> dpc_reset_link(pdev) directly it will try to find related dpc_dev using
> to_dpc_dev() function. But this will not work in FF mode where DPC
> is handled by firmware and hence we will not have DPC pcie_port
> service driver enumerated for this device.
The patch below might help this case.
> > > +void dpc_dev_init(struct pci_dev *pdev, struct dpc_dev *dpc)
> > > +{
> > Can you include the kzalloc() here so we don't have to do the alloc in
> > pci_acpi_add_edr_notifier()?
> Currently dpc driver allocates dpc_dev structure using pcie_port->device
> reference in its devm_alloc* calls. But if we allocate dpc_dev inside
> this function we should use pci_dev->dev reference for it. Is it OK to us
> pci_dev->dev reference for DPC driver allocations ?
I think the patch below would solve this issue too because we don't
need the alloc at all.
> > I think there's also a leak there: pci_acpi_add_edr_notifier()
> > kzallocs a struct dpc_dev, but I don't see a corresponding kfree().
> since we are using devm_allocs, It should be freed when removing
> the PCI device right?
Oh, right, sorry.
commit 7fb9f4495711 ("PCI/DPC: Move data to struct pci_dev")
Author: Bjorn Helgaas <bhelgaas@google.com>
Date: Wed Feb 26 08:00:55 2020 -0600
PCI/DPC: Move data to struct pci_dev
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index e06f42f58d3d..6b116d7fdb89 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -17,13 +17,6 @@
#include "portdrv.h"
#include "../pci.h"
-struct dpc_dev {
- struct pcie_device *dev;
- u16 cap_pos;
- bool rp_extensions;
- u8 rp_log_size;
-};
-
static const char * const rp_pio_error_string[] = {
"Configuration Request received UR Completion", /* Bit Position 0 */
"Configuration Request received CA Completion", /* Bit Position 1 */
@@ -46,63 +39,42 @@ static const char * const rp_pio_error_string[] = {
"Memory Request Completion Timeout", /* Bit Position 18 */
};
-static struct dpc_dev *to_dpc_dev(struct pci_dev *dev)
-{
- struct device *device;
-
- device = pcie_port_find_device(dev, PCIE_PORT_SERVICE_DPC);
- if (!device)
- return NULL;
- return get_service_data(to_pcie_device(device));
-}
-
void pci_save_dpc_state(struct pci_dev *dev)
{
- struct dpc_dev *dpc;
struct pci_cap_saved_state *save_state;
u16 *cap;
if (!pci_is_pcie(dev))
return;
- dpc = to_dpc_dev(dev);
- if (!dpc)
- return;
-
save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_DPC);
if (!save_state)
return;
cap = (u16 *)&save_state->cap.data[0];
- pci_read_config_word(dev, dpc->cap_pos + PCI_EXP_DPC_CTL, cap);
+ pci_read_config_word(dev, dev->dpc_cap + PCI_EXP_DPC_CTL, cap);
}
void pci_restore_dpc_state(struct pci_dev *dev)
{
- struct dpc_dev *dpc;
struct pci_cap_saved_state *save_state;
u16 *cap;
if (!pci_is_pcie(dev))
return;
- dpc = to_dpc_dev(dev);
- if (!dpc)
- return;
-
save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_DPC);
if (!save_state)
return;
cap = (u16 *)&save_state->cap.data[0];
- pci_write_config_word(dev, dpc->cap_pos + PCI_EXP_DPC_CTL, *cap);
+ pci_write_config_word(dev, dev->dpc_cap + PCI_EXP_DPC_CTL, *cap);
}
-static int dpc_wait_rp_inactive(struct dpc_dev *dpc)
+static int dpc_wait_rp_inactive(struct pci_dev *pdev)
{
unsigned long timeout = jiffies + HZ;
- struct pci_dev *pdev = dpc->dev->port;
- u16 cap = dpc->cap_pos, status;
+ u16 cap = pdev->dpc_cap, status;
pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
while (status & PCI_EXP_DPC_RP_BUSY &&
@@ -119,15 +91,13 @@ static int dpc_wait_rp_inactive(struct dpc_dev *dpc)
static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
{
- struct dpc_dev *dpc;
u16 cap;
/*
* DPC disables the Link automatically in hardware, so it has
* already been reset by the time we get here.
*/
- dpc = to_dpc_dev(pdev);
- cap = dpc->cap_pos;
+ cap = pdev->dpc_cap;
/*
* Wait until the Link is inactive, then clear DPC Trigger Status
@@ -135,7 +105,7 @@ static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
*/
pcie_wait_for_link(pdev, false);
- if (dpc->rp_extensions && dpc_wait_rp_inactive(dpc))
+ if (pdev->dpc_rp_extensions && dpc_wait_rp_inactive(pdev))
return PCI_ERS_RESULT_DISCONNECT;
pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
@@ -147,10 +117,9 @@ static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
return PCI_ERS_RESULT_RECOVERED;
}
-static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
+static void dpc_process_rp_pio_error(struct pci_dev *pdev)
{
- struct pci_dev *pdev = dpc->dev->port;
- u16 cap = dpc->cap_pos, dpc_status, first_error;
+ u16 cap = pdev->dpc_cap, dpc_status, first_error;
u32 status, mask, sev, syserr, exc, dw0, dw1, dw2, dw3, log, prefix;
int i;
@@ -175,7 +144,7 @@ static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
first_error == i ? " (First)" : "");
}
- if (dpc->rp_log_size < 4)
+ if (pdev->dpc_rp_log_size < 4)
goto clear_status;
pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_HEADER_LOG,
&dw0);
@@ -188,12 +157,12 @@ static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
pci_err(pdev, "TLP Header: %#010x %#010x %#010x %#010x\n",
dw0, dw1, dw2, dw3);
- if (dpc->rp_log_size < 5)
+ if (pdev->dpc_rp_log_size < 5)
goto clear_status;
pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_IMPSPEC_LOG, &log);
pci_err(pdev, "RP PIO ImpSpec Log %#010x\n", log);
- for (i = 0; i < dpc->rp_log_size - 5; i++) {
+ for (i = 0; i < pdev->dpc_rp_log_size - 5; i++) {
pci_read_config_dword(pdev,
cap + PCI_EXP_DPC_RP_PIO_TLPPREFIX_LOG, &prefix);
pci_err(pdev, "TLP Prefix Header: dw%d, %#010x\n", i, prefix);
@@ -226,10 +195,9 @@ static int dpc_get_aer_uncorrect_severity(struct pci_dev *dev,
static irqreturn_t dpc_handler(int irq, void *context)
{
+ struct pci_dev *pdev = context;
+ u16 cap = pdev->dpc_cap, status, source, reason, ext_reason;
struct aer_err_info info;
- struct dpc_dev *dpc = context;
- struct pci_dev *pdev = dpc->dev->port;
- u16 cap = dpc->cap_pos, status, source, reason, ext_reason;
pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
pci_read_config_word(pdev, cap + PCI_EXP_DPC_SOURCE_ID, &source);
@@ -248,8 +216,8 @@ static irqreturn_t dpc_handler(int irq, void *context)
"reserved error");
/* show RP PIO error detail information */
- if (dpc->rp_extensions && reason == 3 && ext_reason == 0)
- dpc_process_rp_pio_error(dpc);
+ if (pdev->dpc_rp_extensions && reason == 3 && ext_reason == 0)
+ dpc_process_rp_pio_error(pdev);
else if (reason == 0 &&
dpc_get_aer_uncorrect_severity(pdev, &info) &&
aer_get_device_error_info(pdev, &info)) {
@@ -266,9 +234,8 @@ static irqreturn_t dpc_handler(int irq, void *context)
static irqreturn_t dpc_irq(int irq, void *context)
{
- struct dpc_dev *dpc = (struct dpc_dev *)context;
- struct pci_dev *pdev = dpc->dev->port;
- u16 cap = dpc->cap_pos, status;
+ struct pci_dev *pdev = context;
+ u16 cap = pdev->dpc_cap, status;
pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
@@ -285,7 +252,6 @@ static irqreturn_t dpc_irq(int irq, void *context)
#define FLAG(x, y) (((x) & (y)) ? '+' : '-')
static int dpc_probe(struct pcie_device *dev)
{
- struct dpc_dev *dpc;
struct pci_dev *pdev = dev->port;
struct device *device = &dev->device;
int status;
@@ -294,43 +260,37 @@ static int dpc_probe(struct pcie_device *dev)
if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native)
return -ENOTSUPP;
- dpc = devm_kzalloc(device, sizeof(*dpc), GFP_KERNEL);
- if (!dpc)
- return -ENOMEM;
-
- dpc->cap_pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DPC);
- dpc->dev = dev;
- set_service_data(dev, dpc);
+ pdev->dpc_cap = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DPC);
status = devm_request_threaded_irq(device, dev->irq, dpc_irq,
dpc_handler, IRQF_SHARED,
- "pcie-dpc", dpc);
+ "pcie-dpc", pdev);
if (status) {
pci_warn(pdev, "request IRQ%d failed: %d\n", dev->irq,
status);
return status;
}
- pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CAP, &cap);
- pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, &ctl);
+ pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CAP, &cap);
+ pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl);
- dpc->rp_extensions = (cap & PCI_EXP_DPC_CAP_RP_EXT);
- if (dpc->rp_extensions) {
- dpc->rp_log_size = (cap & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8;
- if (dpc->rp_log_size < 4 || dpc->rp_log_size > 9) {
+ pdev->dpc_rp_extensions = (cap & PCI_EXP_DPC_CAP_RP_EXT) ? 1 : 0;
+ if (pdev->dpc_rp_extensions) {
+ pdev->dpc_rp_log_size = (cap & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8;
+ if (pdev->dpc_rp_log_size < 4 || pdev->dpc_rp_log_size > 9) {
pci_err(pdev, "RP PIO log size %u is invalid\n",
- dpc->rp_log_size);
- dpc->rp_log_size = 0;
+ pdev->dpc_rp_log_size);
+ pdev->dpc_rp_log_size = 0;
}
}
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);
+ pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl);
pci_info(pdev, "error containment capabilities: Int Msg #%d, RPExt%c PoisonedTLP%c SwTrigger%c RP PIO Log %d, DL_ActiveErr%c\n",
cap & PCI_EXP_DPC_IRQ, FLAG(cap, PCI_EXP_DPC_CAP_RP_EXT),
FLAG(cap, PCI_EXP_DPC_CAP_POISONED_TLP),
- FLAG(cap, PCI_EXP_DPC_CAP_SW_TRIGGER), dpc->rp_log_size,
+ FLAG(cap, PCI_EXP_DPC_CAP_SW_TRIGGER), pdev->dpc_rp_log_size,
FLAG(cap, PCI_EXP_DPC_CAP_DL_ACTIVE));
pci_add_ext_cap_save_buffer(pdev, PCI_EXT_CAP_ID_DPC, sizeof(u16));
@@ -339,13 +299,12 @@ static int dpc_probe(struct pcie_device *dev)
static void dpc_remove(struct pcie_device *dev)
{
- struct dpc_dev *dpc = get_service_data(dev);
struct pci_dev *pdev = dev->port;
u16 ctl;
- pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, &ctl);
+ pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl);
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);
+ pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl);
}
static struct pcie_port_service_driver dpcdriver = {
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 3840a541a9de..a0b7e7a53741 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -444,6 +444,11 @@ struct pci_dev {
const struct attribute_group **msi_irq_groups;
#endif
struct pci_vpd *vpd;
+#ifdef CONFIG_PCIE_DPC
+ u16 dpc_cap;
+ unsigned int dpc_rp_extensions:1;
+ u8 dpc_rp_log_size;
+#endif
#ifdef CONFIG_PCI_ATS
union {
struct pci_sriov *sriov; /* PF: SR-IOV info */
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v15 3/5] PCI/EDR: Export AER, DPC and error recovery functions
2020-02-26 21:13 ` Bjorn Helgaas
@ 2020-02-26 21:26 ` Kuppuswamy Sathyanarayanan
2020-02-26 21:48 ` Bjorn Helgaas
0 siblings, 1 reply; 18+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2020-02-26 21:26 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-pci, linux-kernel, ashok.raj
Hi Bjorn,
On 2/26/20 1:13 PM, Bjorn Helgaas wrote:
> On Wed, Feb 26, 2020 at 11:30:43AM -0800, Kuppuswamy Sathyanarayanan wrote:
>> On 2/25/20 5:02 PM, Bjorn Helgaas wrote:
>>> On Thu, Feb 13, 2020 at 10:20:15AM -0800, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>>>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>> ...
>>>> +int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
>>>> +{
>>>> + if (pcie_aer_get_firmware_first(dev))
>>>> + return -EIO;
>>>> +
>>>> + return pci_aer_clear_err_status_regs(dev);
>>>> +}
>>> We started with these:
>>>
>>> pci_cleanup_aer_uncorrect_error_status()
>>> pci_aer_clear_fatal_status()
>>> pci_cleanup_aer_error_status_regs()
>>>
>>> This was already a mess. They do basically similar things, but the
>>> function names are a complete jumble. Let's start with preliminary
>>> patches to name them consistently, e.g.,
>>>
>>> pci_aer_clear_nonfatal_status()
>>> pci_aer_clear_fatal_status()
>>> pci_aer_clear_status()
>>>
>>> Now, for EDR you eventually add this in edr_handle_event():
>>>
>>> edr_handle_event()
>>> {
>>> ...
>>> pci_aer_clear_err_uncor_status(pdev);
>>> pci_aer_clear_err_fatal_status(pdev);
>>> pci_aer_clear_err_status_regs(pdev);
>>>
>>> I see that this path needs to clear the status even in the
>>> firmware-first case, so you do need some change for that. But
>>> pci_aer_clear_err_status_regs() does exactly the same thing as
>>> pci_aer_clear_err_uncor_status() and pci_aer_clear_err_fatal_status()
>>> plus a little more (clearing PCI_ERR_ROOT_STATUS), so it should be
>>> sufficient to just call pci_aer_clear_err_status_regs().
>>>
>>> So I don't think you need to make wrappers for
>>> pci_aer_clear_nonfatal_status() and pci_aer_clear_fatal_status() at
>>> all since they're not needed by the EDR path.
>>>
>>> You *do* need a wrapper for pci_aer_clear_status(), but instead of
>>> just adding random words ("err", "regs") to the name, please name it
>>> something like pci_aer_raw_clear_status() as a hint that it skips
>>> some sort of check.
> What do you think about the above?
I agree with your comments.. I will use your recommendation in
naming the wrappers I need.
>
>>> I would split this into a separate patch. This patch contains a bunch
>>> of things that aren't really related except that they're needed for
>>> EDR. I think it will be more readable if each patch just does one
>>> piece of it.
>> Ok. I will split it into multiple patches. I just grouped them together
>> as a preparatory patch for adding EDR support.
> Sounds good.
>
>>>> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
>>>> index 99fca8400956..acae12dbf9ff 100644
>>>> --- a/drivers/pci/pcie/dpc.c
>>>> +++ b/drivers/pci/pcie/dpc.c
>>>> @@ -15,15 +15,9 @@
>>>> #include <linux/pci.h>
>>>> #include "portdrv.h"
>>>> +#include "dpc.h"
>>>> #include "../pci.h"
>>>> -struct dpc_dev {
>>>> - struct pci_dev *pdev;
>>>> - u16 cap_pos;
>>>> - bool rp_extensions;
>>>> - u8 rp_log_size;
>>>> -};
>>> Adding dpc.h shouldn't be in this patch because there's no need for a
>>> separate dpc.h file yet -- it's only included this one place. I'm not
>>> positive a dpc.h is needed at all -- see comments on patch [4/5].
>> Yes, if we use pdev in dpc functions used by EDR code, we should
>> not need it.
> I think struct dpc_dev might be more trouble than it's worth. I
> attached a possible patch at the end that folds the 25 bits of
> DPC-related info into the struct pci_dev and gets rid of struct
> dpc_dev completely. I compiled it but haven't tested it.
That would solve most of my export issues. Do you want me
to add it to my patch list or merge it separately.
>
>>>> -static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
>>>> +pci_ers_result_t dpc_reset_link_common(struct dpc_dev *dpc)
>>>> {
>>> I don't see why you need to split this into dpc_reset_link_common()
>>> and dpc_reset_link(). IIUC, you split it so the DPC driver path can
>>> supply a pci_dev * via
>>>
>>> dpc_handler
>>> pcie_do_recovery
>>> pcie_do_recovery_common(..., NULL, NULL)
>>> reset_link(..., NULL, NULL)
>>> driver->reset_link(pdev)
>>> dpc_reset_link(pdev)
>>> dpc = to_dpc_dev(pdev)
>> I have split it mainly because of dpc_dev dependency. If we use
>> dpc_reset_link(pdev) directly it will try to find related dpc_dev using
>> to_dpc_dev() function. But this will not work in FF mode where DPC
>> is handled by firmware and hence we will not have DPC pcie_port
>> service driver enumerated for this device.
> The patch below might help this case.
Yes.
>
>>>> +void dpc_dev_init(struct pci_dev *pdev, struct dpc_dev *dpc)
>>>> +{
>>> Can you include the kzalloc() here so we don't have to do the alloc in
>>> pci_acpi_add_edr_notifier()?
>> Currently dpc driver allocates dpc_dev structure using pcie_port->device
>> reference in its devm_alloc* calls. But if we allocate dpc_dev inside
>> this function we should use pci_dev->dev reference for it. Is it OK to us
>> pci_dev->dev reference for DPC driver allocations ?
> I think the patch below would solve this issue too because we don't
> need the alloc at all.
Agree.
>
>>> I think there's also a leak there: pci_acpi_add_edr_notifier()
>>> kzallocs a struct dpc_dev, but I don't see a corresponding kfree().
>> since we are using devm_allocs, It should be freed when removing
>> the PCI device right?
> Oh, right, sorry.
>
>
> commit 7fb9f4495711 ("PCI/DPC: Move data to struct pci_dev")
> Author: Bjorn Helgaas <bhelgaas@google.com>
> Date: Wed Feb 26 08:00:55 2020 -0600
>
> PCI/DPC: Move data to struct pci_dev
>
>
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index e06f42f58d3d..6b116d7fdb89 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -17,13 +17,6 @@
> #include "portdrv.h"
> #include "../pci.h"
>
> -struct dpc_dev {
> - struct pcie_device *dev;
> - u16 cap_pos;
> - bool rp_extensions;
> - u8 rp_log_size;
> -};
> -
> static const char * const rp_pio_error_string[] = {
> "Configuration Request received UR Completion", /* Bit Position 0 */
> "Configuration Request received CA Completion", /* Bit Position 1 */
> @@ -46,63 +39,42 @@ static const char * const rp_pio_error_string[] = {
> "Memory Request Completion Timeout", /* Bit Position 18 */
> };
>
> -static struct dpc_dev *to_dpc_dev(struct pci_dev *dev)
> -{
> - struct device *device;
> -
> - device = pcie_port_find_device(dev, PCIE_PORT_SERVICE_DPC);
> - if (!device)
> - return NULL;
> - return get_service_data(to_pcie_device(device));
> -}
> -
> void pci_save_dpc_state(struct pci_dev *dev)
> {
> - struct dpc_dev *dpc;
> struct pci_cap_saved_state *save_state;
> u16 *cap;
>
> if (!pci_is_pcie(dev))
> return;
>
> - dpc = to_dpc_dev(dev);
> - if (!dpc)
> - return;
> -
> save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_DPC);
> if (!save_state)
> return;
>
> cap = (u16 *)&save_state->cap.data[0];
> - pci_read_config_word(dev, dpc->cap_pos + PCI_EXP_DPC_CTL, cap);
> + pci_read_config_word(dev, dev->dpc_cap + PCI_EXP_DPC_CTL, cap);
> }
>
> void pci_restore_dpc_state(struct pci_dev *dev)
> {
> - struct dpc_dev *dpc;
> struct pci_cap_saved_state *save_state;
> u16 *cap;
>
> if (!pci_is_pcie(dev))
> return;
>
> - dpc = to_dpc_dev(dev);
> - if (!dpc)
> - return;
> -
> save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_DPC);
> if (!save_state)
> return;
>
> cap = (u16 *)&save_state->cap.data[0];
> - pci_write_config_word(dev, dpc->cap_pos + PCI_EXP_DPC_CTL, *cap);
> + pci_write_config_word(dev, dev->dpc_cap + PCI_EXP_DPC_CTL, *cap);
> }
>
> -static int dpc_wait_rp_inactive(struct dpc_dev *dpc)
> +static int dpc_wait_rp_inactive(struct pci_dev *pdev)
> {
> unsigned long timeout = jiffies + HZ;
> - struct pci_dev *pdev = dpc->dev->port;
> - u16 cap = dpc->cap_pos, status;
> + u16 cap = pdev->dpc_cap, status;
>
> pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
> while (status & PCI_EXP_DPC_RP_BUSY &&
> @@ -119,15 +91,13 @@ static int dpc_wait_rp_inactive(struct dpc_dev *dpc)
>
> static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
> {
> - struct dpc_dev *dpc;
> u16 cap;
>
> /*
> * DPC disables the Link automatically in hardware, so it has
> * already been reset by the time we get here.
> */
> - dpc = to_dpc_dev(pdev);
> - cap = dpc->cap_pos;
> + cap = pdev->dpc_cap;
>
> /*
> * Wait until the Link is inactive, then clear DPC Trigger Status
> @@ -135,7 +105,7 @@ static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
> */
> pcie_wait_for_link(pdev, false);
>
> - if (dpc->rp_extensions && dpc_wait_rp_inactive(dpc))
> + if (pdev->dpc_rp_extensions && dpc_wait_rp_inactive(pdev))
> return PCI_ERS_RESULT_DISCONNECT;
>
> pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
> @@ -147,10 +117,9 @@ static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
> return PCI_ERS_RESULT_RECOVERED;
> }
>
> -static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
> +static void dpc_process_rp_pio_error(struct pci_dev *pdev)
> {
> - struct pci_dev *pdev = dpc->dev->port;
> - u16 cap = dpc->cap_pos, dpc_status, first_error;
> + u16 cap = pdev->dpc_cap, dpc_status, first_error;
> u32 status, mask, sev, syserr, exc, dw0, dw1, dw2, dw3, log, prefix;
> int i;
>
> @@ -175,7 +144,7 @@ static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
> first_error == i ? " (First)" : "");
> }
>
> - if (dpc->rp_log_size < 4)
> + if (pdev->dpc_rp_log_size < 4)
> goto clear_status;
> pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_HEADER_LOG,
> &dw0);
> @@ -188,12 +157,12 @@ static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
> pci_err(pdev, "TLP Header: %#010x %#010x %#010x %#010x\n",
> dw0, dw1, dw2, dw3);
>
> - if (dpc->rp_log_size < 5)
> + if (pdev->dpc_rp_log_size < 5)
> goto clear_status;
> pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_IMPSPEC_LOG, &log);
> pci_err(pdev, "RP PIO ImpSpec Log %#010x\n", log);
>
> - for (i = 0; i < dpc->rp_log_size - 5; i++) {
> + for (i = 0; i < pdev->dpc_rp_log_size - 5; i++) {
> pci_read_config_dword(pdev,
> cap + PCI_EXP_DPC_RP_PIO_TLPPREFIX_LOG, &prefix);
> pci_err(pdev, "TLP Prefix Header: dw%d, %#010x\n", i, prefix);
> @@ -226,10 +195,9 @@ static int dpc_get_aer_uncorrect_severity(struct pci_dev *dev,
>
> static irqreturn_t dpc_handler(int irq, void *context)
> {
> + struct pci_dev *pdev = context;
> + u16 cap = pdev->dpc_cap, status, source, reason, ext_reason;
> struct aer_err_info info;
> - struct dpc_dev *dpc = context;
> - struct pci_dev *pdev = dpc->dev->port;
> - u16 cap = dpc->cap_pos, status, source, reason, ext_reason;
>
> pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
> pci_read_config_word(pdev, cap + PCI_EXP_DPC_SOURCE_ID, &source);
> @@ -248,8 +216,8 @@ static irqreturn_t dpc_handler(int irq, void *context)
> "reserved error");
>
> /* show RP PIO error detail information */
> - if (dpc->rp_extensions && reason == 3 && ext_reason == 0)
> - dpc_process_rp_pio_error(dpc);
> + if (pdev->dpc_rp_extensions && reason == 3 && ext_reason == 0)
> + dpc_process_rp_pio_error(pdev);
> else if (reason == 0 &&
> dpc_get_aer_uncorrect_severity(pdev, &info) &&
> aer_get_device_error_info(pdev, &info)) {
> @@ -266,9 +234,8 @@ static irqreturn_t dpc_handler(int irq, void *context)
>
> static irqreturn_t dpc_irq(int irq, void *context)
> {
> - struct dpc_dev *dpc = (struct dpc_dev *)context;
> - struct pci_dev *pdev = dpc->dev->port;
> - u16 cap = dpc->cap_pos, status;
> + struct pci_dev *pdev = context;
> + u16 cap = pdev->dpc_cap, status;
>
> pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
>
> @@ -285,7 +252,6 @@ static irqreturn_t dpc_irq(int irq, void *context)
> #define FLAG(x, y) (((x) & (y)) ? '+' : '-')
> static int dpc_probe(struct pcie_device *dev)
> {
> - struct dpc_dev *dpc;
> struct pci_dev *pdev = dev->port;
> struct device *device = &dev->device;
> int status;
> @@ -294,43 +260,37 @@ static int dpc_probe(struct pcie_device *dev)
> if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native)
> return -ENOTSUPP;
>
> - dpc = devm_kzalloc(device, sizeof(*dpc), GFP_KERNEL);
> - if (!dpc)
> - return -ENOMEM;
> -
> - dpc->cap_pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DPC);
> - dpc->dev = dev;
> - set_service_data(dev, dpc);
> + pdev->dpc_cap = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DPC);
>
> status = devm_request_threaded_irq(device, dev->irq, dpc_irq,
> dpc_handler, IRQF_SHARED,
> - "pcie-dpc", dpc);
> + "pcie-dpc", pdev);
> if (status) {
> pci_warn(pdev, "request IRQ%d failed: %d\n", dev->irq,
> status);
> return status;
> }
>
> - pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CAP, &cap);
> - pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, &ctl);
> + pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CAP, &cap);
> + pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl);
>
> - dpc->rp_extensions = (cap & PCI_EXP_DPC_CAP_RP_EXT);
> - if (dpc->rp_extensions) {
> - dpc->rp_log_size = (cap & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8;
> - if (dpc->rp_log_size < 4 || dpc->rp_log_size > 9) {
> + pdev->dpc_rp_extensions = (cap & PCI_EXP_DPC_CAP_RP_EXT) ? 1 : 0;
> + if (pdev->dpc_rp_extensions) {
> + pdev->dpc_rp_log_size = (cap & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8;
> + if (pdev->dpc_rp_log_size < 4 || pdev->dpc_rp_log_size > 9) {
> pci_err(pdev, "RP PIO log size %u is invalid\n",
> - dpc->rp_log_size);
> - dpc->rp_log_size = 0;
> + pdev->dpc_rp_log_size);
> + pdev->dpc_rp_log_size = 0;
> }
> }
>
> 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);
> + pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl);
>
> pci_info(pdev, "error containment capabilities: Int Msg #%d, RPExt%c PoisonedTLP%c SwTrigger%c RP PIO Log %d, DL_ActiveErr%c\n",
> cap & PCI_EXP_DPC_IRQ, FLAG(cap, PCI_EXP_DPC_CAP_RP_EXT),
> FLAG(cap, PCI_EXP_DPC_CAP_POISONED_TLP),
> - FLAG(cap, PCI_EXP_DPC_CAP_SW_TRIGGER), dpc->rp_log_size,
> + FLAG(cap, PCI_EXP_DPC_CAP_SW_TRIGGER), pdev->dpc_rp_log_size,
> FLAG(cap, PCI_EXP_DPC_CAP_DL_ACTIVE));
>
> pci_add_ext_cap_save_buffer(pdev, PCI_EXT_CAP_ID_DPC, sizeof(u16));
> @@ -339,13 +299,12 @@ static int dpc_probe(struct pcie_device *dev)
>
> static void dpc_remove(struct pcie_device *dev)
> {
> - struct dpc_dev *dpc = get_service_data(dev);
> struct pci_dev *pdev = dev->port;
> u16 ctl;
>
> - pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, &ctl);
> + pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl);
> 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);
> + pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl);
> }
>
> static struct pcie_port_service_driver dpcdriver = {
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 3840a541a9de..a0b7e7a53741 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -444,6 +444,11 @@ struct pci_dev {
> const struct attribute_group **msi_irq_groups;
> #endif
> struct pci_vpd *vpd;
> +#ifdef CONFIG_PCIE_DPC
> + u16 dpc_cap;
> + unsigned int dpc_rp_extensions:1;
> + u8 dpc_rp_log_size;
> +#endif
> #ifdef CONFIG_PCI_ATS
> union {
> struct pci_sriov *sriov; /* PF: SR-IOV info */
>
--
Sathyanarayanan Kuppuswamy
Linux kernel developer
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v15 4/5] PCI/DPC: Add Error Disconnect Recover (EDR) support
2020-02-26 18:42 ` Kuppuswamy Sathyanarayanan
@ 2020-02-26 21:32 ` Bjorn Helgaas
2020-02-26 22:11 ` Kuppuswamy Sathyanarayanan
0 siblings, 1 reply; 18+ messages in thread
From: Bjorn Helgaas @ 2020-02-26 21:32 UTC (permalink / raw)
To: Kuppuswamy Sathyanarayanan; +Cc: linux-pci, linux-kernel, ashok.raj
On Wed, Feb 26, 2020 at 10:42:27AM -0800, Kuppuswamy Sathyanarayanan wrote:
> On 2/25/20 5:02 PM, Bjorn Helgaas wrote:
> > On Thu, Feb 13, 2020 at 10:20:16AM -0800, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > ...
> > > +static void edr_handle_event(acpi_handle handle, u32 event, void *data)
> > > +{
> > > + struct dpc_dev *dpc = data, ndpc;
> > There's actually very little use of struct dpc_dev in this file. I
> > bet with a little elbow grease, we could remove it completely and just
> > use the pci_dev * or maybe an opaque pointer.
> Yes, we could remove it. But it might need some more changes to
> dpc driver functions. I can think of two ways,
>
> 1. Re-factor the DPC driver not to use dpc_dev structure and just use
> pci_dev in their functions implementation. But this might lead to
> re-reading following dpc_dev structure members every time we
> use it in dpc driver functions.
>
> (Currently in dpc driver probe they cache the following device parameters )
>
> 9 u16 cap_pos;
> 10 bool rp_extensions;
> 11 u8 rp_log_size;
> 12 u16 ctl;
> 13 u16 cap;
I think this is basically what I proposed with the sample patch in my
response to your 3/5 patch. But I don't see the ctl/cap part, so
maybe I missed something.
> 2. We can create a new variant of dpc_process_err() which depends on pci_dev
> structure and move the dpc_dev initialization to it. Downside is, we should
> do this
> initialization every time we get DPC event (which should be rare).
>
> void dpc_process_error(struct pci_dev *pdev)
> {
> struct dpc_dev dpc;
> dpc_dev_init(pdev, &dpc);
>
> ....
>
> }
>
> Let me know your comments.
>
> >
> > > + struct pci_dev *pdev = dpc->pdev;
> > > + pci_ers_result_t estate = PCI_ERS_RESULT_DISCONNECT;
> > > + u16 status;
> > > +
> > > + pci_info(pdev, "ACPI event %#x received\n", event);
> > > +
> > > + if (event != ACPI_NOTIFY_DISCONNECT_RECOVER)
> > > + return;
> > > +
> > > + /*
> > > + * Check if _DSM(0xD) is available, and if present locate the
> > > + * port which issued EDR event.
> > > + */
> > > + pdev = acpi_locate_dpc_port(pdev);
> > This function name should include "get" since it's part of the
> > pci_dev_get()/pci_dev_put() sequence.
> How about acpi_dpc_port_get(pdev) ?
OK.
> > > + if (!pdev) {
> > > + pci_err(dpc->pdev, "No valid port found\n");
This message should be expanded somehow. I think the point is that we
got an EDR notification, but firmware couldn't tell us where the
containment event occurred. Should that ever happen? Or is it a
firmware defect if it *does* happen?
In any event, I think the message should say something like "Can't
identify source of EDR notification".
> > > + return;
> > > + }
> > > +
> > > + if (pdev != dpc->pdev) {
> > > + pci_warn(pdev, "Initializing dpc again\n");
> > > + dpc_dev_init(pdev, &ndpc);
> > This seems... I'm not sure what. I guess it's really just reading
> > the DPC capability for use by dpc_process_error(), so maybe it's OK.
> > But it's a little strange to read.
I *think* maybe if we move the DPC info into the struct pci_dev it
will solve this issue too? I.e., we won't have a struct dpc_dev, so
we won't have this funny-looking dpc_dev_init().
> > Is this something we should be warning about?
> No this is a valid case. it will only happen if we have a non-acpi
> based switch attached to root port.
I agree this is a valid case (as I mentioned below). My point was
just that if it is a valid case, we might not want to use pci_warn()
here. Maybe pci_info() if you think it's important, or maybe no
message at all. I don't think "Initializing dpc again" is going to be
useful to a user.
> > I think the ECR says
> > it's legitimate to return a child device, doesn't it?
> > > + * TODO: Remove dependency on ACPI FIRMWARE_FIRST bit to
> > > + * determine ownership of DPC between firmware or OS.
> > Extend the comment to say how we *should* determine ownership.
> Yes, ownership should be based on _OSC negotiation. I will add necessary
> comments here.
Why are we not doing this via _OSC negotiation in this series? It
would be much better if we could just do it instead of adding a
comment that we *should* do it. Nobody knows more about this than you
do, so probably nobody else is going to come along and finish this
up :)
> > > + dpc = devm_kzalloc(&pdev->dev, sizeof(*dpc), GFP_KERNEL);
> > This kzalloc should be in dpc.c, not here.
> >
> > And I don't see a corresponding free.
> It will be freed when removing the pdev right ? Do you want to free it
> explicitly in this driver ?
Nope, you're right. I always forget about the devm magic, sorry.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v15 3/5] PCI/EDR: Export AER, DPC and error recovery functions
2020-02-26 21:26 ` Kuppuswamy Sathyanarayanan
@ 2020-02-26 21:48 ` Bjorn Helgaas
0 siblings, 0 replies; 18+ messages in thread
From: Bjorn Helgaas @ 2020-02-26 21:48 UTC (permalink / raw)
To: Kuppuswamy Sathyanarayanan; +Cc: linux-pci, linux-kernel, ashok.raj
On Wed, Feb 26, 2020 at 01:26:09PM -0800, Kuppuswamy Sathyanarayanan wrote:
> On 2/26/20 1:13 PM, Bjorn Helgaas wrote:
> > On Wed, Feb 26, 2020 at 11:30:43AM -0800, Kuppuswamy Sathyanarayanan wrote:
> > > On 2/25/20 5:02 PM, Bjorn Helgaas wrote:
> > > > On Thu, Feb 13, 2020 at 10:20:15AM -0800, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> > > > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > > > ...
> > > > > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > > > > index 99fca8400956..acae12dbf9ff 100644
> > > > > --- a/drivers/pci/pcie/dpc.c
> > > > > +++ b/drivers/pci/pcie/dpc.c
> > > > > @@ -15,15 +15,9 @@
> > > > > #include <linux/pci.h>
> > > > > #include "portdrv.h"
> > > > > +#include "dpc.h"
> > > > > #include "../pci.h"
> > > > > -struct dpc_dev {
> > > > > - struct pci_dev *pdev;
> > > > > - u16 cap_pos;
> > > > > - bool rp_extensions;
> > > > > - u8 rp_log_size;
> > > > > -};
> > > > Adding dpc.h shouldn't be in this patch because there's no need for a
> > > > separate dpc.h file yet -- it's only included this one place. I'm not
> > > > positive a dpc.h is needed at all -- see comments on patch [4/5].
> > > Yes, if we use pdev in dpc functions used by EDR code, we should
> > > not need it.
> > I think struct dpc_dev might be more trouble than it's worth. I
> > attached a possible patch at the end that folds the 25 bits of
> > DPC-related info into the struct pci_dev and gets rid of struct
> > dpc_dev completely. I compiled it but haven't tested it.
> That would solve most of my export issues. Do you want me
> to add it to my patch list or merge it separately.
If you want to use that, I would merge it first, followed by the error
clearing cleanup, followed by the other EDR stuff.
Bjorn
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v15 4/5] PCI/DPC: Add Error Disconnect Recover (EDR) support
2020-02-26 21:32 ` Bjorn Helgaas
@ 2020-02-26 22:11 ` Kuppuswamy Sathyanarayanan
2020-02-26 22:41 ` Bjorn Helgaas
0 siblings, 1 reply; 18+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2020-02-26 22:11 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-pci, linux-kernel, ashok.raj
On 2/26/20 1:32 PM, Bjorn Helgaas wrote:
> On Wed, Feb 26, 2020 at 10:42:27AM -0800, Kuppuswamy Sathyanarayanan wrote:
>> On 2/25/20 5:02 PM, Bjorn Helgaas wrote:
>>> On Thu, Feb 13, 2020 at 10:20:16AM -0800, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>>>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>> ...
>
>> Yes, we could remove it. But it might need some more changes to
>> dpc driver functions. I can think of two ways,
>>
>> 1. Re-factor the DPC driver not to use dpc_dev structure and just use
>> pci_dev in their functions implementation. But this might lead to
>> re-reading following dpc_dev structure members every time we
>> use it in dpc driver functions.
>>
>> (Currently in dpc driver probe they cache the following device parameters )
>>
>> 9 u16 cap_pos;
>> 10 bool rp_extensions;
>> 11 u8 rp_log_size;
>> 12 u16 ctl;
>> 13 u16 cap;
> I think this is basically what I proposed with the sample patch in my
> response to your 3/5 patch. But I don't see the ctl/cap part, so
> maybe I missed something.
if its costly to carry it in pci_dev, we can always re-read them.
if its ok to use pci_dev, If you want, I can extend your patch to
include the cap and ctl.
> This message should be expanded somehow. I think the point is that we
> got an EDR notification, but firmware couldn't tell us where the
> containment event occurred. Should that ever happen? Or is it a
> firmware defect if it *does* happen?
Yes, if we hit this error then its a firmware defect. Either
firmware sent wrong BDF value or used invalid return type.
I was planning to add some extra error info in acpi_locate_dpc_port()
166 + if (obj->type != ACPI_TYPE_INTEGER) {
167 + ACPI_FREE(obj);
168 + return NULL;
169 + }
>
> In any event, I think the message should say something like "Can't
> identify source of EDR notification".
I will use your suggestion here along with above mentioned change.
>
>>> This seems... I'm not sure what. I guess it's really just reading
>>> the DPC capability for use by dpc_process_error(), so maybe it's OK.
>>> But it's a little strange to read.
> I *think* maybe if we move the DPC info into the struct pci_dev it
> will solve this issue too? I.e., we won't have a struct dpc_dev, so
> we won't have this funny-looking dpc_dev_init().
Yes, your patch will resolve this issue.
>
>> No this is a valid case. it will only happen if we have a non-acpi
>> based switch attached to root port.
> I agree this is a valid case (as I mentioned below). My point was
> just that if it is a valid case, we might not want to use pci_warn()
> here. Maybe pci_info() if you think it's important, or maybe no
> message at all. I don't think "Initializing dpc again" is going to be
> useful to a user.
Got it. Adding pci_info here will be helpful to understand the flow.
Since EDR is a rare case, it should not pollute the dmesg. So I will add it.
>
>> Yes, ownership should be based on _OSC negotiation. I will add necessary
>> comments here.
> Why are we not doing this via _OSC negotiation in this series? It
> would be much better if we could just do it instead of adding a
> comment that we *should* do it. Nobody knows more about this than you
> do, so probably nobody else is going to come along and finish this
> up :)
Actually Alex G already proposed a patch to fix it.
https://lkml.org/lkml/2018/11/16/202
But that discussion never reached a conclusion. Since a proper fix
for it would affect some legacy hardwares which solely relies on
HEST tables, it did not make everyone happy. So it might take a
lot to convince all the stake holders to merge such patch. So its
better not to mix both of these patch sets together.
Once this patch set is done, If Alex G is no longer working on it,
I can work on it.
>
--
Sathyanarayanan Kuppuswamy
Linux kernel developer
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v15 4/5] PCI/DPC: Add Error Disconnect Recover (EDR) support
2020-02-26 22:11 ` Kuppuswamy Sathyanarayanan
@ 2020-02-26 22:41 ` Bjorn Helgaas
0 siblings, 0 replies; 18+ messages in thread
From: Bjorn Helgaas @ 2020-02-26 22:41 UTC (permalink / raw)
To: Kuppuswamy Sathyanarayanan; +Cc: linux-pci, linux-kernel, ashok.raj
On Wed, Feb 26, 2020 at 02:11:53PM -0800, Kuppuswamy Sathyanarayanan wrote:
> On 2/26/20 1:32 PM, Bjorn Helgaas wrote:
> > On Wed, Feb 26, 2020 at 10:42:27AM -0800, Kuppuswamy Sathyanarayanan wrote:
> > > On 2/25/20 5:02 PM, Bjorn Helgaas wrote:
> > > > On Thu, Feb 13, 2020 at 10:20:16AM -0800, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> > > > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > > > ...
> >
> > > Yes, we could remove it. But it might need some more changes to
> > > dpc driver functions. I can think of two ways,
> > >
> > > 1. Re-factor the DPC driver not to use dpc_dev structure and just use
> > > pci_dev in their functions implementation. But this might lead to
> > > re-reading following dpc_dev structure members every time we
> > > use it in dpc driver functions.
> > >
> > > (Currently in dpc driver probe they cache the following device parameters )
> > >
> > > 9 u16 cap_pos;
> > > 10 bool rp_extensions;
> > > 11 u8 rp_log_size;
> > > 12 u16 ctl;
> > > 13 u16 cap;
> > I think this is basically what I proposed with the sample patch in my
> > response to your 3/5 patch. But I don't see the ctl/cap part, so
> > maybe I missed something.
> if its costly to carry it in pci_dev, we can always re-read them.
> if its ok to use pci_dev, If you want, I can extend your patch to
> include the cap and ctl.
Ah, I see, you added cap & ctl as part of your series. But I don't
think they're ever used after dpc_probe(), are they? So I don't think
they need to be saved at all.
> > > Yes, ownership should be based on _OSC negotiation. I will add necessary
> > > comments here.
> > Why are we not doing this via _OSC negotiation in this series? It
> > would be much better if we could just do it instead of adding a
> > comment that we *should* do it. Nobody knows more about this than you
> > do, so probably nobody else is going to come along and finish this
> > up :)
> Actually Alex G already proposed a patch to fix it.
>
> https://lkml.org/lkml/2018/11/16/202
>
> But that discussion never reached a conclusion. Since a proper fix
> for it would affect some legacy hardwares which solely relies on
> HEST tables, it did not make everyone happy. So it might take a
> lot to convince all the stake holders to merge such patch. So its
> better not to mix both of these patch sets together.
>
> Once this patch set is done, If Alex G is no longer working on it,
> I can work on it.
OK, great, maybe we can make progress on that later.
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-02-26 22:41 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-13 18:20 [PATCH v15 0/5] Add Error Disconnect Recover (EDR) support sathyanarayanan.kuppuswamy
2020-02-13 18:20 ` [PATCH v15 1/5] PCI/ERR: Update error status after reset_link() sathyanarayanan.kuppuswamy
2020-02-26 13:41 ` Bjorn Helgaas
2020-02-13 18:20 ` [PATCH v15 2/5] PCI/DPC: Remove pcie_device reference from dpc_dev structure sathyanarayanan.kuppuswamy
2020-02-26 13:43 ` Bjorn Helgaas
2020-02-13 18:20 ` [PATCH v15 3/5] PCI/EDR: Export AER, DPC and error recovery functions sathyanarayanan.kuppuswamy
2020-02-26 1:02 ` Bjorn Helgaas
2020-02-26 19:30 ` Kuppuswamy Sathyanarayanan
2020-02-26 21:13 ` Bjorn Helgaas
2020-02-26 21:26 ` Kuppuswamy Sathyanarayanan
2020-02-26 21:48 ` Bjorn Helgaas
2020-02-13 18:20 ` [PATCH v15 4/5] PCI/DPC: Add Error Disconnect Recover (EDR) support sathyanarayanan.kuppuswamy
2020-02-26 1:02 ` Bjorn Helgaas
2020-02-26 18:42 ` Kuppuswamy Sathyanarayanan
2020-02-26 21:32 ` Bjorn Helgaas
2020-02-26 22:11 ` Kuppuswamy Sathyanarayanan
2020-02-26 22:41 ` Bjorn Helgaas
2020-02-13 18:20 ` [PATCH v15 5/5] PCI/ACPI: Enable EDR support sathyanarayanan.kuppuswamy
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).