[PATCH -next 0/2] PM: suspend: Optimized suspend is fail returned by wakeup

[PATCH -next 0/2] PM: suspend: Optimized suspend is fail returned by wakeup

[xiongxin]

When the suspend process is executed from the /sys/power/state entry,
the pm_wakeup_clear() signal is cleared in advance, and the wakeup
signal can be captured to fail the suspend process when the suspend
process is notified by the notifier;

Expanding the scope of the pm_suspend_target_state variable also allows
the device driver to know that the system has entered the suspend
process earlier.

xiongxin (2):
  PM: suspend: expand the assignment scope of the
    pm_suspend_target_state
  PM: suspend: advanced pm_wakeup_clear() for normal suspend/hibernate

 kernel/power/process.c |  5 ++++-
 kernel/power/suspend.c | 13 +++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

[PATCH -next 1/2] PM: suspend: expand the assignment scope of the pm_suspend_target_state

[xiongxin]

The pm_suspend_target_state variable can be used as a suspend state
identifier and given to the specific device driver as a code judgment;

Because the suspend_prepare() function is time-consuming for the
operation of freezing processes, and this stage is actually in the suspend
stage, it is necessary to expand the scope of the pm_suspend_target_state
variable to be assigned to the suspend state at enter_state() function;

Another reason is that the specific device driver can locate whether it is
in the suspend state based on this variable, so as to determine the
validity of its wake-up source.

Signed-off-by: xiongxin <xiongxin@kylinos.cn>
---
 kernel/power/suspend.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 827075944d28..c754b084ec03 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -564,6 +564,12 @@ static int enter_state(suspend_state_t state)
 	if (state == PM_SUSPEND_TO_IDLE)
 		s2idle_begin();
 
+	/*
+	 * Expand the scope of suspend state for suspend operations
+	 * performed from the /sys/power/state entry.
+	 */
+	pm_suspend_target_state = state;
+
 	if (sync_on_suspend_enabled) {
 		trace_suspend_resume(TPS("sync_filesystems"), 0, true);
 		ksys_sync_helper();
@@ -590,6 +596,7 @@ static int enter_state(suspend_state_t state)
 	pm_pr_dbg("Finishing wakeup.\n");
 	suspend_finish();
  Unlock:
+	pm_suspend_target_state = PM_SUSPEND_ON;
 	mutex_unlock(&system_transition_mutex);
 	return error;
 }
-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

[PATCH -next 2/2] PM: suspend: advanced pm_wakeup_clear() for normal suspend/hibernate

[xiongxin]

pm_wakeup_clear() will clear the wakeup source, which can ensure that it
is not disturbed by useless wakeup signals when doing suspend/hibernate;

At the beginning of the suspend/hibernate process, the notifier
mechanism is used to notify other device drivers. This action is
time-consuming (second-level time-consuming). If the process fails due
to the received wakeup signal during the execution of these functions,
it can better improve the experience of failing suspend/hibernate
returns;

Therefore, it is recommended here that for the suspend/hibernate process
normally called from /sys/power/state, the pm_wakeup_clear() function
should be brought before the notifier call; for the freeze_process()
function called from other places, the original logic is kept;

The pm_suspend_target_state variable is used here to identify whether the
suspend process is going normally.

Signed-off-by: xiongxin <xiongxin@kylinos.cn>
---
 kernel/power/process.c | 5 ++++-
 kernel/power/suspend.c | 6 ++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/kernel/power/process.c b/kernel/power/process.c
index 3068601e585a..3fde0240b3d1 100644
--- a/kernel/power/process.c
+++ b/kernel/power/process.c
@@ -131,7 +131,10 @@ int freeze_processes(void)
 	if (!pm_freezing)
 		atomic_inc(&system_freezing_cnt);
 
-	pm_wakeup_clear(0);
+	if (pm_suspend_target_state != PM_SUSPEND_ON)
+		pm_wakeup_clear(1);
+	else
+		pm_wakeup_clear(0);
 	pr_info("Freezing user space processes ... ");
 	pm_freezing = true;
 	error = try_to_freeze_tasks(true);
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index c754b084ec03..f4259f6c1cc2 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -569,6 +569,12 @@ static int enter_state(suspend_state_t state)
 	 * performed from the /sys/power/state entry.
 	 */
 	pm_suspend_target_state = state;
+	/*
+	 * Put pm_wakeup_clear() before the notifier notification chain to
+	 * optimize in the suspend process, the wakeup signal can interrupt
+	 * the suspend in advance and fail to return.
+	 */
+	pm_wakeup_clear(0);
 
 	if (sync_on_suspend_enabled) {
 		trace_suspend_resume(TPS("sync_filesystems"), 0, true);
-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

Re: [PATCH -next 2/2] PM: suspend: advanced pm_wakeup_clear() for normal suspend/hibernate

[kernel test robot]

Hi xiongxin,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on next-20220628]

url:    https://github.com/intel-lab-lkp/linux/commits/xiongxin/PM-suspend-Optimized-suspend-is-fail-returned-by-wakeup/20220629-114731
base:    cb71b93c2dc36d18a8b05245973328d018272cdf
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20220629/202206291941.94EV5b6M-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/2e0bc447b95996d1757038708bd6adf613f0b936
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review xiongxin/PM-suspend-Optimized-suspend-is-fail-returned-by-wakeup/20220629-114731
        git checkout 2e0bc447b95996d1757038708bd6adf613f0b936
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=sh SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   kernel/power/process.c: In function 'freeze_processes':
>> kernel/power/process.c:134:13: error: 'pm_suspend_target_state' undeclared (first use in this function)
     134 |         if (pm_suspend_target_state != PM_SUSPEND_ON)
         |             ^~~~~~~~~~~~~~~~~~~~~~~
   kernel/power/process.c:134:13: note: each undeclared identifier is reported only once for each function it appears in


vim +/pm_suspend_target_state +134 kernel/power/process.c

   112	
   113	/**
   114	 * freeze_processes - Signal user space processes to enter the refrigerator.
   115	 * The current thread will not be frozen.  The same process that calls
   116	 * freeze_processes must later call thaw_processes.
   117	 *
   118	 * On success, returns 0.  On failure, -errno and system is fully thawed.
   119	 */
   120	int freeze_processes(void)
   121	{
   122		int error;
   123	
   124		error = __usermodehelper_disable(UMH_FREEZING);
   125		if (error)
   126			return error;
   127	
   128		/* Make sure this task doesn't get frozen */
   129		current->flags |= PF_SUSPEND_TASK;
   130	
   131		if (!pm_freezing)
   132			atomic_inc(&system_freezing_cnt);
   133	
 > 134		if (pm_suspend_target_state != PM_SUSPEND_ON)
   135			pm_wakeup_clear(1);
   136		else
   137			pm_wakeup_clear(0);
   138		pr_info("Freezing user space processes ... ");
   139		pm_freezing = true;
   140		error = try_to_freeze_tasks(true);
   141		if (!error) {
   142			__usermodehelper_set_disable_depth(UMH_DISABLED);
   143			pr_cont("done.");
   144		}
   145		pr_cont("\n");
   146		BUG_ON(in_atomic());
   147	
   148		/*
   149		 * Now that the whole userspace is frozen we need to disable
   150		 * the OOM killer to disallow any further interference with
   151		 * killable tasks. There is no guarantee oom victims will
   152		 * ever reach a point they go away we have to wait with a timeout.
   153		 */
   154		if (!error && !oom_killer_disable(msecs_to_jiffies(freeze_timeout_msecs)))
   155			error = -EBUSY;
   156	
   157		if (error)
   158			thaw_processes();
   159		return error;
   160	}
   161	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

[PATCH -next 0/2] PM: suspend: Optimized suspend is fail returned by wakeup

[xiongxin]

When the suspend process is executed from the /sys/power/state entry,
the pm_wakeup_clear() signal is cleared in advance, and the wakeup
signal can be captured to fail the suspend process when the suspend
process is notified by the notifier;

Expanding the scope of the pm_suspend_target_state variable also allows
the device driver to know that the system has entered the suspend
process earlier.

xiongxin (2):
  PM: suspend: expand the assignment scope of the
    pm_suspend_target_state
  PM: suspend: advanced pm_wakeup_clear() for normal suspend/hibernate

 kernel/power/process.c |  5 ++++-
 kernel/power/suspend.c | 13 +++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

[PATCH -next 2/2] PM: suspend: advanced pm_wakeup_clear() for normal suspend/hibernate

[xiongxin]

pm_wakeup_clear() will clear the wakeup source, which can ensure that it
is not disturbed by useless wakeup signals when doing suspend/hibernate;

At the beginning of the suspend/hibernate process, the notifier
mechanism is used to notify other device drivers. This action is
time-consuming (second-level time-consuming). If the process fails due
to the received wakeup signal during the execution of these functions,
it can better improve the experience of failing suspend/hibernate
returns;

Therefore, it is recommended here that for the suspend/hibernate process
normally called from /sys/power/state, the pm_wakeup_clear() function
should be brought before the notifier call; for the freeze_process()
function called from other places, the original logic is kept;

The pm_suspend_target_state variable is used here to identify whether the
suspend process is going normally.

Signed-off-by: xiongxin <xiongxin@kylinos.cn>
---
 kernel/power/process.c | 5 ++++-
 kernel/power/suspend.c | 6 ++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/kernel/power/process.c b/kernel/power/process.c
index 3068601e585a..3fde0240b3d1 100644
--- a/kernel/power/process.c
+++ b/kernel/power/process.c
@@ -131,7 +131,10 @@ int freeze_processes(void)
 	if (!pm_freezing)
 		atomic_inc(&system_freezing_cnt);
 
-	pm_wakeup_clear(0);
+	if (pm_suspend_target_state != PM_SUSPEND_ON)
+		pm_wakeup_clear(1);
+	else
+		pm_wakeup_clear(0);
 	pr_info("Freezing user space processes ... ");
 	pm_freezing = true;
 	error = try_to_freeze_tasks(true);
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index c754b084ec03..f4259f6c1cc2 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -569,6 +569,12 @@ static int enter_state(suspend_state_t state)
 	 * performed from the /sys/power/state entry.
 	 */
 	pm_suspend_target_state = state;
+	/*
+	 * Put pm_wakeup_clear() before the notifier notification chain to
+	 * optimize in the suspend process, the wakeup signal can interrupt
+	 * the suspend in advance and fail to return.
+	 */
+	pm_wakeup_clear(0);
 
 	if (sync_on_suspend_enabled) {
 		trace_suspend_resume(TPS("sync_filesystems"), 0, true);
-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

Re: [PATCH -next 2/2] PM: suspend: advanced pm_wakeup_clear() for normal suspend/hibernate

[Rafael J. Wysocki]

On Wed, Jun 29, 2022 at 5:35 AM xiongxin <xiongxin@kylinos.cn> wrote:
>
> pm_wakeup_clear() will clear the wakeup source, which can ensure that it
> is not disturbed by useless wakeup signals when doing suspend/hibernate;
>
> At the beginning of the suspend/hibernate process, the notifier
> mechanism is used to notify other device drivers. This action is
> time-consuming (second-level time-consuming). If the process fails due
> to the received wakeup signal during the execution of these functions,
> it can better improve the experience of failing suspend/hibernate
> returns;
>
> Therefore, it is recommended here that for the suspend/hibernate process
> normally called from /sys/power/state, the pm_wakeup_clear() function
> should be brought before the notifier call; for the freeze_process()
> function called from other places, the original logic is kept;
>
> The pm_suspend_target_state variable is used here to identify whether the
> suspend process is going normally.

You seem to be arguing that the previous wakeup IRQ should be cleared
before invoking PM notifiers.  However, it is only set in
pm_system_irq_wakeup() which is only called after
suspend_device_irqs(), so clearing it anywhere before calling that
function in the given cycle should be sufficient.

> Signed-off-by: xiongxin <xiongxin@kylinos.cn>
> ---
>  kernel/power/process.c | 5 ++++-
>  kernel/power/suspend.c | 6 ++++++
>  2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/power/process.c b/kernel/power/process.c
> index 3068601e585a..3fde0240b3d1 100644
> --- a/kernel/power/process.c
> +++ b/kernel/power/process.c
> @@ -131,7 +131,10 @@ int freeze_processes(void)
>         if (!pm_freezing)
>                 atomic_inc(&system_freezing_cnt);
>
> -       pm_wakeup_clear(0);
> +       if (pm_suspend_target_state != PM_SUSPEND_ON)
> +               pm_wakeup_clear(1);

This doesn't make sense.

> +       else
> +               pm_wakeup_clear(0);
>         pr_info("Freezing user space processes ... ");
>         pm_freezing = true;
>         error = try_to_freeze_tasks(true);
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index c754b084ec03..f4259f6c1cc2 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -569,6 +569,12 @@ static int enter_state(suspend_state_t state)
>          * performed from the /sys/power/state entry.
>          */
>         pm_suspend_target_state = state;
> +       /*
> +        * Put pm_wakeup_clear() before the notifier notification chain to
> +        * optimize in the suspend process, the wakeup signal can interrupt
> +        * the suspend in advance and fail to return.
> +        */

The comment above is too hard to understand for me, sorry.

> +       pm_wakeup_clear(0);
>
>         if (sync_on_suspend_enabled) {
>                 trace_suspend_resume(TPS("sync_filesystems"), 0, true);
> --
> 2.25.1
>
>
> No virus found
>                 Checked by Hillstone Network AntiVirus