[PATCH] PCI: Freeze PME scan before suspending devices

[PATCH] PCI: Freeze PME scan before suspending devices

[Lukas Wunner]

Laurent Pinchart reported that the Renesas R-Car H2 Lager board
(r8a7790) crashes during suspend tests.  Geert Uytterhoeven managed to
reproduce the issue on an M2-W Koelsch board (r8a7791):

It occurs when the PME scan runs, once per second.  During PME scan, the
PCI host bridge (rcar-pci) registers are accessed while its module clock
has already been disabled, leading to the crash.

One reproducer is to configure s2ram to use "s2idle" instead of "deep"
suspend:

# echo 0 > /sys/module/printk/parameters/console_suspend
# echo s2idle > /sys/power/mem_sleep
# echo mem > /sys/power/state

Another reproducer is to write either "platform" or "processors" to
/sys/power/pm_test.  It does not (or is less likely) to happen during
full system suspend ("core" or "none") because system suspend also
disables timers, and thus the workqueue handling PME scans no longer
runs.  Geert believes the issue may still happen in the small window
between disabling module clocks and disabling timers:

# echo 0 > /sys/module/printk/parameters/console_suspend
# echo platform > /sys/power/pm_test    # Or "processors"
# echo mem > /sys/power/state

(Make sure CONFIG_PCI_RCAR_GEN2 and CONFIG_USB_OHCI_HCD_PCI are
enabled.)

Rafael Wysocki agrees that PME scans should be suspended before the host
bridge registers become inaccessible.  To that end, queue the task on a
workqueue that gets frozen before devices suspend.

Rafael notes however that as a result, some wakeup events may be missed
if they are delivered via PME from a device without working IRQ (which
hence must be polled) and occur after the workqueue has been frozen.
If that turns out to be an issue in practice, it may be possible to
solve it by calling pci_pme_list_scan() once directly from one of the
host bridge's pm_ops callbacks.

Stacktrace for posterity:

PM: Syncing filesystems ... [   38.566237] done.
PM: Preparing system for sleep (mem)
Freezing user space processes ... [   38.579813] (elapsed 0.001 seconds) done.
Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
PM: Suspending system (mem)
PM: suspend of devices complete after 152.456 msecs
PM: late suspend of devices complete after 2.809 msecs
PM: noirq suspend of devices complete after 29.863 msecs
suspend debug: Waiting for 5 second(s).
Unhandled fault: asynchronous external abort (0x1211) at 0x00000000
pgd = c0003000
[00000000] *pgd=80000040004003, *pmd=00000000
Internal error: : 1211 [#1] SMP ARM
Modules linked in:
CPU: 1 PID: 20 Comm: kworker/1:1 Not tainted
4.9.0-rc1-koelsch-00011-g68db9bc814362e7f #3383
Hardware name: Generic R8A7791 (Flattened Device Tree)
Workqueue: events pci_pme_list_scan
task: eb56e140 task.stack: eb58e000
PC is at pci_generic_config_read+0x64/0x6c
LR is at rcar_pci_cfg_base+0x64/0x84
pc : [<c041d7b4>]    lr : [<c04309a0>]    psr: 600d0093
sp : eb58fe98  ip : c041d750  fp : 00000008
r10: c0e2283c  r9 : 00000000  r8 : 600d0013
r7 : 00000008  r6 : eb58fed6  r5 : 00000002  r4 : eb58feb4
r3 : 00000000  r2 : 00000044  r1 : 00000008  r0 : 00000000
Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment user
Control: 30c5387d  Table: 6a9f6c80  DAC: 55555555
Process kworker/1:1 (pid: 20, stack limit = 0xeb58e210)
Stack: (0xeb58fe98 to 0xeb590000)
fe80:                                                       00000002 00000044
fea0: eb6f5800 c041d9b0 eb58feb4 00000008 00000044 00000000 eb78a000 eb78a000
fec0: 00000044 00000000 eb9aff00 c0424bf0 eb78a000 00000000 eb78a000 c0e22830
fee0: ea8a6fc0 c0424c5c eaae79c0 c0424ce0 eb55f380 c0e22838 eb9a9800 c0235fbc
ff00: eb55f380 c0e22838 eb55f380 eb9a9800 eb9a9800 eb58e000 eb9a9824 c0e02100
ff20: eb55f398 c02366c4 eb56e140 eb5631c0 00000000 eb55f380 c023641c 00000000
ff40: 00000000 00000000 00000000 c023a928 cd105598 00000000 40506a34 eb55f380
ff60: 00000000 00000000 dead4ead ffffffff ffffffff eb58ff74 eb58ff74 00000000
ff80: 00000000 dead4ead ffffffff ffffffff eb58ff90 eb58ff90 eb58ffac eb5631c0
ffa0: c023a844 00000000 00000000 c0206d68 00000000 00000000 00000000 00000000
ffc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
ffe0: 00000000 00000000 00000000 00000000 00000013 00000000 3a81336c 10ccd1dd
[<c041d7b4>] (pci_generic_config_read) from [<c041d9b0>]
(pci_bus_read_config_word+0x58/0x80)
[<c041d9b0>] (pci_bus_read_config_word) from [<c0424bf0>]
(pci_check_pme_status+0x34/0x78)
[<c0424bf0>] (pci_check_pme_status) from [<c0424c5c>] (pci_pme_wakeup+0x28/0x54)
[<c0424c5c>] (pci_pme_wakeup) from [<c0424ce0>] (pci_pme_list_scan+0x58/0xb4)
[<c0424ce0>] (pci_pme_list_scan) from [<c0235fbc>]
(process_one_work+0x1bc/0x308)
[<c0235fbc>] (process_one_work) from [<c02366c4>] (worker_thread+0x2a8/0x3e0)
[<c02366c4>] (worker_thread) from [<c023a928>] (kthread+0xe4/0xfc)
[<c023a928>] (kthread) from [<c0206d68>] (ret_from_fork+0x14/0x2c)
Code: ea000000 e5903000 f57ff04f e3a00000 (e5843000)
---[ end trace 667d43ba3aa9e589 ]---

Reported-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reported-and-tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Cc: Simon Horman <horms+renesas@verge.net.au>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>
Cc: stable@vger.kernel.org # 2.6.37+
Fixes: df17e62e5bff ("PCI: Add support for polling PME state on suspended legacy PCI devices")
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/pci/pci.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index aa55501d2179..c561a9e4916a 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1784,8 +1784,8 @@ static void pci_pme_list_scan(struct work_struct *work)
 		}
 	}
 	if (!list_empty(&pci_pme_list))
-		schedule_delayed_work(&pci_pme_work,
-				      msecs_to_jiffies(PME_TIMEOUT));
+		queue_delayed_work(system_freezable_wq, &pci_pme_work,
+				   msecs_to_jiffies(PME_TIMEOUT));
 	mutex_unlock(&pci_pme_list_mutex);
 }
 
@@ -1850,8 +1850,9 @@ void pci_pme_active(struct pci_dev *dev, bool enable)
 			mutex_lock(&pci_pme_list_mutex);
 			list_add(&pme_dev->list, &pci_pme_list);
 			if (list_is_singular(&pci_pme_list))
-				schedule_delayed_work(&pci_pme_work,
-						      msecs_to_jiffies(PME_TIMEOUT));
+				queue_delayed_work(system_freezable_wq,
+						   &pci_pme_work,
+						   msecs_to_jiffies(PME_TIMEOUT));
 			mutex_unlock(&pci_pme_list_mutex);
 		} else {
 			mutex_lock(&pci_pme_list_mutex);
-- 
2.11.0

Re: [PATCH] PCI: Freeze PME scan before suspending devices

[Bjorn Helgaas]

On Tue, Apr 18, 2017 at 08:44:30PM +0200, Lukas Wunner wrote:
> Laurent Pinchart reported that the Renesas R-Car H2 Lager board
> (r8a7790) crashes during suspend tests.  Geert Uytterhoeven managed to
> reproduce the issue on an M2-W Koelsch board (r8a7791):
> 
> It occurs when the PME scan runs, once per second.  During PME scan, the
> PCI host bridge (rcar-pci) registers are accessed while its module clock
> has already been disabled, leading to the crash.
> 
> One reproducer is to configure s2ram to use "s2idle" instead of "deep"
> suspend:
> 
> # echo 0 > /sys/module/printk/parameters/console_suspend
> # echo s2idle > /sys/power/mem_sleep
> # echo mem > /sys/power/state
> 
> Another reproducer is to write either "platform" or "processors" to
> /sys/power/pm_test.  It does not (or is less likely) to happen during
> full system suspend ("core" or "none") because system suspend also
> disables timers, and thus the workqueue handling PME scans no longer
> runs.  Geert believes the issue may still happen in the small window
> between disabling module clocks and disabling timers:
> 
> # echo 0 > /sys/module/printk/parameters/console_suspend
> # echo platform > /sys/power/pm_test    # Or "processors"
> # echo mem > /sys/power/state
> 
> (Make sure CONFIG_PCI_RCAR_GEN2 and CONFIG_USB_OHCI_HCD_PCI are
> enabled.)
> 
> Rafael Wysocki agrees that PME scans should be suspended before the host
> bridge registers become inaccessible.  To that end, queue the task on a
> workqueue that gets frozen before devices suspend.
> 
> Rafael notes however that as a result, some wakeup events may be missed
> if they are delivered via PME from a device without working IRQ (which
> hence must be polled) and occur after the workqueue has been frozen.
> If that turns out to be an issue in practice, it may be possible to
> solve it by calling pci_pme_list_scan() once directly from one of the
> host bridge's pm_ops callbacks.
> 
> Stacktrace for posterity:
> 
> PM: Syncing filesystems ... [   38.566237] done.
> PM: Preparing system for sleep (mem)
> Freezing user space processes ... [   38.579813] (elapsed 0.001 seconds) done.
> Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
> PM: Suspending system (mem)
> PM: suspend of devices complete after 152.456 msecs
> PM: late suspend of devices complete after 2.809 msecs
> PM: noirq suspend of devices complete after 29.863 msecs
> suspend debug: Waiting for 5 second(s).
> Unhandled fault: asynchronous external abort (0x1211) at 0x00000000
> pgd = c0003000
> [00000000] *pgd=80000040004003, *pmd=00000000
> Internal error: : 1211 [#1] SMP ARM
> Modules linked in:
> CPU: 1 PID: 20 Comm: kworker/1:1 Not tainted
> 4.9.0-rc1-koelsch-00011-g68db9bc814362e7f #3383
> Hardware name: Generic R8A7791 (Flattened Device Tree)
> Workqueue: events pci_pme_list_scan
> task: eb56e140 task.stack: eb58e000
> PC is at pci_generic_config_read+0x64/0x6c
> LR is at rcar_pci_cfg_base+0x64/0x84
> pc : [<c041d7b4>]    lr : [<c04309a0>]    psr: 600d0093
> sp : eb58fe98  ip : c041d750  fp : 00000008
> r10: c0e2283c  r9 : 00000000  r8 : 600d0013
> r7 : 00000008  r6 : eb58fed6  r5 : 00000002  r4 : eb58feb4
> r3 : 00000000  r2 : 00000044  r1 : 00000008  r0 : 00000000
> Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment user
> Control: 30c5387d  Table: 6a9f6c80  DAC: 55555555
> Process kworker/1:1 (pid: 20, stack limit = 0xeb58e210)
> Stack: (0xeb58fe98 to 0xeb590000)
> fe80:                                                       00000002 00000044
> fea0: eb6f5800 c041d9b0 eb58feb4 00000008 00000044 00000000 eb78a000 eb78a000
> fec0: 00000044 00000000 eb9aff00 c0424bf0 eb78a000 00000000 eb78a000 c0e22830
> fee0: ea8a6fc0 c0424c5c eaae79c0 c0424ce0 eb55f380 c0e22838 eb9a9800 c0235fbc
> ff00: eb55f380 c0e22838 eb55f380 eb9a9800 eb9a9800 eb58e000 eb9a9824 c0e02100
> ff20: eb55f398 c02366c4 eb56e140 eb5631c0 00000000 eb55f380 c023641c 00000000
> ff40: 00000000 00000000 00000000 c023a928 cd105598 00000000 40506a34 eb55f380
> ff60: 00000000 00000000 dead4ead ffffffff ffffffff eb58ff74 eb58ff74 00000000
> ff80: 00000000 dead4ead ffffffff ffffffff eb58ff90 eb58ff90 eb58ffac eb5631c0
> ffa0: c023a844 00000000 00000000 c0206d68 00000000 00000000 00000000 00000000
> ffc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> ffe0: 00000000 00000000 00000000 00000000 00000013 00000000 3a81336c 10ccd1dd
> [<c041d7b4>] (pci_generic_config_read) from [<c041d9b0>]
> (pci_bus_read_config_word+0x58/0x80)
> [<c041d9b0>] (pci_bus_read_config_word) from [<c0424bf0>]
> (pci_check_pme_status+0x34/0x78)
> [<c0424bf0>] (pci_check_pme_status) from [<c0424c5c>] (pci_pme_wakeup+0x28/0x54)
> [<c0424c5c>] (pci_pme_wakeup) from [<c0424ce0>] (pci_pme_list_scan+0x58/0xb4)
> [<c0424ce0>] (pci_pme_list_scan) from [<c0235fbc>]
> (process_one_work+0x1bc/0x308)
> [<c0235fbc>] (process_one_work) from [<c02366c4>] (worker_thread+0x2a8/0x3e0)
> [<c02366c4>] (worker_thread) from [<c023a928>] (kthread+0xe4/0xfc)
> [<c023a928>] (kthread) from [<c0206d68>] (ret_from_fork+0x14/0x2c)
> Code: ea000000 e5903000 f57ff04f e3a00000 (e5843000)
> ---[ end trace 667d43ba3aa9e589 ]---
> 
> Reported-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Reported-and-tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> Cc: Simon Horman <horms+renesas@verge.net.au>
> Cc: Yinghai Lu <yinghai@kernel.org>
> Cc: Matthew Garrett <mjg59@srcf.ucam.org>
> Cc: stable@vger.kernel.org # 2.6.37+
> Fixes: df17e62e5bff ("PCI: Add support for polling PME state on suspended legacy PCI devices")
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Applied to pci/pm for v4.12, thanks, Lukas!

> ---
>  drivers/pci/pci.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index aa55501d2179..c561a9e4916a 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1784,8 +1784,8 @@ static void pci_pme_list_scan(struct work_struct *work)
>  		}
>  	}
>  	if (!list_empty(&pci_pme_list))
> -		schedule_delayed_work(&pci_pme_work,
> -				      msecs_to_jiffies(PME_TIMEOUT));
> +		queue_delayed_work(system_freezable_wq, &pci_pme_work,
> +				   msecs_to_jiffies(PME_TIMEOUT));
>  	mutex_unlock(&pci_pme_list_mutex);
>  }
>  
> @@ -1850,8 +1850,9 @@ void pci_pme_active(struct pci_dev *dev, bool enable)
>  			mutex_lock(&pci_pme_list_mutex);
>  			list_add(&pme_dev->list, &pci_pme_list);
>  			if (list_is_singular(&pci_pme_list))
> -				schedule_delayed_work(&pci_pme_work,
> -						      msecs_to_jiffies(PME_TIMEOUT));
> +				queue_delayed_work(system_freezable_wq,
> +						   &pci_pme_work,
> +						   msecs_to_jiffies(PME_TIMEOUT));
>  			mutex_unlock(&pci_pme_list_mutex);
>  		} else {
>  			mutex_lock(&pci_pme_list_mutex);
> -- 
> 2.11.0
> 

Re: [PATCH] PCI: Freeze PME scan before suspending devices

[Bjorn Helgaas]

On Tue, Apr 18, 2017 at 08:44:30PM +0200, Lukas Wunner wrote:
> Laurent Pinchart reported that the Renesas R-Car H2 Lager board
> (r8a7790) crashes during suspend tests.  Geert Uytterhoeven managed to
> reproduce the issue on an M2-W Koelsch board (r8a7791):
> 
> It occurs when the PME scan runs, once per second.  During PME scan, the
> PCI host bridge (rcar-pci) registers are accessed while its module clock
> has already been disabled, leading to the crash.
> 
> One reproducer is to configure s2ram to use "s2idle" instead of "deep"
> suspend:
> 
> # echo 0 > /sys/module/printk/parameters/console_suspend
> # echo s2idle > /sys/power/mem_sleep
> # echo mem > /sys/power/state
> 
> Another reproducer is to write either "platform" or "processors" to
> /sys/power/pm_test.  It does not (or is less likely) to happen during
> full system suspend ("core" or "none") because system suspend also
> disables timers, and thus the workqueue handling PME scans no longer
> runs.  Geert believes the issue may still happen in the small window
> between disabling module clocks and disabling timers:
> 
> # echo 0 > /sys/module/printk/parameters/console_suspend
> # echo platform > /sys/power/pm_test    # Or "processors"
> # echo mem > /sys/power/state
> 
> (Make sure CONFIG_PCI_RCAR_GEN2 and CONFIG_USB_OHCI_HCD_PCI are
> enabled.)
> 
> Rafael Wysocki agrees that PME scans should be suspended before the host
> bridge registers become inaccessible.  To that end, queue the task on a
> workqueue that gets frozen before devices suspend.
> 
> Rafael notes however that as a result, some wakeup events may be missed
> if they are delivered via PME from a device without working IRQ (which
> hence must be polled) and occur after the workqueue has been frozen.
> If that turns out to be an issue in practice, it may be possible to
> solve it by calling pci_pme_list_scan() once directly from one of the
> host bridge's pm_ops callbacks.
> 
> Stacktrace for posterity:
> 
> PM: Syncing filesystems ... [   38.566237] done.
> PM: Preparing system for sleep (mem)
> Freezing user space processes ... [   38.579813] (elapsed 0.001 seconds) done.
> Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
> PM: Suspending system (mem)
> PM: suspend of devices complete after 152.456 msecs
> PM: late suspend of devices complete after 2.809 msecs
> PM: noirq suspend of devices complete after 29.863 msecs
> suspend debug: Waiting for 5 second(s).
> Unhandled fault: asynchronous external abort (0x1211) at 0x00000000
> pgd = c0003000
> [00000000] *pgd=80000040004003, *pmd=00000000
> Internal error: : 1211 [#1] SMP ARM
> Modules linked in:
> CPU: 1 PID: 20 Comm: kworker/1:1 Not tainted
> 4.9.0-rc1-koelsch-00011-g68db9bc814362e7f #3383
> Hardware name: Generic R8A7791 (Flattened Device Tree)
> Workqueue: events pci_pme_list_scan
> task: eb56e140 task.stack: eb58e000
> PC is at pci_generic_config_read+0x64/0x6c
> LR is at rcar_pci_cfg_base+0x64/0x84
> pc : [<c041d7b4>]    lr : [<c04309a0>]    psr: 600d0093
> sp : eb58fe98  ip : c041d750  fp : 00000008
> r10: c0e2283c  r9 : 00000000  r8 : 600d0013
> r7 : 00000008  r6 : eb58fed6  r5 : 00000002  r4 : eb58feb4
> r3 : 00000000  r2 : 00000044  r1 : 00000008  r0 : 00000000
> Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment user
> Control: 30c5387d  Table: 6a9f6c80  DAC: 55555555
> Process kworker/1:1 (pid: 20, stack limit = 0xeb58e210)
> Stack: (0xeb58fe98 to 0xeb590000)
> fe80:                                                       00000002 00000044
> fea0: eb6f5800 c041d9b0 eb58feb4 00000008 00000044 00000000 eb78a000 eb78a000
> fec0: 00000044 00000000 eb9aff00 c0424bf0 eb78a000 00000000 eb78a000 c0e22830
> fee0: ea8a6fc0 c0424c5c eaae79c0 c0424ce0 eb55f380 c0e22838 eb9a9800 c0235fbc
> ff00: eb55f380 c0e22838 eb55f380 eb9a9800 eb9a9800 eb58e000 eb9a9824 c0e02100
> ff20: eb55f398 c02366c4 eb56e140 eb5631c0 00000000 eb55f380 c023641c 00000000
> ff40: 00000000 00000000 00000000 c023a928 cd105598 00000000 40506a34 eb55f380
> ff60: 00000000 00000000 dead4ead ffffffff ffffffff eb58ff74 eb58ff74 00000000
> ff80: 00000000 dead4ead ffffffff ffffffff eb58ff90 eb58ff90 eb58ffac eb5631c0
> ffa0: c023a844 00000000 00000000 c0206d68 00000000 00000000 00000000 00000000
> ffc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> ffe0: 00000000 00000000 00000000 00000000 00000013 00000000 3a81336c 10ccd1dd
> [<c041d7b4>] (pci_generic_config_read) from [<c041d9b0>]
> (pci_bus_read_config_word+0x58/0x80)
> [<c041d9b0>] (pci_bus_read_config_word) from [<c0424bf0>]
> (pci_check_pme_status+0x34/0x78)
> [<c0424bf0>] (pci_check_pme_status) from [<c0424c5c>] (pci_pme_wakeup+0x28/0x54)
> [<c0424c5c>] (pci_pme_wakeup) from [<c0424ce0>] (pci_pme_list_scan+0x58/0xb4)
> [<c0424ce0>] (pci_pme_list_scan) from [<c0235fbc>]
> (process_one_work+0x1bc/0x308)
> [<c0235fbc>] (process_one_work) from [<c02366c4>] (worker_thread+0x2a8/0x3e0)
> [<c02366c4>] (worker_thread) from [<c023a928>] (kthread+0xe4/0xfc)
> [<c023a928>] (kthread) from [<c0206d68>] (ret_from_fork+0x14/0x2c)
> Code: ea000000 e5903000 f57ff04f e3a00000 (e5843000)
> ---[ end trace 667d43ba3aa9e589 ]---
> 
> Reported-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Reported-and-tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Cc: Simon Horman <horms+renesas@verge.net.au>
> Cc: Yinghai Lu <yinghai@kernel.org>
> Cc: Matthew Garrett <mjg59@srcf.ucam.org>
> Cc: stable@vger.kernel.org # 2.6.37+
> Fixes: df17e62e5bff ("PCI: Add support for polling PME state on suspended legacy PCI devices")
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Applied to pci/pm for v4.12, thanks, Lukas!

> ---
>  drivers/pci/pci.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index aa55501d2179..c561a9e4916a 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1784,8 +1784,8 @@ static void pci_pme_list_scan(struct work_struct *work)
>  		}
>  	}
>  	if (!list_empty(&pci_pme_list))
> -		schedule_delayed_work(&pci_pme_work,
> -				      msecs_to_jiffies(PME_TIMEOUT));
> +		queue_delayed_work(system_freezable_wq, &pci_pme_work,
> +				   msecs_to_jiffies(PME_TIMEOUT));
>  	mutex_unlock(&pci_pme_list_mutex);
>  }
>  
> @@ -1850,8 +1850,9 @@ void pci_pme_active(struct pci_dev *dev, bool enable)
>  			mutex_lock(&pci_pme_list_mutex);
>  			list_add(&pme_dev->list, &pci_pme_list);
>  			if (list_is_singular(&pci_pme_list))
> -				schedule_delayed_work(&pci_pme_work,
> -						      msecs_to_jiffies(PME_TIMEOUT));
> +				queue_delayed_work(system_freezable_wq,
> +						   &pci_pme_work,
> +						   msecs_to_jiffies(PME_TIMEOUT));
>  			mutex_unlock(&pci_pme_list_mutex);
>  		} else {
>  			mutex_lock(&pci_pme_list_mutex);
> -- 
> 2.11.0
> 

Re: [PATCH] PCI: Freeze PME scan before suspending devices

[Laurent Pinchart]

Hi Lukas,

Thank you for the patch.

On Tuesday 18 Apr 2017 20:44:30 Lukas Wunner wrote:
> Laurent Pinchart reported that the Renesas R-Car H2 Lager board
> (r8a7790) crashes during suspend tests.  Geert Uytterhoeven managed to
> reproduce the issue on an M2-W Koelsch board (r8a7791):
> 
> It occurs when the PME scan runs, once per second.  During PME scan, the
> PCI host bridge (rcar-pci) registers are accessed while its module clock
> has already been disabled, leading to the crash.
> 
> One reproducer is to configure s2ram to use "s2idle" instead of "deep"
> suspend:
> 
> # echo 0 > /sys/module/printk/parameters/console_suspend
> # echo s2idle > /sys/power/mem_sleep
> # echo mem > /sys/power/state
> 
> Another reproducer is to write either "platform" or "processors" to
> /sys/power/pm_test.  It does not (or is less likely) to happen during
> full system suspend ("core" or "none") because system suspend also
> disables timers, and thus the workqueue handling PME scans no longer
> runs.  Geert believes the issue may still happen in the small window
> between disabling module clocks and disabling timers:
> 
> # echo 0 > /sys/module/printk/parameters/console_suspend
> # echo platform > /sys/power/pm_test    # Or "processors"
> # echo mem > /sys/power/state
> 
> (Make sure CONFIG_PCI_RCAR_GEN2 and CONFIG_USB_OHCI_HCD_PCI are
> enabled.)
> 
> Rafael Wysocki agrees that PME scans should be suspended before the host
> bridge registers become inaccessible.  To that end, queue the task on a
> workqueue that gets frozen before devices suspend.
> 
> Rafael notes however that as a result, some wakeup events may be missed
> if they are delivered via PME from a device without working IRQ (which
> hence must be polled) and occur after the workqueue has been frozen.
> If that turns out to be an issue in practice, it may be possible to
> solve it by calling pci_pme_list_scan() once directly from one of the
> host bridge's pm_ops callbacks.
> 
> Stacktrace for posterity:
> 
> PM: Syncing filesystems ... [   38.566237] done.
> PM: Preparing system for sleep (mem)
> Freezing user space processes ... [   38.579813] (elapsed 0.001 seconds)
> done. Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
> PM: Suspending system (mem)
> PM: suspend of devices complete after 152.456 msecs
> PM: late suspend of devices complete after 2.809 msecs
> PM: noirq suspend of devices complete after 29.863 msecs
> suspend debug: Waiting for 5 second(s).
> Unhandled fault: asynchronous external abort (0x1211) at 0x00000000
> pgd = c0003000
> [00000000] *pgd=80000040004003, *pmd=00000000
> Internal error: : 1211 [#1] SMP ARM
> Modules linked in:
> CPU: 1 PID: 20 Comm: kworker/1:1 Not tainted
> 4.9.0-rc1-koelsch-00011-g68db9bc814362e7f #3383
> Hardware name: Generic R8A7791 (Flattened Device Tree)
> Workqueue: events pci_pme_list_scan
> task: eb56e140 task.stack: eb58e000
> PC is at pci_generic_config_read+0x64/0x6c
> LR is at rcar_pci_cfg_base+0x64/0x84
> pc : [<c041d7b4>]    lr : [<c04309a0>]    psr: 600d0093
> sp : eb58fe98  ip : c041d750  fp : 00000008
> r10: c0e2283c  r9 : 00000000  r8 : 600d0013
> r7 : 00000008  r6 : eb58fed6  r5 : 00000002  r4 : eb58feb4
> r3 : 00000000  r2 : 00000044  r1 : 00000008  r0 : 00000000
> Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment user
> Control: 30c5387d  Table: 6a9f6c80  DAC: 55555555
> Process kworker/1:1 (pid: 20, stack limit = 0xeb58e210)
> Stack: (0xeb58fe98 to 0xeb590000)
> fe80:                                                       00000002
> 00000044 fea0: eb6f5800 c041d9b0 eb58feb4 00000008 00000044 00000000
> eb78a000 eb78a000 fec0: 00000044 00000000 eb9aff00 c0424bf0 eb78a000
> 00000000 eb78a000 c0e22830 fee0: ea8a6fc0 c0424c5c eaae79c0 c0424ce0
> eb55f380 c0e22838 eb9a9800 c0235fbc ff00: eb55f380 c0e22838 eb55f380
> eb9a9800 eb9a9800 eb58e000 eb9a9824 c0e02100 ff20: eb55f398 c02366c4
> eb56e140 eb5631c0 00000000 eb55f380 c023641c 00000000 ff40: 00000000
> 00000000 00000000 c023a928 cd105598 00000000 40506a34 eb55f380 ff60:
> 00000000 00000000 dead4ead ffffffff ffffffff eb58ff74 eb58ff74 00000000
> ff80: 00000000 dead4ead ffffffff ffffffff eb58ff90 eb58ff90 eb58ffac
> eb5631c0 ffa0: c023a844 00000000 00000000 c0206d68 00000000 00000000
> 00000000 00000000 ffc0: 00000000 00000000 00000000 00000000 00000000
> 00000000 00000000 00000000 ffe0: 00000000 00000000 00000000 00000000
> 00000013 00000000 3a81336c 10ccd1dd [<c041d7b4>] (pci_generic_config_read)
> from [<c041d9b0>]
> (pci_bus_read_config_word+0x58/0x80)
> [<c041d9b0>] (pci_bus_read_config_word) from [<c0424bf0>]
> (pci_check_pme_status+0x34/0x78)
> [<c0424bf0>] (pci_check_pme_status) from [<c0424c5c>]
> (pci_pme_wakeup+0x28/0x54) [<c0424c5c>] (pci_pme_wakeup) from [<c0424ce0>]
> (pci_pme_list_scan+0x58/0xb4) [<c0424ce0>] (pci_pme_list_scan) from
> [<c0235fbc>]
> (process_one_work+0x1bc/0x308)
> [<c0235fbc>] (process_one_work) from [<c02366c4>]
> (worker_thread+0x2a8/0x3e0) [<c02366c4>] (worker_thread) from [<c023a928>]
> (kthread+0xe4/0xfc) [<c023a928>] (kthread) from [<c0206d68>]
> (ret_from_fork+0x14/0x2c) Code: ea000000 e5903000 f57ff04f e3a00000
> (e5843000)
> ---[ end trace 667d43ba3aa9e589 ]---
> 
> Reported-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

My Lager board now fails to resume (but that's also the case without this 
patch, I'll have to investigate that separately) but the PCI crash is gone, so

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Reported-and-tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Cc: Simon Horman <horms+renesas@verge.net.au>
> Cc: Yinghai Lu <yinghai@kernel.org>
> Cc: Matthew Garrett <mjg59@srcf.ucam.org>
> Cc: stable@vger.kernel.org # 2.6.37+
> Fixes: df17e62e5bff ("PCI: Add support for polling PME state on suspended
> legacy PCI devices") Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/pci/pci.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index aa55501d2179..c561a9e4916a 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1784,8 +1784,8 @@ static void pci_pme_list_scan(struct work_struct
> *work) }
>  	}
>  	if (!list_empty(&pci_pme_list))
> -		schedule_delayed_work(&pci_pme_work,
> -				      msecs_to_jiffies(PME_TIMEOUT));
> +		queue_delayed_work(system_freezable_wq, &pci_pme_work,
> +				   msecs_to_jiffies(PME_TIMEOUT));
>  	mutex_unlock(&pci_pme_list_mutex);
>  }
> 
> @@ -1850,8 +1850,9 @@ void pci_pme_active(struct pci_dev *dev, bool enable)
>  			mutex_lock(&pci_pme_list_mutex);
>  			list_add(&pme_dev->list, &pci_pme_list);
>  			if (list_is_singular(&pci_pme_list))
> -				schedule_delayed_work(&pci_pme_work,
> -						      
msecs_to_jiffies(PME_TIMEOUT));
> +				queue_delayed_work(system_freezable_wq,
> +						   &pci_pme_work,
> +						   
msecs_to_jiffies(PME_TIMEOUT));
>  			mutex_unlock(&pci_pme_list_mutex);
>  		} else {
>  			mutex_lock(&pci_pme_list_mutex);

-- 
Regards,

Laurent Pinchart

Re: [PATCH] PCI: Freeze PME scan before suspending devices

[Laurent Pinchart]

Hi Lukas,

Thank you for the patch.

On Tuesday 18 Apr 2017 20:44:30 Lukas Wunner wrote:
> Laurent Pinchart reported that the Renesas R-Car H2 Lager board
> (r8a7790) crashes during suspend tests.  Geert Uytterhoeven managed t=
o
> reproduce the issue on an M2-W Koelsch board (r8a7791):
>=20
> It occurs when the PME scan runs, once per second.  During PME scan, =
the
> PCI host bridge (rcar-pci) registers are accessed while its module cl=
ock
> has already been disabled, leading to the crash.
>=20
> One reproducer is to configure s2ram to use "s2idle" instead of "deep=
"
> suspend:
>=20
> # echo 0 > /sys/module/printk/parameters/console_suspend
> # echo s2idle > /sys/power/mem_sleep
> # echo mem > /sys/power/state
>=20
> Another reproducer is to write either "platform" or "processors" to
> /sys/power/pm_test.  It does not (or is less likely) to happen during=

> full system suspend ("core" or "none") because system suspend also
> disables timers, and thus the workqueue handling PME scans no longer
> runs.  Geert believes the issue may still happen in the small window
> between disabling module clocks and disabling timers:
>=20
> # echo 0 > /sys/module/printk/parameters/console_suspend
> # echo platform > /sys/power/pm_test    # Or "processors"
> # echo mem > /sys/power/state
>=20
> (Make sure CONFIG_PCI_RCAR_GEN2 and CONFIG_USB_OHCI_HCD_PCI are
> enabled.)
>=20
> Rafael Wysocki agrees that PME scans should be suspended before the h=
ost
> bridge registers become inaccessible.  To that end, queue the task on=
 a
> workqueue that gets frozen before devices suspend.
>=20
> Rafael notes however that as a result, some wakeup events may be miss=
ed
> if they are delivered via PME from a device without working IRQ (whic=
h
> hence must be polled) and occur after the workqueue has been frozen.
> If that turns out to be an issue in practice, it may be possible to
> solve it by calling pci_pme_list_scan() once directly from one of the=

> host bridge's pm_ops callbacks.
>=20
> Stacktrace for posterity:
>=20
> PM: Syncing filesystems ... [   38.566237] done.
> PM: Preparing system for sleep (mem)
> Freezing user space processes ... [   38.579813] (elapsed 0.001 secon=
ds)
> done. Freezing remaining freezable tasks ... (elapsed 0.001 seconds) =
done.
> PM: Suspending system (mem)
> PM: suspend of devices complete after 152.456 msecs
> PM: late suspend of devices complete after 2.809 msecs
> PM: noirq suspend of devices complete after 29.863 msecs
> suspend debug: Waiting for 5 second(s).
> Unhandled fault: asynchronous external abort (0x1211) at 0x00000000
> pgd =3D c0003000
> [00000000] *pgd=3D80000040004003, *pmd=3D00000000
> Internal error: : 1211 [#1] SMP ARM
> Modules linked in:
> CPU: 1 PID: 20 Comm: kworker/1:1 Not tainted
> 4.9.0-rc1-koelsch-00011-g68db9bc814362e7f #3383
> Hardware name: Generic R8A7791 (Flattened Device Tree)
> Workqueue: events pci_pme_list_scan
> task: eb56e140 task.stack: eb58e000
> PC is at pci_generic_config_read+0x64/0x6c
> LR is at rcar_pci_cfg_base+0x64/0x84
> pc : [<c041d7b4>]    lr : [<c04309a0>]    psr: 600d0093
> sp : eb58fe98  ip : c041d750  fp : 00000008
> r10: c0e2283c  r9 : 00000000  r8 : 600d0013
> r7 : 00000008  r6 : eb58fed6  r5 : 00000002  r4 : eb58feb4
> r3 : 00000000  r2 : 00000044  r1 : 00000008  r0 : 00000000
> Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment user
> Control: 30c5387d  Table: 6a9f6c80  DAC: 55555555
> Process kworker/1:1 (pid: 20, stack limit =3D 0xeb58e210)
> Stack: (0xeb58fe98 to 0xeb590000)
> fe80:                                                       00000002
> 00000044 fea0: eb6f5800 c041d9b0 eb58feb4 00000008 00000044 00000000
> eb78a000 eb78a000 fec0: 00000044 00000000 eb9aff00 c0424bf0 eb78a000
> 00000000 eb78a000 c0e22830 fee0: ea8a6fc0 c0424c5c eaae79c0 c0424ce0
> eb55f380 c0e22838 eb9a9800 c0235fbc ff00: eb55f380 c0e22838 eb55f380
> eb9a9800 eb9a9800 eb58e000 eb9a9824 c0e02100 ff20: eb55f398 c02366c4
> eb56e140 eb5631c0 00000000 eb55f380 c023641c 00000000 ff40: 00000000
> 00000000 00000000 c023a928 cd105598 00000000 40506a34 eb55f380 ff60:
> 00000000 00000000 dead4ead ffffffff ffffffff eb58ff74 eb58ff74 000000=
00
> ff80: 00000000 dead4ead ffffffff ffffffff eb58ff90 eb58ff90 eb58ffac
> eb5631c0 ffa0: c023a844 00000000 00000000 c0206d68 00000000 00000000
> 00000000 00000000 ffc0: 00000000 00000000 00000000 00000000 00000000
> 00000000 00000000 00000000 ffe0: 00000000 00000000 00000000 00000000
> 00000013 00000000 3a81336c 10ccd1dd [<c041d7b4>] (pci_generic_config_=
read)
> from [<c041d9b0>]
> (pci_bus_read_config_word+0x58/0x80)
> [<c041d9b0>] (pci_bus_read_config_word) from [<c0424bf0>]
> (pci_check_pme_status+0x34/0x78)
> [<c0424bf0>] (pci_check_pme_status) from [<c0424c5c>]
> (pci_pme_wakeup+0x28/0x54) [<c0424c5c>] (pci_pme_wakeup) from [<c0424=
ce0>]
> (pci_pme_list_scan+0x58/0xb4) [<c0424ce0>] (pci_pme_list_scan) from
> [<c0235fbc>]
> (process_one_work+0x1bc/0x308)
> [<c0235fbc>] (process_one_work) from [<c02366c4>]
> (worker_thread+0x2a8/0x3e0) [<c02366c4>] (worker_thread) from [<c023a=
928>]
> (kthread+0xe4/0xfc) [<c023a928>] (kthread) from [<c0206d68>]
> (ret_from_fork+0x14/0x2c) Code: ea000000 e5903000 f57ff04f e3a00000
> (e5843000)
> ---[ end trace 667d43ba3aa9e589 ]---
>=20
> Reported-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.=
com>

My Lager board now fails to resume (but that's also the case without th=
is=20
patch, I'll have to investigate that separately) but the PCI crash is g=
one, so

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Reported-and-tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Niklas S=F6derlund <niklas.soderlund+renesas@ragnatech.se>
> Cc: Simon Horman <horms+renesas@verge.net.au>
> Cc: Yinghai Lu <yinghai@kernel.org>
> Cc: Matthew Garrett <mjg59@srcf.ucam.org>
> Cc: stable@vger.kernel.org # 2.6.37+
> Fixes: df17e62e5bff ("PCI: Add support for polling PME state on suspe=
nded
> legacy PCI devices") Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/pci/pci.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>=20
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index aa55501d2179..c561a9e4916a 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1784,8 +1784,8 @@ static void pci_pme_list_scan(struct work_struc=
t
> *work) }
>  =09}
>  =09if (!list_empty(&pci_pme_list))
> -=09=09schedule_delayed_work(&pci_pme_work,
> -=09=09=09=09      msecs_to_jiffies(PME_TIMEOUT));
> +=09=09queue_delayed_work(system_freezable_wq, &pci_pme_work,
> +=09=09=09=09   msecs_to_jiffies(PME_TIMEOUT));
>  =09mutex_unlock(&pci_pme_list_mutex);
>  }
>=20
> @@ -1850,8 +1850,9 @@ void pci_pme_active(struct pci_dev *dev, bool e=
nable)
>  =09=09=09mutex_lock(&pci_pme_list_mutex);
>  =09=09=09list_add(&pme_dev->list, &pci_pme_list);
>  =09=09=09if (list_is_singular(&pci_pme_list))
> -=09=09=09=09schedule_delayed_work(&pci_pme_work,
> -=09=09=09=09=09=09     =20
msecs_to_jiffies(PME_TIMEOUT));
> +=09=09=09=09queue_delayed_work(system_freezable_wq,
> +=09=09=09=09=09=09   &pci_pme_work,
> +=09=09=09=09=09=09  =20
msecs_to_jiffies(PME_TIMEOUT));
>  =09=09=09mutex_unlock(&pci_pme_list_mutex);
>  =09=09} else {
>  =09=09=09mutex_lock(&pci_pme_list_mutex);

--=20
Regards,

Laurent Pinchart

Re: [PATCH] PCI: Freeze PME scan before suspending devices

[Geert Uytterhoeven]

Hi Laurent,

On Wed, Apr 19, 2017 at 12:05 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Tuesday 18 Apr 2017 20:44:30 Lukas Wunner wrote:
>> Laurent Pinchart reported that the Renesas R-Car H2 Lager board
>> (r8a7790) crashes during suspend tests.  Geert Uytterhoeven managed to
>> reproduce the issue on an M2-W Koelsch board (r8a7791):

[...]

>> Reported-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
> My Lager board now fails to resume (but that's also the case without this
> patch, I'll have to investigate that separately) but the PCI crash is gone, so

"mm: move pcp and lru-pcp draining into single wq" broke resume from s2ram
(https://lkml.org/lkml/2017/4/18/614)

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks for double checking!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] PCI: Freeze PME scan before suspending devices

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 702 bytes --]

On Tue, Apr 18, 2017 at 08:44:30PM +0200, Lukas Wunner wrote:
> Laurent Pinchart reported that the Renesas R-Car H2 Lager board
> (r8a7790) crashes during suspend tests.  Geert Uytterhoeven managed to
> reproduce the issue on an M2-W Koelsch board (r8a7791):
> 
> It occurs when the PME scan runs, once per second.  During PME scan, the
> PCI host bridge (rcar-pci) registers are accessed while its module clock
> has already been disabled, leading to the crash.

Cool! I stumbled over this while testing I2C suspend behaviour the last
days, and I can confirm that the crash is gone with this patch on my
Lager board. Thanks!

Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]