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@xxxxxxxxxx> wrote: > > > > > > On Fri, Apr 29, 2022 at 09:42:52AM +0000, Conor.Dooley@xxxxxxxxxxxxx wrote: > > > > > > > On 28/04/2022 10:29, Lorenzo Pieralisi wrote: > > > > > > > > On Tue, Apr 05, 2022 at 12:17:51PM +0100, daire.mcnamara@xxxxxxxxxxxxx wrote: > > > > > > > > > From: Daire McNamara <daire.mcnamara@xxxxxxxxxxxxx> > > > > > > > > > > > > > > > > > > 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@xxxxxxxxxxxxx/ > > Thanks, > Conor.
