diff mbox series

[2/2] PM/hibernate: remove the bogus call to get_gendisk in software_resume

Message ID 20200925161447.1486883-3-hch@lst.de
State Accepted
Commit 428805c0c5e76ef643b1fbc893edfb636b3d8aef
Headers show
Series None | expand

Commit Message

Christoph Hellwig Sept. 25, 2020, 4:14 p.m. UTC
get_gendisk grabs a reference on the disk and file operation, so this
code will leak both of them while having absolutely no use for the
gendisk itself.

This effectively reverts commit 2df83fa4bce421f
("PM / Hibernate: Use get_gendisk to verify partition if resume_file is integer format")

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 kernel/power/hibernate.c | 11 -----------
 1 file changed, 11 deletions(-)

Comments

Pavel Machek Sept. 25, 2020, 6:38 p.m. UTC | #1
Hi!

> get_gendisk grabs a reference on the disk and file operation, so this
> code will leak both of them while having absolutely no use for the
> gendisk itself.
> 
> This effectively reverts commit 2df83fa4bce421f
> ("PM / Hibernate: Use get_gendisk to verify partition if resume_file is integer format")
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  kernel/power/hibernate.c | 11 -----------
>  1 file changed, 11 deletions(-)
> 
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index e7aa57fb2fdc33..7d0b99d2e69631 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -948,17 +948,6 @@ static int software_resume(void)
>  
>  	/* Check if the device is there */
>  	swsusp_resume_device = name_to_dev_t(resume_file);
> -
> -	/*
> -	 * name_to_dev_t is ineffective to verify parition if resume_file is in
> -	 * integer format. (e.g. major:minor)
> -	 */
> -	if (isdigit(resume_file[0]) && resume_wait) {
> -		int partno;
> -		while (!get_gendisk(swsusp_resume_device, &partno))
> -			msleep(10);
> -	}

I believe point of this code was to wait for resume device to appear
-- see the resume_wait condition. It should not be simply removed.

Best regards,
								Pavel
Christoph Hellwig Sept. 26, 2020, 2:05 p.m. UTC | #2
On Fri, Sep 25, 2020 at 08:38:28PM +0200, Pavel Machek wrote:
> > -	 * name_to_dev_t is ineffective to verify parition if resume_file is in

> > -	 * integer format. (e.g. major:minor)

> > -	 */

> > -	if (isdigit(resume_file[0]) && resume_wait) {

> > -		int partno;

> > -		while (!get_gendisk(swsusp_resume_device, &partno))

> > -			msleep(10);

> > -	}

> 

> I believe point of this code was to wait for resume device to appear

> -- see the resume_wait condition. It should not be simply removed.


But get_gendisk has absolutely no relation to a device appearing.  So
whatever this code tried to do doesn't make any sense.
Rafael J. Wysocki Sept. 30, 2020, 3:45 p.m. UTC | #3
On Fri, Sep 25, 2020 at 6:15 PM Christoph Hellwig <hch@lst.de> wrote:
>

> get_gendisk grabs a reference on the disk and file operation, so this

> code will leak both of them while having absolutely no use for the

> gendisk itself.

>

> This effectively reverts commit 2df83fa4bce421f

> ("PM / Hibernate: Use get_gendisk to verify partition if resume_file is integer format")

>

> Signed-off-by: Christoph Hellwig <hch@lst.de>


Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>


> ---

>  kernel/power/hibernate.c | 11 -----------

>  1 file changed, 11 deletions(-)

>

> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c

> index e7aa57fb2fdc33..7d0b99d2e69631 100644

> --- a/kernel/power/hibernate.c

> +++ b/kernel/power/hibernate.c

> @@ -948,17 +948,6 @@ static int software_resume(void)

>

>         /* Check if the device is there */

>         swsusp_resume_device = name_to_dev_t(resume_file);

> -

> -       /*

> -        * name_to_dev_t is ineffective to verify parition if resume_file is in

> -        * integer format. (e.g. major:minor)

> -        */

> -       if (isdigit(resume_file[0]) && resume_wait) {

> -               int partno;

> -               while (!get_gendisk(swsusp_resume_device, &partno))

> -                       msleep(10);

> -       }

> -

>         if (!swsusp_resume_device) {

>                 /*

>                  * Some device discovery might still be in progress; we need

> --

> 2.28.0

>
Christoph Hellwig Oct. 2, 2020, 6:50 a.m. UTC | #4
On Wed, Sep 30, 2020 at 05:45:27PM +0200, Rafael J. Wysocki wrote:
> On Fri, Sep 25, 2020 at 6:15 PM Christoph Hellwig <hch@lst.de> wrote:
> >
> > get_gendisk grabs a reference on the disk and file operation, so this
> > code will leak both of them while having absolutely no use for the
> > gendisk itself.
> >
> > This effectively reverts commit 2df83fa4bce421f
> > ("PM / Hibernate: Use get_gendisk to verify partition if resume_file is integer format")
> >
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Can you pick it up through the PM tree?  The big rework in this area
I have planned won't land before 5.11 anyway.
Rafael J. Wysocki Oct. 2, 2020, 2:11 p.m. UTC | #5
On Fri, Oct 2, 2020 at 8:50 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Wed, Sep 30, 2020 at 05:45:27PM +0200, Rafael J. Wysocki wrote:
> > On Fri, Sep 25, 2020 at 6:15 PM Christoph Hellwig <hch@lst.de> wrote:
> > >
> > > get_gendisk grabs a reference on the disk and file operation, so this
> > > code will leak both of them while having absolutely no use for the
> > > gendisk itself.
> > >
> > > This effectively reverts commit 2df83fa4bce421f
> > > ("PM / Hibernate: Use get_gendisk to verify partition if resume_file is integer format")
> > >
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> >
> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Can you pick it up through the PM tree?  The big rework in this area
> I have planned won't land before 5.11 anyway.

Will do, thanks!
diff mbox series

Patch

diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index e7aa57fb2fdc33..7d0b99d2e69631 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -948,17 +948,6 @@  static int software_resume(void)
 
 	/* Check if the device is there */
 	swsusp_resume_device = name_to_dev_t(resume_file);
-
-	/*
-	 * name_to_dev_t is ineffective to verify parition if resume_file is in
-	 * integer format. (e.g. major:minor)
-	 */
-	if (isdigit(resume_file[0]) && resume_wait) {
-		int partno;
-		while (!get_gendisk(swsusp_resume_device, &partno))
-			msleep(10);
-	}
-
 	if (!swsusp_resume_device) {
 		/*
 		 * Some device discovery might still be in progress; we need