Re: [PATCH 04/24] PM: hibernate: move finding the resume device out of software_resume

From: "Rafael J. Wysocki" <rafael@kernel.org>
To: Christoph Hellwig <hch@lst.de>
Cc: Jens Axboe <axboe@kernel.dk>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Mike Snitzer <snitzer@kernel.org>,
	Joern Engel <joern@lazybastard.org>,
	Miquel Raynal <miquel.raynal@bootlin.com>,
	Richard Weinberger <richard@nod.at>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Pavel Machek <pavel@ucw.cz>,
	dm-devel@redhat.com, linux-kernel@vger.kernel.org,
	linux-block@vger.kernel.org, linux-mtd@lists.infradead.org,
	linux-pm@vger.kernel.org
Subject: Re: [PATCH 04/24] PM: hibernate: move finding the resume device out of software_resume
Date: Tue, 23 May 2023 20:33:44 +0200	[thread overview]
Message-ID: <CAJZ5v0jrj3PaC5oZt22DQoJARUcpXaerS-Cmx+34du7=p9WDSw@mail.gmail.com> (raw)
In-Reply-To: <20230523074535.249802-5-hch@lst.de>

On Tue, May 23, 2023 at 9:45 AM Christoph Hellwig <hch@lst.de> wrote:
>
> software_resume can be called either from an init call in the boot code,
> or from sysfs once the system has finished booting, and the two
> invocation methods this can't race with each other.
>
> For the latter case we did just parse the suspend device manually, while
> the former might not have one.  Split software_resume so that the search
> only happens for the boot case, which also means the special lockdep
> nesting annotation can go away as the system transition mutex can be
> taken a little later and doesn't have the sysfs locking nest inside it.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Acked-by: Rafael J. Wysocki <rafael@kernel.org>

> ---
>  kernel/power/hibernate.c | 80 ++++++++++++++++++++--------------------
>  1 file changed, 39 insertions(+), 41 deletions(-)
>
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index 78696aa04f5ca3..45e24b02cd50b6 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -907,7 +907,7 @@ int hibernate_quiet_exec(int (*func)(void *data), void *data)
>  }
>  EXPORT_SYMBOL_GPL(hibernate_quiet_exec);
>
> -static int find_resume_device(void)
> +static int __init find_resume_device(void)
>  {
>         if (!strlen(resume_file))
>                 return -ENOENT;
> @@ -942,53 +942,16 @@ static int find_resume_device(void)
>         return 0;
>  }
>
> -/**
> - * software_resume - Resume from a saved hibernation image.
> - *
> - * This routine is called as a late initcall, when all devices have been
> - * discovered and initialized already.
> - *
> - * The image reading code is called to see if there is a hibernation image
> - * available for reading.  If that is the case, devices are quiesced and the
> - * contents of memory is restored from the saved image.
> - *
> - * If this is successful, control reappears in the restored target kernel in
> - * hibernation_snapshot() which returns to hibernate().  Otherwise, the routine
> - * attempts to recover gracefully and make the kernel return to the normal mode
> - * of operation.
> - */
>  static int software_resume(void)
>  {
>         int error;
>
> -       /*
> -        * If the user said "noresume".. bail out early.
> -        */
> -       if (noresume || !hibernation_available())
> -               return 0;
> -
> -       /*
> -        * name_to_dev_t() below takes a sysfs buffer mutex when sysfs
> -        * is configured into the kernel. Since the regular hibernate
> -        * trigger path is via sysfs which takes a buffer mutex before
> -        * calling hibernate functions (which take system_transition_mutex)
> -        * this can cause lockdep to complain about a possible ABBA deadlock
> -        * which cannot happen since we're in the boot code here and
> -        * sysfs can't be invoked yet. Therefore, we use a subclass
> -        * here to avoid lockdep complaining.
> -        */
> -       mutex_lock_nested(&system_transition_mutex, SINGLE_DEPTH_NESTING);
> -
> -       if (!swsusp_resume_device) {
> -               error = find_resume_device();
> -               if (error)
> -                       goto Unlock;
> -       }
> -
>         pm_pr_dbg("Hibernation image partition %d:%d present\n",
>                 MAJOR(swsusp_resume_device), MINOR(swsusp_resume_device));
>
>         pm_pr_dbg("Looking for hibernation image.\n");
> +
> +       mutex_lock(&system_transition_mutex);
>         error = swsusp_check(false);
>         if (error)
>                 goto Unlock;
> @@ -1035,7 +998,39 @@ static int software_resume(void)
>         goto Finish;
>  }
>
> -late_initcall_sync(software_resume);
> +/**
> + * software_resume_initcall - Resume from a saved hibernation image.
> + *
> + * This routine is called as a late initcall, when all devices have been
> + * discovered and initialized already.
> + *
> + * The image reading code is called to see if there is a hibernation image
> + * available for reading.  If that is the case, devices are quiesced and the
> + * contents of memory is restored from the saved image.
> + *
> + * If this is successful, control reappears in the restored target kernel in
> + * hibernation_snapshot() which returns to hibernate().  Otherwise, the routine
> + * attempts to recover gracefully and make the kernel return to the normal mode
> + * of operation.
> + */
> +static int __init software_resume_initcall(void)
> +{
> +       /*
> +        * If the user said "noresume".. bail out early.
> +        */
> +       if (noresume || !hibernation_available())
> +               return 0;
> +
> +       if (!swsusp_resume_device) {
> +               int error = find_resume_device();
> +
> +               if (error)
> +                       return error;
> +       }
> +
> +       return software_resume();
> +}
> +late_initcall_sync(software_resume_initcall);
>
>
>  static const char * const hibernation_modes[] = {
> @@ -1176,6 +1171,9 @@ static ssize_t resume_store(struct kobject *kobj, struct kobj_attribute *attr,
>         char *name;
>         dev_t res;
>
> +       if (!hibernation_available())
> +               return 0;
> +
>         if (len && buf[len-1] == '\n')
>                 len--;
>         name = kstrndup(buf, len, GFP_KERNEL);
> --
> 2.39.2
>