Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1572862 > unrolled thread
| Started by | Bjorn Helgaas <helgaas@kernel.org> |
|---|---|
| First post | 2017-02-03 04:00 +0100 |
| Last post | 2017-02-03 18:00 +0100 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH] pciehp: Fix race condition handling surprise link-down Bjorn Helgaas <helgaas@kernel.org> - 2017-02-03 04:00 +0100
Re: [PATCH] pciehp: Fix race condition handling surprise link-down "Raj, Ashok" <ashok.raj@intel.com> - 2017-02-03 07:10 +0100
Re: [PATCH] pciehp: Fix race condition handling surprise link-down Bjorn Helgaas <helgaas@kernel.org> - 2017-02-03 18:00 +0100
| From | Bjorn Helgaas <helgaas@kernel.org> |
|---|---|
| Date | 2017-02-03 04:00 +0100 |
| Subject | Re: [PATCH] pciehp: Fix race condition handling surprise link-down |
| Message-ID | <t6Cgp-41e-1@gated-at.bofh.it> |
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
>
[toc] | [next] | [standalone]
| From | "Raj, Ashok" <ashok.raj@intel.com> |
|---|---|
| Date | 2017-02-03 07:10 +0100 |
| Message-ID | <t6Feh-6c1-5@gated-at.bofh.it> |
| In reply to | #1572862 |
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
[toc] | [prev] | [next] | [standalone]
| From | Bjorn Helgaas <helgaas@kernel.org> |
|---|---|
| Date | 2017-02-03 18:00 +0100 |
| Message-ID | <t6Pnj-4ec-3@gated-at.bofh.it> |
| In reply to | #1572902 |
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.
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web