Re: [PATCH] PCI: dwc: Fix interrupt race in when handling MSI

[Trent Piepho <tpiepho@impinj.com>]

On Tue, 2018-11-13 at 00:41 +0000, Gustavo Pimentel wrote:
> On 09/11/2018 11:34, Marc Zyngier wrote:
> > 
> > Gustavo, here's one simple ask. Please reply to this email with a step
> > by step, register by register description of how an MSI must be handled
> > on this HW. We do need it, and we need it now.
> 
> Hi Marc, I thought that I did respond to your question on Nov 8. However I will
> repeat it and add some extra information to it now.
> 
> About the registers:
> 
> PCIE_MSI_INTR0_MASK
> When an MSI is received for a masked interrupt, the corresponding status bit
> gets set in the interrupt status register but the controller will not signal it.
> As soon as the masked interrupt is unmasked and assuming the status bit is still
> set the controller will signal it.
> 
> PCIE_MSI_INTR0_ENABLE
> Enables/disables every interrupt. When an MSI is received from a disabled
> interrupt, no status bit gets set in MSI controller interrupt status register.

If the MSI is unmasked and the status bit is still set, *not* from a
new MSI but rather from one that arrived before the MSI was masked,
does the controller still signal it?

I would suspect the answer is no: only a new MSI will cause the
interrupt be signaled on unmask.  And then only if the status bit has
not been cleared before the unmask (as you stated already).

But for that to be the case, there must be another bit of interrupt
status (i.e., "pending") that we don't know about yet.  The bit in the
status register alone is not enough.

This pending bit would need to be set on a 0->1 transition of the
status bit.  An MSI arriving while the status bit is already 1 does not
trigger a new interrupt.

> About this new callbacks:
> 
> The dw_pci_bottom_mask() and dw_pci_bottom_unmask() callbacks were added on
> Linux kernel v4.17 on PCI: dwc: Move MSI IRQs allocation to IRQ domains
> hierarchical API patch [1] and based on what I've seen so far, before this patch
> there were no interrupt masking been performed.

Yes.  Or rather, the status bit being set effectively masks the
interrupt.

> Based on the information provided, its very likely that I should use the
> PCIE_MSI_INTR0_MASK register instead of PCIE_MSI_INTR0_ENABLE on the
> dw_pci_bottom_mask() and dw_pci_bottom_unmask() callbacks. As soon as I return
> from Linux plumbers conference, I will test the system using the
> PCIE_MSI_INTR0_MASK register.

Using enable to mask the interrupt is broken.  It will allow an MSI to
be lost if it arrives while the MSI in not enabled.  It is impossible
to prevent that from racing against the pci device driver's final check
that no MSI-signaled condition in the hardware is present.