| Message ID | 20231024091447.108072-1-chris.feng@mediatek.com |
|---|---|
| State | New |
| Headers | show |
| Series | PM: hibernate: Fix the bug where wake events cannot wake system during hibernation | expand |
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 > >
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!
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) { + swsusp_unmark(); + events_check_enabled = false; + pr_err("Hibernation Abort.\n"); + return; + } fallthrough; case HIBERNATION_SHUTDOWN: if (kernel_can_power_off())