diff mbox series

drivers/firmware: psci_checker: Park kthreads before stopping them

Message ID 20190424174728.13826-1-jean-philippe.brucker@arm.com (mailing list archive)
State New, archived
Headers show
Series drivers/firmware: psci_checker: Park kthreads before stopping them | expand

Commit Message

Jean-Philippe Brucker April 24, 2019, 5:47 p.m. UTC 
Since commit 85f1abe0019f ("kthread, sched/wait: Fix kthread_parkme()
completion issue"), kthreads that are bound to a CPU must be parked
before being stopped. At the moment the PSCI checker calls
kthread_stop() directly on the suspend kthread, which triggers the
following warning:

[    6.068288] WARNING: CPU: 1 PID: 1 at kernel/kthread.c:398 __kthread_bind_mask+0x20/0x78
               ...
[    6.190151] Call trace:
[    6.192566]  __kthread_bind_mask+0x20/0x78
[    6.196615]  kthread_unpark+0x74/0x80
[    6.200235]  kthread_stop+0x44/0x1d8
[    6.203769]  psci_checker+0x3bc/0x484
[    6.207389]  do_one_initcall+0x48/0x260
[    6.211180]  kernel_init_freeable+0x2c8/0x368
[    6.215488]  kernel_init+0x10/0x100
[    6.218935]  ret_from_fork+0x10/0x1c
[    6.222467] ---[ end trace e05e22863d043cd3 ]---

kthread_unpark() tries to bind the thread to its CPU and aborts with a
WARN() if the thread wasn't in TASK_PARKED state. Park the kthreads
before stopping them.

Fixes: 85f1abe0019f ("kthread, sched/wait: Fix kthread_parkme() completion issue")
Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
---
See also 060586324648 ("io_uring: park SQPOLL thread if it's percpu")
for a similar fix.
---
 drivers/firmware/psci_checker.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

Sudeep Holla April 25, 2019, 9:03 a.m. UTC | #1 
Hi Jean-Philippe,

Thanks for fixing this, I did see this issue when I was working on
commit 7401056de5f8 ("drivers/firmware: psci_checker: stash and use
topology_core_cpumask for hotplug tests") but was not consistent.

On Wed, Apr 24, 2019 at 06:47:28PM +0100, Jean-Philippe Brucker wrote:
> Since commit 85f1abe0019f ("kthread, sched/wait: Fix kthread_parkme()
> completion issue"), kthreads that are bound to a CPU must be parked
> before being stopped. At the moment the PSCI checker calls
> kthread_stop() directly on the suspend kthread, which triggers the
> following warning:
> 
> [    6.068288] WARNING: CPU: 1 PID: 1 at kernel/kthread.c:398 __kthread_bind_mask+0x20/0x78
>                ...
> [    6.190151] Call trace:
> [    6.192566]  __kthread_bind_mask+0x20/0x78
> [    6.196615]  kthread_unpark+0x74/0x80
> [    6.200235]  kthread_stop+0x44/0x1d8
> [    6.203769]  psci_checker+0x3bc/0x484
> [    6.207389]  do_one_initcall+0x48/0x260
> [    6.211180]  kernel_init_freeable+0x2c8/0x368
> [    6.215488]  kernel_init+0x10/0x100
> [    6.218935]  ret_from_fork+0x10/0x1c
> [    6.222467] ---[ end trace e05e22863d043cd3 ]---
> 
> kthread_unpark() tries to bind the thread to its CPU and aborts with a
> WARN() if the thread wasn't in TASK_PARKED state. Park the kthreads
> before stopping them.
> 
> Fixes: 85f1abe0019f ("kthread, sched/wait: Fix kthread_parkme() completion issue")

Now that this file along with psci.c is move into drivers/firmware/psci/
you may have to repost. The changes are in linux-next and may land
upstream via linux-pm tree. I assume we can get this merged as fix after
merge window ?

FWIW,

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

> Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> ---
> See also 060586324648 ("io_uring: park SQPOLL thread if it's percpu")
> for a similar fix.
> ---
>  drivers/firmware/psci_checker.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/firmware/psci_checker.c b/drivers/firmware/psci_checker.c
> index 346943657962..cbd53cb1b2d4 100644
> --- a/drivers/firmware/psci_checker.c
> +++ b/drivers/firmware/psci_checker.c
> @@ -366,16 +366,16 @@ static int suspend_test_thread(void *arg)
>  	for (;;) {
>  		/* Needs to be set first to avoid missing a wakeup. */
>  		set_current_state(TASK_INTERRUPTIBLE);
> -		if (kthread_should_stop()) {
> -			__set_current_state(TASK_RUNNING);
> +		if (kthread_should_park())
>  			break;
> -		}
>  		schedule();
>  	}
>  
>  	pr_info("CPU %d suspend test results: success %d, shallow states %d, errors %d\n",
>  		cpu, nb_suspend, nb_shallow_sleep, nb_err);
>  
> +	kthread_parkme();
> +
>  	return nb_err;
>  }
>  
> @@ -440,8 +440,10 @@ static int suspend_tests(void)
>  
>  
>  	/* Stop and destroy all threads, get return status. */
> -	for (i = 0; i < nb_threads; ++i)
> +	for (i = 0; i < nb_threads; ++i) {
> +		err += kthread_park(threads[i]);
>  		err += kthread_stop(threads[i]);
> +	}
>   out:
>  	cpuidle_resume_and_unlock();
>  	kfree(threads);
> -- 
> 2.21.0
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Jean-Philippe Brucker April 25, 2019, 9:38 a.m. UTC | #2 
On 25/04/2019 10:03, Sudeep Holla wrote:
> Hi Jean-Philippe,
> 
> Thanks for fixing this, I did see this issue when I was working on
> commit 7401056de5f8 ("drivers/firmware: psci_checker: stash and use
> topology_core_cpumask for hotplug tests") but was not consistent.
> 
> On Wed, Apr 24, 2019 at 06:47:28PM +0100, Jean-Philippe Brucker wrote:
>> Since commit 85f1abe0019f ("kthread, sched/wait: Fix kthread_parkme()
>> completion issue"), kthreads that are bound to a CPU must be parked
>> before being stopped. At the moment the PSCI checker calls
>> kthread_stop() directly on the suspend kthread, which triggers the
>> following warning:
>>
>> [    6.068288] WARNING: CPU: 1 PID: 1 at kernel/kthread.c:398 __kthread_bind_mask+0x20/0x78
>>                ...
>> [    6.190151] Call trace:
>> [    6.192566]  __kthread_bind_mask+0x20/0x78
>> [    6.196615]  kthread_unpark+0x74/0x80
>> [    6.200235]  kthread_stop+0x44/0x1d8
>> [    6.203769]  psci_checker+0x3bc/0x484
>> [    6.207389]  do_one_initcall+0x48/0x260
>> [    6.211180]  kernel_init_freeable+0x2c8/0x368
>> [    6.215488]  kernel_init+0x10/0x100
>> [    6.218935]  ret_from_fork+0x10/0x1c
>> [    6.222467] ---[ end trace e05e22863d043cd3 ]---
>>
>> kthread_unpark() tries to bind the thread to its CPU and aborts with a
>> WARN() if the thread wasn't in TASK_PARKED state. Park the kthreads
>> before stopping them.
>>
>> Fixes: 85f1abe0019f ("kthread, sched/wait: Fix kthread_parkme() completion issue")
> 
> Now that this file along with psci.c is move into drivers/firmware/psci/
> you may have to repost. The changes are in linux-next and may land
> upstream via linux-pm tree. I assume we can get this merged as fix after
> merge window ?

Ah right, I didn't notice the file was moving. The fix doesn't seem too
urgent so I'll resend after the merge window

> 
> FWIW,
> 
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

Thanks!
Jean
Lorenzo Pieralisi April 25, 2019, 9:53 a.m. UTC | #3 
On Thu, Apr 25, 2019 at 10:38:50AM +0100, Jean-Philippe Brucker wrote:
> On 25/04/2019 10:03, Sudeep Holla wrote:
> > Hi Jean-Philippe,
> > 
> > Thanks for fixing this, I did see this issue when I was working on
> > commit 7401056de5f8 ("drivers/firmware: psci_checker: stash and use
> > topology_core_cpumask for hotplug tests") but was not consistent.
> > 
> > On Wed, Apr 24, 2019 at 06:47:28PM +0100, Jean-Philippe Brucker wrote:
> >> Since commit 85f1abe0019f ("kthread, sched/wait: Fix kthread_parkme()
> >> completion issue"), kthreads that are bound to a CPU must be parked
> >> before being stopped. At the moment the PSCI checker calls
> >> kthread_stop() directly on the suspend kthread, which triggers the
> >> following warning:
> >>
> >> [    6.068288] WARNING: CPU: 1 PID: 1 at kernel/kthread.c:398 __kthread_bind_mask+0x20/0x78
> >>                ...
> >> [    6.190151] Call trace:
> >> [    6.192566]  __kthread_bind_mask+0x20/0x78
> >> [    6.196615]  kthread_unpark+0x74/0x80
> >> [    6.200235]  kthread_stop+0x44/0x1d8
> >> [    6.203769]  psci_checker+0x3bc/0x484
> >> [    6.207389]  do_one_initcall+0x48/0x260
> >> [    6.211180]  kernel_init_freeable+0x2c8/0x368
> >> [    6.215488]  kernel_init+0x10/0x100
> >> [    6.218935]  ret_from_fork+0x10/0x1c
> >> [    6.222467] ---[ end trace e05e22863d043cd3 ]---
> >>
> >> kthread_unpark() tries to bind the thread to its CPU and aborts with a
> >> WARN() if the thread wasn't in TASK_PARKED state. Park the kthreads
> >> before stopping them.
> >>
> >> Fixes: 85f1abe0019f ("kthread, sched/wait: Fix kthread_parkme() completion issue")
> > 
> > Now that this file along with psci.c is move into drivers/firmware/psci/
> > you may have to repost. The changes are in linux-next and may land
> > upstream via linux-pm tree. I assume we can get this merged as fix after
> > merge window ?
> 
> Ah right, I didn't notice the file was moving. The fix doesn't seem too
> urgent so I'll resend after the merge window

Thanks, I will pick it up and send it to arm-soc then.

Lorenzo
diff mbox series

Patch

diff --git a/drivers/firmware/psci_checker.c b/drivers/firmware/psci_checker.c
index 346943657962..cbd53cb1b2d4 100644
--- a/drivers/firmware/psci_checker.c
+++ b/drivers/firmware/psci_checker.c
@@ -366,16 +366,16 @@  static int suspend_test_thread(void *arg)
 	for (;;) {
 		/* Needs to be set first to avoid missing a wakeup. */
 		set_current_state(TASK_INTERRUPTIBLE);
-		if (kthread_should_stop()) {
-			__set_current_state(TASK_RUNNING);
+		if (kthread_should_park())
 			break;
-		}
 		schedule();
 	}
 
 	pr_info("CPU %d suspend test results: success %d, shallow states %d, errors %d\n",
 		cpu, nb_suspend, nb_shallow_sleep, nb_err);
 
+	kthread_parkme();
+
 	return nb_err;
 }
 
@@ -440,8 +440,10 @@  static int suspend_tests(void)
 
 
 	/* Stop and destroy all threads, get return status. */
-	for (i = 0; i < nb_threads; ++i)
+	for (i = 0; i < nb_threads; ++i) {
+		err += kthread_park(threads[i]);
 		err += kthread_stop(threads[i]);
+	}
  out:
 	cpuidle_resume_and_unlock();
 	kfree(threads);