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;
![]() |