Add Error Disconnect Recover (EDR) support.
Subsystems (3)
v2 cover Add Error Disconnect Recover (EDR) support.
From: Kuppuswamy Sathyanarayanan <[email protected]>
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 v1:
* Rebased on top of v5.1-rc1
Kuppuswamy Sathyanarayanan (2):
PCI/ACPI: Add _OSC based negotiation support for DPC PCI/DPC: Add Error Disconnect Recover (EDR) support
drivers/acpi/pci_root.c | 6 + drivers/pci/pcie/Kconfig | 10 + drivers/pci/pcie/dpc.c | 326 ++++++++++++++++++++++++++++++-- drivers/pci/pcie/portdrv_core.c | 8 +- drivers/pci/probe.c | 1 + include/linux/acpi.h | 3 +- include/linux/pci.h | 1 + 7 files changed, 339 insertions(+), 16 deletions(-)
Signature · 1 line
2.20.1
PATCH v2 1/2 PCI/ACPI: Add _OSC based negotiation support for DPC
From: Kuppuswamy Sathyanarayanan <[email protected]>
As per PCI Firmware Specification, r3.2 ECN
(https://members.pcisig.com/wg/PCI-SIG/document/12614), 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.
--- drivers/acpi/pci_root.c | 6 ++++++ drivers/pci/pcie/portdrv_core.c | 3 ++- drivers/pci/probe.c | 1 + include/linux/acpi.h | 3 ++- include/linux/pci.h | 1 + 5 files changed, 12 insertions(+), 2 deletions(-)
drivers/acpi/pci_root.c +6 -0
@@ -154,6 +154,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,
@@ -499,6 +500,9 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm, control |=OSC_PCI_EXPRESS_AER_CONTROL ; } +if (IS_ENABLED (CONFIG_PCIE_DPC )) + control |=OSC_PCI_EXPRESS_DPC_CONTROL ; + requested = control; status =acpi_pci_osc_control_set (handle, &control,OSC_PCI_EXPRESS_CAPABILITY_CONTROL );
@@ -927,6 +931,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 ;pci_scan_child_bus (bus); pci_set_host_bridge_release(host_bridge, acpi_pci_root_release_info,
drivers/pci/pcie/portdrv_core.c +2 -1
@@ -250,7 +250,8 @@ static int get_port_device_capability(struct pci_dev *dev) }if (pci_find_ext_capability (dev,PCI_EXT_CAP_ID_DPC ) && -pci_aer_available () && services &PCIE_PORT_SERVICE_AER ) +pci_aer_available () && services &PCIE_PORT_SERVICE_AER && + (pcie_ports_native || host->native_dpc)) services |=PCIE_PORT_SERVICE_DPC ; if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
drivers/pci/probe.c +1 -0
@@ -608,6 +608,7 @@ struct pci_host_bridge *pci_alloc_host_bridge(size_t priv) bridge->native_shpc_hotplug =1 ; bridge->native_pme =1 ; bridge->native_ltr =1 ; + bridge->native_dpc =1 ;return bridge; }
include/linux/acpi.h +2 -1
@@ -522,7 +522,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 0x000000ff#define ACPI_GSB_ACCESS_ATTRIB_QUICK 0x00000002#define ACPI_GSB_ACCESS_ATTRIB_SEND_RCV 0x00000004
include/linux/pci.h +1 -0
@@ -502,6 +502,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 *//* Resource alignment requirements */ resource_size_t (*align_resource)(struct pci_dev *dev, const struct resource *res,
Signature · 1 line
2.20.1
PATCH v2 2/2 PCI/DPC: Add Error Disconnect Recover (EDR) support
In reply to [email protected]From: Kuppuswamy Sathyanarayanan <[email protected]>
As per PCI firmware specification v3.2 ECN
(https://members.pcisig.com/wg/PCI-SIG/document/12614), 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 let firmware know the recovery status via _OST ACPI call.
Since EDR is a hybrid service, DPC service shall be enabled in OS even if AER is not natively enabled in OS.
--- drivers/pci/pcie/Kconfig | 10 + drivers/pci/pcie/dpc.c | 326 ++++++++++++++++++++++++++++++-- drivers/pci/pcie/portdrv_core.c | 9 +- 3 files changed, 329 insertions(+), 16 deletions(-)
drivers/pci/pcie/Kconfig +10 -0
@@ -142,3 +142,13 @@ config PCIE_PTM This is only useful if you have devices that support PTM, but it is safe to enable even if you don't. + +config PCIE_EDR + bool "PCI Express Error Disconnect Recover support" + default n + depends on PCIE_DPC && ACPI + help + This options adds Error Disconnect Recover support as specified + in PCI firmware specification v3.2 Downstream Port Containment + Related Enhancements ECN. Enable this if you want to support hybrid + DPC model which uses both firmware and OS to implement DPC.
drivers/pci/pcie/dpc.c +312 -14
405 lines not on this page · raw message
drivers/pci/pcie/portdrv_core.c +7 -2
@@ -249,9 +249,14 @@ static int get_port_device_capability(struct pci_dev *dev)pcie_pme_interrupt_enable (dev, false); } +/* +* If EDR support is enabled in OS, then even if AER is not handled in +* OS, DPC service can be enabled. +*/ if (pci_find_ext_capability (dev,PCI_EXT_CAP_ID_DPC ) && -pci_aer_available () && services &PCIE_PORT_SERVICE_AER && - (pcie_ports_native || host->native_dpc)) + ((IS_ENABLED (CONFIG_PCIE_EDR ) && !host->native_dpc) || + (pci_aer_available () && services &PCIE_PORT_SERVICE_AER && + (pcie_ports_native || host->native_dpc)))) services |=PCIE_PORT_SERVICE_DPC ; if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
Signature · 1 line
2.20.1
Re: [PATCH v2 2/2] PCI/DPC: Add Error Disconnect Recover (EDR) support
Quoted 10 lines · On Tue, Mar 19, 2019 at 01:47:29PM -0700…
From: Kuppuswamy Sathyanarayanan <[email protected]>
As per PCI firmware specification v3.2 ECN
(https://members.pcisig.com/wg/PCI-SIG/document/12614), 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 let firmware know the recovery status via_OSTACPI call.
Quoted 7 lines
Since EDR is a hybrid service, DPC service shall be enabled in OS even if AER is not natively enabled in OS.
Signed-off-by Kuppuswamy Sathyanarayanan <[email protected]>--- drivers/pci/pcie/Kconfig | 10 + drivers/pci/pcie/dpc.c | 326 ++++++++++++++++++++++++++++++--
I'll be looking for Keith's reviewed-by for this eventually.
It looks like this could be split into some smaller patches:
- Add dpc_dev.native_dpc (hopefully we can find a less confusing name)
- Convert dpc_handler() to dpc_handler() + dpc_process_error()
- Add EDR support
Quoted 34 lines
drivers/pci/pcie/portdrv_core.c | 9 +- 3 files changed, 329 insertions(+), 16 deletions(-)drivers/pci/pcie/Kconfig +10
-0@@ -142,3 +142,13 @@ config PCIE_PTM This is only useful if you have devices that support PTM, but it is safe to enable even if you don't. + +config PCIE_EDR + bool "PCI Express Error Disconnect Recover support" + default n + depends on PCIE_DPC && ACPI + help + This options adds Error Disconnect Recover support as specified + in PCI firmware specification v3.2 Downstream Port Containment + Related Enhancements ECN. Enable this if you want to support hybrid + DPC model which uses both firmware and OS to implement DPC.drivers/pci/pcie/dpc.c +1
-0@@ -11,6 +11,7 @@#include <linux/interrupt.h> #include <linux/init.h> #include <linux/pci.h> +#include <linux/acpi.h> #include "portdrv.h" #include "../pci.h"
drivers/pci/pcie/dpc.c struct dpc_dev { · line 24
Quoted 4 lines
drivers/pci/pcie/dpc.c +1
-0@@ -20,8 +21,23 @@ struct dpc_dev {u16 cap_pos;bool rp_extensions;u8 rp_log_size; +bool native_dpc;
This is going to be way too confusing with a "native_dpc" in both the struct pci_host_bridge and the struct dpc_dev.
Quoted 66 lines
drivers/pci/pcie/dpc.c +28
-2+pci_ers_result_t error_state; +#ifdef CONFIG_ACPI +struct acpi_device *adev; +#endif }; +#ifdef CONFIG_ACPI + +#define EDR_PORT_ENABLE_DSM 0x0C +#define EDR_PORT_LOCATE_DSM 0x0D + +static const guid_t pci_acpi_dsm_guid = +GUID_INIT (0xe5c937d0 ,0x3553 ,0x4d7a , +0x91 ,0x17 ,0xea ,0x4d ,0x19 ,0xc3 ,0x43 ,0x4d ); +#endif +static const char *const rp_pio_error_string[] = {"Configuration Request received UR Completion" ,/* Bit Position 0 */ "Configuration Request received CA Completion" ,/* Bit Position 1 */ @@ -67,6 +83,9 @@ void pci_save_dpc_state(struct pci_dev *dev)if (!dpc)return ; +if (!dpc->native_dpc) +return ; + save_state =pci_find_saved_ext_cap (dev,PCI_EXT_CAP_ID_DPC );if (!save_state)return ;@@ -88,6 +107,9 @@ void pci_restore_dpc_state(struct pci_dev *dev)if (!dpc)return ; +if (!dpc->native_dpc) +return ; + save_state =pci_find_saved_ext_cap (dev,PCI_EXT_CAP_ID_DPC );if (!save_state)return ;@@ -224,10 +246,9 @@ static int dpc_get_aer_uncorrect_severity(struct pci_dev *dev,return 1 ; } -static irqreturn_t dpc_handler (int irq,void *context) +static void dpc_process_error (struct dpc_dev *dpc) {struct aer_err_info info; -struct dpc_dev *dpc = context;struct pci_dev *pdev = dpc->dev->port;struct device *dev = &dpc->dev->device;u16 cap = dpc->cap_pos, status, source, reason, ext_reason;@@ -261,6 +282,13 @@ static irqreturn_t dpc_handler(int irq, void *context)/* We configure DPC so it only triggers on ERR_FATAL */ pcie_do_recovery (pdev, pci_channel_io_frozen,PCIE_PORT_SERVICE_DPC ); +} + +static irqreturn_t dpc_handler (int irq,void *context) +{ +struct dpc_dev *dpc = context; + +dpc_process_error (dpc);return IRQ_HANDLED ; }
drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 314
Quoted 4 lines
drivers/pci/pcie/dpc.c +1
-0@@ -283,6 +311,230 @@ static irqreturn_t dpc_irq(int irq, void *context)return IRQ_HANDLED ; } +void dpc_error_resume (struct pci_dev *dev)
Looks like this should be static?
drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 320
Quoted 6 lines
drivers/pci/pcie/dpc.c +6
-0+{ +struct dpc_dev *dpc; + + dpc =to_dpc_dev (dev); +if (!dpc) +return ;
I don't think it's possible for dpc to be NULL, is it? Remove the test if not.
Quoted 11 lines
drivers/pci/pcie/dpc.c +11
-0+ dpc->error_state =PCI_ERS_RESULT_RECOVERED ; +} + +#ifdef CONFIG_ACPI + +/* +* _DSM wrapper function to enable/disable DPC port. +* @dpc : DPC device structure +* @enable: status of DPC port (0 or 1). +* +* returns 0 on success or errno on failure.
drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 342
Quoted 10 lines
drivers/pci/pcie/dpc.c +10
-0+*/ +static int acpi_enable_dpc_port (struct dpc_dev *dpc,bool enable) +{ +union acpi_object *obj; +int status; +union acpi_object argv4; + +/* Check whether EDR_PORT_ENABLE_DSM is supported in firmware */ + status =acpi_check_dsm (dpc->adev->handle, &pci_acpi_dsm_guid,1 , +1 <<EDR_PORT_ENABLE_DSM );
I don't think this acpi_check_dsm() is necessary, is it? I expect acpi_evaluate_dsm() to fail nicely if there's no _DSM or it doesn't support the function.
Quoted 26 lines
drivers/pci/pcie/dpc.c +26
-0+if (!status) +return -ENOTSUPP ; + + argv4.type =ACPI_TYPE_INTEGER ; + argv4.integer.value = enable; + + obj =acpi_evaluate_dsm (dpc->adev->handle, &pci_acpi_dsm_guid,1 , +EDR_PORT_ENABLE_DSM , &argv4); +if (!obj) +return -EIO ; + +if (obj->type ==ACPI_TYPE_INTEGER && obj->integer.value == enable) + status =0 ; +else + status = -EIO ; + +ACPI_FREE (obj); + +return status; +} + +/* +* _DSM wrapper function to locate DPC port. +* @dpc : DPC device structure +* +* returns pci_dev or NULL.
drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 378
Quoted 10 lines
drivers/pci/pcie/dpc.c +10
-0+*/ +static struct pci_dev *acpi_locate_dpc_port (struct dpc_dev *dpc) +{ +union acpi_object *obj; +int status; +u16 port; + +/* Check whether EDR_PORT_LOCATE_DSM is supported in firmware */ + status =acpi_check_dsm (dpc->adev->handle, &pci_acpi_dsm_guid,1 , +1 <<EDR_PORT_LOCATE_DSM );
Unnecessary?
drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 379
Quoted 2 lines
drivers/pci/pcie/dpc.c +2
-0+if (!status) +return dpc->dev->port;
If you *do* need the acpi_check_dsm(), I would have expected to return NULL in this error case?
drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 381
Quoted 2 lines
drivers/pci/pcie/dpc.c +2
-0+ +
Extra blank line here.
Quoted 17 lines
drivers/pci/pcie/dpc.c +17
-0+ obj =acpi_evaluate_dsm (dpc->adev->handle, &pci_acpi_dsm_guid,1 , +EDR_PORT_LOCATE_DSM ,NULL ); +if (!obj) +return NULL ; + +if (obj->type ==ACPI_TYPE_INTEGER ) { +/* +* 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); + }else { +ACPI_FREE (obj); +return NULL ;
drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 409
Quoted 10 lines
drivers/pci/pcie/dpc.c +10
-0+ } + +return pci_get_domain_bus_and_slot (0 ,PCI_BUS_NUM (port), port &0xff ); +} + +/* +* _OST wrapper function to let firmware know the status of EDR event. +* @dpc : DPC device structure. +* @status: Status of EDR event. +*
Spurious blank line in the comment.
drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 422
Quoted 13 lines
drivers/pci/pcie/dpc.c +13
-0+*/ +static int acpi_send_edr_status (struct dpc_dev *dpc,u16 status) +{ +u32 ost_status; +struct pci_dev *pdev = dpc->dev->port; + +dev_dbg (&pdev->dev,"Sending EDR status :%x\n" , status); + + ost_status =PCI_DEVID (pdev->bus->number, pdev->devfn); + ost_status = (ost_status <<16 ) | status; + +if (!acpi_has_method (dpc->adev->handle,"_OST" )) +return -ENOTSUPP ;
This acpi_has_method() check is superfluous, isn't it? I would expect acpi_evaluate_ost() to fail gracefully if there's no _OST.
I do see several other instances of code of the form:
if (!acpi_has_method(handle, "XXXX"))
return false;
status = acpi_evaluate_*(handle, "XXXX", ...);
But I think that's a pointless pattern. I think we could just try to evaluate the method directly, since the evaluation will fail if the method doesn't exist:
status = acpi_evaluate_*(handle, "XXXX", ...);
Quoted 40 lines
drivers/pci/pcie/dpc.c +40
-0+ status =acpi_evaluate_ost (dpc->adev->handle, +ACPI_NOTIFY_DISCONNECT_RECOVER , + ost_status,NULL ); +if (ACPI_FAILURE (status)) +return -EINVAL ; + +return 0 ; +} + +/* +* Helper function used for disconnecting the child devices when EDR event is +* received from firmware. +*/ +static void dpc_disconnect_devices (struct pci_dev *dev) +{ +struct pci_dev *udev; +struct pci_bus *parent; +struct pci_dev *pdev, *temp; + +dev_dbg (&dev->dev,"Disconnecting the child devices\n" ); + +if (dev->hdr_type ==PCI_HEADER_TYPE_BRIDGE ) + udev = dev; +else + udev = dev->bus->self; + + parent = udev->subordinate; +pci_walk_bus (parent, pci_dev_set_disconnected,NULL ); + +pci_lock_rescan_remove (); +pci_dev_get (dev); +list_for_each_entry_safe_reverse (pdev, temp, &parent->devices, + bus_list) { +pci_stop_and_remove_bus_device (pdev); + } +pci_dev_put (dev); +pci_unlock_rescan_remove (); +} + +static void edr_handle_event (acpi_handle handle,u32 event,void *data)
drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 473
Quoted 10 lines
drivers/pci/pcie/dpc.c +10
-0+{ +struct dpc_dev *dpc = (struct dpc_dev *) data; +struct pci_dev *pdev; +u16 status, cap; + +if (event !=ACPI_NOTIFY_DISCONNECT_RECOVER ) +return ; + +if (!data) { +pr_err ("Invalid EDR event\n" );
In what instance can "data" be NULL? I think *never*, unless ACPI screwed up and lost the context you supplied to acpi_install_notify_handler(). In that case, we should do something more significant than print a message and just return. I think we should just omit this test, and if ACPI screwed up, we'll take a null pointer dereference oops, we'll see exactly where, and we'll have a chance to fix the problem.
drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 477
Quoted 4 lines
drivers/pci/pcie/dpc.c +4
-0+return ; + } + +dev_dbg (&dpc->dev->port->dev,"Valid EDR event received\n" );
Set "pdev" at declaration and use it in these printks.
drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 483
Quoted 6 lines
drivers/pci/pcie/dpc.c +6
-0+ +/* +* Check if _DSM(0xD) is available, and if present locate the +* port which issued EDR event. +*/ + pdev =acpi_locate_dpc_port (dpc);
Can this be done at probe-time instead of event-time?
drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 494
Quoted 11 lines
drivers/pci/pcie/dpc.c +11
-0+if (!pdev) { +dev_err (&dpc->dev->port->dev,"No valid port found\n" ); +return ; + } + +/* +* Get DPC priv data for given pdev +*/ + dpc =to_dpc_dev (pdev); + dpc->error_state =PCI_ERS_RESULT_DISCONNECT ; + pdev = dpc->dev->port;
It's quite confusing to reassign pdev here. The comment about "Get DPC priv data for given pdev" is really superfluous since that much is obvious from the code. What's *not* obvious is whether this "pdev = dpc->dev->port" changes anything and if so, why.
drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 505
Quoted 11 lines
drivers/pci/pcie/dpc.c +11
-0+ cap = dpc->cap_pos; + +/* +* Check if the port supports DPC: +* +* if port does not support DPC, then let firmware handle +* the error recovery and OS is responsible for cleaning +* up the child devices. +* +* if port supports DPC, then fall back to default error +* recovery.
Capitalize first letter of sentences.
drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 512
Quoted 7 lines
drivers/pci/pcie/dpc.c +7
-0+* +*/ +if (cap) { +/* Check if there is a valid DPC trigger */ +pci_read_config_word (pdev, cap +PCI_EXP_DPC_STATUS , &status); +if (!(status &PCI_EXP_DPC_STATUS_TRIGGER )) { +dev_err (&pdev->dev,"Invalid DPC trigger\n" );
Include "status" value in the printk.
Quoted 71 lines
drivers/pci/pcie/dpc.c +46
-10+ return; + } +dpc_process_error (dpc); + } + +if (dpc->error_state ==PCI_ERS_RESULT_RECOVERED ) { +/* +* Recovery is successful, so send +* _OST(0xF, BDF << 16 | 0x80, "") to firmware. +*/ + status =0x80 ; + }else { +/* +* Recovery is not successful, so disconnect child devices +* and send _OST(0xF, BDF << 16 | 0x81, "") to firmware. +*/ +dpc_disconnect_devices (pdev); + status =0x81 ; + } + +acpi_send_edr_status (dpc, status); +} + +#endif +#define FLAG (x, y) (((x) & (y)) ? '+' : '-')static int dpc_probe (struct pcie_device *dev) {@@ -291,9 +543,10 @@ static int dpc_probe(struct pcie_device *dev)struct device *device = &dev->device;int status;u16 ctl, cap; - -if (pcie_aer_get_firmware_first (pdev)) -return -ENOTSUPP ; +#ifdef CONFIG_ACPI +struct acpi_device *adev =ACPI_COMPANION (&pdev->dev); +acpi_status astatus; +#endif dpc =devm_kzalloc (device,sizeof (*dpc),GFP_KERNEL );if (!dpc)@@ -302,16 +555,53 @@ static int dpc_probe(struct pcie_device *dev) dpc->cap_pos =pci_find_ext_capability (pdev,PCI_EXT_CAP_ID_DPC ); dpc->dev = dev;set_service_data (dev, dpc); + dpc->error_state =PCI_ERS_RESULT_NONE ; + +if (!pcie_aer_get_firmware_first (pdev)) +if (pci_aer_available () && dpc->cap_pos) + dpc->native_dpc =1 ; - status =devm_request_threaded_irq (device, dev->irq, dpc_irq, - dpc_handler,IRQF_SHARED , -"pcie-dpc" , dpc); -if (status) { -dev_warn (device,"request IRQ%d failed: %d\n" , dev->irq, - status); -return status; +/* +* If native support is not enabled and ACPI is not +* enabled then return error. +*/ +if (!dpc->native_dpc && !IS_ENABLED (CONFIG_APCI )) +return -ENODEV ; + +if (dpc->native_dpc) { + status =devm_request_threaded_irq (device, dev->irq, dpc_irq, + dpc_handler,IRQF_SHARED , +"pcie-dpc" , dpc); +if (status) {
drivers/pci/pcie/dpc.c static int dpc_probe(struct pcie_device *dev) · line 585
Quoted 10 lines
drivers/pci/pcie/dpc.c +8
-0+dev_warn (device,"request IRQ%d failed: %d\n" , dev->irq, + status); +return status; + } } +#ifdef CONFIG_ACPI +if (!dpc->native_dpc) { +if (!adev) { +dev_err (device,"No valid acpi device found\n" );
s/acpi/ACPI/ (in comments, printk, other English text)
drivers/pci/pcie/dpc.c static int dpc_probe(struct pcie_device *dev) · line 591
Quoted 6 lines
drivers/pci/pcie/dpc.c +6
-0+return -ENODEV ; + } + + dpc->adev = adev; + +/* Register ACPI notifier for EDR event */
"Register handler for System events, one of which is the EDR event"?
drivers/pci/pcie/dpc.c static int dpc_probe(struct pcie_device *dev) · line 598
Quoted 7 lines
drivers/pci/pcie/dpc.c +7
-0+ astatus =acpi_install_notify_handler (adev->handle, +ACPI_SYSTEM_NOTIFY , + edr_handle_event, + dpc); + +if (ACPI_FAILURE (astatus)) { +dev_err (device,"Install notifier failed\n" );
Mention "ACPI notify handler" here.
Quoted 50 lines
drivers/pci/pcie/dpc.c +16
-2+ return -EBUSY; + } + +acpi_enable_dpc_port (dpc, true); + } +#endif 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);@@ -325,8 +615,12 @@ static int dpc_probe(struct pcie_device *dev) } } - 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); +if (dpc->native_dpc) { + ctl = (ctl &0xfff4 ) |PCI_EXP_DPC_CTL_EN_FATAL | +PCI_EXP_DPC_CTL_INT_EN ; +pci_write_config_word (pdev, dpc->cap_pos +PCI_EXP_DPC_CTL , + ctl); + } dev_info(device, "DPC error containment capabilities: Int Msg #%d, RPExt%c PoisonedTLP%c SwTrigger%c RP PIO Log %d, DL_ActiveErr%c\n", cap & PCI_EXP_DPC_IRQ, FLAG(cap, PCI_EXP_DPC_CAP_RP_EXT),@@ -344,6 +638,9 @@ static void dpc_remove(struct pcie_device *dev)struct pci_dev *pdev = dev->port;u16 ctl; +if (!dpc->native_dpc) +return ; +pci_read_config_word (pdev, dpc->cap_pos +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);@@ -356,6 +653,7 @@ static struct pcie_port_service_driver dpcdriver = { .probe = dpc_probe, .remove = dpc_remove, .reset_link = dpc_reset_link, + .error_resume = dpc_error_resume, };int __init pcie_dpc_init (void )drivers/pci/pcie/portdrv_core.c +0
-0@@ -249,9 +249,14 @@ static int get_port_device_capability(struct pci_dev *dev)pcie_pme_interrupt_enable (dev, false); }
drivers/pci/pcie/portdrv_core.c static int get_port_device_capability(struct pci_dev *dev) · line 259
Quoted 10 lines
drivers/pci/pcie/portdrv_core.c +7
-2+/* +* If EDR support is enabled in OS, then even if AER is not handled in +* OS, DPC service can be enabled. +*/ if (pci_find_ext_capability (dev,PCI_EXT_CAP_ID_DPC ) && -pci_aer_available () && services &PCIE_PORT_SERVICE_AER && - (pcie_ports_native || host->native_dpc)) + ((IS_ENABLED (CONFIG_PCIE_EDR ) && !host->native_dpc) || + (pci_aer_available () && services &PCIE_PORT_SERVICE_AER && + (pcie_ports_native || host->native_dpc))))
Holy cow, I think I'll have to schedule an hour sometime to figure out what's going on here :)
Quoted 4 lines
drivers/pci/pcie/portdrv_core.c +0
-0services |=PCIE_PORT_SERVICE_DPC ; if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM || -- 2.20.1
Re: [PATCH v2 2/2] PCI/DPC: Add Error Disconnect Recover (EDR) support
Hi ,
Quoted 11 lines · Bjorn Helgaas
Quoted 10 lines · On Tue, Mar 19, 2019 at 01:47:29PM -0700…
From: Kuppuswamy Sathyanarayanan <[email protected]>
As per PCI firmware specification v3.2 ECN
(https://members.pcisig.com/wg/PCI-SIG/document/12614), 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 let firmware know the recovery status via_OSTACPI call.
Quoted 7 lines
Quoted 7 lines
Since EDR is a hybrid service, DPC service shall be enabled in OS even if AER is not natively enabled in OS.
Signed-off-by Kuppuswamy Sathyanarayanan <[email protected]>--- drivers/pci/pcie/Kconfig | 10 + drivers/pci/pcie/dpc.c | 326 ++++++++++++++++++++++++++++++--
Thanks for the review comments.
Quoted 7 lines
I'll be looking for Keith's reviewed-by for this eventually.
It looks like this could be split into some smaller patches:
- Add dpc_dev.native_dpc (hopefully we can find a less confusing name)
- Convertdpc_handler()todpc_handler()+dpc_process_error()
- Add EDR support
Ok. I will split them in next patch set.
Quoted 32 lines
Quoted 32 lines
drivers/pci/pcie/portdrv_core.c | 9 +- 3 files changed, 329 insertions(+), 16 deletions(-)drivers/pci/pcie/Kconfig +10
-0@@ -142,3 +142,13 @@ config PCIE_PTM This is only useful if you have devices that support PTM, but it is safe to enable even if you don't. + +config PCIE_EDR + bool "PCI Express Error Disconnect Recover support" + default n + depends on PCIE_DPC && ACPI + help + This options adds Error Disconnect Recover support as specified + in PCI firmware specification v3.2 Downstream Port Containment + Related Enhancements ECN. Enable this if you want to support hybrid + DPC model which uses both firmware and OS to implement DPC.drivers/pci/pcie/dpc.c +1
-0@@ -11,6 +11,7 @@#include <linux/interrupt.h> #include <linux/init.h> #include <linux/pci.h> +#include <linux/acpi.h>
Quoted 9 lines
drivers/pci/pcie/dpc.c struct dpc_dev { · line 24
Quoted 7 lines
drivers/pci/pcie/dpc.c +1
-0#include "portdrv.h" #include "../pci.h" @@ -20,8 +21,23 @@ struct dpc_dev {u16 cap_pos;bool rp_extensions;u8 rp_log_size; +bool native_dpc;This is going to be way too confusing with a "
native_dpc" in both the structpci_host_bridgeand the structdpc_dev.
Any suggestions? what about native_mode ?
Quoted 64 lines
Quoted 64 lines
drivers/pci/pcie/dpc.c +28
-2+pci_ers_result_t error_state; +#ifdef CONFIG_ACPI +struct acpi_device *adev; +#endif }; +#ifdef CONFIG_ACPI + +#define EDR_PORT_ENABLE_DSM 0x0C +#define EDR_PORT_LOCATE_DSM 0x0D + +static const guid_t pci_acpi_dsm_guid = +GUID_INIT (0xe5c937d0 ,0x3553 ,0x4d7a , +0x91 ,0x17 ,0xea ,0x4d ,0x19 ,0xc3 ,0x43 ,0x4d ); +#endif +static const char *const rp_pio_error_string[] = {"Configuration Request received UR Completion" ,/* Bit Position 0 */ "Configuration Request received CA Completion" ,/* Bit Position 1 */ @@ -67,6 +83,9 @@ void pci_save_dpc_state(struct pci_dev *dev)if (!dpc)return ; +if (!dpc->native_dpc) +return ; + save_state =pci_find_saved_ext_cap (dev,PCI_EXT_CAP_ID_DPC );if (!save_state)return ;@@ -88,6 +107,9 @@ void pci_restore_dpc_state(struct pci_dev *dev)if (!dpc)return ; +if (!dpc->native_dpc) +return ; + save_state =pci_find_saved_ext_cap (dev,PCI_EXT_CAP_ID_DPC );if (!save_state)return ;@@ -224,10 +246,9 @@ static int dpc_get_aer_uncorrect_severity(struct pci_dev *dev,return 1 ; } -static irqreturn_t dpc_handler (int irq,void *context) +static void dpc_process_error (struct dpc_dev *dpc) {struct aer_err_info info; -struct dpc_dev *dpc = context;struct pci_dev *pdev = dpc->dev->port;struct device *dev = &dpc->dev->device;u16 cap = dpc->cap_pos, status, source, reason, ext_reason;@@ -261,6 +282,13 @@ static irqreturn_t dpc_handler(int irq, void *context)/* We configure DPC so it only triggers on ERR_FATAL */ pcie_do_recovery (pdev, pci_channel_io_frozen,PCIE_PORT_SERVICE_DPC ); +} + +static irqreturn_t dpc_handler (int irq,void *context) +{ +struct dpc_dev *dpc = context; + +dpc_process_error (dpc);
Quoted 8 lines
drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 314
Quoted 7 lines
drivers/pci/pcie/dpc.c +1
-0return IRQ_HANDLED ; }@@ -283,6 +311,230 @@ static irqreturn_t dpc_irq(int irq, void *context)return IRQ_HANDLED ; } +void dpc_error_resume (struct pci_dev *dev)Looks like this should be static?
yes. I will fix it in next version.
Quoted 9 lines
drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 320
Quoted 7 lines
drivers/pci/pcie/dpc.c +6
-0+{ +struct dpc_dev *dpc; + + dpc =to_dpc_dev (dev); +if (!dpc) +return ;I don't think it's possible for dpc to be NULL, is it? Remove the test if not.
ok.
Quoted 17 lines
Quoted 17 lines
drivers/pci/pcie/dpc.c +16
-0+ dpc->error_state =PCI_ERS_RESULT_RECOVERED ; +} + +#ifdef CONFIG_ACPI + +/* +* _DSM wrapper function to enable/disable DPC port. +* @dpc : DPC 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 dpc_dev *dpc,bool enable) +{ +union acpi_object *obj; +int status;
Quoted 8 lines
drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 342
Quoted 5 lines
drivers/pci/pcie/dpc.c +5
-0+union acpi_object argv4; + +/* Check whether EDR_PORT_ENABLE_DSM is supported in firmware */ + status =acpi_check_dsm (dpc->adev->handle, &pci_acpi_dsm_guid,1 , +1 <<EDR_PORT_ENABLE_DSM );I don't think this
acpi_check_dsm()is necessary, is it? I expectacpi_evaluate_dsm()to fail nicely if there's no_DSMor it doesn't support the function.
will remove it next version.
Quoted 30 lines
Quoted 30 lines
drivers/pci/pcie/dpc.c +29
-0+if (!status) +return -ENOTSUPP ; + + argv4.type =ACPI_TYPE_INTEGER ; + argv4.integer.value = enable; + + obj =acpi_evaluate_dsm (dpc->adev->handle, &pci_acpi_dsm_guid,1 , +EDR_PORT_ENABLE_DSM , &argv4); +if (!obj) +return -EIO ; + +if (obj->type ==ACPI_TYPE_INTEGER && obj->integer.value == enable) + status =0 ; +else + status = -EIO ; + +ACPI_FREE (obj); + +return status; +} + +/* +* _DSM wrapper function to locate DPC port. +* @dpc : DPC device structure +* +* returns pci_dev or NULL. +*/ +static struct pci_dev *acpi_locate_dpc_port (struct dpc_dev *dpc) +{
Quoted 8 lines
drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 378
Quoted 7 lines
drivers/pci/pcie/dpc.c +7
-0+union acpi_object *obj; +int status; +u16 port; + +/* Check whether EDR_PORT_LOCATE_DSM is supported in firmware */ + status =acpi_check_dsm (dpc->adev->handle, &pci_acpi_dsm_guid,1 , +1 <<EDR_PORT_LOCATE_DSM );Unnecessary?
Agreed
Quoted 62 lines
drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 379
Quoted 3 lines
drivers/pci/pcie/dpc.c +2
-0+if (!status) +return dpc->dev->port;If you *do* need the
acpi_check_dsm(), I would have expected to return NULL in this error case?drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 381
Quoted 2 lines
drivers/pci/pcie/dpc.c +2
-0+ +Extra blank line here.
Quoted 17 lines
drivers/pci/pcie/dpc.c +17
-0+ obj =acpi_evaluate_dsm (dpc->adev->handle, &pci_acpi_dsm_guid,1 , +EDR_PORT_LOCATE_DSM ,NULL ); +if (!obj) +return NULL ; + +if (obj->type ==ACPI_TYPE_INTEGER ) { +/* +* 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); + }else { +ACPI_FREE (obj); +return NULL ;drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 409
Quoted 10 lines
drivers/pci/pcie/dpc.c +10
-0+ } + +return pci_get_domain_bus_and_slot (0 ,PCI_BUS_NUM (port), port &0xff ); +} + +/* +* _OST wrapper function to let firmware know the status of EDR event. +* @dpc : DPC device structure. +* @status: Status of EDR event. +* Spurious blank line in the comment.
drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 422
Quoted 13 lines
drivers/pci/pcie/dpc.c +13
-0+*/ +static int acpi_send_edr_status (struct dpc_dev *dpc,u16 status) +{ +u32 ost_status; +struct pci_dev *pdev = dpc->dev->port; + +dev_dbg (&pdev->dev,"Sending EDR status :%x\n" , status); + + ost_status =PCI_DEVID (pdev->bus->number, pdev->devfn); + ost_status = (ost_status <<16 ) | status; + +if (!acpi_has_method (dpc->adev->handle,"_OST" )) +return -ENOTSUPP ;This
acpi_has_method()check is superfluous, isn't it? I would expectacpi_evaluate_ost()to fail gracefully if there's no_OST.I do see several other instances of code of the form:
if (!acpi_has_method(handle, "XXXX")) return false; status = acpi_evaluate_*(handle, "XXXX", ...);
Quoted 5 lines
But I think that's a pointless pattern. I think we could just try to evaluate the method directly, since the evaluation will fail if the method doesn't exist:
status = acpi_evaluate_*(handle, "XXXX", ...);
Agreed.
Quoted 50 lines
Quoted 50 lines
drivers/pci/pcie/dpc.c +49
-0+ status =acpi_evaluate_ost (dpc->adev->handle, +ACPI_NOTIFY_DISCONNECT_RECOVER , + ost_status,NULL ); +if (ACPI_FAILURE (status)) +return -EINVAL ; + +return 0 ; +} + +/* +* Helper function used for disconnecting the child devices when EDR event is +* received from firmware. +*/ +static void dpc_disconnect_devices (struct pci_dev *dev) +{ +struct pci_dev *udev; +struct pci_bus *parent; +struct pci_dev *pdev, *temp; + +dev_dbg (&dev->dev,"Disconnecting the child devices\n" ); + +if (dev->hdr_type ==PCI_HEADER_TYPE_BRIDGE ) + udev = dev; +else + udev = dev->bus->self; + + parent = udev->subordinate; +pci_walk_bus (parent, pci_dev_set_disconnected,NULL ); + +pci_lock_rescan_remove (); +pci_dev_get (dev); +list_for_each_entry_safe_reverse (pdev, temp, &parent->devices, + bus_list) { +pci_stop_and_remove_bus_device (pdev); + } +pci_dev_put (dev); +pci_unlock_rescan_remove (); +} + +static void edr_handle_event (acpi_handle handle,u32 event,void *data) +{ +struct dpc_dev *dpc = (struct dpc_dev *) data; +struct pci_dev *pdev; +u16 status, cap; + +if (event !=ACPI_NOTIFY_DISCONNECT_RECOVER ) +return ; + +if (!data) {
Quoted 8 lines
drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 473
Quoted 1 line
drivers/pci/pcie/dpc.c +1
-0+pr_err ("Invalid EDR event\n" );In what instance can "data" be NULL? I think *never*, unless ACPI screwed up and lost the context you supplied to
acpi_install_notify_handler(). In that case, we should do something more significant than print a message and just return. I think we should just omit this test, and if ACPI screwed up, we'll take a null pointer dereference oops, we'll see exactly where, and we'll have a chance to fix the problem.
Makes sense. I will fix it in next version.
Quoted 15 lines
drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 477
Quoted 5 lines
drivers/pci/pcie/dpc.c +4
-0+return ; + } + +dev_dbg (&dpc->dev->port->dev,"Valid EDR event received\n" );Set "pdev" at declaration and use it in these printks.
drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 483
Quoted 6 lines
drivers/pci/pcie/dpc.c +6
-0+ +/* +* Check if _DSM(0xD) is available, and if present locate the +* port which issued EDR event. +*/ + pdev =acpi_locate_dpc_port (dpc);Can this be done at probe-time instead of event-time?
Quoted 15 lines
drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 494
Quoted 11 lines
drivers/pci/pcie/dpc.c +11
-0+if (!pdev) { +dev_err (&dpc->dev->port->dev,"No valid port found\n" ); +return ; + } + +/* +* Get DPC priv data for given pdev +*/ + dpc =to_dpc_dev (pdev); + dpc->error_state =PCI_ERS_RESULT_DISCONNECT ; + pdev = dpc->dev->port;It's quite confusing to reassign pdev here. The comment about "Get DPC priv data for given pdev" is really superfluous since that much is obvious from the code. What's *not* obvious is whether this "pdev =
dpc->dev->port" changes anything and if so, why.
No its not required. It does not add any value. I will remove it in next version.
Quoted 13 lines
drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 505
Quoted 12 lines
drivers/pci/pcie/dpc.c +11
-0+ cap = dpc->cap_pos; + +/* +* Check if the port supports DPC: +* +* if port does not support DPC, then let firmware handle +* the error recovery and OS is responsible for cleaning +* up the child devices. +* +* if port supports DPC, then fall back to default error +* recovery. Capitalize first letter of sentences.
ok
Quoted 9 lines
drivers/pci/pcie/dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 512
Quoted 8 lines
drivers/pci/pcie/dpc.c +7
-0+* +*/ +if (cap) { +/* Check if there is a valid DPC trigger */ +pci_read_config_word (pdev, cap +PCI_EXP_DPC_STATUS , &status); +if (!(status &PCI_EXP_DPC_STATUS_TRIGGER )) { +dev_err (&pdev->dev,"Invalid DPC trigger\n" );Include "status" value in the printk.
ok
Quoted 78 lines
Quoted 78 lines
drivers/pci/pcie/dpc.c +50
-10+ return; + } +dpc_process_error (dpc); + } + +if (dpc->error_state ==PCI_ERS_RESULT_RECOVERED ) { +/* +* Recovery is successful, so send +* _OST(0xF, BDF << 16 | 0x80, "") to firmware. +*/ + status =0x80 ; + }else { +/* +* Recovery is not successful, so disconnect child devices +* and send _OST(0xF, BDF << 16 | 0x81, "") to firmware. +*/ +dpc_disconnect_devices (pdev); + status =0x81 ; + } + +acpi_send_edr_status (dpc, status); +} + +#endif +#define FLAG (x, y) (((x) & (y)) ? '+' : '-')static int dpc_probe (struct pcie_device *dev) {@@ -291,9 +543,10 @@ static int dpc_probe(struct pcie_device *dev)struct device *device = &dev->device;int status;u16 ctl, cap; - -if (pcie_aer_get_firmware_first (pdev)) -return -ENOTSUPP ; +#ifdef CONFIG_ACPI +struct acpi_device *adev =ACPI_COMPANION (&pdev->dev); +acpi_status astatus; +#endif dpc =devm_kzalloc (device,sizeof (*dpc),GFP_KERNEL );if (!dpc)@@ -302,16 +555,53 @@ static int dpc_probe(struct pcie_device *dev) dpc->cap_pos =pci_find_ext_capability (pdev,PCI_EXT_CAP_ID_DPC ); dpc->dev = dev;set_service_data (dev, dpc); + dpc->error_state =PCI_ERS_RESULT_NONE ; + +if (!pcie_aer_get_firmware_first (pdev)) +if (pci_aer_available () && dpc->cap_pos) + dpc->native_dpc =1 ; - status =devm_request_threaded_irq (device, dev->irq, dpc_irq, - dpc_handler,IRQF_SHARED , -"pcie-dpc" , dpc); -if (status) { -dev_warn (device,"request IRQ%d failed: %d\n" , dev->irq, - status); -return status; +/* +* If native support is not enabled and ACPI is not +* enabled then return error. +*/ +if (!dpc->native_dpc && !IS_ENABLED (CONFIG_APCI )) +return -ENODEV ; + +if (dpc->native_dpc) { + status =devm_request_threaded_irq (device, dev->irq, dpc_irq, + dpc_handler,IRQF_SHARED , +"pcie-dpc" , dpc); +if (status) { +dev_warn (device,"request IRQ%d failed: %d\n" , dev->irq, + status); +return status; + } }
Quoted 5 lines
drivers/pci/pcie/dpc.c static int dpc_probe(struct pcie_device *dev) · line 585
Quoted 4 lines
drivers/pci/pcie/dpc.c +4
-0+#ifdef CONFIG_ACPI +if (!dpc->native_dpc) { +if (!adev) { +dev_err (device,"No valid acpi device found\n" );s/acpi/ACPI/ (in comments, printk, other English text)
ok
Quoted 8 lines
drivers/pci/pcie/dpc.c static int dpc_probe(struct pcie_device *dev) · line 591
Quoted 7 lines
drivers/pci/pcie/dpc.c +6
-0+return -ENODEV ; + } + + dpc->adev = adev; + +/* Register ACPI notifier for EDR event */ "Register handler for System events, one of which is the EDR event"?
In our case, we only handle EDR event. So I just explicitly mentioned it.
Quoted 9 lines
drivers/pci/pcie/dpc.c static int dpc_probe(struct pcie_device *dev) · line 598
Quoted 8 lines
drivers/pci/pcie/dpc.c +7
-0+ astatus =acpi_install_notify_handler (adev->handle, +ACPI_SYSTEM_NOTIFY , + edr_handle_event, + dpc); + +if (ACPI_FAILURE (astatus)) { +dev_err (device,"Install notifier failed\n" );Mention "ACPI notify handler" here.
ok
Quoted 51 lines
Quoted 51 lines
drivers/pci/pcie/dpc.c +16
-2+ return -EBUSY; + } + +acpi_enable_dpc_port (dpc, true); + } +#endif 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);@@ -325,8 +615,12 @@ static int dpc_probe(struct pcie_device *dev) } } - 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); +if (dpc->native_dpc) { + ctl = (ctl &0xfff4 ) |PCI_EXP_DPC_CTL_EN_FATAL | +PCI_EXP_DPC_CTL_INT_EN ; +pci_write_config_word (pdev, dpc->cap_pos +PCI_EXP_DPC_CTL , + ctl); + } dev_info(device, "DPC error containment capabilities: Int Msg #%d, RPExt%c PoisonedTLP%c SwTrigger%c RP PIO Log %d, DL_ActiveErr%c\n", cap & PCI_EXP_DPC_IRQ, FLAG(cap, PCI_EXP_DPC_CAP_RP_EXT),@@ -344,6 +638,9 @@ static void dpc_remove(struct pcie_device *dev)struct pci_dev *pdev = dev->port;u16 ctl; +if (!dpc->native_dpc) +return ; +pci_read_config_word (pdev, dpc->cap_pos +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);@@ -356,6 +653,7 @@ static struct pcie_port_service_driver dpcdriver = { .probe = dpc_probe, .remove = dpc_remove, .reset_link = dpc_reset_link, + .error_resume = dpc_error_resume, };int __init pcie_dpc_init (void )drivers/pci/pcie/portdrv_core.c +0
-0@@ -249,9 +249,14 @@ static int get_port_device_capability(struct pci_dev *dev)pcie_pme_interrupt_enable (dev, false); }
Quoted 12 lines
drivers/pci/pcie/portdrv_core.c static int get_port_device_capability(struct pci_dev *dev) · line 259
Quoted 10 lines
drivers/pci/pcie/portdrv_core.c +7
-2+/* +* If EDR support is enabled in OS, then even if AER is not handled in +* OS, DPC service can be enabled. +*/ if (pci_find_ext_capability (dev,PCI_EXT_CAP_ID_DPC ) && -pci_aer_available () && services &PCIE_PORT_SERVICE_AER && - (pcie_ports_native || host->native_dpc)) + ((IS_ENABLED (CONFIG_PCIE_EDR ) && !host->native_dpc) || + (pci_aer_available () && services &PCIE_PORT_SERVICE_AER && + (pcie_ports_native || host->native_dpc))))Holy cow, I think I'll have to schedule an hour sometime to figure out what's going on here :)
Please check the previous patch in this series for details related to host->native_dpc.
host->native_dpc is set/unset based on _OSC negotiation in drivers/acpi/pci_root.c
Previously if BIOS handles AER and DPC then we don't need to enable AER/DPC services in OS. But with EDR support (hybrid mode), even if BIOS handles DPC support, there is way for it let OS handle the port recovery and error handling. So we should enable the DPC service independently. That's why I have added additional if condition for non-native DPC mode with EDR support enabled.
If we expand the if condition it would look like,
if (port has dpc capability) {
if (EDR support is supported/enabled in OS and bios_handles_dpc) then services |= PCIE_PORT_SERVICE_DPC;
if ( AER/DPC is handled in OS)
services |= PCIE_PORT_SERVICE_DPC;
}
Quoted 5 lines
Quoted 5 lines
drivers/pci/pcie/portdrv_core.c +0
-0services |=PCIE_PORT_SERVICE_DPC ; if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM || -- 2.20.1
Signature · 2 lines
Sathyanarayanan Kuppuswamy Linux kernel developer
Re: [PATCH v2 2/2] PCI/DPC: Add Error Disconnect Recover (EDR) support
Quoted 12 lines · sathyanarayanan kuppuswamy
Quoted 11 lines · Bjorn Helgaas
Quoted 10 lines · On Tue, Mar 19, 2019 at 01:47:29PM -0700…
From: Kuppuswamy Sathyanarayanan <[email protected]>
As per PCI firmware specification v3.2 ECN
(https://members.pcisig.com/wg/PCI-SIG/document/12614), 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 let firmware know the recovery status via_OSTACPI call.
Quoted 7 lines
Quoted 6 lines
drivers/pci/pcie/dpc.c struct dpc_dev { · line 24
Quoted 4 lines
Since EDR is a hybrid service, DPC service shall be enabled in OS even if AER is not natively enabled in OS.
drivers/pci/pcie/dpc.c +1
-0+bool native_dpc;This is going to be way too confusing with a "
native_dpc" in both the structpci_host_bridgeand the structdpc_dev.Any suggestions? what about
native_mode?
"native_mode" is different but doesn't contain any additional information. I haven't worked out what this new symbol actually does; if it's related to EDR, maybe that could be incorporated somehow?
Quoted 3 lines
Quoted 2 lines
drivers/pci/pcie/dpc.c static int dpc_probe(struct pcie_device *dev) · line 591
Quoted 1 line
drivers/pci/pcie/dpc.c +1
-0+/* Register ACPI notifier for EDR event */ "Register handler for System events, one of which is the EDR event"?
In our case, we only handle EDR event. So I just explicitly mentioned it.
Right. As a courtesy to the reader, I think it's worth making the comment match the code, i.e., you can't pass an event type to acpi_install_notify_handler(), so obviously the handler will be called for *all* System events. By saying "one of which is the EDR event", you give the reader a hint that the handler will have to filter out the others.
The reason I noticed this is because I read "Register notifier for EDR event" and wondered to myself "Hmmm, how does this work? I don't see anything passed to acpi_install_notify_handler() that would identify an EDR event."
Quoted 12 lines
Quoted 4 lines
Quoted 4 lines
drivers/pci/pcie/portdrv_core.c +4
-0+/* +* If EDR support is enabled in OS, then even if AER is not handled in +* OS, DPC service can be enabled. +*/ Quoted 8 lines
drivers/pci/pcie/portdrv_core.c static int get_port_device_capability(struct pci_dev *dev) · line 259
Quoted 6 lines
if (
pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC)&&drivers/pci/pcie/portdrv_core.c +3
-2-pci_aer_available () && services &PCIE_PORT_SERVICE_AER && - (pcie_ports_native || host->native_dpc)) + ((IS_ENABLED (CONFIG_PCIE_EDR ) && !host->native_dpc) || + (pci_aer_available () && services &PCIE_PORT_SERVICE_AER && + (pcie_ports_native || host->native_dpc))))Holy cow, I think I'll have to schedule an hour sometime to figure out what's going on here :)
Quoted 2 lines
Please check the previous patch in this series for details related to
host->native_dpc.
Yeah, I'm just saying that conditional is starting to look pretty gnarly. Possibly there's nothing to do about it.
Bjorn