DPC updates

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 +0 -0

index 38e40c6c576f..a9be6938417f 100644

drivers/pci/pcie/pcie-dpc.c +0 -0

 	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 0c79eac5e9b8..9cfcecdc3ec7 100644
@@ -980,6 +980,7 @@
 #define PCI_EXP_DPC_CTL			6	/* DPC control */
 #define  PCI_EXP_DPC_CTL_EN_NONFATAL 	0x0002	/* Enable trigger on ERR_NONFATAL message */
 #define  PCI_EXP_DPC_CTL_INT_EN 	0x0008	/* DPC Interrupt Enable */
+#define  PCI_EXP_DPC_CTL_ERR_COR 	0x0010	/* DPC ERR_COR Enable */
 
 #define PCI_EXP_DPC_STATUS		8	/* DPC Status */
 #define  PCI_EXP_DPC_STATUS_TRIGGER	    0x0001 /* Trigger Status */

Signature · 1 line

2.14.3

Quoted 3 lines · 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.

Who is the intended consumer of the ERR_COR message? I guess if the root port supports AER, we'll see AER logging when we didn't before? Is that what you mean by "additional notification"?

Or, since the DPC ERR_COR implementation note (sec 6.2.10.2) suggests that ERR_COR is primarily intended for use by platform firmware, does this change enable notification via the firmware? If so, how does that work? Does it require the platform to retain control of AER so it can see these events? But if the platform retains AER control, I don't think we'd be enabling DPC in Linux. I'm confused.

The similar implementation note in 6.2.10.5 suggests that DL_ACTIVE_ERR_COR is also intended for use by the platform. Should we be setting that for the same reason we're setting PCI_EXP_DPC_CTL_ERR_COR?

Quoted 16 lines · Bjorn Helgaas

Quoted 3 lines · 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.

Who is the intended consumer of the ERR_COR message? I guess if the root port supports AER, we'll see AER logging when we didn't before? Is that what you mean by "additional notification"?

Or, since the DPC ERR_COR implementation note (sec 6.2.10.2) suggests that ERR_COR is primarily intended for use by platform firmware, does this change enable notification via the firmware? If so, how does that work? Does it require the platform to retain control of AER so it can see these events? But if the platform retains AER control, I don't think we'd be enabling DPC in Linux. I'm confused.

Quoted 4 lines

The similar implementation note in 6.2.10.5 suggests that DL_ACTIVE_ERR_COR is also intended for use by the platform. Should we be setting that for the same reason we're setting PCI_EXP_DPC_CTL_ERR_COR?

I think there are various ways this could play out. I added this to the series per a request for future use, but I actually don't have an environment where I could test as intended. As I understand, it was for platform firmware that may own the root port, but not switches in an external enclosure, so the OS would own the DPC control. The ERR_COR is to notify firmware so it may note an event in a platform log.

I think you bring up a good point on DL_Active.

This should be harmless even if there are no consumers for the message, or if the OS owns the root port AER control. But I'm totally fine with dropping this one in the series, and it's not related to the rest anyway. I think I at least need to circle back with platform makers and really test the feature on capable hardware.

[+cc Rafael, linux-acpi]

Quoted 22 lines · Bjorn Helgaas

Quoted 16 lines · Keith Busch

Quoted 3 lines

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.

Who is the intended consumer of the ERR_COR message? I guess if the root port supports AER, we'll see AER logging when we didn't before? Is that what you mean by "additional notification"?

Or, since the DPC ERR_COR implementation note (sec 6.2.10.2) suggests that ERR_COR is primarily intended for use by platform firmware, does this change enable notification via the firmware? If so, how does that work? Does it require the platform to retain control of AER so it can see these events? But if the platform retains AER control, I don't think we'd be enabling DPC in Linux. I'm confused.

Quoted 4 lines

The similar implementation note in 6.2.10.5 suggests that DL_ACTIVE_ERR_COR is also intended for use by the platform. Should we be setting that for the same reason we're setting PCI_EXP_DPC_CTL_ERR_COR?

Quoted 6 lines

I think there are various ways this could play out. I added this to the series per a request for future use, but I actually don't have an environment where I could test as intended. As I understand, it was for platform firmware that may own the root port, but not switches in an external enclosure, so the OS would own the DPC control. The ERR_COR is to notify firmware so it may note an event in a platform log.

This picture doesn't make sense to me yet. Per the PCI Firmware spec, r3.2, sec 4.5.2, we use _OSC to negotiate control over AER (and now DPC). I think _OSC is only allowed directly under a host bridge device (PNP0A08 or PNP0A03), and it applies to the entire hierarchy under the host bridge.

I don't know how firmware could claim to own AER on a root port, but not on switches (external or otherwise) below that root port.

If _OSC says firmware owns AER, we won't touch AER on the root port, but we also won't touch DPC on switches below the root port. So I don't see how this change will help.

Quoted 7 lines · Bjorn Helgaas

This picture doesn't make sense to me yet. Per the PCI Firmware spec, r3.2, sec 4.5.2, we use _OSC to negotiate control over AER (and now DPC). I think _OSC is only allowed directly under a host bridge device (PNP0A08 or PNP0A03), and it applies to the entire hierarchy under the host bridge.

Quoted 6 lines

I don't know how firmware could claim to own AER on a root port, but not on switches (external or otherwise) below that root port.

If _OSC says firmware owns AER, we won't touch AER on the root port, but we also won't touch DPC on switches below the root port. So I don't see how this change will help.

This matches my understanding of that part of the spec.

From: Oza Pawandeep <[email protected]>

The DPC driver was acknowledging the DPC interrupt status in deferred work. That works for edge triggered interrupts, but causes an interrupt storm with level triggered legacy interrupts.

This patch fixes that by clearing the interrupt status in interrupt handler.

Signed-off-by Oza Pawandeep <[email protected]>

[changelog]

Reviewed-by Keith Busch <[email protected]>

---
 drivers/pci/pcie/pcie-dpc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/pci/pcie/pcie-dpc.c +4 -1

index a9be6938417f..82644245cb4d 100644
@@ -112,7 +112,7 @@ static void dpc_work(struct work_struct *work)
 	}
 
 	pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
-		PCI_EXP_DPC_STATUS_TRIGGER | PCI_EXP_DPC_STATUS_INTERRUPT);
+			      PCI_EXP_DPC_STATUS_TRIGGER);
 
 	pci_read_config_word(pdev, cap + PCI_EXP_DPC_CTL, &ctl);
 	pci_write_config_word(pdev, cap + PCI_EXP_DPC_CTL,

@@ -222,6 +222,9 @@ static irqreturn_t dpc_irq(int irq, void *context)
 	if (dpc->rp_extensions && reason == 3 && ext_reason == 0)
 		dpc_process_rp_pio_error(dpc);
 
+	pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
+			      PCI_EXP_DPC_STATUS_INTERRUPT);
+
 	schedule_work(&dpc->work);
 
 	return IRQ_HANDLED;

Signature · 1 line

2.14.3

[+cc Thomas, Rafael]

Quoted 8 lines · Keith Busch

From: Oza Pawandeep <[email protected]>

The DPC driver was acknowledging the DPC interrupt status in deferred work. That works for edge triggered interrupts, but causes an interrupt storm with level triggered legacy interrupts.

This patch fixes that by clearing the interrupt status in interrupt handler.

I'm fuzzy on this question of edge vs. level and where the interrupt should be cleared. I'd like to understand this better and include the general rule in the changelog in case I'm not the only one who is unclear on this.

Here's my understanding, gleaned from kernel/irq/chip.c and
https://notes.shichao.io/lkd/ch7/ :

The generic IRQ handling code ensures that an interrupt handler runs with its interrupt masked or disabled. If the interrupt is level-triggered, the interrupt handler must tell its device to stop asserting the interrupt before returning. If it doesn't, we will immediately take the interrupt again when the handler returns and the generic code unmasks the interrupt.

The driver doesn't know whether its interrupt is edge- or level-triggered, so it must clear its interrupt source directly in its interrupt handler.

I'd also like to convince myself that we don't have similar issues with other services, e.g., AER, PME, pciehp. Here are my notes about those:

1) pciehp:

    request_irq(irq, pcie_isr, ...)
    pcie_isr
      pciehp_isr
        # clear Slot Status event bits
        pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &status);
	events = status & (...)
	pcie_capability_write_word(pdev, PCI_EXP_SLTSTA, events);

  2) AER:
    request_irq(dev->irq, aer_irq, ...)
    aer_irq
      # clear AER Root Error Status bits
      pci_read_config_dword(pdev->port, pos + PCI_ERR_ROOT_STATUS, &status);
      pci_write_config_dword(pdev->port, pos + PCI_ERR_ROOT_STATUS, status);

3) DPC:

    devm_request_irq(..., dev->irq, dpc_irq, ...)
    dpc_irq
      schedule_work(<dpc_work>)
    ...
    dpc_work
      # clear DPC Interrupt Status
      pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS, PCI_EXP_DPC_STATUS_INTERRUPT);

4) PME:

    request_irq(srv->irq, pcie_pme_irq, ...)
    pcie_pme_irq
      pcie_pme_interrupt_enable(port, false);
        # clear PME Interrupt Enable
	pcie_capability_clear_word(dev, PCI_EXP_RTCTL, PCI_EXP_RTCTL_PMEIE);
      schedule_work(<pcie_pme_work_fn>)
    ...
    pcie_pme_work_fn
      # clear PME Status
      pcie_capability_set_dword(dev, PCI_EXP_RTSTA, PCI_EXP_RTSTA_PME);
      pcie_pme_interrupt_enable(port, true);
        # set PME Interrupt Enable
        pcie_capability_set_word(dev, PCI_EXP_RTCTL, PCI_EXP_RTCTL_PMEIE);

The pciehp and AER cases look OK to me. DPC looks definitely wrong, and this patch should fix it.

I *guess* PME looks OK, although I would feel better about it if it used the same strategy as the others. All of these things have both "Interrupt Enable" and "Interrupt Status" bits. PME is the only one that twiddles the Interrupt Enable in the interrupt path.

Bottom line, I think this patch is fine and I applied it with the following changelog:

PCI/DPC: Clear interrupt status in interrupt handler top half

The generic IRQ handling code ensures that an interrupt handler runs with its interrupt masked or disabled. If the interrupt is level-triggered, the interrupt handler must tell its device to stop asserting the interrupt before returning. If it doesn't, we will immediately take the interrupt again when the handler returns and the generic code unmasks the interrupt.

The driver doesn't know whether its interrupt is edge- or level-triggered, so it must clear its interrupt source directly in its interrupt handler.

Previously we cleared the DPC interrupt status in the bottom half, i.e., in deferred work, which can cause an interrupt storm if the DPC interrupt happens to be level-triggered, e.g., if we're using INTx instead of MSI.

Clear the DPC interrupt status bit in the interrupt handler, not in the deferred work.

Signed-off-by Oza Pawandeep <[email protected]>

Signed-off-by Bjorn Helgaas <[email protected]>

[bhelgaas: changelog]

Reviewed-by Keith Busch <[email protected]>

Quoted 33 lines

Signed-off-by Oza Pawandeep <[email protected]>

[changelog]

Reviewed-by Keith Busch <[email protected]>

---
 drivers/pci/pcie/pcie-dpc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/pci/pcie/pcie-dpc.c +4 -1

index a9be6938417f..82644245cb4d 100644
@@ -112,7 +112,7 @@ static void dpc_work(struct work_struct *work)
 	}
 
 	pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
-		PCI_EXP_DPC_STATUS_TRIGGER | PCI_EXP_DPC_STATUS_INTERRUPT);
+			      PCI_EXP_DPC_STATUS_TRIGGER);
 
 	pci_read_config_word(pdev, cap + PCI_EXP_DPC_CTL, &ctl);
 	pci_write_config_word(pdev, cap + PCI_EXP_DPC_CTL,

@@ -222,6 +222,9 @@ static irqreturn_t dpc_irq(int irq, void *context)
 	if (dpc->rp_extensions && reason == 3 && ext_reason == 0)
 		dpc_process_rp_pio_error(dpc);
 
+	pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
+			      PCI_EXP_DPC_STATUS_INTERRUPT);
+
 	schedule_work(&dpc->work);
 
 	return IRQ_HANDLED;

Signature · 1 line

2.14.3

[+cc Thomas, Rafael for real]

Quoted 8 lines · Bjorn Helgaas

[+cc Thomas, Rafael]

Quoted 5 lines · Keith Busch

From: Oza Pawandeep <[email protected]>

The DPC driver was acknowledging the DPC interrupt status in deferred work. That works for edge triggered interrupts, but causes an interrupt storm with level triggered legacy interrupts.

Quoted 4 lines · Bjorn Helgaas

Quoted 3 lines · Bjorn Helgaas

Quoted 2 lines · Keith Busch

From: Oza Pawandeep <[email protected]>

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

The DPC driver was acknowledging the DPC interrupt status in deferred work. That works for edge triggered interrupts, but causes an interrupt storm with level triggered legacy interrupts.

The problem is homebrewn in the driver. So, yes it needs to mask the interrupt before returning from the irq handler if the rest of the magic is done in a worker.

[+cc Thomas, Rafael]

Quoted 8 lines · Keith Busch

From: Oza Pawandeep <[email protected]>

The DPC driver was acknowledging the DPC interrupt status in deferred work. That works for edge triggered interrupts, but causes an interrupt storm with level triggered legacy interrupts.

This patch fixes that by clearing the interrupt status in interrupt handler.

I'm fuzzy on this question of edge vs. level and where the interrupt should be cleared. I'd like to understand this better and include the general rule in the changelog in case I'm not the only one who is unclear on this.

Here's my understanding, gleaned from kernel/irq/chip.c and
https://notes.shichao.io/lkd/ch7/ :

The generic IRQ handling code ensures that an interrupt handler runs with its interrupt masked or disabled. If the interrupt is level-triggered, the interrupt handler must tell its device to stop asserting the interrupt before returning. If it doesn't, we will immediately take the interrupt again when the handler returns and the generic code unmasks the interrupt.

The driver doesn't know whether its interrupt is edge- or level-triggered, so it must clear its interrupt source directly in its interrupt handler.

I'd also like to convince myself that we don't have similar issues with other services, e.g., AER, PME, pciehp. Here are my notes about those:

1) pciehp:

    request_irq(irq, pcie_isr, ...)
    pcie_isr
      pciehp_isr
        # clear Slot Status event bits
        pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &status);
	events = status & (...)
	pcie_capability_write_word(pdev, PCI_EXP_SLTSTA, events);

2) AER:

    request_irq(dev->irq, aer_irq, ...)
    aer_irq
      # clear AER Root Error Status bits
      pci_read_config_dword(pdev->port, pos + PCI_ERR_ROOT_STATUS, 
&status);
      pci_write_config_dword(pdev->port, pos + PCI_ERR_ROOT_STATUS, 

status);

3) DPC:
devm_request_irq(..., dev->irq, dpc_irq, ...) dpc_irq
schedule_work(<dpc_work>)
...
dpc_work
# clear DPC Interrupt Status pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
PCI_EXP_DPC_STATUS_INTERRUPT);

4) PME:

    request_irq(srv->irq, pcie_pme_irq, ...)
    pcie_pme_irq
      pcie_pme_interrupt_enable(port, false);
        # clear PME Interrupt Enable
	pcie_capability_clear_word(dev, PCI_EXP_RTCTL, PCI_EXP_RTCTL_PMEIE);
      schedule_work(<pcie_pme_work_fn>)
    ...
    pcie_pme_work_fn
      # clear PME Status
      pcie_capability_set_dword(dev, PCI_EXP_RTSTA, PCI_EXP_RTSTA_PME);
      pcie_pme_interrupt_enable(port, true);
        # set PME Interrupt Enable
        pcie_capability_set_word(dev, PCI_EXP_RTCTL, 

PCI_EXP_RTCTL_PMEIE);

The pciehp and AER cases look OK to me. DPC looks definitely wrong, and this patch should fix it.

I *guess* PME looks OK, although I would feel better about it if it used the same strategy as the others. All of these things have both "Interrupt Enable" and "Interrupt Status" bits. PME is the only one that twiddles the Interrupt Enable in the interrupt path.

Bottom line, I think this patch is fine and I applied it with the following changelog:

PCI/DPC: Clear interrupt status in interrupt handler top half

The generic IRQ handling code ensures that an interrupt handler runs
with
its interrupt masked or disabled. If the interrupt is
level-triggered, the
interrupt handler must tell its device to stop asserting the
interrupt
before returning. If it doesn't, we will immediately take the
interrupt
again when the handler returns and the generic code unmasks the
interrupt.

The driver doesn't know whether its interrupt is edge- or
level-triggered,
so it must clear its interrupt source directly in its interrupt
handler.

Previously we cleared the DPC interrupt status in the bottom half,
i.e., in
deferred work, which can cause an interrupt storm if the DPC
interrupt
happens to be level-triggered, e.g., if we're using INTx instead of
MSI.

Clear the DPC interrupt status bit in the interrupt handler, not in
the
deferred work.

Signed-off-by Oza Pawandeep <[email protected]>

Signed-off-by Bjorn Helgaas <[email protected]>

[bhelgaas: changelog]

Reviewed-by Keith Busch <[email protected]>

Quoted 29 lines

Signed-off-by Oza Pawandeep <[email protected]>

[changelog]

Reviewed-by Keith Busch <[email protected]>

---
 drivers/pci/pcie/pcie-dpc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/pci/pcie/pcie-dpc.c +4 -1

index a9be6938417f..82644245cb4d 100644
@@ -112,7 +112,7 @@ static void dpc_work(struct work_struct *work)
 	}
 
 	pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
-		PCI_EXP_DPC_STATUS_TRIGGER | PCI_EXP_DPC_STATUS_INTERRUPT);
+			      PCI_EXP_DPC_STATUS_TRIGGER);
 
 	pci_read_config_word(pdev, cap + PCI_EXP_DPC_CTL, &ctl);
 	pci_write_config_word(pdev, cap + PCI_EXP_DPC_CTL,

@@ -222,6 +222,9 @@ static irqreturn_t dpc_irq(int irq, void *context)
 	if (dpc->rp_extensions && reason == 3 && ext_reason == 0)
 		dpc_process_rp_pio_error(dpc);
 
+	pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
+			      PCI_EXP_DPC_STATUS_INTERRUPT);
+
 	schedule_work(&dpc->work);
 

Quoted 4 lines

drivers/pci/pcie/pcie-dpc.c +0 -1

 	return IRQ_HANDLED;
--

2.14.3

Bottom line, I think this patch is fine and I applied it with the following changelog:

PCI/DPC: Clear interrupt status in interrupt handler top half

Quoted 4 lines · Bjorn Helgaas

Bottom line, I think this patch is fine and I applied it with the following changelog:

Quoted 1 line

PCI/DPC: Clear interrupt status in interrupt handler top half

Bjorn,

I can't find this patch (with either subject line) anywhere on
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/ or linux-next. I see you pulled in Oza's other patch set, but this patch is still MIA.

Sent: 02 April 2018 17:22
To: Linux PCI; Bjorn Helgaas

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

drivers/pci/pcie/aer/aerdrv_core.c +2 -2

index a4bfea52e7d4..4fb24003cac3 100644
@@ -638,7 +638,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
  *

@@ -646,7 +646,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;
 

@@ -705,11 +705,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++) {

Should there be an EXPORT_SYMBOL(aer_get_device_error_info) here? (preferable without _GPL).

David

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 | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

drivers/pci/pcie/pcie-dpc.c +1 -0

index e12837ee4f1c..76f963a5089e 100644
@@ -85,6 +85,7 @@ static void dpc_wait_link_inactive(struct dpc_dev 
 *dpc)
 
 static void dpc_work(struct work_struct *work)
 {
+	struct aer_err_info info;
 	struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
 	struct pci_dev *dev, *temp, *pdev = dpc->dev->port;

drivers/pci/pcie/pcie-dpc.c +0 -0

 	struct pci_bus *parent = pdev->subordinate;

@@ -108,8 +109,12 @@ static void dpc_work(struct work_struct *work)
 		 (ext_reason == 1) ? "software trigger" :
 				     "reserved error");
 

Quoted 27 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 | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

drivers/pci/pcie/pcie-dpc.c +1 -0

index e12837ee4f1c..76f963a5089e 100644
@@ -85,6 +85,7 @@ static void dpc_wait_link_inactive(struct dpc_dev 
 *dpc)
 
 static void dpc_work(struct work_struct *work)
 {
+	struct aer_err_info info;
 	struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
 	struct pci_dev *dev, *temp, *pdev = dpc->dev->port;

drivers/pci/pcie/pcie-dpc.c +0 -0

 	struct pci_bus *parent = pdev->subordinate;

@@ -108,8 +109,12 @@ static void dpc_work(struct work_struct *work)
 		 (ext_reason == 1) ? "software trigger" :
 				     "reserved error");
 

drivers/pci/pcie/pcie-dpc.c static void dpc_work(struct work_struct *work) · line 120

Quoted 10 lines

drivers/pci/pcie/pcie-dpc.c +5 -1

-	if (dpc->rp_extensions && reason == 3 && ext_reason == 0)
+	if (dpc->rp_extensions && reason == 3 && ext_reason == 0) {
 		pio_status = dpc_process_rp_pio_error(dpc);
+	} else if (reason == 0 && aer_get_device_error_info(pdev, &info)) {
+		aer_print_error(pdev, &info);
+		pci_cleanup_aer_uncorrect_error_status(pdev);
+	}
 
 	pci_lock_rescan_remove();
 	list_for_each_entry_safe_reverse(dev, temp, &parent->devices,

I am not sure if this series is in pursuit. but was wondering why do we need to clear aer status in DPC (when DPC is triggered)? because

6.2.10 for Downstream Port Containment:

When DPC is triggered due to receipt of an uncorrectable error
Message,
the Requester ID from the Message is recorded in the DPC Error Source ID register and that Message is discarded and not forwarded Upstream. When DPC is triggered by an unmasked uncorrectable error, that error will not be signaled with an uncorrectable error Message, even if otherwise enabled.

so when DPC is active, the msg is discarded and not forwarded to upstream. which means that we should find AER status set in RP or Switch.

Regards,
Oza.

I am terribly sorry about the delay getting this back to you. I had some scheduling issues with test equipment and other unrelated conflicts.

I added Oza's legacy IRQ fix into this series, as I didn't see this staged yet, and it really does fix some issues as well as provide the means to clean up other parts.

I'm sending this later than I intended, but hoping you might see a way to including this to 4.17. I can rebase to 'next' where the file is renamed to 'dpc.c'.

Keith Busch (6):
PCI/DPC: Enable ERR_COR
PCI/DPC: Leave interrupts enabled while handling event PCI/DPC: Defer event handling to work queue PCI/DPC: Remove rp_pio_status from dpc struct PCI/AER: API for obtaining AER information PCI/DPC: Print AER status in DPC event handling

Oza Pawandeep (1):
PCI/DPC: Fix PCI legacy interrupt acknowledgement

 drivers/pci/pcie/aer/aerdrv.h      |   1 +
 drivers/pci/pcie/aer/aerdrv_core.c |   8 +--
 drivers/pci/pcie/pcie-dpc.c        | 106 +++++++++++++++++--------------------
 include/uapi/linux/pci_regs.h      |   1 +
 4 files changed, 55 insertions(+), 61 deletions(-)