Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 30 Mar 2020 20:40:14 +0100
From:   Qais Yousef <qais.yousef@arm.com>
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     Boqun Feng <boqun.feng@gmail.com>, peterz@infradead.org,
        linux-kernel@vger.kernel.org, len.brown@intel.com,
        pkondeti@codeaurora.org, jpoimboe@redhat.com, pavel@ucw.cz,
        konrad.wilk@oracle.com, mojha@codeaurora.org, jkosina@suse.cz,
        mingo@kernel.org, hpa@zytor.com, rjw@rjwysocki.net,
        linux-tip-commits@vger.kernel.org
Subject: Re: [tip:smp/hotplug] cpu/hotplug: Abort disabling secondary CPUs if
 wakeup is pending
Message-ID: <20200330194014.lwpmbv2zekfk6ywx@e107158-lin.cambridge.arm.com>
References: <1559536263-16472-1-git-send-email-pkondeti@codeaurora.org>
 <tip-a66d955e910ab0e598d7a7450cbe6139f52befe7@git.kernel.org>
 <20200327025311.GA58760@debian-boqun.qqnc3lrjykvubdpftowmye0fmh.lx.internal.cloudapp.net>
 <874kuaxdiz.fsf@nanos.tec.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <874kuaxdiz.fsf@nanos.tec.linutronix.de>
User-Agent: NeoMutt/20171215
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 03/27/20 12:06, Thomas Gleixner wrote:
> Boqun Feng <boqun.feng@gmail.com> 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;