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.
