| Message ID | 20180402162203.3370-3-keith.busch@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
[+cc Thomas, Rafael] On Mon, Apr 02, 2018 at 10:21:58AM -0600, Keith Busch wrote: > From: Oza Pawandeep <poza@codeaurora.org> > > 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@codeaurora.org> Signed-off-by: Bjorn Helgaas <helgaas@kernel.org> [bhelgaas: changelog] Reviewed-by: Keith Busch <keith.busch@intel.com> > Signed-off-by: Oza Pawandeep <poza@codeaurora.org> > [changelog] > Reviewed-by: Keith Busch <keith.busch@intel.com> > --- > 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 >
[+cc Thomas, Rafael for real] On Tue, Apr 03, 2018 at 03:38:47PM -0500, Bjorn Helgaas wrote: > [+cc Thomas, Rafael] > > On Mon, Apr 02, 2018 at 10:21:58AM -0600, Keith Busch wrote: > > From: Oza Pawandeep <poza@codeaurora.org> > > > > 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@codeaurora.org> > Signed-off-by: Bjorn Helgaas <helgaas@kernel.org> > [bhelgaas: changelog] > Reviewed-by: Keith Busch <keith.busch@intel.com> > > > > Signed-off-by: Oza Pawandeep <poza@codeaurora.org> > > [changelog] > > Reviewed-by: Keith Busch <keith.busch@intel.com> > > --- > > 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 > >
On Tue, 3 Apr 2018, Bjorn Helgaas wrote: > [+cc Thomas, Rafael for real] > On Tue, Apr 03, 2018 at 03:38:47PM -0500, Bjorn Helgaas wrote: > > [+cc Thomas, Rafael] > > > > On Mon, Apr 02, 2018 at 10:21:58AM -0600, Keith Busch wrote: > > > From: Oza Pawandeep <poza@codeaurora.org> > > > > > > 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. Though this could have been avoided if the driver simply would have used a threaded interrupt. The core handles that correctly for any type of interrupts, edge/level. The logic there is: low_level_handler() if (level) irq_mask(); primary_handler(); wake_thread(); if (level && !thread_pending) irq_unmask(); and the thread handler does thread_handler() thread_handler_function(); if (!thread_pending && masked && !disabled) irq_unmask(); There is a reason why threaded interrupt handlers exist.... Thanks, tglx
On Wed, Apr 04, 2018 at 10:13:32AM +0200, Thomas Gleixner wrote: > On Tue, 3 Apr 2018, Bjorn Helgaas wrote: > > On Tue, Apr 03, 2018 at 03:38:47PM -0500, Bjorn Helgaas wrote: > > > On Mon, Apr 02, 2018 at 10:21:58AM -0600, Keith Busch wrote: > > > > From: Oza Pawandeep <poza@codeaurora.org> > > > > > > > > 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. If the IRQ is shared, the other handlers are starved for a brief period of time between the IRQ being masked in the handler and unmasked in the worker. What is the recommended way to handle this? I'm asking because I'm working on patches to runtime suspend pciehp hotplug ports to D3hot. The first few Thunderbolt controllers that came to market had broken MSI signalling and are thus using INTx. If a hotplug port is signaling an interrupt while its parent(s) are in D3hot, its config space is inaccessible for the IRQ handler, so the parent(s) have to be resumed to D0 first. This takes too long to be done in an IRQ handler and I believe can also sleep, so it's done by a worker. Now the problem is, INTx interrupts may be shared by multiple devices, and they *are* on my machine. The conundrum I'm facing is to mask the IRQ and starve all the other handlers while the device is woken, or not mask it but risk getting lots of spurious interrupts. For now I've gone with the latter approach, so I leave the IRQ unmasked and return IRQ_NONE because I don't know yet if the IRQ originated from this particular hotplug port or from something else. Now if I check /proc/interrupts, I can see that about 5000 spurious interrupts were accumulated until the hotplug port's parents were woken and the interrupt could finally be handled. Is there a better way to deal with this? Just so you get an idea what I'm talking about, this is /proc/interrupts on a MacBookPro9,1 with a daisy-chain of two Light Ridge Thunderbolt controllers and one Port Ridge (all with broken MSI signaling): 16: IO-APIC 16-fasteoi pciehp 17: IO-APIC 17-fasteoi pciehp, pciehp, pciehp, mmc0, snd_hda_intel, b43 18: IO-APIC 18-fasteoi pciehp, pciehp, i801_smbus 19: IO-APIC 19-fasteoi pciehp Thanks! Lukas
On 2018-04-04 02:08, Bjorn Helgaas wrote: > [+cc Thomas, Rafael] > > On Mon, Apr 02, 2018 at 10:21:58AM -0600, Keith Busch wrote: >> From: Oza Pawandeep <poza@codeaurora.org> >> >> 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@codeaurora.org> > Signed-off-by: Bjorn Helgaas <helgaas@kernel.org> > [bhelgaas: changelog] > Reviewed-by: Keith Busch <keith.busch@intel.com> > > >> Signed-off-by: Oza Pawandeep <poza@codeaurora.org> >> [changelog] >> Reviewed-by: Keith Busch <keith.busch@intel.com> >> --- >> 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 >> Hi Bjorn, Is this change left out in 4.17 for some reason ? Regards, Oza.
On Tue, Apr 3, 2018 at 3:38 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: > > 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 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.
On Wed, May 16, 2018 at 10:16:15AM -0500, Timur Tabi wrote: > On Tue, Apr 3, 2018 at 3:38 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > 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 > > 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. I blew it, sorry. I think I was hoping to clarify the question of using threaded interrupts, but that never happened. I applied this to pci/dpc for v4.18 and pushed it. Bjorn
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;