lkml.org 
[lkml]   [2020]   [Apr]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 5.5 113/257] PCI: pciehp: Fix indefinite wait on sysfs requests
    Date
    From: Lukas Wunner <lukas@wunner.de>

    commit 3e487d2e4aa466decd226353755c9d423e8fbacc upstream.

    David Hoyer reports that powering pciehp slots up or down via sysfs may
    hang: The call to wait_event() in pciehp_sysfs_enable_slot() and
    _disable_slot() does not return because ctrl->ist_running remains true.

    This flag, which was introduced by commit 157c1062fcd8 ("PCI: pciehp: Avoid
    returning prematurely from sysfs requests"), signifies that the IRQ thread
    pciehp_ist() is running. It is set to true at the top of pciehp_ist() and
    reset to false at the end. However there are two additional return
    statements in pciehp_ist() before which the commit neglected to reset the
    flag to false and wake up waiters for the flag.

    That omission opens up the following race when powering up the slot:

    * pciehp_ist() runs because a PCI_EXP_SLTSTA_PDC event was requested
    by pciehp_sysfs_enable_slot()

    * pciehp_ist() turns on slot power via the following call stack:
    pciehp_handle_presence_or_link_change() -> pciehp_enable_slot() ->
    __pciehp_enable_slot() -> board_added() -> pciehp_power_on_slot()

    * after slot power is turned on, the link comes up, resulting in a
    PCI_EXP_SLTSTA_DLLSC event

    * the IRQ handler pciehp_isr() stores the event in ctrl->pending_events
    and returns IRQ_WAKE_THREAD

    * the IRQ thread is already woken (it's bringing up the slot), but the
    genirq code remembers to re-run the IRQ thread after it has finished
    (such that it can deal with the new event) by setting IRQTF_RUNTHREAD
    via __handle_irq_event_percpu() -> __irq_wake_thread()

    * the IRQ thread removes PCI_EXP_SLTSTA_DLLSC from ctrl->pending_events
    via board_added() -> pciehp_check_link_status() in order to deal with
    presence and link flaps per commit 6c35a1ac3da6 ("PCI: pciehp:
    Tolerate initially unstable link")

    * after pciehp_ist() has successfully brought up the slot, it resets
    ctrl->ist_running to false and wakes up the sysfs requester

    * the genirq code re-runs pciehp_ist(), which sets ctrl->ist_running
    to true but then returns with IRQ_NONE because ctrl->pending_events
    is empty

    * pciehp_sysfs_enable_slot() is finally woken but notices that
    ctrl->ist_running is true, hence continues waiting

    The only way to get the hung task going again is to trigger a hotplug
    event which brings down the slot, e.g. by yanking out the card.

    The same race exists when powering down the slot because remove_board()
    likewise clears link or presence changes in ctrl->pending_events per commit
    3943af9d01e9 ("PCI: pciehp: Ignore Link State Changes after powering off a
    slot") and thereby may cause a re-run of pciehp_ist() which returns with
    IRQ_NONE without resetting ctrl->ist_running to false.

    Fix by adding a goto label before the teardown steps at the end of
    pciehp_ist() and jumping to that label from the two return statements which
    currently neglect to reset the ctrl->ist_running flag.

    Fixes: 157c1062fcd8 ("PCI: pciehp: Avoid returning prematurely from sysfs requests")
    Link: https://lore.kernel.org/r/cca1effa488065cb055120aa01b65719094bdcb5.1584530321.git.lukas@wunner.de
    Reported-by: David Hoyer <David.Hoyer@netapp.com>
    Signed-off-by: Lukas Wunner <lukas@wunner.de>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    Reviewed-by: Keith Busch <kbusch@kernel.org>
    Cc: stable@vger.kernel.org # v4.19+
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

    ---
    drivers/pci/hotplug/pciehp_hpc.c | 14 +++++++-------
    1 file changed, 7 insertions(+), 7 deletions(-)

    --- a/drivers/pci/hotplug/pciehp_hpc.c
    +++ b/drivers/pci/hotplug/pciehp_hpc.c
    @@ -625,17 +625,15 @@ static irqreturn_t pciehp_ist(int irq, v
    if (atomic_fetch_and(~RERUN_ISR, &ctrl->pending_events) & RERUN_ISR) {
    ret = pciehp_isr(irq, dev_id);
    enable_irq(irq);
    - if (ret != IRQ_WAKE_THREAD) {
    - pci_config_pm_runtime_put(pdev);
    - return ret;
    - }
    + if (ret != IRQ_WAKE_THREAD)
    + goto out;
    }

    synchronize_hardirq(irq);
    events = atomic_xchg(&ctrl->pending_events, 0);
    if (!events) {
    - pci_config_pm_runtime_put(pdev);
    - return IRQ_NONE;
    + ret = IRQ_NONE;
    + goto out;
    }

    /* Check Attention Button Pressed */
    @@ -664,10 +662,12 @@ static irqreturn_t pciehp_ist(int irq, v
    pciehp_handle_presence_or_link_change(ctrl, events);
    up_read(&ctrl->reset_lock);

    + ret = IRQ_HANDLED;
    +out:
    pci_config_pm_runtime_put(pdev);
    ctrl->ist_running = false;
    wake_up(&ctrl->requester);
    - return IRQ_HANDLED;
    + return ret;
    }

    static int pciehp_poll(void *data)

    \
     
     \ /
      Last update: 2020-04-16 15:52    [W:3.264 / U:0.040 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site