[PATCH 0/3] ata port runtime power management support

[PATCH 0/3] ata port runtime power management support

[Lin Ming]

Hi, all

These 3 patches add ata port runtime pm support, which is done through scsi layer.
scsi host runtime pm mechanism is already there(scsi_pm.c).
These patches add hooks for ata port.

Lin Ming (3):
      scsi: fix potential dead lock for host runtime pm
      scsi: add hooks for host runtime power management
      ata: implement ata port runtime pm hooks

 drivers/ata/libata-core.c |   96 +++++++++++++++++++++++++++++---------------
 drivers/scsi/scsi_error.c |    4 +-
 drivers/scsi/scsi_pm.c    |   45 +++++++++++++++++++-
 drivers/scsi/scsi_priv.h  |    2 +
 include/linux/libata.h    |    7 +++
 include/scsi/scsi_host.h  |    8 ++++
 6 files changed, 125 insertions(+), 37 deletions(-)

Thanks for any comment.
Lin Ming

[PATCH 1/3] scsi: fix potential dead lock for host runtime pm

[Lin Ming]

In later patch hooks will be added to do ata port runtime pm through scsi layer.
libata schedules scsi EH to handle suspend, then dead lock happens
because scsi EH in turn waits for the ongoing suspend, as below.

<scsi host runtime suspend>
 scsi_autopm_put_host
   pm_runtime_put_sync
     <scsi_host runtime pm status updated to RPM_SUSPENDING>
     ......
       <call libata hook to do suspend>
         <wake up scsi EH to handle suspend>
         <wait for scsi EH ...>

<scsi EH wake up>
 scsi_error_handler
   <resume scsi host>
   scsi_autopm_get_host
     pm_runtime_get_sync
     .....
       <sleep to wait for the ongoing scsi host suspend>

This patch fixes the dead lock by checking if there is ongoing runtime PM request.
If there is ongoing runtime PM request, scsi_autopm_get_host_noresume is called to
increase the usage count, but don't resume the host.

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

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index a4b9cdb..d35d8f7 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -1804,7 +1804,9 @@ 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 (scsi_autopm_host_busy(shost))
+			scsi_autopm_get_host_noresume(shost);
+		else if (scsi_autopm_get_host(shost) != 0) {
 			SCSI_LOG_ERROR_RECOVERY(1,
 				printk(KERN_ERR "Error handler scsi_eh_%d "
 						"unable to autoresume\n",
diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index d82a023a..1be6c5a 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -185,6 +185,17 @@ void scsi_autopm_put_host(struct Scsi_Host *shost)
 	pm_runtime_put_sync(&shost->shost_gendev);
 }
 
+bool scsi_autopm_host_busy(struct Scsi_Host *shost)
+{
+	return (shost->shost_gendev.power.runtime_status == RPM_RESUMING
+		|| shost->shost_gendev.power.runtime_status == RPM_SUSPENDING);
+}
+
+void scsi_autopm_get_host_noresume(struct Scsi_Host *shost)
+{
+	pm_runtime_get_noresume(&shost->shost_gendev);
+}
+
 #else
 
 #define scsi_runtime_suspend	NULL
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index 2a58895..1750651 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -156,6 +156,8 @@ extern void scsi_autopm_get_target(struct scsi_target *);
 extern void scsi_autopm_put_target(struct scsi_target *);
 extern int scsi_autopm_get_host(struct Scsi_Host *);
 extern void scsi_autopm_put_host(struct Scsi_Host *);
+extern bool scsi_autopm_host_busy(struct Scsi_Host *shost);
+extern void scsi_autopm_get_host_noresume(struct Scsi_Host *);
 #else
 static inline void scsi_autopm_get_target(struct scsi_target *t) {}
 static inline void scsi_autopm_put_target(struct scsi_target *t) {}
-- 
1.7.2.5

Re: [PATCH 1/3] scsi: fix potential dead lock for host runtime pm

[Alan Stern]

On Wed, 2 Nov 2011, Lin Ming wrote:

> In later patch hooks will be added to do ata port runtime pm through scsi layer.
> libata schedules scsi EH to handle suspend, then dead lock happens
> because scsi EH in turn waits for the ongoing suspend, as below.
> 
> <scsi host runtime suspend>
>  scsi_autopm_put_host
>    pm_runtime_put_sync
>      <scsi_host runtime pm status updated to RPM_SUSPENDING>
>      ......
>        <call libata hook to do suspend>
>          <wake up scsi EH to handle suspend>
>          <wait for scsi EH ...>
> 
> <scsi EH wake up>
>  scsi_error_handler
>    <resume scsi host>
>    scsi_autopm_get_host
>      pm_runtime_get_sync
>      .....
>        <sleep to wait for the ongoing scsi host suspend>
> 
> This patch fixes the dead lock by checking if there is ongoing runtime PM request.
> If there is ongoing runtime PM request, scsi_autopm_get_host_noresume is called to
> increase the usage count, but don't resume the host.
> 
> Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> ---
>  drivers/scsi/scsi_error.c |    4 +++-
>  drivers/scsi/scsi_pm.c    |   11 +++++++++++
>  drivers/scsi/scsi_priv.h  |    2 ++
>  3 files changed, 16 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index a4b9cdb..d35d8f7 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -1804,7 +1804,9 @@ 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 (scsi_autopm_host_busy(shost))
> +			scsi_autopm_get_host_noresume(shost);
> +		else if (scsi_autopm_get_host(shost) != 0) {
>  			SCSI_LOG_ERROR_RECOVERY(1,
>  				printk(KERN_ERR "Error handler scsi_eh_%d "
>  						"unable to autoresume\n",

Here's what I'm worried about:  Suppose during normal operation, an 
error occurs.  When the command is completed, runtime PM might start to 
suspend the host.  But then the error-handler thread starts up, and 
of course it needs the host to be powered-up in order to recover from 
the error.  The code you're adding could prevent this from working.

What we really need is a way to prevent host from going into runtime
suspend while the error-handler is pending or running.  Do you have any
ideas on how to do this?

> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
> index d82a023a..1be6c5a 100644
> --- a/drivers/scsi/scsi_pm.c
> +++ b/drivers/scsi/scsi_pm.c
> @@ -185,6 +185,17 @@ void scsi_autopm_put_host(struct Scsi_Host *shost)
>  	pm_runtime_put_sync(&shost->shost_gendev);
>  }
>  
> +bool scsi_autopm_host_busy(struct Scsi_Host *shost)
> +{
> +	return (shost->shost_gendev.power.runtime_status == RPM_RESUMING
> +		|| shost->shost_gendev.power.runtime_status == RPM_SUSPENDING);
> +}
> +
> +void scsi_autopm_get_host_noresume(struct Scsi_Host *shost)
> +{
> +	pm_runtime_get_noresume(&shost->shost_gendev);
> +}
> +
>  #else
>  
>  #define scsi_runtime_suspend	NULL
> diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
> index 2a58895..1750651 100644
> --- a/drivers/scsi/scsi_priv.h
> +++ b/drivers/scsi/scsi_priv.h
> @@ -156,6 +156,8 @@ extern void scsi_autopm_get_target(struct scsi_target *);
>  extern void scsi_autopm_put_target(struct scsi_target *);
>  extern int scsi_autopm_get_host(struct Scsi_Host *);
>  extern void scsi_autopm_put_host(struct Scsi_Host *);
> +extern bool scsi_autopm_host_busy(struct Scsi_Host *shost);
> +extern void scsi_autopm_get_host_noresume(struct Scsi_Host *);
>  #else
>  static inline void scsi_autopm_get_target(struct scsi_target *t) {}
>  static inline void scsi_autopm_put_target(struct scsi_target *t) {}

I bet you didn't try compiling this with CONFIG_PM_RUNTIME turned off.

Alan Stern

[PATCH 2/3] scsi: add hooks for host runtime power management

[Lin Ming]

Adds ->suspend and ->resume callbacks in struct scsi_host_template
to do host runtime power management.

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

diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index 1be6c5a..5742bed2 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -42,6 +42,28 @@ static int scsi_dev_type_resume(struct device *dev)
 	return err;
 }
 
+static int scsi_host_suspend(struct device *dev)
+{
+	struct Scsi_Host *shost = dev_to_shost(dev);
+	int err = 0;
+
+	if (shost->hostt->suspend)
+		err = shost->hostt->suspend(shost);
+
+	return err;
+}
+
+static int scsi_host_resume(struct device *dev)
+{
+	struct Scsi_Host *shost = dev_to_shost(dev);
+	int err = 0;
+
+	if (shost->hostt->resume)
+		err = shost->hostt->resume(shost);
+
+	return err;
+}
+
 #ifdef CONFIG_PM_SLEEP
 
 static int scsi_bus_suspend_common(struct device *dev, pm_message_t msg)
@@ -106,7 +128,10 @@ static int scsi_runtime_suspend(struct device *dev)
 				round_jiffies_up_relative(HZ/10)));
 	}
 
-	/* Insert hooks here for targets, hosts, and transport classes */
+	/* Insert hooks here for targets and transport classes */
+
+	if (scsi_is_host_device(dev))
+		err = scsi_host_suspend(dev);
 
 	return err;
 }
@@ -119,7 +144,10 @@ static int scsi_runtime_resume(struct device *dev)
 	if (scsi_is_sdev_device(dev))
 		err = scsi_dev_type_resume(dev);
 
-	/* Insert hooks here for targets, hosts, and transport classes */
+	/* Insert hooks here for targets and transport classes */
+
+	if (scsi_is_host_device(dev))
+		err = scsi_host_resume(dev);
 
 	return err;
 }
@@ -132,7 +160,7 @@ static int scsi_runtime_idle(struct device *dev)
 
 	/* Insert hooks here for targets, hosts, and transport classes */
 
-	if (scsi_is_sdev_device(dev))
+	if (scsi_is_sdev_device(dev) || scsi_is_host_device(dev))
 		err = pm_schedule_suspend(dev, 100);
 	else
 		err = pm_runtime_suspend(dev);
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index f1f2644..512e2a0 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -356,6 +356,14 @@ struct scsi_host_template {
 	enum blk_eh_timer_return (*eh_timed_out)(struct scsi_cmnd *);
 
 	/*
+	 * Optional routine for scsi host runtime pm.
+	 *
+	 * Status: OPTIONAL
+	 */
+	int (*suspend)(struct Scsi_Host *);
+	int (*resume)(struct Scsi_Host *);
+
+	/*
 	 * Name of proc directory
 	 */
 	const char *proc_name;
-- 
1.7.2.5

Re: [PATCH 2/3] scsi: add hooks for host runtime power management

[Alan Stern]

On Wed, 2 Nov 2011, Lin Ming wrote:

> Adds ->suspend and ->resume callbacks in struct scsi_host_template
> to do host runtime power management.
> 
> Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> ---
>  drivers/scsi/scsi_pm.c   |   34 +++++++++++++++++++++++++++++++---
>  include/scsi/scsi_host.h |    8 ++++++++
>  2 files changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
> index 1be6c5a..5742bed2 100644
> --- a/drivers/scsi/scsi_pm.c
> +++ b/drivers/scsi/scsi_pm.c
> @@ -42,6 +42,28 @@ static int scsi_dev_type_resume(struct device *dev)
>  	return err;
>  }
>  
> +static int scsi_host_suspend(struct device *dev)
> +{
> +	struct Scsi_Host *shost = dev_to_shost(dev);
> +	int err = 0;
> +
> +	if (shost->hostt->suspend)
> +		err = shost->hostt->suspend(shost);
> +
> +	return err;
> +}
> +
> +static int scsi_host_resume(struct device *dev)
> +{
> +	struct Scsi_Host *shost = dev_to_shost(dev);
> +	int err = 0;
> +
> +	if (shost->hostt->resume)
> +		err = shost->hostt->resume(shost);
> +
> +	return err;
> +}

These don't need to be independent routines.  You can put them inline 
in scsi_runtime_suspend() and scsi_runtime_resume() below; that's how 
the device code is written.

> @@ -132,7 +160,7 @@ static int scsi_runtime_idle(struct device *dev)
>  
>  	/* Insert hooks here for targets, hosts, and transport classes */
>  
> -	if (scsi_is_sdev_device(dev))
> +	if (scsi_is_sdev_device(dev) || scsi_is_host_device(dev))
>  		err = pm_schedule_suspend(dev, 100);

This is wrong.  The 100-ms delay is meant for devices only.  There's 
no reason to delay suspending a host once everything beneath it is 
suspended.  Just leave the code the way it is now.

>  	else
>  		err = pm_runtime_suspend(dev);
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index f1f2644..512e2a0 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -356,6 +356,14 @@ struct scsi_host_template {
>  	enum blk_eh_timer_return (*eh_timed_out)(struct scsi_cmnd *);
>  
>  	/*
> +	 * Optional routine for scsi host runtime pm.
> +	 *
> +	 * Status: OPTIONAL
> +	 */
> +	int (*suspend)(struct Scsi_Host *);
> +	int (*resume)(struct Scsi_Host *);

The comment should explain what drivers are supposed to do when these 
hooks are called.  In particular, the suspend callback should _not_ 
power-down the entire host adapter; it should power-down only the 
circuitry that controls the interface to the SCSI bus.  The upstream 
interface to the CPU should remain at full power.

If there's no way to power-down just that part of the host adapter then 
there's no need to have these callbacks at all.  The low-level driver 
can simply use the runtime PM API on its own struct device.

Alan Stern

Re: [PATCH 2/3] scsi: add hooks for host runtime power management

[Lin Ming]

On Wed, 2011-11-02 at 22:53 +0800, Alan Stern wrote:
> On Wed, 2 Nov 2011, Lin Ming wrote:
> 
> > Adds ->suspend and ->resume callbacks in struct scsi_host_template
> > to do host runtime power management.
> > 
> > Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> > ---
> >  drivers/scsi/scsi_pm.c   |   34 +++++++++++++++++++++++++++++++---
> >  include/scsi/scsi_host.h |    8 ++++++++
> >  2 files changed, 39 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
> > index 1be6c5a..5742bed2 100644
> > --- a/drivers/scsi/scsi_pm.c
> > +++ b/drivers/scsi/scsi_pm.c
> > @@ -42,6 +42,28 @@ static int scsi_dev_type_resume(struct device *dev)
> >  	return err;
> >  }
> >  
> > +static int scsi_host_suspend(struct device *dev)
> > +{
> > +	struct Scsi_Host *shost = dev_to_shost(dev);
> > +	int err = 0;
> > +
> > +	if (shost->hostt->suspend)
> > +		err = shost->hostt->suspend(shost);
> > +
> > +	return err;
> > +}
> > +
> > +static int scsi_host_resume(struct device *dev)
> > +{
> > +	struct Scsi_Host *shost = dev_to_shost(dev);
> > +	int err = 0;
> > +
> > +	if (shost->hostt->resume)
> > +		err = shost->hostt->resume(shost);
> > +
> > +	return err;
> > +}
> 
> These don't need to be independent routines.  You can put them inline 
> in scsi_runtime_suspend() and scsi_runtime_resume() below; that's how 
> the device code is written.

OK.

> 
> > @@ -132,7 +160,7 @@ static int scsi_runtime_idle(struct device *dev)
> >  
> >  	/* Insert hooks here for targets, hosts, and transport classes */
> >  
> > -	if (scsi_is_sdev_device(dev))
> > +	if (scsi_is_sdev_device(dev) || scsi_is_host_device(dev))
> >  		err = pm_schedule_suspend(dev, 100);
> 
> This is wrong.  The 100-ms delay is meant for devices only.  There's 
> no reason to delay suspending a host once everything beneath it is 
> suspended.  Just leave the code the way it is now.

OK.

> 
> >  	else
> >  		err = pm_runtime_suspend(dev);
> > diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> > index f1f2644..512e2a0 100644
> > --- a/include/scsi/scsi_host.h
> > +++ b/include/scsi/scsi_host.h
> > @@ -356,6 +356,14 @@ struct scsi_host_template {
> >  	enum blk_eh_timer_return (*eh_timed_out)(struct scsi_cmnd *);
> >  
> >  	/*
> > +	 * Optional routine for scsi host runtime pm.
> > +	 *
> > +	 * Status: OPTIONAL
> > +	 */
> > +	int (*suspend)(struct Scsi_Host *);
> > +	int (*resume)(struct Scsi_Host *);
> 
> The comment should explain what drivers are supposed to do when these 
> hooks are called.  In particular, the suspend callback should _not_ 
> power-down the entire host adapter; it should power-down only the 
> circuitry that controls the interface to the SCSI bus.  The upstream 
> interface to the CPU should remain at full power.
> 
> If there's no way to power-down just that part of the host adapter then 
> there's no need to have these callbacks at all.  The low-level driver 
> can simply use the runtime PM API on its own struct device.

Good point.

I realize that this is not the natural way to do ata port runtime pm.
Hooking it to scsi host runtime pm is not good. It does not deal with
the races with system suspend/resume of host controller.

How about making ata port as the parent device of scsi host?
Then, for example, the runtime suspend happens as below,

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

Current device tree is:
/sys/devices/pci0000:00/0000:00:1f.2/
|-- ata1
|-- host0

After the change, the tree will become as:
/sys/devices/pci0000:00/0000:00:1f.2/ata1/
|-- host0

The tricky part is 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.

We can fix this by adding a flag somewhere to tell scsi EH don't resume
the host in ata port pm request handling case.

What do you think?

Thanks,
Lin Ming

Re: [PATCH 2/3] scsi: add hooks for host runtime power management

[Alan Stern]

On Thu, 3 Nov 2011, Lin Ming wrote:

> I realize that this is not the natural way to do ata port runtime pm.
> Hooking it to scsi host runtime pm is not good. It does not deal with
> the races with system suspend/resume of host controller.
> 
> How about making ata port as the parent device of scsi host?
> Then, for example, the runtime suspend happens as below,
> 
> disk suspend --> scsi target suspend --> scsi host suspend --> ata port
> suspend.
> 
> Current device tree is:
> /sys/devices/pci0000:00/0000:00:1f.2/
> |-- ata1
> |-- host0
> 
> After the change, the tree will become as:
> /sys/devices/pci0000:00/0000:00:1f.2/ata1/
> |-- host0

I don't know enough about the ATA subsystem to say much, except that 
this looks more logical.

> The tricky part is 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.
> 
> We can fix this by adding a flag somewhere to tell scsi EH don't resume
> the host in ata port pm request handling case.
> 
> What do you think?

Yes, it is a problem.  But it looks like the underlying issue is that
you're using the SCSI error handler to do something it was not intended
for.  Can't you suspend and resume the ATA port without using the error
handler?

Alan Stern

Re: [PATCH 2/3] scsi: add hooks for host runtime power management

[Lin Ming]

On Thu, 2011-11-03 at 22:22 +0800, Alan Stern wrote:
> On Thu, 3 Nov 2011, Lin Ming wrote:
> 
> > I realize that this is not the natural way to do ata port runtime pm.
> > Hooking it to scsi host runtime pm is not good. It does not deal with
> > the races with system suspend/resume of host controller.
> > 
> > How about making ata port as the parent device of scsi host?
> > Then, for example, the runtime suspend happens as below,
> > 
> > disk suspend --> scsi target suspend --> scsi host suspend --> ata port
> > suspend.
> > 
> > Current device tree is:
> > /sys/devices/pci0000:00/0000:00:1f.2/
> > |-- ata1
> > |-- host0
> > 
> > After the change, the tree will become as:
> > /sys/devices/pci0000:00/0000:00:1f.2/ata1/
> > |-- host0
> 
> I don't know enough about the ATA subsystem to say much, except that 
> this looks more logical.
> 
> > The tricky part is 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.
> > 
> > We can fix this by adding a flag somewhere to tell scsi EH don't resume
> > the host in ata port pm request handling case.
> > 
> > What do you think?
> 
> Yes, it is a problem.  But it looks like the underlying issue is that
> you're using the SCSI error handler to do something it was not intended
> for.  Can't you suspend and resume the ATA port without using the error
> handler?

The system suspend and resume of the ATA port uses the error handler.
I think the runtime suspend and resume should use the error handler too.

Tejun,

Could you comment more on this?

Thanks.

> 
> Alan Stern
> 

Re: [PATCH 2/3] scsi: add hooks for host runtime power management

[Tejun Heo]

Hello,

On Thu, Nov 3, 2011 at 7:37 AM, Lin Ming <ming.m.lin@intel.com> wrote:
>> Yes, it is a problem.  But it looks like the underlying issue is that
>> you're using the SCSI error handler to do something it was not intended
>> for.  Can't you suspend and resume the ATA port without using the error
>> handler?
>
> The system suspend and resume of the ATA port uses the error handler.
> I think the runtime suspend and resume should use the error handler too.
>
> Tejun,
>
> Could you comment more on this?

I don't know. I haven't really thought about it but as it's currently
designed, I don't think it'll be possible to avoid going through EH to
put ATA ports into suspend mode.

Thanks.

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

Re: [PATCH 2/3] scsi: add hooks for host runtime power management

[Alan Stern]

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

On Thu, 3 Nov 2011, Tejun Heo wrote:

> Hello,
> 
> On Thu, Nov 3, 2011 at 7:37 AM, Lin Ming <ming.m.lin@intel.com> wrote:
> >> Yes, it is a problem.  But it looks like the underlying issue is that
> >> you're using the SCSI error handler to do something it was not intended
> >> for.  Can't you suspend and resume the ATA port without using the error
> >> handler?
> >
> > The system suspend and resume of the ATA port uses the error handler.
> > I think the runtime suspend and resume should use the error handler too.
> >
> > Tejun,
> >
> > Could you comment more on this?
> 
> I don't know. I haven't really thought about it but as it's currently
> designed, I don't think it'll be possible to avoid going through EH to
> put ATA ports into suspend mode.

Why not?

Alan Stern

Re: [PATCH 2/3] scsi: add hooks for host runtime power management

[Tejun Heo]

Hello,

On Thu, Nov 3, 2011 at 8:39 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> I don't know. I haven't really thought about it but as it's currently
>> designed, I don't think it'll be possible to avoid going through EH to
>> put ATA ports into suspend mode.
>
> Why not?

It's just how things are designed now.  There's no reason why it's
fundamentally impossible but going around that would require some
amount of hackery or preferably re-design.  Synchronization against
command processing, interrupts and all are built around EH.

Thanks.

-- 
tejun

Re: [PATCH 2/3] scsi: add hooks for host runtime power management

[Alan Stern]

On Thu, 3 Nov 2011, Tejun Heo wrote:

> Hello,
> 
> On Thu, Nov 3, 2011 at 8:39 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> >> I don't know. I haven't really thought about it but as it's currently
> >> designed, I don't think it'll be possible to avoid going through EH to
> >> put ATA ports into suspend mode.
> >
> > Why not?
> 
> It's just how things are designed now.  There's no reason why it's
> fundamentally impossible but going around that would require some
> amount of hackery or preferably re-design.  Synchronization against
> command processing, interrupts and all are built around EH.

Then it sounds like the best idea is something like what Ming proposed 
earlier: Have the error handler make some sort of test to determine 
whether it has been invoked for suspend/resume handling, and skip the 
runtime-PM calls on the host if it has.

This new test, whatever it is, should apply both to system suspend and 
runtime suspend.  By the way, what would happen if a system suspend 
occurred while the ATA port was already runtime-suspended?

Alan Stern

[PATCH 3/3] ata: implement ata port runtime pm hooks

[Lin Ming]

Split a new function ata_port_request_pm from ata_host_request_pm.
Implement runtime suspend and resume hooks for scsi layer.

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 drivers/ata/libata-core.c |   96 +++++++++++++++++++++++++++++---------------
 include/linux/libata.h    |    7 +++
 2 files changed, 70 insertions(+), 33 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 4a3a5ae..2e191e7 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"
@@ -5234,51 +5235,62 @@ 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;
+	int rc;
 
-	for (i = 0; i < host->n_ports; i++) {
-		struct ata_port *ap = host->ports[i];
-		struct ata_link *link;
+	/* 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);
+	}
 
-		/* wait and check result */
-		if (wait) {
-			ata_port_wait_eh(ap);
-			WARN_ON(ap->pflags & ATA_PFLAG_PM_PENDING);
-			if (rc)
-				return rc;
-		}
+	return rc;
+}
+
+static int ata_host_request_pm(struct ata_host *host, pm_message_t mesg,
+			       unsigned int action, unsigned int ehi_flags,
+			       int wait)
+{
+	int i, rc;
+
+	for (i = 0; i < host->n_ports; i++) {
+		struct ata_port *ap = host->ports[i];
+
+		rc = ata_port_request_pm(ap, mesg, action, ehi_flags, wait);
+		if (rc)
+			return rc;
 	}
 
 	return 0;
@@ -5338,6 +5350,24 @@ void ata_host_resume(struct ata_host *host)
 			    ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET, 0);
 	host->dev->power.power_state = PMSG_ON;
 }
+
+#define to_ata_port(shost) (*(struct ata_port **)&shost->hostdata[0])
+
+int ata_scsi_host_suspend(struct Scsi_Host *shost)
+{
+	struct ata_port *ap = to_ata_port(shost);
+
+	return ata_port_request_pm(ap, PMSG_SUSPEND, 0, ATA_EHI_QUIET, 1);
+}
+
+int ata_scsi_host_resume(struct Scsi_Host *shost)
+{
+	struct ata_port *ap = to_ata_port(shost);
+
+	return ata_port_request_pm(ap, PMSG_ON, ATA_EH_RESET,
+		ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET, 1);
+}
+
 #endif
 
 /**
diff --git a/include/linux/libata.h b/include/linux/libata.h
index efd6f98..2fb9720 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1010,6 +1010,11 @@ extern bool ata_link_offline(struct ata_link *link);
 #ifdef CONFIG_PM
 extern int ata_host_suspend(struct ata_host *host, pm_message_t mesg);
 extern void ata_host_resume(struct ata_host *host);
+extern int ata_scsi_host_suspend(struct Scsi_Host *shost);
+extern int ata_scsi_host_resume(struct Scsi_Host *shost);
+#else
+static inline int ata_scsi_host_suspend(struct Scsi_Host *shost) { return 0; }
+static inline int ata_scsi_host_resume(struct Scsi_Host *shost) { return 0; };
 #endif
 extern int ata_ratelimit(void);
 extern void ata_msleep(struct ata_port *ap, unsigned int msecs);
@@ -1193,6 +1198,8 @@ extern struct device_attribute *ata_common_sdev_attrs[];
 	.name			= drv_name,			\
 	.ioctl			= ata_scsi_ioctl,		\
 	.queuecommand		= ata_scsi_queuecmd,		\
+	.suspend		= ata_scsi_host_suspend,	\
+	.resume			= ata_scsi_host_resume,		\
 	.can_queue		= ATA_DEF_QUEUE,		\
 	.this_id		= ATA_SHT_THIS_ID,		\
 	.cmd_per_lun		= ATA_SHT_CMD_PER_LUN,		\
-- 
1.7.2.5

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

[Tejun Heo]

Hello,

On Wed, Nov 02, 2011 at 02:21:37PM +0800, Lin Ming wrote:
> These 3 patches add ata port runtime pm support, which is done through scsi layer.
> scsi host runtime pm mechanism is already there(scsi_pm.c).
> These patches add hooks for ata port.
> 
> Lin Ming (3):
>       scsi: fix potential dead lock for host runtime pm
>       scsi: add hooks for host runtime power management
>       ata: implement ata port runtime pm hooks

Hmmm.... Some questions.

* How much does this add on top of hardware timed auto spindown and
  dynamic link power management?

* Do the added benefits justify yet another runtime powersaving
  mechanism?

* Any way we can tie the other stuff with overall runtime PM?

Thanks.

-- 
tejun

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

[Lin Ming]

On Wed, 2011-11-02 at 23:17 +0800, Tejun Heo wrote:
> Hello,
> 
> On Wed, Nov 02, 2011 at 02:21:37PM +0800, Lin Ming wrote:
> > These 3 patches add ata port runtime pm support, which is done through scsi layer.
> > scsi host runtime pm mechanism is already there(scsi_pm.c).
> > These patches add hooks for ata port.
> > 
> > Lin Ming (3):
> >       scsi: fix potential dead lock for host runtime pm
> >       scsi: add hooks for host runtime power management
> >       ata: implement ata port runtime pm hooks
> 
> Hmmm.... Some questions.
> 
> * How much does this add on top of hardware timed auto spindown and
>   dynamic link power management?

Sorry, I don't understand this question.
What do you mean?

> 
> * Do the added benefits justify yet another runtime powersaving
>   mechanism?

There will be ACPI firmware which supports ata port runtime D3Cold.
To enable ata port runtime PM is the first step.

> 
> * Any way we can tie the other stuff with overall runtime PM?

What's the other stuff?
Do you mean tie disk/link/port/controller stuff with overall runtime PM?

I'm not sure for now. Need to think about it more.

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

[Tejun Heo]

Hello,

On Thu, Nov 3, 2011 at 7:21 AM, Lin Ming <ming.m.lin@intel.com> wrote:
>> * How much does this add on top of hardware timed auto spindown and
>>   dynamic link power management?
>
> Sorry, I don't understand this question.
> What do you mean?

I was curious about the amount of additional power saving on top of
the existing PM features.

>> * Do the added benefits justify yet another runtime powersaving
>>   mechanism?
>
> There will be ACPI firmware which supports ata port runtime D3Cold.
> To enable ata port runtime PM is the first step.

libata can already turn off unused ports although it wouldn't go as
far as putting the whole controller into D3.

>> * Any way we can tie the other stuff with overall runtime PM?
>
> What's the other stuff?
> Do you mean tie disk/link/port/controller stuff with overall runtime PM?

Hardware initiated spin-down (hdparm -S) and link power saving (look
for ata_lpm_*). It would be great if there's an overall design how
they interact before adding yet another PM vector.

Thanks.

-- 
tejun

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

[Lin Ming]

On Thu, 2011-11-03 at 22:30 +0800, Tejun Heo wrote:
> Hello,
> 
> On Thu, Nov 3, 2011 at 7:21 AM, Lin Ming <ming.m.lin@intel.com> wrote:
> >> * How much does this add on top of hardware timed auto spindown and
> >>   dynamic link power management?
> >
> > Sorry, I don't understand this question.
> > What do you mean?
> 
> I was curious about the amount of additional power saving on top of
> the existing PM features.

I'll measure the power saving.

> 
> >> * Do the added benefits justify yet another runtime powersaving
> >>   mechanism?
> >
> > There will be ACPI firmware which supports ata port runtime D3Cold.
> > To enable ata port runtime PM is the first step.
> 
> libata can already turn off unused ports although it wouldn't go as
> far as putting the whole controller into D3.

At runtime?
Could you point me which piece of code implement this?

> 
> >> * Any way we can tie the other stuff with overall runtime PM?
> >
> > What's the other stuff?
> > Do you mean tie disk/link/port/controller stuff with overall runtime PM?
> 
> Hardware initiated spin-down (hdparm -S) and link power saving (look
> for ata_lpm_*). It would be great if there's an overall design how
> they interact before adding yet another PM vector.

Yes. Need to think about it.

Thanks.

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

[Tejun Heo]

Hello,

On Thu, Nov 3, 2011 at 7:47 AM, Lin Ming <ming.m.lin@intel.com> wrote:
>> > There will be ACPI firmware which supports ata port runtime D3Cold.
>> > To enable ata port runtime PM is the first step.
>>
>> libata can already turn off unused ports although it wouldn't go as
>> far as putting the whole controller into D3.
>
> At runtime?

Nope, I meant unoccupied (no device attached). IIRC it's part of the LPM code.

-- 
tejun