Patchwork [v3,3/6,SCSI] runtime resume device before system suspend

login
register
mail settings
Submitter Lin Ming
Date Nov. 24, 2011, 12:21 p.m.
Message ID <1322137287.2597.10.camel@hp6530s>
Download mbox | patch
Permalink /patch/127489/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Lin Ming - Nov. 24, 2011, 12:21 p.m.
On Thu, 2011-11-24 at 01:02 +0800, Alan Stern wrote:
> On Wed, 23 Nov 2011, Lin Ming wrote:
> 
> > From: Alan Stern <stern@rowland.harvard.edu>
> > 
> > scsi device runtime PM is using PMSG_SUSPEND. System PM may use other state
> > that may not be compatiable with PMSG_SUSPEND.
> 
> Actually SCSI runtime PM uses PMSG_AUTO_AUTOSUSPEND, which is the same
> as PMSG_SUSPEND except that the PM_EVENT_AUTO bit is also set in the
> pm_message.event field.  Currently they _are_ compatible.
> 
> > So we need to runtime resume the device before system suspend.
> > 
> > Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> > ---
> > 
> > Alan,
> > 
> > Could you add your Signed-off-by?
> > Free free to change the commit logs.
> 
> I don't know; this is a little questionable.
> 
> The point of this patch is to handle drivers that do different things
> for runtime suspend and system sleep.  The only SCSI driver that
> currently supports runtime suspend is sd, and it treats runtime suspend
> the same as system sleep.  (Earlier I said it doesn't spin down disks
> for runtime suspend -- that was wrong, it does.  It skips the spin-down
> step only for PM_EVENT_FREEZE, which is part of the hibernation
> procedure.)
> 
> Until other SCSI drivers support runtime suspend, this patch shouldn't 
> be needed.  And spinning up runtime-suspended disks could add a lengthy 
> delay to the system sleep transition, so it's better not to do this if 
> at all possible.

For sd driver, PMSG_SUSPEND and PMSG_HIBERNATE are compatible with
PMSG_AUTO_SUSPEND. PMSG_FREEZE is not compatible.

So we only need to runtime resume sd for PMSG_FREEZE case.
How about below?


Lin Ming

> 
> Alan Stern
> 
> >  drivers/scsi/scsi_pm.c |    4 +++-
> >  1 files changed, 3 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
> > index d329f8b..549ea72 100644
> > --- a/drivers/scsi/scsi_pm.c
> > +++ b/drivers/scsi/scsi_pm.c
> > @@ -49,8 +49,10 @@ static int scsi_bus_suspend_common(struct device *dev, pm_message_t msg)
> >  {
> >  	int err = 0;
> >  
> > -	if (scsi_is_sdev_device(dev))
> > +	if (scsi_is_sdev_device(dev)) {
> > +		pm_runtime_resume(dev);
> >  		err = scsi_dev_type_suspend(dev, msg);
> > +	}
> >  	return err;
> >  }
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Stern - Nov. 24, 2011, 4:36 p.m.
On Thu, 24 Nov 2011, Lin Ming wrote:

> > The point of this patch is to handle drivers that do different things
> > for runtime suspend and system sleep.  The only SCSI driver that
> > currently supports runtime suspend is sd, and it treats runtime suspend
> > the same as system sleep.  (Earlier I said it doesn't spin down disks
> > for runtime suspend -- that was wrong, it does.  It skips the spin-down
> > step only for PM_EVENT_FREEZE, which is part of the hibernation
> > procedure.)
> > 
> > Until other SCSI drivers support runtime suspend, this patch shouldn't 
> > be needed.  And spinning up runtime-suspended disks could add a lengthy 
> > delay to the system sleep transition, so it's better not to do this if 
> > at all possible.
> 
> For sd driver, PMSG_SUSPEND and PMSG_HIBERNATE are compatible with
> PMSG_AUTO_SUSPEND. PMSG_FREEZE is not compatible.

I'm not sure what you mean.  In the sd driver, PMSG_SUSPEND,
PMSG_HIBERNATE, and PMSG_AUTO_SUSPEND all do exactly the same thing.  
PMSG_FREEZE does a little less -- it doesn't spin down the drive.

> So we only need to runtime resume sd for PMSG_FREEZE case.

No, we don't.  PMSG_FREEZE does not care whether the drive is spinning
or not.  (That's why it skips the spin-down step.)  Therefore it's
silly to restart a stopped drive just in order to do a PMSG_FREEZE.

> How about below?
> 
> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
> index 549ea72..e2759d9 100644
> --- a/drivers/scsi/scsi_pm.c
> +++ b/drivers/scsi/scsi_pm.c
> @@ -50,7 +50,13 @@ static int scsi_bus_suspend_common(struct device *dev, pm_message_t msg)
>  	int err = 0;
>  
>  	if (scsi_is_sdev_device(dev)) {
> -		pm_runtime_resume(dev);
> +		if (pm_runtime_suspended(dev)) {
> +			if (msg.event == PM_EVENT_FREEZE)
> +				pm_runtime_resume(dev);
> +			else

The 3 lines above aren't needed.

> +				return 0;
> +		}
> +
>  		err = scsi_dev_type_suspend(dev, msg);
>  	}
>  	return err;

Of course, this leaves the patch in pretty much the same state as what 
Tejun objected to in

	http://marc.info/?l=linux-ide&m=132136894329965&w=2

I think this email discussion has answered his objection: The only SCSI
top-level driver implementing runtime suspend is sd, and sd treats
runtime suspend the same as system sleep.  It might be a good idea to
add a comment with this explanation along with the new code, however.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - Nov. 24, 2011, 7:57 p.m.
Hello,

On Thu, Nov 24, 2011 at 11:36:20AM -0500, Alan Stern wrote:
> I think this email discussion has answered his objection: The only SCSI
> top-level driver implementing runtime suspend is sd, and sd treats
> runtime suspend the same as system sleep.  It might be a good idea to
> add a comment with this explanation along with the new code, however.

Hmmm... I see.  This shouldn't affect host controller states - we'll
be skipping only the drive PM transitions, right?  As long as it's
well documented, I guess it's okay but at the same time I don't think
it's such a big deal to spin up drives when the system is entering
hibernation from runtime powersave.  On most systems, they'll need to
be spun up for image dump pretty soon anyway.

Thanks.
Alan Stern - Nov. 24, 2011, 10:54 p.m.
On Thu, 24 Nov 2011, Tejun Heo wrote:

> Hello,
> 
> On Thu, Nov 24, 2011 at 11:36:20AM -0500, Alan Stern wrote:
> > I think this email discussion has answered his objection: The only SCSI
> > top-level driver implementing runtime suspend is sd, and sd treats
> > runtime suspend the same as system sleep.  It might be a good idea to
> > add a comment with this explanation along with the new code, however.
> 
> Hmmm... I see.  This shouldn't affect host controller states - we'll
> be skipping only the drive PM transitions, right?

Right.

>  As long as it's
> well documented, I guess it's okay but at the same time I don't think
> it's such a big deal to spin up drives when the system is entering
> hibernation from runtime powersave.  On most systems, they'll need to
> be spun up for image dump pretty soon anyway.

Hmm, that's true.  Or more precisely, they'll all be spun up for the
THAW stage of hibernation, prior to storing the memory image -- even
though the image is generally stored on only a single drive.

Okay, Ming, your most recent suggested patch can be used as is (though
adding a comment would be a good idea).  Here's my suggestion for the
patch description:

The only high-level SCSI driver that currently implements runtime PM is
sd, and sd treats runtime suspend exactly the same as the SUSPEND and
HIBERNATE stages of system sleep, but not the same as the FREEZE stage.

Therefore, when entering the SUSPEND or HIBERNATE stages of system
sleep, we can skip the callback to the driver if the device is already
in runtime suspend.  When entering the FREEZE stage, however, we should
first issue a runtime resume.  The overhead of doing this is
negligible, because a suspended drive would be spun up during the THAW
stage of hibernation anyway.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - Nov. 24, 2011, 11:01 p.m.
Hello,

On Thu, Nov 24, 2011 at 2:54 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> Therefore, when entering the SUSPEND or HIBERNATE stages of system
> sleep, we can skip the callback to the driver if the device is already
> in runtime suspend.  When entering the FREEZE stage, however, we should
> first issue a runtime resume.  The overhead of doing this is
> negligible, because a suspended drive would be spun up during the THAW
> stage of hibernation anyway.

My brain is half fried at the moment so maybe you're saying the same
thing, but I hope this is something simple. Like...

    /* for libata runtime suspend is equivalent to suspend */
    if (runtime suspended && target == SUSPEND) {
        yeah! skip drive pm ops;
    } else {
        end runtime pm;
        ask EH to enter target PM state;
    }

I really don't think we need to optimize this further than this.

Thanks.

Patch

diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index 549ea72..e2759d9 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -50,7 +50,13 @@  static int scsi_bus_suspend_common(struct device *dev, pm_message_t msg)
 	int err = 0;
 
 	if (scsi_is_sdev_device(dev)) {
-		pm_runtime_resume(dev);
+		if (pm_runtime_suspended(dev)) {
+			if (msg.event == PM_EVENT_FREEZE)
+				pm_runtime_resume(dev);
+			else
+				return 0;
+		}
+
 		err = scsi_dev_type_suspend(dev, msg);
 	}
 	return err;