diff mbox series

PM: hibernate: Get test_resume hibernation mode working again

Message ID 20230405034502.583304-1-quic_pkondeti@quicinc.com (mailing list archive)
State Superseded, archived
Headers show
Series PM: hibernate: Get test_resume hibernation mode working again | expand

Commit Message

Pavan Kondeti April 5, 2023, 3:45 a.m. UTC
Commit 39fbef4b0f77 ("PM: hibernate: Get block device exclusively in
swsusp_check()") changed the opening mode of the block device to
(FMODE_READ | FMODE_EXCL) during resume. This breaks the test_resume
hibernation mode as the block device is not available for exclusive
open. Because the block device is configured as swap during hibernation.

Fix this issue by opening the device in FMODE_READ only in test_resume
restore path. Cache the flags used in opening the block device so that
callers of swsusp_close() does not need to worry about test_resume vs
normal restore.

Fixes: 39fbef4b0f77 ("PM: hibernate: Get block device exclusively in swsusp_check()")
Signed-off-by: Pavankumar Kondeti <quic_pkondeti@quicinc.com>
---
 kernel/power/hibernate.c | 21 +++++++++++-----
 kernel/power/power.h     |  4 +--
 kernel/power/swap.c      | 53 +++++++++++++++++++++++-----------------
 3 files changed, 48 insertions(+), 30 deletions(-)

Comments

Chen Yu April 5, 2023, 4:43 a.m. UTC | #1
Hi Pavankumar,
On 2023-04-05 at 09:15:02 +0530, Pavankumar Kondeti wrote:
> Commit 39fbef4b0f77 ("PM: hibernate: Get block device exclusively in
> swsusp_check()") changed the opening mode of the block device to
> (FMODE_READ | FMODE_EXCL) during resume. This breaks the test_resume
> hibernation mode as the block device is not available for exclusive
> open. Because the block device is configured as swap during hibernation.
> 
> Fix this issue by opening the device in FMODE_READ only in test_resume
> restore path. Cache the flags used in opening the block device so that
> callers of swsusp_close() does not need to worry about test_resume vs
> normal restore.
> 
> Fixes: 39fbef4b0f77 ("PM: hibernate: Get block device exclusively in swsusp_check()")
> Signed-off-by: Pavankumar Kondeti <quic_pkondeti@quicinc.com>
> ---
>  kernel/power/hibernate.c | 21 +++++++++++-----
>  kernel/power/power.h     |  4 +--
>  kernel/power/swap.c      | 53 +++++++++++++++++++++++-----------------
>  3 files changed, 48 insertions(+), 30 deletions(-)
>
Thanks for taking a look at this, would the following fix make sense?
https://lore.kernel.org/lkml/20230401165540.322665-1-yu.c.chen@intel.com/

thanks,
Chenyu
Pavan Kondeti April 5, 2023, 7:02 a.m. UTC | #2
On Wed, Apr 05, 2023 at 12:43:03PM +0800, Chen Yu wrote:
> Hi Pavankumar,
> On 2023-04-05 at 09:15:02 +0530, Pavankumar Kondeti wrote:
> > Commit 39fbef4b0f77 ("PM: hibernate: Get block device exclusively in
> > swsusp_check()") changed the opening mode of the block device to
> > (FMODE_READ | FMODE_EXCL) during resume. This breaks the test_resume
> > hibernation mode as the block device is not available for exclusive
> > open. Because the block device is configured as swap during hibernation.
> > 
> > Fix this issue by opening the device in FMODE_READ only in test_resume
> > restore path. Cache the flags used in opening the block device so that
> > callers of swsusp_close() does not need to worry about test_resume vs
> > normal restore.
> > 
> > Fixes: 39fbef4b0f77 ("PM: hibernate: Get block device exclusively in swsusp_check()")
> > Signed-off-by: Pavankumar Kondeti <quic_pkondeti@quicinc.com>
> > ---
> >  kernel/power/hibernate.c | 21 +++++++++++-----
> >  kernel/power/power.h     |  4 +--
> >  kernel/power/swap.c      | 53 +++++++++++++++++++++++-----------------
> >  3 files changed, 48 insertions(+), 30 deletions(-)
> >
> Thanks for taking a look at this, would the following fix make sense?
> https://lore.kernel.org/lkml/20230401165540.322665-1-yu.c.chen@intel.com/
> 

Thanks Chenyu for pointing it out.

Yes, your patch works. It looks good also. I left a minor comment. You
can probably clarify what "safe" means there either by a comment or
changing the variable name. I also left an alternative approach, I initially
started with.

Thanks,
Pavan
diff mbox series

Patch

diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 793c55a2becb..d78edf32a45c 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -693,12 +693,12 @@  static int load_image_and_restore(void)
 	lock_device_hotplug();
 	error = create_basic_memory_bitmaps();
 	if (error) {
-		swsusp_close(FMODE_READ | FMODE_EXCL);
+		swsusp_close();
 		goto Unlock;
 	}
 
 	error = swsusp_read(&flags);
-	swsusp_close(FMODE_READ | FMODE_EXCL);
+	swsusp_close();
 	if (!error)
 		error = hibernation_restore(flags & SF_PLATFORM_MODE);
 
@@ -785,7 +785,12 @@  int hibernate(void)
 	unlock_device_hotplug();
 	if (snapshot_test) {
 		pm_pr_dbg("Checking hibernation image\n");
-		error = swsusp_check();
+		/*
+		 * Block device assigned for storing hibernation image
+		 * is already configured as swap. So exclusive mode
+		 * is not needed. see comments in software_resume().
+		 */
+		error = swsusp_check(FMODE_READ);
 		if (!error)
 			error = load_image_and_restore();
 	}
@@ -983,14 +988,18 @@  static int software_resume(void)
 		MAJOR(swsusp_resume_device), MINOR(swsusp_resume_device));
 
 	pm_pr_dbg("Looking for hibernation image.\n");
-	error = swsusp_check();
+	/*
+	 * Exclusively get the block device that contains the hibernation
+	 * image so that we can bail out if the block device is mounted.
+	 */
+	error = swsusp_check(FMODE_READ | FMODE_EXCL);
 	if (error)
 		goto Unlock;
 
 	/* The snapshot device should not be opened while we're running */
 	if (!hibernate_acquire()) {
 		error = -EBUSY;
-		swsusp_close(FMODE_READ | FMODE_EXCL);
+		swsusp_close();
 		goto Unlock;
 	}
 
@@ -1025,7 +1034,7 @@  static int software_resume(void)
 	pm_pr_dbg("Hibernation image not present or could not be loaded.\n");
 	return error;
  Close_Finish:
-	swsusp_close(FMODE_READ | FMODE_EXCL);
+	swsusp_close();
 	goto Finish;
 }
 
diff --git a/kernel/power/power.h b/kernel/power/power.h
index b4f433943209..9877f33cc966 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -173,11 +173,11 @@  extern int swsusp_swap_in_use(void);
 #define SF_HW_SIG		8
 
 /* kernel/power/hibernate.c */
-extern int swsusp_check(void);
+extern int swsusp_check(fmode_t mode);
 extern void swsusp_free(void);
 extern int swsusp_read(unsigned int *flags_p);
 extern int swsusp_write(unsigned int flags);
-extern void swsusp_close(fmode_t);
+extern void swsusp_close(void);
 #ifdef CONFIG_SUSPEND
 extern int swsusp_unmark(void);
 #endif
diff --git a/kernel/power/swap.c b/kernel/power/swap.c
index 277434b6c0bf..005c0cfe116a 100644
--- a/kernel/power/swap.c
+++ b/kernel/power/swap.c
@@ -338,6 +338,28 @@  static int mark_swapfiles(struct swap_map_handle *handle, unsigned int flags)
 	return error;
 }
 
+static fmode_t hib_resume_bdev_mode;
+static int swsusp_setup_bdev(fmode_t mode)
+{
+	struct block_device *bdev;
+	void *holder;
+	int error;
+
+	bdev = blkdev_get_by_dev(swsusp_resume_device, mode, &holder);
+	if (IS_ERR(bdev))
+		return PTR_ERR(bdev);
+
+	error = set_blocksize(bdev, PAGE_SIZE);
+	if (error) {
+		blkdev_put(bdev, mode);
+		return error;
+	}
+
+	hib_resume_bdev = bdev;
+	hib_resume_bdev_mode = mode;
+	return 0;
+}
+
 /**
  *	swsusp_swap_check - check if the resume device is a swap device
  *	and get its index (if so)
@@ -356,15 +378,7 @@  static int swsusp_swap_check(void)
 		return res;
 	root_swap = res;
 
-	hib_resume_bdev = blkdev_get_by_dev(swsusp_resume_device, FMODE_WRITE,
-			NULL);
-	if (IS_ERR(hib_resume_bdev))
-		return PTR_ERR(hib_resume_bdev);
-
-	res = set_blocksize(hib_resume_bdev, PAGE_SIZE);
-	if (res < 0)
-		blkdev_put(hib_resume_bdev, FMODE_WRITE);
-
+	res = swsusp_setup_bdev(FMODE_WRITE);
 	return res;
 }
 
@@ -443,7 +457,7 @@  static int get_swap_writer(struct swap_map_handle *handle)
 err_rel:
 	release_swap_writer(handle);
 err_close:
-	swsusp_close(FMODE_WRITE);
+	swsusp_close();
 	return ret;
 }
 
@@ -508,7 +522,7 @@  static int swap_writer_finish(struct swap_map_handle *handle,
 	if (error)
 		free_all_swap_pages(root_swap);
 	release_swap_writer(handle);
-	swsusp_close(FMODE_WRITE);
+	swsusp_close();
 
 	return error;
 }
@@ -1514,15 +1528,12 @@  int swsusp_read(unsigned int *flags_p)
  *      swsusp_check - Check for swsusp signature in the resume device
  */
 
-int swsusp_check(void)
+int swsusp_check(fmode_t mode)
 {
 	int error;
-	void *holder;
 
-	hib_resume_bdev = blkdev_get_by_dev(swsusp_resume_device,
-					    FMODE_READ | FMODE_EXCL, &holder);
-	if (!IS_ERR(hib_resume_bdev)) {
-		set_blocksize(hib_resume_bdev, PAGE_SIZE);
+	error = swsusp_setup_bdev(mode);
+	if (!error) {
 		clear_page(swsusp_header);
 		error = hib_submit_io(REQ_OP_READ, swsusp_resume_block,
 					swsusp_header, NULL);
@@ -1547,11 +1558,9 @@  int swsusp_check(void)
 
 put:
 		if (error)
-			blkdev_put(hib_resume_bdev, FMODE_READ | FMODE_EXCL);
+			blkdev_put(hib_resume_bdev, mode);
 		else
 			pr_debug("Image signature found, resuming\n");
-	} else {
-		error = PTR_ERR(hib_resume_bdev);
 	}
 
 	if (error)
@@ -1564,14 +1573,14 @@  int swsusp_check(void)
  *	swsusp_close - close swap device.
  */
 
-void swsusp_close(fmode_t mode)
+void swsusp_close(void)
 {
 	if (IS_ERR(hib_resume_bdev)) {
 		pr_debug("Image device not initialised\n");
 		return;
 	}
 
-	blkdev_put(hib_resume_bdev, mode);
+	blkdev_put(hib_resume_bdev, hib_resume_bdev_mode);
 }
 
 /**