[PATCH] pciehp: Fix race condition handling surprise link-down

[PATCH] pciehp: Fix race condition handling surprise link-down

[Ashok Raj]

Changes from v1:
	Address comments from Bjorn:
		Added p_slot->lock mutex around changes to p_slot->state
		Updated commit message to call out mutex names

A surprise link down may retrain very quickly, causing the same slot to
generate a link up event before handling the link down completes.

Since the link is active, the power off work queued from the first link
down will cause a second down event when the power is disabled. The second
down event should be ignored because the slot is already powering off;
however, the "link up" event sets the slot state to POWERON before the
event to handle this is enqueued, making the second down event believe
it needs to do something. This creates a constant link up and down
event cycle.

This patch fixes that by setting the p_slot->state only when the work to
handle the power event is executing, protected by the p_slot->hotplug_lock.

To: Bjorn Helgass <bhelgaas@google.com>
Cc: linux-kernel@vger.kernel.org
Cc: Keith Busch <keith.busch@intel.com>

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Reviewed-by: Keith Busch <keith.busch@intel.com>
---
 drivers/pci/hotplug/pciehp_ctrl.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index ec0b4c1..4cf4772 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -182,6 +182,9 @@ static void pciehp_power_thread(struct work_struct *work)
 	switch (info->req) {
 	case DISABLE_REQ:
 		mutex_lock(&p_slot->hotplug_lock);
+		mutex_lock(&p_slot->lock);
+		p_slot->state = POWEROFF_STATE;
+		mutex_unlock(&p_slot->lock);
 		pciehp_disable_slot(p_slot);
 		mutex_unlock(&p_slot->hotplug_lock);
 		mutex_lock(&p_slot->lock);
@@ -190,6 +193,9 @@ static void pciehp_power_thread(struct work_struct *work)
 		break;
 	case ENABLE_REQ:
 		mutex_lock(&p_slot->hotplug_lock);
+		mutex_lock(&p_slot->lock);
+		p_slot->state = POWERON_STATE;
+		mutex_unlock(&p_slot->lock);
 		ret = pciehp_enable_slot(p_slot);
 		mutex_unlock(&p_slot->hotplug_lock);
 		if (ret)
@@ -209,8 +215,6 @@ static void pciehp_queue_power_work(struct slot *p_slot, int req)
 {
 	struct power_work_info *info;
 
-	p_slot->state = (req == ENABLE_REQ) ? POWERON_STATE : POWEROFF_STATE;
-
 	info = kmalloc(sizeof(*info), GFP_KERNEL);
 	if (!info) {
 		ctrl_err(p_slot->ctrl, "no memory to queue %s request\n",
-- 
2.7.4

Re: [PATCH] pciehp: Fix race condition handling surprise link-down

[Raj, Ashok]

Hi Keith

did you have a chance to look at this?

Cheers,
Ashok

On Fri, Dec 09, 2016 at 01:06:04PM -0800, Ashok Raj wrote:
> Changes from v1:
> 	Address comments from Bjorn:
> 		Added p_slot->lock mutex around changes to p_slot->state
> 		Updated commit message to call out mutex names
> 
> A surprise link down may retrain very quickly, causing the same slot to
> generate a link up event before handling the link down completes.
> 
> Since the link is active, the power off work queued from the first link
> down will cause a second down event when the power is disabled. The second
> down event should be ignored because the slot is already powering off;
> however, the "link up" event sets the slot state to POWERON before the
> event to handle this is enqueued, making the second down event believe
> it needs to do something. This creates a constant link up and down
> event cycle.
> 
> This patch fixes that by setting the p_slot->state only when the work to
> handle the power event is executing, protected by the p_slot->hotplug_lock.
> 
> To: Bjorn Helgass <bhelgaas@google.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: Keith Busch <keith.busch@intel.com>
> 
> Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> Reviewed-by: Keith Busch <keith.busch@intel.com>
> ---
>  drivers/pci/hotplug/pciehp_ctrl.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index ec0b4c1..4cf4772 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -182,6 +182,9 @@ static void pciehp_power_thread(struct work_struct *work)
>  	switch (info->req) {
>  	case DISABLE_REQ:
>  		mutex_lock(&p_slot->hotplug_lock);
> +		mutex_lock(&p_slot->lock);
> +		p_slot->state = POWEROFF_STATE;
> +		mutex_unlock(&p_slot->lock);
>  		pciehp_disable_slot(p_slot);
>  		mutex_unlock(&p_slot->hotplug_lock);
>  		mutex_lock(&p_slot->lock);
> @@ -190,6 +193,9 @@ static void pciehp_power_thread(struct work_struct *work)
>  		break;
>  	case ENABLE_REQ:
>  		mutex_lock(&p_slot->hotplug_lock);
> +		mutex_lock(&p_slot->lock);
> +		p_slot->state = POWERON_STATE;
> +		mutex_unlock(&p_slot->lock);
>  		ret = pciehp_enable_slot(p_slot);
>  		mutex_unlock(&p_slot->hotplug_lock);
>  		if (ret)
> @@ -209,8 +215,6 @@ static void pciehp_queue_power_work(struct slot *p_slot, int req)
>  {
>  	struct power_work_info *info;
>  
> -	p_slot->state = (req == ENABLE_REQ) ? POWERON_STATE : POWEROFF_STATE;
> -
>  	info = kmalloc(sizeof(*info), GFP_KERNEL);
>  	if (!info) {
>  		ctrl_err(p_slot->ctrl, "no memory to queue %s request\n",
> -- 
> 2.7.4
> 

Re: [PATCH] pciehp: Fix race condition handling surprise link-down

[Raj, Ashok]

Hi Bjorn

Sorry to bug you, didn't hear from you after i added the lock for consistency
to address the feedback.

Let me know if there is anymore changes you like to see.

Cheers,
Ashok

On Fri, Dec 09, 2016 at 01:06:04PM -0800, Ashok Raj wrote:
> Changes from v1:
> 	Address comments from Bjorn:
> 		Added p_slot->lock mutex around changes to p_slot->state
> 		Updated commit message to call out mutex names
> 
> A surprise link down may retrain very quickly, causing the same slot to
> generate a link up event before handling the link down completes.
> 
> Since the link is active, the power off work queued from the first link
> down will cause a second down event when the power is disabled. The second
> down event should be ignored because the slot is already powering off;
> however, the "link up" event sets the slot state to POWERON before the
> event to handle this is enqueued, making the second down event believe
> it needs to do something. This creates a constant link up and down
> event cycle.
> 
> This patch fixes that by setting the p_slot->state only when the work to
> handle the power event is executing, protected by the p_slot->hotplug_lock.
> 
> To: Bjorn Helgass <bhelgaas@google.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: Keith Busch <keith.busch@intel.com>
> 
> Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> Reviewed-by: Keith Busch <keith.busch@intel.com>
> ---
>  drivers/pci/hotplug/pciehp_ctrl.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index ec0b4c1..4cf4772 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -182,6 +182,9 @@ static void pciehp_power_thread(struct work_struct *work)
>  	switch (info->req) {
>  	case DISABLE_REQ:
>  		mutex_lock(&p_slot->hotplug_lock);
> +		mutex_lock(&p_slot->lock);
> +		p_slot->state = POWEROFF_STATE;
> +		mutex_unlock(&p_slot->lock);
>  		pciehp_disable_slot(p_slot);
>  		mutex_unlock(&p_slot->hotplug_lock);
>  		mutex_lock(&p_slot->lock);
> @@ -190,6 +193,9 @@ static void pciehp_power_thread(struct work_struct *work)
>  		break;
>  	case ENABLE_REQ:
>  		mutex_lock(&p_slot->hotplug_lock);
> +		mutex_lock(&p_slot->lock);
> +		p_slot->state = POWERON_STATE;
> +		mutex_unlock(&p_slot->lock);
>  		ret = pciehp_enable_slot(p_slot);
>  		mutex_unlock(&p_slot->hotplug_lock);
>  		if (ret)
> @@ -209,8 +215,6 @@ static void pciehp_queue_power_work(struct slot *p_slot, int req)
>  {
>  	struct power_work_info *info;
>  
> -	p_slot->state = (req == ENABLE_REQ) ? POWERON_STATE : POWEROFF_STATE;
> -
>  	info = kmalloc(sizeof(*info), GFP_KERNEL);
>  	if (!info) {
>  		ctrl_err(p_slot->ctrl, "no memory to queue %s request\n",
> -- 
> 2.7.4
> 

Re: [PATCH] pciehp: Fix race condition handling surprise link-down

[Keith Busch]

Hi Bjorn,

This fix looks good to me as well now. Any other concerns before staging
this one for inclusion?

Thanks,
Keith

On Tue, Jan 17, 2017 at 11:15:40AM -0800, Raj, Ashok wrote:
> Hi Bjorn
> 
> Sorry to bug you, didn't hear from you after i added the lock for consistency
> to address the feedback.
> 
> Let me know if there is anymore changes you like to see.
> 
> Cheers,
> Ashok
> 
> On Fri, Dec 09, 2016 at 01:06:04PM -0800, Ashok Raj wrote:
> > Changes from v1:
> > 	Address comments from Bjorn:
> > 		Added p_slot->lock mutex around changes to p_slot->state
> > 		Updated commit message to call out mutex names
> > 
> > A surprise link down may retrain very quickly, causing the same slot to
> > generate a link up event before handling the link down completes.
> > 
> > Since the link is active, the power off work queued from the first link
> > down will cause a second down event when the power is disabled. The second
> > down event should be ignored because the slot is already powering off;
> > however, the "link up" event sets the slot state to POWERON before the
> > event to handle this is enqueued, making the second down event believe
> > it needs to do something. This creates a constant link up and down
> > event cycle.
> > 
> > This patch fixes that by setting the p_slot->state only when the work to
> > handle the power event is executing, protected by the p_slot->hotplug_lock.
> > 
> > To: Bjorn Helgass <bhelgaas@google.com>
> > Cc: linux-kernel@vger.kernel.org
> > Cc: Keith Busch <keith.busch@intel.com>
> > 
> > Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> > Reviewed-by: Keith Busch <keith.busch@intel.com>
> > ---
> >  drivers/pci/hotplug/pciehp_ctrl.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> > index ec0b4c1..4cf4772 100644
> > --- a/drivers/pci/hotplug/pciehp_ctrl.c
> > +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> > @@ -182,6 +182,9 @@ static void pciehp_power_thread(struct work_struct *work)
> >  	switch (info->req) {
> >  	case DISABLE_REQ:
> >  		mutex_lock(&p_slot->hotplug_lock);
> > +		mutex_lock(&p_slot->lock);
> > +		p_slot->state = POWEROFF_STATE;
> > +		mutex_unlock(&p_slot->lock);
> >  		pciehp_disable_slot(p_slot);
> >  		mutex_unlock(&p_slot->hotplug_lock);
> >  		mutex_lock(&p_slot->lock);
> > @@ -190,6 +193,9 @@ static void pciehp_power_thread(struct work_struct *work)
> >  		break;
> >  	case ENABLE_REQ:
> >  		mutex_lock(&p_slot->hotplug_lock);
> > +		mutex_lock(&p_slot->lock);
> > +		p_slot->state = POWERON_STATE;
> > +		mutex_unlock(&p_slot->lock);
> >  		ret = pciehp_enable_slot(p_slot);
> >  		mutex_unlock(&p_slot->hotplug_lock);
> >  		if (ret)
> > @@ -209,8 +215,6 @@ static void pciehp_queue_power_work(struct slot *p_slot, int req)
> >  {
> >  	struct power_work_info *info;
> >  
> > -	p_slot->state = (req == ENABLE_REQ) ? POWERON_STATE : POWEROFF_STATE;
> > -
> >  	info = kmalloc(sizeof(*info), GFP_KERNEL);
> >  	if (!info) {
> >  		ctrl_err(p_slot->ctrl, "no memory to queue %s request\n",
> > -- 
> > 2.7.4

Re: [PATCH] pciehp: Fix race condition handling surprise link-down

[Bjorn Helgaas]

Sorry for the delay, I'm just trying to clear out some of our defects from
bugzilla and have gotten really behind on the list, so I just haven't
gotten to this yet.

On Wed, Jan 18, 2017 at 01:47:11PM -0500, Keith Busch wrote:
> Hi Bjorn,
> 
> This fix looks good to me as well now. Any other concerns before staging
> this one for inclusion?
> 
> Thanks,
> Keith
> 
> On Tue, Jan 17, 2017 at 11:15:40AM -0800, Raj, Ashok wrote:
> > Hi Bjorn
> > 
> > Sorry to bug you, didn't hear from you after i added the lock for consistency
> > to address the feedback.
> > 
> > Let me know if there is anymore changes you like to see.
> > 
> > Cheers,
> > Ashok
> > 
> > On Fri, Dec 09, 2016 at 01:06:04PM -0800, Ashok Raj wrote:
> > > Changes from v1:
> > > 	Address comments from Bjorn:
> > > 		Added p_slot->lock mutex around changes to p_slot->state
> > > 		Updated commit message to call out mutex names
> > > 
> > > A surprise link down may retrain very quickly, causing the same slot to
> > > generate a link up event before handling the link down completes.
> > > 
> > > Since the link is active, the power off work queued from the first link
> > > down will cause a second down event when the power is disabled. The second
> > > down event should be ignored because the slot is already powering off;
> > > however, the "link up" event sets the slot state to POWERON before the
> > > event to handle this is enqueued, making the second down event believe
> > > it needs to do something. This creates a constant link up and down
> > > event cycle.
> > > 
> > > This patch fixes that by setting the p_slot->state only when the work to
> > > handle the power event is executing, protected by the p_slot->hotplug_lock.
> > > 
> > > To: Bjorn Helgass <bhelgaas@google.com>
> > > Cc: linux-kernel@vger.kernel.org
> > > Cc: Keith Busch <keith.busch@intel.com>
> > > 
> > > Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> > > Reviewed-by: Keith Busch <keith.busch@intel.com>
> > > ---
> > >  drivers/pci/hotplug/pciehp_ctrl.c | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> > > index ec0b4c1..4cf4772 100644
> > > --- a/drivers/pci/hotplug/pciehp_ctrl.c
> > > +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> > > @@ -182,6 +182,9 @@ static void pciehp_power_thread(struct work_struct *work)
> > >  	switch (info->req) {
> > >  	case DISABLE_REQ:
> > >  		mutex_lock(&p_slot->hotplug_lock);
> > > +		mutex_lock(&p_slot->lock);
> > > +		p_slot->state = POWEROFF_STATE;
> > > +		mutex_unlock(&p_slot->lock);
> > >  		pciehp_disable_slot(p_slot);
> > >  		mutex_unlock(&p_slot->hotplug_lock);
> > >  		mutex_lock(&p_slot->lock);
> > > @@ -190,6 +193,9 @@ static void pciehp_power_thread(struct work_struct *work)
> > >  		break;
> > >  	case ENABLE_REQ:
> > >  		mutex_lock(&p_slot->hotplug_lock);
> > > +		mutex_lock(&p_slot->lock);
> > > +		p_slot->state = POWERON_STATE;
> > > +		mutex_unlock(&p_slot->lock);
> > >  		ret = pciehp_enable_slot(p_slot);
> > >  		mutex_unlock(&p_slot->hotplug_lock);
> > >  		if (ret)
> > > @@ -209,8 +215,6 @@ static void pciehp_queue_power_work(struct slot *p_slot, int req)
> > >  {
> > >  	struct power_work_info *info;
> > >  
> > > -	p_slot->state = (req == ENABLE_REQ) ? POWERON_STATE : POWEROFF_STATE;
> > > -
> > >  	info = kmalloc(sizeof(*info), GFP_KERNEL);
> > >  	if (!info) {
> > >  		ctrl_err(p_slot->ctrl, "no memory to queue %s request\n",
> > > -- 
> > > 2.7.4
> --
> 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] pciehp: Fix race condition handling surprise link-down

[Bjorn Helgaas]

Hi Ashok,

Sorry it took me so long to review this.  I never felt like I really
understood it, and it took me a long time to try to figure out a more
useful response.

On Fri, Dec 09, 2016 at 01:06:04PM -0800, Ashok Raj wrote:
> Changes from v1:
> 	Address comments from Bjorn:
> 		Added p_slot->lock mutex around changes to p_slot->state
> 		Updated commit message to call out mutex names
> 
> A surprise link down may retrain very quickly, causing the same slot to
> generate a link up event before handling the link down completes.
> 
> Since the link is active, the power off work queued from the first link
> down will cause a second down event when the power is disabled. The second
> down event should be ignored because the slot is already powering off;
> however, the "link up" event sets the slot state to POWERON before the
> event to handle this is enqueued, making the second down event believe
> it needs to do something. This creates a constant link up and down
> event cycle.
> 
> This patch fixes that by setting the p_slot->state only when the work to
> handle the power event is executing, protected by the p_slot->hotplug_lock.

What I don't like about this patch is that I can't figure out what
we're fixing from the patch.  That's not your fault; it's just a
symptom of the convoluted logic in pciehp.  But if we can fix the
problem by simplifying that logic, I'd rather do that than patch up
the holes.

So let me first try to understand what's going on with the current
code.  In the normal case where a device is removed or turned off and
pciehp can complete everything before another device appears, I think
the flow is like this:

      p_slot->state == STATIC_STATE (powered on, link up)

                        <-- surprise link down interrupt
      pciehp_isr()
        queue INT_LINK_DOWN work

      interrupt_event_handler(INT_LINK_DOWN)
        set p_slot->state = POWEROFF_STATE
        queue DISABLE_REQ work

      pciehp_power_thread(DISABLE_REQ)
        send PCI_EXP_SLTCTL_PWR_OFF command
        wait for power-off to complete
        set p_slot->state = STATIC_STATE

      p_slot->state == STATIC_STATE (powered off)

In the problem case, the link goes down, and while pciehp is still
dealing with that, the link comes back up.  So I think one possible
sequence is like this:

      p_slot->state == STATIC_STATE (powered on, link up)

                        <-- surprise link down interrupt
  1a  pciehp_isr()
        queue INT_LINK_DOWN work                     # queued: 1-LD

  1b  interrupt_event_handler(INT_LINK_DOWN)         # process 1-LD
        # handle_link_event() sees case STATIC_STATE
        set p_slot->state = POWEROFF_STATE
        queue DISABLE_REQ work                       # queued: 1-DR

                        <-- surprise link up interrupt
  2a  pciehp_isr()
        queue INT_LINK_UP work                       # queued: 1-DR 2-LU

  1c  pciehp_power_thread(DISABLE_REQ)               # process 1-DR
        send PCI_EXP_SLTCTL_PWR_OFF command
        wait for power-off to complete
        set p_slot->state = STATIC_STATE

                        <-- link down interrupt (result of PWR_OFF)
  3a  pciehp_isr()
        queue INT_LINK_DOWN work                     # queued: 2-LU 3-LD

  2b  interrupt_event_handler(INT_LINK_UP)           # process 2-LU
        # handle_link_event() sees case STATIC_STATE
        set p_slot->state = POWERON_STATE
        queue ENABLE_REQ work                        # queued: 3-LD 2-ER

  3b  interrupt_event_handler(INT_LINK_DOWN)         # process 3-LD
        # handle_link_event() sees case POWERON_STATE, so we emit
        # "Link Down event queued; currently getting powered on"
        set p_slot->state = POWEROFF_STATE
        queue DISABLE_REQ work                       # queued: 2-ER 3-DR

  2c  pciehp_power_thread(ENABLE_REQ)                # process 2-ER
        send PCI_EXP_SLTCTL_PWR_ON command
        wait for power-on to complete
        set p_slot->state = STATIC_STATE

                        <-- link up interrupt (result of PWR_ON)
  4a  pciehp_isr()
        queue INT_LINK_UP work                       # queued: 3-DR 4-LU

  3c  pciehp_power_thread(DISABLE_REQ)               # process 3-DR
        send PCI_EXP_SLTCTL_PWR_OFF command
        wait for power-off to complete
        set p_slot->state = STATIC_STATE

                        <-- link down interrupt (result of PWR_OFF)
  5a  pciehp_isr()
        queue INT_LINK_DOWN work                     # queued: 4-LU 5-LD

State 5a is the same as 3a (we're in STATIC_STATE with Link Up and
Link Down work items queued), so the whole cycle can repeat.

Now let's assume we apply this patch and see what changes.  The patch
changes where we set p_slot->state.  Currently we set POWEROFF_STATE
or POWERON_STATE in the interrupt_event_handler() work item.  The
patch moves that to the pciehp_power_thread() work item, where the
power commands are actually sent.

      p_slot->state == STATIC_STATE (powered on, link up)

                        <-- surprise link down interrupt
  1A  pciehp_isr()
        queue INT_LINK_DOWN work                     # queued: 1-LD

  1B  interrupt_event_handler(INT_LINK_DOWN)         # process 1-LD
        # handle_link_event() sees case STATIC_STATE
        # set p_slot->state = POWEROFF_STATE         # (removed by patch)
        queue DISABLE_REQ work                       # queued: 1-DR

                        <-- surprise link up interrupt
  2A  pciehp_isr()
        queue INT_LINK_UP work                       # queued: 1-DR 2-LU

  1C  pciehp_power_thread(DISABLE_REQ)               # process 1-DR
        set p_slot->state = POWEROFF_STATE           # (added by patch)
        send PCI_EXP_SLTCTL_PWR_OFF command
        wait for power-off to complete
        set p_slot->state = STATIC_STATE

                        <-- link down interrupt (result of PWR_OFF)
  3A  pciehp_isr()
        queue INT_LINK_DOWN work                     # queued: 2-LU 3-LD

  2B  interrupt_event_handler(INT_LINK_UP)           # process 2-LU
        # handle_link_event() sees case STATIC_STATE
        # set p_slot->state = POWERON_STATE          # (removed by patch)
        queue ENABLE_REQ work                        # queued: 3-LD 2-ER

  3B  interrupt_event_handler(INT_LINK_DOWN)         # process 3-LD
        # handle_link_event() sees case STATIC_STATE,
        # unlike 3b above, which saw POWERON_STATE;
        # doesn't emit a message, but still queues DISABLE_REQ
        # set p_slot->state = POWEROFF_STATE         # (removed by patch)
        queue DISABLE_REQ work                       # queued: 2-ER 3-DR

  2C  pciehp_power_thread(ENABLE_REQ)                # process 2-ER
        set p_slot->state = POWERON_STATE            # (added by patch)
        send PCI_EXP_SLTCTL_PWR_ON command
        wait for power-on to complete
        set p_slot->state = STATIC_STATE

                        <-- link up interrupt (result of PWR_ON)
  4A  pciehp_isr()
        queue INT_LINK_UP work                       # queued: 3-DR 4-LU

  3C  pciehp_power_thread(DISABLE_REQ)               # process 3-DR
        set p_slot->state = POWEROFF_STATE           # (added by patch)
        send PCI_EXP_SLTCTL_PWR_OFF command
        wait for power-off to complete
        set p_slot->state = STATIC_STATE

                        <-- link down interrupt (result of PWR_OFF)
  5A  pciehp_isr()
        queue INT_LINK_DOWN work                     # queued: 4-LU 5-LD

With this particular ordering, I think we still have the same problem:
5A is the same as 3A, so I think the cycle could repeat.

Obviously many other orderings are possible.  Since the patch fixes
your system, I suspect you're seeing an ordering where at 3B,
handle_link_event() sees POWEROFF_STATE.  In that case, you probably
see the "Link Down event ignored" message, but we don't queue the
DISABLE_REQ work, and there is no cycle.

This all makes me a little bleary-eyed, so maybe I'm missing
something, but it looks to me like we could still hit the same problem
even with this patch.

I really think the only way to make this all intelligible and reliable
is to rework this to use ordered workqueues so we only have one thing
at a time going on.  That wouldn't be a panacea, but I think it would
make things a lot simpler.

Bjorn

> To: Bjorn Helgass <bhelgaas@google.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: Keith Busch <keith.busch@intel.com>
> 
> Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> Reviewed-by: Keith Busch <keith.busch@intel.com>
> ---
>  drivers/pci/hotplug/pciehp_ctrl.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index ec0b4c1..4cf4772 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -182,6 +182,9 @@ static void pciehp_power_thread(struct work_struct *work)
>  	switch (info->req) {
>  	case DISABLE_REQ:
>  		mutex_lock(&p_slot->hotplug_lock);
> +		mutex_lock(&p_slot->lock);
> +		p_slot->state = POWEROFF_STATE;
> +		mutex_unlock(&p_slot->lock);
>  		pciehp_disable_slot(p_slot);
>  		mutex_unlock(&p_slot->hotplug_lock);
>  		mutex_lock(&p_slot->lock);
> @@ -190,6 +193,9 @@ static void pciehp_power_thread(struct work_struct *work)
>  		break;
>  	case ENABLE_REQ:
>  		mutex_lock(&p_slot->hotplug_lock);
> +		mutex_lock(&p_slot->lock);
> +		p_slot->state = POWERON_STATE;
> +		mutex_unlock(&p_slot->lock);
>  		ret = pciehp_enable_slot(p_slot);
>  		mutex_unlock(&p_slot->hotplug_lock);
>  		if (ret)
> @@ -209,8 +215,6 @@ static void pciehp_queue_power_work(struct slot *p_slot, int req)
>  {
>  	struct power_work_info *info;
>  
> -	p_slot->state = (req == ENABLE_REQ) ? POWERON_STATE : POWEROFF_STATE;
> -
>  	info = kmalloc(sizeof(*info), GFP_KERNEL);
>  	if (!info) {
>  		ctrl_err(p_slot->ctrl, "no memory to queue %s request\n",
> -- 
> 2.7.4
> 

Re: [PATCH] pciehp: Fix race condition handling surprise link-down

[Raj, Ashok]

Hi Bjorn

On Thu, Feb 02, 2017 at 08:59:01PM -0600, Bjorn Helgaas wrote:
> Hi Ashok,
> 
> Sorry it took me so long to review this.  I never felt like I really
> understood it, and it took me a long time to try to figure out a more
> useful response.

No worries. Agree its a litte tricky, and took me several iterations before
doing someting that was simple enough, without a complete overhaul of
state management. 

Thanks a ton for capturing the sequence, I did capture
some debug output along at that time. My apologies for not adding it
along. But this becomes excellant notes and perhaps would be good to 
capture in commit or in the documentation. Going through this isn't fun :-)


Responses below:
> > 
> > This patch fixes that by setting the p_slot->state only when the work to
> > handle the power event is executing, protected by the p_slot->hotplug_lock.
> 
> So let me first try to understand what's going on with the current
> code.  In the normal case where a device is removed or turned off and
> pciehp can complete everything before another device appears, I think
> the flow is like this:

You got this problem part right. Spot on!
> 
>       p_slot->state == STATIC_STATE (powered on, link up)
> 
>                         <-- surprise link down interrupt
>       pciehp_isr()
>         queue INT_LINK_DOWN work
> 
>       interrupt_event_handler(INT_LINK_DOWN)
>         set p_slot->state = POWEROFF_STATE
>         queue DISABLE_REQ work
> 
>       pciehp_power_thread(DISABLE_REQ)
>         send PCI_EXP_SLTCTL_PWR_OFF command
>         wait for power-off to complete
>         set p_slot->state = STATIC_STATE
> 
>       p_slot->state == STATIC_STATE (powered off)
> 
> In the problem case, the link goes down, and while pciehp is still
> dealing with that, the link comes back up.  So I think one possible
> sequence is like this:
> 
>       p_slot->state == STATIC_STATE (powered on, link up)
> 
>                         <-- surprise link down interrupt
>   1a  pciehp_isr()
>         queue INT_LINK_DOWN work                     # queued: 1-LD
> 
>   1b  interrupt_event_handler(INT_LINK_DOWN)         # process 1-LD
>         # handle_link_event() sees case STATIC_STATE
>         set p_slot->state = POWEROFF_STATE
>         queue DISABLE_REQ work                       # queued: 1-DR
> 
>                         <-- surprise link up interrupt
>   2a  pciehp_isr()
>         queue INT_LINK_UP work                       # queued: 1-DR 2-LU
> 
>   1c  pciehp_power_thread(DISABLE_REQ)               # process 1-DR
>         send PCI_EXP_SLTCTL_PWR_OFF command
>         wait for power-off to complete
>         set p_slot->state = STATIC_STATE
> 
>                         <-- link down interrupt (result of PWR_OFF)
>   3a  pciehp_isr()
>         queue INT_LINK_DOWN work                     # queued: 2-LU 3-LD
> 
>   2b  interrupt_event_handler(INT_LINK_UP)           # process 2-LU
>         # handle_link_event() sees case STATIC_STATE
>         set p_slot->state = POWERON_STATE
>         queue ENABLE_REQ work                        # queued: 3-LD 2-ER
> 
>   3b  interrupt_event_handler(INT_LINK_DOWN)         # process 3-LD
>         # handle_link_event() sees case POWERON_STATE, so we emit
>         # "Link Down event queued; currently getting powered on"
>         set p_slot->state = POWEROFF_STATE
>         queue DISABLE_REQ work                       # queued: 2-ER 3-DR
> 
>   2c  pciehp_power_thread(ENABLE_REQ)                # process 2-ER
>         send PCI_EXP_SLTCTL_PWR_ON command
>         wait for power-on to complete
>         set p_slot->state = STATIC_STATE
> 
>                         <-- link up interrupt (result of PWR_ON)
>   4a  pciehp_isr()
>         queue INT_LINK_UP work                       # queued: 3-DR 4-LU
> 
>   3c  pciehp_power_thread(DISABLE_REQ)               # process 3-DR
>         send PCI_EXP_SLTCTL_PWR_OFF command
>         wait for power-off to complete
>         set p_slot->state = STATIC_STATE
> 
>                         <-- link down interrupt (result of PWR_OFF)
>   5a  pciehp_isr()
>         queue INT_LINK_DOWN work                     # queued: 4-LU 5-LD
> 
> State 5a is the same as 3a (we're in STATIC_STATE with Link Up and
> Link Down work items queued), so the whole cycle can repeat.
> 
> Now let's assume we apply this patch and see what changes.  The patch
> changes where we set p_slot->state.  Currently we set POWEROFF_STATE
> or POWERON_STATE in the interrupt_event_handler() work item.  The
> patch moves that to the pciehp_power_thread() work item, where the
> power commands are actually sent.

Right. The difference with this patch is when we set the state to 
POWERON_STATE or POWEROFF_STATE, we only do that when the previous
POWER* operation has entirely completed. Since now its protected with the
hotplug_lock mutex.

In the problem case, since we set the state before the pciehp_power_thread,
we end up changing the state to POWER*_STATE before the previous POWER*
action has completed.
> 
>       p_slot->state == STATIC_STATE (powered on, link up)
> 
>                         <-- surprise link down interrupt
>   1A  pciehp_isr()
>         queue INT_LINK_DOWN work                     # queued: 1-LD
> 
>   1B  interrupt_event_handler(INT_LINK_DOWN)         # process 1-LD
>         # handle_link_event() sees case STATIC_STATE
>         # set p_slot->state = POWEROFF_STATE         # (removed by patch)
>         queue DISABLE_REQ work                       # queued: 1-DR
> 
>                         <-- surprise link up interrupt
>   2A  pciehp_isr()
>         queue INT_LINK_UP work                       # queued: 1-DR 2-LU
> 
>   1C  pciehp_power_thread(DISABLE_REQ)               # process 1-DR

	Also mutex hotplug_lock is held.

>         set p_slot->state = POWEROFF_STATE           # (added by patch)
>         send PCI_EXP_SLTCTL_PWR_OFF command
>         wait for power-off to complete
>         set p_slot->state = STATIC_STATE
> 
>                         <-- link down interrupt (result of PWR_OFF)
>   3A  pciehp_isr()
>         queue INT_LINK_DOWN work                     # queued: 2-LU 3-LD

The above INT_LINK_DOWN will eventually be ignored in handle_link_event()
because we are in POWEROFF_STATE, and a link down while in POWEROFF will
be ignored.
> 
>   2B  interrupt_event_handler(INT_LINK_UP)           # process 2-LU
>         # handle_link_event() sees case STATIC_STATE
>         # set p_slot->state = POWERON_STATE          # (removed by patch)
>         queue ENABLE_REQ work                        # queued: 3-LD 2-ER
> 
>   3B  interrupt_event_handler(INT_LINK_DOWN)         # process 3-LD
>         # handle_link_event() sees case STATIC_STATE,
>         # unlike 3b above, which saw POWERON_STATE;
>         # doesn't emit a message, but still queues DISABLE_REQ
>         # set p_slot->state = POWEROFF_STATE         # (removed by patch)
>         queue DISABLE_REQ work                       # queued: 2-ER 3-DR

3B will be ignored, since handle_link_event() knows we are in process
of POWEROFF.

> 
>   2C  pciehp_power_thread(ENABLE_REQ)                # process 2-ER

We are also protected by mutex hotplug_lock here. So  the following
wont get executed until step 1C has run to completion and the 
mutex is released.

>         set p_slot->state = POWERON_STATE            # (added by patch)
>         send PCI_EXP_SLTCTL_PWR_ON command
>         wait for power-on to complete
>         set p_slot->state = STATIC_STATE
> 
>                         <-- link up interrupt (result of PWR_ON)
>   4A  pciehp_isr()
>         queue INT_LINK_UP work                       # queued: 3-DR 4-LU

handle_link_event() would eventually dismiss the INT_LINK_UP since
it knows we are in process of POWERON.
> 
>   3C  pciehp_power_thread(DISABLE_REQ)               # process 3-DR
>         set p_slot->state = POWEROFF_STATE           # (added by patch)
>         send PCI_EXP_SLTCTL_PWR_OFF command
>         wait for power-off to complete
>         set p_slot->state = STATIC_STATE
> 
>                         <-- link down interrupt (result of PWR_OFF)
>   5A  pciehp_isr()
>         queue INT_LINK_DOWN work                     # queued: 4-LU 5-LD
> 
> With this particular ordering, I think we still have the same problem:
> 5A is the same as 3A, so I think the cycle could repeat.

I think the sequence is almost right, except the fact since we are protected
by hotplug_lock, we don't allow another POWERON or POWEROFF to be processed
until the previous POWER* operation is completed entirely.

We have run through several experiments with managed power on, power off,
using attention button sequence and surprise link down, surprise remove
in the lab and we haven't run into any other issues.

Just to summarize, we only queue the POWEROFF due to surprise link down
and another POWERON due to link becoming back up. The transient link-down 
events are coveniently ignored.

Hope this helps, and sincere thanks for looking into this in great detail!

Cheers,
Ashok

Re: [PATCH] pciehp: Fix race condition handling surprise link-down

[Bjorn Helgaas]

On Thu, Feb 02, 2017 at 10:00:53PM -0800, Raj, Ashok wrote:
> Hi Bjorn
> 
> On Thu, Feb 02, 2017 at 08:59:01PM -0600, Bjorn Helgaas wrote:
> > Hi Ashok,
> > 
> > Sorry it took me so long to review this.  I never felt like I really
> > understood it, and it took me a long time to try to figure out a more
> > useful response.
> 
> No worries. Agree its a litte tricky, and took me several iterations before
> doing someting that was simple enough, without a complete overhaul of
> state management. 
> 
> Thanks a ton for capturing the sequence, I did capture
> some debug output along at that time. My apologies for not adding it
> along. But this becomes excellant notes and perhaps would be good to 
> capture in commit or in the documentation. Going through this isn't fun :-)

Maybe you could open a kernel.org bugzilla and attach the dmesg log
and "lspci -vv" output.  Then we could capture some of your logs and
this discussion there and include a pointer in the changelog.

> Responses below:
> > > 
> > > This patch fixes that by setting the p_slot->state only when the work to
> > > handle the power event is executing, protected by the p_slot->hotplug_lock.
> > 
> > So let me first try to understand what's going on with the current
> > code.  In the normal case where a device is removed or turned off and
> > pciehp can complete everything before another device appears, I think
> > the flow is like this:
> 
> You got this problem part right. Spot on!
> > 
> >       p_slot->state == STATIC_STATE (powered on, link up)
> > 
> >                         <-- surprise link down interrupt
> >       pciehp_isr()
> >         queue INT_LINK_DOWN work
> > 
> >       interrupt_event_handler(INT_LINK_DOWN)
> >         set p_slot->state = POWEROFF_STATE
> >         queue DISABLE_REQ work
> > 
> >       pciehp_power_thread(DISABLE_REQ)
> >         send PCI_EXP_SLTCTL_PWR_OFF command
> >         wait for power-off to complete
> >         set p_slot->state = STATIC_STATE
> > 
> >       p_slot->state == STATIC_STATE (powered off)
> > 
> > In the problem case, the link goes down, and while pciehp is still
> > dealing with that, the link comes back up.  So I think one possible
> > sequence is like this:
> > 
> >       p_slot->state == STATIC_STATE (powered on, link up)
> > 
> >                         <-- surprise link down interrupt
> >   1a  pciehp_isr()
> >         queue INT_LINK_DOWN work                     # queued: 1-LD
> > 
> >   1b  interrupt_event_handler(INT_LINK_DOWN)         # process 1-LD
> >         # handle_link_event() sees case STATIC_STATE
> >         set p_slot->state = POWEROFF_STATE
> >         queue DISABLE_REQ work                       # queued: 1-DR
> > 
> >                         <-- surprise link up interrupt
> >   2a  pciehp_isr()
> >         queue INT_LINK_UP work                       # queued: 1-DR 2-LU
> > 
> >   1c  pciehp_power_thread(DISABLE_REQ)               # process 1-DR
> >         send PCI_EXP_SLTCTL_PWR_OFF command
> >         wait for power-off to complete
> >         set p_slot->state = STATIC_STATE
> > 
> >                         <-- link down interrupt (result of PWR_OFF)
> >   3a  pciehp_isr()
> >         queue INT_LINK_DOWN work                     # queued: 2-LU 3-LD
> > 
> >   2b  interrupt_event_handler(INT_LINK_UP)           # process 2-LU
> >         # handle_link_event() sees case STATIC_STATE
> >         set p_slot->state = POWERON_STATE
> >         queue ENABLE_REQ work                        # queued: 3-LD 2-ER
> > 
> >   3b  interrupt_event_handler(INT_LINK_DOWN)         # process 3-LD
> >         # handle_link_event() sees case POWERON_STATE, so we emit
> >         # "Link Down event queued; currently getting powered on"
> >         set p_slot->state = POWEROFF_STATE
> >         queue DISABLE_REQ work                       # queued: 2-ER 3-DR
> > 
> >   2c  pciehp_power_thread(ENABLE_REQ)                # process 2-ER
> >         send PCI_EXP_SLTCTL_PWR_ON command
> >         wait for power-on to complete
> >         set p_slot->state = STATIC_STATE
> > 
> >                         <-- link up interrupt (result of PWR_ON)
> >   4a  pciehp_isr()
> >         queue INT_LINK_UP work                       # queued: 3-DR 4-LU
> > 
> >   3c  pciehp_power_thread(DISABLE_REQ)               # process 3-DR
> >         send PCI_EXP_SLTCTL_PWR_OFF command
> >         wait for power-off to complete
> >         set p_slot->state = STATIC_STATE
> > 
> >                         <-- link down interrupt (result of PWR_OFF)
> >   5a  pciehp_isr()
> >         queue INT_LINK_DOWN work                     # queued: 4-LU 5-LD
> > 
> > State 5a is the same as 3a (we're in STATIC_STATE with Link Up and
> > Link Down work items queued), so the whole cycle can repeat.
> > 
> > Now let's assume we apply this patch and see what changes.  The patch
> > changes where we set p_slot->state.  Currently we set POWEROFF_STATE
> > or POWERON_STATE in the interrupt_event_handler() work item.  The
> > patch moves that to the pciehp_power_thread() work item, where the
> > power commands are actually sent.
> 
> Right. The difference with this patch is when we set the state to 
> POWERON_STATE or POWEROFF_STATE, we only do that when the previous
> POWER* operation has entirely completed. Since now its protected with the
> hotplug_lock mutex.
> 
> In the problem case, since we set the state before the pciehp_power_thread,
> we end up changing the state to POWER*_STATE before the previous POWER*
> action has completed.
> > 
> >       p_slot->state == STATIC_STATE (powered on, link up)
> > 
> >                         <-- surprise link down interrupt
> >   1A  pciehp_isr()
> >         queue INT_LINK_DOWN work                     # queued: 1-LD
> > 
> >   1B  interrupt_event_handler(INT_LINK_DOWN)         # process 1-LD
> >         # handle_link_event() sees case STATIC_STATE
> >         # set p_slot->state = POWEROFF_STATE         # (removed by patch)
> >         queue DISABLE_REQ work                       # queued: 1-DR
> > 
> >                         <-- surprise link up interrupt
> >   2A  pciehp_isr()
> >         queue INT_LINK_UP work                       # queued: 1-DR 2-LU
> > 
> >   1C  pciehp_power_thread(DISABLE_REQ)               # process 1-DR
> 
> 	Also mutex hotplug_lock is held.
> 
> >         set p_slot->state = POWEROFF_STATE           # (added by patch)
> >         send PCI_EXP_SLTCTL_PWR_OFF command
> >         wait for power-off to complete
> >         set p_slot->state = STATIC_STATE
> > 
> >                         <-- link down interrupt (result of PWR_OFF)
> >   3A  pciehp_isr()
> >         queue INT_LINK_DOWN work                     # queued: 2-LU 3-LD
> 
> The above INT_LINK_DOWN will eventually be ignored in handle_link_event()
> because we are in POWEROFF_STATE, and a link down while in POWEROFF will
> be ignored.
> > 
> >   2B  interrupt_event_handler(INT_LINK_UP)           # process 2-LU
> >         # handle_link_event() sees case STATIC_STATE
> >         # set p_slot->state = POWERON_STATE          # (removed by patch)
> >         queue ENABLE_REQ work                        # queued: 3-LD 2-ER
> > 
> >   3B  interrupt_event_handler(INT_LINK_DOWN)         # process 3-LD
> >         # handle_link_event() sees case STATIC_STATE,
> >         # unlike 3b above, which saw POWERON_STATE;
> >         # doesn't emit a message, but still queues DISABLE_REQ
> >         # set p_slot->state = POWEROFF_STATE         # (removed by patch)
> >         queue DISABLE_REQ work                       # queued: 2-ER 3-DR
> 
> 3B will be ignored, since handle_link_event() knows we are in process
> of POWEROFF.

What enforces this ordering?  handle_link_event() will only see
POWEROFF_STATE if it happens to read the state after
pciehp_power_thread() sets POWEROFF_STATE and before it
sets it back to STATIC_STATE.  Given our work item concurrency,
I think that's possible, but I don't see how it's guaranteed.

> >   2C  pciehp_power_thread(ENABLE_REQ)                # process 2-ER
> 
> We are also protected by mutex hotplug_lock here. So  the following
> wont get executed until step 1C has run to completion and the 
> mutex is released.
> 
> >         set p_slot->state = POWERON_STATE            # (added by patch)
> >         send PCI_EXP_SLTCTL_PWR_ON command
> >         wait for power-on to complete
> >         set p_slot->state = STATIC_STATE
> > 
> >                         <-- link up interrupt (result of PWR_ON)
> >   4A  pciehp_isr()
> >         queue INT_LINK_UP work                       # queued: 3-DR 4-LU
> 
> handle_link_event() would eventually dismiss the INT_LINK_UP since
> it knows we are in process of POWERON.
> > 
> >   3C  pciehp_power_thread(DISABLE_REQ)               # process 3-DR
> >         set p_slot->state = POWEROFF_STATE           # (added by patch)
> >         send PCI_EXP_SLTCTL_PWR_OFF command
> >         wait for power-off to complete
> >         set p_slot->state = STATIC_STATE
> > 
> >                         <-- link down interrupt (result of PWR_OFF)
> >   5A  pciehp_isr()
> >         queue INT_LINK_DOWN work                     # queued: 4-LU 5-LD
> > 
> > With this particular ordering, I think we still have the same problem:
> > 5A is the same as 3A, so I think the cycle could repeat.
> 
> I think the sequence is almost right, except the fact since we are protected
> by hotplug_lock, we don't allow another POWERON or POWEROFF to be processed
> until the previous POWER* operation is completed entirely.

handle_link_event() is protected by "lock" but not by "hotplug_lock",
so I think it can queue ENABLE/DISABLE items even before the previous
POWER* operation completes.

You're right that I omitted the hotplug_lock details.  I added them to
my outline (at https://goo.gl/szqWTC if you're interested), but I
don't see how that prevents the scenario above.

> Just to summarize, we only queue the POWEROFF due to surprise link down
> and another POWERON due to link becoming back up. The transient link-down 
> events are coveniently ignored.

I'm leery about ignoring events, though it happens to be convenient in
this case.  I think we're ignoring them because we're running work
items simultaneously with other items, and I think that concurrency is
unnecessary complexity.

I think it would be safer to queue every event and process every event
serially.

Re: [PATCH] pciehp: Fix race condition handling surprise link-down

[Bjorn Helgaas]

On Fri, Feb 03, 2017 at 10:51:04AM -0600, Bjorn Helgaas wrote:
> On Thu, Feb 02, 2017 at 10:00:53PM -0800, Raj, Ashok wrote:
> > ...
> > Just to summarize, we only queue the POWEROFF due to surprise link down
> > and another POWERON due to link becoming back up. The transient link-down 
> > events are coveniently ignored.
> 
> I'm leery about ignoring events, though it happens to be convenient in
> this case.  I think we're ignoring them because we're running work
> items simultaneously with other items, and I think that concurrency is
> unnecessary complexity.
> 
> I think it would be safer to queue every event and process every event
> serially.

Hi Ashok,

Just a ping to make sure we're not deadlocked.  I'm waiting for you,
so I hope you're not also waiting for me :)  I'm not trying to rush you;
I just don't want to drop this by mistake.

Bjorn

Re: [PATCH] pciehp: Fix race condition handling surprise link-down

[Raj, Ashok]

On Mon, Mar 06, 2017 at 06:24:17PM -0600, Bjorn Helgaas wrote:
> On Fri, Feb 03, 2017 at 10:51:04AM -0600, Bjorn Helgaas wrote:
> 
> Hi Ashok,
> 
> Just a ping to make sure we're not deadlocked.  I'm waiting for you,
> so I hope you're not also waiting for me :)  I'm not trying to rush you;
> I just don't want to drop this by mistake.
> 
Hi Bjorn

no we aren't deadlocked :-). I didn't get around changing it to ordered
queue yet, mostly worried about having to retest all the different 
combinations with ATTN, POWER_CTL, SLD.

I'm depending on other folks to test SLD. They are tied up with other
issues ATM.

I have had another OEM test with several disks and multiple ATTN's 
pressed/cancel and current code seems to be working well so far, except the 
SLD case.

The change in the patch was only ensuring that we don't start another 
POWER_ON or POWER_OFF before the earlier operation was complete.

Would it be alright to fix SLD with this version while we can probe a clean
approach that can give us sufficient time to test a clean approach that works
with all the different combinations and OEM systems?

Cheers
Ashok

Re: [PATCH] pciehp: Fix race condition handling surprise link-down

[Bjorn Helgaas]

On Wed, Mar 08, 2017 at 04:27:26AM -0800, Raj, Ashok wrote:
> On Mon, Mar 06, 2017 at 06:24:17PM -0600, Bjorn Helgaas wrote:
> > On Fri, Feb 03, 2017 at 10:51:04AM -0600, Bjorn Helgaas wrote:
> > 
> > Hi Ashok,
> > 
> > Just a ping to make sure we're not deadlocked.  I'm waiting for you,
> > so I hope you're not also waiting for me :)  I'm not trying to rush you;
> > I just don't want to drop this by mistake.
> > 
> Hi Bjorn
> 
> no we aren't deadlocked :-). I didn't get around changing it to ordered
> queue yet, mostly worried about having to retest all the different 
> combinations with ATTN, POWER_CTL, SLD.
> 
> I'm depending on other folks to test SLD. They are tied up with other
> issues ATM.
> 
> I have had another OEM test with several disks and multiple ATTN's 
> pressed/cancel and current code seems to be working well so far, except the 
> SLD case.
> 
> The change in the patch was only ensuring that we don't start another 
> POWER_ON or POWER_OFF before the earlier operation was complete.
> 
> Would it be alright to fix SLD with this version while we can probe a clean
> approach that can give us sufficient time to test a clean approach that works
> with all the different combinations and OEM systems?

This version avoids some SLD issues, but we can't guarantee that it is
a complete solution.

I don't really like to put in an incomplete solution because it
reduces the urgency for doing a proper fix, and it also complicates
debugging if we trip over an SLD issue we haven't seen yet during
testing.

Bjorn