linux-kernel - Re: [PATCH v3 04/15] arm: Don't use disable_nonboot

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200320130700.GR25745@shell.armlinux.org.uk>
Date:   Fri, 20 Mar 2020 13:07:01 +0000
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Qais Yousef <qais.yousef@....com>
Cc:     "Paul E . McKenney" <paulmck@...nel.org>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH v3 04/15] arm: Don't use disable_nonboot_cpus()

On Fri, Mar 20, 2020 at 11:04:31AM +0000, Qais Yousef wrote:
> On 02/23/20 19:29, Qais Yousef wrote:
> > disable_nonboot_cpus() is not safe to use when doing machine_down(),
> > because it relies on freeze_secondary_cpus() which in turn is
> > a suspend/resume related freeze and could abort if the logic detects any
> > pending activities that can prevent finishing the offlining process.
> > 
> > Beside disable_nonboot_cpus() is dependent on CONFIG_PM_SLEEP_SMP which
> > is an othogonal config to rely on to ensure this function works
> > correctly.
> > 
> > Use `reboot_cpu` variable instead of hardcoding 0 as the reboot cpu.

I think that should be a separate change - you have two separate
changes in this patch:

1. to switch to using the new helper.
2. to change the CPU that we potentially use for the final steps of
   shutdown

These should be two seperate changes, so if (2) causes a regression
it can be reverted independently of (1).

> > 
> > Signed-off-by: Qais Yousef <qais.yousef@....com>
> > CC: Russell King <linux@...linux.org.uk>
> > CC: linux-arm-kernel@...ts.infradead.org
> > CC: linux-kernel@...r.kernel.org
> > ---
> 
> Hi Russel
> 
> Does the updated version look good to you now?
> 
> Thanks
> 
> --
> Qais Yousef
> 
> >  arch/arm/kernel/reboot.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm/kernel/reboot.c b/arch/arm/kernel/reboot.c
> > index bb18ed0539f4..0ce388f15422 100644
> > --- a/arch/arm/kernel/reboot.c
> > +++ b/arch/arm/kernel/reboot.c
> > @@ -88,11 +88,11 @@ void soft_restart(unsigned long addr)
> >   * to execute e.g. a RAM-based pin loop is not sufficient. This allows the
> >   * kexec'd kernel to use any and all RAM as it sees fit, without having to
> >   * avoid any code or data used by any SW CPU pin loop. The CPU hotplug
> > - * functionality embodied in disable_nonboot_cpus() to achieve this.
> > + * functionality embodied in smp_shutdown_nonboot_cpus() to achieve this.
> >   */
> >  void machine_shutdown(void)
> >  {
> > -	disable_nonboot_cpus();
> > +	smp_shutdown_nonboot_cpus(reboot_cpu);
> >  }
> >  
> >  /*
> > -- 
> > 2.17.1
> > 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up