[PATCH] PM / Qos: Ensure device not in PRM_SUSPENDED when pm qos flags request functions are invoked in the pm core.

[PATCH] PM / Qos: Ensure device not in PRM_SUSPENDED when pm qos flags request functions are invoked in the pm core.

[Lan Tianyu]

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);
 	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;
 }
 
-- 
1.7.9.5

Re: [PATCH] PM / Qos: Ensure device not in PRM_SUSPENDED when pm qos flags request functions are invoked in the pm core.

[Rafael J. Wysocki]

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


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] PM / Qos: Ensure device not in PRM_SUSPENDED when pm qos flags request functions are invoked in the pm core.

[Lan Tianyu]

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
>
>


-- 
Best Regards
Tianyu Lan
linux kernel enabling team

Re: [PATCH] PM / Qos: Ensure device not in PRM_SUSPENDED when pm qos flags request functions are invoked in the pm core.

[Rafael J. Wysocki]

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


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] PM / Qos: Ensure device not in PRM_SUSPENDED when pm qos flags request functions are invoked in the pm core.

[Lan Tianyu]

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


-- 
Best Regards
Tianyu Lan
linux kernel enabling team

Re: [PATCH] PM / Qos: Ensure device not in PRM_SUSPENDED when pm qos flags request functions are invoked in the pm core.

[Lan Tianyu]

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
>
>


-- 
Best Regards
Tianyu Lan
linux kernel enabling team

Re: [PATCH] PM / Qos: Ensure device not in PRM_SUSPENDED when pm qos flags request functions are invoked in the pm core.

[Rafael J. Wysocki]

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


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.