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

Message ID 20180620213833.25072-1-keith.busch@intel.com
State Accepted
Delegated to: Bjorn Helgaas
Headers show
Series
  • [1/7] PCI/DPC: Leave interrupts enabled while handling event
Related show

Commit Message

Keith Busch June 20, 2018, 9:38 p.m.
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(-)

Comments

Sinan Kaya June 20, 2018, 9:42 p.m. | #1
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.
Keith Busch June 20, 2018, 9:54 p.m. | #2
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.
Sinan Kaya June 20, 2018, 9:59 p.m. | #3
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.
Oza Pawandeep June 22, 2018, 5:26 a.m. | #4
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>
Bjorn Helgaas July 16, 2018, 10:07 p.m. | #5
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

Patch

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