[PATCH v2] PM: ACPI: reboot: Reinstate S5 for reboot

[PATCH v2] PM: ACPI: reboot: Reinstate S5 for reboot

[Kai-Heng Feng]

Commit d60cd06331a3 ("PM: ACPI: reboot: Use S5 for reboot") caused Dell
PowerEdge r440 hangs at boot.

The issue is fixed by commit 2ca1c94ce0b6 ("tg3: Disable tg3 device on
system reboot to avoid triggering AER"), so reinstate the patch again.

Cc: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v2:
 - Use do_kernel_power_off_prepare() instead.

 kernel/reboot.c | 55 +++++++++++++++++++++++++------------------------
 1 file changed, 28 insertions(+), 27 deletions(-)

diff --git a/kernel/reboot.c b/kernel/reboot.c
index 3c35445bf5ad3..39cbb45afc54a 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -243,28 +243,6 @@ void migrate_to_reboot_cpu(void)
 	set_cpus_allowed_ptr(current, cpumask_of(cpu));
 }
 
-/**
- *	kernel_restart - reboot the system
- *	@cmd: pointer to buffer containing command to execute for restart
- *		or %NULL
- *
- *	Shutdown everything and perform a clean reboot.
- *	This is not safe to call in interrupt context.
- */
-void kernel_restart(char *cmd)
-{
-	kernel_restart_prepare(cmd);
-	migrate_to_reboot_cpu();
-	syscore_shutdown();
-	if (!cmd)
-		pr_emerg("Restarting system\n");
-	else
-		pr_emerg("Restarting system with command '%s'\n", cmd);
-	kmsg_dump(KMSG_DUMP_SHUTDOWN);
-	machine_restart(cmd);
-}
-EXPORT_SYMBOL_GPL(kernel_restart);
-
 static void kernel_shutdown_prepare(enum system_states state)
 {
 	blocking_notifier_call_chain(&reboot_notifier_list,
@@ -301,6 +279,34 @@ static BLOCKING_NOTIFIER_HEAD(power_off_prep_handler_list);
  */
 static ATOMIC_NOTIFIER_HEAD(power_off_handler_list);
 
+static void do_kernel_power_off_prepare(void)
+{
+	blocking_notifier_call_chain(&power_off_prep_handler_list, 0, NULL);
+}
+
+/**
+ *	kernel_restart - reboot the system
+ *	@cmd: pointer to buffer containing command to execute for restart
+ *		or %NULL
+ *
+ *	Shutdown everything and perform a clean reboot.
+ *	This is not safe to call in interrupt context.
+ */
+void kernel_restart(char *cmd)
+{
+	kernel_restart_prepare(cmd);
+	do_kernel_power_off_prepare();
+	migrate_to_reboot_cpu();
+	syscore_shutdown();
+	if (!cmd)
+		pr_emerg("Restarting system\n");
+	else
+		pr_emerg("Restarting system with command '%s'\n", cmd);
+	kmsg_dump(KMSG_DUMP_SHUTDOWN);
+	machine_restart(cmd);
+}
+EXPORT_SYMBOL_GPL(kernel_restart);
+
 static int sys_off_notify(struct notifier_block *nb,
 			  unsigned long mode, void *cmd)
 {
@@ -606,11 +612,6 @@ static int legacy_pm_power_off(struct sys_off_data *data)
 	return NOTIFY_DONE;
 }
 
-static void do_kernel_power_off_prepare(void)
-{
-	blocking_notifier_call_chain(&power_off_prep_handler_list, 0, NULL);
-}
-
 /**
  *	do_kernel_power_off - Execute kernel power-off handler call chain
  *
-- 
2.36.1

Re: [PATCH v2] PM: ACPI: reboot: Reinstate S5 for reboot

[Luis Chamberlain]

On Tue, Sep 06, 2022 at 10:31:07PM +0800, Kai-Heng Feng wrote:
> Commit d60cd06331a3 ("PM: ACPI: reboot: Use S5 for reboot") caused Dell
> PowerEdge r440 hangs at boot.
> 
> The issue is fixed by commit 2ca1c94ce0b6 ("tg3: Disable tg3 device on
> system reboot to avoid triggering AER"), so reinstate the patch again.
> 
> Cc: Josef Bacik <josef@toxicpanda.com>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

The addition of do_kernel_power_off_prepare() is not clear from
your patch, it would be easier to review and therefore detect
regressions more easily if you first moved the the code without
modifications and then after make another change in another patch.

  Luis

Re: [PATCH v2] PM: ACPI: reboot: Reinstate S5 for reboot

[Dmitry Osipenko]

06.09.2022 17:31, Kai-Heng Feng пишет:
> Commit d60cd06331a3 ("PM: ACPI: reboot: Use S5 for reboot") caused Dell
> PowerEdge r440 hangs at boot.
> 
> The issue is fixed by commit 2ca1c94ce0b6 ("tg3: Disable tg3 device on
> system reboot to avoid triggering AER"), so reinstate the patch again.
> 
> Cc: Josef Bacik <josef@toxicpanda.com>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
> v2:
>  - Use do_kernel_power_off_prepare() instead.
> 
>  kernel/reboot.c | 55 +++++++++++++++++++++++++------------------------
>  1 file changed, 28 insertions(+), 27 deletions(-)
> 
> diff --git a/kernel/reboot.c b/kernel/reboot.c
> index 3c35445bf5ad3..39cbb45afc54a 100644
> --- a/kernel/reboot.c
> +++ b/kernel/reboot.c
> @@ -243,28 +243,6 @@ void migrate_to_reboot_cpu(void)
>  	set_cpus_allowed_ptr(current, cpumask_of(cpu));
>  }
>  
> -/**
> - *	kernel_restart - reboot the system
> - *	@cmd: pointer to buffer containing command to execute for restart
> - *		or %NULL
> - *
> - *	Shutdown everything and perform a clean reboot.
> - *	This is not safe to call in interrupt context.
> - */
> -void kernel_restart(char *cmd)
> -{
> -	kernel_restart_prepare(cmd);
> -	migrate_to_reboot_cpu();
> -	syscore_shutdown();
> -	if (!cmd)
> -		pr_emerg("Restarting system\n");
> -	else
> -		pr_emerg("Restarting system with command '%s'\n", cmd);
> -	kmsg_dump(KMSG_DUMP_SHUTDOWN);
> -	machine_restart(cmd);
> -}
> -EXPORT_SYMBOL_GPL(kernel_restart);
> -
>  static void kernel_shutdown_prepare(enum system_states state)
>  {
>  	blocking_notifier_call_chain(&reboot_notifier_list,
> @@ -301,6 +279,34 @@ static BLOCKING_NOTIFIER_HEAD(power_off_prep_handler_list);
>   */
>  static ATOMIC_NOTIFIER_HEAD(power_off_handler_list);
>  
> +static void do_kernel_power_off_prepare(void)
> +{
> +	blocking_notifier_call_chain(&power_off_prep_handler_list, 0, NULL);
> +}
> +
> +/**
> + *	kernel_restart - reboot the system
> + *	@cmd: pointer to buffer containing command to execute for restart
> + *		or %NULL
> + *
> + *	Shutdown everything and perform a clean reboot.
> + *	This is not safe to call in interrupt context.
> + */
> +void kernel_restart(char *cmd)
> +{
> +	kernel_restart_prepare(cmd);
> +	do_kernel_power_off_prepare();

Looks like an abuse to me. Adding new SYS_OFF_MODE_RESTART_PREPARE and
updating acpi_sleep_init() to use it should be a better solution.

Re: [PATCH v2] PM: ACPI: reboot: Reinstate S5 for reboot

[Kai-Heng Feng]

On Wed, Sep 7, 2022 at 4:07 AM Dmitry Osipenko <digetx@gmail.com> wrote:
>
> 06.09.2022 17:31, Kai-Heng Feng пишет:
> > Commit d60cd06331a3 ("PM: ACPI: reboot: Use S5 for reboot") caused Dell
> > PowerEdge r440 hangs at boot.
> >
> > The issue is fixed by commit 2ca1c94ce0b6 ("tg3: Disable tg3 device on
> > system reboot to avoid triggering AER"), so reinstate the patch again.
> >
> > Cc: Josef Bacik <josef@toxicpanda.com>
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > ---
> > v2:
> >  - Use do_kernel_power_off_prepare() instead.
> >
> >  kernel/reboot.c | 55 +++++++++++++++++++++++++------------------------
> >  1 file changed, 28 insertions(+), 27 deletions(-)
> >
> > diff --git a/kernel/reboot.c b/kernel/reboot.c
> > index 3c35445bf5ad3..39cbb45afc54a 100644
> > --- a/kernel/reboot.c
> > +++ b/kernel/reboot.c
> > @@ -243,28 +243,6 @@ void migrate_to_reboot_cpu(void)
> >       set_cpus_allowed_ptr(current, cpumask_of(cpu));
> >  }
> >
> > -/**
> > - *   kernel_restart - reboot the system
> > - *   @cmd: pointer to buffer containing command to execute for restart
> > - *           or %NULL
> > - *
> > - *   Shutdown everything and perform a clean reboot.
> > - *   This is not safe to call in interrupt context.
> > - */
> > -void kernel_restart(char *cmd)
> > -{
> > -     kernel_restart_prepare(cmd);
> > -     migrate_to_reboot_cpu();
> > -     syscore_shutdown();
> > -     if (!cmd)
> > -             pr_emerg("Restarting system\n");
> > -     else
> > -             pr_emerg("Restarting system with command '%s'\n", cmd);
> > -     kmsg_dump(KMSG_DUMP_SHUTDOWN);
> > -     machine_restart(cmd);
> > -}
> > -EXPORT_SYMBOL_GPL(kernel_restart);
> > -
> >  static void kernel_shutdown_prepare(enum system_states state)
> >  {
> >       blocking_notifier_call_chain(&reboot_notifier_list,
> > @@ -301,6 +279,34 @@ static BLOCKING_NOTIFIER_HEAD(power_off_prep_handler_list);
> >   */
> >  static ATOMIC_NOTIFIER_HEAD(power_off_handler_list);
> >
> > +static void do_kernel_power_off_prepare(void)
> > +{
> > +     blocking_notifier_call_chain(&power_off_prep_handler_list, 0, NULL);
> > +}
> > +
> > +/**
> > + *   kernel_restart - reboot the system
> > + *   @cmd: pointer to buffer containing command to execute for restart
> > + *           or %NULL
> > + *
> > + *   Shutdown everything and perform a clean reboot.
> > + *   This is not safe to call in interrupt context.
> > + */
> > +void kernel_restart(char *cmd)
> > +{
> > +     kernel_restart_prepare(cmd);
> > +     do_kernel_power_off_prepare();
>
> Looks like an abuse to me. Adding new SYS_OFF_MODE_RESTART_PREPARE and
> updating acpi_sleep_init() to use it should be a better solution.

Thanks, will send a new one based on your comment.

Kai-Heng