| Message ID | e47fc487980d5330e6059ac6e16416bec88cda0e.1676358308.git.jpoimboe@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | cpu,sched: Mark arch_cpu_idle_dead() __noreturn | expand |
On 14/2/23 08:05, Josh Poimboeuf wrote: > cpu_die() doesn't return. Annotate it as such. By extension this also > makes arch_cpu_idle_dead() noreturn. > > Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> > --- > arch/arm64/include/asm/smp.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h > index fc55f5a57a06..5733a31bab08 100644 > --- a/arch/arm64/include/asm/smp.h > +++ b/arch/arm64/include/asm/smp.h > @@ -100,7 +100,7 @@ static inline void arch_send_wakeup_ipi_mask(const struct cpumask *mask) > extern int __cpu_disable(void); > > extern void __cpu_die(unsigned int cpu); > -extern void cpu_die(void); > +extern void __noreturn cpu_die(void); > extern void cpu_die_early(void); Shouldn't cpu_operations::cpu_die() be declared noreturn first?
On Tue, Feb 14, 2023 at 09:13:08AM +0100, Philippe Mathieu-Daudé wrote: > On 14/2/23 08:05, Josh Poimboeuf wrote: > > cpu_die() doesn't return. Annotate it as such. By extension this also > > makes arch_cpu_idle_dead() noreturn. > > > > Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> > > --- > > arch/arm64/include/asm/smp.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h > > index fc55f5a57a06..5733a31bab08 100644 > > --- a/arch/arm64/include/asm/smp.h > > +++ b/arch/arm64/include/asm/smp.h > > @@ -100,7 +100,7 @@ static inline void arch_send_wakeup_ipi_mask(const struct cpumask *mask) > > extern int __cpu_disable(void); > > extern void __cpu_die(unsigned int cpu); > > -extern void cpu_die(void); > > +extern void __noreturn cpu_die(void); > > extern void cpu_die_early(void); > > Shouldn't cpu_operations::cpu_die() be declared noreturn first? The cpu_die() function ends with a BUG(), and so does not return, even if a cpu_operations::cpu_die() function that it calls erroneously returned. We *could* mark cpu_operations::cpu_die() as noreturn, but I'd prefer that we did not so that the compiler doesn't optimize away the BUG() which is there to catch such erroneous returns. That said, could we please add __noreturn to the implementation of cpu_die() in arch/arm64/kernel/smp.c? i.e. the fixup below. With that fixup: Acked-by: Mark Rutland <mark.rutland@arm.com> Mark. ---->8---- diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c index ffc5d76cf695..a98a76f7c1c6 100644 --- a/arch/arm64/kernel/smp.c +++ b/arch/arm64/kernel/smp.c @@ -361,7 +361,7 @@ void __cpu_die(unsigned int cpu) * Called from the idle thread for the CPU which has been shutdown. * */ -void cpu_die(void) +void __noreturn cpu_die(void) { unsigned int cpu = smp_processor_id(); const struct cpu_operations *ops = get_cpu_ops(cpu);
On Wed, Feb 15, 2023 at 01:09:21PM +0000, Mark Rutland wrote: > On Tue, Feb 14, 2023 at 09:13:08AM +0100, Philippe Mathieu-Daudé wrote: > > On 14/2/23 08:05, Josh Poimboeuf wrote: > > > cpu_die() doesn't return. Annotate it as such. By extension this also > > > makes arch_cpu_idle_dead() noreturn. > > > > > > Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> > > > --- > > > arch/arm64/include/asm/smp.h | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h > > > index fc55f5a57a06..5733a31bab08 100644 > > > --- a/arch/arm64/include/asm/smp.h > > > +++ b/arch/arm64/include/asm/smp.h > > > @@ -100,7 +100,7 @@ static inline void arch_send_wakeup_ipi_mask(const struct cpumask *mask) > > > extern int __cpu_disable(void); > > > extern void __cpu_die(unsigned int cpu); > > > -extern void cpu_die(void); > > > +extern void __noreturn cpu_die(void); > > > extern void cpu_die_early(void); > > > > Shouldn't cpu_operations::cpu_die() be declared noreturn first? > > The cpu_die() function ends with a BUG(), and so does not return, even if a > cpu_operations::cpu_die() function that it calls erroneously returned. > > We *could* mark cpu_operations::cpu_die() as noreturn, but I'd prefer that we > did not so that the compiler doesn't optimize away the BUG() which is there to > catch such erroneous returns. > > That said, could we please add __noreturn to the implementation of cpu_die() in > arch/arm64/kernel/smp.c? i.e. the fixup below. Done. > With that fixup: > > Acked-by: Mark Rutland <mark.rutland@arm.com> Thanks!
diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h index fc55f5a57a06..5733a31bab08 100644 --- a/arch/arm64/include/asm/smp.h +++ b/arch/arm64/include/asm/smp.h @@ -100,7 +100,7 @@ static inline void arch_send_wakeup_ipi_mask(const struct cpumask *mask) extern int __cpu_disable(void); extern void __cpu_die(unsigned int cpu); -extern void cpu_die(void); +extern void __noreturn cpu_die(void); extern void cpu_die_early(void); static inline void cpu_park_loop(void)
cpu_die() doesn't return. Annotate it as such. By extension this also makes arch_cpu_idle_dead() noreturn. Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> --- arch/arm64/include/asm/smp.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)