Skip to contentSkip to conversation

PCI/DPC updates

PATCH In review Keith Busch 2 participants 10 messages Jan 17, 2018 – Jan 25, 2018 PCI

lore
Share & tools
lore

Keith Busch

author
#0 lore raw

v2 cover PCI/DPC updates

v1 -> v2:
Updated change logs with additional information

Print AER status _only_ if the trigger reason is an uncorrectable error.

Clear the uncorrectable status register after printing so we don't log the same errors multiple times.

Added a patch to fix masking a field being printed.

Added a patch to enable ERR_COR.

Keith Busch (6):
PCI/AER: Return approrpiate value when AER is not supported PCI/AER: Provide API for getting AER information PCI/DPC: Enable DPC in conjuction with AER PCI/DPC: Print AER status in DPC event handling PCI/DPC: Fix interrupt message number print PCI/DPC: Enable ERR_COR

 drivers/pci/pcie/Kconfig           |  2 +-
 drivers/pci/pcie/aer/aerdrv.h      |  1 +
 drivers/pci/pcie/aer/aerdrv_core.c | 10 +++++-----
 drivers/pci/pcie/pcie-dpc.c        | 23 +++++++++++++++++++++--
 drivers/pci/pcie/portdrv_core.c    |  4 ++--
 include/uapi/linux/pci_regs.h      |  3 ++-
 6 files changed, 32 insertions(+), 11 deletions(-)
Signature · 1 line
2.13.6
author
#1 lore raw

PATCH v2 1/6 PCI/AER: Return correct value when AER is not supported

Getting the AER information is documented to return 0 when it failed to get the information. If called using a device that does not support AER, it may cause erroneous data to be logged.

There should not be a case this currently fails, but this patch is preparing for additional use cases.

Signed-off-by Keith Busch <[email protected]>
---
 drivers/pci/pcie/aer/aerdrv_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/pcie/aer/aerdrv_core.c +1 -1
index 744805232155..ea0dc1cc7fc7 100644
@@ -660,7 +660,7 @@ static int get_device_error_info(struct pci_dev *dev, struct aer_err_info *info)
 
 	/* The device might not support AER */
 	if (!pos)
-		return 1;
+		return 0;
 
 	if (info->severity == AER_CORRECTABLE) {
 		pci_read_config_dword(dev, pos + PCI_ERR_COR_STATUS,
Signature · 1 line
2.13.6
author
#2 lore raw

PATCH v2 2/6 PCI/AER: API for obtaining AER information

In reply to Keith Busch

Since this is making the function externally visible, appending "aer_" prefix to the function name.

Signed-off-by Keith Busch <[email protected]>
---
 drivers/pci/pcie/aer/aerdrv.h      | 1 +
 drivers/pci/pcie/aer/aerdrv_core.c | 8 ++++----
 2 files changed, 5 insertions(+), 4 deletions(-)
drivers/pci/pcie/aer/aerdrv.h +1 -0
index 5449e5ce139d..7c833a1d897e 100644
@@ -107,6 +107,7 @@ static inline pci_ers_result_t merge_result(enum pci_ers_result orig,
 }
 
 extern struct bus_type pcie_port_bus_type;
+int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info);
 void aer_isr(struct work_struct *work);
 void aer_print_error(struct pci_dev *dev, struct aer_err_info *info);
 void aer_print_port_info(struct pci_dev *dev, struct aer_err_info *info);
drivers/pci/pcie/aer/aerdrv_core.c +4 -4
index ea0dc1cc7fc7..4ba1fbd4700b 100644
@@ -640,7 +640,7 @@ static void aer_recover_work_func(struct work_struct *work)
 #endif
 
 /**
- * get_device_error_info - read error status from dev and store it to info
+ * aer_get_device_error_info - read error status from dev and store it to info
  * @dev: pointer to the device expected to have a error record
  * @info: pointer to structure to store the error record
  *

@@ -648,7 +648,7 @@ static void aer_recover_work_func(struct work_struct *work)
  *
  * Note that @info is reused among all error devices. Clear fields properly.
  */
-static int get_device_error_info(struct pci_dev *dev, struct aer_err_info *info)
+int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info)
 {
 	int pos, temp;
 

@@ -707,11 +707,11 @@ static inline void aer_process_err_devices(struct pcie_device *p_device,
 
 	/* Report all before handle them, not to lost records by reset etc. */
 	for (i = 0; i < e_info->error_dev_num && e_info->dev[i]; i++) {
-		if (get_device_error_info(e_info->dev[i], e_info))
+		if (aer_get_device_error_info(e_info->dev[i], e_info))
 			aer_print_error(e_info->dev[i], e_info);
 	}
 	for (i = 0; i < e_info->error_dev_num && e_info->dev[i]; i++) {
-		if (get_device_error_info(e_info->dev[i], e_info))
+		if (aer_get_device_error_info(e_info->dev[i], e_info))
 			handle_error_source(p_device, e_info->dev[i], e_info);
 	}
 }
Signature · 1 line
2.13.6
author
#3 lore raw

PATCH v2 3/6 PCI/DPC: Enable DPC in conjuction with AER

In reply to Keith Busch

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 <[email protected]>
---
 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(-)
drivers/pci/pcie/Kconfig +1 -1
index ac53edbc9613..d658dfa53b87 100644
@@ -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)
drivers/pci/pcie/pcie-dpc.c +4 -0
index f7cf5ae7dec2..d1fbd83cd240 100644
@@ -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;

@@ -309,6 +310,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;
drivers/pci/pcie/portdrv_core.c +2 -2
index a59210350c44..ef3bad4ad010 100644
@@ -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);
Signature · 1 line
2.13.6
author
#4 lore raw

PATCH v2 4/6 PCI/DPC: Print AER status in DPC event handling

In reply to Keith Busch

A DPC enabled device suppresses ERR_(NON)FATAL messages, preventing the AER handler from reporting error details. If the DPC trigger reason says the downstream port detected the error, this patch has the DPC driver collect the AER uncorrectable status for logging, then clears the status.

Signed-off-by Keith Busch <[email protected]>
---
 drivers/pci/pcie/pcie-dpc.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
drivers/pci/pcie/pcie-dpc.c +12 -0
index d1fbd83cd240..85350b00f251 100644
@@ -44,6 +44,7 @@ struct dpc_dev {
 	int			cap_pos;
 	bool			rp;
 	u32			rp_pio_status;
+	struct aer_err_info	info;
 };
 
 static const char * const rp_pio_error_string[] = {

@@ -112,6 +113,12 @@ static void interrupt_event_handler(struct work_struct *work)
 	struct pci_bus *parent = pdev->subordinate;
 	u16 ctl;
 
+	if (dpc->info.severity == AER_NONFATAL) {
+		if (aer_get_device_error_info(pdev, &dpc->info)) {
+			aer_print_error(pdev, &dpc->info);
+			pci_cleanup_aer_uncorrect_error_status(pdev);
+		}
+	}
 	pci_lock_rescan_remove();
 	list_for_each_entry_safe_reverse(dev, temp, &parent->devices,
 					 bus_list) {

@@ -298,6 +305,11 @@ static irqreturn_t dpc_irq(int irq, void *context)
 
 	schedule_work(&dpc->work);
 
+	if (reason == 0)
+		dpc->info.severity = AER_NONFATAL;
+	else
+		dpc->info.severity = AER_CORRECTABLE;
+
 	return IRQ_HANDLED;
 }
Signature · 1 line
2.13.6
#5 lore raw

Re: [PATCHv2 4/6] PCI/DPC: Print AER status in DPC event handling

Quoted 36 lines · Keith Busch

A DPC enabled device suppresses ERR_(NON)FATAL messages, preventing the AER handler from reporting error details. If the DPC trigger reason says the downstream port detected the error, this patch has the DPC driver collect the AER uncorrectable status for logging, then clears the status.

Signed-off-by Keith Busch <[email protected]>
---
 drivers/pci/pcie/pcie-dpc.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
drivers/pci/pcie/pcie-dpc.c +7 -0
index d1fbd83cd240..85350b00f251 100644
@@ -44,6 +44,7 @@ struct dpc_dev {
 	int			cap_pos;
 	bool			rp;
 	u32			rp_pio_status;
+	struct aer_err_info	info;
 };
 
 static const char * const rp_pio_error_string[] = {

@@ -112,6 +113,12 @@ static void interrupt_event_handler(struct work_struct *work)
 	struct pci_bus *parent = pdev->subordinate;
 	u16 ctl;
 
+	if (dpc->info.severity == AER_NONFATAL) {
+		if (aer_get_device_error_info(pdev, &dpc->info)) {
+			aer_print_error(pdev, &dpc->info);
+			pci_cleanup_aer_uncorrect_error_status(pdev);
+		}
+	}
 	pci_lock_rescan_remove();
 	list_for_each_entry_safe_reverse(dev, temp, &parent->devices,
 					 bus_list) {

drivers/pci/pcie/pcie-dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 311

Quoted 7 lines
drivers/pci/pcie/pcie-dpc.c +4 -0
@@ -298,6 +305,11 @@ static irqreturn_t dpc_irq(int irq, void *context)
 
 	schedule_work(&dpc->work);
 
+	if (reason == 0)
+		dpc->info.severity = AER_NONFATAL;
+	else
+		dpc->info.severity = AER_CORRECTABLE;

This looks wrong because we call schedule_work() before we set dpc->info.severity. I think that's racy because interrupt_event_handler() might read dpc->info.severity before dpc_irq() sets it.

That raises the question of how we pass information from dpc_irq() to interrupt_event_handler(). I think interrupt_event_handler() is a work item because it removes devices and might need to sleep. That makes sense.

But we pass dpc->info and dpc->rp_pio_status from dpc_irq() to interrupt_event_handler() via the single struct dpc_dev. That doesn't make sense in general because dpc_irq() may be invoked several times for each invocation of interrupt_event_handler(). I think the general purpose solution for passing information from interrupt context to process context would be a queue.

It looks like we side-step that issue by disabling interrupts in dpc_irq() and re-enabling them in interrupt_event_handler(). That probably does work, but it's strange and requires extra config accesses. The fact that we clear PCI_EXP_DPC_STATUS_INTERRUPT at the end of interrupt_event_handler() should be enough -- sec 6.2.10.1 says we should only get a new MSI when the logical AND of things including PCI_EXP_DPC_STATUS_INTERRUPT transitions from FALSE to TRUE.

I think all the log registers (Error Source ID and RP PIO Logs) are latched when DPC Trigger Status is set, so I think all the dmesg logging and dpc_process_rp_pio_error() stuff *could* be done in interrupt_event_handler(). Then it would be together with the PCI_EXP_DPC_STATUS write that clears DPC Trigger Status (and clears the latched log registers) and we wouldn't have the question about what belongs on dpc_irq() and what belongs in interrupt_event_handler().

After we clear PCI_EXP_DPC_STATUS_TRIGGER, we're supposed to "honor timing requirements ... with respect to the first Configuration Read following a Conventional Reset" (PCIe r4.0, sec 7.9.15.4). Where does that happen?

Quoted 6 lines
drivers/pci/pcie/pcie-dpc.c +0 -0
 	return IRQ_HANDLED;
 }
Signature · 1 line
2.13.6
author
#6 lore raw

Re: [PATCHv2 4/6] PCI/DPC: Print AER status in DPC event handling

Quoted 4 lines · Bjorn Helgaas
Quoted 3 lines · Keith Busch

schedule_work(&dpc->work);

Quoted 9 lines

drivers/pci/pcie/pcie-dpc.c static irqreturn_t dpc_irq(int irq, void *context) · line 311

Quoted 4 lines
drivers/pci/pcie/pcie-dpc.c +4 -0
+	if (reason == 0)
+		dpc->info.severity = AER_NONFATAL;
+	else
+		dpc->info.severity = AER_CORRECTABLE;

This looks wrong because we call schedule_work() before we set dpc->info.severity. I think that's racy because interrupt_event_handler() might read dpc->info.severity before dpc_irq() sets it.

You're right, my mistake.

Quoted 12 lines

That raises the question of how we pass information from dpc_irq() to interrupt_event_handler(). I think interrupt_event_handler() is a work item because it removes devices and might need to sleep. That makes sense.

But we pass dpc->info and dpc->rp_pio_status from dpc_irq() to interrupt_event_handler() via the single struct dpc_dev. That doesn't make sense in general because dpc_irq() may be invoked several times for each invocation of interrupt_event_handler(). I think the general purpose solution for passing information from interrupt context to process context would be a queue.

Quoted 7 lines

It looks like we side-step that issue by disabling interrupts in dpc_irq() and re-enabling them in interrupt_event_handler(). That probably does work, but it's strange and requires extra config accesses. The fact that we clear PCI_EXP_DPC_STATUS_INTERRUPT at the end of interrupt_event_handler() should be enough -- sec 6.2.10.1 says we should only get a new MSI when the logical AND of things including PCI_EXP_DPC_STATUS_INTERRUPT transitions from FALSE to TRUE.

Yes, that's true. We could get the same interrupt for non-DPC related events, and the interrupt config setting was something Alex proposed to make sure we don't note the same events multiple times. Using these config registers seems a bit overkill, though.

Quoted 8 lines

I think all the log registers (Error Source ID and RP PIO Logs) are latched when DPC Trigger Status is set, so I think all the dmesg logging and dpc_process_rp_pio_error() stuff *could* be done in interrupt_event_handler(). Then it would be together with the PCI_EXP_DPC_STATUS write that clears DPC Trigger Status (and clears the latched log registers) and we wouldn't have the question about what belongs on dpc_irq() and what belongs in interrupt_event_handler().

That sounds great. If we defer all logging and containment handling to the work-queue, that should even fix up the issues the interrupt disable/enabling was trying to solve.

That doesn't even sound difficult to code. Let me send you a proposal instead of this series, maybe by tomorrow.

Quoted 4 lines

After we clear PCI_EXP_DPC_STATUS_TRIGGER, we're supposed to "honor timing requirements ... with respect to the first Configuration Read following a Conventional Reset" (PCIe r4.0, sec 7.9.15.4). Where does that happen?

That is referring to reading the downstream port, and that is not handled by the DPC driver. The expectation is that the link is down on a DPC event, and the Link Up is handled by the pciehp driver, which should be honoring those timing requirements.

author
#7 lore raw

Re: [PATCHv2 4/6] PCI/DPC: Print AER status in DPC event handling

Quoted 11 lines · Keith Busch
Quoted 4 lines · Bjorn Helgaas

After we clear PCI_EXP_DPC_STATUS_TRIGGER, we're supposed to "honor timing requirements ... with respect to the first Configuration Read following a Conventional Reset" (PCIe r4.0, sec 7.9.15.4). Where does that happen?

That is referring to reading the downstream port, and that is not handled by the DPC driver. The expectation is that the link is down on a DPC event, and the Link Up is handled by the pciehp driver, which should be honoring those timing requirements.

Sorry, I didn't mean "reading the downstream port". You can config read that anytime. I meant issuing a config read to the device connected downstream the contained port.

author
#8 lore raw

PATCH v2 5/6 PCI/DPC: Fix interrupt message number print

In reply to Keith Busch

The interrupt message number is the first 5 bits, but the driver was masking only the first 4 bits. This patch fixes that by using the existing define.

This patch is also fixing the define formatting, as it wasn't aligned.

Signed-off-by Keith Busch <[email protected]>
---
 drivers/pci/pcie/pcie-dpc.c   | 2 +-
 include/uapi/linux/pci_regs.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
drivers/pci/pcie/pcie-dpc.c +1 -1
index 85350b00f251..5dfdb4935bac 100644
@@ -351,7 +351,7 @@ static int dpc_probe(struct pcie_device *dev)
 	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 & 0xf, FLAG(cap, PCI_EXP_DPC_CAP_RP_EXT),
+		cap & PCI_EXP_DPC_IRQ, FLAG(cap, PCI_EXP_DPC_CAP_RP_EXT),
 		FLAG(cap, PCI_EXP_DPC_CAP_POISONED_TLP),
 		FLAG(cap, PCI_EXP_DPC_CAP_SW_TRIGGER), (cap >> 8) & 0xf,
 		FLAG(cap, PCI_EXP_DPC_CAP_DL_ACTIVE));
include/uapi/linux/pci_regs.h +1 -1
index 70c2b2ade048..295a5ebdf9e4 100644
@@ -966,7 +966,7 @@
 
 /* Downstream Port Containment */
 #define PCI_EXP_DPC_CAP			4	/* DPC Capability */
-#define PCI_EXP_DPC_IRQ			0x1f	/* DPC Interrupt Message Number */
+#define  PCI_EXP_DPC_IRQ		0x1f	/* DPC Interrupt Message Number */
 #define  PCI_EXP_DPC_CAP_RP_EXT		0x20	/* Root Port Extensions for DPC */
 #define  PCI_EXP_DPC_CAP_POISONED_TLP	0x40	/* Poisoned TLP Egress Blocking Supported */
 #define  PCI_EXP_DPC_CAP_SW_TRIGGER	0x80	/* Software Triggering Supported */
Signature · 1 line
2.13.6
author
#9 lore raw

PATCH v2 6/6 PCI/DPC: Enable ERR_COR

In reply to Keith Busch

A DPC port may be configured to send ERR_COR message when the triggered. This patch enables this feature so additional notification of the event is possible.

Signed-off-by Keith Busch <[email protected]>
---
 drivers/pci/pcie/pcie-dpc.c   | 5 ++++-
 include/uapi/linux/pci_regs.h | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)
drivers/pci/pcie/pcie-dpc.c +4 -1
index 5dfdb4935bac..b7b327367588 100644
@@ -347,7 +347,10 @@ static int dpc_probe(struct pcie_device *dev)
 
 	dpc->rp = (cap & PCI_EXP_DPC_CAP_RP_EXT);
 
-	ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_NONFATAL | PCI_EXP_DPC_CTL_INT_EN;
+	ctl = (ctl & 0xfff4) |
+		PCI_EXP_DPC_CTL_EN_NONFATAL |
+		PCI_EXP_DPC_CTL_INT_EN |
+		PCI_EXP_DPC_CTL_ERR_COR;
 	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",
include/uapi/linux/pci_regs.h +1 -0
index 295a5ebdf9e4..0bee9a99dae1 100644
@@ -976,6 +976,7 @@
 #define PCI_EXP_DPC_CTL			6	/* DPC control */
 #define  PCI_EXP_DPC_CTL_EN_NONFATAL 	0x02	/* Enable trigger on ERR_NONFATAL message */
 #define  PCI_EXP_DPC_CTL_INT_EN 	0x08	/* DPC Interrupt Enable */
+#define  PCI_EXP_DPC_CTL_ERR_COR 	0x10	/* DPC ERR_COR Enable */
 
 #define PCI_EXP_DPC_STATUS		8	/* DPC Status */
 #define  PCI_EXP_DPC_STATUS_TRIGGER	0x01	/* Trigger Status */
Signature · 1 line
2.13.6