Message ID | 50641BC5.2090000@intel.com |
---|---|
State | Not Applicable |
Delegated to: | David Miller |
Headers | show |
On Thu, 27 Sep 2012, Aaron Lu wrote: > > Moreover, I'd like to migrate SCSI drivers to the PM handling based on struct > > dev_pm_ops eventually and your change is kind of going in the opposite > > direction. I don't know how much effort the migration is going to take, > > though, so perhaps we can just make this change first. > > Does the following change look OK? > > diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c > index dc0ad85..1fb7ccc 100644 > --- a/drivers/scsi/scsi_pm.c > +++ b/drivers/scsi/scsi_pm.c > @@ -143,7 +143,15 @@ static int scsi_runtime_suspend(struct device *dev) > > dev_dbg(dev, "scsi_runtime_suspend\n"); > if (scsi_is_sdev_device(dev)) { > - err = scsi_dev_type_suspend(dev, PMSG_AUTO_SUSPEND); > + err = scsi_device_quiesce(to_scsi_device(dev)); > + if (err) > + goto out; > + > + err = pm_generic_runtime_suspend(dev); > + if (!err) > + goto out; > + > + scsi_device_resume(to_scsi_device(dev)); > if (err == -EAGAIN) > pm_schedule_suspend(dev, jiffies_to_msecs( > round_jiffies_up_relative(HZ/10))); Maybe in the end it will be better, but for now this looks like unnecessary code duplication. Basically you are copying scsi_dev_type_suspend() into scsi_runtime_suspend(), except that you omitted the debugging statement. 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 09/27/2012 10:42 PM, Alan Stern wrote: > On Thu, 27 Sep 2012, Aaron Lu wrote: > >>> Moreover, I'd like to migrate SCSI drivers to the PM handling based on struct >>> dev_pm_ops eventually and your change is kind of going in the opposite >>> direction. I don't know how much effort the migration is going to take, >>> though, so perhaps we can just make this change first. >> >> Does the following change look OK? >> >> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c >> index dc0ad85..1fb7ccc 100644 >> --- a/drivers/scsi/scsi_pm.c >> +++ b/drivers/scsi/scsi_pm.c >> @@ -143,7 +143,15 @@ static int scsi_runtime_suspend(struct device *dev) >> >> dev_dbg(dev, "scsi_runtime_suspend\n"); >> if (scsi_is_sdev_device(dev)) { >> - err = scsi_dev_type_suspend(dev, PMSG_AUTO_SUSPEND); >> + err = scsi_device_quiesce(to_scsi_device(dev)); >> + if (err) >> + goto out; >> + >> + err = pm_generic_runtime_suspend(dev); >> + if (!err) >> + goto out; >> + >> + scsi_device_resume(to_scsi_device(dev)); >> if (err == -EAGAIN) >> pm_schedule_suspend(dev, jiffies_to_msecs( >> round_jiffies_up_relative(HZ/10))); > > Maybe in the end it will be better, but for now this looks like > unnecessary code duplication. Basically you are copying > scsi_dev_type_suspend() into scsi_runtime_suspend(), except that you > omitted the debugging statement. And I've used pm_generic_runtime_suspend :-) That would allow me to write runtime callbacks of dev_pm_ops for indivisual scsi drivers, like sr. Thanks, Aaron -- 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 Thursday, September 27, 2012, Aaron Lu wrote: > On 09/27/2012 10:42 PM, Alan Stern wrote: > > On Thu, 27 Sep 2012, Aaron Lu wrote: > > > >>> Moreover, I'd like to migrate SCSI drivers to the PM handling based on struct > >>> dev_pm_ops eventually and your change is kind of going in the opposite > >>> direction. I don't know how much effort the migration is going to take, > >>> though, so perhaps we can just make this change first. > >> > >> Does the following change look OK? > >> > >> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c > >> index dc0ad85..1fb7ccc 100644 > >> --- a/drivers/scsi/scsi_pm.c > >> +++ b/drivers/scsi/scsi_pm.c > >> @@ -143,7 +143,15 @@ static int scsi_runtime_suspend(struct device *dev) > >> > >> dev_dbg(dev, "scsi_runtime_suspend\n"); > >> if (scsi_is_sdev_device(dev)) { > >> - err = scsi_dev_type_suspend(dev, PMSG_AUTO_SUSPEND); > >> + err = scsi_device_quiesce(to_scsi_device(dev)); > >> + if (err) > >> + goto out; > >> + > >> + err = pm_generic_runtime_suspend(dev); > >> + if (!err) > >> + goto out; > >> + > >> + scsi_device_resume(to_scsi_device(dev)); > >> if (err == -EAGAIN) > >> pm_schedule_suspend(dev, jiffies_to_msecs( > >> round_jiffies_up_relative(HZ/10))); > > > > Maybe in the end it will be better, but for now this looks like > > unnecessary code duplication. Basically you are copying > > scsi_dev_type_suspend() into scsi_runtime_suspend(), except that you > > omitted the debugging statement. > > And I've used pm_generic_runtime_suspend :-) > That would allow me to write runtime callbacks of dev_pm_ops for > indivisual scsi drivers, like sr. Well, actually, I meant something different. The above patch would make it possible for drivers to provide runtime PM callbacks through dev->driver.pm, but drv->suspend and drv->resume would still be used for system suspend/resume (and hibernation). The transition to struct dev_pm_ops I meant, however, would be to start using callbacks from dev->driver.pm for _all_ device PM operations by default and fall back to drv->suspend and drv->resume for the drivers whose dev->driver.pm is NULL (along the lines of what PCI does). The next step would be to convert all drivers to use dev->driver.pm only. So I wouldn't like any drivers to use dev->driver.pm callbacks for some operations and drv->suspend and drv->resume for the remaining ones at the same time. Thanks, Rafael -- 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 dc0ad85..1fb7ccc 100644 --- a/drivers/scsi/scsi_pm.c +++ b/drivers/scsi/scsi_pm.c @@ -143,7 +143,15 @@ static int scsi_runtime_suspend(struct device *dev) dev_dbg(dev, "scsi_runtime_suspend\n"); if (scsi_is_sdev_device(dev)) { - err = scsi_dev_type_suspend(dev, PMSG_AUTO_SUSPEND); + err = scsi_device_quiesce(to_scsi_device(dev)); + if (err) + goto out; + + err = pm_generic_runtime_suspend(dev); + if (!err) + goto out; + + scsi_device_resume(to_scsi_device(dev)); if (err == -EAGAIN) pm_schedule_suspend(dev, jiffies_to_msecs( round_jiffies_up_relative(HZ/10))); @@ -151,6 +159,7 @@ static int scsi_runtime_suspend(struct device *dev) /* Insert hooks here for targets, hosts, and transport classes */ +out: return err; } @@ -159,11 +168,17 @@ static int scsi_runtime_resume(struct device *dev) int err = 0; dev_dbg(dev, "scsi_runtime_resume\n"); - if (scsi_is_sdev_device(dev)) + if (scsi_is_sdev_device(dev)) { + err = pm_generic_runtime_resume(dev); + if (err) + goto out; + err = scsi_dev_type_resume(dev); + } /* Insert hooks here for targets, hosts, and transport classes */ +out: return err; }