| Message ID | 20220509233235.995021-17-dmitry.osipenko@collabora.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | Introduce power-off+restart call chain API | expand |
Hi Dmitry, On Tue, May 10, 2022 at 1:34 AM Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > Kernel now supports chained power-off handlers. Use > register_power_off_handler() that registers power-off handlers and > do_kernel_power_off() that invokes chained power-off handlers. Legacy > pm_power_off() will be removed once all drivers will be converted to > the new sys-off API. > > Normally arch code should adopt only the do_kernel_power_off() at first, > but m68k is a special case because it uses pm_power_off() "inside out", > i.e. pm_power_off() invokes machine_power_off() [in fact it does nothing], > while it's machine_power_off() that should invoke the pm_power_off(), and > thus, we can't convert platforms to the new API separately. There are only > two platforms changed here, so it's not a big deal. > > Acked-by: Geert Uytterhoeven <geert@linux-m68k.org> > Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> Thanks for your patch, which is now commit f0f7e5265b3b37b0 ("m68k: Switch to new sys-off handler API") upstream. > --- a/arch/m68k/emu/natfeat.c > +++ b/arch/m68k/emu/natfeat.c > @@ -15,6 +15,7 @@ > #include <linux/string.h> > #include <linux/kernel.h> > #include <linux/module.h> > +#include <linux/reboot.h> > #include <linux/io.h> > #include <asm/machdep.h> > #include <asm/natfeat.h> > @@ -90,5 +91,5 @@ void __init nf_init(void) > pr_info("NatFeats found (%s, %lu.%lu)\n", buf, version >> 16, > version & 0xffff); > > - mach_power_off = nf_poweroff; > + register_platform_power_off(nf_poweroff); Unfortunately nothing is registered, as this is called very early (from setup_arch(), before the memory allocator is available. Hence register_sys_off_handler() fails with -ENOMEM, and poweroff stops working. Possible solutions: - As at most one handler can be registered, register_platform_power_off() could use a static struct sys_off_handler instance, - Keep mach_power_off, and call register_platform_power_off() later. Anything else? Thanks! > --- a/arch/m68k/mac/config.c > +++ b/arch/m68k/mac/config.c > @@ -12,6 +12,7 @@ > > #include <linux/errno.h> > #include <linux/module.h> > +#include <linux/reboot.h> > #include <linux/types.h> > #include <linux/mm.h> > #include <linux/tty.h> > @@ -140,7 +141,6 @@ void __init config_mac(void) > mach_hwclk = mac_hwclk; > mach_reset = mac_reset; > mach_halt = mac_poweroff; > - mach_power_off = mac_poweroff; > #if IS_ENABLED(CONFIG_INPUT_M68K_BEEP) > mach_beep = mac_mksound; > #endif > @@ -160,6 +160,8 @@ void __init config_mac(void) > > if (macintosh_config->ident == MAC_MODEL_IICI) > mach_l2_flush = via_l2_flush; > + > + register_platform_power_off(mac_poweroff); > } This must have the same problem. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 5/31/22 22:04, Geert Uytterhoeven wrote: > Hi Dmitry, > > On Tue, May 10, 2022 at 1:34 AM Dmitry Osipenko > <dmitry.osipenko@collabora.com> wrote: >> Kernel now supports chained power-off handlers. Use >> register_power_off_handler() that registers power-off handlers and >> do_kernel_power_off() that invokes chained power-off handlers. Legacy >> pm_power_off() will be removed once all drivers will be converted to >> the new sys-off API. >> >> Normally arch code should adopt only the do_kernel_power_off() at first, >> but m68k is a special case because it uses pm_power_off() "inside out", >> i.e. pm_power_off() invokes machine_power_off() [in fact it does nothing], >> while it's machine_power_off() that should invoke the pm_power_off(), and >> thus, we can't convert platforms to the new API separately. There are only >> two platforms changed here, so it's not a big deal. >> >> Acked-by: Geert Uytterhoeven <geert@linux-m68k.org> >> Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> >> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > > Thanks for your patch, which is now commit f0f7e5265b3b37b0 > ("m68k: Switch to new sys-off handler API") upstream. > >> --- a/arch/m68k/emu/natfeat.c >> +++ b/arch/m68k/emu/natfeat.c >> @@ -15,6 +15,7 @@ >> #include <linux/string.h> >> #include <linux/kernel.h> >> #include <linux/module.h> >> +#include <linux/reboot.h> >> #include <linux/io.h> >> #include <asm/machdep.h> >> #include <asm/natfeat.h> >> @@ -90,5 +91,5 @@ void __init nf_init(void) >> pr_info("NatFeats found (%s, %lu.%lu)\n", buf, version >> 16, >> version & 0xffff); >> >> - mach_power_off = nf_poweroff; >> + register_platform_power_off(nf_poweroff); > > Unfortunately nothing is registered, as this is called very early > (from setup_arch(), before the memory allocator is available. > Hence register_sys_off_handler() fails with -ENOMEM, and poweroff > stops working. > > Possible solutions: > - As at most one handler can be registered, > register_platform_power_off() could use a static struct sys_off_handler > instance, > - Keep mach_power_off, and call register_platform_power_off() later. > > Anything else? > Thanks! > >> --- a/arch/m68k/mac/config.c >> +++ b/arch/m68k/mac/config.c >> @@ -12,6 +12,7 @@ >> >> #include <linux/errno.h> >> #include <linux/module.h> >> +#include <linux/reboot.h> >> #include <linux/types.h> >> #include <linux/mm.h> >> #include <linux/tty.h> >> @@ -140,7 +141,6 @@ void __init config_mac(void) >> mach_hwclk = mac_hwclk; >> mach_reset = mac_reset; >> mach_halt = mac_poweroff; >> - mach_power_off = mac_poweroff; >> #if IS_ENABLED(CONFIG_INPUT_M68K_BEEP) >> mach_beep = mac_mksound; >> #endif >> @@ -160,6 +160,8 @@ void __init config_mac(void) >> >> if (macintosh_config->ident == MAC_MODEL_IICI) >> mach_l2_flush = via_l2_flush; >> + >> + register_platform_power_off(mac_poweroff); >> } > > This must have the same problem. The static variant should be better, IMO. I'm not sure whether other platforms won't face the same problem once they will start using register_platform_power_off(). I'll send the fix, thank you for the testing! --- >8 --- diff --git a/kernel/reboot.c b/kernel/reboot.c index a091145ee710..4fea05d387dc 100644 --- a/kernel/reboot.c +++ b/kernel/reboot.c @@ -315,6 +315,37 @@ static int sys_off_notify(struct notifier_block *nb, return handler->sys_off_cb(&data); } +static struct sys_off_handler platform_sys_off_handler; + +static struct sys_off_handler *alloc_sys_off_handler(int priority) +{ + struct sys_off_handler *handler; + + /* + * Platforms like m68k can't allocate sys_off handler dynamically + * at the early boot time. + */ + if (priority == SYS_OFF_PRIO_PLATFORM) { + handler = &platform_sys_off_handler; + if (handler->cb_data) + return ERR_PTR(-EBUSY); + } else { + handler = kzalloc(sizeof(*handler), GFP_KERNEL); + if (!handler) + return ERR_PTR(-ENOMEM); + } + + return handler; +} + +static void free_sys_off_handler(struct sys_off_handler *handler) +{ + if (handler == &platform_sys_off_handler) + memset(handler, 0, sizeof(*handler)); + else + kfree(handler); +} + /** * register_sys_off_handler - Register sys-off handler * @mode: Sys-off mode @@ -345,9 +376,9 @@ register_sys_off_handler(enum sys_off_mode mode, struct sys_off_handler *handler; int err; - handler = kzalloc(sizeof(*handler), GFP_KERNEL); - if (!handler) - return ERR_PTR(-ENOMEM); + handler = alloc_sys_off_handler(priority); + if (IS_ERR(handler)) + return handler; switch (mode) { case SYS_OFF_MODE_POWER_OFF_PREPARE: @@ -364,7 +395,7 @@ register_sys_off_handler(enum sys_off_mode mode, break; default: - kfree(handler); + free_sys_off_handler(handler); return ERR_PTR(-EINVAL); } @@ -391,7 +422,7 @@ register_sys_off_handler(enum sys_off_mode mode, } if (err) { - kfree(handler); + free_sys_off_handler(handler); return ERR_PTR(err); } @@ -422,7 +453,7 @@ void unregister_sys_off_handler(struct sys_off_handler *handler) /* sanity check, shall never happen */ WARN_ON(err); - kfree(handler); + free_sys_off_handler(handler); } EXPORT_SYMBOL_GPL(unregister_sys_off_handler);
diff --git a/arch/m68k/emu/natfeat.c b/arch/m68k/emu/natfeat.c index 71b78ecee75c..b19dc00026d9 100644 --- a/arch/m68k/emu/natfeat.c +++ b/arch/m68k/emu/natfeat.c @@ -15,6 +15,7 @@ #include <linux/string.h> #include <linux/kernel.h> #include <linux/module.h> +#include <linux/reboot.h> #include <linux/io.h> #include <asm/machdep.h> #include <asm/natfeat.h> @@ -90,5 +91,5 @@ void __init nf_init(void) pr_info("NatFeats found (%s, %lu.%lu)\n", buf, version >> 16, version & 0xffff); - mach_power_off = nf_poweroff; + register_platform_power_off(nf_poweroff); } diff --git a/arch/m68k/include/asm/machdep.h b/arch/m68k/include/asm/machdep.h index 8fd80ef1b77e..8d8c3ee2069f 100644 --- a/arch/m68k/include/asm/machdep.h +++ b/arch/m68k/include/asm/machdep.h @@ -24,7 +24,6 @@ extern int (*mach_get_rtc_pll)(struct rtc_pll_info *); extern int (*mach_set_rtc_pll)(struct rtc_pll_info *); extern void (*mach_reset)( void ); extern void (*mach_halt)( void ); -extern void (*mach_power_off)( void ); extern unsigned long (*mach_hd_init) (unsigned long, unsigned long); extern void (*mach_hd_setup)(char *, int *); extern void (*mach_heartbeat) (int); diff --git a/arch/m68k/kernel/process.c b/arch/m68k/kernel/process.c index 221feb0269f1..2cb4a61bcfac 100644 --- a/arch/m68k/kernel/process.c +++ b/arch/m68k/kernel/process.c @@ -67,12 +67,11 @@ void machine_halt(void) void machine_power_off(void) { - if (mach_power_off) - mach_power_off(); + do_kernel_power_off(); for (;;); } -void (*pm_power_off)(void) = machine_power_off; +void (*pm_power_off)(void); EXPORT_SYMBOL(pm_power_off); void show_regs(struct pt_regs * regs) diff --git a/arch/m68k/kernel/setup_mm.c b/arch/m68k/kernel/setup_mm.c index 78ab562beb31..42691abcd908 100644 --- a/arch/m68k/kernel/setup_mm.c +++ b/arch/m68k/kernel/setup_mm.c @@ -98,7 +98,6 @@ EXPORT_SYMBOL(mach_get_rtc_pll); EXPORT_SYMBOL(mach_set_rtc_pll); void (*mach_reset)( void ); void (*mach_halt)( void ); -void (*mach_power_off)( void ); #ifdef CONFIG_HEARTBEAT void (*mach_heartbeat) (int); EXPORT_SYMBOL(mach_heartbeat); diff --git a/arch/m68k/kernel/setup_no.c b/arch/m68k/kernel/setup_no.c index 5e4104f07a44..00bf82258233 100644 --- a/arch/m68k/kernel/setup_no.c +++ b/arch/m68k/kernel/setup_no.c @@ -55,7 +55,6 @@ int (*mach_hwclk) (int, struct rtc_time*); /* machine dependent reboot functions */ void (*mach_reset)(void); void (*mach_halt)(void); -void (*mach_power_off)(void); #ifdef CONFIG_M68000 #if defined(CONFIG_M68328) diff --git a/arch/m68k/mac/config.c b/arch/m68k/mac/config.c index 65d124ec80bb..382f656c29ea 100644 --- a/arch/m68k/mac/config.c +++ b/arch/m68k/mac/config.c @@ -12,6 +12,7 @@ #include <linux/errno.h> #include <linux/module.h> +#include <linux/reboot.h> #include <linux/types.h> #include <linux/mm.h> #include <linux/tty.h> @@ -140,7 +141,6 @@ void __init config_mac(void) mach_hwclk = mac_hwclk; mach_reset = mac_reset; mach_halt = mac_poweroff; - mach_power_off = mac_poweroff; #if IS_ENABLED(CONFIG_INPUT_M68K_BEEP) mach_beep = mac_mksound; #endif @@ -160,6 +160,8 @@ void __init config_mac(void) if (macintosh_config->ident == MAC_MODEL_IICI) mach_l2_flush = via_l2_flush; + + register_platform_power_off(mac_poweroff); }