Message ID | 201106231946.30004.rjw@sisk.pl (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
On Thu, 23 Jun 2011, Rafael J. Wysocki wrote: > OK, so what about the appended patch (the last hunk is necessary to make the > SCSI error handling work when runtime PM is disabled, it should be a separate > patch)? > > Rafael > > --- > drivers/base/power/main.c | 27 ++++++++++++++++++++++----- > drivers/base/power/runtime.c | 10 ++++++---- > 2 files changed, 28 insertions(+), 9 deletions(-) > > Index: linux-2.6/drivers/base/power/main.c > =================================================================== > --- linux-2.6.orig/drivers/base/power/main.c > +++ linux-2.6/drivers/base/power/main.c > @@ -505,6 +505,7 @@ static int legacy_resume(struct device * > static int device_resume(struct device *dev, pm_message_t state, bool async) > { > int error = 0; > + bool put = false; > > TRACE_DEVICE(dev); > TRACE_RESUME(0); > @@ -521,6 +522,9 @@ static int device_resume(struct device * > if (!dev->power.is_suspended) > goto Unlock; > > + pm_runtime_enable(dev); > + put = true; > + > if (dev->pwr_domain) { > pm_dev_dbg(dev, state, "power domain "); > error = pm_op(dev, &dev->pwr_domain->ops, state); > @@ -563,6 +567,10 @@ static int device_resume(struct device * > complete_all(&dev->power.completion); > > TRACE_RESUME(error); > + > + if (put) > + pm_runtime_put_sync(dev); > + > return error; > } > > @@ -843,16 +851,22 @@ static int __device_suspend(struct devic > int error = 0; > > dpm_wait_for_children(dev, async); > - device_lock(dev); > > if (async_error) > - goto Unlock; > + return 0; > + > + pm_runtime_get_noresume(dev); > + if (pm_runtime_barrier(dev) && device_may_wakeup(dev)) > + pm_wakeup_event(dev, 0); > > if (pm_wakeup_pending()) { > + pm_runtime_put_sync(dev); > async_error = -EBUSY; > - goto Unlock; > + return 0; > } > > + device_lock(dev); > + > if (dev->pwr_domain) { > pm_dev_dbg(dev, state, "power domain "); > error = pm_op(dev, &dev->pwr_domain->ops, state); > @@ -890,12 +904,15 @@ static int __device_suspend(struct devic > End: > dev->power.is_suspended = !error; > > - Unlock: > device_unlock(dev); > complete_all(&dev->power.completion); > > - if (error) > + if (error) { > + pm_runtime_put_sync(dev); > async_error = error; > + } else if (dev->power.is_suspended) { > + __pm_runtime_disable(dev, false); > + } > > return error; > } Looks right. > Index: linux-2.6/drivers/base/power/runtime.c > =================================================================== > --- linux-2.6.orig/drivers/base/power/runtime.c > +++ linux-2.6/drivers/base/power/runtime.c > @@ -455,12 +455,14 @@ static int rpm_resume(struct device *dev > dev_dbg(dev, "%s flags 0x%x\n", __func__, rpmflags); > > repeat: > - if (dev->power.runtime_error) > + if (dev->power.runtime_error) { > retval = -EINVAL; > - else if (dev->power.disable_depth > 0) > - retval = -EAGAIN; > - if (retval) > goto out; > + } else if (dev->power.disable_depth > 0) { > + if (!(rpmflags & RPM_GET_PUT)) > + retval = -EAGAIN; Do you also want to check the current status? If it isn't RPM_ACTIVE then perhaps you should return an error. > + goto out; > + } > > /* > * Other scheduled or pending requests need to be canceled. Small Alan Stern
On Thursday, June 23, 2011, Alan Stern wrote: > On Thu, 23 Jun 2011, Rafael J. Wysocki wrote: > > > OK, so what about the appended patch (the last hunk is necessary to make the > > SCSI error handling work when runtime PM is disabled, it should be a separate > > patch)? > > > > Rafael > > > > --- > > drivers/base/power/main.c | 27 ++++++++++++++++++++++----- > > drivers/base/power/runtime.c | 10 ++++++---- > > 2 files changed, 28 insertions(+), 9 deletions(-) > > > > Index: linux-2.6/drivers/base/power/main.c > > =================================================================== > > --- linux-2.6.orig/drivers/base/power/main.c > > +++ linux-2.6/drivers/base/power/main.c > > @@ -505,6 +505,7 @@ static int legacy_resume(struct device * > > static int device_resume(struct device *dev, pm_message_t state, bool async) > > { > > int error = 0; > > + bool put = false; > > > > TRACE_DEVICE(dev); > > TRACE_RESUME(0); > > @@ -521,6 +522,9 @@ static int device_resume(struct device * > > if (!dev->power.is_suspended) > > goto Unlock; > > > > + pm_runtime_enable(dev); > > + put = true; > > + > > if (dev->pwr_domain) { > > pm_dev_dbg(dev, state, "power domain "); > > error = pm_op(dev, &dev->pwr_domain->ops, state); > > @@ -563,6 +567,10 @@ static int device_resume(struct device * > > complete_all(&dev->power.completion); > > > > TRACE_RESUME(error); > > + > > + if (put) > > + pm_runtime_put_sync(dev); > > + > > return error; > > } > > > > @@ -843,16 +851,22 @@ static int __device_suspend(struct devic > > int error = 0; > > > > dpm_wait_for_children(dev, async); > > - device_lock(dev); > > > > if (async_error) > > - goto Unlock; > > + return 0; > > + > > + pm_runtime_get_noresume(dev); > > + if (pm_runtime_barrier(dev) && device_may_wakeup(dev)) > > + pm_wakeup_event(dev, 0); > > > > if (pm_wakeup_pending()) { > > + pm_runtime_put_sync(dev); > > async_error = -EBUSY; > > - goto Unlock; > > + return 0; > > } > > > > + device_lock(dev); > > + > > if (dev->pwr_domain) { > > pm_dev_dbg(dev, state, "power domain "); > > error = pm_op(dev, &dev->pwr_domain->ops, state); > > @@ -890,12 +904,15 @@ static int __device_suspend(struct devic > > End: > > dev->power.is_suspended = !error; > > > > - Unlock: > > device_unlock(dev); > > complete_all(&dev->power.completion); > > > > - if (error) > > + if (error) { > > + pm_runtime_put_sync(dev); > > async_error = error; > > + } else if (dev->power.is_suspended) { > > + __pm_runtime_disable(dev, false); > > + } > > > > return error; > > } > > Looks right. Great! I'll prepare a final version with a changelog and documentation update, then. > > Index: linux-2.6/drivers/base/power/runtime.c > > =================================================================== > > --- linux-2.6.orig/drivers/base/power/runtime.c > > +++ linux-2.6/drivers/base/power/runtime.c > > @@ -455,12 +455,14 @@ static int rpm_resume(struct device *dev > > dev_dbg(dev, "%s flags 0x%x\n", __func__, rpmflags); > > > > repeat: > > - if (dev->power.runtime_error) > > + if (dev->power.runtime_error) { > > retval = -EINVAL; > > - else if (dev->power.disable_depth > 0) > > - retval = -EAGAIN; > > - if (retval) > > goto out; > > + } else if (dev->power.disable_depth > 0) { > > + if (!(rpmflags & RPM_GET_PUT)) > > + retval = -EAGAIN; > > Do you also want to check the current status? If it isn't RPM_ACTIVE > then perhaps you should return an error. That depends on whether or not we want RPM_ACTIVE to have any meaning for devices whose power.disable_depth is nonzero. My opinion is that if power.disable_depth is nonzero, the runtime PM status of the device shouldn't matter (it may be changed on the fly in ways that need not reflect the real status). Thanks, Rafael
Index: linux-2.6/drivers/base/power/main.c =================================================================== --- linux-2.6.orig/drivers/base/power/main.c +++ linux-2.6/drivers/base/power/main.c @@ -505,6 +505,7 @@ static int legacy_resume(struct device * static int device_resume(struct device *dev, pm_message_t state, bool async) { int error = 0; + bool put = false; TRACE_DEVICE(dev); TRACE_RESUME(0); @@ -521,6 +522,9 @@ static int device_resume(struct device * if (!dev->power.is_suspended) goto Unlock; + pm_runtime_enable(dev); + put = true; + if (dev->pwr_domain) { pm_dev_dbg(dev, state, "power domain "); error = pm_op(dev, &dev->pwr_domain->ops, state); @@ -563,6 +567,10 @@ static int device_resume(struct device * complete_all(&dev->power.completion); TRACE_RESUME(error); + + if (put) + pm_runtime_put_sync(dev); + return error; } @@ -843,16 +851,22 @@ static int __device_suspend(struct devic int error = 0; dpm_wait_for_children(dev, async); - device_lock(dev); if (async_error) - goto Unlock; + return 0; + + pm_runtime_get_noresume(dev); + if (pm_runtime_barrier(dev) && device_may_wakeup(dev)) + pm_wakeup_event(dev, 0); if (pm_wakeup_pending()) { + pm_runtime_put_sync(dev); async_error = -EBUSY; - goto Unlock; + return 0; } + device_lock(dev); + if (dev->pwr_domain) { pm_dev_dbg(dev, state, "power domain "); error = pm_op(dev, &dev->pwr_domain->ops, state); @@ -890,12 +904,15 @@ static int __device_suspend(struct devic End: dev->power.is_suspended = !error; - Unlock: device_unlock(dev); complete_all(&dev->power.completion); - if (error) + if (error) { + pm_runtime_put_sync(dev); async_error = error; + } else if (dev->power.is_suspended) { + __pm_runtime_disable(dev, false); + } return error; } Index: linux-2.6/drivers/base/power/runtime.c =================================================================== --- linux-2.6.orig/drivers/base/power/runtime.c +++ linux-2.6/drivers/base/power/runtime.c @@ -455,12 +455,14 @@ static int rpm_resume(struct device *dev dev_dbg(dev, "%s flags 0x%x\n", __func__, rpmflags); repeat: - if (dev->power.runtime_error) + if (dev->power.runtime_error) { retval = -EINVAL; - else if (dev->power.disable_depth > 0) - retval = -EAGAIN; - if (retval) goto out; + } else if (dev->power.disable_depth > 0) { + if (!(rpmflags & RPM_GET_PUT)) + retval = -EAGAIN; + goto out; + } /* * Other scheduled or pending requests need to be canceled. Small