Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220629011146.299521-1-xiongxin@kylinos.cn> <20220629011146.299521-3-xiongxin@kylinos.cn>
In-Reply-To: <20220629011146.299521-3-xiongxin@kylinos.cn>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Tue, 12 Jul 2022 20:53:59 +0200
Message-ID: <CAJZ5v0hURC-mhHszh=BhrGYfP9hwCs_nTQVxmtnV4Ot2yuajLA@mail.gmail.com>
Subject: Re: [PATCH -next 2/2] PM: suspend: advanced pm_wakeup_clear() for
 normal suspend/hibernate
To:     xiongxin <xiongxin@kylinos.cn>
Cc:     "Rafael J. Wysocki" <rafael@kernel.org>,
        Len Brown <len.brown@intel.com>, Pavel Machek <pavel@ucw.cz>,
        Riwen Lu <luriwen@kylinos.cn>,
        Linux PM <linux-pm@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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