linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH RFC v2 1/1] scsi: pm: Leave runtime PM status alone during system resume/thaw/restore
@ 2020-11-20  8:37 Can Guo
  2020-11-20 16:35 ` Alan Stern
  0 siblings, 1 reply; 6+ messages in thread
From: Can Guo @ 2020-11-20  8:37 UTC (permalink / raw)
  To: asutoshd, nguyenb, hongwus, ziqichen, rnayak, linux-scsi,
	kernel-team, saravanak, salyzyn, cang
  Cc: Stanley Chu, Bart Van Assche, Alan Stern, James E.J. Bottomley,
	Martin K. Petersen, Matthias Brugger, open list,
	moderated list:ARM/Mediatek SoC support,
	moderated list:ARM/Mediatek SoC support

Runtime resume is handled by runtime PM framework, no need to forcibly
set runtime PM status to RPM_ACTIVE during system resume/thaw/restore.

Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Can Guo <cang@codeaurora.org>
---

Changes since v1:
- Incorporated Bart's comments

---
 drivers/scsi/scsi_pm.c | 24 +-----------------------
 1 file changed, 1 insertion(+), 23 deletions(-)

diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index 3717eea..908f27f 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -79,25 +79,6 @@ static int scsi_dev_type_resume(struct device *dev,
 	scsi_device_resume(to_scsi_device(dev));
 	dev_dbg(dev, "scsi resume: %d\n", err);
 
-	if (err == 0) {
-		pm_runtime_disable(dev);
-		err = pm_runtime_set_active(dev);
-		pm_runtime_enable(dev);
-
-		/*
-		 * Forcibly set runtime PM status of request queue to "active"
-		 * to make sure we can again get requests from the queue
-		 * (see also blk_pm_peek_request()).
-		 *
-		 * The resume hook will correct runtime PM status of the disk.
-		 */
-		if (!err && scsi_is_sdev_device(dev)) {
-			struct scsi_device *sdev = to_scsi_device(dev);
-
-			blk_set_runtime_active(sdev->request_queue);
-		}
-	}
-
 	return err;
 }
 
@@ -165,11 +146,8 @@ static int scsi_bus_resume_common(struct device *dev,
 		 */
 		if (strncmp(scsi_scan_type, "async", 5) != 0)
 			async_synchronize_full_domain(&scsi_sd_pm_domain);
-	} else {
-		pm_runtime_disable(dev);
-		pm_runtime_set_active(dev);
-		pm_runtime_enable(dev);
 	}
+
 	return 0;
 }
 
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH RFC v2 1/1] scsi: pm: Leave runtime PM status alone during system resume/thaw/restore
  2020-11-20  8:37 [PATCH RFC v2 1/1] scsi: pm: Leave runtime PM status alone during system resume/thaw/restore Can Guo
@ 2020-11-20 16:35 ` Alan Stern
  2020-11-21 17:00   ` Bart Van Assche
  2020-11-23  1:23   ` Can Guo
  0 siblings, 2 replies; 6+ messages in thread
From: Alan Stern @ 2020-11-20 16:35 UTC (permalink / raw)
  To: Can Guo
  Cc: asutoshd, nguyenb, hongwus, ziqichen, rnayak, linux-scsi,
	kernel-team, saravanak, salyzyn, Stanley Chu, Bart Van Assche,
	James E.J. Bottomley, Martin K. Petersen, Matthias Brugger,
	open list, moderated list:ARM/Mediatek SoC support,
	moderated list:ARM/Mediatek SoC support

On Fri, Nov 20, 2020 at 12:37:22AM -0800, Can Guo wrote:
> Runtime resume is handled by runtime PM framework, no need to forcibly
> set runtime PM status to RPM_ACTIVE during system resume/thaw/restore.

Sorry, I don't understand this explanation at all.

Sure, runtime resume is handled by the runtime PM framework.  But this 
patch changes the code for system resume, which is completely different.

Following a system resume, the hardware will be at full power.  We don't 
want the kernel to think that the device is still in runtime suspend; 
otherwise is would never put the device back into low-power mode.

Alan Stern

> Cc: Stanley Chu <stanley.chu@mediatek.com>
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Signed-off-by: Can Guo <cang@codeaurora.org>
> ---
> 
> Changes since v1:
> - Incorporated Bart's comments
> 
> ---
>  drivers/scsi/scsi_pm.c | 24 +-----------------------
>  1 file changed, 1 insertion(+), 23 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
> index 3717eea..908f27f 100644
> --- a/drivers/scsi/scsi_pm.c
> +++ b/drivers/scsi/scsi_pm.c
> @@ -79,25 +79,6 @@ static int scsi_dev_type_resume(struct device *dev,
>  	scsi_device_resume(to_scsi_device(dev));
>  	dev_dbg(dev, "scsi resume: %d\n", err);
>  
> -	if (err == 0) {
> -		pm_runtime_disable(dev);
> -		err = pm_runtime_set_active(dev);
> -		pm_runtime_enable(dev);
> -
> -		/*
> -		 * Forcibly set runtime PM status of request queue to "active"
> -		 * to make sure we can again get requests from the queue
> -		 * (see also blk_pm_peek_request()).
> -		 *
> -		 * The resume hook will correct runtime PM status of the disk.
> -		 */
> -		if (!err && scsi_is_sdev_device(dev)) {
> -			struct scsi_device *sdev = to_scsi_device(dev);
> -
> -			blk_set_runtime_active(sdev->request_queue);
> -		}
> -	}
> -
>  	return err;
>  }
>  
> @@ -165,11 +146,8 @@ static int scsi_bus_resume_common(struct device *dev,
>  		 */
>  		if (strncmp(scsi_scan_type, "async", 5) != 0)
>  			async_synchronize_full_domain(&scsi_sd_pm_domain);
> -	} else {
> -		pm_runtime_disable(dev);
> -		pm_runtime_set_active(dev);
> -		pm_runtime_enable(dev);
>  	}
> +
>  	return 0;
>  }
>  
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH RFC v2 1/1] scsi: pm: Leave runtime PM status alone during system resume/thaw/restore
  2020-11-20 16:35 ` Alan Stern
@ 2020-11-21 17:00   ` Bart Van Assche
  2020-11-21 17:32     ` Alan Stern
  2020-11-23  1:23   ` Can Guo
  1 sibling, 1 reply; 6+ messages in thread
From: Bart Van Assche @ 2020-11-21 17:00 UTC (permalink / raw)
  To: Alan Stern, Can Guo
  Cc: asutoshd, nguyenb, hongwus, ziqichen, rnayak, linux-scsi,
	kernel-team, saravanak, salyzyn, Stanley Chu,
	James E.J. Bottomley, Martin K. Petersen, Matthias Brugger,
	open list, moderated list:ARM/Mediatek SoC support,
	moderated list:ARM/Mediatek SoC support

On 11/20/20 8:35 AM, Alan Stern wrote:
> On Fri, Nov 20, 2020 at 12:37:22AM -0800, Can Guo wrote:
>> Runtime resume is handled by runtime PM framework, no need to forcibly
>> set runtime PM status to RPM_ACTIVE during system resume/thaw/restore.
> 
> Sorry, I don't understand this explanation at all.
> 
> Sure, runtime resume is handled by the runtime PM framework.  But this 
> patch changes the code for system resume, which is completely different.
> 
> Following a system resume, the hardware will be at full power.  We don't 
> want the kernel to think that the device is still in runtime suspend; 
> otherwise is would never put the device back into low-power mode.

Hi Alan,

Does this mean that every driver needs similar code for handling runtime
suspended devices upon system resume? If so, would it be possible to
move that code into the power management core (drivers/base/power)?

Thanks,

Bart.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH RFC v2 1/1] scsi: pm: Leave runtime PM status alone during system resume/thaw/restore
  2020-11-21 17:00   ` Bart Van Assche
@ 2020-11-21 17:32     ` Alan Stern
  0 siblings, 0 replies; 6+ messages in thread
From: Alan Stern @ 2020-11-21 17:32 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Can Guo, asutoshd, nguyenb, hongwus, ziqichen, rnayak,
	linux-scsi, kernel-team, saravanak, salyzyn, Stanley Chu,
	James E.J. Bottomley, Martin K. Petersen, Matthias Brugger,
	open list, moderated list:ARM/Mediatek SoC support,
	moderated list:ARM/Mediatek SoC support

On Sat, Nov 21, 2020 at 09:00:02AM -0800, Bart Van Assche wrote:
> On 11/20/20 8:35 AM, Alan Stern wrote:
> > On Fri, Nov 20, 2020 at 12:37:22AM -0800, Can Guo wrote:
> >> Runtime resume is handled by runtime PM framework, no need to forcibly
> >> set runtime PM status to RPM_ACTIVE during system resume/thaw/restore.
> > 
> > Sorry, I don't understand this explanation at all.
> > 
> > Sure, runtime resume is handled by the runtime PM framework.  But this 
> > patch changes the code for system resume, which is completely different.
> > 
> > Following a system resume, the hardware will be at full power.  We don't 
> > want the kernel to think that the device is still in runtime suspend; 
> > otherwise is would never put the device back into low-power mode.
> 
> Hi Alan,
> 
> Does this mean that every driver needs similar code for handling runtime
> suspended devices upon system resume? If so, would it be possible to
> move that code into the power management core (drivers/base/power)?

That's a complicated story.

In short, many drivers need to do this, but not all.  There is a complex 
collection of settings available for subsystems or drivers that would 
like their devices to remain in runtime system across a system sleep.

For the subsystems/drivers that don't care to deal with this complexity 
or don't have any special requirements -- yes, they all need to include 
code like this in their system-resume paths.

I had a very long discussion with Rafael Wysocki about all this starting 
last March; you can find the relevant emails beginning roughly here:

	https://marc.info/?l=linux-pm&m=158516934924947&w=2

and continuing through a few different threads.

Rafael ended up making a large number of changes to the PM core and API 
to simplify things, straighten them out, and improve the documentation.  
But we never did try to add this automatic set-runtime-active thing into 
the core.  Probably we wanted all the other changes to settle down 
before trying to do it, and then just forgot about it.  In fact, I'm not 
certain that it is possible now, but we should look into it.

Alan Stern

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH RFC v2 1/1] scsi: pm: Leave runtime PM status alone during system resume/thaw/restore
  2020-11-20 16:35 ` Alan Stern
  2020-11-21 17:00   ` Bart Van Assche
@ 2020-11-23  1:23   ` Can Guo
  2020-11-23  3:02     ` Alan Stern
  1 sibling, 1 reply; 6+ messages in thread
From: Can Guo @ 2020-11-23  1:23 UTC (permalink / raw)
  To: Alan Stern
  Cc: asutoshd, nguyenb, hongwus, ziqichen, rnayak, linux-scsi,
	kernel-team, saravanak, salyzyn, Stanley Chu, Bart Van Assche,
	James E.J. Bottomley, Martin K. Petersen, Matthias Brugger,
	open list, moderated list:ARM/Mediatek SoC support,
	moderated list:ARM/Mediatek SoC support

Hi Alan,

On 2020-11-21 00:35, Alan Stern wrote:
> On Fri, Nov 20, 2020 at 12:37:22AM -0800, Can Guo wrote:
>> Runtime resume is handled by runtime PM framework, no need to forcibly
>> set runtime PM status to RPM_ACTIVE during system resume/thaw/restore.
> 
> Sorry, I don't understand this explanation at all.
> 
> Sure, runtime resume is handled by the runtime PM framework.  But this
> patch changes the code for system resume, which is completely 
> different.
> 
> Following a system resume, the hardware will be at full power.  We 
> don't
> want the kernel to think that the device is still in runtime suspend;
> otherwise is would never put the device back into low-power mode.

How about adding below lines to the patch?

diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index 908f27f..7ebe582 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -75,9 +75,11 @@ static int scsi_dev_type_resume(struct device *dev,
         const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : 
NULL;
         int err = 0;

-       err = cb(dev, pm);
-       scsi_device_resume(to_scsi_device(dev));
-       dev_dbg(dev, "scsi resume: %d\n", err);
+       if (pm_runtime_active(dev)) {
+               err = cb(dev, pm);
+               scsi_device_resume(to_scsi_device(dev));
+               dev_dbg(dev, "scsi resume: %d\n", err);
+       }

         return err;
  }

Whenever a device is accessed, the issuer or somewhere in the path
should do something like pm_runtime_get_sync (e.g. in sg_open()) or
pm_runtime_resume() (e.g. in blk_queue_enter()), in either sync or
async way. After the job (read/write/ioctl or whatever) is done,
either a pm_runtime_put_sync() or auto runtime suspend puts the device
back into runtime suspended/low-power mode. Since the func
scsi_bus_suspend_common() does nothing if device is already in runtime
suspended mode, scsi_dev_type_resume() should only resume the device
if it is runtime active.

Thanks,

Can Guo.

> Alan Stern
> 
>> Cc: Stanley Chu <stanley.chu@mediatek.com>
>> Cc: Bart Van Assche <bvanassche@acm.org>
>> Cc: Alan Stern <stern@rowland.harvard.edu>
>> Signed-off-by: Can Guo <cang@codeaurora.org>
>> ---
>> 
>> Changes since v1:
>> - Incorporated Bart's comments
>> 
>> ---
>>  drivers/scsi/scsi_pm.c | 24 +-----------------------
>>  1 file changed, 1 insertion(+), 23 deletions(-)
>> 
>> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
>> index 3717eea..908f27f 100644
>> --- a/drivers/scsi/scsi_pm.c
>> +++ b/drivers/scsi/scsi_pm.c
>> @@ -79,25 +79,6 @@ static int scsi_dev_type_resume(struct device *dev,
>>  	scsi_device_resume(to_scsi_device(dev));
>>  	dev_dbg(dev, "scsi resume: %d\n", err);
>> 
>> -	if (err == 0) {
>> -		pm_runtime_disable(dev);
>> -		err = pm_runtime_set_active(dev);
>> -		pm_runtime_enable(dev);
>> -
>> -		/*
>> -		 * Forcibly set runtime PM status of request queue to "active"
>> -		 * to make sure we can again get requests from the queue
>> -		 * (see also blk_pm_peek_request()).
>> -		 *
>> -		 * The resume hook will correct runtime PM status of the disk.
>> -		 */
>> -		if (!err && scsi_is_sdev_device(dev)) {
>> -			struct scsi_device *sdev = to_scsi_device(dev);
>> -
>> -			blk_set_runtime_active(sdev->request_queue);
>> -		}
>> -	}
>> -
>>  	return err;
>>  }
>> 
>> @@ -165,11 +146,8 @@ static int scsi_bus_resume_common(struct device 
>> *dev,
>>  		 */
>>  		if (strncmp(scsi_scan_type, "async", 5) != 0)
>>  			async_synchronize_full_domain(&scsi_sd_pm_domain);
>> -	} else {
>> -		pm_runtime_disable(dev);
>> -		pm_runtime_set_active(dev);
>> -		pm_runtime_enable(dev);
>>  	}
>> +
>>  	return 0;
>>  }
>> 
>> --
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
>> Linux Foundation Collaborative Project.
>> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH RFC v2 1/1] scsi: pm: Leave runtime PM status alone during system resume/thaw/restore
  2020-11-23  1:23   ` Can Guo
@ 2020-11-23  3:02     ` Alan Stern
  0 siblings, 0 replies; 6+ messages in thread
From: Alan Stern @ 2020-11-23  3:02 UTC (permalink / raw)
  To: Can Guo
  Cc: asutoshd, nguyenb, hongwus, ziqichen, rnayak, linux-scsi,
	kernel-team, saravanak, salyzyn, Stanley Chu, Bart Van Assche,
	James E.J. Bottomley, Martin K. Petersen, Matthias Brugger,
	open list, moderated list:ARM/Mediatek SoC support,
	moderated list:ARM/Mediatek SoC support

On Mon, Nov 23, 2020 at 09:23:53AM +0800, Can Guo wrote:
> Hi Alan,
> 
> On 2020-11-21 00:35, Alan Stern wrote:
> > On Fri, Nov 20, 2020 at 12:37:22AM -0800, Can Guo wrote:
> > > Runtime resume is handled by runtime PM framework, no need to forcibly
> > > set runtime PM status to RPM_ACTIVE during system resume/thaw/restore.
> > 
> > Sorry, I don't understand this explanation at all.
> > 
> > Sure, runtime resume is handled by the runtime PM framework.  But this
> > patch changes the code for system resume, which is completely different.
> > 
> > Following a system resume, the hardware will be at full power.  We don't
> > want the kernel to think that the device is still in runtime suspend;
> > otherwise is would never put the device back into low-power mode.
> 
> How about adding below lines to the patch?
> 
> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
> index 908f27f..7ebe582 100644
> --- a/drivers/scsi/scsi_pm.c
> +++ b/drivers/scsi/scsi_pm.c
> @@ -75,9 +75,11 @@ static int scsi_dev_type_resume(struct device *dev,
>         const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>         int err = 0;
> 
> -       err = cb(dev, pm);
> -       scsi_device_resume(to_scsi_device(dev));
> -       dev_dbg(dev, "scsi resume: %d\n", err);
> +       if (pm_runtime_active(dev)) {
> +               err = cb(dev, pm);
> +               scsi_device_resume(to_scsi_device(dev));
> +               dev_dbg(dev, "scsi resume: %d\n", err);
> +       }
> 
>         return err;
>  }
> 
> Whenever a device is accessed, the issuer or somewhere in the path
> should do something like pm_runtime_get_sync (e.g. in sg_open()) or
> pm_runtime_resume() (e.g. in blk_queue_enter()), in either sync or
> async way. After the job (read/write/ioctl or whatever) is done,
> either a pm_runtime_put_sync() or auto runtime suspend puts the device
> back into runtime suspended/low-power mode. Since the func
> scsi_bus_suspend_common() does nothing if device is already in runtime
> suspended mode, scsi_dev_type_resume() should only resume the device
> if it is runtime active.

You're starting to think along the right lines, but you are ignoring all 
the other work that people have already done for handling these cases.

Please read Documentation/driver-api/pm/devices.rst very carefully, 
especially the parts about returning a positive value from the ->prepare 
callback (also known as "direct-complete" and related to the 
DPM_FLAG_NO_DIRECT_COMPLETE and DPM_FLAG_SMART_PREPARE flags) and the 
parts about the DPM_FLAG_SMART_SUSPEND and DPM_FLAG_MAY_SKIP_RESUME 
flags.  Then think about what you want to accomplish and write a patch 
that takes all this information into account.

Key point: At no time should any part of the kernel think that the 
device is in a low-power state when it is actually in a high-power 
state, or vice versa.

Alan Stern

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-11-23  3:02 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-20  8:37 [PATCH RFC v2 1/1] scsi: pm: Leave runtime PM status alone during system resume/thaw/restore Can Guo
2020-11-20 16:35 ` Alan Stern
2020-11-21 17:00   ` Bart Van Assche
2020-11-21 17:32     ` Alan Stern
2020-11-23  1:23   ` Can Guo
2020-11-23  3:02     ` Alan Stern

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).