| Message ID | 1320214900-2112-3-git-send-email-ming.m.lin@intel.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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
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.
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.
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
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;
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(-)