On 27/10/2018 01:00, Trent Piepho wrote:
This reverts commit 8c934095fa2f ("PCI: dwc: Clear MSI interrupt status after it is handled, not before").
This is a very real race that we observed quickly after switching from 4.13 to 4.16. Using a custom PCI-e endpoint and driver, I was able to track it to the precise race and verify the fixed behavior, as will be described below.
This bug was originally fixed in 2013, in commit ca1658921b63 ("PCI: designware: Fix missing MSI IRQs") The discussion of that commit, archived in patchwork [1], is informative and worth reading.
The bug was re-added in the '8c934 commit this is reverting, which appeared in the 4.14 kernel.
Unfortunately, Synopsys appears to consider the operation of this PCI-e controller secret. They provide no publicly available docs for it nor allow the references manuals of SoCs using the controller to publish any documentation of it.
So I can not say certain this code is correctly using the controller's features. However, extensive testing gives me high confidence in the accuracy of what is described below.
If an MSI is received by the PCI-e controller while the status register bit for that MSI is set, the PCI-e controller will NOT generate another interrupt. In addition, it will NOT queue or otherwise mark the interrupt as "pending", which means it will NOT generate an interrupt when the status bit is unmasked.
This gives the following race scenario:
- An MSI is received by, and the status bit for the MSI is set in, the
DWC PCI-e controller. 2. dw_handle_msi_irq() calls a driver's registered interrupt handler for the MSI received. 3. At some point, the interrupt handler must decide, correctly, that there is no more work to do and return. 4. The hardware generates a new MSI. As the MSI's status bit is still set, this new MSI is ignored. 6. dw_handle_msi_irq() unsets the MSI status bit.
The MSI received at point 4 will never be acted upon. It occurred after the driver had finished checking the hardware status for interrupt conditions to act on. Since the MSI status was masked, it does not generated a new IRQ, neither when it was received nor when the MSI is unmasked.
It seems clear there is an unsolvable race here.
After this patch, the sequence becomes as follows:
- An MSI is received and the status bit for the MSI is set in the
DWC PCI-e controller. 2. dw_handle_msi_irq() clears this MSI status bit. 3. dw_handle_msi_irq() calls a driver's registered interrupt handler for the MSI received. 3. At some point, the interrupt handler must decide, correctly, that there is no more work to do and return. 4. The hardware generates a new MSI. This sets the MSI status bit and triggers another interrupt in the interrupt controller(s) above the DWC PCI-e controller. As the the dwc-pcie handler is not re-entrant, it is not run again at this time. 6. dw_handle_msi_irq() finishes. The MSI status bit remains set. 7. The IRQ is re-raised and dw_handle_msi_irq() runs again. 8. dw_handle_msi_irq() invokes the MSI's registered interrupt handler again as the status bit was still set.
The observant will notice that point 4 present the opportunity for the SoC's interrupt controller to lose the interrupt in the same manner as the bug in this driver. The driver for that interrupt controller will be written to properly deal with this. In some cases the hardware supports an EOI concept, where the 2nd IRQ is masked and internally queued in the hardware, to be re-raised at EOI in step 7. In other cases the IRQ will be unmasked and re-raised at step 4, but the kernel will see the handler is INPROGRESS and not re-invoke it, and instead set a PENDING flag, which causes the handler to re-run at step 7.
[1] https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_pa...
Fixes: 8c934095fa2f ("PCI: dwc: Clear MSI interrupt status after it is handled, not before") Cc: stable@vger.kernel.org Cc: Vignesh R vigneshr@ti.com Cc: Faiz Abbas faiz_abbas@ti.com Cc: Jingoo Han jingoohan1@gmail.com Cc: Gustavo Pimentel gustavo.pimentel@synopsys.com Cc: Joao Pinto jpinto@synopsys.com Cc: Lorenzo Pieralisi lorenzo.pieralisi@arm.com Cc: Bjorn Helgaas bhelgaas@google.com Signed-off-by: Trent Piepho tpiepho@impinj.com
drivers/pci/controller/dwc/pcie-designware-host.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c index 29a05759a294..9a3960c95ad3 100644 --- a/drivers/pci/controller/dwc/pcie-designware-host.c +++ b/drivers/pci/controller/dwc/pcie-designware-host.c @@ -98,10 +98,10 @@ irqreturn_t dw_handle_msi_irq(struct pcie_port *pp) irq = irq_find_mapping(pp->irq_domain, (i * MAX_MSI_IRQS_PER_CTRL) + pos);
generic_handle_irq(irq); dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + (i * MSI_REG_CTRL_BLOCK_SIZE), 4, 1 << pos);
generic_handle_irq(irq);
Makes sense.
Acked-by: Gustavo Pimentel gustavo.pimentel@synopsys.com
pos++; }}