[v1,2/2] PCI: pciehp: Rework hotplug interrupt routine

Commit Message

Patel, Mayurkumar Aug. 23, 2016, 8:59 a.m. UTC

First scenario, on any slot events, pcie_isr() does as following,
pcie_isr() -> do {...} while(detected) loop in which it
reads PCI_EXP_SLTSTA, stores it in the intr_loc, then
clears respective interrupts by writing to PCI_EXP_SLTSTA.
Again, due to loop, it reads PCI_EXP_SLTSTA, which might
have been changed already for the same type of interrupts
because in the previous iteration they already got cleared.
In this case, it will execute pciehp_queue_interrupt_event() only once
based on the last event happened. This can be problematic
for PCI_EXP_SLTSTA_PDC and PCI_EXP_SLTSTA_DLLSC types of
interrupts as if they miss to process previous events then PCIe device
enumeration can get effected.

Second scenario, pcie_isr() after clearing interrupts, it calls
pciehp_get_adapter_status() before processing PCI_EXP_SLTSTA_PDS
and pciehp_check_link_active() before processing PCI_EXP_SLTSTA_DLLSC
and takes decisions based on that to do pciehp_queue_interrupt_event()
which might also have already got changed due to the same
fact that the respective interrupts got cleared earlier.

The patch removes re-inspection to avoid first scenario happening
and just reads the events once and clears them as soon as possible.
To successfully execute right Slot events for PDC and DLLSC types which
triggered pcie_isr() it reads the PCI_EXP_SLTSTA_PDS and
PCI_EXP_LNKSTA_DLLLA earlier before clearing the respective interrupts
and executes pciehp_queue_interrupt_event() based on the stored
status of these two Slot events.

Signed-off-by: Mayurkumar Patel <[e-mail]>
---
Resending the patch.
The patch is just one proposal. It is just prototype tested on
HW on which I had PATCH 1 issue and currently I don't know it
would work any HW.

 drivers/pci/hotplug/pciehp_hpc.c |   45 ++++++++++++++++----------------------
 1 file changed, 19 insertions(+), 26 deletions(-)

--
1.7.9.5

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: [phone], www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Bjorn Helgaas Sept. 12, 2016, 8:56 p.m. UTC | #1

On Tue, Aug 23, 2016 at 08:59:49AM +0000, Patel, Mayurkumar wrote:
> First scenario, on any slot events, pcie_isr() does as following,
> pcie_isr() -> do {...} while(detected) loop in which it
> reads PCI_EXP_SLTSTA, stores it in the intr_loc, then
> clears respective interrupts by writing to PCI_EXP_SLTSTA.
> Again, due to loop, it reads PCI_EXP_SLTSTA, which might
> have been changed already for the same type of interrupts
> because in the previous iteration they already got cleared.
> In this case, it will execute pciehp_queue_interrupt_event() only once
> based on the last event happened. This can be problematic
> for PCI_EXP_SLTSTA_PDC and PCI_EXP_SLTSTA_DLLSC types of
> interrupts as if they miss to process previous events then PCIe device
> enumeration can get effected.
> 
> Second scenario, pcie_isr() after clearing interrupts, it calls
> pciehp_get_adapter_status() before processing PCI_EXP_SLTSTA_PDS
> and pciehp_check_link_active() before processing PCI_EXP_SLTSTA_DLLSC
> and takes decisions based on that to do pciehp_queue_interrupt_event()
> which might also have already got changed due to the same
> fact that the respective interrupts got cleared earlier.
> 
> The patch removes re-inspection to avoid first scenario happening
> and just reads the events once and clears them as soon as possible.
> To successfully execute right Slot events for PDC and DLLSC types which
> triggered pcie_isr() it reads the PCI_EXP_SLTSTA_PDS and
> PCI_EXP_LNKSTA_DLLLA earlier before clearing the respective interrupts
> and executes pciehp_queue_interrupt_event() based on the stored
> status of these two Slot events.

I think this is great.  I split it into two patches, one to deal with
the loop and a second to stop re-reading PCI_EXP_SLTSTA.

I propose that we keep the loop, but restructure it a little.  If we
remove the loop completely, I worry that we may be able to miss an
interrupt.  I'm not confident that there is a hole, so maybe we
*could* remove the loop competely; I just couldn't convince myself
that it was safe both for INTx and MSI/MSI-X signaling.

I also added a few trivial patches for cleanup in the area.  I'll post
the whole set as a v2 for your comments.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patel, Mayurkumar Sept. 13, 2016, 4:12 p.m. UTC | #2

> On Tue, Aug 23, 2016 at 08:59:49AM +0000, Patel, Mayurkumar wrote:
> > First scenario, on any slot events, pcie_isr() does as following,
> > pcie_isr() -> do {...} while(detected) loop in which it
> > reads PCI_EXP_SLTSTA, stores it in the intr_loc, then
> > clears respective interrupts by writing to PCI_EXP_SLTSTA.
> > Again, due to loop, it reads PCI_EXP_SLTSTA, which might
> > have been changed already for the same type of interrupts
> > because in the previous iteration they already got cleared.
> > In this case, it will execute pciehp_queue_interrupt_event() only once
> > based on the last event happened. This can be problematic
> > for PCI_EXP_SLTSTA_PDC and PCI_EXP_SLTSTA_DLLSC types of
> > interrupts as if they miss to process previous events then PCIe device
> > enumeration can get effected.
> >
> > Second scenario, pcie_isr() after clearing interrupts, it calls
> > pciehp_get_adapter_status() before processing PCI_EXP_SLTSTA_PDS
> > and pciehp_check_link_active() before processing PCI_EXP_SLTSTA_DLLSC
> > and takes decisions based on that to do pciehp_queue_interrupt_event()
> > which might also have already got changed due to the same
> > fact that the respective interrupts got cleared earlier.
> >
> > The patch removes re-inspection to avoid first scenario happening
> > and just reads the events once and clears them as soon as possible.
> > To successfully execute right Slot events for PDC and DLLSC types which
> > triggered pcie_isr() it reads the PCI_EXP_SLTSTA_PDS and
> > PCI_EXP_LNKSTA_DLLLA earlier before clearing the respective interrupts
> > and executes pciehp_queue_interrupt_event() based on the stored
> > status of these two Slot events.
> 
> I think this is great.  I split it into two patches, one to deal with
> the loop and a second to stop re-reading PCI_EXP_SLTSTA.
> 
> I propose that we keep the loop, but restructure it a little.  If we
> remove the loop completely, I worry that we may be able to miss an
> interrupt.  I'm not confident that there is a hole, so maybe we
> *could* remove the loop competely; I just couldn't convince myself
> that it was safe both for INTx and MSI/MSI-X signaling.
> 
> I also added a few trivial patches for cleanup in the area.  I'll post
> the whole set as a v2 for your comments.
> 

Thanks for splitting the commit. The v2 patches are ok for me.


> Bjorn
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: [phone], www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 5c24e93..2d01b7d 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -542,36 +542,30 @@  static irqreturn_t pcie_isr(int irq, void *dev_id)
 	struct pci_bus *subordinate = pdev->subordinate;
 	struct pci_dev *dev;
 	struct slot *slot = ctrl->slot;
-	u16 detected, intr_loc;
+	u16 slot_status, intr_loc = 0;
 	u8 present;
 	bool link;

+	pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
+	if (slot_status == (u16) ~0) {
+		ctrl_info(ctrl, "%s: no response from device\n",
+			  __func__);
+		return IRQ_HANDLED;
+	}
+	intr_loc = (slot_status & (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD |
+		     PCI_EXP_SLTSTA_PDC |
+		     PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC));
+	if (!intr_loc)
+		return IRQ_NONE;
+
 	/*
-	 * In order to guarantee that all interrupt events are
-	 * serviced, we need to re-inspect Slot Status register after
-	 * clearing what is presumed to be the last pending interrupt.
+	 * update link status before clearing interrupts to process
+	 * it later
 	 */
-	intr_loc = 0;
-	do {
-		pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &detected);
-		if (detected == (u16) ~0) {
-			ctrl_info(ctrl, "%s: no response from device\n",
-				  __func__);
-			return IRQ_HANDLED;
-		}
-
-		detected &= (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD |
-			     PCI_EXP_SLTSTA_PDC |
-			     PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC);
-		detected &= ~intr_loc;
-		intr_loc |= detected;
-		if (!intr_loc)
-			return IRQ_NONE;
-		if (detected)
-			pcie_capability_write_word(pdev, PCI_EXP_SLTSTA,
-						   intr_loc);
-	} while (detected);
+	if (intr_loc & PCI_EXP_SLTSTA_DLLSC)
+		link = pciehp_check_link_active(ctrl);

+	pcie_capability_write_word(pdev, PCI_EXP_SLTSTA, intr_loc);
 	ctrl_dbg(ctrl, "pending interrupts %#06x from Slot Status\n", intr_loc);

 	/* Check Command Complete Interrupt Pending */
@@ -603,7 +597,7 @@  static irqreturn_t pcie_isr(int irq, void *dev_id)

 	/* Check Presence Detect Changed */
 	if (intr_loc & PCI_EXP_SLTSTA_PDC) {
-		pciehp_get_adapter_status(slot, &present);
+		present = !!(slot_status & PCI_EXP_SLTSTA_PDS);
 		ctrl_info(ctrl, "Card %spresent on Slot(%s)\n",
 			  present ? "" : "not ", slot_name(slot));
 		pciehp_queue_interrupt_event(slot, present ? INT_PRESENCE_ON :
@@ -618,7 +612,6 @@  static irqreturn_t pcie_isr(int irq, void *dev_id)
 	}

 	if (intr_loc & PCI_EXP_SLTSTA_DLLSC) {
-		link = pciehp_check_link_active(ctrl);
 		ctrl_info(ctrl, "slot(%s): Link %s event\n",
 			  slot_name(slot), link ? "Up" : "Down");
 		pciehp_queue_interrupt_event(slot, link ? INT_LINK_UP :