* [PATCH] PCI / PM: Fix native PME handling during system suspend/resume
@ 2017-07-06 0:32 Rafael J. Wysocki
2017-07-12 22:42 ` Bjorn Helgaas
0 siblings, 1 reply; 3+ messages in thread
From: Rafael J. Wysocki @ 2017-07-06 0:32 UTC (permalink / raw)
To: Linux PCI
Cc: Bjorn Helgaas, Mika Westerberg, Linux PM, LKML, Sadashiva Rao Pv,
Naresh Solanki, Rafael J. Wysocki
From: Rafael J. Wysocki <rafael.j.wysocki@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.solanki@intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
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;
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] PCI / PM: Fix native PME handling during system suspend/resume
2017-07-06 0:32 [PATCH] PCI / PM: Fix native PME handling during system suspend/resume Rafael J. Wysocki
@ 2017-07-12 22:42 ` Bjorn Helgaas
0 siblings, 0 replies; 3+ messages in thread
From: Bjorn Helgaas @ 2017-07-12 22:42 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Linux PCI, Bjorn Helgaas, Mika Westerberg, Linux PM, LKML,
Sadashiva Rao Pv, Naresh Solanki, Rafael J. Wysocki
On Thu, Jul 06, 2017 at 02:32:02AM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@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.solanki@intel.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Acked-by: Bjorn Helgaas <bhelgaas@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;
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] PCI / PM: Fix native PME handling during system suspend/resume
[not found] <2516f96e-bc32-46ac-acc9-8a5e16c44245@email.android.com>
@ 2017-07-12 23:39 ` Rafael J. Wysocki
0 siblings, 0 replies; 3+ messages in thread
From: Rafael J. Wysocki @ 2017-07-12 23:39 UTC (permalink / raw)
To: Solanki, Naresh
Cc: Rao Pv, Sadashiva, Linux PM, Mika Westerberg, Bjorn Helgaas,
Linux PCI, LKML, Wysocki, Rafael J
On Wednesday, July 12, 2017 11:32:17 PM Solanki, Naresh wrote:
> Hi Rafael,
>
> We tested the patch and completed ~400cycles of s3 and s0ix. No regression observed.
Awesome, thanks!
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-07-12 23:46 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-06 0:32 [PATCH] PCI / PM: Fix native PME handling during system suspend/resume Rafael J. Wysocki
2017-07-12 22:42 ` Bjorn Helgaas
[not found] <2516f96e-bc32-46ac-acc9-8a5e16c44245@email.android.com>
2017-07-12 23:39 ` Rafael J. Wysocki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).