| Message ID | 20210406125047.547501-1-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | ACPI: processor: Fix build when CONFIG_ACPI_PROCESSOR=m | expand |
On Tue, Apr 6, 2021 at 2:50 PM Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > > Commit 8cdddd182bd7 ("ACPI: processor: Fix CPU0 wakeup in > acpi_idle_play_dead()") tried to fix CPU0 hotplug breakage by copying > wakeup_cpu0() + start_cpu0() logic from hlt_play_dead()//mwait_play_dead() > into acpi_idle_play_dead(). The problem is that these functions are not > exported to modules so when CONFIG_ACPI_PROCESSOR=m build fails. > > The issue could've been fixed by exporting both wakeup_cpu0()/start_cpu0() > (the later from assembly) but it seems putting the whole pattern into a > new function and exporting it instead is better. > > Reported-by: kernel test robot <lkp@intel.com> > Fixes: 8cdddd182bd7 ("CPI: processor: Fix CPU0 wakeup in acpi_idle_play_dead()") > Cc: <stable@vger.kernel.org> # 5.10+ > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > --- > arch/x86/include/asm/smp.h | 2 +- > arch/x86/kernel/smpboot.c | 15 ++++++++++----- > drivers/acpi/processor_idle.c | 3 +-- > 3 files changed, 12 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h > index 57ef2094af93..6f79deb1f970 100644 > --- a/arch/x86/include/asm/smp.h > +++ b/arch/x86/include/asm/smp.h > @@ -132,7 +132,7 @@ void native_play_dead(void); > void play_dead_common(void); > void wbinvd_on_cpu(int cpu); > int wbinvd_on_all_cpus(void); > -bool wakeup_cpu0(void); > +void wakeup_cpu0_if_needed(void); > > void native_smp_send_reschedule(int cpu); > void native_send_call_func_ipi(const struct cpumask *mask); > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c > index f877150a91da..9547d870ee27 100644 > --- a/arch/x86/kernel/smpboot.c > +++ b/arch/x86/kernel/smpboot.c > @@ -1659,7 +1659,7 @@ void play_dead_common(void) > local_irq_disable(); > } > > -bool wakeup_cpu0(void) > +static bool wakeup_cpu0(void) > { > if (smp_processor_id() == 0 && enable_start_cpu0) > return true; > @@ -1667,6 +1667,13 @@ bool wakeup_cpu0(void) > return false; > } > > +void wakeup_cpu0_if_needed(void) > +{ > + if (wakeup_cpu0()) > + start_cpu0(); Note that all of the callers of wakeup_cpu0 do the above, so maybe make them all use the new function? In which case it can be rewritten in the following way void cond_wakeup_cpu0(void) { if (smp_processor_id() == 0 && enable_start_cpu0) start_cpu0(); } EXPORT_SYMBOL_GPL(cond_wakeup_cpu0); Also please add a proper kerneldoc comment to it and maybe drop the comments at the call sites? > +} > +EXPORT_SYMBOL_GPL(wakeup_cpu0_if_needed); > + > /* > * We need to flush the caches before going to sleep, lest we have > * dirty data in our caches when we come back up. > @@ -1737,8 +1744,7 @@ static inline void mwait_play_dead(void) > /* > * If NMI wants to wake up CPU0, start CPU0. > */ > - if (wakeup_cpu0()) > - start_cpu0(); > + wakeup_cpu0_if_needed(); > } > } > > @@ -1752,8 +1758,7 @@ void hlt_play_dead(void) > /* > * If NMI wants to wake up CPU0, start CPU0. > */ > - if (wakeup_cpu0()) > - start_cpu0(); > + wakeup_cpu0_if_needed(); > } > } > > diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c > index 768a6b4d2368..de15116b754a 100644 > --- a/drivers/acpi/processor_idle.c > +++ b/drivers/acpi/processor_idle.c > @@ -545,8 +545,7 @@ static int acpi_idle_play_dead(struct cpuidle_device *dev, int index) > > #if defined(CONFIG_X86) && defined(CONFIG_HOTPLUG_CPU) > /* If NMI wants to wake up CPU0, start CPU0. */ > - if (wakeup_cpu0()) > - start_cpu0(); > + wakeup_cpu0_if_needed(); > #endif > } > > -- > 2.30.2 >
"Rafael J. Wysocki" <rafael@kernel.org> writes: > On Tue, Apr 6, 2021 at 2:50 PM Vitaly Kuznetsov <vkuznets@redhat.com> wrote: >> >> Commit 8cdddd182bd7 ("ACPI: processor: Fix CPU0 wakeup in >> acpi_idle_play_dead()") tried to fix CPU0 hotplug breakage by copying >> wakeup_cpu0() + start_cpu0() logic from hlt_play_dead()//mwait_play_dead() >> into acpi_idle_play_dead(). The problem is that these functions are not >> exported to modules so when CONFIG_ACPI_PROCESSOR=m build fails. >> >> The issue could've been fixed by exporting both wakeup_cpu0()/start_cpu0() >> (the later from assembly) but it seems putting the whole pattern into a >> new function and exporting it instead is better. >> >> Reported-by: kernel test robot <lkp@intel.com> >> Fixes: 8cdddd182bd7 ("CPI: processor: Fix CPU0 wakeup in acpi_idle_play_dead()") >> Cc: <stable@vger.kernel.org> # 5.10+ >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> >> --- >> arch/x86/include/asm/smp.h | 2 +- >> arch/x86/kernel/smpboot.c | 15 ++++++++++----- >> drivers/acpi/processor_idle.c | 3 +-- >> 3 files changed, 12 insertions(+), 8 deletions(-) >> >> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h >> index 57ef2094af93..6f79deb1f970 100644 >> --- a/arch/x86/include/asm/smp.h >> +++ b/arch/x86/include/asm/smp.h >> @@ -132,7 +132,7 @@ void native_play_dead(void); >> void play_dead_common(void); >> void wbinvd_on_cpu(int cpu); >> int wbinvd_on_all_cpus(void); >> -bool wakeup_cpu0(void); >> +void wakeup_cpu0_if_needed(void); >> >> void native_smp_send_reschedule(int cpu); >> void native_send_call_func_ipi(const struct cpumask *mask); >> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c >> index f877150a91da..9547d870ee27 100644 >> --- a/arch/x86/kernel/smpboot.c >> +++ b/arch/x86/kernel/smpboot.c >> @@ -1659,7 +1659,7 @@ void play_dead_common(void) >> local_irq_disable(); >> } >> >> -bool wakeup_cpu0(void) >> +static bool wakeup_cpu0(void) >> { >> if (smp_processor_id() == 0 && enable_start_cpu0) >> return true; >> @@ -1667,6 +1667,13 @@ bool wakeup_cpu0(void) >> return false; >> } >> >> +void wakeup_cpu0_if_needed(void) >> +{ >> + if (wakeup_cpu0()) >> + start_cpu0(); > > Note that all of the callers of wakeup_cpu0 do the above, so maybe > make them all use the new function? > > In which case it can be rewritten in the following way > > void cond_wakeup_cpu0(void) > { > if (smp_processor_id() == 0 && enable_start_cpu0) > start_cpu0(); > } > EXPORT_SYMBOL_GPL(cond_wakeup_cpu0); > Sure, separate wakeup_cpu0() is no longer needed. > Also please add a proper kerneldoc comment to it and maybe drop the > comments at the call sites? Also a good idea. v2 is coming, thanks! > > >> +} >> +EXPORT_SYMBOL_GPL(wakeup_cpu0_if_needed); >> + >> /* >> * We need to flush the caches before going to sleep, lest we have >> * dirty data in our caches when we come back up. >> @@ -1737,8 +1744,7 @@ static inline void mwait_play_dead(void) >> /* >> * If NMI wants to wake up CPU0, start CPU0. >> */ >> - if (wakeup_cpu0()) >> - start_cpu0(); >> + wakeup_cpu0_if_needed(); >> } >> } >> >> @@ -1752,8 +1758,7 @@ void hlt_play_dead(void) >> /* >> * If NMI wants to wake up CPU0, start CPU0. >> */ >> - if (wakeup_cpu0()) >> - start_cpu0(); >> + wakeup_cpu0_if_needed(); >> } >> } >> >> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c >> index 768a6b4d2368..de15116b754a 100644 >> --- a/drivers/acpi/processor_idle.c >> +++ b/drivers/acpi/processor_idle.c >> @@ -545,8 +545,7 @@ static int acpi_idle_play_dead(struct cpuidle_device *dev, int index) >> >> #if defined(CONFIG_X86) && defined(CONFIG_HOTPLUG_CPU) >> /* If NMI wants to wake up CPU0, start CPU0. */ >> - if (wakeup_cpu0()) >> - start_cpu0(); >> + wakeup_cpu0_if_needed(); >> #endif >> } >> >> -- >> 2.30.2 >> >
diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h index 57ef2094af93..6f79deb1f970 100644 --- a/arch/x86/include/asm/smp.h +++ b/arch/x86/include/asm/smp.h @@ -132,7 +132,7 @@ void native_play_dead(void); void play_dead_common(void); void wbinvd_on_cpu(int cpu); int wbinvd_on_all_cpus(void); -bool wakeup_cpu0(void); +void wakeup_cpu0_if_needed(void); void native_smp_send_reschedule(int cpu); void native_send_call_func_ipi(const struct cpumask *mask); diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c index f877150a91da..9547d870ee27 100644 --- a/arch/x86/kernel/smpboot.c +++ b/arch/x86/kernel/smpboot.c @@ -1659,7 +1659,7 @@ void play_dead_common(void) local_irq_disable(); } -bool wakeup_cpu0(void) +static bool wakeup_cpu0(void) { if (smp_processor_id() == 0 && enable_start_cpu0) return true; @@ -1667,6 +1667,13 @@ bool wakeup_cpu0(void) return false; } +void wakeup_cpu0_if_needed(void) +{ + if (wakeup_cpu0()) + start_cpu0(); +} +EXPORT_SYMBOL_GPL(wakeup_cpu0_if_needed); + /* * We need to flush the caches before going to sleep, lest we have * dirty data in our caches when we come back up. @@ -1737,8 +1744,7 @@ static inline void mwait_play_dead(void) /* * If NMI wants to wake up CPU0, start CPU0. */ - if (wakeup_cpu0()) - start_cpu0(); + wakeup_cpu0_if_needed(); } } @@ -1752,8 +1758,7 @@ void hlt_play_dead(void) /* * If NMI wants to wake up CPU0, start CPU0. */ - if (wakeup_cpu0()) - start_cpu0(); + wakeup_cpu0_if_needed(); } } diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c index 768a6b4d2368..de15116b754a 100644 --- a/drivers/acpi/processor_idle.c +++ b/drivers/acpi/processor_idle.c @@ -545,8 +545,7 @@ static int acpi_idle_play_dead(struct cpuidle_device *dev, int index) #if defined(CONFIG_X86) && defined(CONFIG_HOTPLUG_CPU) /* If NMI wants to wake up CPU0, start CPU0. */ - if (wakeup_cpu0()) - start_cpu0(); + wakeup_cpu0_if_needed(); #endif }
Commit 8cdddd182bd7 ("ACPI: processor: Fix CPU0 wakeup in acpi_idle_play_dead()") tried to fix CPU0 hotplug breakage by copying wakeup_cpu0() + start_cpu0() logic from hlt_play_dead()//mwait_play_dead() into acpi_idle_play_dead(). The problem is that these functions are not exported to modules so when CONFIG_ACPI_PROCESSOR=m build fails. The issue could've been fixed by exporting both wakeup_cpu0()/start_cpu0() (the later from assembly) but it seems putting the whole pattern into a new function and exporting it instead is better. Reported-by: kernel test robot <lkp@intel.com> Fixes: 8cdddd182bd7 ("CPI: processor: Fix CPU0 wakeup in acpi_idle_play_dead()") Cc: <stable@vger.kernel.org> # 5.10+ Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- arch/x86/include/asm/smp.h | 2 +- arch/x86/kernel/smpboot.c | 15 ++++++++++----- drivers/acpi/processor_idle.c | 3 +-- 3 files changed, 12 insertions(+), 8 deletions(-)