| Message ID | 20200223192942.18420-5-qais.yousef@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,01/15] cpu: Add new {add,remove}_cpu() functions | expand |
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. > > Signed-off-by: Qais Yousef <qais.yousef@arm.com> > CC: Russell King <linux@armlinux.org.uk> > CC: linux-arm-kernel@lists.infradead.org > CC: linux-kernel@vger.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 >
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@arm.com> > > CC: Russell King <linux@armlinux.org.uk> > > CC: linux-arm-kernel@lists.infradead.org > > CC: linux-kernel@vger.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 > > >
On 03/20/20 13:07, Russell King - ARM Linux admin wrote: > 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). Okay will do. Thanks -- Qais Yousef > > > > > > > Signed-off-by: Qais Yousef <qais.yousef@arm.com> > > > CC: Russell King <linux@armlinux.org.uk> > > > CC: linux-arm-kernel@lists.infradead.org > > > CC: linux-kernel@vger.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
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); } /*
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. Signed-off-by: Qais Yousef <qais.yousef@arm.com> CC: Russell King <linux@armlinux.org.uk> CC: linux-arm-kernel@lists.infradead.org CC: linux-kernel@vger.kernel.org --- arch/arm/kernel/reboot.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)