[PATCH 1/7] PCI/DPC: Leave interrupts enabled while handling event

[PATCH 1/7] PCI/DPC: Leave interrupts enabled while handling event

[Keith Busch]

Now that the DPC driver clears the interrupt status before exiting the
irq handler, we don't need to abuse the DPC control register to know if
a shared interrupt is for a new DPC event: a DPC port can not trigger
a second interrupt until the host clears the trigger status later in the
work queue handler.

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 drivers/pci/pcie/dpc.c | 23 +++++------------------
 1 file changed, 5 insertions(+), 18 deletions(-)

diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 921ed979109d..972aac892846 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -77,7 +77,7 @@ static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
 	struct dpc_dev *dpc;
 	struct pcie_device *pciedev;
 	struct device *devdpc;
-	u16 cap, ctl;
+	u16 cap;
 
 	/*
 	 * DPC disables the Link automatically in hardware, so it has
@@ -105,10 +105,6 @@ static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
 	pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
 			      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,
-			      ctl | PCI_EXP_DPC_CTL_INT_EN);
-
 	return PCI_ERS_RESULT_RECOVERED;
 }
 
@@ -183,16 +179,11 @@ static irqreturn_t dpc_irq(int irq, void *context)
 	struct dpc_dev *dpc = (struct dpc_dev *)context;
 	struct pci_dev *pdev = dpc->dev->port;
 	struct device *dev = &dpc->dev->device;
-	u16 cap = dpc->cap_pos, ctl, status, source, reason, ext_reason;
-
-	pci_read_config_word(pdev, cap + PCI_EXP_DPC_CTL, &ctl);
-
-	if (!(ctl & PCI_EXP_DPC_CTL_INT_EN) || ctl == (u16)(~0))
-		return IRQ_NONE;
+	u16 cap = dpc->cap_pos, status, source, reason, ext_reason;
 
 	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
 
-	if (!(status & PCI_EXP_DPC_STATUS_INTERRUPT))
+	if (!(status & PCI_EXP_DPC_STATUS_INTERRUPT) || status == (u16)(~0))
 		return IRQ_NONE;
 
 	if (!(status & PCI_EXP_DPC_STATUS_TRIGGER)) {
@@ -201,9 +192,6 @@ static irqreturn_t dpc_irq(int irq, void *context)
 		return IRQ_HANDLED;
 	}
 
-	pci_write_config_word(pdev, cap + PCI_EXP_DPC_CTL,
-			      ctl & ~PCI_EXP_DPC_CTL_INT_EN);
-
 	pci_read_config_word(pdev, cap + PCI_EXP_DPC_SOURCE_ID,
 			     &source);
 
@@ -226,9 +214,8 @@ static irqreturn_t dpc_irq(int irq, void *context)
 
 	pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
 			      PCI_EXP_DPC_STATUS_INTERRUPT);
-
-	schedule_work(&dpc->work);
-
+	if (status & PCI_EXP_DPC_STATUS_TRIGGER)
+		schedule_work(&dpc->work);
 	return IRQ_HANDLED;
 }
 
-- 
2.14.3

[PATCH 2/7] PCI/DPC: Defer event handling to work queue

[Keith Busch]

This moves all event handling to the existing work queue, which will
make passing event information simpler to the handler.

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 drivers/pci/pcie/dpc.c | 44 ++++++++++++++++++++------------------------
 1 file changed, 20 insertions(+), 24 deletions(-)

diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 972aac892846..5f5fac279c75 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -108,14 +108,6 @@ static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
 	return PCI_ERS_RESULT_RECOVERED;
 }
 
-static void dpc_work(struct work_struct *work)
-{
-	struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
-	struct pci_dev *pdev = dpc->dev->port;
-
-	/* We configure DPC so it only triggers on ERR_FATAL */
-	pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_DPC);
-}
 
 static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
 {
@@ -174,33 +166,21 @@ static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
 	}
 }
 
-static irqreturn_t dpc_irq(int irq, void *context)
+static void dpc_work(struct work_struct *work)
 {
-	struct dpc_dev *dpc = (struct dpc_dev *)context;
+	struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
 	struct pci_dev *pdev = dpc->dev->port;
 	struct device *dev = &dpc->dev->device;
 	u16 cap = dpc->cap_pos, status, source, reason, ext_reason;
 
 	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
-
-	if (!(status & PCI_EXP_DPC_STATUS_INTERRUPT) || status == (u16)(~0))
-		return IRQ_NONE;
-
-	if (!(status & PCI_EXP_DPC_STATUS_TRIGGER)) {
-		pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
-				      PCI_EXP_DPC_STATUS_INTERRUPT);
-		return IRQ_HANDLED;
-	}
-
-	pci_read_config_word(pdev, cap + PCI_EXP_DPC_SOURCE_ID,
-			     &source);
+	pci_read_config_word(pdev, cap + PCI_EXP_DPC_SOURCE_ID, &source);
 
 	dev_info(dev, "DPC containment event, status:%#06x source:%#06x\n",
-		status, source);
+		 status, source);
 
 	reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN) >> 1;
 	ext_reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT) >> 5;
-
 	dev_warn(dev, "DPC %s detected, remove downstream devices\n",
 		 (reason == 0) ? "unmasked uncorrectable error" :
 		 (reason == 1) ? "ERR_NONFATAL" :
@@ -208,10 +188,26 @@ static irqreturn_t dpc_irq(int irq, void *context)
 		 (ext_reason == 0) ? "RP PIO error" :
 		 (ext_reason == 1) ? "software trigger" :
 				     "reserved error");
+
 	/* show RP PIO error detail information */
 	if (dpc->rp_extensions && reason == 3 && ext_reason == 0)
 		dpc_process_rp_pio_error(dpc);
 
+	/* We configure DPC so it only triggers on ERR_FATAL */
+	pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_DPC);
+}
+
+static irqreturn_t dpc_irq(int irq, void *context)
+{
+	struct dpc_dev *dpc = (struct dpc_dev *)context;
+	struct pci_dev *pdev = dpc->dev->port;
+	u16 cap = dpc->cap_pos, status;
+
+	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
+
+	if (!(status & PCI_EXP_DPC_STATUS_INTERRUPT) || status == (u16)(~0))
+		return IRQ_NONE;
+
 	pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
 			      PCI_EXP_DPC_STATUS_INTERRUPT);
 	if (status & PCI_EXP_DPC_STATUS_TRIGGER)
-- 
2.14.3

Re: [PATCH 2/7] PCI/DPC: Defer event handling to work queue

[Christoph Hellwig]

On Wed, Jun 20, 2018 at 03:38:28PM -0600, Keith Busch wrote:
> This moves all event handling to the existing work queue, which will
> make passing event information simpler to the handler.

I wonder if just using a threaded interrupt would be useful for
the DPC handler?

Re: [PATCH 2/7] PCI/DPC: Defer event handling to work queue

[Christoph Hellwig]

On Thu, Jun 21, 2018 at 12:27:27AM -0700, Christoph Hellwig wrote:
> On Wed, Jun 20, 2018 at 03:38:28PM -0600, Keith Busch wrote:
> > This moves all event handling to the existing work queue, which will
> > make passing event information simpler to the handler.
> 
> I wonder if just using a threaded interrupt would be useful for
> the DPC handler?

Ok, I should have read until the end of the thread first..

Re: [PATCH 2/7] PCI/DPC: Defer event handling to work queue

[Keith Busch]

On Thu, Jun 21, 2018 at 12:28:19AM -0700, Christoph Hellwig wrote:
> On Thu, Jun 21, 2018 at 12:27:27AM -0700, Christoph Hellwig wrote:
> > On Wed, Jun 20, 2018 at 03:38:28PM -0600, Keith Busch wrote:
> > > This moves all event handling to the existing work queue, which will
> > > make passing event information simpler to the handler.
> > 
> > I wonder if just using a threaded interrupt would be useful for
> > the DPC handler?
> 
> Ok, I should have read until the end of the thread first..

It's still good to know the series was on the right path. :)

[PATCH 3/7] PCI/DPC: Remove rp_pio_status from dpc struct

[Keith Busch]

We don't need to save the rp pio status across multiple contexts as all
DPC event handling occurs in a single work queue context.

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 drivers/pci/pcie/dpc.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 5f5fac279c75..1b0b25ba947c 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -19,7 +19,6 @@ struct dpc_dev {
 	struct work_struct	work;
 	u16			cap_pos;
 	bool			rp_extensions;
-	u32			rp_pio_status;
 	u8			rp_log_size;
 };
 
@@ -96,11 +95,6 @@ static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
 
 	if (dpc->rp_extensions && dpc_wait_rp_inactive(dpc))
 		return PCI_ERS_RESULT_DISCONNECT;
-	if (dpc->rp_extensions && dpc->rp_pio_status) {
-		pci_write_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS,
-				       dpc->rp_pio_status);
-		dpc->rp_pio_status = 0;
-	}
 
 	pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
 			      PCI_EXP_DPC_STATUS_TRIGGER);
@@ -122,8 +116,6 @@ static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
 	dev_err(dev, "rp_pio_status: %#010x, rp_pio_mask: %#010x\n",
 		status, mask);
 
-	dpc->rp_pio_status = status;
-
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_SEVERITY, &sev);
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_SYSERROR, &syserr);
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_EXCEPTION, &exc);
@@ -134,15 +126,14 @@ static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
 	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &dpc_status);
 	first_error = (dpc_status & 0x1f00) >> 8;
 
-	status &= ~mask;
 	for (i = 0; i < ARRAY_SIZE(rp_pio_error_string); i++) {
-		if (status & (1 << i))
+		if ((status & ~mask) & (1 << i))
 			dev_err(dev, "[%2d] %s%s\n", i, rp_pio_error_string[i],
 				first_error == i ? " (First)" : "");
 	}
 
 	if (dpc->rp_log_size < 4)
-		return;
+		goto clear_status;
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_HEADER_LOG,
 			      &dw0);
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_HEADER_LOG + 4,
@@ -155,7 +146,7 @@ static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
 		dw0, dw1, dw2, dw3);
 
 	if (dpc->rp_log_size < 5)
-		return;
+		goto clear_status;
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_IMPSPEC_LOG, &log);
 	dev_err(dev, "RP PIO ImpSpec Log %#010x\n", log);
 
@@ -164,6 +155,8 @@ static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
 			cap + PCI_EXP_DPC_RP_PIO_TLPPREFIX_LOG, &prefix);
 		dev_err(dev, "TLP Prefix Header: dw%d, %#010x\n", i, prefix);
 	}
+ clear_status:
+	pci_write_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS, status);
 }
 
 static void dpc_work(struct work_struct *work)
-- 
2.14.3

[PATCH 4/7] PCI/AER: API for obtaining AER information

[Keith Busch]

This exports some commone AER functions and structures for other drivers
to use. Since this is making the function externally visible, appending
"aer_" prefix to the function name.

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 drivers/pci/pcie/aer.c | 30 +++++-------------------------
 include/linux/aer.h    | 24 ++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 25 deletions(-)

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index a2e88386af28..0a60275f0582 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -31,26 +31,6 @@
 #include "portdrv.h"
 
 #define AER_ERROR_SOURCES_MAX		100
-#define AER_MAX_MULTI_ERR_DEVICES	5	/* Not likely to have more */
-
-struct aer_err_info {
-	struct pci_dev *dev[AER_MAX_MULTI_ERR_DEVICES];
-	int error_dev_num;
-
-	unsigned int id:16;
-
-	unsigned int severity:2;	/* 0:NONFATAL | 1:FATAL | 2:COR */
-	unsigned int __pad1:5;
-	unsigned int multi_error_valid:1;
-
-	unsigned int first_error:5;
-	unsigned int __pad2:2;
-	unsigned int tlp_header_valid:1;
-
-	unsigned int status;		/* COR/UNCOR Error Status */
-	unsigned int mask;		/* COR/UNCOR Error Mask */
-	struct aer_header_log_regs tlp;	/* TLP Header */
-};
 
 struct aer_err_source {
 	unsigned int status;
@@ -547,7 +527,7 @@ static void __aer_print_error(struct pci_dev *dev,
 	}
 }
 
-static void aer_print_error(struct pci_dev *dev, struct aer_err_info *info)
+void aer_print_error(struct pci_dev *dev, struct aer_err_info *info)
 {
 	int layer, agent;
 	int id = ((dev->bus->number << 8) | dev->devfn);
@@ -876,7 +856,7 @@ EXPORT_SYMBOL_GPL(aer_recover_queue);
 #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
  *
@@ -884,7 +864,7 @@ EXPORT_SYMBOL_GPL(aer_recover_queue);
  *
  * 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;
 
@@ -942,11 +922,11 @@ static inline void aer_process_err_devices(struct aer_err_info *e_info)
 
 	/* 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(e_info->dev[i], e_info);
 	}
 }
diff --git a/include/linux/aer.h b/include/linux/aer.h
index 514bffa11dbb..58fa04cc7407 100644
--- a/include/linux/aer.h
+++ b/include/linux/aer.h
@@ -40,6 +40,27 @@ struct aer_capability_regs {
 	u16 uncor_err_source;
 };
 
+#define AER_MAX_MULTI_ERR_DEVICES	5	/* Not likely to have more */
+
+struct aer_err_info {
+	struct pci_dev *dev[AER_MAX_MULTI_ERR_DEVICES];
+	int error_dev_num;
+
+	unsigned int id:16;
+
+	unsigned int severity:2;	/* 0:NONFATAL | 1:FATAL | 2:COR */
+	unsigned int __pad1:5;
+	unsigned int multi_error_valid:1;
+
+	unsigned int first_error:5;
+	unsigned int __pad2:2;
+	unsigned int tlp_header_valid:1;
+
+	unsigned int status;		/* COR/UNCOR Error Status */
+	unsigned int mask;		/* COR/UNCOR Error Mask */
+	struct aer_header_log_regs tlp;	/* TLP Header */
+};
+
 #if defined(CONFIG_PCIEAER)
 /* PCIe port driver needs this function to enable AER */
 int pci_enable_pcie_error_reporting(struct pci_dev *dev);
@@ -65,6 +86,9 @@ static inline int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
 }
 #endif
 
+int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info);
+void aer_print_error(struct pci_dev *dev, struct aer_err_info *info);
+
 void cper_print_aer(struct pci_dev *dev, int aer_severity,
 		    struct aer_capability_regs *aer);
 int cper_severity_to_aer(int cper_severity);
-- 
2.14.3

[PATCH 5/7] PCI/DPC: Print AER status in DPC event handling

[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 <keith.busch@intel.com>
---
 drivers/pci/pcie/dpc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 1b0b25ba947c..f6098dd171f3 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -6,6 +6,7 @@
  * Copyright (C) 2016 Intel Corp.
  */
 
+#include <linux/aer.h>
 #include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/init.h>
@@ -161,6 +162,7 @@ static void dpc_process_rp_pio_error(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 *pdev = dpc->dev->port;
 	struct device *dev = &dpc->dev->device;
@@ -185,6 +187,10 @@ static void dpc_work(struct work_struct *work)
 	/* show RP PIO error detail information */
 	if (dpc->rp_extensions && reason == 3 && ext_reason == 0)
 		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);
+	}
 
 	/* We configure DPC so it only triggers on ERR_FATAL */
 	pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_DPC);
-- 
2.14.3

Re: [PATCH 5/7] PCI/DPC: Print AER status in DPC event handling

[poza]

On 2018-06-21 03:08, Keith Busch wrote:
> 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 <keith.busch@intel.com>
> ---
>  drivers/pci/pcie/dpc.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index 1b0b25ba947c..f6098dd171f3 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -6,6 +6,7 @@
>   * Copyright (C) 2016 Intel Corp.
>   */
> 
> +#include <linux/aer.h>
>  #include <linux/delay.h>
>  #include <linux/interrupt.h>
>  #include <linux/init.h>
> @@ -161,6 +162,7 @@ static void dpc_process_rp_pio_error(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 *pdev = dpc->dev->port;
>  	struct device *dev = &dpc->dev->device;
> @@ -185,6 +187,10 @@ static void dpc_work(struct work_struct *work)
>  	/* show RP PIO error detail information */
>  	if (dpc->rp_extensions && reason == 3 && ext_reason == 0)
>  		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);

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.

Inst the message is discarded and not forwarded to upstream.
which means that we should not find AER status set in RP or Switch.
in other words, at time either we will find DPC or AER triggered but not 
both at the same time.
then when DPC is triggered why do we need to 
pci_cleanup_aer_uncorrect_error_status(pdev); ?

Regards,
Oza.


> +	}
> 
>  	/* We configure DPC so it only triggers on ERR_FATAL */
>  	pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_DPC);

Re: [PATCH 5/7] PCI/DPC: Print AER status in DPC event handling

[Keith Busch]

On Thu, Jun 21, 2018 at 02:46:10PM +0530, poza@codeaurora.org wrote:
> On 2018-06-21 03:08, Keith Busch wrote:
> > @@ -185,6 +187,10 @@ static void dpc_work(struct work_struct *work)
> >  	/* show RP PIO error detail information */
> >  	if (dpc->rp_extensions && reason == 3 && ext_reason == 0)
> >  		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);
> 
> 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.
> 
> Inst the message is discarded and not forwarded to upstream.
> which means that we should not find AER status set in RP or Switch.
> in other words, at time either we will find DPC or AER triggered but not
> both at the same time.
> then when DPC is triggered why do we need to
> pci_cleanup_aer_uncorrect_error_status(pdev); ?

According to the sequence diagram in 6.2.5, an uncorrectable error has
the cooresponding bits set in the Device Status and AER Uncorrectable
Error Status registers before DPC specifics are considered. DPC just
suppresses the ERR_[NON]FATAL messages, but the detecting ports AER
status, if implemented, should reflect what occured.

Re: [PATCH 5/7] PCI/DPC: Print AER status in DPC event handling

[poza]

On 2018-06-21 19:35, Keith Busch wrote:
> On Thu, Jun 21, 2018 at 02:46:10PM +0530, poza@codeaurora.org wrote:
>> On 2018-06-21 03:08, Keith Busch wrote:
>> > @@ -185,6 +187,10 @@ static void dpc_work(struct work_struct *work)
>> >  	/* show RP PIO error detail information */
>> >  	if (dpc->rp_extensions && reason == 3 && ext_reason == 0)
>> >  		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);
>> 
>> 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.
>> 
>> Inst the message is discarded and not forwarded to upstream.
>> which means that we should not find AER status set in RP or Switch.
>> in other words, at time either we will find DPC or AER triggered but 
>> not
>> both at the same time.
>> then when DPC is triggered why do we need to
>> pci_cleanup_aer_uncorrect_error_status(pdev); ?
> 
> According to the sequence diagram in 6.2.5, an uncorrectable error has
> the cooresponding bits set in the Device Status and AER Uncorrectable
> Error Status registers before DPC specifics are considered. DPC just
> suppresses the ERR_[NON]FATAL messages, but the detecting ports AER
> status, if implemented, should reflect what occured.

Okay, I see. Thanks for clarifying it.

Regards,
Oza.

Re: [PATCH 5/7] PCI/DPC: Print AER status in DPC event handling

[poza]

On 2018-06-21 19:35, Keith Busch wrote:
> On Thu, Jun 21, 2018 at 02:46:10PM +0530, poza@codeaurora.org wrote:
>> On 2018-06-21 03:08, Keith Busch wrote:
>> > @@ -185,6 +187,10 @@ static void dpc_work(struct work_struct *work)
>> >  	/* show RP PIO error detail information */
>> >  	if (dpc->rp_extensions && reason == 3 && ext_reason == 0)
>> >  		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);
>> 
>> 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.
>> 
>> Inst the message is discarded and not forwarded to upstream.
>> which means that we should not find AER status set in RP or Switch.
>> in other words, at time either we will find DPC or AER triggered but 
>> not
>> both at the same time.
>> then when DPC is triggered why do we need to
>> pci_cleanup_aer_uncorrect_error_status(pdev); ?
> 
> According to the sequence diagram in 6.2.5, an uncorrectable error has
> the cooresponding bits set in the Device Status and AER Uncorrectable
> Error Status registers before DPC specifics are considered. DPC just
> suppresses the ERR_[NON]FATAL messages, but the detecting ports AER
> status, if implemented, should reflect what occured.

Hi Keith,

was thinking that current code
pcie_do_fatal_recovery already does call

if ((service == PCIE_PORT_SERVICE_AER) &&
	    (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)) {
		/*
		 * If the error is reported by a bridge, we think this error
		 * is related to the downstream link of the bridge, so we
		 * do error recovery on all subordinates of the bridge instead
		 * of the bridge and clear the error status of the bridge.
		 */
		pci_cleanup_aer_uncorrect_error_status(dev);
	}


instead of calling it here in dpc driver, can we make use of that 
existing call ?
probably we just might need to remove
if ((service == PCIE_PORT_SERVICE_AER) condition

Regards,
Oza.

Re: [PATCH 5/7] PCI/DPC: Print AER status in DPC event handling

[Keith Busch]

On Fri, Jun 22, 2018 at 03:41:50PM +0530, poza@codeaurora.org wrote:
> was thinking that current code
> pcie_do_fatal_recovery already does call
> 
> if ((service == PCIE_PORT_SERVICE_AER) &&
> 	    (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)) {
> 		/*
> 		 * If the error is reported by a bridge, we think this error
> 		 * is related to the downstream link of the bridge, so we
> 		 * do error recovery on all subordinates of the bridge instead
> 		 * of the bridge and clear the error status of the bridge.
> 		 */
> 		pci_cleanup_aer_uncorrect_error_status(dev);
> 	}
> 
> 
> instead of calling it here in dpc driver, can we make use of that existing
> call ?
> probably we just might need to remove
> if ((service == PCIE_PORT_SERVICE_AER) condition

That's really only desirable when DPC error status is 0. It should be
harmless, though, so your update is fine with me.

[PATCH 6/7] PCI/DPC: Use threaded IRQ for bottom half handling

[Keith Busch]

This patch removes the work struct that was being used to handle a DPC
event and uses a threaded IRQ instead for the purpose.

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 drivers/pci/pcie/dpc.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index f6098dd171f3..348b59954961 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -17,7 +17,6 @@
 
 struct dpc_dev {
 	struct pcie_device	*dev;
-	struct work_struct	work;
 	u16			cap_pos;
 	bool			rp_extensions;
 	u8			rp_log_size;
@@ -160,10 +159,10 @@ static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
 	pci_write_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS, status);
 }
 
-static void dpc_work(struct work_struct *work)
+static irqreturn_t dpc_handler(int irq, void *context)
 {
 	struct aer_err_info info;
-	struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
+	struct dpc_dev *dpc = (struct dpc_dev *)context;
 	struct pci_dev *pdev = dpc->dev->port;
 	struct device *dev = &dpc->dev->device;
 	u16 cap = dpc->cap_pos, status, source, reason, ext_reason;
@@ -194,6 +193,8 @@ static void dpc_work(struct work_struct *work)
 
 	/* We configure DPC so it only triggers on ERR_FATAL */
 	pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_DPC);
+
+	return IRQ_HANDLED;
 }
 
 static irqreturn_t dpc_irq(int irq, void *context)
@@ -210,7 +211,7 @@ static irqreturn_t dpc_irq(int irq, void *context)
 	pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
 			      PCI_EXP_DPC_STATUS_INTERRUPT);
 	if (status & PCI_EXP_DPC_STATUS_TRIGGER)
-		schedule_work(&dpc->work);
+		return IRQ_WAKE_THREAD;
 	return IRQ_HANDLED;
 }
 
@@ -232,11 +233,11 @@ static int dpc_probe(struct pcie_device *dev)
 
 	dpc->cap_pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DPC);
 	dpc->dev = dev;
-	INIT_WORK(&dpc->work, dpc_work);
 	set_service_data(dev, dpc);
 
-	status = devm_request_irq(device, dev->irq, dpc_irq, IRQF_SHARED,
-				  "pcie-dpc", 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);
-- 
2.14.3

Re: [PATCH 6/7] PCI/DPC: Use threaded IRQ for bottom half handling

[Christoph Hellwig]

> -	struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
> +	struct dpc_dev *dpc = (struct dpc_dev *)context;

No need to cast a void pointer.

[PATCH 7/7] PCI/DPC: Remove indirection waiting for inactive link

[Keith Busch]

This patch just simplifies waiting for the contained link to become
inactive, removing the indirection to a unnecessary DPC specific handler.

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 drivers/pci/pcie/dpc.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 348b59954961..921e68c461bd 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -64,18 +64,12 @@ static int dpc_wait_rp_inactive(struct dpc_dev *dpc)
 	return 0;
 }
 
-static void dpc_wait_link_inactive(struct dpc_dev *dpc)
-{
-	struct pci_dev *pdev = dpc->dev->port;
-
-	pcie_wait_for_link(pdev, false);
-}
-
 static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
 {
 	struct dpc_dev *dpc;
 	struct pcie_device *pciedev;
 	struct device *devdpc;
+
 	u16 cap;
 
 	/*
@@ -91,7 +85,7 @@ static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
 	 * Wait until the Link is inactive, then clear DPC Trigger Status
 	 * to allow the Port to leave DPC.
 	 */
-	dpc_wait_link_inactive(dpc);
+	pcie_wait_for_link(pdev, false);
 
 	if (dpc->rp_extensions && dpc_wait_rp_inactive(dpc))
 		return PCI_ERS_RESULT_DISCONNECT;
-- 
2.14.3

Re: [PATCH 1/7] PCI/DPC: Leave interrupts enabled while handling event

[Sinan Kaya]

On 6/20/2018 5:38 PM, Keith Busch wrote:
> Now that the DPC driver clears the interrupt status before exiting the
> irq handler, we don't need to abuse the DPC control register to know if
> a shared interrupt is for a new DPC event: a DPC port can not trigger
> a second interrupt until the host clears the trigger status later in the
> work queue handler.
> 
> Signed-off-by: Keith Busch <keith.busch@intel.com>

Isn't this a problem with legacy interrupts on the root ports with no MSI?
(can be tested with pci=nomsi)

DPC interrupt handler gets called for all service driver interrupts like AER, PME and
HP.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 1/7] PCI/DPC: Leave interrupts enabled while handling event

[Keith Busch]

On Wed, Jun 20, 2018 at 05:42:51PM -0400, Sinan Kaya wrote:
> On 6/20/2018 5:38 PM, Keith Busch wrote:
> > Now that the DPC driver clears the interrupt status before exiting the
> > irq handler, we don't need to abuse the DPC control register to know if
> > a shared interrupt is for a new DPC event: a DPC port can not trigger
> > a second interrupt until the host clears the trigger status later in the
> > work queue handler.
> > 
> > Signed-off-by: Keith Busch <keith.busch@intel.com>
> 
> Isn't this a problem with legacy interrupts on the root ports with no MSI?
> (can be tested with pci=nomsi)
> 
> DPC interrupt handler gets called for all service driver interrupts like AER, PME and
> HP.

According to PCIe spec, the DPC level-triggered interrupt requires
these:

  The Interrupt Disable bit in Comand register is 0b
  The value of DPC Interrupt Enable bit is 1b
  The value of the DPC Interrupt Status bit is 1b

We now clear DPC Interrupt Status bit prior to exiting the IRQ handler,
so that should satisfy clearing INTx messages, making the DPC Interrupt
Enable control toggling redundant. We weren't doing that before, so this
would have been a problem back then.

If a shared interrupt occurs for another service (say, AER) before DPC's
bottom half handler runs, the DPC Interrupt Status won't be set, so the
DPC driver will return IRQ_NONE.

Re: [PATCH 1/7] PCI/DPC: Leave interrupts enabled while handling event

[Sinan Kaya]

On 6/20/2018 5:54 PM, Keith Busch wrote:
> If a shared interrupt occurs for another service (say, AER) before DPC's
> bottom half handler runs, the DPC Interrupt Status won't be set, so the
> DPC driver will return IRQ_NONE.

OK. This is what I was after. I saw that you removed a bunch of checks.
I was worried that status check was gone too.

Reviewed-by: Sinan Kaya <okaya@kernel.org>

for the series.

P.S. please use my new email address: okaya@kernel.org moving forward.
I'll lose access to okaya@codeaurora.org email address in a month.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 1/7] PCI/DPC: Leave interrupts enabled while handling event

[poza]

On 2018-06-21 03:08, Keith Busch wrote:
> Now that the DPC driver clears the interrupt status before exiting the
> irq handler, we don't need to abuse the DPC control register to know if
> a shared interrupt is for a new DPC event: a DPC port can not trigger
> a second interrupt until the host clears the trigger status later in 
> the
> work queue handler.
> 
> Signed-off-by: Keith Busch <keith.busch@intel.com>
> ---
>  drivers/pci/pcie/dpc.c | 23 +++++------------------
>  1 file changed, 5 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index 921ed979109d..972aac892846 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -77,7 +77,7 @@ static pci_ers_result_t dpc_reset_link(struct pci_dev 
> *pdev)
>  	struct dpc_dev *dpc;
>  	struct pcie_device *pciedev;
>  	struct device *devdpc;
> -	u16 cap, ctl;
> +	u16 cap;
> 
>  	/*
>  	 * DPC disables the Link automatically in hardware, so it has
> @@ -105,10 +105,6 @@ static pci_ers_result_t dpc_reset_link(struct
> pci_dev *pdev)
>  	pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
>  			      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,
> -			      ctl | PCI_EXP_DPC_CTL_INT_EN);
> -
>  	return PCI_ERS_RESULT_RECOVERED;
>  }
> 
> @@ -183,16 +179,11 @@ static irqreturn_t dpc_irq(int irq, void 
> *context)
>  	struct dpc_dev *dpc = (struct dpc_dev *)context;
>  	struct pci_dev *pdev = dpc->dev->port;
>  	struct device *dev = &dpc->dev->device;
> -	u16 cap = dpc->cap_pos, ctl, status, source, reason, ext_reason;
> -
> -	pci_read_config_word(pdev, cap + PCI_EXP_DPC_CTL, &ctl);
> -
> -	if (!(ctl & PCI_EXP_DPC_CTL_INT_EN) || ctl == (u16)(~0))
> -		return IRQ_NONE;
> +	u16 cap = dpc->cap_pos, status, source, reason, ext_reason;
> 
>  	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
> 
> -	if (!(status & PCI_EXP_DPC_STATUS_INTERRUPT))
> +	if (!(status & PCI_EXP_DPC_STATUS_INTERRUPT) || status == (u16)(~0))
>  		return IRQ_NONE;
> 
>  	if (!(status & PCI_EXP_DPC_STATUS_TRIGGER)) {
> @@ -201,9 +192,6 @@ static irqreturn_t dpc_irq(int irq, void *context)
>  		return IRQ_HANDLED;
>  	}
> 
> -	pci_write_config_word(pdev, cap + PCI_EXP_DPC_CTL,
> -			      ctl & ~PCI_EXP_DPC_CTL_INT_EN);
> -
>  	pci_read_config_word(pdev, cap + PCI_EXP_DPC_SOURCE_ID,
>  			     &source);
> 
> @@ -226,9 +214,8 @@ static irqreturn_t dpc_irq(int irq, void *context)
> 
>  	pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
>  			      PCI_EXP_DPC_STATUS_INTERRUPT);
> -
> -	schedule_work(&dpc->work);
> -
> +	if (status & PCI_EXP_DPC_STATUS_TRIGGER)
> +		schedule_work(&dpc->work);
>  	return IRQ_HANDLED;
>  }



Reviewed-by: Oza Pawandeep <poza@codeaurora.org>
<For whole series>

Re: [PATCH 1/7] PCI/DPC: Leave interrupts enabled while handling event

[Bjorn Helgaas]

On Wed, Jun 20, 2018 at 03:38:27PM -0600, Keith Busch wrote:
> Now that the DPC driver clears the interrupt status before exiting the
> irq handler, we don't need to abuse the DPC control register to know if
> a shared interrupt is for a new DPC event: a DPC port can not trigger
> a second interrupt until the host clears the trigger status later in the
> work queue handler.
> 
> Signed-off-by: Keith Busch <keith.busch@intel.com>

I applied all of these with Sinan and Oza's reviewed-by to pci/dpc for
v4.19, thanks!

I moved the AER API stuff from include/linux/aer.h to
drivers/pci/pci.h.  Let me know if that's a problem.  I didn't see a
reason to expose those details outside the PCI core.

Bjorn