[PATCH 0/2] Migrate SCSI drivers to use dev_pm_ops

[PATCH 0/2] Migrate SCSI drivers to use dev_pm_ops

[Aaron Lu]

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 (2):
  [SCSI] pm: use callbacks from dev_pm_ops for scsi devices
  [SCSI] sd: update sd to use the new pm callbacks

 drivers/scsi/scsi_pm.c | 85 +++++++++++++++++++++++++++++++++++++++-----------
 drivers/scsi/sd.c      | 18 ++++++++---
 2 files changed, 79 insertions(+), 24 deletions(-)

-- 
1.7.12.21.g871e293

[PATCH 1/2] [SCSI] 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.

NO logic changes in scsi_pm.c, a new function called run_dev_callback is
added, which will choose the proper callback for the device based on the
mesg param passed in and then call that chosen callback function with
the help of pm generic function APIs. So run_dev_callback will be used
to replace the previous drv->suspend/drv->resume call.

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 | 85 +++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 66 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index dc0ad85..aeb151e 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -16,32 +16,62 @@
 
 #include "scsi_priv.h"
 
+static int run_dev_callback(struct device *dev, pm_message_t mesg)
+{
+	int ret;
+
+	switch (mesg.event) {
+		case PM_EVENT_SUSPEND:
+			ret = pm_generic_suspend(dev);
+			break;
+		case PM_EVENT_RESUME:
+			ret = pm_generic_resume(dev);
+			break;
+		case PM_EVENT_FREEZE:
+			ret = pm_generic_freeze(dev);
+			break;
+		case PM_EVENT_THAW:
+			ret = pm_generic_thaw(dev);
+			break;
+		case PM_EVENT_HIBERNATE:
+			ret = pm_generic_poweroff(dev);
+			break;
+		case PM_EVENT_RESTORE:
+			ret = pm_generic_restore(dev);
+			break;
+		case PM_EVENT_AUTO_SUSPEND:
+			ret = pm_generic_runtime_suspend(dev);
+			break;
+		case PM_EVENT_AUTO_RESUME:
+			ret = pm_generic_runtime_resume(dev);
+			break;
+		default:
+			ret = -1;
+			break;
+	}
+
+	return ret;
+}
+
 static int scsi_dev_type_suspend(struct device *dev, pm_message_t msg)
 {
-	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 (err)
-				scsi_device_resume(to_scsi_device(dev));
-		}
+		err = run_dev_callback(dev, msg);
+		if (err)
+			scsi_device_resume(to_scsi_device(dev));
 	}
 	dev_dbg(dev, "scsi suspend: %d\n", err);
 	return err;
 }
 
-static int scsi_dev_type_resume(struct device *dev)
+static int scsi_dev_type_resume(struct device *dev, pm_message_t mesg)
 {
-	struct device_driver *drv;
 	int err = 0;
 
-	drv = dev->driver;
-	if (drv && drv->resume)
-		err = drv->resume(dev);
+	err = run_dev_callback(dev, mesg);
 	scsi_device_resume(to_scsi_device(dev));
 	dev_dbg(dev, "scsi resume: %d\n", err);
 	return err;
@@ -72,7 +102,7 @@ static int scsi_bus_suspend_common(struct device *dev, pm_message_t msg)
 	return err;
 }
 
-static int scsi_bus_resume_common(struct device *dev)
+static int scsi_bus_resume_common(struct device *dev, pm_message_t mesg)
 {
 	int err = 0;
 
@@ -85,7 +115,7 @@ static int scsi_bus_resume_common(struct device *dev)
 	pm_runtime_get_sync(dev->parent);
 
 	if (scsi_is_sdev_device(dev))
-		err = scsi_dev_type_resume(dev);
+		err = scsi_dev_type_resume(dev, mesg);
 	if (err == 0) {
 		pm_runtime_disable(dev);
 		pm_runtime_set_active(dev);
@@ -115,23 +145,40 @@ static int scsi_bus_suspend(struct device *dev)
 	return scsi_bus_suspend_common(dev, PMSG_SUSPEND);
 }
 
+static int scsi_bus_resume(struct device *dev)
+{
+	return scsi_bus_resume_common(dev, PMSG_RESUME);
+}
+
 static int scsi_bus_freeze(struct device *dev)
 {
 	return scsi_bus_suspend_common(dev, PMSG_FREEZE);
 }
 
+static int scsi_bus_thaw(struct device *dev)
+{
+	return scsi_bus_resume_common(dev, PMSG_THAW);
+}
+
 static int scsi_bus_poweroff(struct device *dev)
 {
 	return scsi_bus_suspend_common(dev, PMSG_HIBERNATE);
 }
 
+static int scsi_bus_restore(struct device *dev)
+{
+	return scsi_bus_resume_common(dev, PMSG_RESTORE);
+}
+
 #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 */
 
@@ -160,7 +207,7 @@ static int scsi_runtime_resume(struct device *dev)
 
 	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, PMSG_AUTO_RESUME);
 
 	/* Insert hooks here for targets, hosts, and transport classes */
 
@@ -239,11 +286,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 1/2] [SCSI] pm: use callbacks from dev_pm_ops for scsi devices

[Alan Stern]

On Wed, 10 Oct 2012, Aaron Lu wrote:

> 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.
> 
> NO logic changes in scsi_pm.c, a new function called run_dev_callback is
> added, which will choose the proper callback for the device based on the
> mesg param passed in and then call that chosen callback function with
> the help of pm generic function APIs. So run_dev_callback will be used
> to replace the previous drv->suspend/drv->resume call.
> 
> 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>

I'm sorry, but this is a really ugly patch.

>  drivers/scsi/scsi_pm.c | 85 +++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 66 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
> index dc0ad85..aeb151e 100644
> --- a/drivers/scsi/scsi_pm.c
> +++ b/drivers/scsi/scsi_pm.c
> @@ -16,32 +16,62 @@
>  
>  #include "scsi_priv.h"
>  
> +static int run_dev_callback(struct device *dev, pm_message_t mesg)
> +{
> +	int ret;
> +
> +	switch (mesg.event) {
> +		case PM_EVENT_SUSPEND:
> +			ret = pm_generic_suspend(dev);
> +			break;
> +		case PM_EVENT_RESUME:
> +			ret = pm_generic_resume(dev);
> +			break;
> +		case PM_EVENT_FREEZE:
> +			ret = pm_generic_freeze(dev);
> +			break;
> +		case PM_EVENT_THAW:
> +			ret = pm_generic_thaw(dev);
> +			break;
> +		case PM_EVENT_HIBERNATE:
> +			ret = pm_generic_poweroff(dev);
> +			break;
> +		case PM_EVENT_RESTORE:
> +			ret = pm_generic_restore(dev);
> +			break;
> +		case PM_EVENT_AUTO_SUSPEND:
> +			ret = pm_generic_runtime_suspend(dev);
> +			break;
> +		case PM_EVENT_AUTO_RESUME:
> +			ret = pm_generic_runtime_resume(dev);
> +			break;
> +		default:
> +			ret = -1;
> +			break;
> +	}
> +
> +	return ret;
> +}

> @@ -115,23 +145,40 @@ static int scsi_bus_suspend(struct device *dev)
>  	return scsi_bus_suspend_common(dev, PMSG_SUSPEND);
>  }
>  
> +static int scsi_bus_resume(struct device *dev)
> +{
> +	return scsi_bus_resume_common(dev, PMSG_RESUME);
> +}
> +
>  static int scsi_bus_freeze(struct device *dev)
>  {
>  	return scsi_bus_suspend_common(dev, PMSG_FREEZE);
>  }
>  
> +static int scsi_bus_thaw(struct device *dev)
> +{
> +	return scsi_bus_resume_common(dev, PMSG_THAW);
> +}
> +
>  static int scsi_bus_poweroff(struct device *dev)
>  {
>  	return scsi_bus_suspend_common(dev, PMSG_HIBERNATE);
>  }
>  
> +static int scsi_bus_restore(struct device *dev)
> +{
> +	return scsi_bus_resume_common(dev, PMSG_RESTORE);
> +}
> +

This is just dumb.  Do the parts that depend on the event type here, in 
these routines, instead of using a big "switch" statement somewhere 
else.  For example, have these routines figure out which callback 
routine or generic routine to use, and pass that pointer to 
scsi_bus_{suspend,resume}_common() instead of the PMSG_* argument.

One thing may help simplify the conversion (you can do this in a
separate patch).  Linus's current git tree contains a new commit
88d26136a256576e444db312179e17af6dd0ea87 (PM: Prevent runtime suspend
during system resume) which makes some of the code in scsi_pm.c
unnecessary.  The following two commits can now be reverted:

	33a2285d96b5e7b9500612ec623bf4313397bb53
	[SCSI] scsi_pm: set device runtime state before parent suspended

	28fd00d42cca178638f51c08efa986a777c24a4b
	[SCSI] runtime resume parent for child's system-resume

This will reduce the amount of code you need to worry about.

Alan Stern

Re: [PATCH 1/2] [SCSI] pm: use callbacks from dev_pm_ops for scsi devices

[Aaron Lu]

On Wed, Oct 10, 2012 at 02:32:13PM -0400, Alan Stern wrote:
> On Wed, 10 Oct 2012, Aaron Lu wrote:
> 
> > +static int run_dev_callback(struct device *dev, pm_message_t mesg)
> > +{
> > +	int ret;
> > +
> > +	switch (mesg.event) {
> > +		case PM_EVENT_SUSPEND:
> > +			ret = pm_generic_suspend(dev);
> > +			break;
> > +		case PM_EVENT_RESUME:
> > +			ret = pm_generic_resume(dev);
> > +			break;
> > +		case PM_EVENT_FREEZE:
> > +			ret = pm_generic_freeze(dev);
> > +			break;
> > +		case PM_EVENT_THAW:
> > +			ret = pm_generic_thaw(dev);
> > +			break;
> > +		case PM_EVENT_HIBERNATE:
> > +			ret = pm_generic_poweroff(dev);
> > +			break;
> > +		case PM_EVENT_RESTORE:
> > +			ret = pm_generic_restore(dev);
> > +			break;
> > +		case PM_EVENT_AUTO_SUSPEND:
> > +			ret = pm_generic_runtime_suspend(dev);
> > +			break;
> > +		case PM_EVENT_AUTO_RESUME:
> > +			ret = pm_generic_runtime_resume(dev);
> > +			break;
> > +		default:
> > +			ret = -1;
> > +			break;
> > +	}
> > +
> > +	return ret;
> > +}
> 
> > @@ -115,23 +145,40 @@ static int scsi_bus_suspend(struct device *dev)
> >  	return scsi_bus_suspend_common(dev, PMSG_SUSPEND);
> >  }
> >  
> > +static int scsi_bus_resume(struct device *dev)
> > +{
> > +	return scsi_bus_resume_common(dev, PMSG_RESUME);
> > +}
> > +
> >  static int scsi_bus_freeze(struct device *dev)
> >  {
> >  	return scsi_bus_suspend_common(dev, PMSG_FREEZE);
> >  }
> >  
> > +static int scsi_bus_thaw(struct device *dev)
> > +{
> > +	return scsi_bus_resume_common(dev, PMSG_THAW);
> > +}
> > +
> >  static int scsi_bus_poweroff(struct device *dev)
> >  {
> >  	return scsi_bus_suspend_common(dev, PMSG_HIBERNATE);
> >  }
> >  
> > +static int scsi_bus_restore(struct device *dev)
> > +{
> > +	return scsi_bus_resume_common(dev, PMSG_RESTORE);
> > +}
> > +
> 
> This is just dumb.  Do the parts that depend on the event type here, in 
> these routines, instead of using a big "switch" statement somewhere 
> else.  For example, have these routines figure out which callback 
> routine or generic routine to use, and pass that pointer to 
> scsi_bus_{suspend,resume}_common() instead of the PMSG_* argument.

Updated in v2, please take a look.

> 
> One thing may help simplify the conversion (you can do this in a
> separate patch).  Linus's current git tree contains a new commit
> 88d26136a256576e444db312179e17af6dd0ea87 (PM: Prevent runtime suspend
> during system resume) which makes some of the code in scsi_pm.c
> unnecessary.  The following two commits can now be reverted:
> 
> 	33a2285d96b5e7b9500612ec623bf4313397bb53
> 	[SCSI] scsi_pm: set device runtime state before parent suspended
> 
> 	28fd00d42cca178638f51c08efa986a777c24a4b
> 	[SCSI] runtime resume parent for child's system-resume
> 
> This will reduce the amount of code you need to worry about.

Yes, thanks for the info.

-Aaron

[PATCH 2/2] [SCSI] 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.

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

sd_runtime_suspend also uses sd_suspend, so when a scsi disk is runtime
suspended, it will be placed to stopped power state.

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

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 12f6fdf..09ab14a 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 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,7 +2919,7 @@ 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 (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 2/2] [SCSI] sd: update sd to use the new pm callbacks

[Alan Stern]

On Wed, 10 Oct 2012, Aaron Lu wrote:

> 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.
> 
> Both suspend and poweroff shares the same routine sd_suspend, which will
> sync flush and then stop the drive, this is the same as before.
> 
> sd_runtime_suspend also uses sd_suspend, so when a scsi disk is runtime
> suspended, it will be placed to stopped power state.

This is not the same as before.  The existing code does not spin down 
the disk during runtime suspend.

Now maybe it should -- I don't know.  The point is that this patch 
makes two separate changes, which is generally not a good idea.

Alan Stern

Re: [PATCH 2/2] [SCSI] sd: update sd to use the new pm callbacks

[Aaron Lu]

On Wed, Oct 10, 2012 at 02:35:47PM -0400, Alan Stern wrote:
> On Wed, 10 Oct 2012, Aaron Lu wrote:
> 
> > 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.
> > 
> > Both suspend and poweroff shares the same routine sd_suspend, which will
> > sync flush and then stop the drive, this is the same as before.
> > 
> > sd_runtime_suspend also uses sd_suspend, so when a scsi disk is runtime
> > suspended, it will be placed to stopped power state.
> 
> This is not the same as before.  The existing code does not spin down 
> the disk during runtime suspend.
> 
> Now maybe it should -- I don't know.  The point is that this patch 
> makes two separate changes, which is generally not a good idea.

Agree. Will seperate, thanks.

-Aaron