diff mbox

[3/4] PCI/DPC: Enable DPC in conjuction with AER

Message ID 20171219210643.24615-3-keith.busch@intel.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Keith Busch Dec. 19, 2017, 9:06 p.m. UTC
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(-)

Comments

Sinan Kaya Jan. 15, 2018, 2:43 p.m. UTC | #1
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
Keith Busch Jan. 16, 2018, 1:33 a.m. UTC | #2
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?
Sinan Kaya Jan. 16, 2018, 3:04 a.m. UTC | #3
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 mbox

Patch

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);