[+cc Thomas, Rafael]
On Mon, Apr 02, 2018 at 10:21:58AM -0600, Keith Busch wrote:
From: Oza Pawandeep <poza@xxxxxxxxxxxxxx>
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 <poza@xxxxxxxxxxxxxx>
Signed-off-by: Bjorn Helgaas <helgaas@xxxxxxxxxx>
[bhelgaas: changelog]
Reviewed-by: Keith Busch <keith.busch@xxxxxxxxx>
Signed-off-by: Oza Pawandeep <poza@xxxxxxxxxxxxxx>
[changelog]
Reviewed-by: Keith Busch <keith.busch@xxxxxxxxx>
---
drivers/pci/pcie/pcie-dpc.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c
index a9be6938417f..82644245cb4d 100644
--- a/drivers/pci/pcie/pcie-dpc.c
+++ b/drivers/pci/pcie/pcie-dpc.c
@@ -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;
--
2.14.3
