linux-kernel - Re: [PATCH v1 2/2] PM: hibernate: Combine return paths in power

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e47c4c32-faea-4b24-a77e-9a3ec0e2b757@kernel.org>
Date: Fri, 26 Sep 2025 13:29:28 -0500
From: Mario Limonciello <superm1@...nel.org>
To: "Rafael J. Wysocki" <rafael@...nel.org>,
 Linux PM <linux-pm@...r.kernel.org>
Cc: LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 2/2] PM: hibernate: Combine return paths in
 power_down()

On 9/26/25 11:41 AM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> 
> To avoid code duplication and improve clarity, combine the code
> paths in power_down() leading to a return from that function.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>

The actual change looks fine to me.

Since it's used in "both" a success and failure path would you consider 
using "wakeup" for the label instead of "rollback"?  Or anything else 
you can think of that doesn't have a connotation of failure.

Reviewed-by: Mario Limonciello (AMD) <superm1@...nel.org>

> ---
>   kernel/power/hibernate.c |   32 ++++++++++++++------------------
>   1 file changed, 14 insertions(+), 18 deletions(-)
> 
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -708,21 +708,11 @@ static void power_down(void)
>   	if (hibernation_mode == HIBERNATION_SUSPEND) {
>   		pm_restore_gfp_mask();
>   		error = suspend_devices_and_enter(mem_sleep_current);
> -		if (error) {
> -			hibernation_mode = hibernation_ops ?
> -						HIBERNATION_PLATFORM :
> -						HIBERNATION_SHUTDOWN;
> -		} else {
> -			/* Match pm_restore_gfp_mask() call in hibernate() */
> -			pm_restrict_gfp_mask();
> +		if (!error)
> +			goto rollback;
>   
> -			/* Restore swap signature. */
> -			error = swsusp_unmark();
> -			if (error)
> -				pr_err("Swap will be unusable! Try swapon -a.\n");
> -
> -			return;
> -		}
> +		hibernation_mode = hibernation_ops ? HIBERNATION_PLATFORM :
> +						     HIBERNATION_SHUTDOWN;
>   	}
>   #endif
>   
> @@ -733,12 +723,9 @@ static void power_down(void)
>   	case HIBERNATION_PLATFORM:
>   		error = hibernation_platform_enter();
>   		if (error == -EAGAIN || error == -EBUSY) {
> -			/* Match pm_restore_gfp_mask() in hibernate(). */
> -			pm_restrict_gfp_mask();
> -			swsusp_unmark();
>   			events_check_enabled = false;
>   			pr_info("Wakeup event detected during hibernation, rolling back.\n");
> -			return;
> +			goto rollback;
>   		}
>   		fallthrough;
>   	case HIBERNATION_SHUTDOWN:
> @@ -757,6 +744,15 @@ static void power_down(void)
>   	pr_crit("Power down manually\n");
>   	while (1)
>   		cpu_relax();
> +
> +rollback:
> +	/* Match the pm_restore_gfp_mask() call in hibernate(). */
> +	pm_restrict_gfp_mask();
> +
> +	/* Restore swap signature. */
> +	error = swsusp_unmark();
> +	if (error)
> +		pr_err("Swap will be unusable! Try swapon -a.\n");
>   }
>   
>   static int load_image_and_restore(void)
> 
> 
>