Re: [RESEND PATCH v1 1/1] PCI: microchip: Fix potential race in interrupt handling

[Bjorn Helgaas <helgaas@kernel.org>]

[+to Marc]

On Fri, Apr 29, 2022 at 09:42:52AM +0000, Conor.Dooley@microchip.com wrote:
> On 28/04/2022 10:29, Lorenzo Pieralisi wrote:
> > On Tue, Apr 05, 2022 at 12:17:51PM +0100, daire.mcnamara@microchip.com wrote:
> >> From: Daire McNamara <daire.mcnamara@microchip.com>
> >>
> >> Clear MSI bit in ISTATUS register after reading it before
> >> handling individual MSI bits
> > 
> > That explains nothing. If you are fixing a bug please describe
> > the issue and how the patch is fixing it.
> 
> Someone in the pantheon of IT gods has it out for Daire, so I am
> sending this on his behalf, but is the following revised commit
> message better?
> 
> Clear the MSI bit in ISTATUS register after reading it, but before
> reading and handling individual MSI bits from the IMSI register.
> This avoids a potential race where new MSI bits may be set on the
> IMSI register after it was read and be missed when the MSI bit in
> the ISTATUS register is cleared.

"ISTATUS" doesn't appear in the code as a register name.  Neither does
"IMSI".  Please use names that match the code.

Honestly, I don't understand enough about IRQs to determine whether
this is a correct fix.  Hopefully Marc will chime in.  All I really
know how to do is compare all the drivers and see which ones don't fit
the typical patterns.

And speaking of that, I looked at all the users of
irq_set_chained_handler_and_data() in drivers/pci.  All the handlers
except mc_handle_intx() and mc_handle_msi() call chained_irq_enter()
and chained_irq_exit().

Are mc_handle_intx() and mc_handle_msi() just really special, or is
this a mistake?

> Reported-by: Bjorn Helgaas <helgaas@kernel.org>

Please use this address instead:

Reported by: Bjorn Helgaas <bhelgaas@google.com>

> Link: https://lore.kernel.org/linux-pci/20220127202000.GA126335@bhelgaas/
> Fixes: 6f15a9c9f941 ("PCI: microchip: Add Microchip PolarFire PCIe controller driver")
> Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
> 
> >> This fixes a potential race condition pointed out by Bjorn Helgaas:
> >> https://lore.kernel.org/linux-pci/20220127202000.GA126335@bhelgaas/
> >>
> >> Fixes: 6f15a9c9f941 ("PCI: microchip: Add Microchip PolarFire PCIe controller driver")
> >> Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
> >> ---
> >> Adding linux-pci mailing list
> >>   drivers/pci/controller/pcie-microchip-host.c | 6 +-----
> >>   1 file changed, 1 insertion(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/pci/controller/pcie-microchip-host.c b/drivers/pci/controller/pcie-microchip-host.c
> >> index 29d8e81e4181..da8e3fdc97b3 100644
> >> --- a/drivers/pci/controller/pcie-microchip-host.c
> >> +++ b/drivers/pci/controller/pcie-microchip-host.c
> >> @@ -416,6 +416,7 @@ static void mc_handle_msi(struct irq_desc *desc)
> >>   
> >>   	status = readl_relaxed(bridge_base_addr + ISTATUS_LOCAL);
> >>   	if (status & PM_MSI_INT_MSI_MASK) {
> >> +		writel_relaxed(status & PM_MSI_INT_MSI_MASK, bridge_base_addr + ISTATUS_LOCAL);
> > 
> > What does ISTATUS_LOCAL contain vs ISTATUS_MSI ? If you explain that
> > to me I could help you write the commit log.
> > 
> > Thanks,
> > Lorenzo
> > 
> >>   		status = readl_relaxed(bridge_base_addr + ISTATUS_MSI);
> >>   		for_each_set_bit(bit, &status, msi->num_vectors) {
> >>   			ret = generic_handle_domain_irq(msi->dev_domain, bit);
> >> @@ -432,13 +433,8 @@ static void mc_msi_bottom_irq_ack(struct irq_data *data)
> >>   	void __iomem *bridge_base_addr =
> >>   		port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
> >>   	u32 bitpos = data->hwirq;
> >> -	unsigned long status;
> >>   
> >>   	writel_relaxed(BIT(bitpos), bridge_base_addr + ISTATUS_MSI);
> >> -	status = readl_relaxed(bridge_base_addr + ISTATUS_MSI);
> >> -	if (!status)
> >> -		writel_relaxed(BIT(PM_MSI_INT_MSI_SHIFT),
> >> -			       bridge_base_addr + ISTATUS_LOCAL);
> >>   }
> >>   
> >>   static void mc_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
> >> -- 
> >> 2.25.1
> >>