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

[Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>]

On Wed, May 11, 2022 at 11:00:18AM +0100, Conor Dooley wrote:
> On 04/05/2022 17:59, Bjorn Helgaas wrote:
> > On Wed, May 04, 2022 at 04:12:39PM +0100, Conor Dooley wrote:
> > > On 02/05/2022 20:22, Bjorn Helgaas wrote:
> > > > On Sat, Apr 30, 2022 at 12:33:51AM +0100, Marc Zyngier wrote:
> > > > > On Fri, 29 Apr 2022 22:57:33 +0100,
> > > > > Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > 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
> > > > 
> > > > > > > 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.
> > 
> > Restoring the context here:
> > 
> > > > > > "ISTATUS" doesn't appear in the code as a register name.
> > > > > > Neither does "IMSI".  Please use names that match the code.
> > 
> > > Daire is still having the IT issues, so before I resend the patch with
> > > a new commit message, how is the following:
> > > 
> > > Clear the MSI bit in ISTATUS_LOCAL register after reading it, but
> > > before reading and handling individual MSI bits from the ISTATUS_MSI
> > > register. This avoids a potential race where new MSI bits may be set
> > > on the ISTATUS_MSI register after it was read and be missed when the
> > > MSI bit in the ISTATUS_LOCAL register is cleared.
> > 
> > Looks good, thank you!
> 
> Hmm, there's now a response saying that the proposed commit message is
> fine and one saying it isn't. Which is it?

I would like the commit log to contain an explanation of what
ISTATUS_LOCAL reg is there for and how it is related to ISTATUS_MSI
please.

Thanks,
Lorenzo

> > > > > > 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?
> > > > > 
> > > > > That's just a bug. On the right HW, this would just result in lost
> > > > > interrupts.
> > > 
> > > Separate issue, separate patch. Do you want them in a series or as
> > > another standalone patch?
> > 
> > Agreed, should be a separate patch.  Doesn't need to be a series
> > unless that patch only applies correctly on top of this one.
> 
> Cool, just sent one:
> https://lore.kernel.org/linux-pci/20220511095504.2273799-1-conor.dooley@microchip.com/
> 
> Thanks,
> Conor.