[PATCH v2 0/5] Migrate SCSI drivers to use dev_pm_ops

[PATCH v2 0/5] Migrate SCSI drivers to use dev_pm_ops

[Aaron Lu]

v2:
Change the runtime suspend behaviour of sd driver by putting the device
into stopped power state.
Revert 2 patches which are no longer needed as pointed out by Alan Stern.
Find out device callbacks in bus callbacks as suggested by Alan Stern.

Due to these changes, patch number grows from 2 -> 5.

v1:
The 2 patches will migrate SCSI drivers to use the pm callbacks defined
in dev_pm_ops as pm_message is deprecated and should not be used by driver.
Bus level callback is changed to use callbacks defined in dev_pm_ops when
needed and sd's pm callback is updated to use what are defined in dev_pm_ops.

Aaron Lu (5):
  sd: put to stopped power state when runtime suspend
  Revert "[SCSI] scsi_pm: set device runtime state before parent
    suspended"
  Revert "[SCSI] runtime resume parent for child's system-resume"
  pm: use callbacks from dev_pm_ops for scsi devices
  sd: update sd to use the new pm callbacks

 drivers/scsi/scsi_pm.c | 138 +++++++++++++++++++++++++++++--------------------
 drivers/scsi/sd.c      |  18 +++++--
 2 files changed, 96 insertions(+), 60 deletions(-)

-- 
1.7.12.21.g871e293

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

[Aaron Lu]

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) {
 		sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
 		ret = sd_start_stop_device(sdkp, 0);
 	}
-- 
1.7.12.21.g871e293

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

[Alan Stern]

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

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

[Aaron Lu]

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

[PATCH v2 2/5] Revert "[SCSI] scsi_pm: set device runtime state before parent suspended"

[Aaron Lu]

This reverts commit 33a2285d96b5e7b9500612ec623bf4313397bb53.

With commit 88d26136a256576e444db312179e17af6dd0ea87 (PM: Prevent
runtime suspend during system resume), this patch is no longer needed.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/scsi/scsi_pm.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index dc0ad85..d4201de 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -76,24 +76,23 @@ static int scsi_bus_resume_common(struct device *dev)
 {
 	int err = 0;
 
-	/*
-	 * Parent device may have runtime suspended as soon as
-	 * it is woken up during the system resume.
-	 *
-	 * Resume it on behalf of child.
-	 */
-	pm_runtime_get_sync(dev->parent);
-
-	if (scsi_is_sdev_device(dev))
+	if (scsi_is_sdev_device(dev)) {
+		/*
+		 * Parent device may have runtime suspended as soon as
+		 * it is woken up during the system resume.
+		 *
+		 * Resume it on behalf of child.
+		 */
+		pm_runtime_get_sync(dev->parent);
 		err = scsi_dev_type_resume(dev);
+		pm_runtime_put_sync(dev->parent);
+	}
+
 	if (err == 0) {
 		pm_runtime_disable(dev);
 		pm_runtime_set_active(dev);
 		pm_runtime_enable(dev);
 	}
-
-	pm_runtime_put_sync(dev->parent);
-
 	return err;
 }
 
-- 
1.7.12.21.g871e293

[PATCH v2 3/5] Revert "[SCSI] runtime resume parent for child's system-resume"

[Aaron Lu]

This reverts commit 28fd00d42cca178638f51c08efa986a777c24a4b.

With commit 88d26136a256576e444db312179e17af6dd0ea87 (PM: Prevent
runtime suspend during system resume), this patch is no longer needed.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/scsi/scsi_pm.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index d4201de..9923b26 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -76,17 +76,8 @@ static int scsi_bus_resume_common(struct device *dev)
 {
 	int err = 0;
 
-	if (scsi_is_sdev_device(dev)) {
-		/*
-		 * Parent device may have runtime suspended as soon as
-		 * it is woken up during the system resume.
-		 *
-		 * Resume it on behalf of child.
-		 */
-		pm_runtime_get_sync(dev->parent);
+	if (scsi_is_sdev_device(dev))
 		err = scsi_dev_type_resume(dev);
-		pm_runtime_put_sync(dev->parent);
-	}
 
 	if (err == 0) {
 		pm_runtime_disable(dev);
-- 
1.7.12.21.g871e293

[PATCH v2 4/5] pm: use callbacks from dev_pm_ops for scsi devices

[Aaron Lu]

Use of pm_message_t is deprecated and device driver is not supposed
to use that. This patch tries to migrate the SCSI bus level pm callbacks
to call device's pm callbacks defined in its driver's dev_pm_ops.

This is achieved by finding out which device pm callback should be used
in bus callback function, and then pass that callback function pointer
as a param to scsi_dev_type_suspend/resume.

scsi_bus_suspend_common is gone, as there is a logic to check if the
device is already runtime suspended and if so, based on the system pm
operation we are going through now by the pm message, different things
will be done(do nothing on suspend/hibernate, runtime resume the device
on freeze). With the deleting of the pm message param now, this logic
can no longer be done there, but in individual bus callbacks like
suspend/poweroff/freeze. And this makes the scsi_bus_suspend_common
function useless and thus deleted.

Since only sd has implemented drv->suspend/drv->resume, and I'll update
sd driver to use the new callbacks in the following patch, there is no
need to fallback to call drv->suspend/drv->resume if dev_pm_ops is NULL.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/scsi/scsi_pm.c | 126 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 82 insertions(+), 44 deletions(-)

diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index 9923b26..aa97dca 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -16,16 +16,16 @@
 
 #include "scsi_priv.h"
 
-static int scsi_dev_type_suspend(struct device *dev, pm_message_t msg)
+typedef int (*pm_callback_t)(struct device *);
+
+static int scsi_dev_type_suspend(struct device *dev, pm_callback_t callback)
 {
-	struct device_driver *drv;
 	int err;
 
 	err = scsi_device_quiesce(to_scsi_device(dev));
 	if (err == 0) {
-		drv = dev->driver;
-		if (drv && drv->suspend) {
-			err = drv->suspend(dev, msg);
+		if (callback) {
+			err = callback(dev);
 			if (err)
 				scsi_device_resume(to_scsi_device(dev));
 		}
@@ -34,14 +34,12 @@ static int scsi_dev_type_suspend(struct device *dev, pm_message_t msg)
 	return err;
 }
 
-static int scsi_dev_type_resume(struct device *dev)
+static int scsi_dev_type_resume(struct device *dev, pm_callback_t callback)
 {
-	struct device_driver *drv;
 	int err = 0;
 
-	drv = dev->driver;
-	if (drv && drv->resume)
-		err = drv->resume(dev);
+	if (callback)
+		err = callback(dev);
 	scsi_device_resume(to_scsi_device(dev));
 	dev_dbg(dev, "scsi resume: %d\n", err);
 	return err;
@@ -49,35 +47,12 @@ static int scsi_dev_type_resume(struct device *dev)
 
 #ifdef CONFIG_PM_SLEEP
 
-static int scsi_bus_suspend_common(struct device *dev, pm_message_t msg)
-{
-	int err = 0;
-
-	if (scsi_is_sdev_device(dev)) {
-		/*
-		 * sd is the only high-level SCSI driver to implement runtime
-		 * PM, and sd treats runtime suspend, system suspend, and
-		 * system hibernate identically (but not system freeze).
-		 */
-		if (pm_runtime_suspended(dev)) {
-			if (msg.event == PM_EVENT_SUSPEND ||
-			    msg.event == PM_EVENT_HIBERNATE)
-				return 0;	/* already suspended */
-
-			/* wake up device so that FREEZE will succeed */
-			pm_runtime_resume(dev);
-		}
-		err = scsi_dev_type_suspend(dev, msg);
-	}
-	return err;
-}
-
-static int scsi_bus_resume_common(struct device *dev)
+static int scsi_bus_resume_common(struct device *dev, pm_callback_t callback)
 {
 	int err = 0;
 
 	if (scsi_is_sdev_device(dev))
-		err = scsi_dev_type_resume(dev);
+		err = scsi_dev_type_resume(dev, callback);
 
 	if (err == 0) {
 		pm_runtime_disable(dev);
@@ -102,26 +77,87 @@ static int scsi_bus_prepare(struct device *dev)
 
 static int scsi_bus_suspend(struct device *dev)
 {
-	return scsi_bus_suspend_common(dev, PMSG_SUSPEND);
+	int err = 0;
+	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+
+	if (scsi_is_sdev_device(dev)) {
+		/*
+		 * sd is the only high-level SCSI driver to implement runtime
+		 * PM, and sd treats runtime suspend, system suspend, and
+		 * system hibernate identically.
+		 */
+		if (pm_runtime_suspended(dev))
+			return 0;
+
+		err = scsi_dev_type_suspend(dev, pm ? pm->suspend : NULL);
+	}
+
+	return err;
+}
+
+static int scsi_bus_resume(struct device *dev)
+{
+	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	return scsi_bus_resume_common(dev, pm ? pm->resume : NULL);
 }
 
 static int scsi_bus_freeze(struct device *dev)
 {
-	return scsi_bus_suspend_common(dev, PMSG_FREEZE);
+	int err = 0;
+	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+
+	if (scsi_is_sdev_device(dev)) {
+		/* wake up device so that FREEZE will succeed */
+		if (pm_runtime_suspended(dev))
+			pm_runtime_resume(dev);
+
+		err = scsi_dev_type_suspend(dev, pm ? pm->freeze : NULL);
+	}
+
+	return err;
+}
+
+static int scsi_bus_thaw(struct device *dev)
+{
+	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	return scsi_bus_resume_common(dev, pm ? pm->thaw : NULL);
 }
 
 static int scsi_bus_poweroff(struct device *dev)
 {
-	return scsi_bus_suspend_common(dev, PMSG_HIBERNATE);
+	int err = 0;
+	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+
+	if (scsi_is_sdev_device(dev)) {
+		/*
+		 * sd is the only high-level SCSI driver to implement runtime
+		 * PM, and sd treats runtime suspend, system suspend, and
+		 * system hibernate identically.
+		 */
+		if (pm_runtime_suspended(dev))
+			return 0;
+
+		err = scsi_dev_type_suspend(dev, pm ? pm->poweroff : NULL);
+	}
+
+	return err;
+}
+
+static int scsi_bus_restore(struct device *dev)
+{
+	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	return scsi_bus_resume_common(dev, pm ? pm->restore : NULL);
 }
 
 #else /* CONFIG_PM_SLEEP */
 
-#define scsi_bus_resume_common		NULL
 #define scsi_bus_prepare		NULL
 #define scsi_bus_suspend		NULL
+#define scsi_bus_resume			NULL
 #define scsi_bus_freeze			NULL
+#define scsi_bus_thaw			NULL
 #define scsi_bus_poweroff		NULL
+#define scsi_bus_restore		NULL
 
 #endif /* CONFIG_PM_SLEEP */
 
@@ -130,10 +166,11 @@ static int scsi_bus_poweroff(struct device *dev)
 static int scsi_runtime_suspend(struct device *dev)
 {
 	int err = 0;
+	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 
 	dev_dbg(dev, "scsi_runtime_suspend\n");
 	if (scsi_is_sdev_device(dev)) {
-		err = scsi_dev_type_suspend(dev, PMSG_AUTO_SUSPEND);
+		err = scsi_dev_type_suspend(dev, pm ? pm->runtime_suspend : NULL);
 		if (err == -EAGAIN)
 			pm_schedule_suspend(dev, jiffies_to_msecs(
 				round_jiffies_up_relative(HZ/10)));
@@ -147,10 +184,11 @@ static int scsi_runtime_suspend(struct device *dev)
 static int scsi_runtime_resume(struct device *dev)
 {
 	int err = 0;
+	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 
 	dev_dbg(dev, "scsi_runtime_resume\n");
 	if (scsi_is_sdev_device(dev))
-		err = scsi_dev_type_resume(dev);
+		err = scsi_dev_type_resume(dev, pm ? pm->runtime_resume : NULL);
 
 	/* Insert hooks here for targets, hosts, and transport classes */
 
@@ -229,11 +267,11 @@ void scsi_autopm_put_host(struct Scsi_Host *shost)
 const struct dev_pm_ops scsi_bus_pm_ops = {
 	.prepare =		scsi_bus_prepare,
 	.suspend =		scsi_bus_suspend,
-	.resume =		scsi_bus_resume_common,
+	.resume =		scsi_bus_resume,
 	.freeze =		scsi_bus_freeze,
-	.thaw =			scsi_bus_resume_common,
+	.thaw =			scsi_bus_thaw,
 	.poweroff =		scsi_bus_poweroff,
-	.restore =		scsi_bus_resume_common,
+	.restore =		scsi_bus_restore,
 	.runtime_suspend =	scsi_runtime_suspend,
 	.runtime_resume =	scsi_runtime_resume,
 	.runtime_idle =		scsi_runtime_idle,
-- 
1.7.12.21.g871e293

Re: [PATCH v2 4/5] pm: use callbacks from dev_pm_ops for scsi devices

[Alan Stern]

I have a couple of small changes to suggest.  Nothing major.

On Thu, 11 Oct 2012, Aaron Lu wrote:

> @@ -102,26 +77,87 @@ static int scsi_bus_prepare(struct device *dev)
>  
>  static int scsi_bus_suspend(struct device *dev)
>  {
> -	return scsi_bus_suspend_common(dev, PMSG_SUSPEND);
> +	int err = 0;
> +	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +
> +	if (scsi_is_sdev_device(dev)) {
> +		/*
> +		 * sd is the only high-level SCSI driver to implement runtime
> +		 * PM, and sd treats runtime suspend, system suspend, and
> +		 * system hibernate identically.
> +		 */

It would be better if we don't refer to sd specifically.  The comment 
could be changed to something like this:

		/*
		 * All the high-level SCSI drivers that implement runtime PM
		 * treat runtime suspend, system suspend, and system
		 * hibernate identically.
		 */

>  static int scsi_bus_freeze(struct device *dev)
>  {
> -	return scsi_bus_suspend_common(dev, PMSG_FREEZE);
> +	int err = 0;
> +	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +
> +	if (scsi_is_sdev_device(dev)) {
> +		/* wake up device so that FREEZE will succeed */
> +		if (pm_runtime_suspended(dev))
> +			pm_runtime_resume(dev);

I'm not sure why this is here.  If none of the high-level drivers 
implement FREEZE then it's not needed.


>  static int scsi_bus_poweroff(struct device *dev)
>  {
> -	return scsi_bus_suspend_common(dev, PMSG_HIBERNATE);
> +	int err = 0;
> +	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +
> +	if (scsi_is_sdev_device(dev)) {
> +		/*
> +		 * sd is the only high-level SCSI driver to implement runtime
> +		 * PM, and sd treats runtime suspend, system suspend, and
> +		 * system hibernate identically.
> +		 */

Same change as above for this comment.

Alan Stern

Re: [PATCH v2 4/5] pm: use callbacks from dev_pm_ops for scsi devices

[Aaron Lu]

On Thu, Oct 11, 2012 at 10:57:26AM -0400, Alan Stern wrote:
> I have a couple of small changes to suggest.  Nothing major.
> 
> On Thu, 11 Oct 2012, Aaron Lu wrote:
> 
> > @@ -102,26 +77,87 @@ static int scsi_bus_prepare(struct device *dev)
> >  
> >  static int scsi_bus_suspend(struct device *dev)
> >  {
> > -	return scsi_bus_suspend_common(dev, PMSG_SUSPEND);
> > +	int err = 0;
> > +	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> > +
> > +	if (scsi_is_sdev_device(dev)) {
> > +		/*
> > +		 * sd is the only high-level SCSI driver to implement runtime
> > +		 * PM, and sd treats runtime suspend, system suspend, and
> > +		 * system hibernate identically.
> > +		 */
> 
> It would be better if we don't refer to sd specifically.  The comment 
> could be changed to something like this:
> 
> 		/*
> 		 * All the high-level SCSI drivers that implement runtime PM
> 		 * treat runtime suspend, system suspend, and system
> 		 * hibernate identically.
> 		 */

OK, will do this.

> 
> >  static int scsi_bus_freeze(struct device *dev)
> >  {
> > -	return scsi_bus_suspend_common(dev, PMSG_FREEZE);
> > +	int err = 0;
> > +	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> > +
> > +	if (scsi_is_sdev_device(dev)) {
> > +		/* wake up device so that FREEZE will succeed */
> > +		if (pm_runtime_suspended(dev))
> > +			pm_runtime_resume(dev);
> 
> I'm not sure why this is here.  If none of the high-level drivers 
> implement FREEZE then it's not needed.

Agree.
I should have checked the git log to see why it is here.
>From the log, it doesn't seem to have a reason.
Thanks for pointing this out.

> 
> 
> >  static int scsi_bus_poweroff(struct device *dev)
> >  {
> > -	return scsi_bus_suspend_common(dev, PMSG_HIBERNATE);
> > +	int err = 0;
> > +	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> > +
> > +	if (scsi_is_sdev_device(dev)) {
> > +		/*
> > +		 * sd is the only high-level SCSI driver to implement runtime
> > +		 * PM, and sd treats runtime suspend, system suspend, and
> > +		 * system hibernate identically.
> > +		 */
> 
> Same change as above for this comment.

OK.

And thanks for reviewing.

-Aaron

[PATCH v2 5/5] sd: update sd to use the new pm callbacks

[Aaron Lu]

Update sd driver to use the callbacks defined in dev_pm_ops.

sd_freeze is NULL, the bus level callback has taken care of quiescing
the device so there should be nothing needs to be done here.
Consequently, sd_thaw is not needed here either.

suspend, poweroff and runtime suspend share the same routine sd_suspend,
which will sync flush and then stop the drive, this is the same as before.

resume, restore and runtime resume share the same routine sd_resume,
which will start the drive by putting it into active power state, this
is also the same as before.

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

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 8b6e004..6564305 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -104,7 +104,7 @@ static void sd_unlock_native_capacity(struct gendisk *disk);
 static int  sd_probe(struct device *);
 static int  sd_remove(struct device *);
 static void sd_shutdown(struct device *);
-static int sd_suspend(struct device *, pm_message_t state);
+static int sd_suspend(struct device *);
 static int sd_resume(struct device *);
 static void sd_rescan(struct device *);
 static int sd_done(struct scsi_cmnd *);
@@ -423,15 +423,23 @@ static struct class sd_disk_class = {
 	.dev_attrs	= sd_disk_attrs,
 };
 
+static const struct dev_pm_ops sd_pm_ops = {
+	.suspend		= sd_suspend,
+	.resume			= sd_resume,
+	.poweroff		= sd_suspend,
+	.restore		= sd_resume,
+	.runtime_suspend	= sd_suspend,
+	.runtime_resume		= sd_resume,
+};
+
 static struct scsi_driver sd_template = {
 	.owner			= THIS_MODULE,
 	.gendrv = {
 		.name		= "sd",
 		.probe		= sd_probe,
 		.remove		= sd_remove,
-		.suspend	= sd_suspend,
-		.resume		= sd_resume,
 		.shutdown	= sd_shutdown,
+		.pm		= &sd_pm_ops,
 	},
 	.rescan			= sd_rescan,
 	.done			= sd_done,
@@ -2896,7 +2904,7 @@ exit:
 	scsi_disk_put(sdkp);
 }
 
-static int sd_suspend(struct device *dev, pm_message_t mesg)
+static int sd_suspend(struct device *dev)
 {
 	struct scsi_disk *sdkp = scsi_disk_get_from_dev(dev);
 	int ret = 0;
@@ -2911,8 +2919,7 @@ static int sd_suspend(struct device *dev, pm_message_t mesg)
 			goto done;
 	}
 
-	if (((mesg.event & PM_EVENT_SLEEP) || PMSG_IS_AUTO(mesg)) &&
-			sdkp->device->manage_start_stop) {
+	if (sdkp->device->manage_start_stop) {
 		sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
 		ret = sd_start_stop_device(sdkp, 0);
 	}
-- 
1.7.12.21.g871e293