From: Bjorn Helgaas <helgaas@kernel.org>
To: Conor.Dooley@microchip.com, maz@kernel.org
Cc: lorenzo.pieralisi@arm.com, Daire.McNamara@microchip.com,
bhelgaas@google.com, Cyril.Jean@microchip.com,
david.abdurachmanov@gmail.com, linux-pci@vger.kernel.org,
robh@kernel.org
Subject: Re: [RESEND PATCH v1 1/1] PCI: microchip: Fix potential race in interrupt handling
Date: Fri, 29 Apr 2022 16:57:33 -0500 [thread overview]
Message-ID: <20220429215733.GA97739@bhelgaas> (raw)
In-Reply-To: <79102463-adc1-9555-70ba-bdde58a77401@microchip.com>
[+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
> >>
next prev parent reply other threads:[~2022-04-29 21:57 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-05 11:17 [RESEND PATCH v1 1/1] PCI: microchip: Fix potential race in interrupt handling daire.mcnamara
2022-04-28 6:30 ` Conor.Dooley
2022-04-28 9:29 ` Lorenzo Pieralisi
2022-04-29 9:42 ` Conor.Dooley
2022-04-29 21:57 ` Bjorn Helgaas [this message]
2022-04-29 23:33 ` Marc Zyngier
2022-05-02 19:22 ` Bjorn Helgaas
2022-05-04 15:12 ` Conor Dooley
2022-05-04 16:53 ` Lorenzo Pieralisi
2022-05-04 16:57 ` Conor Dooley
2022-05-04 16:59 ` Bjorn Helgaas
2022-05-11 10:00 ` Conor Dooley
2022-05-11 12:41 ` Lorenzo Pieralisi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220429215733.GA97739@bhelgaas \
--to=helgaas@kernel.org \
--cc=Conor.Dooley@microchip.com \
--cc=Cyril.Jean@microchip.com \
--cc=Daire.McNamara@microchip.com \
--cc=bhelgaas@google.com \
--cc=david.abdurachmanov@gmail.com \
--cc=linux-pci@vger.kernel.org \
--cc=lorenzo.pieralisi@arm.com \
--cc=maz@kernel.org \
--cc=robh@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).