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

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

On 07/01/2019 17:16, Stephen Warren wrote:
> On 1/7/19 2:36 AM, Gustavo Pimentel wrote:
>> 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")
> 
> OK. I assume that dw_pci_bottom_ack() gets called before 
> dw_handle_msi_irq() for each IRQ? In which case, the last of those 3 

Yes, the dw_pci_bottom_ack() gets called before dw_handle_msi_irq().

> changes is essentially identical to the patch I proposed, it's just that 
> the acking is handled in a separate function rather than earlier in the 
> same function.
> 

No, there is also two other fixes. A missing lock/unlock on the ack and the
correct use of the MASK/UNMASK interrupt register instead of ENABLE interrupt
register.