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

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

[Patel, Mayurkumar]

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 <mayurkumar.patel@intel.com>
---
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(-)

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 =3D pdev->subordinate;
 	struct pci_dev *dev;
 	struct slot *slot =3D ctrl->slot;
-	u16 detected, intr_loc;
+	u16 slot_status, intr_loc =3D 0;
 	u8 present;
 	bool link;

+	pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
+	if (slot_status =3D=3D (u16) ~0) {
+		ctrl_info(ctrl, "%s: no response from device\n",
+			  __func__);
+		return IRQ_HANDLED;
+	}
+	intr_loc =3D (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 =3D 0;
-	do {
-		pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &detected);
-		if (detected =3D=3D (u16) ~0) {
-			ctrl_info(ctrl, "%s: no response from device\n",
-				  __func__);
-			return IRQ_HANDLED;
-		}
-
-		detected &=3D (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD |
-			     PCI_EXP_SLTSTA_PDC |
-			     PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC);
-		detected &=3D ~intr_loc;
-		intr_loc |=3D 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 =3D 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 =3D !!(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 =3D 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 :
--
1.7.9.5

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, 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

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

[Bjorn Helgaas]

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

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

[Patel, Mayurkumar]

> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, 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

RE: [PATCH v1] PCI: pciehp: Fix presence detect change interrupt handling

[Patel, Mayurkumar]

Hi Bjorn and Rajat
Thanks for replying.
 =

> Hi Rajat, thanks for chiming in!
> =

> On Wed, Aug 17, 2016 at 10:54:12AM -0700, Rajat Jain wrote:
> > On Wed, Aug 17, 2016 at 10:12 AM, Bjorn Helgaas <helgaas@kernel.org> wr=
ote:
> > >
> > > Hi Mayurkumar,
> > >
> > > On Wed, Aug 17, 2016 at 01:42:18PM +0000, Patel, Mayurkumar wrote:
> > > > Currently, if very fast hotplug removal and insertion event comes
> > > > as following
> > > >
> > > > [  608.823412] pciehp 0000:00:1c.1:pcie04: Card not present on Slot=
(1)
> > > > [  608.835249] pciehp 0000:00:1c.1:pcie04: Card present on Slot(1)
> > > >
> > > > In this case following scenario happens,
> > > >
> > > > While removal:
> > > > pcie_isr() -> pciehp_queue_interrupt_event() -> triggers queue_work=
().
> > > > work invokes interrupt_event_handler() -> case INT_PRESENCE_OFF
> > > > and calls handle_surprise_event().
> > > >
> > > > handle_surprise_event() again calls pciehp_get_adapter_status()
> > > > and reads slot status which might have been changed
> > > > already due to PCI_EXP_SLTSTA_PDC event for hotplug insertion
> > > > has happened. So it queues, ENABLE_REQ for both removal
> > > > and insertion interrupt based on latest slot status.
> > > >
> > > > In this case, PCIe device can not be hot-add again because
> > > > it was never removed due to which device can not get enabled.
> > > >
> > > > handle_surprise_event() can be removed and pciehp_queue_power_work()
> > > > can be directly triggered based on INT_PRESENCE_ON and INT_PRESENCE=
_OFF
> > > > from the switch case exist in interrupt_event_hanlder().
> > > >
> > > > The patch ensures the pciehp_queue_power_work() processes
> > > > presence detect change for removal and insertion correctly.
> > > >
> > > > Signed-off-by: Mayurkumar Patel <mayurkumar.patel@intel.com>
> >
> > Acked-by: Rajat Jain <rajatxjain@gmail.com>
> >
> > >
> > > > ---
> > > >  Resending the patch addressing to PCI Maintainer Bjorn Helgaas.
> > > >
> > > >  drivers/pci/hotplug/pciehp_ctrl.c |   18 ++----------------
> > > >  1 file changed, 2 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplu=
g/pciehp_ctrl.c
> > > > index 880978b..87c5bea 100644
> > > > --- a/drivers/pci/hotplug/pciehp_ctrl.c
> > > > +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> > > > @@ -301,20 +301,6 @@ static void handle_button_press_event(struct s=
lot *p_slot)
> > > >  /*
> > > >   * Note: This function must be called with slot->lock held
> > > >   */
> > > > -static void handle_surprise_event(struct slot *p_slot)
> > > > -{
> > > > -     u8 getstatus;
> > > > -
> > > > -     pciehp_get_adapter_status(p_slot, &getstatus);
> > > > -     if (!getstatus)
> > > > -             pciehp_queue_power_work(p_slot, DISABLE_REQ);
> > > > -     else
> > > > -             pciehp_queue_power_work(p_slot, ENABLE_REQ);
> > > > -}
> > > > -
> > > > -/*
> > > > - * Note: This function must be called with slot->lock held
> > > > - */
> > > >  static void handle_link_event(struct slot *p_slot, u32 event)
> > > >  {
> > > >       struct controller *ctrl =3D p_slot->ctrl;
> > > > @@ -377,14 +363,14 @@ static void interrupt_event_handler(struct wo=
rk_struct *work)
> > > >               pciehp_green_led_off(p_slot);
> > > >               break;
> > > >       case INT_PRESENCE_ON:
> > > > -             handle_surprise_event(p_slot);
> > > > +             pciehp_queue_power_work(p_slot, ENABLE_REQ);
> > > >               break;
> > > >       case INT_PRESENCE_OFF:
> > > >               /*
> > > >                * Regardless of surprise capability, we need to
> > > >                * definitely remove a card that has been pulled out!
> > > >                */
> > > > -             handle_surprise_event(p_slot);
> > > > +             pciehp_queue_power_work(p_slot, DISABLE_REQ);
> > > >               break;
> > > >       case INT_LINK_UP:
> > > >       case INT_LINK_DOWN:
> > >
> > > Thanks a lot for this.  I think other people have seen the same issue.
> > >
> > > Even with this fix, don't we have essentially the same problem one
> > > layer back?  The first thing pcie_isr() does is read PCI_EXP_SLTSTA,
> > > then few lines down, we call pciehp_get_adapter_status(), which reads
> > > PCI_EXP_SLTSTA *again*.  So I think the window is smaller but still
> > > there.
> > >
> > > I think what we really should do is read the status registers
> > > (PCI_EXP_SLTSTA and probably also PCI_EXP_LNKSTA) *once* in
> > > pcie_isr(), before we write PCI_EXP_SLTSTA to clear the RW1C bits
> > > there, and then queue up events based on those values, without
> > > re-reading the registers.
> > >
> > > What do you think?
> >
> >
> > Yes, I agree. =


Yes indeed that should be done too.

> > We need to do something about that *in addition * to the
> > above patch to cover the
> > whole story. However I think there still will be a room for some
> > interrupt misses because we are
> > collecting the interrupts in intr_loc, and theoretically we could be
> > in a situation where in the pcie_isr, the
> >
> > do {
> >     ...
> > } while(detected)
> >
> > loop gets a removal->insertion->removal all while in the same
> > invocation of pcie_isr().
> > If this happens, the intr_loc will have recorded a single insertion
> > and a single removal, and
> > the final result will depend on the order in which we decide to
> > process the events in intr_loc.
> =

> I don't quite understand how that "do { .. } while (detected)" loop
> works or why it's done that way.  Collecting interrupt status bits in
> an ISR is obviously a very common task; it seems like there should be
> a standard, idiomatic way of doing it, but I don't know it.
> =

> > Or, may be we can make the calls to pciehp_queue_interrupt_event()
> > before clearing the
> > RW1C in the slot status register (in the loop)?
> =

> Yeah, it seems like we should read PCI_EXP_SLTSTA once, queue up any
> events related to it, then clear the relevant SLTSTA bits.
> =


Do you mean to remove the do {...} while loop and just
read PCI_EXP_SLTSTA once in ISR , queue the work and clear interrupts?


> Bjorn
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, 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

[PATCH v1 1/2] PCI: pciehp: Fix presence detect change interrupt handling

[Mayurkumar Patel]

Currently, if very fast hotplug removal and insertion event comes
as following

[  608.823412] pciehp 0000:00:1c.1:pcie04: Card not present on Slot(1)
[  608.835249] pciehp 0000:00:1c.1:pcie04: Card present on Slot(1)

In this case following scenario happens,

While removal:
pcie_isr() -> pciehp_queue_interrupt_event() -> triggers queue_work().
work invokes interrupt_event_handler() -> case INT_PRESENCE_OFF
and calls handle_surprise_event().

handle_surprise_event() again calls pciehp_get_adapter_status()
and reads slot status which might have been changed
already due to PCI_EXP_SLTSTA_PDC event for hotplug insertion
has happened. So it queues, ENABLE_REQ for both removal
and insertion interrupt based on latest slot status.

In this case, PCIe device can not be hot-add again because
it was never removed due to which device can not get enabled.

handle_surprise_event() can be removed and pciehp_queue_power_work()
can be directly triggered based on INT_PRESENCE_ON and INT_PRESENCE_OFF
from the switch case exist in interrupt_event_hanlder().

The patch ensures the pciehp_queue_power_work() processes
presence detect change for removal and insertion correctly.

Signed-off-by: Mayurkumar Patel <mayurkumar.patel@intel.com>
Acked-by: Rajat Jain <rajatxjain@gmail.com>
---
 drivers/pci/hotplug/pciehp_ctrl.c |   18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index 880978b..87c5bea 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -301,20 +301,6 @@ static void handle_button_press_event(struct slot *p_slot)
 /*
  * Note: This function must be called with slot->lock held
  */
-static void handle_surprise_event(struct slot *p_slot)
-{
-	u8 getstatus;
-
-	pciehp_get_adapter_status(p_slot, &getstatus);
-	if (!getstatus)
-		pciehp_queue_power_work(p_slot, DISABLE_REQ);
-	else
-		pciehp_queue_power_work(p_slot, ENABLE_REQ);
-}
-
-/*
- * Note: This function must be called with slot->lock held
- */
 static void handle_link_event(struct slot *p_slot, u32 event)
 {
 	struct controller *ctrl = p_slot->ctrl;
@@ -377,14 +363,14 @@ static void interrupt_event_handler(struct work_struct *work)
 		pciehp_green_led_off(p_slot);
 		break;
 	case INT_PRESENCE_ON:
-		handle_surprise_event(p_slot);
+		pciehp_queue_power_work(p_slot, ENABLE_REQ);
 		break;
 	case INT_PRESENCE_OFF:
 		/*
 		 * Regardless of surprise capability, we need to
 		 * definitely remove a card that has been pulled out!
 		 */
-		handle_surprise_event(p_slot);
+		pciehp_queue_power_work(p_slot, DISABLE_REQ);
 		break;
 	case INT_LINK_UP:
 	case INT_LINK_DOWN:
--
1.7.9.5

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

[Mayurkumar Patel]

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 is just one proposal. It is just prototype and currently
I don't know it would work any HW.
It 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 <mayurkumar.patel@intel.com>
---
 drivers/pci/hotplug/pciehp_hpc.c |   45 ++++++++++++++++----------------------
 1 file changed, 19 insertions(+), 26 deletions(-)

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 :
--
1.7.9.5