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

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

[Chris Feng (冯保林)]

On Wed, 2023-10-25 at 20:28 +0200, Rafael J. Wysocki wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  
> On Tue, Oct 24, 2023 at 11:15 AM <chris.feng@mediatek.com> wrote:
> >
> > From: Chris Feng <chris.feng@mediatek.com>
> >
> > 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.
> >
> > To solve this problem, check the return value of
> > hibernation_platform_enter(). When it returns a non-zero value,
> execute
> > the hibernation recovery process, discard the previously saved
> image, and
> > ultimately return to the working state.
> 
> While I agree with the problem statement, I don't completely agree
> with the patch.
> 
> > Signed-off-by: Chris Feng <chris.feng@mediatek.com>
> > ---
> >  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..16d8027a195d 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) {
> 
> This error need not be -EAGAIN which means pending wakeup.  There are
> other errors that can be returned for which the fallback to shutdown
> is still desirable.
> 

Thank you for your comments. I have some questions:
In function hibernation_platform_enter(), if function
dpm_suspend_start/end returns error, it goes to resume devices flow.
After the system restores the devices, the system shuts down.  Is this
expected design behivour?  If it is, would you help to give the design
reasons in short ? From my point of view, since the deivces are
resumed, why dose the system go to shut down state ? And also, after
the system shuts down , and when the system is waked up by power key,
the devices will be resumed again when restoring the saved system's
image.

> > +                       swsusp_unmark();
> > +                       events_check_enabled = false;
> > +                       pr_err("Hibernation Abort.\n");
> > +                       return;
> > +               }
> >                 fallthrough;
> >         case HIBERNATION_SHUTDOWN:
> >                 if (kernel_can_power_off())
> > --
> > 2.17.0
> >

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

[Chris Feng (冯保林)]

Hi Rafael J,

On Fri, 2023-10-27 at 13:22 +0800, Chris Feng wrote:
> On Wed, 2023-10-25 at 20:28 +0200, Rafael J. Wysocki wrote:
> > On Tue, Oct 24, 2023 at 11:15 AM <chris.feng@mediatek.com> wrote:
> > > 
> > > ---
> > >  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..16d8027a195d 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) {
> > 
> > This error need not be -EAGAIN which means pending wakeup.  There
> > are
> > other errors that can be returned for which the fallback to
> > shutdown
> > is still desirable.
> > 
> 
> Thank you for your comments. I have some questions:
> In function hibernation_platform_enter(), if function
> dpm_suspend_start/end returns error, it goes to resume devices flow.
> After the system restores the devices, the system shuts down.  Is
> this
> expected design behivour?  If it is, would you help to give the
> design
> reasons in short ? From my point of view, since the deivces are
> resumed, why dose the system go to shut down state ? And also, after
> the system shuts down , and when the system is waked up by power key,
> the devices will be resumed again when restoring the saved system's
> image.
> 

We are looking forward to your reply.

> > > +                       swsusp_unmark();
> > > +                       events_check_enabled = false;
> > > +                       pr_err("Hibernation Abort.\n");
> > > +                       return;
> > > +               }
> > >                 fallthrough;
> > >         case HIBERNATION_SHUTDOWN:
> > >                 if (kernel_can_power_off())
> > > --
> > > 2.17.0
> > > 

Thanks!