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

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

[Patel, Mayurkumar]

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



Mayurkumar Patel
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




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

[Bjorn Helgaas]

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

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?

Bjorn

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

[Rajat Jain]

On Wed, Aug 17, 2016 at 10:12 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> 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/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:
>
> 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. 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.

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)?

Thanks,

rajat

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

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

[Bjorn Helgaas]

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> wrote:
> >
> > 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/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:
> >
> > 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. 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.

Bjorn

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

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

[Bjorn Helgaas]

On Wed, Aug 17, 2016 at 10:37:13PM +0000, Patel, Mayurkumar wrote:
> > On Wed, Aug 17, 2016 at 10:54:12AM -0700, Rajat Jain wrote:

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

I don't know if removing the loop is the right thing or not.  We need
to understand why the loop is there in the first place and make sure
removing it wouldn't break things.

But I do think that in the resulting code, the connection between

  (1) the events we learn from the interrupt and
  (2) the queued work items

needs to be crystal clear.  Right now it's a bit muddy because of
things like the case you fixed: a work item that goes back and looks
at PCI_EXP_SLTSTA after it's been cleared and the hardware may have
already set bits for new events.

Bjorn

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

[Patel, Mayurkumar]

 =

> On Wed, Aug 17, 2016 at 10:37:13PM +0000, Patel, Mayurkumar wrote:
> > > On Wed, Aug 17, 2016 at 10:54:12AM -0700, Rajat Jain wrote:
> =

> > > > 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?
> =

> I don't know if removing the loop is the right thing or not.  We need
> to understand why the loop is there in the first place and make sure
> removing it wouldn't break things.
> =



The pcie base spec. 3.0 says from chapter 6.7.3.1. Slot Events:

	A Downstream Port with hot-plug capabilities monitors the slot it controls=
 for the slot events listed
	above. When one of these slot events is detected, the Port indicates that =
the event has occurred by
	setting the status field associated with the event. At that point, the eve=
nt is pending until software
	clears the status field.
	Once a slot event is pending on a particular slot, all subsequent events o=
f that type are ignored on
	that slot until the event is cleared. The Port must continue to monitor th=
e slot for all other slot
	event types and report them as they occur.


So does it mean that the port would continue providing MSI if there has been
any other events occurred apart from the event which is not cleared? If that
is the case then it's not sure why the loop is still needed.

Also with having a loop, pcie_isr() is reading the PCI_EXP_SLTSTA again and=
 taking action just based on
latest event happens. That may mean removal->insertion happens fast enough =
then
removal could be overwritten by insertion and pciehp_get_adapter_status ()
will return insertion only and we may miss the removal event to be processe=
d for the loop.

If we don't clear the Slot event quick enough then according to spec. state=
ment above
that event could get ignored and SW may never get notified.



> But I do think that in the resulting code, the connection between
> =

>   (1) the events we learn from the interrupt and
>   (2) the queued work items
> =

> needs to be crystal clear.  Right now it's a bit muddy because of
> things like the case you fixed: a work item that goes back and looks
> at PCI_EXP_SLTSTA after it's been cleared and the hardware may have
> already set bits for new events.
> =


I have made a prototype patch in my follow up reply and tested ok on my exi=
sting setup =

on which I caught the previous issue although I cant say it will work on an=
y HW.
Clearing events after queuing events gave some problems and did not work pr=
operly on my
existing HW where I test very fast insertion and removal events, in which
case only "removal" event comes and "insertion" does not occur even if HW g=
ets powered.

> 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

[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] PCI: pciehp: Fix presence detect change interrupt handling

[Rajat Jain]

On Thu, Aug 18, 2016 at 1:59 PM, Patel, Mayurkumar
<mayurkumar.patel@intel.com> wrote:
>
>
>
> > On Wed, Aug 17, 2016 at 10:37:13PM +0000, Patel, Mayurkumar wrote:
> > > > On Wed, Aug 17, 2016 at 10:54:12AM -0700, Rajat Jain wrote:
> >
> > > > > 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?
> >
> > I don't know if removing the loop is the right thing or not.  We need
> > to understand why the loop is there in the first place and make sure
> > removing it wouldn't break things.
> >
>
>
> The pcie base spec. 3.0 says from chapter 6.7.3.1. Slot Events:
>
>         A Downstream Port with hot-plug capabilities monitors the slot it controls for the slot events listed
>         above. When one of these slot events is detected, the Port indicates that the event has occurred by
>         setting the status field associated with the event. At that point, the event is pending until software
>         clears the status field.
>         Once a slot event is pending on a particular slot, all subsequent events of that type are ignored on
>         that slot until the event is cleared. The Port must continue to monitor the slot for all other slot
>         event types and report them as they occur.
>
>
> So does it mean that the port would continue providing MSI if there has been
> any other events occurred apart from the event which is not cleared? If that
> is the case then it's not sure why the loop is still needed.

I'd think that the loop is needed because we don't want to handle just
one event on one interrupt. We want to handle as many events as we can
(to keep the interrupt latency overhead low), that have happened
during the ISR invocation. And hence the loop.

>
> Also with having a loop, pcie_isr() is reading the PCI_EXP_SLTSTA again and taking action just based on
> latest event happens. That may mean removal->insertion happens fast enough then
> removal could be overwritten by insertion and pciehp_get_adapter_status ()
> will return insertion only and we may miss the removal event to be processed for the loop.

Current. This is the problem that Bjorn highlighted above. I think the
current code works well when events of different kinds have happened
(due to the loop).

In case of similar kind of events (insertion / removal), I think there
are 2 problems: (1) we use the transient values of slot status which
may not be valid at that time. (2) because we ack the event before we
queue it up, it leaves chance for error. I think we'd need more
careful examination in order to fix these issues.

>
> If we don't clear the Slot event quick enough then according to spec. statement above
> that event could get ignored and SW may never get notified.
>
>
>
> > But I do think that in the resulting code, the connection between
> >
> >   (1) the events we learn from the interrupt and
> >   (2) the queued work items
> >
> > needs to be crystal clear.  Right now it's a bit muddy because of
> > things like the case you fixed: a work item that goes back and looks
> > at PCI_EXP_SLTSTA after it's been cleared and the hardware may have
> > already set bits for new events.
> >
>
> I have made a prototype patch in my follow up reply and tested ok on my existing setup
> on which I caught the previous issue although I cant say it will work on any HW.
> Clearing events after queuing events gave some problems and did not work properly on my
> existing HW where I test very fast insertion and removal events, in which
> case only "removal" event comes and "insertion" does not occur even if HW gets powered.
>
> > 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
>

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

[Patel, Mayurkumar]

DQo+ID4NCj4gPiA+IE9uIFdlZCwgQXVnIDE3LCAyMDE2IGF0IDEwOjM3OjEzUE0gKzAwMDAsIFBh
dGVsLCBNYXl1cmt1bWFyIHdyb3RlOg0KPiA+ID4gPiA+IE9uIFdlZCwgQXVnIDE3LCAyMDE2IGF0
IDEwOjU0OjEyQU0gLTA3MDAsIFJhamF0IEphaW4gd3JvdGU6DQo+ID4gPg0KPiA+ID4gPiA+ID4g
V2UgbmVlZCB0byBkbyBzb21ldGhpbmcgYWJvdXQgdGhhdCAqaW4gYWRkaXRpb24gKiB0byB0aGUN
Cj4gPiA+ID4gPiA+IGFib3ZlIHBhdGNoIHRvIGNvdmVyIHRoZQ0KPiA+ID4gPiA+ID4gd2hvbGUg
c3RvcnkuIEhvd2V2ZXIgSSB0aGluayB0aGVyZSBzdGlsbCB3aWxsIGJlIGEgcm9vbSBmb3Igc29t
ZQ0KPiA+ID4gPiA+ID4gaW50ZXJydXB0IG1pc3NlcyBiZWNhdXNlIHdlIGFyZQ0KPiA+ID4gPiA+
ID4gY29sbGVjdGluZyB0aGUgaW50ZXJydXB0cyBpbiBpbnRyX2xvYywgYW5kIHRoZW9yZXRpY2Fs
bHkgd2UgY291bGQgYmUNCj4gPiA+ID4gPiA+IGluIGEgc2l0dWF0aW9uIHdoZXJlIGluIHRoZSBw
Y2llX2lzciwgdGhlDQo+ID4gPiA+ID4gPg0KPiA+ID4gPiA+ID4gZG8gew0KPiA+ID4gPiA+ID4g
ICAgIC4uLg0KPiA+ID4gPiA+ID4gfSB3aGlsZShkZXRlY3RlZCkNCj4gPiA+ID4gPiA+DQo+ID4g
PiA+ID4gPiBsb29wIGdldHMgYSByZW1vdmFsLT5pbnNlcnRpb24tPnJlbW92YWwgYWxsIHdoaWxl
IGluIHRoZSBzYW1lDQo+ID4gPiA+ID4gPiBpbnZvY2F0aW9uIG9mIHBjaWVfaXNyKCkuDQo+ID4g
PiA+ID4gPiBJZiB0aGlzIGhhcHBlbnMsIHRoZSBpbnRyX2xvYyB3aWxsIGhhdmUgcmVjb3JkZWQg
YSBzaW5nbGUgaW5zZXJ0aW9uDQo+ID4gPiA+ID4gPiBhbmQgYSBzaW5nbGUgcmVtb3ZhbCwgYW5k
DQo+ID4gPiA+ID4gPiB0aGUgZmluYWwgcmVzdWx0IHdpbGwgZGVwZW5kIG9uIHRoZSBvcmRlciBp
biB3aGljaCB3ZSBkZWNpZGUgdG8NCj4gPiA+ID4gPiA+IHByb2Nlc3MgdGhlIGV2ZW50cyBpbiBp
bnRyX2xvYy4NCj4gPiA+ID4gPg0KPiA+ID4gPiA+IEkgZG9uJ3QgcXVpdGUgdW5kZXJzdGFuZCBo
b3cgdGhhdCAiZG8geyAuLiB9IHdoaWxlIChkZXRlY3RlZCkiIGxvb3ANCj4gPiA+ID4gPiB3b3Jr
cyBvciB3aHkgaXQncyBkb25lIHRoYXQgd2F5LiAgQ29sbGVjdGluZyBpbnRlcnJ1cHQgc3RhdHVz
IGJpdHMgaW4NCj4gPiA+ID4gPiBhbiBJU1IgaXMgb2J2aW91c2x5IGEgdmVyeSBjb21tb24gdGFz
azsgaXQgc2VlbXMgbGlrZSB0aGVyZSBzaG91bGQgYmUNCj4gPiA+ID4gPiBhIHN0YW5kYXJkLCBp
ZGlvbWF0aWMgd2F5IG9mIGRvaW5nIGl0LCBidXQgSSBkb24ndCBrbm93IGl0Lg0KPiA+ID4gPiA+
DQo+ID4gPiA+ID4gPiBPciwgbWF5IGJlIHdlIGNhbiBtYWtlIHRoZSBjYWxscyB0byBwY2llaHBf
cXVldWVfaW50ZXJydXB0X2V2ZW50KCkNCj4gPiA+ID4gPiA+IGJlZm9yZSBjbGVhcmluZyB0aGUN
Cj4gPiA+ID4gPiA+IFJXMUMgaW4gdGhlIHNsb3Qgc3RhdHVzIHJlZ2lzdGVyIChpbiB0aGUgbG9v
cCk/DQo+ID4gPiA+ID4NCj4gPiA+ID4gPiBZZWFoLCBpdCBzZWVtcyBsaWtlIHdlIHNob3VsZCBy
ZWFkIFBDSV9FWFBfU0xUU1RBIG9uY2UsIHF1ZXVlIHVwIGFueQ0KPiA+ID4gPiA+IGV2ZW50cyBy
ZWxhdGVkIHRvIGl0LCB0aGVuIGNsZWFyIHRoZSByZWxldmFudCBTTFRTVEEgYml0cy4NCj4gPiA+
ID4gPg0KPiA+ID4gPg0KPiA+ID4gPiBEbyB5b3UgbWVhbiB0byByZW1vdmUgdGhlIGRvIHsuLi59
IHdoaWxlIGxvb3AgYW5kIGp1c3QNCj4gPiA+ID4gcmVhZCBQQ0lfRVhQX1NMVFNUQSBvbmNlIGlu
IElTUiAsIHF1ZXVlIHRoZSB3b3JrIGFuZCBjbGVhciBpbnRlcnJ1cHRzPw0KPiA+ID4NCj4gPiA+
IEkgZG9uJ3Qga25vdyBpZiByZW1vdmluZyB0aGUgbG9vcCBpcyB0aGUgcmlnaHQgdGhpbmcgb3Ig
bm90LiAgV2UgbmVlZA0KPiA+ID4gdG8gdW5kZXJzdGFuZCB3aHkgdGhlIGxvb3AgaXMgdGhlcmUg
aW4gdGhlIGZpcnN0IHBsYWNlIGFuZCBtYWtlIHN1cmUNCj4gPiA+IHJlbW92aW5nIGl0IHdvdWxk
bid0IGJyZWFrIHRoaW5ncy4NCj4gPiA+DQo+ID4NCj4gPg0KPiA+IFRoZSBwY2llIGJhc2Ugc3Bl
Yy4gMy4wIHNheXMgZnJvbSBjaGFwdGVyIDYuNy4zLjEuIFNsb3QgRXZlbnRzOg0KPiA+DQo+ID4g
ICAgICAgICBBIERvd25zdHJlYW0gUG9ydCB3aXRoIGhvdC1wbHVnIGNhcGFiaWxpdGllcyBtb25p
dG9ycyB0aGUgc2xvdCBpdCBjb250cm9scyBmb3IgdGhlIHNsb3QgZXZlbnRzIGxpc3RlZA0KPiA+
ICAgICAgICAgYWJvdmUuIFdoZW4gb25lIG9mIHRoZXNlIHNsb3QgZXZlbnRzIGlzIGRldGVjdGVk
LCB0aGUgUG9ydCBpbmRpY2F0ZXMgdGhhdCB0aGUgZXZlbnQgaGFzIG9jY3VycmVkIGJ5DQo+ID4g
ICAgICAgICBzZXR0aW5nIHRoZSBzdGF0dXMgZmllbGQgYXNzb2NpYXRlZCB3aXRoIHRoZSBldmVu
dC4gQXQgdGhhdCBwb2ludCwgdGhlIGV2ZW50IGlzIHBlbmRpbmcgdW50aWwgc29mdHdhcmUNCj4g
PiAgICAgICAgIGNsZWFycyB0aGUgc3RhdHVzIGZpZWxkLg0KPiA+ICAgICAgICAgT25jZSBhIHNs
b3QgZXZlbnQgaXMgcGVuZGluZyBvbiBhIHBhcnRpY3VsYXIgc2xvdCwgYWxsIHN1YnNlcXVlbnQg
ZXZlbnRzIG9mIHRoYXQgdHlwZSBhcmUgaWdub3JlZCBvbg0KPiA+ICAgICAgICAgdGhhdCBzbG90
IHVudGlsIHRoZSBldmVudCBpcyBjbGVhcmVkLiBUaGUgUG9ydCBtdXN0IGNvbnRpbnVlIHRvIG1v
bml0b3IgdGhlIHNsb3QgZm9yIGFsbCBvdGhlciBzbG90DQo+ID4gICAgICAgICBldmVudCB0eXBl
cyBhbmQgcmVwb3J0IHRoZW0gYXMgdGhleSBvY2N1ci4NCj4gPg0KPiA+DQo+ID4gU28gZG9lcyBp
dCBtZWFuIHRoYXQgdGhlIHBvcnQgd291bGQgY29udGludWUgcHJvdmlkaW5nIE1TSSBpZiB0aGVy
ZSBoYXMgYmVlbg0KPiA+IGFueSBvdGhlciBldmVudHMgb2NjdXJyZWQgYXBhcnQgZnJvbSB0aGUg
ZXZlbnQgd2hpY2ggaXMgbm90IGNsZWFyZWQ/IElmIHRoYXQNCj4gPiBpcyB0aGUgY2FzZSB0aGVu
IGl0J3Mgbm90IHN1cmUgd2h5IHRoZSBsb29wIGlzIHN0aWxsIG5lZWRlZC4NCj4gDQo+IEknZCB0
aGluayB0aGF0IHRoZSBsb29wIGlzIG5lZWRlZCBiZWNhdXNlIHdlIGRvbid0IHdhbnQgdG8gaGFu
ZGxlIGp1c3QNCj4gb25lIGV2ZW50IG9uIG9uZSBpbnRlcnJ1cHQuIFdlIHdhbnQgdG8gaGFuZGxl
IGFzIG1hbnkgZXZlbnRzIGFzIHdlIGNhbg0KPiAodG8ga2VlcCB0aGUgaW50ZXJydXB0IGxhdGVu
Y3kgb3ZlcmhlYWQgbG93KSwgdGhhdCBoYXZlIGhhcHBlbmVkDQo+IGR1cmluZyB0aGUgSVNSIGlu
dm9jYXRpb24uIEFuZCBoZW5jZSB0aGUgbG9vcC4NCj4gDQoNCkhtbSwgUmlnaHQuIEkgY3JlYXRl
ZCBhIHByb3Bvc2FsIHBhdGNoDQooPT5bdjEsMi8yXSBQQ0k6IHBjaWVocDogUmV3b3JrIGhvdHBs
dWcgaW50ZXJydXB0IHJvdXRpbmUpDQp0byByZW1vdmUgdGhlIGRvIHsuLi59IHdoaWxlIGxvb3Ag
d2hpY2ggDQpJIGd1ZXNzIHRoZW4gc2hvdWxkIGJlIHJlamVjdGVkLCByaWdodD8gQXMgd2UgZGVj
aWRlIHRvIGtlZXAgdGhpcyBsb29wPw0KDQo+ID4NCj4gPiBBbHNvIHdpdGggaGF2aW5nIGEgbG9v
cCwgcGNpZV9pc3IoKSBpcyByZWFkaW5nIHRoZSBQQ0lfRVhQX1NMVFNUQSBhZ2FpbiBhbmQgdGFr
aW5nIGFjdGlvbiBqdXN0IGJhc2VkIG9uDQo+ID4gbGF0ZXN0IGV2ZW50IGhhcHBlbnMuIFRoYXQg
bWF5IG1lYW4gcmVtb3ZhbC0+aW5zZXJ0aW9uIGhhcHBlbnMgZmFzdCBlbm91Z2ggdGhlbg0KPiA+
IHJlbW92YWwgY291bGQgYmUgb3ZlcndyaXR0ZW4gYnkgaW5zZXJ0aW9uIGFuZCBwY2llaHBfZ2V0
X2FkYXB0ZXJfc3RhdHVzICgpDQo+ID4gd2lsbCByZXR1cm4gaW5zZXJ0aW9uIG9ubHkgYW5kIHdl
IG1heSBtaXNzIHRoZSByZW1vdmFsIGV2ZW50IHRvIGJlIHByb2Nlc3NlZCBmb3IgdGhlIGxvb3Au
DQo+IA0KPiBDdXJyZW50LiBUaGlzIGlzIHRoZSBwcm9ibGVtIHRoYXQgQmpvcm4gaGlnaGxpZ2h0
ZWQgYWJvdmUuIEkgdGhpbmsgdGhlDQo+IGN1cnJlbnQgY29kZSB3b3JrcyB3ZWxsIHdoZW4gZXZl
bnRzIG9mIGRpZmZlcmVudCBraW5kcyBoYXZlIGhhcHBlbmVkDQo+IChkdWUgdG8gdGhlIGxvb3Ap
Lg0KPiANCj4gSW4gY2FzZSBvZiBzaW1pbGFyIGtpbmQgb2YgZXZlbnRzIChpbnNlcnRpb24gLyBy
ZW1vdmFsKSwgSSB0aGluayB0aGVyZQ0KPiBhcmUgMiBwcm9ibGVtczogKDEpIHdlIHVzZSB0aGUg
dHJhbnNpZW50IHZhbHVlcyBvZiBzbG90IHN0YXR1cyB3aGljaA0KPiBtYXkgbm90IGJlIHZhbGlk
IGF0IHRoYXQgdGltZS4gKDIpIGJlY2F1c2Ugd2UgYWNrIHRoZSBldmVudCBiZWZvcmUgd2UNCj4g
cXVldWUgaXQgdXAsIGl0IGxlYXZlcyBjaGFuY2UgZm9yIGVycm9yLiBJIHRoaW5rIHdlJ2QgbmVl
ZCBtb3JlDQo+IGNhcmVmdWwgZXhhbWluYXRpb24gaW4gb3JkZXIgdG8gZml4IHRoZXNlIGlzc3Vl
cy4NCj4gDQoNCkkgYWxzbyBzZWUgM3JkIHByb2JsZW0gdG9vIHdpdGggdGhpcyBMb29wLiANCkZv
ciBleGFtcGxlLCBpZiB3ZSBhcmUgaW4gdGhlIGxvb3AsDQpJZiBvbmx5IHNsb3QgcmVtb3ZhbCBj
b21lcyAtPiBkZXRlY3QgPSBQQ0lfRVhQX1NMVFNUQV9QREMgLT4gaW50cl9sb2NrIHw9IGRldGVj
dGVkDQotPiBkZXRlY3RlZD09dHJ1ZSBzbyBjbGVhciBQREMgaW50ZXJydXB0IGJpdCA9PiBTbyB3
ZSBoYXZlIGEgY2hhbmNlIHRvIGRldGVjdA0KYW5vdGhlciBpbnNlcnRpb24gZXZlbnQuDQoNCldl
IGFyZSBpbiB0aGUgc2FtZSBsb29wICYgaWYgaW5zZXJ0aW9uIGNvbWVzIGFnYWluIC0+IHNvIGRl
dGVjdGVkID0gUENJX0VYUF9TTFRTVEFfUERDDQotPiB0aGVuIGRldGVjdGVkICY9IH5pbnRyX2xv
YyAtPiBkZXRlY3RlZCA9PSBmYWxzZSA9PiBUaGlzIG1lYW5zDQpQQ0lfRVhQX1NMVFNUQSB3b3Vs
ZCBub3QgYmUgY2xlYXJlZCBmb3IgaW5zZXJ0aW9uIGV2ZW50IGhhcHBlbmVkIA0KYW5kIHRoZXJl
IHdlIGNvdWxkIGdldCBhIHByb2JsZW0gdGhhdCBObyBmdXJ0aGVyIFBEQyBldmVudHMgY2FuIGdl
dCB0cmlnZ2VyZWQgYnkgSFcuDQoNClNvIGlmIHdlIHdhbnQgdG8ga2VlcCB0aGUgbG9vcCwgSSB0
aGluayBjbGVhcmluZyBQQ0lfRVhQX1NMVFNUQSAgc2hvdWxkIGJlIHNoaWZ0ZWQNCm91dHNpZGUg
b2YgdGhlIGxvb3AgdG8gY2xlYXIgU2xvdCBldmVudHMoYnV0IG5vdCBiYXNlZCBvbiBkZXRlY3Rl
ZCkgdG8gZW5zdXJlIHRoYXQgbG9vcA0KanVzdCBsb29rcyBmb3IgdGhlIGV2ZW50cyBXaGljaCBo
YXMgbm90IG9jY3VycmVkLCBhbHNvIGJlZm9yZSBjbGVhcmluZyBTTFRTVEEgb3V0c2lkZQ0KdGhl
IGxvb3AsIHdlIGNhbiB1cGRhdGUgbGluayBhbmQgc2xvdF9zdGF0dXMgdG8gdHJpZ2dlciB0aGUg
cmlnaHQgZXZlbnRzIGZvciBETExTQyBhbmQgUERDIHR5cGVzLg0KDQpXaGF0IGRvIHlvdSB0aGlu
ayBhYm91dCBmb2xsb3dpbmcgcHJvcG9zYWw/DQoNCkBAIC01NDIsNyArNTQyLDcgQEAgc3RhdGlj
IGlycXJldHVybl90IHBjaWVfaXNyKGludCBpcnEsIHZvaWQgKmRldl9pZCkNCiAJc3RydWN0IHBj
aV9idXMgKnN1Ym9yZGluYXRlID0gcGRldi0+c3Vib3JkaW5hdGU7DQogCXN0cnVjdCBwY2lfZGV2
ICpkZXY7DQogCXN0cnVjdCBzbG90ICpzbG90ID0gY3RybC0+c2xvdDsNCi0JdTE2IGRldGVjdGVk
LCBpbnRyX2xvYzsNCisJdTE2IGRldGVjdGVkLCBpbnRyX2xvYywgc2xvdF9zdGF0dXM7DQogCXU4
IHByZXNlbnQ7DQogCWJvb2wgbGluazsNCiANCkBAIC01NTMsMjYgKzU1MywzMCBAQCBzdGF0aWMg
aXJxcmV0dXJuX3QgcGNpZV9pc3IoaW50IGlycSwgdm9pZCAqZGV2X2lkKQ0KIAkgKi8NCiAJaW50
cl9sb2MgPSAwOw0KIAlkbyB7DQotCQlwY2llX2NhcGFiaWxpdHlfcmVhZF93b3JkKHBkZXYsIFBD
SV9FWFBfU0xUU1RBLCAmZGV0ZWN0ZWQpOw0KLQkJaWYgKGRldGVjdGVkID09ICh1MTYpIH4wKSB7
DQorCQlwY2llX2NhcGFiaWxpdHlfcmVhZF93b3JkKHBkZXYsIFBDSV9FWFBfU0xUU1RBLCAmc2xv
dF9zdGF0dXMpOw0KKwkJaWYgKHNsb3Rfc3RhdHVzID09ICh1MTYpIH4wKSB7DQogCQkJY3RybF9p
bmZvKGN0cmwsICIlczogbm8gcmVzcG9uc2UgZnJvbSBkZXZpY2VcbiIsDQogCQkJCSAgX19mdW5j
X18pOw0KIAkJCXJldHVybiBJUlFfSEFORExFRDsNCiAJCX0NCiANCi0JCWRldGVjdGVkICY9IChQ
Q0lfRVhQX1NMVFNUQV9BQlAgfCBQQ0lfRVhQX1NMVFNUQV9QRkQgfA0KKwkJZGV0ZWN0ZWQgPSBz
bG90X3N0YXR1cyAmIChQQ0lfRVhQX1NMVFNUQV9BQlAgfA0KKwkJCSAgICAgUENJX0VYUF9TTFRT
VEFfUEZEIHwNCiAJCQkgICAgIFBDSV9FWFBfU0xUU1RBX1BEQyB8DQogCQkJICAgICBQQ0lfRVhQ
X1NMVFNUQV9DQyB8IFBDSV9FWFBfU0xUU1RBX0RMTFNDKTsNCiAJCWRldGVjdGVkICY9IH5pbnRy
X2xvYzsNCiAJCWludHJfbG9jIHw9IGRldGVjdGVkOw0KIAkJaWYgKCFpbnRyX2xvYykNCiAJCQly
ZXR1cm4gSVJRX05PTkU7DQotCQlpZiAoZGV0ZWN0ZWQpDQotCQkJcGNpZV9jYXBhYmlsaXR5X3dy
aXRlX3dvcmQocGRldiwgUENJX0VYUF9TTFRTVEEsDQotCQkJCQkJICAgaW50cl9sb2MpOw0KIAl9
IHdoaWxlIChkZXRlY3RlZCk7DQogDQogCWN0cmxfZGJnKGN0cmwsICJwZW5kaW5nIGludGVycnVw
dHMgJSMwNnggZnJvbSBTbG90IFN0YXR1c1xuIiwgaW50cl9sb2MpOw0KKwlpZiAoaW50cl9sb2Mg
JiBQQ0lfRVhQX1NMVFNUQV9QREMpDQorCQlwcmVzZW50ID0gISEoc2xvdF9zdGF0dXMgJiBQQ0lf
RVhQX1NMVFNUQV9QRFMpOw0KKwlpZiAoaW50cl9sb2MgJiBQQ0lfRVhQX1NMVFNUQV9ETExTQykN
CisJCWxpbmsgPSBwY2llaHBfY2hlY2tfbGlua19hY3RpdmUoY3RybCk7DQorDQorCXBjaWVfY2Fw
YWJpbGl0eV93cml0ZV93b3JkKHBkZXYsIFBDSV9FWFBfU0xUU1RBLCBpbnRyX2xvYyk7DQogDQog
CS8qIENoZWNrIENvbW1hbmQgQ29tcGxldGUgSW50ZXJydXB0IFBlbmRpbmcgKi8NCiAJaWYgKGlu
dHJfbG9jICYgUENJX0VYUF9TTFRTVEFfQ0MpIHsNCkBAIC02MDMsNyArNjA3LDYgQEAgc3RhdGlj
IGlycXJldHVybl90IHBjaWVfaXNyKGludCBpcnEsIHZvaWQgKmRldl9pZCkNCiANCiAJLyogQ2hl
Y2sgUHJlc2VuY2UgRGV0ZWN0IENoYW5nZWQgKi8NCiAJaWYgKGludHJfbG9jICYgUENJX0VYUF9T
TFRTVEFfUERDKSB7DQotCQlwY2llaHBfZ2V0X2FkYXB0ZXJfc3RhdHVzKHNsb3QsICZwcmVzZW50
KTsNCiAJCWN0cmxfaW5mbyhjdHJsLCAiQ2FyZCAlc3ByZXNlbnQgb24gU2xvdCglcylcbiIsDQog
CQkJICBwcmVzZW50ID8gIiIgOiAibm90ICIsIHNsb3RfbmFtZShzbG90KSk7DQogCQlwY2llaHBf
cXVldWVfaW50ZXJydXB0X2V2ZW50KHNsb3QsIHByZXNlbnQgPyBJTlRfUFJFU0VOQ0VfT04gOg0K
QEAgLTYxOCw3ICs2MjEsNiBAQCBzdGF0aWMgaXJxcmV0dXJuX3QgcGNpZV9pc3IoaW50IGlycSwg
dm9pZCAqZGV2X2lkKQ0KIAl9DQogDQogCWlmIChpbnRyX2xvYyAmIFBDSV9FWFBfU0xUU1RBX0RM
TFNDKSB7DQotCQlsaW5rID0gcGNpZWhwX2NoZWNrX2xpbmtfYWN0aXZlKGN0cmwpOw0KIAkJY3Ry
bF9pbmZvKGN0cmwsICJzbG90KCVzKTogTGluayAlcyBldmVudFxuIiwNCiAJCQkgIHNsb3RfbmFt
ZShzbG90KSwgbGluayA/ICJVcCIgOiAiRG93biIpOw0KIAkJcGNpZWhwX3F1ZXVlX2ludGVycnVw
dF9ldmVudChzbG90LCBsaW5rID8gSU5UX0xJTktfVVAgOg0KDQoNCj4gPg0KPiA+IElmIHdlIGRv
bid0IGNsZWFyIHRoZSBTbG90IGV2ZW50IHF1aWNrIGVub3VnaCB0aGVuIGFjY29yZGluZyB0byBz
cGVjLiBzdGF0ZW1lbnQgYWJvdmUNCj4gPiB0aGF0IGV2ZW50IGNvdWxkIGdldCBpZ25vcmVkIGFu
ZCBTVyBtYXkgbmV2ZXIgZ2V0IG5vdGlmaWVkLg0KPiA+DQo+ID4NCj4gPg0KPiA+ID4gQnV0IEkg
ZG8gdGhpbmsgdGhhdCBpbiB0aGUgcmVzdWx0aW5nIGNvZGUsIHRoZSBjb25uZWN0aW9uIGJldHdl
ZW4NCj4gPiA+DQo+ID4gPiAgICgxKSB0aGUgZXZlbnRzIHdlIGxlYXJuIGZyb20gdGhlIGludGVy
cnVwdCBhbmQNCj4gPiA+ICAgKDIpIHRoZSBxdWV1ZWQgd29yayBpdGVtcw0KPiA+ID4NCj4gPiA+
IG5lZWRzIHRvIGJlIGNyeXN0YWwgY2xlYXIuICBSaWdodCBub3cgaXQncyBhIGJpdCBtdWRkeSBi
ZWNhdXNlIG9mDQo+ID4gPiB0aGluZ3MgbGlrZSB0aGUgY2FzZSB5b3UgZml4ZWQ6IGEgd29yayBp
dGVtIHRoYXQgZ29lcyBiYWNrIGFuZCBsb29rcw0KPiA+ID4gYXQgUENJX0VYUF9TTFRTVEEgYWZ0
ZXIgaXQncyBiZWVuIGNsZWFyZWQgYW5kIHRoZSBoYXJkd2FyZSBtYXkgaGF2ZQ0KPiA+ID4gYWxy
ZWFkeSBzZXQgYml0cyBmb3IgbmV3IGV2ZW50cy4NCj4gPiA+DQo+ID4NCj4gPiBJIGhhdmUgbWFk
ZSBhIHByb3RvdHlwZSBwYXRjaCBpbiBteSBmb2xsb3cgdXAgcmVwbHkgYW5kIHRlc3RlZCBvayBv
biBteSBleGlzdGluZyBzZXR1cA0KPiA+IG9uIHdoaWNoIEkgY2F1Z2h0IHRoZSBwcmV2aW91cyBp
c3N1ZSBhbHRob3VnaCBJIGNhbnQgc2F5IGl0IHdpbGwgd29yayBvbiBhbnkgSFcuDQo+ID4gQ2xl
YXJpbmcgZXZlbnRzIGFmdGVyIHF1ZXVpbmcgZXZlbnRzIGdhdmUgc29tZSBwcm9ibGVtcyBhbmQg
ZGlkIG5vdCB3b3JrIHByb3Blcmx5IG9uIG15DQo+ID4gZXhpc3RpbmcgSFcgd2hlcmUgSSB0ZXN0
IHZlcnkgZmFzdCBpbnNlcnRpb24gYW5kIHJlbW92YWwgZXZlbnRzLCBpbiB3aGljaA0KPiA+IGNh
c2Ugb25seSAicmVtb3ZhbCIgZXZlbnQgY29tZXMgYW5kICJpbnNlcnRpb24iIGRvZXMgbm90IG9j
Y3VyIGV2ZW4gaWYgSFcgZ2V0cyBwb3dlcmVkLg0KPiA+DQoNCg0KSW50ZWwgRGV1dHNjaGxhbmQg
R21iSApSZWdpc3RlcmVkIEFkZHJlc3M6IEFtIENhbXBlb24gMTAtMTIsIDg1NTc5IE5ldWJpYmVy
ZywgR2VybWFueQpUZWw6ICs0OSA4OSA5OSA4ODUzLTAsIHd3dy5pbnRlbC5kZQpNYW5hZ2luZyBE
aXJlY3RvcnM6IENocmlzdGluIEVpc2Vuc2NobWlkLCBDaHJpc3RpYW4gTGFtcHJlY2h0ZXIKQ2hh
aXJwZXJzb24gb2YgdGhlIFN1cGVydmlzb3J5IEJvYXJkOiBOaWNvbGUgTGF1ClJlZ2lzdGVyZWQg
T2ZmaWNlOiBNdW5pY2gKQ29tbWVyY2lhbCBSZWdpc3RlcjogQW10c2dlcmljaHQgTXVlbmNoZW4g
SFJCIDE4NjkyOAo=

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

[Keith Busch]

On Tue, Aug 23, 2016 at 04:47:44PM -0700, Rajat Jain wrote:
> On Thu, Aug 18, 2016 at 1:59 PM, Patel, Mayurkumar
> <mayurkumar.patel@intel.com> wrote:
> >
> > So does it mean that the port would continue providing MSI if there has been
> > any other events occurred apart from the event which is not cleared? If that
> > is the case then it's not sure why the loop is still needed.
> 
> I'd think that the loop is needed because we don't want to handle just
> one event on one interrupt. We want to handle as many events as we can
> (to keep the interrupt latency overhead low), that have happened
> during the ISR invocation. And hence the loop.

The Slot Status doesn't have to provide a single event at a time. But
even if it does, this isn't exactly a performance path that would be
hurt by having one interrupt per event, right?

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

[Patel, Mayurkumar]

DQogDQo+IA0KPiA+ID4NCj4gPiA+ID4gT24gV2VkLCBBdWcgMTcsIDIwMTYgYXQgMTA6Mzc6MTNQ
TSArMDAwMCwgUGF0ZWwsIE1heXVya3VtYXIgd3JvdGU6DQo+ID4gPiA+ID4gPiBPbiBXZWQsIEF1
ZyAxNywgMjAxNiBhdCAxMDo1NDoxMkFNIC0wNzAwLCBSYWphdCBKYWluIHdyb3RlOg0KPiA+ID4g
Pg0KPiA+ID4gPiA+ID4gPiBXZSBuZWVkIHRvIGRvIHNvbWV0aGluZyBhYm91dCB0aGF0ICppbiBh
ZGRpdGlvbiAqIHRvIHRoZQ0KPiA+ID4gPiA+ID4gPiBhYm92ZSBwYXRjaCB0byBjb3ZlciB0aGUN
Cj4gPiA+ID4gPiA+ID4gd2hvbGUgc3RvcnkuIEhvd2V2ZXIgSSB0aGluayB0aGVyZSBzdGlsbCB3
aWxsIGJlIGEgcm9vbSBmb3Igc29tZQ0KPiA+ID4gPiA+ID4gPiBpbnRlcnJ1cHQgbWlzc2VzIGJl
Y2F1c2Ugd2UgYXJlDQo+ID4gPiA+ID4gPiA+IGNvbGxlY3RpbmcgdGhlIGludGVycnVwdHMgaW4g
aW50cl9sb2MsIGFuZCB0aGVvcmV0aWNhbGx5IHdlIGNvdWxkIGJlDQo+ID4gPiA+ID4gPiA+IGlu
IGEgc2l0dWF0aW9uIHdoZXJlIGluIHRoZSBwY2llX2lzciwgdGhlDQo+ID4gPiA+ID4gPiA+DQo+
ID4gPiA+ID4gPiA+IGRvIHsNCj4gPiA+ID4gPiA+ID4gICAgIC4uLg0KPiA+ID4gPiA+ID4gPiB9
IHdoaWxlKGRldGVjdGVkKQ0KPiA+ID4gPiA+ID4gPg0KPiA+ID4gPiA+ID4gPiBsb29wIGdldHMg
YSByZW1vdmFsLT5pbnNlcnRpb24tPnJlbW92YWwgYWxsIHdoaWxlIGluIHRoZSBzYW1lDQo+ID4g
PiA+ID4gPiA+IGludm9jYXRpb24gb2YgcGNpZV9pc3IoKS4NCj4gPiA+ID4gPiA+ID4gSWYgdGhp
cyBoYXBwZW5zLCB0aGUgaW50cl9sb2Mgd2lsbCBoYXZlIHJlY29yZGVkIGEgc2luZ2xlIGluc2Vy
dGlvbg0KPiA+ID4gPiA+ID4gPiBhbmQgYSBzaW5nbGUgcmVtb3ZhbCwgYW5kDQo+ID4gPiA+ID4g
PiA+IHRoZSBmaW5hbCByZXN1bHQgd2lsbCBkZXBlbmQgb24gdGhlIG9yZGVyIGluIHdoaWNoIHdl
IGRlY2lkZSB0bw0KPiA+ID4gPiA+ID4gPiBwcm9jZXNzIHRoZSBldmVudHMgaW4gaW50cl9sb2Mu
DQo+ID4gPiA+ID4gPg0KPiA+ID4gPiA+ID4gSSBkb24ndCBxdWl0ZSB1bmRlcnN0YW5kIGhvdyB0
aGF0ICJkbyB7IC4uIH0gd2hpbGUgKGRldGVjdGVkKSIgbG9vcA0KPiA+ID4gPiA+ID4gd29ya3Mg
b3Igd2h5IGl0J3MgZG9uZSB0aGF0IHdheS4gIENvbGxlY3RpbmcgaW50ZXJydXB0IHN0YXR1cyBi
aXRzIGluDQo+ID4gPiA+ID4gPiBhbiBJU1IgaXMgb2J2aW91c2x5IGEgdmVyeSBjb21tb24gdGFz
azsgaXQgc2VlbXMgbGlrZSB0aGVyZSBzaG91bGQgYmUNCj4gPiA+ID4gPiA+IGEgc3RhbmRhcmQs
IGlkaW9tYXRpYyB3YXkgb2YgZG9pbmcgaXQsIGJ1dCBJIGRvbid0IGtub3cgaXQuDQo+ID4gPiA+
ID4gPg0KPiA+ID4gPiA+ID4gPiBPciwgbWF5IGJlIHdlIGNhbiBtYWtlIHRoZSBjYWxscyB0byBw
Y2llaHBfcXVldWVfaW50ZXJydXB0X2V2ZW50KCkNCj4gPiA+ID4gPiA+ID4gYmVmb3JlIGNsZWFy
aW5nIHRoZQ0KPiA+ID4gPiA+ID4gPiBSVzFDIGluIHRoZSBzbG90IHN0YXR1cyByZWdpc3RlciAo
aW4gdGhlIGxvb3ApPw0KPiA+ID4gPiA+ID4NCj4gPiA+ID4gPiA+IFllYWgsIGl0IHNlZW1zIGxp
a2Ugd2Ugc2hvdWxkIHJlYWQgUENJX0VYUF9TTFRTVEEgb25jZSwgcXVldWUgdXAgYW55DQo+ID4g
PiA+ID4gPiBldmVudHMgcmVsYXRlZCB0byBpdCwgdGhlbiBjbGVhciB0aGUgcmVsZXZhbnQgU0xU
U1RBIGJpdHMuDQo+ID4gPiA+ID4gPg0KPiA+ID4gPiA+DQo+ID4gPiA+ID4gRG8geW91IG1lYW4g
dG8gcmVtb3ZlIHRoZSBkbyB7Li4ufSB3aGlsZSBsb29wIGFuZCBqdXN0DQo+ID4gPiA+ID4gcmVh
ZCBQQ0lfRVhQX1NMVFNUQSBvbmNlIGluIElTUiAsIHF1ZXVlIHRoZSB3b3JrIGFuZCBjbGVhciBp
bnRlcnJ1cHRzPw0KPiA+ID4gPg0KPiA+ID4gPiBJIGRvbid0IGtub3cgaWYgcmVtb3ZpbmcgdGhl
IGxvb3AgaXMgdGhlIHJpZ2h0IHRoaW5nIG9yIG5vdC4gIFdlIG5lZWQNCj4gPiA+ID4gdG8gdW5k
ZXJzdGFuZCB3aHkgdGhlIGxvb3AgaXMgdGhlcmUgaW4gdGhlIGZpcnN0IHBsYWNlIGFuZCBtYWtl
IHN1cmUNCj4gPiA+ID4gcmVtb3ZpbmcgaXQgd291bGRuJ3QgYnJlYWsgdGhpbmdzLg0KPiA+ID4g
Pg0KPiA+ID4NCj4gPiA+DQo+ID4gPiBUaGUgcGNpZSBiYXNlIHNwZWMuIDMuMCBzYXlzIGZyb20g
Y2hhcHRlciA2LjcuMy4xLiBTbG90IEV2ZW50czoNCj4gPiA+DQo+ID4gPiAgICAgICAgIEEgRG93
bnN0cmVhbSBQb3J0IHdpdGggaG90LXBsdWcgY2FwYWJpbGl0aWVzIG1vbml0b3JzIHRoZSBzbG90
IGl0IGNvbnRyb2xzIGZvciB0aGUgc2xvdCBldmVudHMgbGlzdGVkDQo+ID4gPiAgICAgICAgIGFi
b3ZlLiBXaGVuIG9uZSBvZiB0aGVzZSBzbG90IGV2ZW50cyBpcyBkZXRlY3RlZCwgdGhlIFBvcnQg
aW5kaWNhdGVzIHRoYXQgdGhlIGV2ZW50IGhhcyBvY2N1cnJlZCBieQ0KPiA+ID4gICAgICAgICBz
ZXR0aW5nIHRoZSBzdGF0dXMgZmllbGQgYXNzb2NpYXRlZCB3aXRoIHRoZSBldmVudC4gQXQgdGhh
dCBwb2ludCwgdGhlIGV2ZW50IGlzIHBlbmRpbmcgdW50aWwgc29mdHdhcmUNCj4gPiA+ICAgICAg
ICAgY2xlYXJzIHRoZSBzdGF0dXMgZmllbGQuDQo+ID4gPiAgICAgICAgIE9uY2UgYSBzbG90IGV2
ZW50IGlzIHBlbmRpbmcgb24gYSBwYXJ0aWN1bGFyIHNsb3QsIGFsbCBzdWJzZXF1ZW50IGV2ZW50
cyBvZiB0aGF0IHR5cGUgYXJlIGlnbm9yZWQgb24NCj4gPiA+ICAgICAgICAgdGhhdCBzbG90IHVu
dGlsIHRoZSBldmVudCBpcyBjbGVhcmVkLiBUaGUgUG9ydCBtdXN0IGNvbnRpbnVlIHRvIG1vbml0
b3IgdGhlIHNsb3QgZm9yIGFsbCBvdGhlciBzbG90DQo+ID4gPiAgICAgICAgIGV2ZW50IHR5cGVz
IGFuZCByZXBvcnQgdGhlbSBhcyB0aGV5IG9jY3VyLg0KPiA+ID4NCj4gPiA+DQo+ID4gPiBTbyBk
b2VzIGl0IG1lYW4gdGhhdCB0aGUgcG9ydCB3b3VsZCBjb250aW51ZSBwcm92aWRpbmcgTVNJIGlm
IHRoZXJlIGhhcyBiZWVuDQo+ID4gPiBhbnkgb3RoZXIgZXZlbnRzIG9jY3VycmVkIGFwYXJ0IGZy
b20gdGhlIGV2ZW50IHdoaWNoIGlzIG5vdCBjbGVhcmVkPyBJZiB0aGF0DQo+ID4gPiBpcyB0aGUg
Y2FzZSB0aGVuIGl0J3Mgbm90IHN1cmUgd2h5IHRoZSBsb29wIGlzIHN0aWxsIG5lZWRlZC4NCj4g
Pg0KPiA+IEknZCB0aGluayB0aGF0IHRoZSBsb29wIGlzIG5lZWRlZCBiZWNhdXNlIHdlIGRvbid0
IHdhbnQgdG8gaGFuZGxlIGp1c3QNCj4gPiBvbmUgZXZlbnQgb24gb25lIGludGVycnVwdC4gV2Ug
d2FudCB0byBoYW5kbGUgYXMgbWFueSBldmVudHMgYXMgd2UgY2FuDQo+ID4gKHRvIGtlZXAgdGhl
IGludGVycnVwdCBsYXRlbmN5IG92ZXJoZWFkIGxvdyksIHRoYXQgaGF2ZSBoYXBwZW5lZA0KPiA+
IGR1cmluZyB0aGUgSVNSIGludm9jYXRpb24uIEFuZCBoZW5jZSB0aGUgbG9vcC4NCj4gPg0KPiAN
Cj4gSG1tLCBSaWdodC4gSSBjcmVhdGVkIGEgcHJvcG9zYWwgcGF0Y2gNCj4gKD0+W3YxLDIvMl0g
UENJOiBwY2llaHA6IFJld29yayBob3RwbHVnIGludGVycnVwdCByb3V0aW5lKQ0KPiB0byByZW1v
dmUgdGhlIGRvIHsuLi59IHdoaWxlIGxvb3Agd2hpY2gNCj4gSSBndWVzcyB0aGVuIHNob3VsZCBi
ZSByZWplY3RlZCwgcmlnaHQ/IEFzIHdlIGRlY2lkZSB0byBrZWVwIHRoaXMgbG9vcD8NCj4gDQo+
ID4gPg0KPiA+ID4gQWxzbyB3aXRoIGhhdmluZyBhIGxvb3AsIHBjaWVfaXNyKCkgaXMgcmVhZGlu
ZyB0aGUgUENJX0VYUF9TTFRTVEEgYWdhaW4gYW5kIHRha2luZyBhY3Rpb24ganVzdCBiYXNlZCBv
bg0KPiA+ID4gbGF0ZXN0IGV2ZW50IGhhcHBlbnMuIFRoYXQgbWF5IG1lYW4gcmVtb3ZhbC0+aW5z
ZXJ0aW9uIGhhcHBlbnMgZmFzdCBlbm91Z2ggdGhlbg0KPiA+ID4gcmVtb3ZhbCBjb3VsZCBiZSBv
dmVyd3JpdHRlbiBieSBpbnNlcnRpb24gYW5kIHBjaWVocF9nZXRfYWRhcHRlcl9zdGF0dXMgKCkN
Cj4gPiA+IHdpbGwgcmV0dXJuIGluc2VydGlvbiBvbmx5IGFuZCB3ZSBtYXkgbWlzcyB0aGUgcmVt
b3ZhbCBldmVudCB0byBiZSBwcm9jZXNzZWQgZm9yIHRoZSBsb29wLg0KPiA+DQo+ID4gQ3VycmVu
dC4gVGhpcyBpcyB0aGUgcHJvYmxlbSB0aGF0IEJqb3JuIGhpZ2hsaWdodGVkIGFib3ZlLiBJIHRo
aW5rIHRoZQ0KPiA+IGN1cnJlbnQgY29kZSB3b3JrcyB3ZWxsIHdoZW4gZXZlbnRzIG9mIGRpZmZl
cmVudCBraW5kcyBoYXZlIGhhcHBlbmVkDQo+ID4gKGR1ZSB0byB0aGUgbG9vcCkuDQo+ID4NCj4g
PiBJbiBjYXNlIG9mIHNpbWlsYXIga2luZCBvZiBldmVudHMgKGluc2VydGlvbiAvIHJlbW92YWwp
LCBJIHRoaW5rIHRoZXJlDQo+ID4gYXJlIDIgcHJvYmxlbXM6ICgxKSB3ZSB1c2UgdGhlIHRyYW5z
aWVudCB2YWx1ZXMgb2Ygc2xvdCBzdGF0dXMgd2hpY2gNCj4gPiBtYXkgbm90IGJlIHZhbGlkIGF0
IHRoYXQgdGltZS4gKDIpIGJlY2F1c2Ugd2UgYWNrIHRoZSBldmVudCBiZWZvcmUgd2UNCj4gPiBx
dWV1ZSBpdCB1cCwgaXQgbGVhdmVzIGNoYW5jZSBmb3IgZXJyb3IuIEkgdGhpbmsgd2UnZCBuZWVk
IG1vcmUNCj4gPiBjYXJlZnVsIGV4YW1pbmF0aW9uIGluIG9yZGVyIHRvIGZpeCB0aGVzZSBpc3N1
ZXMuDQo+ID4NCj4gDQo+IEkgYWxzbyBzZWUgM3JkIHByb2JsZW0gdG9vIHdpdGggdGhpcyBMb29w
Lg0KPiBGb3IgZXhhbXBsZSwgaWYgd2UgYXJlIGluIHRoZSBsb29wLA0KPiBJZiBvbmx5IHNsb3Qg
cmVtb3ZhbCBjb21lcyAtPiBkZXRlY3QgPSBQQ0lfRVhQX1NMVFNUQV9QREMgLT4gaW50cl9sb2Nr
IHw9IGRldGVjdGVkDQo+IC0+IGRldGVjdGVkPT10cnVlIHNvIGNsZWFyIFBEQyBpbnRlcnJ1cHQg
Yml0ID0+IFNvIHdlIGhhdmUgYSBjaGFuY2UgdG8gZGV0ZWN0DQo+IGFub3RoZXIgaW5zZXJ0aW9u
IGV2ZW50Lg0KPiANCj4gV2UgYXJlIGluIHRoZSBzYW1lIGxvb3AgJiBpZiBpbnNlcnRpb24gY29t
ZXMgYWdhaW4gLT4gc28gZGV0ZWN0ZWQgPSBQQ0lfRVhQX1NMVFNUQV9QREMNCj4gLT4gdGhlbiBk
ZXRlY3RlZCAmPSB+aW50cl9sb2MgLT4gZGV0ZWN0ZWQgPT0gZmFsc2UgPT4gVGhpcyBtZWFucw0K
PiBQQ0lfRVhQX1NMVFNUQSB3b3VsZCBub3QgYmUgY2xlYXJlZCBmb3IgaW5zZXJ0aW9uIGV2ZW50
IGhhcHBlbmVkDQo+IGFuZCB0aGVyZSB3ZSBjb3VsZCBnZXQgYSBwcm9ibGVtIHRoYXQgTm8gZnVy
dGhlciBQREMgZXZlbnRzIGNhbiBnZXQgdHJpZ2dlcmVkIGJ5IEhXLg0KPiANCj4gU28gaWYgd2Ug
d2FudCB0byBrZWVwIHRoZSBsb29wLCBJIHRoaW5rIGNsZWFyaW5nIFBDSV9FWFBfU0xUU1RBICBz
aG91bGQgYmUgc2hpZnRlZA0KPiBvdXRzaWRlIG9mIHRoZSBsb29wIHRvIGNsZWFyIFNsb3QgZXZl
bnRzKGJ1dCBub3QgYmFzZWQgb24gZGV0ZWN0ZWQpIHRvIGVuc3VyZSB0aGF0IGxvb3ANCj4ganVz
dCBsb29rcyBmb3IgdGhlIGV2ZW50cyBXaGljaCBoYXMgbm90IG9jY3VycmVkLCBhbHNvIGJlZm9y
ZSBjbGVhcmluZyBTTFRTVEEgb3V0c2lkZQ0KPiB0aGUgbG9vcCwgd2UgY2FuIHVwZGF0ZSBsaW5r
IGFuZCBzbG90X3N0YXR1cyB0byB0cmlnZ2VyIHRoZSByaWdodCBldmVudHMgZm9yIERMTFNDIGFu
ZCBQREMgdHlwZXMuDQo+IA0KPiBXaGF0IGRvIHlvdSB0aGluayBhYm91dCBmb2xsb3dpbmcgcHJv
cG9zYWw/DQo+IA0KPiBAQCAtNTQyLDcgKzU0Miw3IEBAIHN0YXRpYyBpcnFyZXR1cm5fdCBwY2ll
X2lzcihpbnQgaXJxLCB2b2lkICpkZXZfaWQpDQo+ICAJc3RydWN0IHBjaV9idXMgKnN1Ym9yZGlu
YXRlID0gcGRldi0+c3Vib3JkaW5hdGU7DQo+ICAJc3RydWN0IHBjaV9kZXYgKmRldjsNCj4gIAlz
dHJ1Y3Qgc2xvdCAqc2xvdCA9IGN0cmwtPnNsb3Q7DQo+IC0JdTE2IGRldGVjdGVkLCBpbnRyX2xv
YzsNCj4gKwl1MTYgZGV0ZWN0ZWQsIGludHJfbG9jLCBzbG90X3N0YXR1czsNCj4gIAl1OCBwcmVz
ZW50Ow0KPiAgCWJvb2wgbGluazsNCj4gDQo+IEBAIC01NTMsMjYgKzU1MywzMCBAQCBzdGF0aWMg
aXJxcmV0dXJuX3QgcGNpZV9pc3IoaW50IGlycSwgdm9pZCAqZGV2X2lkKQ0KPiAgCSAqLw0KPiAg
CWludHJfbG9jID0gMDsNCj4gIAlkbyB7DQo+IC0JCXBjaWVfY2FwYWJpbGl0eV9yZWFkX3dvcmQo
cGRldiwgUENJX0VYUF9TTFRTVEEsICZkZXRlY3RlZCk7DQo+IC0JCWlmIChkZXRlY3RlZCA9PSAo
dTE2KSB+MCkgew0KPiArCQlwY2llX2NhcGFiaWxpdHlfcmVhZF93b3JkKHBkZXYsIFBDSV9FWFBf
U0xUU1RBLCAmc2xvdF9zdGF0dXMpOw0KPiArCQlpZiAoc2xvdF9zdGF0dXMgPT0gKHUxNikgfjAp
IHsNCj4gIAkJCWN0cmxfaW5mbyhjdHJsLCAiJXM6IG5vIHJlc3BvbnNlIGZyb20gZGV2aWNlXG4i
LA0KPiAgCQkJCSAgX19mdW5jX18pOw0KPiAgCQkJcmV0dXJuIElSUV9IQU5ETEVEOw0KPiAgCQl9
DQo+IA0KPiAtCQlkZXRlY3RlZCAmPSAoUENJX0VYUF9TTFRTVEFfQUJQIHwgUENJX0VYUF9TTFRT
VEFfUEZEIHwNCj4gKwkJZGV0ZWN0ZWQgPSBzbG90X3N0YXR1cyAmIChQQ0lfRVhQX1NMVFNUQV9B
QlAgfA0KPiArCQkJICAgICBQQ0lfRVhQX1NMVFNUQV9QRkQgfA0KPiAgCQkJICAgICBQQ0lfRVhQ
X1NMVFNUQV9QREMgfA0KPiAgCQkJICAgICBQQ0lfRVhQX1NMVFNUQV9DQyB8IFBDSV9FWFBfU0xU
U1RBX0RMTFNDKTsNCj4gIAkJZGV0ZWN0ZWQgJj0gfmludHJfbG9jOw0KPiAgCQlpbnRyX2xvYyB8
PSBkZXRlY3RlZDsNCj4gIAkJaWYgKCFpbnRyX2xvYykNCj4gIAkJCXJldHVybiBJUlFfTk9ORTsN
Cj4gLQkJaWYgKGRldGVjdGVkKQ0KPiAtCQkJcGNpZV9jYXBhYmlsaXR5X3dyaXRlX3dvcmQocGRl
diwgUENJX0VYUF9TTFRTVEEsDQo+IC0JCQkJCQkgICBpbnRyX2xvYyk7DQo+ICAJfSB3aGlsZSAo
ZGV0ZWN0ZWQpOw0KPiANCj4gIAljdHJsX2RiZyhjdHJsLCAicGVuZGluZyBpbnRlcnJ1cHRzICUj
MDZ4IGZyb20gU2xvdCBTdGF0dXNcbiIsIGludHJfbG9jKTsNCj4gKwlpZiAoaW50cl9sb2MgJiBQ
Q0lfRVhQX1NMVFNUQV9QREMpDQo+ICsJCXByZXNlbnQgPSAhIShzbG90X3N0YXR1cyAmIFBDSV9F
WFBfU0xUU1RBX1BEUyk7DQo+ICsJaWYgKGludHJfbG9jICYgUENJX0VYUF9TTFRTVEFfRExMU0Mp
DQo+ICsJCWxpbmsgPSBwY2llaHBfY2hlY2tfbGlua19hY3RpdmUoY3RybCk7DQo+ICsNCj4gKwlw
Y2llX2NhcGFiaWxpdHlfd3JpdGVfd29yZChwZGV2LCBQQ0lfRVhQX1NMVFNUQSwgaW50cl9sb2Mp
Ow0KPiANCj4gIAkvKiBDaGVjayBDb21tYW5kIENvbXBsZXRlIEludGVycnVwdCBQZW5kaW5nICov
DQo+ICAJaWYgKGludHJfbG9jICYgUENJX0VYUF9TTFRTVEFfQ0MpIHsNCj4gQEAgLTYwMyw3ICs2
MDcsNiBAQCBzdGF0aWMgaXJxcmV0dXJuX3QgcGNpZV9pc3IoaW50IGlycSwgdm9pZCAqZGV2X2lk
KQ0KPiANCj4gIAkvKiBDaGVjayBQcmVzZW5jZSBEZXRlY3QgQ2hhbmdlZCAqLw0KPiAgCWlmIChp
bnRyX2xvYyAmIFBDSV9FWFBfU0xUU1RBX1BEQykgew0KPiAtCQlwY2llaHBfZ2V0X2FkYXB0ZXJf
c3RhdHVzKHNsb3QsICZwcmVzZW50KTsNCj4gIAkJY3RybF9pbmZvKGN0cmwsICJDYXJkICVzcHJl
c2VudCBvbiBTbG90KCVzKVxuIiwNCj4gIAkJCSAgcHJlc2VudCA/ICIiIDogIm5vdCAiLCBzbG90
X25hbWUoc2xvdCkpOw0KPiAgCQlwY2llaHBfcXVldWVfaW50ZXJydXB0X2V2ZW50KHNsb3QsIHBy
ZXNlbnQgPyBJTlRfUFJFU0VOQ0VfT04gOg0KPiBAQCAtNjE4LDcgKzYyMSw2IEBAIHN0YXRpYyBp
cnFyZXR1cm5fdCBwY2llX2lzcihpbnQgaXJxLCB2b2lkICpkZXZfaWQpDQo+ICAJfQ0KPiANCj4g
IAlpZiAoaW50cl9sb2MgJiBQQ0lfRVhQX1NMVFNUQV9ETExTQykgew0KPiAtCQlsaW5rID0gcGNp
ZWhwX2NoZWNrX2xpbmtfYWN0aXZlKGN0cmwpOw0KPiAgCQljdHJsX2luZm8oY3RybCwgInNsb3Qo
JXMpOiBMaW5rICVzIGV2ZW50XG4iLA0KPiAgCQkJICBzbG90X25hbWUoc2xvdCksIGxpbmsgPyAi
VXAiIDogIkRvd24iKTsNCj4gIAkJcGNpZWhwX3F1ZXVlX2ludGVycnVwdF9ldmVudChzbG90LCBs
aW5rID8gSU5UX0xJTktfVVAgOg0KPiANCj4gDQoNCkhpIFJhamF0L0Jqb3JuDQpDb3VsZCB5b3Ug
cGxlYXNlIGNvbW1lbnQsIGlmIHRoZSBwcm9wb3NhbCBjYW4gYmUgd29ya2VkIG91dCBvciB3ZSBz
aG91bGQgY29uc2lkZXINCnJlbW92aW5nIGRvIHsuLn0gd2hpbGUgYXQgYWxsIGFzIEtlaXRoLCBC
dXNjaCBjb21tZW50ZWQgYXMgdGhhdCdzIG5vdCByZWFsbHkgcGVyZm9ybWFuY2UNCnBhdGggb3Ig
aXMgdGhlcmUgYW55IG90aGVyIHdheS9zdWdnZXN0aW9uIHlvdSB0aGluayBvZj8NCg0KDQo+ID4g
Pg0KPiA+ID4gSWYgd2UgZG9uJ3QgY2xlYXIgdGhlIFNsb3QgZXZlbnQgcXVpY2sgZW5vdWdoIHRo
ZW4gYWNjb3JkaW5nIHRvIHNwZWMuIHN0YXRlbWVudCBhYm92ZQ0KPiA+ID4gdGhhdCBldmVudCBj
b3VsZCBnZXQgaWdub3JlZCBhbmQgU1cgbWF5IG5ldmVyIGdldCBub3RpZmllZC4NCj4gPiA+DQo+
ID4gPg0KPiA+ID4NCj4gPiA+ID4gQnV0IEkgZG8gdGhpbmsgdGhhdCBpbiB0aGUgcmVzdWx0aW5n
IGNvZGUsIHRoZSBjb25uZWN0aW9uIGJldHdlZW4NCj4gPiA+ID4NCj4gPiA+ID4gICAoMSkgdGhl
IGV2ZW50cyB3ZSBsZWFybiBmcm9tIHRoZSBpbnRlcnJ1cHQgYW5kDQo+ID4gPiA+ICAgKDIpIHRo
ZSBxdWV1ZWQgd29yayBpdGVtcw0KPiA+ID4gPg0KPiA+ID4gPiBuZWVkcyB0byBiZSBjcnlzdGFs
IGNsZWFyLiAgUmlnaHQgbm93IGl0J3MgYSBiaXQgbXVkZHkgYmVjYXVzZSBvZg0KPiA+ID4gPiB0
aGluZ3MgbGlrZSB0aGUgY2FzZSB5b3UgZml4ZWQ6IGEgd29yayBpdGVtIHRoYXQgZ29lcyBiYWNr
IGFuZCBsb29rcw0KPiA+ID4gPiBhdCBQQ0lfRVhQX1NMVFNUQSBhZnRlciBpdCdzIGJlZW4gY2xl
YXJlZCBhbmQgdGhlIGhhcmR3YXJlIG1heSBoYXZlDQo+ID4gPiA+IGFscmVhZHkgc2V0IGJpdHMg
Zm9yIG5ldyBldmVudHMuDQo+ID4gPiA+DQo+ID4gPg0KPiA+ID4gSSBoYXZlIG1hZGUgYSBwcm90
b3R5cGUgcGF0Y2ggaW4gbXkgZm9sbG93IHVwIHJlcGx5IGFuZCB0ZXN0ZWQgb2sgb24gbXkgZXhp
c3Rpbmcgc2V0dXANCj4gPiA+IG9uIHdoaWNoIEkgY2F1Z2h0IHRoZSBwcmV2aW91cyBpc3N1ZSBh
bHRob3VnaCBJIGNhbnQgc2F5IGl0IHdpbGwgd29yayBvbiBhbnkgSFcuDQo+ID4gPiBDbGVhcmlu
ZyBldmVudHMgYWZ0ZXIgcXVldWluZyBldmVudHMgZ2F2ZSBzb21lIHByb2JsZW1zIGFuZCBkaWQg
bm90IHdvcmsgcHJvcGVybHkgb24gbXkNCj4gPiA+IGV4aXN0aW5nIEhXIHdoZXJlIEkgdGVzdCB2
ZXJ5IGZhc3QgaW5zZXJ0aW9uIGFuZCByZW1vdmFsIGV2ZW50cywgaW4gd2hpY2gNCj4gPiA+IGNh
c2Ugb25seSAicmVtb3ZhbCIgZXZlbnQgY29tZXMgYW5kICJpbnNlcnRpb24iIGRvZXMgbm90IG9j
Y3VyIGV2ZW4gaWYgSFcgZ2V0cyBwb3dlcmVkLg0KPiA+ID4NCj4gDQpJbnRlbCBEZXV0c2NobGFu
ZCBHbWJIClJlZ2lzdGVyZWQgQWRkcmVzczogQW0gQ2FtcGVvbiAxMC0xMiwgODU1NzkgTmV1Ymli
ZXJnLCBHZXJtYW55ClRlbDogKzQ5IDg5IDk5IDg4NTMtMCwgd3d3LmludGVsLmRlCk1hbmFnaW5n
IERpcmVjdG9yczogQ2hyaXN0aW4gRWlzZW5zY2htaWQsIENocmlzdGlhbiBMYW1wcmVjaHRlcgpD
aGFpcnBlcnNvbiBvZiB0aGUgU3VwZXJ2aXNvcnkgQm9hcmQ6IE5pY29sZSBMYXUKUmVnaXN0ZXJl
ZCBPZmZpY2U6IE11bmljaApDb21tZXJjaWFsIFJlZ2lzdGVyOiBBbXRzZ2VyaWNodCBNdWVuY2hl
biBIUkIgMTg2OTI4Cg==

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