Re: [PATCH] cpufreq: Fix RCU reboot regression on x86 PIC machines

[Ville Syrjälä]

On Thu, Oct 03, 2019 at 05:05:28PM +0200, Rafael J. Wysocki wrote:
> On Thursday, October 3, 2019 4:08:28 PM CEST Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > 
> > Since 4.20-rc1 my PIC machines no longer reboot/shutdown.
> > I bisected this down to commit 45975c7d21a1 ("rcu: Define RCU-sched
> > API in terms of RCU for Tree RCU PREEMPT builds").
> > 
> > I traced the hang into
> > -> cpufreq_suspend()
> >  -> cpufreq_stop_governor()
> >   -> cpufreq_dbs_governor_stop()
> >    -> gov_clear_update_util()
> >     -> synchronize_sched()
> >      -> synchronize_rcu()
> > 
> > Only PREEMPT=y is affected for obvious reasons. The problem
> > is limited to PIC machines since they mask off interrupts
> > in i8259A_shutdown() (syscore_ops.shutdown() registered
> > from device_initcall()).
> 
> Let me treat this as a fresh bug report. :-)
> 
> > I reported this long ago but no better fix has surfaced,
> 
> So I don't recall seeing the original report or if I did, I had not understood
> the problem then.
> 
> > hence sending out my initial workaround which I've been
> > carrying around ever since. I just move cpufreq_core_init()
> > to late_initcall() so the syscore_ops get registered in the
> > oppsite order and thus the .shutdown() hooks get executed
> > in the opposite order as well. Not 100% convinced this is
> > safe (especially moving the cpufreq_global_kobject creation
> > to late_initcall()) but I've not had any problems with it
> > at least.
> 
> The problem is a bug in cpufreq that shouldn't point its syscore shutdown
> callback pointer to cpufreq_suspend(), because the syscore stage is generally
> too lat to call that function and I'm not sure why this has not been causing
> any other issues to trigger (or maybe it did, but they were not reported).
> 
> Does the patch below work for you?

It does. Thanks.

Feel free to slap on
Tested-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>

if you want to go with that.

> 
> ---
>  drivers/base/core.c       |    3 +++
>  drivers/cpufreq/cpufreq.c |   10 ----------
>  2 files changed, 3 insertions(+), 10 deletions(-)
> 
> Index: linux-pm/drivers/cpufreq/cpufreq.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> +++ linux-pm/drivers/cpufreq/cpufreq.c
> @@ -2737,14 +2737,6 @@ int cpufreq_unregister_driver(struct cpu
>  }
>  EXPORT_SYMBOL_GPL(cpufreq_unregister_driver);
>  
> -/*
> - * Stop cpufreq at shutdown to make sure it isn't holding any locks
> - * or mutexes when secondary CPUs are halted.
> - */
> -static struct syscore_ops cpufreq_syscore_ops = {
> -	.shutdown = cpufreq_suspend,
> -};
> -
>  struct kobject *cpufreq_global_kobject;
>  EXPORT_SYMBOL(cpufreq_global_kobject);
>  
> @@ -2756,8 +2748,6 @@ static int __init cpufreq_core_init(void
>  	cpufreq_global_kobject = kobject_create_and_add("cpufreq", &cpu_subsys.dev_root->kobj);
>  	BUG_ON(!cpufreq_global_kobject);
>  
> -	register_syscore_ops(&cpufreq_syscore_ops);
> -
>  	return 0;
>  }
>  module_param(off, int, 0444);
> Index: linux-pm/drivers/base/core.c
> ===================================================================
> --- linux-pm.orig/drivers/base/core.c
> +++ linux-pm/drivers/base/core.c
> @@ -9,6 +9,7 @@
>   */
>  
>  #include <linux/acpi.h>
> +#include <linux/cpufreq.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
>  #include <linux/fwnode.h>
> @@ -3179,6 +3180,8 @@ void device_shutdown(void)
>  	wait_for_device_probe();
>  	device_block_probing();
>  
> +	cpufreq_suspend();
> +
>  	spin_lock(&devices_kset->list_lock);
>  	/*
>  	 * Walk the devices list backward, shutting down each in turn.
> 
> 

-- 
Ville Syrjälä
Intel