diff mbox

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

Message ID 1321247661.6599.110.camel@minggr
State Not Applicable
Delegated to: David Miller
Headers show

Commit Message

Lin Ming Nov. 14, 2011, 5:14 a.m. UTC 
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?



--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Alan Stern Nov. 14, 2011, 3:31 p.m. UTC | #1 
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

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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