| Message ID | 20210406140005.554402-1-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2] ACPI: processor: Fix build when CONFIG_ACPI_PROCESSOR=m | expand |
On Tue, Apr 6, 2021 at 4:01 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> > --- > Changes since v1: > - Rename wakeup_cpu0() to cond_wakeup_cpu0() and fold wakeup_cpu0() in > as it has no other users [Rafael J. Wysocki] > --- > arch/x86/include/asm/smp.h | 2 +- > arch/x86/kernel/smpboot.c | 24 ++++++++++-------------- > drivers/acpi/processor_idle.c | 4 +--- > 3 files changed, 12 insertions(+), 18 deletions(-) > > diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h > index 57ef2094af93..630ff08532be 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 cond_wakeup_cpu0(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..147f1bba9736 100644 > --- a/arch/x86/kernel/smpboot.c > +++ b/arch/x86/kernel/smpboot.c > @@ -1659,13 +1659,15 @@ void play_dead_common(void) > local_irq_disable(); > } > > -bool wakeup_cpu0(void) > +/* > + * If NMI wants to wake up CPU0, start CPU0. > + */ Hasn't checkpatch.pl complained about this? A proper kerneldoc would be something like: /** * cond_wakeup_cpu0 - Wake up CPU0 if needed. * * If NMI wants to wake up CPU0, start CPU0. */ > +void cond_wakeup_cpu0(void) > { > if (smp_processor_id() == 0 && enable_start_cpu0) > - return true; > - > - return false; > + start_cpu0(); > } > +EXPORT_SYMBOL_GPL(cond_wakeup_cpu0); > > /* > * We need to flush the caches before going to sleep, lest we have > @@ -1734,11 +1736,8 @@ static inline void mwait_play_dead(void) > __monitor(mwait_ptr, 0, 0); > mb(); > __mwait(eax, 0); > - /* > - * If NMI wants to wake up CPU0, start CPU0. > - */ > - if (wakeup_cpu0()) > - start_cpu0(); > + > + cond_wakeup_cpu0(); > } > } > > @@ -1749,11 +1748,8 @@ void hlt_play_dead(void) > > while (1) { > native_halt(); > - /* > - * If NMI wants to wake up CPU0, start CPU0. > - */ > - if (wakeup_cpu0()) > - start_cpu0(); > + > + cond_wakeup_cpu0(); > } > } > > diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c > index 768a6b4d2368..4e2d76b8b697 100644 > --- a/drivers/acpi/processor_idle.c > +++ b/drivers/acpi/processor_idle.c > @@ -544,9 +544,7 @@ static int acpi_idle_play_dead(struct cpuidle_device *dev, int index) > return -ENODEV; > > #if defined(CONFIG_X86) && defined(CONFIG_HOTPLUG_CPU) > - /* If NMI wants to wake up CPU0, start CPU0. */ > - if (wakeup_cpu0()) > - start_cpu0(); > + cond_wakeup_cpu0(); > #endif > } > > -- > 2.30.2 >
"Rafael J. Wysocki" <rafael@kernel.org> writes: > On Tue, Apr 6, 2021 at 4:01 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> >> --- >> Changes since v1: >> - Rename wakeup_cpu0() to cond_wakeup_cpu0() and fold wakeup_cpu0() in >> as it has no other users [Rafael J. Wysocki] >> --- >> arch/x86/include/asm/smp.h | 2 +- >> arch/x86/kernel/smpboot.c | 24 ++++++++++-------------- >> drivers/acpi/processor_idle.c | 4 +--- >> 3 files changed, 12 insertions(+), 18 deletions(-) >> >> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h >> index 57ef2094af93..630ff08532be 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 cond_wakeup_cpu0(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..147f1bba9736 100644 >> --- a/arch/x86/kernel/smpboot.c >> +++ b/arch/x86/kernel/smpboot.c >> @@ -1659,13 +1659,15 @@ void play_dead_common(void) >> local_irq_disable(); >> } >> >> -bool wakeup_cpu0(void) >> +/* >> + * If NMI wants to wake up CPU0, start CPU0. >> + */ > > Hasn't checkpatch.pl complained about this? > No, it didn't. > A proper kerneldoc would be something like: > > /** > * cond_wakeup_cpu0 - Wake up CPU0 if needed. > * > * If NMI wants to wake up CPU0, start CPU0. > */ Yea, I didn't do that partly because of my laziness but partly because I don't see much usage of this format in arch/x86/kernel/[smpboot.c]. I can certainly do v3 if it's prefered. > >> +void cond_wakeup_cpu0(void) >> { >> if (smp_processor_id() == 0 && enable_start_cpu0) >> - return true; >> - >> - return false; >> + start_cpu0(); >> } >> +EXPORT_SYMBOL_GPL(cond_wakeup_cpu0); >> >> /* >> * We need to flush the caches before going to sleep, lest we have >> @@ -1734,11 +1736,8 @@ static inline void mwait_play_dead(void) >> __monitor(mwait_ptr, 0, 0); >> mb(); >> __mwait(eax, 0); >> - /* >> - * If NMI wants to wake up CPU0, start CPU0. >> - */ >> - if (wakeup_cpu0()) >> - start_cpu0(); >> + >> + cond_wakeup_cpu0(); >> } >> } >> >> @@ -1749,11 +1748,8 @@ void hlt_play_dead(void) >> >> while (1) { >> native_halt(); >> - /* >> - * If NMI wants to wake up CPU0, start CPU0. >> - */ >> - if (wakeup_cpu0()) >> - start_cpu0(); >> + >> + cond_wakeup_cpu0(); >> } >> } >> >> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c >> index 768a6b4d2368..4e2d76b8b697 100644 >> --- a/drivers/acpi/processor_idle.c >> +++ b/drivers/acpi/processor_idle.c >> @@ -544,9 +544,7 @@ static int acpi_idle_play_dead(struct cpuidle_device *dev, int index) >> return -ENODEV; >> >> #if defined(CONFIG_X86) && defined(CONFIG_HOTPLUG_CPU) >> - /* If NMI wants to wake up CPU0, start CPU0. */ >> - if (wakeup_cpu0()) >> - start_cpu0(); >> + cond_wakeup_cpu0(); >> #endif >> } >> >> -- >> 2.30.2 >> >
On Tue, Apr 6, 2021 at 5:39 PM Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > > "Rafael J. Wysocki" <rafael@kernel.org> writes: > > > On Tue, Apr 6, 2021 at 4:01 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> > >> --- > >> Changes since v1: > >> - Rename wakeup_cpu0() to cond_wakeup_cpu0() and fold wakeup_cpu0() in > >> as it has no other users [Rafael J. Wysocki] > >> --- > >> arch/x86/include/asm/smp.h | 2 +- > >> arch/x86/kernel/smpboot.c | 24 ++++++++++-------------- > >> drivers/acpi/processor_idle.c | 4 +--- > >> 3 files changed, 12 insertions(+), 18 deletions(-) > >> > >> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h > >> index 57ef2094af93..630ff08532be 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 cond_wakeup_cpu0(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..147f1bba9736 100644 > >> --- a/arch/x86/kernel/smpboot.c > >> +++ b/arch/x86/kernel/smpboot.c > >> @@ -1659,13 +1659,15 @@ void play_dead_common(void) > >> local_irq_disable(); > >> } > >> > >> -bool wakeup_cpu0(void) > >> +/* > >> + * If NMI wants to wake up CPU0, start CPU0. > >> + */ > > > > Hasn't checkpatch.pl complained about this? > > > > No, it didn't. > > > A proper kerneldoc would be something like: > > > > /** > > * cond_wakeup_cpu0 - Wake up CPU0 if needed. > > * > > * If NMI wants to wake up CPU0, start CPU0. > > */ > > Yea, I didn't do that partly because of my laziness but partly because > I don't see much usage of this format in arch/x86/kernel/[smpboot.c]. I > can certainly do v3 if it's prefered. Yes, please. Exported functions generally need proper kerneldoc comments.
diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h index 57ef2094af93..630ff08532be 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 cond_wakeup_cpu0(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..147f1bba9736 100644 --- a/arch/x86/kernel/smpboot.c +++ b/arch/x86/kernel/smpboot.c @@ -1659,13 +1659,15 @@ void play_dead_common(void) local_irq_disable(); } -bool wakeup_cpu0(void) +/* + * If NMI wants to wake up CPU0, start CPU0. + */ +void cond_wakeup_cpu0(void) { if (smp_processor_id() == 0 && enable_start_cpu0) - return true; - - return false; + start_cpu0(); } +EXPORT_SYMBOL_GPL(cond_wakeup_cpu0); /* * We need to flush the caches before going to sleep, lest we have @@ -1734,11 +1736,8 @@ static inline void mwait_play_dead(void) __monitor(mwait_ptr, 0, 0); mb(); __mwait(eax, 0); - /* - * If NMI wants to wake up CPU0, start CPU0. - */ - if (wakeup_cpu0()) - start_cpu0(); + + cond_wakeup_cpu0(); } } @@ -1749,11 +1748,8 @@ void hlt_play_dead(void) while (1) { native_halt(); - /* - * If NMI wants to wake up CPU0, start CPU0. - */ - if (wakeup_cpu0()) - start_cpu0(); + + cond_wakeup_cpu0(); } } diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c index 768a6b4d2368..4e2d76b8b697 100644 --- a/drivers/acpi/processor_idle.c +++ b/drivers/acpi/processor_idle.c @@ -544,9 +544,7 @@ static int acpi_idle_play_dead(struct cpuidle_device *dev, int index) return -ENODEV; #if defined(CONFIG_X86) && defined(CONFIG_HOTPLUG_CPU) - /* If NMI wants to wake up CPU0, start CPU0. */ - if (wakeup_cpu0()) - start_cpu0(); + cond_wakeup_cpu0(); #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> --- Changes since v1: - Rename wakeup_cpu0() to cond_wakeup_cpu0() and fold wakeup_cpu0() in as it has no other users [Rafael J. Wysocki] --- arch/x86/include/asm/smp.h | 2 +- arch/x86/kernel/smpboot.c | 24 ++++++++++-------------- drivers/acpi/processor_idle.c | 4 +--- 3 files changed, 12 insertions(+), 18 deletions(-)