* [PATCH v2 0/2] Add Error Disconnect Recover (EDR) support. @ 2019-03-19 20:47 sathyanarayanan.kuppuswamy 2019-03-19 20:47 ` [PATCH v2 1/2] PCI/ACPI: Add _OSC based negotiation support for DPC sathyanarayanan.kuppuswamy 2019-03-19 20:47 ` [PATCH v2 2/2] PCI/DPC: Add Error Disconnect Recover (EDR) support sathyanarayanan.kuppuswamy 0 siblings, 2 replies; 6+ messages in thread From: sathyanarayanan.kuppuswamy @ 2019-03-19 20:47 UTC (permalink / raw) To: bhelgaas, rjw, lenb Cc: linux-pci, linux-acpi, linux-kernel, ashok.raj, keith.busch, 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 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(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/2] PCI/ACPI: Add _OSC based negotiation support for DPC 2019-03-19 20:47 [PATCH v2 0/2] Add Error Disconnect Recover (EDR) support sathyanarayanan.kuppuswamy @ 2019-03-19 20:47 ` sathyanarayanan.kuppuswamy 2019-03-19 20:47 ` [PATCH v2 2/2] PCI/DPC: Add Error Disconnect Recover (EDR) support sathyanarayanan.kuppuswamy 1 sibling, 0 replies; 6+ messages in thread From: sathyanarayanan.kuppuswamy @ 2019-03-19 20:47 UTC (permalink / raw) To: bhelgaas, rjw, lenb Cc: linux-pci, linux-acpi, linux-kernel, ashok.raj, keith.busch, sathyanarayanan.kuppuswamy From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> 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. Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> --- 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(-) diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index 707aafc7c2aa..a60261e50b08 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -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, diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c index 7d04f9d087a6..865f12f4b314 100644 --- a/drivers/pci/pcie/portdrv_core.c +++ b/drivers/pci/pcie/portdrv_core.c @@ -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 || diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 2ec0df04e0dc..ef1118f70860 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -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; } diff --git a/include/linux/acpi.h b/include/linux/acpi.h index d5dcebd7aad3..f68e9513996b 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -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 diff --git a/include/linux/pci.h b/include/linux/pci.h index 77448215ef5b..1c38bcd200d1 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -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, -- 2.20.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] PCI/DPC: Add Error Disconnect Recover (EDR) support 2019-03-19 20:47 [PATCH v2 0/2] Add Error Disconnect Recover (EDR) support sathyanarayanan.kuppuswamy 2019-03-19 20:47 ` [PATCH v2 1/2] PCI/ACPI: Add _OSC based negotiation support for DPC sathyanarayanan.kuppuswamy @ 2019-03-19 20:47 ` sathyanarayanan.kuppuswamy 2019-04-10 18:41 ` Bjorn Helgaas 1 sibling, 1 reply; 6+ messages in thread From: sathyanarayanan.kuppuswamy @ 2019-03-19 20:47 UTC (permalink / raw) To: bhelgaas, rjw, lenb Cc: linux-pci, linux-acpi, linux-kernel, ashok.raj, keith.busch, sathyanarayanan.kuppuswamy From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> 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. Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> --- 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(-) diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig index 5cbdbca904ac..55f65d1cbc9e 100644 --- a/drivers/pci/pcie/Kconfig +++ b/drivers/pci/pcie/Kconfig @@ -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. diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c index 7b77754a82de..bdf4ca8a0acb 100644 --- a/drivers/pci/pcie/dpc.c +++ b/drivers/pci/pcie/dpc.c @@ -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" @@ -20,8 +21,23 @@ struct dpc_dev { u16 cap_pos; bool rp_extensions; u8 rp_log_size; + bool native_dpc; + 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; } @@ -283,6 +311,230 @@ static irqreturn_t dpc_irq(int irq, void *context) return IRQ_HANDLED; } +void dpc_error_resume(struct pci_dev *dev) +{ + struct dpc_dev *dpc; + + dpc = to_dpc_dev(dev); + if (!dpc) + return; + + 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; + 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); + 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) +{ + 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); + if (!status) + return dpc->dev->port; + + + 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; + } + + 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. + * + */ +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; + + 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) { + pr_err("Invalid EDR event\n"); + return; + } + + dev_dbg(&dpc->dev->port->dev, "Valid EDR event received\n"); + + /* + * Check if _DSM(0xD) is available, and if present locate the + * port which issued EDR event. + */ + pdev = acpi_locate_dpc_port(dpc); + 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; + 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. + * + */ + 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"); + 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; + } } +#ifdef CONFIG_ACPI + if (!dpc->native_dpc) { + if (!adev) { + dev_err(device, "No valid acpi device found\n"); + return -ENODEV; + } + + dpc->adev = adev; + + /* Register ACPI notifier for EDR event */ + 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"); + 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) diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c index 865f12f4b314..050cbb7a5083 100644 --- a/drivers/pci/pcie/portdrv_core.c +++ b/drivers/pci/pcie/portdrv_core.c @@ -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 || -- 2.20.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] PCI/DPC: Add Error Disconnect Recover (EDR) support 2019-03-19 20:47 ` [PATCH v2 2/2] PCI/DPC: Add Error Disconnect Recover (EDR) support sathyanarayanan.kuppuswamy @ 2019-04-10 18:41 ` Bjorn Helgaas 2019-04-10 22:12 ` sathyanarayanan kuppuswamy 0 siblings, 1 reply; 6+ messages in thread From: Bjorn Helgaas @ 2019-04-10 18:41 UTC (permalink / raw) To: sathyanarayanan.kuppuswamy Cc: rjw, lenb, linux-pci, linux-acpi, linux-kernel, ashok.raj, keith.busch On Tue, Mar 19, 2019 at 01:47:29PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote: > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > > 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. > > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > --- > 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 > drivers/pci/pcie/portdrv_core.c | 9 +- > 3 files changed, 329 insertions(+), 16 deletions(-) > > diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig > index 5cbdbca904ac..55f65d1cbc9e 100644 > --- a/drivers/pci/pcie/Kconfig > +++ b/drivers/pci/pcie/Kconfig > @@ -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. > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c > index 7b77754a82de..bdf4ca8a0acb 100644 > --- a/drivers/pci/pcie/dpc.c > +++ b/drivers/pci/pcie/dpc.c > @@ -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" > @@ -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. > + 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; > } > @@ -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? > +{ > + 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. > + 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; > + 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. > + 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) > +{ > + 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? > + 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? > + > + Extra blank line here. > + 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; > + } > + > + 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. > + */ > +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", ...); > + 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) { > + 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. > + return; > + } > + > + dev_dbg(&dpc->dev->port->dev, "Valid EDR event received\n"); Set "pdev" at declaration and use it in these printks. > + > + /* > + * 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? > + 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. > + 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. > + * > + */ > + 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. > + 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; > + } > } > > +#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) > + return -ENODEV; > + } > + > + dpc->adev = adev; > + > + /* Register ACPI notifier for EDR event */ "Register handler for System events, one of which is the EDR event"? > + 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. > + 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) > diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c > index 865f12f4b314..050cbb7a5083 100644 > --- a/drivers/pci/pcie/portdrv_core.c > +++ b/drivers/pci/pcie/portdrv_core.c > @@ -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)))) Holy cow, I think I'll have to schedule an hour sometime to figure out what's going on here :) > services |= PCIE_PORT_SERVICE_DPC; > > if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM || -- 2.20.1 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] PCI/DPC: Add Error Disconnect Recover (EDR) support 2019-04-10 18:41 ` Bjorn Helgaas @ 2019-04-10 22:12 ` sathyanarayanan kuppuswamy 2019-04-10 23:08 ` Bjorn Helgaas 0 siblings, 1 reply; 6+ messages in thread From: sathyanarayanan kuppuswamy @ 2019-04-10 22:12 UTC (permalink / raw) To: Bjorn Helgaas Cc: rjw, lenb, linux-pci, linux-acpi, linux-kernel, ashok.raj, keith.busch Hi , On 4/10/19 11:41 AM, Bjorn Helgaas wrote: > On Tue, Mar 19, 2019 at 01:47:29PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote: >> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> >> >> 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. >> >> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> >> --- >> drivers/pci/pcie/Kconfig | 10 + >> drivers/pci/pcie/dpc.c | 326 ++++++++++++++++++++++++++++++-- Thanks for the review comments. > 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 Ok. I will split them in next patch set. > >> drivers/pci/pcie/portdrv_core.c | 9 +- >> 3 files changed, 329 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig >> index 5cbdbca904ac..55f65d1cbc9e 100644 >> --- a/drivers/pci/pcie/Kconfig >> +++ b/drivers/pci/pcie/Kconfig >> @@ -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. >> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c >> index 7b77754a82de..bdf4ca8a0acb 100644 >> --- a/drivers/pci/pcie/dpc.c >> +++ b/drivers/pci/pcie/dpc.c >> @@ -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" >> @@ -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. Any suggestions? what about native_mode ? > >> + 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; >> } >> @@ -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. > >> +{ >> + 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. > >> + 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; >> + 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. will remove it next version. > >> + 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) >> +{ >> + 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 > >> + 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? > >> + >> + > Extra blank line here. > >> + 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; >> + } >> + >> + 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. > >> + */ >> +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", ...); Agreed. > >> + 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) { >> + 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. > >> + return; >> + } >> + >> + dev_dbg(&dpc->dev->port->dev, "Valid EDR event received\n"); > Set "pdev" at declaration and use it in these printks. > >> + >> + /* >> + * 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? > >> + 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. > >> + 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 > >> + * >> + */ >> + 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 > >> + 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; >> + } >> } >> >> +#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 > >> + 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. > >> + 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 > >> + 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) >> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c >> index 865f12f4b314..050cbb7a5083 100644 >> --- a/drivers/pci/pcie/portdrv_core.c >> +++ b/drivers/pci/pcie/portdrv_core.c >> @@ -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)))) > 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; } > >> services |= PCIE_PORT_SERVICE_DPC; >> >> if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM || -- 2.20.1 >> -- Sathyanarayanan Kuppuswamy Linux kernel developer ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] PCI/DPC: Add Error Disconnect Recover (EDR) support 2019-04-10 22:12 ` sathyanarayanan kuppuswamy @ 2019-04-10 23:08 ` Bjorn Helgaas 0 siblings, 0 replies; 6+ messages in thread From: Bjorn Helgaas @ 2019-04-10 23:08 UTC (permalink / raw) To: sathyanarayanan kuppuswamy Cc: rjw, lenb, linux-pci, linux-acpi, linux-kernel, ashok.raj, keith.busch On Wed, Apr 10, 2019 at 03:12:05PM -0700, sathyanarayanan kuppuswamy wrote: > On 4/10/19 11:41 AM, Bjorn Helgaas wrote: > > On Tue, Mar 19, 2019 at 01:47:29PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote: > > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > > > > > > 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. > > > + 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. > 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? > > > + /* 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." > > > + /* > > > + * 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. Yeah, I'm just saying that conditional is starting to look pretty gnarly. Possibly there's nothing to do about it. Bjorn ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-04-10 23:09 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-19 20:47 [PATCH v2 0/2] Add Error Disconnect Recover (EDR) support sathyanarayanan.kuppuswamy 2019-03-19 20:47 ` [PATCH v2 1/2] PCI/ACPI: Add _OSC based negotiation support for DPC sathyanarayanan.kuppuswamy 2019-03-19 20:47 ` [PATCH v2 2/2] PCI/DPC: Add Error Disconnect Recover (EDR) support sathyanarayanan.kuppuswamy 2019-04-10 18:41 ` Bjorn Helgaas 2019-04-10 22:12 ` sathyanarayanan kuppuswamy 2019-04-10 23:08 ` Bjorn Helgaas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).