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@xxxxxxxxx>
> >> ---
> >> 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
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
