[PATCH v3 0/6] ata port runtime power management support

[PATCH v3 0/6] ata port runtime power management support

[Lin Ming]

Hi all,

This is the v3 patches to add ata port runtime pm support.
Applied on top of v3.2-rc2.

v1:
https://lkml.org/lkml/2011/11/2/23

v2:
https://lkml.org/lkml/2011/11/10/71
Acked-by: Tejun Heo <tj@kernel.org>
Kay Sievers reviewed sysfs change

Changes in v3:
- runtime resume scsi device before system suspend (Alan Stern)
- check runtime status in sd_shutdown

Thanks for any comment,
Lin Ming

[PATCH v3 1/6] ata: make ata port as parent device of scsi host

[Lin Ming]

Currently, the device tree of ata port and scsi host looks as below,

        /sys/devices/pci0000:00/0000:00:1f.2    (ahci controller)
        |-- ata1                                (ata port)
        |-- host0                               (scsi host)
           |-- target0:0:0                      (scsi target)
               |-- 0:0:0:0                      (disk)

This patch makes ata port as parent device of scsi host, then it becomes

        /sys/devices/pci0000:00/0000:00:1f.2    (ahci controller)
        |-- ata1                                (ata port)
            |-- host0                           (scsi host)
                |-- target0:0:0                 (scsi target)
                    |-- 0:0:0:0                 (disk)

With this change, the ata port runtime PM is easier.
For example, the ata port runtime suspend will happen as,

disk suspend --> scsi target suspend --> scsi host suspend --> ata port
suspend.

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 drivers/ata/libata-scsi.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 2a5412e..7ae1e77 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3398,7 +3398,7 @@ int ata_scsi_add_hosts(struct ata_host *host, struct scsi_host_template *sht)
 		 */
 		shost->max_host_blocked = 1;
 
-		rc = scsi_add_host(ap->scsi_host, ap->host->dev);
+		rc = scsi_add_host(ap->scsi_host, &ap->tdev);
 		if (rc)
 			goto err_add;
 	}
-- 
1.7.2.5

[PATCH v3 2/6] scsi: add flag to skip the runtime PM calls on the host

[Lin Ming]

With previous change, now the ata port runtime suspend will happen as:

disk suspend --> scsi target suspend --> scsi host suspend --> ata port
suspend

ata port(parent device) suspend need to schedule scsi EH which will resume
scsi host(child device). Then the child device resume will in turn make
parent device resume first. This is kind of recursive.

This patch adds a new flag Scsi_Host::eh_noresume.
ata port will set this flag to skip the runtime PM calls on scsi host.

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 drivers/scsi/scsi_error.c |    5 +++--
 include/scsi/scsi_host.h  |    3 +++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index dc6131e..5f84a14 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -1812,7 +1812,7 @@ int scsi_error_handler(void *data)
 		 * what we need to do to get it up and online again (if we can).
 		 * If we fail, we end up taking the thing offline.
 		 */
-		if (scsi_autopm_get_host(shost) != 0) {
+		if (!shost->eh_noresume && scsi_autopm_get_host(shost) != 0) {
 			SCSI_LOG_ERROR_RECOVERY(1,
 				printk(KERN_ERR "Error handler scsi_eh_%d "
 						"unable to autoresume\n",
@@ -1833,7 +1833,8 @@ int scsi_error_handler(void *data)
 		 * which are still online.
 		 */
 		scsi_restart_operations(shost);
-		scsi_autopm_put_host(shost);
+		if (!shost->eh_noresume)
+			scsi_autopm_put_host(shost);
 		set_current_state(TASK_INTERRUPTIBLE);
 	}
 	__set_current_state(TASK_RUNNING);
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 50266c9..5f7d5b3 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -669,6 +669,9 @@ struct Scsi_Host {
 	/* Asynchronous scan in progress */
 	unsigned async_scan:1;
 
+	/* Don't resume host in EH */
+	unsigned eh_noresume:1;
+
 	/*
 	 * Optional work queue to be utilized by the transport
 	 */
-- 
1.7.2.5

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

[Lin Ming]

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.

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.

 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;
 }
 
-- 
1.7.2.5

[PATCH v3 4/6] [SCSI] sd: check runtime status in sd_shutdown

[Lin Ming]

sd_shutdown is called during reboot/poweroff.
It may fail if parent device, for example, ata port, was runtime suspended.

Fix it by checking runtime status of sd.
Exit immediately if sd was runtime suspended already.

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 drivers/scsi/sd.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index fa3a591..7b3f807 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -50,6 +50,7 @@
 #include <linux/string_helpers.h>
 #include <linux/async.h>
 #include <linux/slab.h>
+#include <linux/pm_runtime.h>
 #include <asm/uaccess.h>
 #include <asm/unaligned.h>
 
@@ -2741,6 +2742,9 @@ static void sd_shutdown(struct device *dev)
 	if (!sdkp)
 		return;         /* this can happen */
 
+	if (pm_runtime_suspended(dev))
+		goto exit;
+
 	if (sdkp->WCE) {
 		sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n");
 		sd_sync_cache(sdkp);
@@ -2751,6 +2755,7 @@ static void sd_shutdown(struct device *dev)
 		sd_start_stop_device(sdkp, 0);
 	}
 
+exit:
 	scsi_disk_put(sdkp);
 }
 
-- 
1.7.2.5

[PATCH v3 5/6] ata: add ata port system PM callbacks

[Lin Ming]

Change ata_host_request_pm to ata_port_request_pm which performs
port suspend/resume.

Add ata port type driver which implements port PM callbacks.

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 drivers/ata/libata-core.c      |  144 ++++++++++++++++++++-------------------
 drivers/ata/libata-transport.c |    1 +
 drivers/ata/libata.h           |    1 +
 3 files changed, 76 insertions(+), 70 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index c04ad68..04c208e 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5234,112 +5234,116 @@ bool ata_link_offline(struct ata_link *link)
 }
 
 #ifdef CONFIG_PM
-static int ata_host_request_pm(struct ata_host *host, pm_message_t mesg,
+static int ata_port_request_pm(struct ata_port *ap, pm_message_t mesg,
 			       unsigned int action, unsigned int ehi_flags,
 			       int wait)
 {
+	struct ata_link *link;
 	unsigned long flags;
-	int i, rc;
-
-	for (i = 0; i < host->n_ports; i++) {
-		struct ata_port *ap = host->ports[i];
-		struct ata_link *link;
+	int rc;
 
-		/* Previous resume operation might still be in
-		 * progress.  Wait for PM_PENDING to clear.
-		 */
-		if (ap->pflags & ATA_PFLAG_PM_PENDING) {
-			ata_port_wait_eh(ap);
-			WARN_ON(ap->pflags & ATA_PFLAG_PM_PENDING);
-		}
+	/* Previous resume operation might still be in
+	 * progress.  Wait for PM_PENDING to clear.
+	 */
+	if (ap->pflags & ATA_PFLAG_PM_PENDING) {
+		ata_port_wait_eh(ap);
+		WARN_ON(ap->pflags & ATA_PFLAG_PM_PENDING);
+	}
 
-		/* request PM ops to EH */
-		spin_lock_irqsave(ap->lock, flags);
+	/* request PM ops to EH */
+	spin_lock_irqsave(ap->lock, flags);
 
-		ap->pm_mesg = mesg;
-		if (wait) {
-			rc = 0;
-			ap->pm_result = &rc;
-		}
+	ap->pm_mesg = mesg;
+	if (wait) {
+		rc = 0;
+		ap->pm_result = &rc;
+	}
 
-		ap->pflags |= ATA_PFLAG_PM_PENDING;
-		ata_for_each_link(link, ap, HOST_FIRST) {
-			link->eh_info.action |= action;
-			link->eh_info.flags |= ehi_flags;
-		}
+	ap->pflags |= ATA_PFLAG_PM_PENDING;
+	ata_for_each_link(link, ap, HOST_FIRST) {
+		link->eh_info.action |= action;
+		link->eh_info.flags |= ehi_flags;
+	}
 
-		ata_port_schedule_eh(ap);
+	ata_port_schedule_eh(ap);
 
-		spin_unlock_irqrestore(ap->lock, flags);
+	spin_unlock_irqrestore(ap->lock, flags);
 
-		/* wait and check result */
-		if (wait) {
-			ata_port_wait_eh(ap);
-			WARN_ON(ap->pflags & ATA_PFLAG_PM_PENDING);
-			if (rc)
-				return rc;
-		}
+	/* wait and check result */
+	if (wait) {
+		ata_port_wait_eh(ap);
+		WARN_ON(ap->pflags & ATA_PFLAG_PM_PENDING);
 	}
 
-	return 0;
+	return rc;
 }
 
+#define to_ata_port(d) container_of(d, struct ata_port, tdev)
+
+static int ata_port_suspend_common(struct device *dev)
+{
+	struct ata_port *ap = to_ata_port(dev);
+	int rc;
+
+	rc = ata_port_request_pm(ap, PMSG_SUSPEND, 0, ATA_EHI_QUIET, 1);
+	return rc;
+}
+
+static int ata_port_suspend(struct device *dev)
+{
+	if (pm_runtime_suspended(dev))
+		return 0;
+
+	return ata_port_suspend_common(dev);
+}
+
+static int ata_port_resume(struct device *dev)
+{
+	struct ata_port *ap = to_ata_port(dev);
+	int rc;
+
+	rc = ata_port_request_pm(ap, PMSG_ON, ATA_EH_RESET,
+		ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET, 1);
+	return rc;
+}
+
+static const struct dev_pm_ops ata_port_pm_ops = {
+	.suspend = ata_port_suspend,
+	.resume = ata_port_resume,
+};
+
 /**
  *	ata_host_suspend - suspend host
  *	@host: host to suspend
  *	@mesg: PM message
  *
- *	Suspend @host.  Actual operation is performed by EH.  This
- *	function requests EH to perform PM operations and waits for EH
- *	to finish.
- *
- *	LOCKING:
- *	Kernel thread context (may sleep).
- *
- *	RETURNS:
- *	0 on success, -errno on failure.
+ *	Suspend @host.  Actual operation is performed by port suspend.
  */
 int ata_host_suspend(struct ata_host *host, pm_message_t mesg)
 {
-	unsigned int ehi_flags = ATA_EHI_QUIET;
-	int rc;
-
-	/*
-	 * On some hardware, device fails to respond after spun down
-	 * for suspend.  As the device won't be used before being
-	 * resumed, we don't need to touch the device.  Ask EH to skip
-	 * the usual stuff and proceed directly to suspend.
-	 *
-	 * http://thread.gmane.org/gmane.linux.ide/46764
-	 */
-	if (mesg.event == PM_EVENT_SUSPEND)
-		ehi_flags |= ATA_EHI_NO_AUTOPSY | ATA_EHI_NO_RECOVERY;
-
-	rc = ata_host_request_pm(host, mesg, 0, ehi_flags, 1);
-	if (rc == 0)
-		host->dev->power.power_state = mesg;
-	return rc;
+	host->dev->power.power_state = mesg;
+	return 0;
 }
 
 /**
  *	ata_host_resume - resume host
  *	@host: host to resume
  *
- *	Resume @host.  Actual operation is performed by EH.  This
- *	function requests EH to perform PM operations and returns.
- *	Note that all resume operations are performed parallelly.
- *
- *	LOCKING:
- *	Kernel thread context (may sleep).
+ *	Resume @host.  Actual operation is performed by port resume.
  */
 void ata_host_resume(struct ata_host *host)
 {
-	ata_host_request_pm(host, PMSG_ON, ATA_EH_RESET,
-			    ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET, 0);
 	host->dev->power.power_state = PMSG_ON;
 }
 #endif
 
+struct device_type ata_port_type = {
+	.name = "ata_port",
+#ifdef CONFIG_PM
+	.pm = &ata_port_pm_ops,
+#endif
+};
+
 /**
  *	ata_dev_init - Initialize an ata_device structure
  *	@dev: Device structure to initialize
diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
index ce9dc62..3ceb3d9 100644
--- a/drivers/ata/libata-transport.c
+++ b/drivers/ata/libata-transport.c
@@ -279,6 +279,7 @@ int ata_tport_add(struct device *parent,
 	struct device *dev = &ap->tdev;
 
 	device_initialize(dev);
+	dev->type = &ata_port_type;
 
 	dev->parent = get_device(parent);
 	dev->release = ata_tport_release;
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 773de97..814486d 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -58,6 +58,7 @@ extern int atapi_passthru16;
 extern int libata_fua;
 extern int libata_noacpi;
 extern int libata_allow_tpm;
+extern struct device_type ata_port_type;
 extern struct ata_link *ata_dev_phys_link(struct ata_device *dev);
 extern void ata_force_cbl(struct ata_port *ap);
 extern u64 ata_tf_to_lba(const struct ata_taskfile *tf);
-- 
1.7.2.5

[PATCH v3 6/6] ata: add ata port runtime PM callbacks

[Lin Ming]

Add ata port runtime suspend/resume/idle callbacks.
Set ->eh_noresume to skip the runtime PM calls on scsi host
in the error handler to avoid dead lock.

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 drivers/ata/libata-core.c      |   10 ++++++++++
 drivers/ata/libata-scsi.c      |    1 +
 drivers/ata/libata-transport.c |    4 ++++
 3 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 04c208e..15a3d4d 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -66,6 +66,7 @@
 #include <asm/byteorder.h>
 #include <linux/cdrom.h>
 #include <linux/ratelimit.h>
+#include <linux/pm_runtime.h>
 
 #include "libata.h"
 #include "libata-transport.h"
@@ -5307,9 +5308,18 @@ static int ata_port_resume(struct device *dev)
 	return rc;
 }
 
+static int ata_port_runtime_idle(struct device *dev)
+{
+	return pm_runtime_suspend(dev);
+}
+
 static const struct dev_pm_ops ata_port_pm_ops = {
 	.suspend = ata_port_suspend,
 	.resume = ata_port_resume,
+
+	.runtime_suspend = ata_port_suspend_common,
+	.runtime_resume = ata_port_resume,
+	.runtime_idle = ata_port_runtime_idle,
 };
 
 /**
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 7ae1e77..508a60b 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3381,6 +3381,7 @@ int ata_scsi_add_hosts(struct ata_host *host, struct scsi_host_template *sht)
 		if (!shost)
 			goto err_alloc;
 
+		shost->eh_noresume = 1;
 		*(struct ata_port **)&shost->hostdata[0] = ap;
 		ap->scsi_host = shost;
 
diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
index 3ceb3d9..9a7f0ea 100644
--- a/drivers/ata/libata-transport.c
+++ b/drivers/ata/libata-transport.c
@@ -32,6 +32,7 @@
 #include <linux/libata.h>
 #include <linux/hdreg.h>
 #include <linux/uaccess.h>
+#include <linux/pm_runtime.h>
 
 #include "libata.h"
 #include "libata-transport.h"
@@ -290,6 +291,9 @@ int ata_tport_add(struct device *parent,
 		goto tport_err;
 	}
 
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
 	transport_add_device(dev);
 	transport_configure_device(dev);
 
-- 
1.7.2.5

Re: [PATCH v3 3/6] [SCSI] runtime resume device before system suspend

[Alan Stern]

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.

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;
>  }

Re: [PATCH v3 3/6] [SCSI] runtime resume device before system suspend

[Lin Ming]

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?

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;

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;
> >  }
> 

Re: [PATCH v3 3/6] [SCSI] runtime resume device before system suspend

[Alan Stern]

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

Re: [PATCH v3 3/6] [SCSI] runtime resume device before system suspend

[Tejun Heo]

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.

-- 
tejun

Re: [PATCH v3 3/6] [SCSI] runtime resume device before system suspend

[Alan Stern]

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

Re: [PATCH v3 3/6] [SCSI] runtime resume device before system suspend

[Tejun Heo]

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.

-- 
tejun

Re: [PATCH v3 3/6] [SCSI] runtime resume device before system suspend

[Alan Stern]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 1576 bytes --]

On Thu, 24 Nov 2011, Tejun Heo wrote:

> 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;
>     }

Almost.  More like this:

	/*
	 * 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 (runtime-suspended) {
		if (target == SUSPEND || target == HIBERNATE)
			return 0;	/* already suspended */
		/* wake up device so that FREEZE will succeed */
		pm_runtime_resume();
	}
	tell device driver to enter target PM state

libata does not contain the SCSI device driver; it contains the host
driver.

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

The above has two lines (plus comments) of optimization.  Okay?

Alan Stern

Re: [PATCH v3 3/6] [SCSI] runtime resume device before system suspend

[Lin Ming]

On Fri, 2011-11-25 at 08:17 +0800, Alan Stern wrote:
> On Thu, 24 Nov 2011, Tejun Heo wrote:
> 
> > 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;
> >     }
> 
> Almost.  More like this:
> 
> 	/*
> 	 * 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 (runtime-suspended) {
> 		if (target == SUSPEND || target == HIBERNATE)
> 			return 0;	/* already suspended */
> 		/* wake up device so that FREEZE will succeed */
> 		pm_runtime_resume();
> 	}
> 	tell device driver to enter target PM state

Below is the updated patch with your suggested patch description and
comments.

Looks OK?

Subject: [PATCH] [SCSI] check runtime PM status in system PM

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.

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 drivers/scsi/scsi_pm.c |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index d329f8b..a633076 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -49,8 +49,22 @@ 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)) {
+		/*
+		 * 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;
 }

--- 
Thanks,
Lin Ming

> 
> libata does not contain the SCSI device driver; it contains the host
> driver.
> 
> > I really don't think we need to optimize this further than this.
> 
> The above has two lines (plus comments) of optimization.  Okay?
> 
> Alan Stern
> 

Re: [PATCH v3 3/6] [SCSI] runtime resume device before system suspend

[Alan Stern]

On Fri, 25 Nov 2011, Lin Ming wrote:

> Below is the updated patch with your suggested patch description and
> comments.
> 
> Looks OK?
> 
> Subject: [PATCH] [SCSI] check runtime PM status in system PM
> 
> 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.
> 
> Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> ---
>  drivers/scsi/scsi_pm.c |   16 +++++++++++++++-
>  1 files changed, 15 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
> index d329f8b..a633076 100644
> --- a/drivers/scsi/scsi_pm.c
> +++ b/drivers/scsi/scsi_pm.c
> @@ -49,8 +49,22 @@ 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)) {
> +		/*
> +		 * 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;
>  }

This looks good to me.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>