| Message ID | 1351843430-8023-1-git-send-email-tianyu.lan@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Friday, November 02, 2012 04:03:50 PM Lan Tianyu wrote: > Since dev_pm_qos_add_request(), dev_pm_qos_update_request() and > dev_pm_qos_remove_request() for pm qos flags should not be invoked > when device in RPM_SUSPENDED. Add pm_runtime_get_sync() and pm_runtime_put() > around these functions in the pm core to ensure device not in RPM_SUSPENDED. > > Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> > --- > drivers/base/power/qos.c | 10 ++++++++-- > drivers/base/power/sysfs.c | 4 ++++ > 2 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c > index 31d3f48..b50ba72 100644 > --- a/drivers/base/power/qos.c > +++ b/drivers/base/power/qos.c > @@ -624,15 +624,19 @@ int dev_pm_qos_expose_flags(struct device *dev, s32 val) > if (!req) > return -ENOMEM; > > + pm_runtime_get_sync(dev); > ret = dev_pm_qos_add_request(dev, req, DEV_PM_QOS_FLAGS, val); > + pm_runtime_put(dev); I would drop this one ... > if (ret < 0) > return ret; > > dev->power.qos->flags_req = req; > ret = pm_qos_sysfs_add_flags(dev); > - if (ret) > + if (ret) { > + pm_runtime_get_sync(dev); > __dev_pm_qos_drop_user_request(dev, DEV_PM_QOS_FLAGS); > - > + pm_runtime_put(dev); ... and move this one before the return statement (plus a label for goto from the ret < 0 check after adding the request). > + } > return ret; > } > EXPORT_SYMBOL_GPL(dev_pm_qos_expose_flags); > @@ -645,7 +649,9 @@ void dev_pm_qos_hide_flags(struct device *dev) > { > if (dev->power.qos && dev->power.qos->flags_req) { > pm_qos_sysfs_remove_flags(dev); > + pm_runtime_get_sync(dev); > __dev_pm_qos_drop_user_request(dev, DEV_PM_QOS_FLAGS); > + pm_runtime_put(dev); I'm not sure if these two are necessary. If we remove a request, then what happens worst case is that some flags will be cleared effectively which means fewer restrictions on the next sleep state. Then, it shouldn't hurt that the current sleep state is more restricted. Hmm. Perhaps a comment would suffice? > } > } > EXPORT_SYMBOL_GPL(dev_pm_qos_hide_flags); > diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c > index 50d16e3..240bfaa 100644 > --- a/drivers/base/power/sysfs.c > +++ b/drivers/base/power/sysfs.c > @@ -264,7 +264,9 @@ static ssize_t pm_qos_no_power_off_store(struct device *dev, > if (ret != 0 && ret != 1) > return -EINVAL; > > + pm_runtime_get_sync(dev); > ret = dev_pm_qos_update_flags(dev, PM_QOS_FLAG_NO_POWER_OFF, ret); > + pm_runtime_put(dev); > return ret < 0 ? ret : n; > } Well, you haven't noticed that dev_pm_qos_update_flags() already does pm_runtime_get_sync()/pm_runtime_put(). :-) > @@ -291,7 +293,9 @@ static ssize_t pm_qos_remote_wakeup_store(struct device *dev, > if (ret != 0 && ret != 1) > return -EINVAL; > > + pm_runtime_get_sync(dev); > ret = dev_pm_qos_update_flags(dev, PM_QOS_FLAG_REMOTE_WAKEUP, ret); > + pm_runtime_put(dev); > return ret < 0 ? ret : n; > } Thanks, Rafael
On 2012/11/2 19:17, Rafael J. Wysocki wrote: > On Friday, November 02, 2012 04:03:50 PM Lan Tianyu wrote: >> Since dev_pm_qos_add_request(), dev_pm_qos_update_request() and >> dev_pm_qos_remove_request() for pm qos flags should not be invoked >> when device in RPM_SUSPENDED. Add pm_runtime_get_sync() and pm_runtime_put() >> around these functions in the pm core to ensure device not in RPM_SUSPENDED. >> >> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> >> --- >> drivers/base/power/qos.c | 10 ++++++++-- >> drivers/base/power/sysfs.c | 4 ++++ >> 2 files changed, 12 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c >> index 31d3f48..b50ba72 100644 >> --- a/drivers/base/power/qos.c >> +++ b/drivers/base/power/qos.c >> @@ -624,15 +624,19 @@ int dev_pm_qos_expose_flags(struct device *dev, s32 val) >> if (!req) >> return -ENOMEM; >> >> + pm_runtime_get_sync(dev); >> ret = dev_pm_qos_add_request(dev, req, DEV_PM_QOS_FLAGS, val); >> + pm_runtime_put(dev); > > I would drop this one ... > >> if (ret < 0) >> return ret; >> >> dev->power.qos->flags_req = req; >> ret = pm_qos_sysfs_add_flags(dev); >> - if (ret) >> + if (ret) { >> + pm_runtime_get_sync(dev); >> __dev_pm_qos_drop_user_request(dev, DEV_PM_QOS_FLAGS); >> - >> + pm_runtime_put(dev); > > ... and move this one before the return statement (plus a label for > goto from the ret < 0 check after adding the request). > What you mean likes following? + pm_runtime_get_sync(dev); ret = dev_pm_qos_add_request(dev, req, DEV_PM_QOS_FLAGS, val); if (ret < 0) - return ret; + goto fail; dev->power.qos->flags_req = req; ret = pm_qos_sysfs_add_flags(dev); if (ret) __dev_pm_qos_drop_user_request(dev, DEV_PM_QOS_FLAGS); +fail: + pm_runtime_put(dev); return ret; } >> + } >> return ret; >> } >> EXPORT_SYMBOL_GPL(dev_pm_qos_expose_flags); >> @@ -645,7 +649,9 @@ void dev_pm_qos_hide_flags(struct device *dev) >> { >> if (dev->power.qos && dev->power.qos->flags_req) { >> pm_qos_sysfs_remove_flags(dev); >> + pm_runtime_get_sync(dev); >> __dev_pm_qos_drop_user_request(dev, DEV_PM_QOS_FLAGS); >> + pm_runtime_put(dev); > > I'm not sure if these two are necessary. If we remove a request, > then what happens worst case is that some flags will be cleared > effectively which means fewer restrictions on the next sleep state. > Then, it shouldn't hurt that the current sleep state is more > restricted. But this mean the device can be put into lower power state(power off). So why not do that? that can save more power, right? > > Hmm. Perhaps a comment would suffice? > >> } >> } >> EXPORT_SYMBOL_GPL(dev_pm_qos_hide_flags); >> diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c >> index 50d16e3..240bfaa 100644 >> --- a/drivers/base/power/sysfs.c >> +++ b/drivers/base/power/sysfs.c >> @@ -264,7 +264,9 @@ static ssize_t pm_qos_no_power_off_store(struct device *dev, >> if (ret != 0 && ret != 1) >> return -EINVAL; >> >> + pm_runtime_get_sync(dev); >> ret = dev_pm_qos_update_flags(dev, PM_QOS_FLAG_NO_POWER_OFF, ret); >> + pm_runtime_put(dev); >> return ret < 0 ? ret : n; >> } > > Well, you haven't noticed that dev_pm_qos_update_flags() already does > pm_runtime_get_sync()/pm_runtime_put(). :-) Oh. Yeah. I ignore it. :) > >> @@ -291,7 +293,9 @@ static ssize_t pm_qos_remote_wakeup_store(struct device *dev, >> if (ret != 0 && ret != 1) >> return -EINVAL; >> >> + pm_runtime_get_sync(dev); >> ret = dev_pm_qos_update_flags(dev, PM_QOS_FLAG_REMOTE_WAKEUP, ret); >> + pm_runtime_put(dev); >> return ret < 0 ? ret : n; >> } > > Thanks, > Rafael > >
On Saturday, November 03, 2012 12:16:51 AM Lan Tianyu wrote: > On 2012/11/2 19:17, Rafael J. Wysocki wrote: > > On Friday, November 02, 2012 04:03:50 PM Lan Tianyu wrote: > >> Since dev_pm_qos_add_request(), dev_pm_qos_update_request() and > >> dev_pm_qos_remove_request() for pm qos flags should not be invoked > >> when device in RPM_SUSPENDED. Add pm_runtime_get_sync() and pm_runtime_put() > >> around these functions in the pm core to ensure device not in RPM_SUSPENDED. > >> > >> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> > >> --- > >> drivers/base/power/qos.c | 10 ++++++++-- > >> drivers/base/power/sysfs.c | 4 ++++ > >> 2 files changed, 12 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c > >> index 31d3f48..b50ba72 100644 > >> --- a/drivers/base/power/qos.c > >> +++ b/drivers/base/power/qos.c > >> @@ -624,15 +624,19 @@ int dev_pm_qos_expose_flags(struct device *dev, s32 val) > >> if (!req) > >> return -ENOMEM; > >> > >> + pm_runtime_get_sync(dev); > >> ret = dev_pm_qos_add_request(dev, req, DEV_PM_QOS_FLAGS, val); > >> + pm_runtime_put(dev); > > > > I would drop this one ... > > > >> if (ret < 0) > >> return ret; > >> > >> dev->power.qos->flags_req = req; > >> ret = pm_qos_sysfs_add_flags(dev); > >> - if (ret) > >> + if (ret) { > >> + pm_runtime_get_sync(dev); > >> __dev_pm_qos_drop_user_request(dev, DEV_PM_QOS_FLAGS); > >> - > >> + pm_runtime_put(dev); > > > > ... and move this one before the return statement (plus a label for > > goto from the ret < 0 check after adding the request). > > > What you mean likes following? > > + pm_runtime_get_sync(dev); > ret = dev_pm_qos_add_request(dev, req, DEV_PM_QOS_FLAGS, val); > if (ret < 0) > - return ret; > + goto fail; > > dev->power.qos->flags_req = req; > ret = pm_qos_sysfs_add_flags(dev); > if (ret) > __dev_pm_qos_drop_user_request(dev, DEV_PM_QOS_FLAGS); > > +fail: > + pm_runtime_put(dev); > return ret; > } > Yes, it does. > >> + } > >> return ret; > >> } > >> EXPORT_SYMBOL_GPL(dev_pm_qos_expose_flags); > >> @@ -645,7 +649,9 @@ void dev_pm_qos_hide_flags(struct device *dev) > >> { > >> if (dev->power.qos && dev->power.qos->flags_req) { > >> pm_qos_sysfs_remove_flags(dev); > >> + pm_runtime_get_sync(dev); > >> __dev_pm_qos_drop_user_request(dev, DEV_PM_QOS_FLAGS); > >> + pm_runtime_put(dev); > > > > I'm not sure if these two are necessary. If we remove a request, > > then what happens worst case is that some flags will be cleared > > effectively which means fewer restrictions on the next sleep state. > > Then, it shouldn't hurt that the current sleep state is more > > restricted. > > But this mean the device can be put into lower power state(power off). > So why not do that? that can save more power, right? Correct. On the other hand, though, if the device already is in the deepest low-power state available, we will resume it unnecessarily this way. Which may not be a big deal, however, and since we do that in other cases, we may as well do it here. Thanks, Rafael
On 2012/11/3 4:11, Rafael J. Wysocki wrote: >>>> } >>>> > >> EXPORT_SYMBOL_GPL(dev_pm_qos_expose_flags); >>>> > >>@@ -645,7 +649,9 @@ void dev_pm_qos_hide_flags(struct device *dev) >>>> > >> { >>>> > >> if (dev->power.qos && dev->power.qos->flags_req) { >>>> > >> pm_qos_sysfs_remove_flags(dev); >>>> > >>+ pm_runtime_get_sync(dev); >>>> > >> __dev_pm_qos_drop_user_request(dev, DEV_PM_QOS_FLAGS); >>>> > >>+ pm_runtime_put(dev); >>> > > >>> > >I'm not sure if these two are necessary. If we remove a request, >>> > >then what happens worst case is that some flags will be cleared >>> > >effectively which means fewer restrictions on the next sleep state. >>> > >Then, it shouldn't hurt that the current sleep state is more >>> > >restricted. >> > >> >But this mean the device can be put into lower power state(power off). >> >So why not do that? that can save more power, right? > Correct. On the other hand, though, if the device already is in the > deepest low-power state available, we will resume it unnecessarily this > way. Which may not be a big deal, however, and since we do that in other > cases, we may as well do it here. Yeah. This seems not very reasonable. But we can optimize this later.From my previous opinion, add notifier for flags and let device driver or bus driver to determine when the device should be resumed. Since you said at another email you would remove all notifiers in the pm qos to make some functions able to be invoked in interrupt context. I have a thought that check the context before call notifiers chain. If it was in interrupt, not call notifier chain and trigger a work queue or other choices to do that. If not, call the chain. Does this make sense? :) > > Thanks, > Rafael
On 2012/11/4 23:09, Lan Tianyu wrote: > On 2012/11/3 4:11, Rafael J. Wysocki wrote: >>>>> } >>>>> > >> EXPORT_SYMBOL_GPL(dev_pm_qos_expose_flags); >>>>> > >>@@ -645,7 +649,9 @@ void dev_pm_qos_hide_flags(struct device *dev) >>>>> > >> { >>>>> > >> if (dev->power.qos && dev->power.qos->flags_req) { >>>>> > >> pm_qos_sysfs_remove_flags(dev); >>>>> > >>+ pm_runtime_get_sync(dev); >>>>> > >> __dev_pm_qos_drop_user_request(dev, DEV_PM_QOS_FLAGS); >>>>> > >>+ pm_runtime_put(dev); >>>> > > >>>> > >I'm not sure if these two are necessary. If we remove a request, >>>> > >then what happens worst case is that some flags will be cleared >>>> > >effectively which means fewer restrictions on the next sleep state. >>>> > >Then, it shouldn't hurt that the current sleep state is more >>>> > >restricted. >>> > >>> >But this mean the device can be put into lower power state(power off). >>> >So why not do that? that can save more power, right? >> Correct. On the other hand, though, if the device already is in the >> deepest low-power state available, we will resume it unnecessarily this >> way. Which may not be a big deal, however, and since we do that in other >> cases, we may as well do it here. > Yeah. This seems not very reasonable. But we can optimize this > later.From my previous opinion, add notifier for flags and let device > driver or bus driver to determine when the device should be resumed. > Since you said at another email you would remove all notifiers in the pm > qos to make some functions able to be invoked in interrupt context. I > have a thought that check the context before call notifiers chain. If it > was in interrupt, not call notifier chain and trigger a work queue or > other choices to do that. If not, call the chain. Does this make sense? :) > Hi Rafael: Do you have some opinions? >> >> Thanks, >> Rafael > >
On Sunday, November 11, 2012 08:08:48 PM Lan Tianyu wrote: > On 2012/11/4 23:09, Lan Tianyu wrote: > > On 2012/11/3 4:11, Rafael J. Wysocki wrote: > >>>>> } > >>>>> > >> EXPORT_SYMBOL_GPL(dev_pm_qos_expose_flags); > >>>>> > >>@@ -645,7 +649,9 @@ void dev_pm_qos_hide_flags(struct device *dev) > >>>>> > >> { > >>>>> > >> if (dev->power.qos && dev->power.qos->flags_req) { > >>>>> > >> pm_qos_sysfs_remove_flags(dev); > >>>>> > >>+ pm_runtime_get_sync(dev); > >>>>> > >> __dev_pm_qos_drop_user_request(dev, DEV_PM_QOS_FLAGS); > >>>>> > >>+ pm_runtime_put(dev); > >>>> > > > >>>> > >I'm not sure if these two are necessary. If we remove a request, > >>>> > >then what happens worst case is that some flags will be cleared > >>>> > >effectively which means fewer restrictions on the next sleep state. > >>>> > >Then, it shouldn't hurt that the current sleep state is more > >>>> > >restricted. > >>> > > >>> >But this mean the device can be put into lower power state(power off). > >>> >So why not do that? that can save more power, right? > >> Correct. On the other hand, though, if the device already is in the > >> deepest low-power state available, we will resume it unnecessarily this > >> way. Which may not be a big deal, however, and since we do that in other > >> cases, we may as well do it here. > > Yeah. This seems not very reasonable. But we can optimize this > > later.From my previous opinion, add notifier for flags and let device > > driver or bus driver to determine when the device should be resumed. > > Since you said at another email you would remove all notifiers in the pm > > qos to make some functions able to be invoked in interrupt context. I > > have a thought that check the context before call notifiers chain. If it > > was in interrupt, not call notifier chain and trigger a work queue or > > other choices to do that. If not, call the chain. Does this make sense? :) > > > Hi Rafael: > Do you have some opinions? First off, I've applied the last version of this patch. :-) Second, I don't think we need notifiers at all in the case of device PM QoS and, moreover, we'd actually be better off without them. There generally are two reasons for the notifiers in that case. One reason is to prevent devices from sleeping too long if they were suspended before a new PM QoS request has been added (or an existing one has been updated). The second reason is to allow things like PM domains to recompute their numbers taking the new request into account. Now, if whoever modifies/adds PM QoS requests for certain device ensures that the device is not RPM_SUSPENDED while the PM QoS constraints are changing, that makes the first reason go away. The second reason may be taken care of by changing the PM core to set a (new) flag whenever PM QoS constraints for a device change, so that whoever cares can take that into account while the device is suspended next time. This way all of the additional computations will only need to happen when devices are suspended and the code flow will be much easier to follow. Thanks, Rafael
diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c index 31d3f48..b50ba72 100644 --- a/drivers/base/power/qos.c +++ b/drivers/base/power/qos.c @@ -624,15 +624,19 @@ int dev_pm_qos_expose_flags(struct device *dev, s32 val) if (!req) return -ENOMEM; + pm_runtime_get_sync(dev); ret = dev_pm_qos_add_request(dev, req, DEV_PM_QOS_FLAGS, val); + pm_runtime_put(dev); if (ret < 0) return ret; dev->power.qos->flags_req = req; ret = pm_qos_sysfs_add_flags(dev); - if (ret) + if (ret) { + pm_runtime_get_sync(dev); __dev_pm_qos_drop_user_request(dev, DEV_PM_QOS_FLAGS); - + pm_runtime_put(dev); + } return ret; } EXPORT_SYMBOL_GPL(dev_pm_qos_expose_flags); @@ -645,7 +649,9 @@ void dev_pm_qos_hide_flags(struct device *dev) { if (dev->power.qos && dev->power.qos->flags_req) { pm_qos_sysfs_remove_flags(dev); + pm_runtime_get_sync(dev); __dev_pm_qos_drop_user_request(dev, DEV_PM_QOS_FLAGS); + pm_runtime_put(dev); } } EXPORT_SYMBOL_GPL(dev_pm_qos_hide_flags); diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c index 50d16e3..240bfaa 100644 --- a/drivers/base/power/sysfs.c +++ b/drivers/base/power/sysfs.c @@ -264,7 +264,9 @@ static ssize_t pm_qos_no_power_off_store(struct device *dev, if (ret != 0 && ret != 1) return -EINVAL; + pm_runtime_get_sync(dev); ret = dev_pm_qos_update_flags(dev, PM_QOS_FLAG_NO_POWER_OFF, ret); + pm_runtime_put(dev); return ret < 0 ? ret : n; } @@ -291,7 +293,9 @@ static ssize_t pm_qos_remote_wakeup_store(struct device *dev, if (ret != 0 && ret != 1) return -EINVAL; + pm_runtime_get_sync(dev); ret = dev_pm_qos_update_flags(dev, PM_QOS_FLAG_REMOTE_WAKEUP, ret); + pm_runtime_put(dev); return ret < 0 ? ret : n; }
Since dev_pm_qos_add_request(), dev_pm_qos_update_request() and dev_pm_qos_remove_request() for pm qos flags should not be invoked when device in RPM_SUSPENDED. Add pm_runtime_get_sync() and pm_runtime_put() around these functions in the pm core to ensure device not in RPM_SUSPENDED. Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> --- drivers/base/power/qos.c | 10 ++++++++-- drivers/base/power/sysfs.c | 4 ++++ 2 files changed, 12 insertions(+), 2 deletions(-)