Messages in this thread Patch in this message | | | Date | Sun, 5 Jun 2022 15:19:11 +0300 | Subject | Re: [PATCH v1] kernel/reboot: Change registration order of legacy power-off handler | From | Dmitry Osipenko <> |
| |
Hi Michael,
On 6/5/22 05:01, Michael Ellerman wrote: > Dmitry Osipenko <dmitry.osipenko@collabora.com> writes: >> We're unconditionally registering sys-off handler for the legacy >> pm_power_off() callback, this causes problem for platforms that don't >> use power-off handlers at all and should be halted. Now reboot syscall >> assumes that there is a power-off handler installed and tries to power >> off system instead of halting it. >> >> To fix the trouble, move the handler's registration to the reboot syscall >> and check the pm_power_off() presence. > > I'm seeing a qemu virtual machine (ppce500) fail to power off using the > gpio-poweroff driver. I bisected it to this commit. > > I think the problem is that the machine is going via kernel_power_off(), > not sys_reboot(), and so legacy_pm_power_off() has not been registered. > > If I just put the core_initcall back then it works as before. Not sure > if that's a safe change in general though.
Thank you very much for the testing and reporting the problem! I see now the two more cases that were missed previously:
1. There is the orderly_poweroff() used by some drivers. 2. PowerPC may invoke do_kernel_power_off() directly from xmon code.
Could you please test this change:
--- >8 ---
diff --git a/kernel/reboot.c b/kernel/reboot.c index 3b19b123efec..0e4a3defcd94 100644 --- a/kernel/reboot.c +++ b/kernel/reboot.c @@ -320,6 +320,7 @@ 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; + gfp_t flags; /* * Platforms like m68k can't allocate sys_off handler dynamically @@ -330,7 +331,12 @@ static struct sys_off_handler *alloc_sys_off_handler(int priority) if (handler->cb_data) return ERR_PTR(-EBUSY); } else { - handler = kzalloc(sizeof(*handler), GFP_KERNEL); + if (system_state > SYSTEM_RUNNING) + flags = GFP_ATOMIC; + else + flags = GFP_KERNEL; + + handler = kzalloc(sizeof(*handler), flags); if (!handler) return ERR_PTR(-ENOMEM); } @@ -615,7 +621,26 @@ static void do_kernel_power_off_prepare(void) */ void do_kernel_power_off(void) { + struct sys_off_handler *sys_off = NULL; + + /* + * Register sys-off handlers for legacy PM callback. This allows + * legacy PM callbacks temporary co-exist with the new sys-off API. + * + * TODO: Remove legacy handlers once all legacy PM users will be + * switched to the sys-off based APIs. + */ + if (pm_power_off) { + sys_off = register_sys_off_handler(SYS_OFF_MODE_POWER_OFF, + SYS_OFF_PRIO_DEFAULT, + legacy_pm_power_off, NULL); + if (IS_ERR(sys_off)) + return; + } + atomic_notifier_call_chain(&power_off_handler_list, 0, NULL); + + unregister_sys_off_handler(sys_off); } /** @@ -626,7 +651,8 @@ void do_kernel_power_off(void) */ bool kernel_can_power_off(void) { - return !atomic_notifier_call_chain_is_empty(&power_off_handler_list); + return !atomic_notifier_call_chain_is_empty(&power_off_handler_list) || + pm_power_off; } EXPORT_SYMBOL_GPL(kernel_can_power_off); @@ -661,7 +687,6 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd, void __user *, arg) { struct pid_namespace *pid_ns = task_active_pid_ns(current); - struct sys_off_handler *sys_off = NULL; char buffer[256]; int ret = 0; @@ -686,21 +711,6 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd, if (ret) return ret; - /* - * Register sys-off handlers for legacy PM callback. This allows - * legacy PM callbacks temporary co-exist with the new sys-off API. - * - * TODO: Remove legacy handlers once all legacy PM users will be - * switched to the sys-off based APIs. - */ - if (pm_power_off) { - sys_off = register_sys_off_handler(SYS_OFF_MODE_POWER_OFF, - SYS_OFF_PRIO_DEFAULT, - legacy_pm_power_off, NULL); - if (IS_ERR(sys_off)) - return PTR_ERR(sys_off); - } - /* Instead of trying to make the power_off code look like * halt when pm_power_off is not set do it the easy way. */ @@ -758,7 +768,6 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd, break; } mutex_unlock(&system_transition_mutex); - unregister_sys_off_handler(sys_off); return ret; }
| |