diff mbox

[v2,1/5] sd: put to stopped power state when runtime suspend

Message ID 1349932091-6856-2-git-send-email-aaron.lu@intel.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Aaron Lu Oct. 11, 2012, 5:08 a.m. UTC
When device is runtime suspended, put it to stopped power state to save
some power.

This will also make the behaviour consistent with what the scsi_pm.c
thinks about sd as the comment says:
sd treats runtime suspend, system suspend and system hibernate identical.
With this patch, it is now identical.
And sd_shutdown will also do nothing when it finds the device has been
runtime suspended, if we do not spin down the disk in runtime suspend
by putting it into stopped power state, the disk will be shut down
incorrectly.
And the the same problem can be solved for runtime power off after
runtime suspended case by this change.

With the current runtime scheme for disk, it will only be runtime
suspended when no process opens the disk, so this shouldn't happen a
lot, which makes it acceptable to spin down the disk when runtime
suspended. If some day a more aggressive runtime scheme is used, like
the 'request based runtime pm for disk' that Alan Stern and Lin Ming
has been working, we can introduce some policy to control this. But for
now, make it simple and correct by spinning down the disk.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/scsi/sd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Alan Stern Oct. 11, 2012, 2:50 p.m. UTC | #1
On Thu, 11 Oct 2012, Aaron Lu wrote:

> When device is runtime suspended, put it to stopped power state to save
> some power.
> 
> This will also make the behaviour consistent with what the scsi_pm.c
> thinks about sd as the comment says:
> sd treats runtime suspend, system suspend and system hibernate identical.
> With this patch, it is now identical.
> And sd_shutdown will also do nothing when it finds the device has been
> runtime suspended, if we do not spin down the disk in runtime suspend
> by putting it into stopped power state, the disk will be shut down
> incorrectly.
> And the the same problem can be solved for runtime power off after
> runtime suspended case by this change.
> 
> With the current runtime scheme for disk, it will only be runtime
> suspended when no process opens the disk, so this shouldn't happen a
> lot, which makes it acceptable to spin down the disk when runtime
> suspended. If some day a more aggressive runtime scheme is used, like
> the 'request based runtime pm for disk' that Alan Stern and Lin Ming
> has been working, we can introduce some policy to control this. But for
> now, make it simple and correct by spinning down the disk.
> 
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> ---
>  drivers/scsi/sd.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 12f6fdf..8b6e004 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2911,7 +2911,8 @@ static int sd_suspend(struct device *dev, pm_message_t mesg)
>  			goto done;
>  	}
>  
> -	if ((mesg.event & PM_EVENT_SLEEP) && sdkp->device->manage_start_stop) {
> +	if (((mesg.event & PM_EVENT_SLEEP) || PMSG_IS_AUTO(mesg)) &&
> +			sdkp->device->manage_start_stop) {

This is more complicated than it needs to be.  Any suspend is either a
system sleep or an autosuspend, so the test doesn't have to be there at
all.  But since you remove the test anyway in patch 5/5, this doesn't
matter very much.  If you end up resubmitting these patches, you might
as well get rid of the test in this one.

Overall this series is much better than before; thanks for rewriting 
it.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Aaron Lu Oct. 12, 2012, 6:45 a.m. UTC | #2
On Thu, Oct 11, 2012 at 10:50:37AM -0400, Alan Stern wrote:
> On Thu, 11 Oct 2012, Aaron Lu wrote:
> 
> > When device is runtime suspended, put it to stopped power state to save
> > some power.
> > 
> > This will also make the behaviour consistent with what the scsi_pm.c
> > thinks about sd as the comment says:
> > sd treats runtime suspend, system suspend and system hibernate identical.
> > With this patch, it is now identical.
> > And sd_shutdown will also do nothing when it finds the device has been
> > runtime suspended, if we do not spin down the disk in runtime suspend
> > by putting it into stopped power state, the disk will be shut down
> > incorrectly.
> > And the the same problem can be solved for runtime power off after
> > runtime suspended case by this change.
> > 
> > With the current runtime scheme for disk, it will only be runtime
> > suspended when no process opens the disk, so this shouldn't happen a
> > lot, which makes it acceptable to spin down the disk when runtime
> > suspended. If some day a more aggressive runtime scheme is used, like
> > the 'request based runtime pm for disk' that Alan Stern and Lin Ming
> > has been working, we can introduce some policy to control this. But for
> > now, make it simple and correct by spinning down the disk.
> > 
> > Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> > ---
> >  drivers/scsi/sd.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> > index 12f6fdf..8b6e004 100644
> > --- a/drivers/scsi/sd.c
> > +++ b/drivers/scsi/sd.c
> > @@ -2911,7 +2911,8 @@ static int sd_suspend(struct device *dev, pm_message_t mesg)
> >  			goto done;
> >  	}
> >  
> > -	if ((mesg.event & PM_EVENT_SLEEP) && sdkp->device->manage_start_stop) {
> > +	if (((mesg.event & PM_EVENT_SLEEP) || PMSG_IS_AUTO(mesg)) &&
> > +			sdkp->device->manage_start_stop) {
> 
> This is more complicated than it needs to be.  Any suspend is either a
> system sleep or an autosuspend, so the test doesn't have to be there at

There is also the freeze case, in which no need to stop the drive.

> all.  But since you remove the test anyway in patch 5/5, this doesn't
> matter very much.  If you end up resubmitting these patches, you might
> as well get rid of the test in this one.
> 
> Overall this series is much better than before; thanks for rewriting 
> it.

Thanks for your suggestion which made the patch a lot better :-)

-Aaron

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 12f6fdf..8b6e004 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2911,7 +2911,8 @@  static int sd_suspend(struct device *dev, pm_message_t mesg)
 			goto done;
 	}
 
-	if ((mesg.event & PM_EVENT_SLEEP) && sdkp->device->manage_start_stop) {
+	if (((mesg.event & PM_EVENT_SLEEP) || PMSG_IS_AUTO(mesg)) &&
+			sdkp->device->manage_start_stop) {
 		sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
 		ret = sd_start_stop_device(sdkp, 0);
 	}