PCI: microchip: Fix potential race in interrupt handling

PATCH RESEND Superseded [email protected] 6 participants 13 messages Apr 5, 2022 – May 11, 2022

Subsystems (2)

PCI PCI native host bridge and endpoint drivers

lore

Share & tools

lore

[email protected]

author

2022-04-05 12:42 UTC #0 lore raw

PATCH RESEND 1/1 PCI: microchip: Fix potential race in interrupt handling

From: Daire McNamara <[email protected]>

Clear MSI bit in ISTATUS register after reading it before handling individual MSI bits

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 <[email protected]>

---
Adding linux-pci mailing list

 drivers/pci/controller/pcie-microchip-host.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

drivers/pci/controller/pcie-microchip-host.c +1 -5

index 29d8e81e4181..da8e3fdc97b3 100644
@@ -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);
 		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)

Signature · 1 line

2.25.1

[email protected]

2022-04-28 06:30 UTC #1 lore raw

Hey all,
Is there anything you need from us on this patch?
Thanks,
Conor

Quoted 41 lines · On 05/04/2022 12:17, [email protected] wrote:

From: Daire McNamara <[email protected]>

Clear MSI bit in ISTATUS register after reading it before handling individual MSI bits

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 <[email protected]>

---
Adding linux-pci mailing list

  drivers/pci/controller/pcie-microchip-host.c | 6 +-----
  1 file changed, 1 insertion(+), 5 deletions(-)

drivers/pci/controller/pcie-microchip-host.c +1 -5

index 29d8e81e4181..da8e3fdc97b3 100644
@@ -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);
  		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)

Lorenzo Pieralisi

2022-04-28 09:46 UTC #2 lore raw

In reply to [email protected]

Quoted 4 lines · On Tue, Apr 05, 2022 at 12:17:51PM +0100, [email protected] wrote:

From: Daire McNamara <[email protected]>
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.

Quoted 15 lines

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 <[email protected]>
---
Adding linux-pci mailing list
 drivers/pci/controller/pcie-microchip-host.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)
drivers/pci/controller/pcie-microchip-host.c +0 -0
index 29d8e81e4181..da8e3fdc97b3 100644

drivers/pci/controller/pcie-microchip-host.c static void mc_handle_msi(struct irq_desc *desc) · line 419

Quoted 4 lines

drivers/pci/controller/pcie-microchip-host.c +1 -0

@@ -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

Quoted 20 lines

drivers/pci/controller/pcie-microchip-host.c +0 -5

 		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)

Signature · 1 line

2.25.1

[email protected]

2022-04-29 09:43 UTC #3 lore raw

Quoted 8 lines · Lorenzo Pieralisi

Quoted 4 lines · On Tue, Apr 05, 2022 at 12:17:51PM +0100, [email protected] wrote:
From: Daire McNamara <[email protected]>
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.

Reported-by Bjorn Helgaas <[email protected]>

Link https://lore.kernel.org/linux-pci/20220127202000.GA126335@bhelgaas/ open here

Fixes 6f15a9c9f941 ("PCI: microchip: Add Microchip PolarFire PCIe controller driver")

Signed-off-by Daire McNamara <[email protected]>

Quoted 47 lines

Quoted 16 lines
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 <[email protected]>
---
Adding linux-pci mailing list
  drivers/pci/controller/pcie-microchip-host.c | 6 +-----
  1 file changed, 1 insertion(+), 5 deletions(-)
drivers/pci/controller/pcie-microchip-host.c +0 -0
index 29d8e81e4181..da8e3fdc97b3 100644
drivers/pci/controller/pcie-microchip-host.c static void mc_handle_msi(struct irq_desc *desc) · line 419
Quoted 4 lines
drivers/pci/controller/pcie-microchip-host.c +1 -0
@@ -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
Quoted 20 lines
drivers/pci/controller/pcie-microchip-host.c +0 -5
  		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)
Signature · 1 line
2.25.1

Bjorn Helgaas

2022-04-29 21:57 UTC #4 lore raw

[+to Marc]

Quoted 14 lines · On Fri, Apr 29, 2022 at 09:42:52AM +0000, [email protected] wrote:

Quoted 8 lines · Lorenzo Pieralisi
Quoted 4 lines · On Tue, Apr 05, 2022 at 12:17:51PM +0100, [email protected] wrote:
From: Daire McNamara <[email protected]>
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?

Quoted 5 lines

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?

Quoted 1 line

Reported-by Bjorn Helgaas <[email protected]>

Please use this address instead:

Reported by: Bjorn Helgaas <[email protected]>

Quoted 50 lines

Link https://lore.kernel.org/linux-pci/20220127202000.GA126335@bhelgaas/ open here
Fixes 6f15a9c9f941 ("PCI: microchip: Add Microchip PolarFire PCIe controller driver")
Signed-off-by Daire McNamara <[email protected]>
Quoted 46 lines
Quoted 15 lines
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 <[email protected]>
---
Adding linux-pci mailing list
  drivers/pci/controller/pcie-microchip-host.c | 6 +-----
  1 file changed, 1 insertion(+), 5 deletions(-)
drivers/pci/controller/pcie-microchip-host.c +0 -0
index 29d8e81e4181..da8e3fdc97b3 100644
drivers/pci/controller/pcie-microchip-host.c static void mc_handle_msi(struct irq_desc *desc) · line 419
Quoted 4 lines
drivers/pci/controller/pcie-microchip-host.c +1 -0
@@ -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
Quoted 20 lines
drivers/pci/controller/pcie-microchip-host.c +0 -5
  		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)
Signature · 1 line
2.25.1

Marc Zyngier

2022-04-29 23:34 UTC #5 lore raw

Quoted 24 lines · Bjorn Helgaas

[+to Marc]
Quoted 14 lines · On Fri, Apr 29, 2022 at 09:42:52AM +0000, [email protected] wrote:
Quoted 8 lines · Lorenzo Pieralisi
Quoted 4 lines · On Tue, Apr 05, 2022 at 12:17:51PM +0100, [email protected] wrote:
From: Daire McNamara <[email protected]>
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?
Quoted 5 lines
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.

Quoted 7 lines

"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.

This seems sensible. In general, edge interrupts need an early Ack *before* the handler can be run. If it happens after, you're pretty much guaranteed to lose edges that would be generated between the handler and the late Ack.

This can be implemented in HW in a variety of ways (read a register, write a register, or even both).

Quoted 8 lines

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.

M.

Signature · 1 line

Without deviation from the norm, progress is not possible.

Bjorn Helgaas

2022-05-02 19:22 UTC #6 lore raw

Quoted 21 lines · Marc Zyngier

Quoted 14 lines · Bjorn Helgaas
Quoted 4 lines · On Fri, Apr 29, 2022 at 09:42:52AM +0000, [email protected] wrote:
Quoted 3 lines · Lorenzo Pieralisi
Quoted 2 lines · On Tue, Apr 05, 2022 at 12:17:51PM +0100, [email protected] wrote:
From: Daire McNamara <[email protected]>
Quoted 8 lines
Quoted 2 lines
Quoted 2 lines
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.
Quoted 4 lines
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.

Quoted 7 lines

This seems sensible. In general, edge interrupts need an early Ack *before* the handler can be run. If it happens after, you're pretty much guaranteed to lose edges that would be generated between the handler and the late Ack.
This can be implemented in HW in a variety of ways (read a register, write a register, or even both).

Is this something that is or could be documented somewhere under Documentation, e.g., "here are the common canonical patterns to use"? I feel like an idiot because I have this kind of question all the time and I never know how to confidently analyze it.

Quoted 10 lines

Quoted 7 lines
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.

I wonder if coccinelle or some other static analyzer would be smart enough to find this kind of error.

Bjorn

Conor Dooley

2022-05-04 15:12 UTC #7 lore raw

Quoted 27 lines · Bjorn Helgaas

Quoted 26 lines · Marc Zyngier
Quoted 14 lines · Bjorn Helgaas
Quoted 4 lines · On Fri, Apr 29, 2022 at 09:42:52AM +0000, [email protected] wrote:
Quoted 3 lines · Lorenzo Pieralisi
Quoted 2 lines · On Tue, Apr 05, 2022 at 12:17:51PM +0100, [email protected] wrote:
From: Daire McNamara <[email protected]>
Quoted 8 lines
Quoted 2 lines
Quoted 2 lines
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.
Quoted 4 lines
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.
This seems sensible. In general, edge interrupts need an early Ack *before* the handler can be run. If it happens after, you're pretty much guaranteed to lose edges that would be generated between the handler and the late Ack.

Quoted 7 lines

Quoted 2 lines
This can be implemented in HW in a variety of ways (read a register, write a register, or even both).
Is this something that is or could be documented somewhere under Documentation, e.g., "here are the common canonical patterns to use"? I feel like an idiot because I have this kind of question all the time and I never know how to confidently analyze it.

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.

Reported by: Bjorn Helgaas <[email protected]>

Link https://lore.kernel.org/linux-pci/20220127202000.GA126335@bhelgaas/ open here

Fixes 6f15a9c9f941 ("PCI: microchip: Add Microchip PolarFire PCIe controller driver")

Signed-off-by Daire McNamara <[email protected]>

Quoted 11 lines

Quoted 11 lines
Quoted 8 lines
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?

Thanks,
Conor.

Lorenzo Pieralisi

2022-05-04 17:02 UTC #8 lore raw

Quoted 36 lines · Conor Dooley

Quoted 27 lines · Bjorn Helgaas
Quoted 26 lines · Marc Zyngier
Quoted 14 lines · Bjorn Helgaas
Quoted 4 lines · On Fri, Apr 29, 2022 at 09:42:52AM +0000, [email protected] wrote:
Quoted 3 lines · Lorenzo Pieralisi
Quoted 2 lines · On Tue, Apr 05, 2022 at 12:17:51PM +0100, [email protected] wrote:
From: Daire McNamara <[email protected]>
Quoted 8 lines
Quoted 2 lines
Quoted 2 lines
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.
Quoted 4 lines
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.
This seems sensible. In general, edge interrupts need an early Ack *before* the handler can be run. If it happens after, you're pretty much guaranteed to lose edges that would be generated between the handler and the late Ack.
Quoted 7 lines
Quoted 2 lines
This can be implemented in HW in a variety of ways (read a register, write a register, or even both).
Is this something that is or could be documented somewhere under Documentation, e.g., "here are the common canonical patterns to use"? I feel like an idiot because I have this kind of question all the time and I never know how to confidently analyze it.

Quoted 8 lines

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.

It is still unclear. You should translate what Marc said above into how ISTATUS_MSI and ISTATUS_LOCAL work (ie describe how HW works).

Please describe what the registers do and use that to describe the fix.

Thanks,
Lorenzo

Quoted 21 lines

Reported by: Bjorn Helgaas <[email protected]>
Link https://lore.kernel.org/linux-pci/20220127202000.GA126335@bhelgaas/ open here
Fixes 6f15a9c9f941 ("PCI: microchip: Add Microchip PolarFire PCIe controller driver")
Signed-off-by Daire McNamara <[email protected]>
Quoted 11 lines
Quoted 11 lines
Quoted 8 lines
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?
Thanks,
Conor.

Conor Dooley

2022-05-04 17:06 UTC #9 lore raw

Quoted 46 lines · Lorenzo Pieralisi

Quoted 36 lines · Conor Dooley
Quoted 27 lines · Bjorn Helgaas
Quoted 26 lines · Marc Zyngier
Quoted 14 lines · Bjorn Helgaas
Quoted 4 lines · On Fri, Apr 29, 2022 at 09:42:52AM +0000, [email protected] wrote:
Quoted 3 lines · Lorenzo Pieralisi
Quoted 2 lines · On Tue, Apr 05, 2022 at 12:17:51PM +0100, [email protected] wrote:
From: Daire McNamara <[email protected]>
Quoted 8 lines
Quoted 2 lines
Quoted 2 lines
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.
Quoted 4 lines
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.
This seems sensible. In general, edge interrupts need an early Ack *before* the handler can be run. If it happens after, you're pretty much guaranteed to lose edges that would be generated between the handler and the late Ack.
Quoted 7 lines
Quoted 2 lines
This can be implemented in HW in a variety of ways (read a register, write a register, or even both).
Is this something that is or could be documented somewhere under Documentation, e.g., "here are the common canonical patterns to use"? I feel like an idiot because I have this kind of question all the time and I never know how to confidently analyze it.
Quoted 8 lines
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.

Quoted 5 lines

It is still unclear. You should translate what Marc said above into how ISTATUS_MSI and ISTATUS_LOCAL work (ie describe how HW works).
Please describe what the registers do and use that to describe the fix.

Sure, best to wait until the IT issues are resolved so!

Conor.

Bjorn Helgaas

2022-05-04 17:13 UTC #10 lore raw

In reply to Conor Dooley

Quoted 9 lines · Conor Dooley

Quoted 8 lines · Bjorn Helgaas
Quoted 7 lines · Marc Zyngier
Quoted 5 lines · Bjorn Helgaas
Quoted 4 lines · On Fri, Apr 29, 2022 at 09:42:52AM +0000, [email protected] wrote:
Quoted 3 lines · Lorenzo Pieralisi
Quoted 2 lines · On Tue, Apr 05, 2022 at 12:17:51PM +0100, [email protected] wrote:
From: Daire McNamara <[email protected]>

Quoted 8 lines

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 2 lines
Quoted 2 lines
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:

Quoted 11 lines

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
"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!

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
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().

Quoted 8 lines

Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
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.

Bjorn

Conor Dooley

2022-05-11 10:02 UTC #11 lore raw

Quoted 27 lines · Bjorn Helgaas

Quoted 9 lines · Conor Dooley
Quoted 8 lines · Bjorn Helgaas
Quoted 7 lines · Marc Zyngier
Quoted 5 lines · Bjorn Helgaas
Quoted 4 lines · On Fri, Apr 29, 2022 at 09:42:52AM +0000, [email protected] wrote:
Quoted 3 lines · Lorenzo Pieralisi
Quoted 2 lines · On Tue, Apr 05, 2022 at 12:17:51PM +0100, [email protected] wrote:
From: Daire McNamara <[email protected]>
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 2 lines
Quoted 2 lines
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:
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
"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:

Quoted 7 lines

Quoted 5 lines
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?

Quoted 9 lines

Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
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?

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
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/[email protected]/

Thanks,
Conor.

Lorenzo Pieralisi

2022-05-11 12:42 UTC #12 lore raw

Quoted 34 lines · Conor Dooley

Quoted 33 lines · Bjorn Helgaas
Quoted 9 lines · Conor Dooley
Quoted 8 lines · Bjorn Helgaas
Quoted 7 lines · Marc Zyngier
Quoted 5 lines · Bjorn Helgaas
Quoted 4 lines · On Fri, Apr 29, 2022 at 09:42:52AM +0000, [email protected] wrote:
Quoted 3 lines · Lorenzo Pieralisi
Quoted 2 lines · On Tue, Apr 05, 2022 at 12:17:51PM +0100, [email protected] wrote:
From: Daire McNamara <[email protected]>
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 2 lines
Quoted 2 lines
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:
Quoted 11 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
"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.

Quoted 4 lines

Quoted 1 line
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

Quoted 22 lines

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
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?
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
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/[email protected]/
Thanks,
Conor.