Re: [PATCH] PCI: dwc: fix MSI IRQ handler ordering

[Gustavo Pimentel <gustavo.pimentel@synopsys.com>]

Hi Stephen,

On 04/01/2019 21:45, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
>
> The current code does this when handling MSI IRQs:
>
> a) Process the irq.
> b) Clear the latched IRQ status.
>
> If a new IRQ occurs any time after (a) has read the IRQ status for the
> last time and before (b), it will be lost. For example, this occurs in
> practice when using a Marvell 9171 AHCI controller with NCQ enabled;
> many command timeouts occur with certain disk access patterns.
>
> Fix the code to do the following instead, so that if any new IRQs are
> raised during the processing of the IRQ, the IRQ status is not cleared,
> so that the IRQ is not lost.
>
> a) Clear the latched IRQ status.
> b) Process the IRQ.
>
> This change reverts commit 8c934095fa2f ("PCI: dwc: Clear MSI interrupt
> status after it is handled, not before")
>
> This change re-applies commit ca1658921b63 ("PCI: designware: Fix
> missing MSI IRQs")
>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Faiz Abbas <faiz_abbas@ti.com>
> Cc: Harro Haan <hrhaan@gmail.com>
> Cc: Jingoo Han <jg1.han@samsung.com>
> Cc: Joao Pinto <jpinto@synopsys.com>
> Cc: Juergen Beisert <jbe@pengutronix.de>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Matthias Mann <m.mann@arkona-technologies.de>
> Cc: Mohit Kumar <mohit.kumar@st.com>
> Cc: Pratyush Anand <pratyush.anand@st.com>
> Cc: Richard Zhu <hong-xing.zhu@freescale.com>
> Cc: Sean Cross <xobs@kosagi.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: Siva Reddy Kallam <siva.kallam@samsung.com>
> Cc: Srikanth T Shivanand <ts.srikanth@samsung.com>
> Cc: Tim Harvey <tharvey@gateworks.com>
> ---
> Note: This issue was found in downstream NVIDIA 4.9 and 4.14 kernels.
> However, the exact same code structure is present in mainline and I have
> no reason to believe the problem would not reproduce there. I have
> compile tested but not runtime tested it in mainline, since my board is
> not yet supported in mainline.
> ---
>  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 692dd1b264fb..7fd6c56a6f35 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);
>  			pos++;
>  		}
>  	}

This fix was already suggested by Trent Piepho, however, after review by Marc
Zyngier and some information obtained from Synopsys IP development team, the
following set of patches was generated to correct the problem.

830920e065e9 ("PCI: dwc: Use interrupt masking instead of disabling")
fce5423e4f43 ("PCI: dwc: Take lock when ACKing an interrupt")
3f7bb2ec20ce ("PCI: dwc: Move interrupt acking into the proper callback")

Regards,

Gustavo