On 03/27/20 12:06, Thomas Gleixner wrote:
> Boqun Feng <boqun.feng@xxxxxxxxx> writes:
> > From the commit message, it makes sense to add the pm_wakeup_pending()
> > check if freeze_secondary_cpus() is used for system suspend. However,
> > freeze_secondary_cpus() is also used in kexec path on arm64:
>
> Bah!
>
> > kernel_kexec():
> > machine_shutdown():
> > disable_nonboot_cpus():
> > freeze_secondary_cpus()
> >
> > , so I wonder whether the pm_wakeup_pending() makes sense in this
> > situation? Because IIUC, in this case we want to reboot the system
> > regardlessly, the pm_wakeup_pending() checking seems to be inappropriate
> > then.
>
> Fix below.
>
> Thanks,
>
> tglx
>
> 8<------------
>
> --- a/include/linux/cpu.h
> +++ b/include/linux/cpu.h
> @@ -133,12 +133,18 @@ static inline void get_online_cpus(void)
> static inline void put_online_cpus(void) { cpus_read_unlock(); }
>
> #ifdef CONFIG_PM_SLEEP_SMP
> -extern int freeze_secondary_cpus(int primary);
> +int __freeze_secondary_cpus(int primary, bool suspend);
> +static inline int freeze_secondary_cpus(int primary)
> +{
> + return __freeze_secondary_cpus(primary, true);
> +}
> +
> static inline int disable_nonboot_cpus(void)
> {
> - return freeze_secondary_cpus(0);
> + return __freeze_secondary_cpus(0, false);
> }
If I read the code correctly, arch/x86/power/cpu.c is calling
disable_nonboot_cpus() from suspend resume, which is the only user in
tip/smp/core after my series.
This means you won't abort a suspend/hibernate if a late wakeup source happens?
Or it might just mean that we'll wakeup slightly later than we would have done.
Anyways. I think it would be better to kill off disable_nonboot_cpus() and
directly call freeze_nonboot_cpus() in x86/power/cpu.c.
I'd be happy to send a patch for this.
Assuming that x86 is okay with the late(r) abort, this patch could stay as-is
for stable trees. Otherwise, maybe we need to revert this and look for another
option for stable trees?
Thanks
--
Qais Yousef
> -extern void enable_nonboot_cpus(void);
> +
> +void enable_nonboot_cpus(void);
>
> static inline int suspend_disable_secondary_cpus(void)
> {
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -1200,7 +1200,7 @@ EXPORT_SYMBOL_GPL(cpu_up);
> #ifdef CONFIG_PM_SLEEP_SMP
> static cpumask_var_t frozen_cpus;
>
> -int freeze_secondary_cpus(int primary)
> +int __freeze_secondary_cpus(int primary, bool suspend)
> {
> int cpu, error = 0;
>
> @@ -1225,7 +1225,7 @@ int freeze_secondary_cpus(int primary)
> if (cpu == primary)
> continue;
>
> - if (pm_wakeup_pending()) {
> + if (suspend && pm_wakeup_pending()) {
> pr_info("Wakeup pending. Abort CPU freeze\n");
> error = -EBUSY;
> break;
|