On Thu, Jul 06, 2017 at 02:32:02AM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wyso...@intel.com>
>
> Commit 76cde7e49590 (PCI / PM: Make PCIe PME interrupts wake up from
> suspend-to-idle) went too far with preventing pcie_pme_work_fn() from
> clearing the root port's PME Status and re-enabling the PME interrupt
> which should be done for PMEs to work correctly after system resume.
>
> The failing scenario is as follows:
>
> 1. pcie_pme_suspend() finds that the PME IRQ should be designated
> for system wakeup, so it calls enable_irq_wake() and then sets
> data->suspend_level to PME_SUSPEND_WAKEUP.
>
> 2. PME interrupt happens at this point.
>
> 3. pcie_pme_irq() runs, disables the PME interrupt and queues up
> the execution of pcie_pme_work_fn().
>
> 4. pcie_pme_work_fn() runs before pcie_pme_resume() and breaks out
> of the loop right away, because data->suspend_level is not
> PME_SUSPEND_NONE, and it doesn't re-enable the PME interrupt
> for the same reason.
>
> 5. pcie_pme_resume() runs and simply calls disable_irq_wake()
> without re-enabling the PME interrupt (because data->suspend_level
> is not PME_SUSPEND_NONE), so the PME interrupt remains disabled
> and the PME Status remains set.
>
> To fix this notice that there is no reason why pcie_pme_work_fn()
> should behave in a special way during system resume if the PME
> interrupt is not disabled by pcie_pme_suspend() and partially revert
> commit 76cde7e49590 and restore the previous (and correct) behavior
> of pcie_pme_work_fn().
>
> Fixes: 76cde7e49590 (PCI / PM: Make PCIe PME interrupts wake up from
> suspend-to-idle)
> Reported-by: Naresh Solanki <naresh.sola...@intel.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wyso...@intel.com>
Acked-by: Bjorn Helgaas <bhelg...@google.com>
I assume you'll apply this; let me know if you need anything else from me.
> ---
> drivers/pci/pcie/pme.c | 35 +++++++++++++----------------------
> 1 file changed, 13 insertions(+), 22 deletions(-)
>
> Index: linux-pm/drivers/pci/pcie/pme.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pcie/pme.c
> +++ linux-pm/drivers/pci/pcie/pme.c
> @@ -40,17 +40,11 @@ static int __init pcie_pme_setup(char *s
> }
> __setup("pcie_pme=", pcie_pme_setup);
>
> -enum pme_suspend_level {
> - PME_SUSPEND_NONE = 0,
> - PME_SUSPEND_WAKEUP,
> - PME_SUSPEND_NOIRQ,
> -};
> -
> struct pcie_pme_service_data {
> spinlock_t lock;
> struct pcie_device *srv;
> struct work_struct work;
> - enum pme_suspend_level suspend_level;
> + bool noirq; /* If set, keep the PME interrupt disabled. */
> };
>
> /**
> @@ -228,7 +222,7 @@ static void pcie_pme_work_fn(struct work
> spin_lock_irq(&data->lock);
>
> for (;;) {
> - if (data->suspend_level != PME_SUSPEND_NONE)
> + if (data->noirq)
> break;
>
> pcie_capability_read_dword(port, PCI_EXP_RTSTA, &rtsta);
> @@ -255,7 +249,7 @@ static void pcie_pme_work_fn(struct work
> spin_lock_irq(&data->lock);
> }
>
> - if (data->suspend_level == PME_SUSPEND_NONE)
> + if (!data->noirq)
> pcie_pme_interrupt_enable(port, true);
>
> spin_unlock_irq(&data->lock);
> @@ -378,7 +372,7 @@ static int pcie_pme_suspend(struct pcie_
> {
> struct pcie_pme_service_data *data = get_service_data(srv);
> struct pci_dev *port = srv->port;
> - bool wakeup, wake_irq_enabled = false;
> + bool wakeup;
> int ret;
>
> if (device_may_wakeup(&port->dev)) {
> @@ -388,19 +382,16 @@ static int pcie_pme_suspend(struct pcie_
> wakeup = pcie_pme_check_wakeup(port->subordinate);
> up_read(&pci_bus_sem);
> }
> - spin_lock_irq(&data->lock);
> if (wakeup) {
> ret = enable_irq_wake(srv->irq);
> - if (ret == 0) {
> - data->suspend_level = PME_SUSPEND_WAKEUP;
> - wake_irq_enabled = true;
> - }
> - }
> - if (!wake_irq_enabled) {
> - pcie_pme_interrupt_enable(port, false);
> - pcie_clear_root_pme_status(port);
> - data->suspend_level = PME_SUSPEND_NOIRQ;
> + if (!ret)
> + return 0;
> }
> +
> + spin_lock_irq(&data->lock);
> + pcie_pme_interrupt_enable(port, false);
> + pcie_clear_root_pme_status(port);
> + data->noirq = true;
> spin_unlock_irq(&data->lock);
>
> synchronize_irq(srv->irq);
> @@ -417,15 +408,15 @@ static int pcie_pme_resume(struct pcie_d
> struct pcie_pme_service_data *data = get_service_data(srv);
>
> spin_lock_irq(&data->lock);
> - if (data->suspend_level == PME_SUSPEND_NOIRQ) {
> + if (data->noirq) {
> struct pci_dev *port = srv->port;
>
> pcie_clear_root_pme_status(port);
> pcie_pme_interrupt_enable(port, true);
> + data->noirq = false;
> } else {
> disable_irq_wake(srv->irq);
> }
> - data->suspend_level = PME_SUSPEND_NONE;
> spin_unlock_irq(&data->lock);
>
> return 0;
>
