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

login
register
mail settings
Submitter Lin Ming
Date Nov. 2, 2011, 6:21 a.m.
Message ID <1320214900-2112-3-git-send-email-ming.m.lin@intel.com>
Download mbox | patch
Permalink /patch/123230/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Lin Ming - Nov. 2, 2011, 6:21 a.m.
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(-)
Alan Stern - Nov. 2, 2011, 2:53 p.m.
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

--
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
Lin Ming - Nov. 3, 2011, 1:08 p.m.
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

--
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
Alan Stern - Nov. 3, 2011, 2:22 p.m.
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

--
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
Lin Ming - Nov. 3, 2011, 2:37 p.m.
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
> 


--
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
Tejun Heo - Nov. 3, 2011, 2:41 p.m.
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 Heo - Nov. 3, 2011, 3:51 p.m.
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.
Alan Stern - Nov. 3, 2011, 4:08 p.m.
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

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

Patch

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;