| Message ID | 20180620213833.25072-1-keith.busch@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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.
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.
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.
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>
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
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; }
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(-)