[PATCH v2 0/4] ata port runtime power management support

[PATCH v2 0/4] ata port runtime power management support

[Lin Ming]

Hi, all

These 4 patches add ata port runtime pm support.

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

v2 is totally different than v1.

v1 performed ata port runtime pm through scsi layer.
Added hook to scsi host runtime suspend/resume code.

I realized that this is not the natural way to do ata port runtime pm.
It does not deal with the races with ata port system suspend/resume.

With v2, ata port is made to be parent device of scsi host.

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)

v2 changes it to:

        /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)

So ata port runtime PM will happen as:

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

This is much cleaner and natural.

Lin Ming (4):
      ata: make ata port as parent device of scsi host
      scsi: add flag to skip the runtime PM calls on host in EH
      ata: add ata port system PM callbacks
      ata: add ata port runtime PM callbacks

 drivers/ata/libata-core.c      |  158 ++++++++++++++++++++++------------------
 drivers/ata/libata-scsi.c      |    3 +-
 drivers/ata/libata-transport.c |    5 +
 drivers/ata/libata.h           |    1 +
 drivers/scsi/scsi_error.c      |    5 +-
 include/scsi/scsi_host.h       |    3 +
 6 files changed, 102 insertions(+), 73 deletions(-)

Thanks for any comment.
Lin Min

[PATCH v2 1/4] 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 72a9770..4c2a524 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3394,7 +3394,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 v2 2/4] scsi: add flag to skip the runtime PM calls on host in EH

[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 dead lock.

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
in the error handler.

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 v2 3/4] 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      |  139 ++++++++++++++++++++--------------------
 drivers/ata/libata-transport.c |    2 +
 drivers/ata/libata.h           |    1 +
 3 files changed, 72 insertions(+), 70 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index c04ad68..b77c40c 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5234,112 +5234,111 @@ 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(struct device *dev)
+{
+	struct ata_port *ap = to_ata_port(dev);
+	int rc;
+
+	if (ap->pflags & ATA_PFLAG_SUSPENDED)
+		return 0;
+
+	rc = ata_port_request_pm(ap, PMSG_SUSPEND, 0, ATA_EHI_QUIET, 1);
+	return rc;
+}
+
+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..a979bd76 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"
@@ -279,6 +280,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 v2 4/4] 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      |   19 +++++++++++++++++++
 drivers/ata/libata-scsi.c      |    1 +
 drivers/ata/libata-transport.c |    3 +++
 3 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index b77c40c..719d23c 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"
@@ -5302,9 +5303,27 @@ static int ata_port_resume(struct device *dev)
 	return rc;
 }
 
+static int ata_port_runtime_suspend(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_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_runtime_suspend,
+	.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 4c2a524..f7afdd5 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3377,6 +3377,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 a979bd76..9a7f0ea 100644
--- a/drivers/ata/libata-transport.c
+++ b/drivers/ata/libata-transport.c
@@ -291,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 v2 4/4] ata: add ata port runtime PM callbacks

[Alan Stern]

On Thu, 10 Nov 2011, Lin Ming wrote:

> 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.

...

> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -3377,6 +3377,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;

Are you really sure you want to keep this flag set throughout the 
lifetime of the port?

What happens if a command fails, then the port suspends, and then the 
error handler is invoked to take care of the failed command?

Alan Stern

Re: [PATCH v2 4/4] ata: add ata port runtime PM callbacks

[Tejun Heo]

On Thu, Nov 10, 2011 at 10:40:21AM -0500, Alan Stern wrote:
> Are you really sure you want to keep this flag set throughout the 
> lifetime of the port?
> 
> What happens if a command fails, then the port suspends, and then the 
> error handler is invoked to take care of the failed command?

Runtime PM doesn't happen if the device is in use in any way, so
there's no request flying in from upper layers.  While the port is
suspended, qc's which which requested EH are short-circuited.  AFAICS,
nothing is really wrong.

Thanks.

-- 
tejun

Re: [PATCH v2 4/4] ata: add ata port runtime PM callbacks

[Lin Ming]

On Mon, 2011-11-14 at 11:22 +0800, Lin Ming wrote:
> 
> 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      |   19 +++++++++++++++++++
>  drivers/ata/libata-scsi.c      |    1 +
>  drivers/ata/libata-transport.c |    3 +++
>  3 files changed, 23 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index b77c40c..719d23c 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"
> @@ -5302,9 +5303,27 @@ static int ata_port_resume(struct device *dev)
>        return rc;
>  }
> 
> +static int ata_port_runtime_suspend(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_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_runtime_suspend,
> +       .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 4c2a524..f7afdd5 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -3377,6 +3377,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 a979bd76..9a7f0ea 100644
> --- a/drivers/ata/libata-transport.c
> +++ b/drivers/ata/libata-transport.c
> @@ -291,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

Current patches has a bug that system suspend fails if ata port was
runtime suspended.

disk suspend issues sync cache and stop device commands that obviously
need ata port to be active. So we need to runtime resume ata port first.

Alan, Tejun

How about below fix?

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index fa3a591..ebb87fbf 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>
 
@@ -2762,6 +2763,14 @@ static int sd_suspend(struct device *dev, pm_message_t mesg)
 	if (!sdkp)
 		return 0;	/* this can happen */
 
+	/*
+	 * Resume parent device to handle sync cache and
+	 * stop device commands
+	 */
+	ret = pm_runtime_get_sync(dev->parent);
+	if (ret)
+		goto exit;
+
 	if (sdkp->WCE) {
 		sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n");
 		ret = sd_sync_cache(sdkp);
@@ -2775,6 +2784,8 @@ static int sd_suspend(struct device *dev, pm_message_t mesg)
 	}
 
 done:
+	pm_runtime_put_sync(dev->parent);
+exit:
 	scsi_disk_put(sdkp);
 	return ret;
 }

Re: [PATCH v2 4/4] ata: add ata port runtime PM callbacks

[Alan Stern]

On Mon, 14 Nov 2011, Lin Ming wrote:

> Current patches has a bug that system suspend fails if ata port was
> runtime suspended.
> 
> disk suspend issues sync cache and stop device commands that obviously
> need ata port to be active. So we need to runtime resume ata port first.

This is wrong.  If the port is already suspended then so are all the 
drives below the port.  Hence there is no need to sync the cache or 
stop the device.

> Alan, Tejun
> 
> How about below fix?
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index fa3a591..ebb87fbf 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>
>  
> @@ -2762,6 +2763,14 @@ static int sd_suspend(struct device *dev, pm_message_t mesg)
>  	if (!sdkp)
>  		return 0;	/* this can happen */
>  
> +	/*
> +	 * Resume parent device to handle sync cache and
> +	 * stop device commands
> +	 */
> +	ret = pm_runtime_get_sync(dev->parent);
> +	if (ret)
> +		goto exit;
> +
>  	if (sdkp->WCE) {
>  		sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n");
>  		ret = sd_sync_cache(sdkp);

This is not the right approach.  You should look instead at 
scsi_dev_type_suspend() in scsi_pm.c.  If the device is already runtime 
suspended then the routine should return immediately.

Alan Stern

Re: [PATCH v2 4/4] ata: add ata port runtime PM callbacks

[Lin Ming]

On Mon, 2011-11-14 at 23:31 +0800, Alan Stern wrote:
> On Mon, 14 Nov 2011, Lin Ming wrote:
> 
> > Current patches has a bug that system suspend fails if ata port was
> > runtime suspended.
> > 
> > disk suspend issues sync cache and stop device commands that obviously
> > need ata port to be active. So we need to runtime resume ata port first.
> 
> This is wrong.  If the port is already suspended then so are all the 
> drives below the port.  Hence there is no need to sync the cache or 
> stop the device.

Ah, got it now!

> 
> > Alan, Tejun
> > 
> > How about below fix?
> > 
> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> > index fa3a591..ebb87fbf 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>
> >  
> > @@ -2762,6 +2763,14 @@ static int sd_suspend(struct device *dev, pm_message_t mesg)
> >  	if (!sdkp)
> >  		return 0;	/* this can happen */
> >  
> > +	/*
> > +	 * Resume parent device to handle sync cache and
> > +	 * stop device commands
> > +	 */
> > +	ret = pm_runtime_get_sync(dev->parent);
> > +	if (ret)
> > +		goto exit;
> > +
> >  	if (sdkp->WCE) {
> >  		sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n");
> >  		ret = sd_sync_cache(sdkp);
> 
> This is not the right approach.  You should look instead at 
> scsi_dev_type_suspend() in scsi_pm.c.  If the device is already runtime 
> suspended then the routine should return immediately.

How about below?

diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index d329f8b..94b60bd 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -20,6 +20,9 @@ static int scsi_dev_type_suspend(struct device *dev, pm_message_t msg)
 	struct device_driver *drv;
 	int err;
 
+	if (pm_runtime_suspended(dev))
+		return 0;
+
 	err = scsi_device_quiesce(to_scsi_device(dev));
 	if (err == 0) {
 		drv = dev->driver;


> 
> Alan Stern
> 

Re: [PATCH v2 4/4] ata: add ata port runtime PM callbacks

[Tejun Heo]

On Tue, Nov 15, 2011 at 04:51:29PM +0800, Lin Ming wrote:
> > This is not the right approach.  You should look instead at 
> > scsi_dev_type_suspend() in scsi_pm.c.  If the device is already runtime 
> > suspended then the routine should return immediately.
> 
> How about below?
> 
> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
> index d329f8b..94b60bd 100644
> --- a/drivers/scsi/scsi_pm.c
> +++ b/drivers/scsi/scsi_pm.c
> @@ -20,6 +20,9 @@ static int scsi_dev_type_suspend(struct device *dev, pm_message_t msg)
>  	struct device_driver *drv;
>  	int err;
>  
> +	if (pm_runtime_suspended(dev))
> +		return 0;
> +

Something to be careful about is there are different types of suspend
states (PMSG_*).  IIUC, runtime PM is using PMSG_SUSPEND.  Other
states may or may not be compatible with PMSG_SUSPEND expectations, so
you can skip suspend operation if the newly requested state is
PMSG_SUSPEND but otherwise the controller needs to be woken up and
told to comply to the new state.

Thanks.

-- 
tejun

Re: [PATCH v2 4/4] ata: add ata port runtime PM callbacks

[Alan Stern]

On Tue, 15 Nov 2011, Tejun Heo wrote:

> On Tue, Nov 15, 2011 at 04:51:29PM +0800, Lin Ming wrote:
> > > This is not the right approach.  You should look instead at 
> > > scsi_dev_type_suspend() in scsi_pm.c.  If the device is already runtime 
> > > suspended then the routine should return immediately.
> > 
> > How about below?
> > 
> > diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
> > index d329f8b..94b60bd 100644
> > --- a/drivers/scsi/scsi_pm.c
> > +++ b/drivers/scsi/scsi_pm.c
> > @@ -20,6 +20,9 @@ static int scsi_dev_type_suspend(struct device *dev, pm_message_t msg)
> >  	struct device_driver *drv;
> >  	int err;
> >  
> > +	if (pm_runtime_suspended(dev))
> > +		return 0;
> > +
> 
> Something to be careful about is there are different types of suspend
> states (PMSG_*).  IIUC, runtime PM is using PMSG_SUSPEND.  Other
> states may or may not be compatible with PMSG_SUSPEND expectations, so
> you can skip suspend operation if the newly requested state is
> PMSG_SUSPEND but otherwise the controller needs to be woken up and
> told to comply to the new state.

That's right.  Surprisingly enough (and contrary to what I wrote
earlier), the sd_suspend() routine doesn't spin down a drive for
runtime suspend.  This probably should be considered a bug.

Anyway, it looks like the correct approach would be more like this:

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

Alan Stern

Re: [PATCH v2 4/4] ata: add ata port runtime PM callbacks

[Lin Ming]

On Wed, 2011-11-16 at 00:08 +0800, Alan Stern wrote:
> On Tue, 15 Nov 2011, Tejun Heo wrote:
> 
> > On Tue, Nov 15, 2011 at 04:51:29PM +0800, Lin Ming wrote:
> > > > This is not the right approach.  You should look instead at 
> > > > scsi_dev_type_suspend() in scsi_pm.c.  If the device is already runtime 
> > > > suspended then the routine should return immediately.
> > > 
> > > How about below?
> > > 
> > > diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
> > > index d329f8b..94b60bd 100644
> > > --- a/drivers/scsi/scsi_pm.c
> > > +++ b/drivers/scsi/scsi_pm.c
> > > @@ -20,6 +20,9 @@ static int scsi_dev_type_suspend(struct device *dev, pm_message_t msg)
> > >  	struct device_driver *drv;
> > >  	int err;
> > >  
> > > +	if (pm_runtime_suspended(dev))
> > > +		return 0;
> > > +
> > 
> > Something to be careful about is there are different types of suspend
> > states (PMSG_*).  IIUC, runtime PM is using PMSG_SUSPEND.  Other
> > states may or may not be compatible with PMSG_SUSPEND expectations, so
> > you can skip suspend operation if the newly requested state is
> > PMSG_SUSPEND but otherwise the controller needs to be woken up and
> > told to comply to the new state.
> 
> That's right.  Surprisingly enough (and contrary to what I wrote
> earlier), the sd_suspend() routine doesn't spin down a drive for
> runtime suspend.  This probably should be considered a bug.
> 
> Anyway, it looks like the correct approach would be more like this:

Thanks.

I think ata_port_suspend also needs to call pm_runtime_resume, as below.

static int ata_port_suspend(struct device *dev)
{
        struct ata_port *ap = to_ata_port(dev);
        int rc;

        pm_runtime_resume(dev);
        rc = ata_port_request_pm(ap, PMSG_SUSPEND, 0, ATA_EHI_QUIET, 1);
        return rc;
}

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

Re: [PATCH v2 4/4] ata: add ata port runtime PM callbacks

[Alan Stern]

On Wed, 16 Nov 2011, Lin Ming wrote:

> > > Something to be careful about is there are different types of suspend
> > > states (PMSG_*).  IIUC, runtime PM is using PMSG_SUSPEND.  Other
> > > states may or may not be compatible with PMSG_SUSPEND expectations, so
> > > you can skip suspend operation if the newly requested state is
> > > PMSG_SUSPEND but otherwise the controller needs to be woken up and
> > > told to comply to the new state.
> > 
> > That's right.  Surprisingly enough (and contrary to what I wrote
> > earlier), the sd_suspend() routine doesn't spin down a drive for
> > runtime suspend.  This probably should be considered a bug.
> > 
> > Anyway, it looks like the correct approach would be more like this:
> 
> Thanks.
> 
> I think ata_port_suspend also needs to call pm_runtime_resume, as below.
> 
> static int ata_port_suspend(struct device *dev)
> {
>         struct ata_port *ap = to_ata_port(dev);
>         int rc;
> 
>         pm_runtime_resume(dev);
>         rc = ata_port_request_pm(ap, PMSG_SUSPEND, 0, ATA_EHI_QUIET, 1);
>         return rc;
> }

Maybe not.  You know a lot more about the state of the ATA ports than
you do about the state of the SCSI devices.  If there's no difference
in the port states for runtime suspend and system sleep then you don't
have to power up the port just in order to power it down again.

Alan Stern

Re: [PATCH v2 0/4] ata port runtime power management support

[Tejun Heo]

Hello,

(cc'ing Kay for the sysfs tree hierarchy change)

On Thu, Nov 10, 2011 at 02:22:42PM +0800, Lin Ming wrote:
> These 4 patches add ata port runtime pm support.
> 
> v1:
> https://lkml.org/lkml/2011/11/2/23
> 
> v2 is totally different than v1.
> 
> v1 performed ata port runtime pm through scsi layer.
> Added hook to scsi host runtime suspend/resume code.
> 
> I realized that this is not the natural way to do ata port runtime pm.
> It does not deal with the races with ata port system suspend/resume.
> 
> With v2, ata port is made to be parent device of scsi host.
> 
> 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)
> 
> v2 changes it to:
> 
>         /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)
> 
> So ata port runtime PM will happen as:
> 
> disk suspend --> scsi target suspend --> scsi host suspend --> ata port
> suspend.
> 
> This is much cleaner and natural.

Yeah, I really like this approach.  Nicely done.  I *think* the
hierarchy change should be okay but cc'ing Kay just in case.

 Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [PATCH v2 0/4] ata port runtime power management support

[Tejun Heo]

Ooh, by the way, can you please write more about how you tested it?

Thanks.

-- 
tejun

Re: [PATCH v2 0/4] ata port runtime power management support

[Lin Ming]

On Thu, 2011-11-10 at 23:31 +0800, Tejun Heo wrote:
> Ooh, by the way, can you please write more about how you tested it?

I have 2 disks installed.
sda installs the OS. I use sdb to test ata port runtime PM.

# set "auto" to enable disk runtime PM
# echo auto > /sys/devices/pci0000:00/0000:00:1f.2/ata3/host2/target2:0:0/2:0:0:0/power/control

mount sdb1 causes ata port resume

umount sdb1 causes ata port suspend

> 
> Thanks.
> 

Re: [PATCH v2 0/4] ata port runtime power management support

[Kay Sievers]

On Thu, Nov 10, 2011 at 16:30, Tejun Heo <tj@kernel.org> wrote:
> On Thu, Nov 10, 2011 at 02:22:42PM +0800, Lin Ming wrote:

>> v2 changes it to:
>>
>>         /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)
>>
>> So ata port runtime PM will happen as:

>> This is much cleaner and natural.
>
> Yeah, I really like this approach.  Nicely done.  I *think* the
> hierarchy change should be okay but cc'ing Kay just in case.

Looks good to me. All non-broken users of sysfs should be fine and
cope with changes like this without any problem.

Kay

Re: [PATCH v2 0/4] ata port runtime power management support

[Lin Ming]

On Thu, 2011-11-10 at 23:30 +0800, Tejun Heo wrote:
> 
> Yeah, I really like this approach.  Nicely done.  I *think* the
> hierarchy change should be okay but cc'ing Kay just in case.
> 
>  Acked-by: Tejun Heo <tj@kernel.org>

Thanks!

> 
> Thanks.
> 

Re: [PATCH v2 0/4] ata port runtime power management support

[Alan Stern]

On Thu, 10 Nov 2011, Lin Ming wrote:

> Hi, all
> 
> These 4 patches add ata port runtime pm support.
> 
> v1:
> https://lkml.org/lkml/2011/11/2/23
> 
> v2 is totally different than v1.
> 
> v1 performed ata port runtime pm through scsi layer.
> Added hook to scsi host runtime suspend/resume code.
> 
> I realized that this is not the natural way to do ata port runtime pm.
> It does not deal with the races with ata port system suspend/resume.
> 
> With v2, ata port is made to be parent device of scsi host.
> 
> 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)
> 
> v2 changes it to:
> 
>         /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)
> 
> So ata port runtime PM will happen as:
> 
> disk suspend --> scsi target suspend --> scsi host suspend --> ata port
> suspend.
> 
> This is much cleaner and natural.

Have you observed any real benefit from this feature?

Currently, disks will not be runtime-suspended unless (1) the user 
requests it by setting the appropriate power/control attribute, and (2) 
the device file is closed (in particular, the disk has no mounted 
filesystems or swap partitions).  I don't imagine this combination of 
events is very common for disks using libata.

Alan Stern

Re: [PATCH v2 0/4] ata port runtime power management support

[Tejun Heo]

On Thu, Nov 10, 2011 at 10:30:53AM -0500, Alan Stern wrote:
> Currently, disks will not be runtime-suspended unless (1) the user 
> requests it by setting the appropriate power/control attribute, and (2) 
> the device file is closed (in particular, the disk has no mounted 
> filesystems or swap partitions).  I don't imagine this combination of 
> events is very common for disks using libata.

Empty ports are pretty common and pm should be able to put the
unoccupied ports and the containing pci device into sleep state which
can save some power.  Would be nice to have some numbers tho.  But
either way, if this doesn't break anything, I don't see any reason to
object to it - the hierarchy reorg is more of logical cleanup and
runtime pm is trivial afterwards.

Thanks.

-- 
tejun