| Message ID | 20171219210643.24615-3-keith.busch@intel.com |
|---|---|
| State | Not Applicable |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [1/4] PCI/AER: Return approrpiate value when AER is not supported | expand |
On 12/19/2017 4:06 PM, Keith Busch wrote: > @@ -289,6 +290,9 @@ static int dpc_probe(struct pcie_device *dev) > int status; > u16 ctl, cap; > > + if (pcie_aer_get_firmware_first(pdev)) > + return -ENOTSUPP; > + There are two ways to support firmware first handling along with DPC. The first one is to tie DPC handling to the firmware first enable. The second one is to enable DPC ERR_COR signalling so that firmware gets notified on each DPC event occurrence. While the first one gives more control to the firmware, I think it beats the purpose of the DPC. The first approach requires firmware to do some "pre-processing" before notifying operating system of a failure. The goal of the DPC is to put hardware into safe state when a PCIe error happens. The best software recovery following this is to notify endpoint drivers of failures and shutdown threads/processes accessing the hardware as quick as possible. The firmware-first event notification is through ACPI GHES and firmware injects an artifical uncorrected AER error to the operating system. Once OS gets notified, it tries to recover drivers through AER fatal error recovery mechanism. While the semantics of this path is clearly defined in ACPI, it is also known to be slow as well. During the time firmware does its business, operating system still could be trying to access the endpoint address space. My suggestion is to enable ERR_COR signalling so firmware gets a notification on each DPC event for logging purposes. OS handles DPC natively and tries to recover hardware without any external influence. Sinan
On Mon, Jan 15, 2018 at 09:43:22AM -0500, Sinan Kaya wrote: > On 12/19/2017 4:06 PM, Keith Busch wrote: > > @@ -289,6 +290,9 @@ static int dpc_probe(struct pcie_device *dev) > > int status; > > u16 ctl, cap; > > > > + if (pcie_aer_get_firmware_first(pdev)) > > + return -ENOTSUPP; > > + > > There are two ways to support firmware first handling along with DPC. > > The first one is to tie DPC handling to the firmware first enable. > > The second one is to enable DPC ERR_COR signalling so that firmware > gets notified on each DPC event occurrence. > > While the first one gives more control to the firmware, I think it beats > the purpose of the DPC. The first approach requires firmware to do some > "pre-processing" before notifying operating system of a failure. > > The goal of the DPC is to put hardware into safe state when a PCIe error > happens. The best software recovery following this is to notify endpoint > drivers of failures and shutdown threads/processes accessing the hardware > as quick as possible. > > The firmware-first event notification is through ACPI GHES and firmware injects > an artifical uncorrected AER error to the operating system. Once OS gets > notified, it tries to recover drivers through AER fatal error recovery mechanism. > > While the semantics of this path is clearly defined in ACPI, it is also known > to be slow as well. During the time firmware does its business, operating > system still could be trying to access the endpoint address space. > > My suggestion is to enable ERR_COR signalling so firmware gets a notification > on each DPC event for logging purposes. > > OS handles DPC natively and tries to recover hardware without any external > influence. I see what you're saying, but if a device has a firmware first policy, doesn't that mean firmware owns the DPC Control register? The OS shouldn't be mucking with it in that case, right?
On 1/15/2018 8:33 PM, Keith Busch wrote: > On Mon, Jan 15, 2018 at 09:43:22AM -0500, Sinan Kaya wrote: >> On 12/19/2017 4:06 PM, Keith Busch wrote: >>> @@ -289,6 +290,9 @@ static int dpc_probe(struct pcie_device *dev) >>> int status; >>> u16 ctl, cap; >>> >>> + if (pcie_aer_get_firmware_first(pdev)) >>> + return -ENOTSUPP; >>> + >> >> There are two ways to support firmware first handling along with DPC. >> >> The first one is to tie DPC handling to the firmware first enable. >> >> The second one is to enable DPC ERR_COR signalling so that firmware >> gets notified on each DPC event occurrence. >> >> While the first one gives more control to the firmware, I think it beats >> the purpose of the DPC. The first approach requires firmware to do some >> "pre-processing" before notifying operating system of a failure. >> >> The goal of the DPC is to put hardware into safe state when a PCIe error >> happens. The best software recovery following this is to notify endpoint >> drivers of failures and shutdown threads/processes accessing the hardware >> as quick as possible. >> >> The firmware-first event notification is through ACPI GHES and firmware injects >> an artifical uncorrected AER error to the operating system. Once OS gets >> notified, it tries to recover drivers through AER fatal error recovery mechanism. >> >> While the semantics of this path is clearly defined in ACPI, it is also known >> to be slow as well. During the time firmware does its business, operating >> system still could be trying to access the endpoint address space. >> >> My suggestion is to enable ERR_COR signalling so firmware gets a notification >> on each DPC event for logging purposes. >> >> OS handles DPC natively and tries to recover hardware without any external >> influence. > > I see what you're saying, but if a device has a firmware first policy, > doesn't that mean firmware owns the DPC Control register? The OS shouldn't > be mucking with it in that case, right? > I agree. I looked at the spec one more time. These are the two paragraphs mentioning firmware first. Unfortunately, it will come down to the quality of firmware implementation to make something useful out of DPC functionality. There should have been a DPC control request as well as a firmware-first control request instead of tying these together. "Determination of DPC Control DPC may be controlled in some configurations by platform firmware and in other configurations by the operating system. DPC functionality is strongly linked with the functionality in Advanced Error Reporting. To avoid conflicts over whether platform firmware or the operating system have control of DPC, it is recommended that platform firmware and operating systems always link the control of DPC to the control of Advanced Error Reporting." "Use of DPC ERR_COR Signaling It is recommended that operating systems use DPC interrupts for signaling when DPC has been triggered. While DPC ERR_COR signaling indicates the same event, DPC ERR_COR signaling is primarily intended for use by platform firmware, when it needs to be notified in order to do its own logging of the event or provide “firmware first” services"
diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig index ac53edbc9613..d658dfa53b87 100644 --- a/drivers/pci/pcie/Kconfig +++ b/drivers/pci/pcie/Kconfig @@ -92,7 +92,7 @@ config PCIE_PME config PCIE_DPC bool "PCIe Downstream Port Containment support" - depends on PCIEPORTBUS + depends on PCIEPORTBUS && PCIEAER default n help This enables PCI Express Downstream Port Containment (DPC) diff --git a/drivers/pci/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c index 2d976a623ddc..ef71a472592c 100644 --- a/drivers/pci/pcie/pcie-dpc.c +++ b/drivers/pci/pcie/pcie-dpc.c @@ -15,6 +15,7 @@ #include <linux/pci.h> #include <linux/pcieport_if.h> #include "../pci.h" +#include "aer/aerdrv.h" struct rp_pio_header_log_regs { u32 dw0; @@ -289,6 +290,9 @@ static int dpc_probe(struct pcie_device *dev) int status; u16 ctl, cap; + if (pcie_aer_get_firmware_first(pdev)) + return -ENOTSUPP; + dpc = devm_kzalloc(device, sizeof(*dpc), GFP_KERNEL); if (!dpc) return -ENOMEM; diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c index a59210350c44..ef3bad4ad010 100644 --- a/drivers/pci/pcie/portdrv_core.c +++ b/drivers/pci/pcie/portdrv_core.c @@ -216,9 +216,9 @@ static int get_port_device_capability(struct pci_dev *dev) return 0; cap_mask = PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_HP - | PCIE_PORT_SERVICE_VC | PCIE_PORT_SERVICE_DPC; + | PCIE_PORT_SERVICE_VC; if (pci_aer_available()) - cap_mask |= PCIE_PORT_SERVICE_AER; + cap_mask |= PCIE_PORT_SERVICE_AER | PCIE_PORT_SERVICE_DPC; if (pcie_ports_auto) pcie_port_platform_notify(dev, &cap_mask);
The PCI Express Base Specification's implementation note on "Determination of DPC Control" recommends the operating system always link DPC control to the control of AER, as the two functionalities are strongly connected. To avoid conflicts over whether platform firmware or the OS control DPC, this patch enables DPC only if AER is enabled in the OS, and the device's error handling does not have a firmware-first AER handling. Signed-off-by: Keith Busch <keith.busch@intel.com> --- drivers/pci/pcie/Kconfig | 2 +- drivers/pci/pcie/pcie-dpc.c | 4 ++++ drivers/pci/pcie/portdrv_core.c | 4 ++-- 3 files changed, 7 insertions(+), 3 deletions(-)