Re: [PATCH v2] PM: hibernate: Fix the bug where wake events cannot wake system during hibernation

[Rafael J. Wysocki]

On Mon, Nov 20, 2023 at 9:15 AM Chris Feng <chris.feng@xxxxxxxxxxxx> wrote:
>
> Wake-up events that occur in the hibernation process's
> hibernation_platform_enter() cannot wake up the system. Although the
> current hibernation framework will execute part of the recovery process
> after a wake-up event occurs, it ultimately performs a shutdown operation
> because the system does not check the return value of
> hibernation_platform_enter(). Moreover, when restoring the device before
> system shutdown, the device's I/O and DMA capabilities will be turned on,
> which can lead to data loss.

This isn't correct, because devices are enabled to do IO and DMA
already before saving the hibernation image.

The problem really only is that if a wakeup event occurs before
putting the system into the final low-power state, it will be missed,
so the patch subject should be something like "Avoid missing wakeup
events during hibernation".

> To solve this problem, check the return value of
> hibernation_platform_enter(). When it returns -EAGAIN or -EBUSY, execute
> the hibernation recovery process, discard the previously saved image, and
> ultimately return to the working state.
>
> Signed-off-by: Chris Feng <chris.feng@xxxxxxxxxxxx>
> ---
> [PATCH v2]:
>  - Execute the hibernation recovery process and return to the working state
>    when the return value of the function hibernation_platform_enter() is
>    -EAGAIN or -EBUSY. Both of the two values may indicate the occurrence of
>    a wake-up event.
> ---
>  kernel/power/hibernate.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index 8d35b9f9aaa3..7e39a9baca9e 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -642,9 +642,9 @@ int hibernation_platform_enter(void)
>   */
>  static void power_down(void)
>  {
> -#ifdef CONFIG_SUSPEND
>         int error;
>
> +#ifdef CONFIG_SUSPEND
>         if (hibernation_mode == HIBERNATION_SUSPEND) {
>                 error = suspend_devices_and_enter(mem_sleep_current);
>                 if (error) {
> @@ -667,7 +667,13 @@ static void power_down(void)
>                 kernel_restart(NULL);
>                 break;
>         case HIBERNATION_PLATFORM:
> -               hibernation_platform_enter();
> +               error = hibernation_platform_enter();
> +               if (error == -EAGAIN || error == -EBUSY) {
> +                       swsusp_unmark();
> +                       events_check_enabled = false;
> +                       pr_err("Hibernation Abort.\n");

This is not an error condition, so pr_err() is not the right level.
pr_info() would suffice.

Also the message needs to be more precise, so whoever reads it will
know that this is about a (legitimate) wakeup event.

> +                       return;
> +               }
>                 fallthrough;
>         case HIBERNATION_SHUTDOWN:
>                 if (kernel_can_power_off())
> --