Re: [RFT][PATCH v2 1/2] PM / domains: Rework governor code to be more consistent

[Rafael J. Wysocki]

On Mon, Nov 6, 2017 at 3:38 PM, Ulf Hansson  wrote:
> [...]
>

 So I guess I'll simply evaluate dev_pm_qos_read_value(dev) if
 subsys_data or subsys_data->domain_data is not there.
>>>
>>> Yes.
>>>
>>> However, if it returns -1, what value should you pick? 0?
>>
>> Without the second patch, -1 will just mean "no suspend", so the
>> parent cannot be suspended too, but that should just work AFAICS
>> (effective_constraint_ns may be -1 too at that point, if present).
>
> I am fine with whatever policy you pick.
>
> However, I suspect it may be more tricky respecting a -1 (no suspend),
> because this means dev_update_qos_constraint() then may continue to
> return a negative value, which you changed the caller,
> default_suspend_ok(), to not cope with.

Oh, I just need to restore the constraint_ns < 0 check I dropped,
because it was never going to trigger.

Thanks,
Rafael

Re: [RFT][PATCH v2 1/2] PM / domains: Rework governor code to be more consistent

[Rafael J. Wysocki]

On Mon, Nov 6, 2017 at 3:38 PM, Ulf Hansson  wrote:
> [...]
>

 So I guess I'll simply evaluate dev_pm_qos_read_value(dev) if
 subsys_data or subsys_data->domain_data is not there.
>>>
>>> Yes.
>>>
>>> However, if it returns -1, what value should you pick? 0?
>>
>> Without the second patch, -1 will just mean "no suspend", so the
>> parent cannot be suspended too, but that should just work AFAICS
>> (effective_constraint_ns may be -1 too at that point, if present).
>
> I am fine with whatever policy you pick.
>
> However, I suspect it may be more tricky respecting a -1 (no suspend),
> because this means dev_update_qos_constraint() then may continue to
> return a negative value, which you changed the caller,
> default_suspend_ok(), to not cope with.

Oh, I just need to restore the constraint_ns < 0 check I dropped,
because it was never going to trigger.

Thanks,
Rafael

Re: [RFT][PATCH v2 1/2] PM / domains: Rework governor code to be more consistent

[Ulf Hansson]

[...]

>>>
>>> So I guess I'll simply evaluate dev_pm_qos_read_value(dev) if
>>> subsys_data or subsys_data->domain_data is not there.
>>
>> Yes.
>>
>> However, if it returns -1, what value should you pick? 0?
>
> Without the second patch, -1 will just mean "no suspend", so the
> parent cannot be suspended too, but that should just work AFAICS
> (effective_constraint_ns may be -1 too at that point, if present).

I am fine with whatever policy you pick.

However, I suspect it may be more tricky respecting a -1 (no suspend),
because this means dev_update_qos_constraint() then may continue to
return a negative value, which you changed the caller,
default_suspend_ok(), to not cope with.

Anyway, let me look at the code one you posted a new version. :-)

[...]

Kind regards
Uffe

Re: [RFT][PATCH v2 1/2] PM / domains: Rework governor code to be more consistent

[Ulf Hansson]

[...]

>>>
>>> So I guess I'll simply evaluate dev_pm_qos_read_value(dev) if
>>> subsys_data or subsys_data->domain_data is not there.
>>
>> Yes.
>>
>> However, if it returns -1, what value should you pick? 0?
>
> Without the second patch, -1 will just mean "no suspend", so the
> parent cannot be suspended too, but that should just work AFAICS
> (effective_constraint_ns may be -1 too at that point, if present).

I am fine with whatever policy you pick.

However, I suspect it may be more tricky respecting a -1 (no suspend),
because this means dev_update_qos_constraint() then may continue to
return a negative value, which you changed the caller,
default_suspend_ok(), to not cope with.

Anyway, let me look at the code one you posted a new version. :-)

[...]

Kind regards
Uffe

Re: [RFT][PATCH v2 1/2] PM / domains: Rework governor code to be more consistent

[Rafael J. Wysocki]

On Mon, Nov 6, 2017 at 1:44 PM, Ulf Hansson  wrote:
> [...]
>
  static int dev_update_qos_constraint(struct device *dev, void *data)
  {
 s64 *constraint_ns_p = data;
 -   s32 constraint_ns = -1;
 -
 -   if (dev->power.subsys_data && dev->power.subsys_data->domain_data)
 -   constraint_ns = 
 dev_gpd_data(dev)->td.effective_constraint_ns;
 +   s64 constraint_ns;

 -   if (constraint_ns < 0) {
 -   constraint_ns = dev_pm_qos_read_value(dev);
 -   constraint_ns *= NSEC_PER_USEC;
 -   }
 -   if (constraint_ns == 0)
 +   if (!dev->power.subsys_data || 
 !dev->power.subsys_data->domain_data)
 return 0;

 /*
 -* constraint_ns cannot be negative here, because the device has 
 been
 -* suspended.
 +* Only take suspend-time QoS constraints of devices into account,
 +* because constraints updated after the device has been suspended 
 are
 +* not guaranteed to be taken into account anyway.  In order for 
 them
 +* to take effect, the device has to be resumed and suspended 
 again.
  */
>>>
>>> This means a change in behavior, because earlier we took into account
>>> QoS values for child devices that were not attached to a genpd.
>>
>> OK
>>
>> I have overlooked it or rather have forgotten about that.
>>
>>> Is there any reason you think we should change this, or is it just
>>> something you overlooked here?
>>>
>>> I understand, that if the QoS constraint has been updated after such
>>> child device has been suspended, it's tricky to take a correct
>>> decision.
>>
>> Right, but if they are not in a domain, the best we can do is to look
>> at the current value.
>>
>>> To really solve this, we would either have to register a QoS notifier
>>> for all children devices that has its parent attached to a genpd, or
>>> always runtime resume devices before updating QoS constraints.
>>>
>>> Non of these options is perfect, so perhaps we should consider a "best
>>> effort" approach instead? Whatever that may be.
>>
>> I think best effort makes most sense.
>
> Okay!
>
>>
>> So I guess I'll simply evaluate dev_pm_qos_read_value(dev) if
>> subsys_data or subsys_data->domain_data is not there.
>
> Yes.
>
> However, if it returns -1, what value should you pick? 0?

Without the second patch, -1 will just mean "no suspend", so the
parent cannot be suspended too, but that should just work AFAICS
(effective_constraint_ns may be -1 too at that point, if present).

With the second patch it cannot be -1 any more. :-)

Thanks,
Rafael

Re: [RFT][PATCH v2 1/2] PM / domains: Rework governor code to be more consistent

[Rafael J. Wysocki]

On Mon, Nov 6, 2017 at 1:44 PM, Ulf Hansson  wrote:
> [...]
>
  static int dev_update_qos_constraint(struct device *dev, void *data)
  {
 s64 *constraint_ns_p = data;
 -   s32 constraint_ns = -1;
 -
 -   if (dev->power.subsys_data && dev->power.subsys_data->domain_data)
 -   constraint_ns = 
 dev_gpd_data(dev)->td.effective_constraint_ns;
 +   s64 constraint_ns;

 -   if (constraint_ns < 0) {
 -   constraint_ns = dev_pm_qos_read_value(dev);
 -   constraint_ns *= NSEC_PER_USEC;
 -   }
 -   if (constraint_ns == 0)
 +   if (!dev->power.subsys_data || 
 !dev->power.subsys_data->domain_data)
 return 0;

 /*
 -* constraint_ns cannot be negative here, because the device has 
 been
 -* suspended.
 +* Only take suspend-time QoS constraints of devices into account,
 +* because constraints updated after the device has been suspended 
 are
 +* not guaranteed to be taken into account anyway.  In order for 
 them
 +* to take effect, the device has to be resumed and suspended 
 again.
  */
>>>
>>> This means a change in behavior, because earlier we took into account
>>> QoS values for child devices that were not attached to a genpd.
>>
>> OK
>>
>> I have overlooked it or rather have forgotten about that.
>>
>>> Is there any reason you think we should change this, or is it just
>>> something you overlooked here?
>>>
>>> I understand, that if the QoS constraint has been updated after such
>>> child device has been suspended, it's tricky to take a correct
>>> decision.
>>
>> Right, but if they are not in a domain, the best we can do is to look
>> at the current value.
>>
>>> To really solve this, we would either have to register a QoS notifier
>>> for all children devices that has its parent attached to a genpd, or
>>> always runtime resume devices before updating QoS constraints.
>>>
>>> Non of these options is perfect, so perhaps we should consider a "best
>>> effort" approach instead? Whatever that may be.
>>
>> I think best effort makes most sense.
>
> Okay!
>
>>
>> So I guess I'll simply evaluate dev_pm_qos_read_value(dev) if
>> subsys_data or subsys_data->domain_data is not there.
>
> Yes.
>
> However, if it returns -1, what value should you pick? 0?

Without the second patch, -1 will just mean "no suspend", so the
parent cannot be suspended too, but that should just work AFAICS
(effective_constraint_ns may be -1 too at that point, if present).

With the second patch it cannot be -1 any more. :-)

Thanks,
Rafael

Re: [RFT][PATCH v2 1/2] PM / domains: Rework governor code to be more consistent

[Ulf Hansson]

[...]

>>>  static int dev_update_qos_constraint(struct device *dev, void *data)
>>>  {
>>> s64 *constraint_ns_p = data;
>>> -   s32 constraint_ns = -1;
>>> -
>>> -   if (dev->power.subsys_data && dev->power.subsys_data->domain_data)
>>> -   constraint_ns = 
>>> dev_gpd_data(dev)->td.effective_constraint_ns;
>>> +   s64 constraint_ns;
>>>
>>> -   if (constraint_ns < 0) {
>>> -   constraint_ns = dev_pm_qos_read_value(dev);
>>> -   constraint_ns *= NSEC_PER_USEC;
>>> -   }
>>> -   if (constraint_ns == 0)
>>> +   if (!dev->power.subsys_data || !dev->power.subsys_data->domain_data)
>>> return 0;
>>>
>>> /*
>>> -* constraint_ns cannot be negative here, because the device has 
>>> been
>>> -* suspended.
>>> +* Only take suspend-time QoS constraints of devices into account,
>>> +* because constraints updated after the device has been suspended 
>>> are
>>> +* not guaranteed to be taken into account anyway.  In order for 
>>> them
>>> +* to take effect, the device has to be resumed and suspended again.
>>>  */
>>
>> This means a change in behavior, because earlier we took into account
>> QoS values for child devices that were not attached to a genpd.
>
> OK
>
> I have overlooked it or rather have forgotten about that.
>
>> Is there any reason you think we should change this, or is it just
>> something you overlooked here?
>>
>> I understand, that if the QoS constraint has been updated after such
>> child device has been suspended, it's tricky to take a correct
>> decision.
>
> Right, but if they are not in a domain, the best we can do is to look
> at the current value.
>
>> To really solve this, we would either have to register a QoS notifier
>> for all children devices that has its parent attached to a genpd, or
>> always runtime resume devices before updating QoS constraints.
>>
>> Non of these options is perfect, so perhaps we should consider a "best
>> effort" approach instead? Whatever that may be.
>
> I think best effort makes most sense.

Okay!

>
> So I guess I'll simply evaluate dev_pm_qos_read_value(dev) if
> subsys_data or subsys_data->domain_data is not there.

Yes.

However, if it returns -1, what value should you pick? 0?

>
> Of course, that doesn't apply to the code in __default_power_down_ok()
> as that only takes device in the domain into account anyway.

Yep, agree!

Kind regards
Uffe

Re: [RFT][PATCH v2 1/2] PM / domains: Rework governor code to be more consistent

[Ulf Hansson]

[...]

>>>  static int dev_update_qos_constraint(struct device *dev, void *data)
>>>  {
>>> s64 *constraint_ns_p = data;
>>> -   s32 constraint_ns = -1;
>>> -
>>> -   if (dev->power.subsys_data && dev->power.subsys_data->domain_data)
>>> -   constraint_ns = 
>>> dev_gpd_data(dev)->td.effective_constraint_ns;
>>> +   s64 constraint_ns;
>>>
>>> -   if (constraint_ns < 0) {
>>> -   constraint_ns = dev_pm_qos_read_value(dev);
>>> -   constraint_ns *= NSEC_PER_USEC;
>>> -   }
>>> -   if (constraint_ns == 0)
>>> +   if (!dev->power.subsys_data || !dev->power.subsys_data->domain_data)
>>> return 0;
>>>
>>> /*
>>> -* constraint_ns cannot be negative here, because the device has 
>>> been
>>> -* suspended.
>>> +* Only take suspend-time QoS constraints of devices into account,
>>> +* because constraints updated after the device has been suspended 
>>> are
>>> +* not guaranteed to be taken into account anyway.  In order for 
>>> them
>>> +* to take effect, the device has to be resumed and suspended again.
>>>  */
>>
>> This means a change in behavior, because earlier we took into account
>> QoS values for child devices that were not attached to a genpd.
>
> OK
>
> I have overlooked it or rather have forgotten about that.
>
>> Is there any reason you think we should change this, or is it just
>> something you overlooked here?
>>
>> I understand, that if the QoS constraint has been updated after such
>> child device has been suspended, it's tricky to take a correct
>> decision.
>
> Right, but if they are not in a domain, the best we can do is to look
> at the current value.
>
>> To really solve this, we would either have to register a QoS notifier
>> for all children devices that has its parent attached to a genpd, or
>> always runtime resume devices before updating QoS constraints.
>>
>> Non of these options is perfect, so perhaps we should consider a "best
>> effort" approach instead? Whatever that may be.
>
> I think best effort makes most sense.

Okay!

>
> So I guess I'll simply evaluate dev_pm_qos_read_value(dev) if
> subsys_data or subsys_data->domain_data is not there.

Yes.

However, if it returns -1, what value should you pick? 0?

>
> Of course, that doesn't apply to the code in __default_power_down_ok()
> as that only takes device in the domain into account anyway.

Yep, agree!

Kind regards
Uffe

Re: [RFT][PATCH v2 1/2] PM / domains: Rework governor code to be more consistent

[Rafael J. Wysocki]

On Mon, Nov 6, 2017 at 1:10 PM, Ulf Hansson  wrote:
> On 3 November 2017 at 12:47, Rafael J. Wysocki  wrote:
>> From: Rafael J. Wysocki 
>>
>> The genpd governor currently uses negative PM QoS values to indicate
>> the "no suspend" condition and 0 as "no restriction", but it doesn't
>> use them consistently.  Moreover, it tries to refresh QoS values for
>> already suspended devices in a quite questionable way.
>>
>> For the above reasons, rework it to be a bit more consistent.
>>
>> First off, note that dev_pm_qos_read_value() in
>> dev_update_qos_constraint() and __default_power_down_ok() is
>> evaluated for devices in suspend.  Moreover, that only happens if the
>> effective_constraint_ns value for them is negative (meaning "no
>> suspend").  It is not evaluated in any other cases, so effectively
>> the QoS values are only updated for devices in suspend that should
>> not have been suspended in the first place.  In all of the other
>> cases, the QoS values taken into account are the effective ones from
>> the time before the device has been suspended, so generally devices
>> need to be resumed and suspended again for new QoS values to take
>> effect anyway.  Thus evaluating dev_update_qos_constraint() in
>> those two places doesn't make sense at all, so drop it.
>>
>> Second, initialize effective_constraint_ns to 0 ("no constraint")
>> rather than to (-1) ("no suspend"), which makes more sense in
>> general and in case effective_constraint_ns is never updated
>> (the device is in suspend all the time or it is never suspended)
>> it doesn't affect the device's parent and so on.
>>
>> Finally, rework default_suspend_ok() to explicitly handle the
>> "no restriction" special case.
>>
>> Signed-off-by: Rafael J. Wysocki 
>> ---
>>  drivers/base/power/domain.c  |2 -
>>  drivers/base/power/domain_governor.c |   61 
>> +--
>>  2 files changed, 38 insertions(+), 25 deletions(-)
>>
>> Index: linux-pm/drivers/base/power/domain.c
>> ===
>> --- linux-pm.orig/drivers/base/power/domain.c
>> +++ linux-pm/drivers/base/power/domain.c
>> @@ -1331,7 +1331,7 @@ static struct generic_pm_domain_data *ge
>>
>> gpd_data->base.dev = dev;
>> gpd_data->td.constraint_changed = true;
>> -   gpd_data->td.effective_constraint_ns = -1;
>> +   gpd_data->td.effective_constraint_ns = 0;
>> gpd_data->nb.notifier_call = genpd_dev_pm_qos_notifier;
>>
>> spin_lock_irq(>power.lock);
>> Index: linux-pm/drivers/base/power/domain_governor.c
>> ===
>> --- linux-pm.orig/drivers/base/power/domain_governor.c
>> +++ linux-pm/drivers/base/power/domain_governor.c
>> @@ -14,22 +14,22 @@
>>  static int dev_update_qos_constraint(struct device *dev, void *data)
>>  {
>> s64 *constraint_ns_p = data;
>> -   s32 constraint_ns = -1;
>> -
>> -   if (dev->power.subsys_data && dev->power.subsys_data->domain_data)
>> -   constraint_ns = 
>> dev_gpd_data(dev)->td.effective_constraint_ns;
>> +   s64 constraint_ns;
>>
>> -   if (constraint_ns < 0) {
>> -   constraint_ns = dev_pm_qos_read_value(dev);
>> -   constraint_ns *= NSEC_PER_USEC;
>> -   }
>> -   if (constraint_ns == 0)
>> +   if (!dev->power.subsys_data || !dev->power.subsys_data->domain_data)
>> return 0;
>>
>> /*
>> -* constraint_ns cannot be negative here, because the device has been
>> -* suspended.
>> +* Only take suspend-time QoS constraints of devices into account,
>> +* because constraints updated after the device has been suspended 
>> are
>> +* not guaranteed to be taken into account anyway.  In order for them
>> +* to take effect, the device has to be resumed and suspended again.
>>  */
>
> This means a change in behavior, because earlier we took into account
> QoS values for child devices that were not attached to a genpd.

OK

I have overlooked it or rather have forgotten about that.

> Is there any reason you think we should change this, or is it just
> something you overlooked here?
>
> I understand, that if the QoS constraint has been updated after such
> child device has been suspended, it's tricky to take a correct
> decision.

Right, but if they are not in a domain, the best we can do is to look
at the current value.

> To really solve this, we would either have to register a QoS notifier
> for all children devices that has its parent attached to a genpd, or
> always runtime resume devices before updating QoS constraints.
>
> Non of these options is perfect, so perhaps we should consider a "best
> effort" approach instead? Whatever that may be.

I think best effort makes most sense.

So I guess I'll simply evaluate 

Re: [RFT][PATCH v2 1/2] PM / domains: Rework governor code to be more consistent

[Rafael J. Wysocki]

On Mon, Nov 6, 2017 at 1:10 PM, Ulf Hansson  wrote:
> On 3 November 2017 at 12:47, Rafael J. Wysocki  wrote:
>> From: Rafael J. Wysocki 
>>
>> The genpd governor currently uses negative PM QoS values to indicate
>> the "no suspend" condition and 0 as "no restriction", but it doesn't
>> use them consistently.  Moreover, it tries to refresh QoS values for
>> already suspended devices in a quite questionable way.
>>
>> For the above reasons, rework it to be a bit more consistent.
>>
>> First off, note that dev_pm_qos_read_value() in
>> dev_update_qos_constraint() and __default_power_down_ok() is
>> evaluated for devices in suspend.  Moreover, that only happens if the
>> effective_constraint_ns value for them is negative (meaning "no
>> suspend").  It is not evaluated in any other cases, so effectively
>> the QoS values are only updated for devices in suspend that should
>> not have been suspended in the first place.  In all of the other
>> cases, the QoS values taken into account are the effective ones from
>> the time before the device has been suspended, so generally devices
>> need to be resumed and suspended again for new QoS values to take
>> effect anyway.  Thus evaluating dev_update_qos_constraint() in
>> those two places doesn't make sense at all, so drop it.
>>
>> Second, initialize effective_constraint_ns to 0 ("no constraint")
>> rather than to (-1) ("no suspend"), which makes more sense in
>> general and in case effective_constraint_ns is never updated
>> (the device is in suspend all the time or it is never suspended)
>> it doesn't affect the device's parent and so on.
>>
>> Finally, rework default_suspend_ok() to explicitly handle the
>> "no restriction" special case.
>>
>> Signed-off-by: Rafael J. Wysocki 
>> ---
>>  drivers/base/power/domain.c  |2 -
>>  drivers/base/power/domain_governor.c |   61 
>> +--
>>  2 files changed, 38 insertions(+), 25 deletions(-)
>>
>> Index: linux-pm/drivers/base/power/domain.c
>> ===
>> --- linux-pm.orig/drivers/base/power/domain.c
>> +++ linux-pm/drivers/base/power/domain.c
>> @@ -1331,7 +1331,7 @@ static struct generic_pm_domain_data *ge
>>
>> gpd_data->base.dev = dev;
>> gpd_data->td.constraint_changed = true;
>> -   gpd_data->td.effective_constraint_ns = -1;
>> +   gpd_data->td.effective_constraint_ns = 0;
>> gpd_data->nb.notifier_call = genpd_dev_pm_qos_notifier;
>>
>> spin_lock_irq(>power.lock);
>> Index: linux-pm/drivers/base/power/domain_governor.c
>> ===
>> --- linux-pm.orig/drivers/base/power/domain_governor.c
>> +++ linux-pm/drivers/base/power/domain_governor.c
>> @@ -14,22 +14,22 @@
>>  static int dev_update_qos_constraint(struct device *dev, void *data)
>>  {
>> s64 *constraint_ns_p = data;
>> -   s32 constraint_ns = -1;
>> -
>> -   if (dev->power.subsys_data && dev->power.subsys_data->domain_data)
>> -   constraint_ns = 
>> dev_gpd_data(dev)->td.effective_constraint_ns;
>> +   s64 constraint_ns;
>>
>> -   if (constraint_ns < 0) {
>> -   constraint_ns = dev_pm_qos_read_value(dev);
>> -   constraint_ns *= NSEC_PER_USEC;
>> -   }
>> -   if (constraint_ns == 0)
>> +   if (!dev->power.subsys_data || !dev->power.subsys_data->domain_data)
>> return 0;
>>
>> /*
>> -* constraint_ns cannot be negative here, because the device has been
>> -* suspended.
>> +* Only take suspend-time QoS constraints of devices into account,
>> +* because constraints updated after the device has been suspended 
>> are
>> +* not guaranteed to be taken into account anyway.  In order for them
>> +* to take effect, the device has to be resumed and suspended again.
>>  */
>
> This means a change in behavior, because earlier we took into account
> QoS values for child devices that were not attached to a genpd.

OK

I have overlooked it or rather have forgotten about that.

> Is there any reason you think we should change this, or is it just
> something you overlooked here?
>
> I understand, that if the QoS constraint has been updated after such
> child device has been suspended, it's tricky to take a correct
> decision.

Right, but if they are not in a domain, the best we can do is to look
at the current value.

> To really solve this, we would either have to register a QoS notifier
> for all children devices that has its parent attached to a genpd, or
> always runtime resume devices before updating QoS constraints.
>
> Non of these options is perfect, so perhaps we should consider a "best
> effort" approach instead? Whatever that may be.

I think best effort makes most sense.

So I guess I'll simply evaluate dev_pm_qos_read_value(dev) if
subsys_data or subsys_data->domain_data is not there.

Of course, that doesn't apply to the 

Re: [RFT][PATCH v2 1/2] PM / domains: Rework governor code to be more consistent

[Ulf Hansson]

On 3 November 2017 at 12:47, Rafael J. Wysocki  wrote:
> From: Rafael J. Wysocki 
>
> The genpd governor currently uses negative PM QoS values to indicate
> the "no suspend" condition and 0 as "no restriction", but it doesn't
> use them consistently.  Moreover, it tries to refresh QoS values for
> already suspended devices in a quite questionable way.
>
> For the above reasons, rework it to be a bit more consistent.
>
> First off, note that dev_pm_qos_read_value() in
> dev_update_qos_constraint() and __default_power_down_ok() is
> evaluated for devices in suspend.  Moreover, that only happens if the
> effective_constraint_ns value for them is negative (meaning "no
> suspend").  It is not evaluated in any other cases, so effectively
> the QoS values are only updated for devices in suspend that should
> not have been suspended in the first place.  In all of the other
> cases, the QoS values taken into account are the effective ones from
> the time before the device has been suspended, so generally devices
> need to be resumed and suspended again for new QoS values to take
> effect anyway.  Thus evaluating dev_update_qos_constraint() in
> those two places doesn't make sense at all, so drop it.
>
> Second, initialize effective_constraint_ns to 0 ("no constraint")
> rather than to (-1) ("no suspend"), which makes more sense in
> general and in case effective_constraint_ns is never updated
> (the device is in suspend all the time or it is never suspended)
> it doesn't affect the device's parent and so on.
>
> Finally, rework default_suspend_ok() to explicitly handle the
> "no restriction" special case.
>
> Signed-off-by: Rafael J. Wysocki 
> ---
>  drivers/base/power/domain.c  |2 -
>  drivers/base/power/domain_governor.c |   61 
> +--
>  2 files changed, 38 insertions(+), 25 deletions(-)
>
> Index: linux-pm/drivers/base/power/domain.c
> ===
> --- linux-pm.orig/drivers/base/power/domain.c
> +++ linux-pm/drivers/base/power/domain.c
> @@ -1331,7 +1331,7 @@ static struct generic_pm_domain_data *ge
>
> gpd_data->base.dev = dev;
> gpd_data->td.constraint_changed = true;
> -   gpd_data->td.effective_constraint_ns = -1;
> +   gpd_data->td.effective_constraint_ns = 0;
> gpd_data->nb.notifier_call = genpd_dev_pm_qos_notifier;
>
> spin_lock_irq(>power.lock);
> Index: linux-pm/drivers/base/power/domain_governor.c
> ===
> --- linux-pm.orig/drivers/base/power/domain_governor.c
> +++ linux-pm/drivers/base/power/domain_governor.c
> @@ -14,22 +14,22 @@
>  static int dev_update_qos_constraint(struct device *dev, void *data)
>  {
> s64 *constraint_ns_p = data;
> -   s32 constraint_ns = -1;
> -
> -   if (dev->power.subsys_data && dev->power.subsys_data->domain_data)
> -   constraint_ns = dev_gpd_data(dev)->td.effective_constraint_ns;
> +   s64 constraint_ns;
>
> -   if (constraint_ns < 0) {
> -   constraint_ns = dev_pm_qos_read_value(dev);
> -   constraint_ns *= NSEC_PER_USEC;
> -   }
> -   if (constraint_ns == 0)
> +   if (!dev->power.subsys_data || !dev->power.subsys_data->domain_data)
> return 0;
>
> /*
> -* constraint_ns cannot be negative here, because the device has been
> -* suspended.
> +* Only take suspend-time QoS constraints of devices into account,
> +* because constraints updated after the device has been suspended are
> +* not guaranteed to be taken into account anyway.  In order for them
> +* to take effect, the device has to be resumed and suspended again.
>  */

This means a change in behavior, because earlier we took into account
QoS values for child devices that were not attached to a genpd.

Is there any reason you think we should change this, or is it just
something you overlooked here?

I understand, that if the QoS constraint has been updated after such
child device has been suspended, it's tricky to take a correct
decision.

To really solve this, we would either have to register a QoS notifier
for all children devices that has its parent attached to a genpd, or
always runtime resume devices before updating QoS constraints.

Non of these options is perfect, so perhaps we should consider a "best
effort" approach instead? Whatever that may be.

> +   constraint_ns = dev_gpd_data(dev)->td.effective_constraint_ns;
> +   /* 0 means "no constraint" */
> +   if (constraint_ns == 0)
> +   return 0;
> +
> if (constraint_ns < *constraint_ns_p || *constraint_ns_p == 0)
> *constraint_ns_p = constraint_ns;
>
> @@ -76,14 +76,29 @@ static bool default_suspend_ok(struct de
> device_for_each_child(dev, _ns,
>   

Re: [RFT][PATCH v2 1/2] PM / domains: Rework governor code to be more consistent

[Ulf Hansson]

On 3 November 2017 at 12:47, Rafael J. Wysocki  wrote:
> From: Rafael J. Wysocki 
>
> The genpd governor currently uses negative PM QoS values to indicate
> the "no suspend" condition and 0 as "no restriction", but it doesn't
> use them consistently.  Moreover, it tries to refresh QoS values for
> already suspended devices in a quite questionable way.
>
> For the above reasons, rework it to be a bit more consistent.
>
> First off, note that dev_pm_qos_read_value() in
> dev_update_qos_constraint() and __default_power_down_ok() is
> evaluated for devices in suspend.  Moreover, that only happens if the
> effective_constraint_ns value for them is negative (meaning "no
> suspend").  It is not evaluated in any other cases, so effectively
> the QoS values are only updated for devices in suspend that should
> not have been suspended in the first place.  In all of the other
> cases, the QoS values taken into account are the effective ones from
> the time before the device has been suspended, so generally devices
> need to be resumed and suspended again for new QoS values to take
> effect anyway.  Thus evaluating dev_update_qos_constraint() in
> those two places doesn't make sense at all, so drop it.
>
> Second, initialize effective_constraint_ns to 0 ("no constraint")
> rather than to (-1) ("no suspend"), which makes more sense in
> general and in case effective_constraint_ns is never updated
> (the device is in suspend all the time or it is never suspended)
> it doesn't affect the device's parent and so on.
>
> Finally, rework default_suspend_ok() to explicitly handle the
> "no restriction" special case.
>
> Signed-off-by: Rafael J. Wysocki 
> ---
>  drivers/base/power/domain.c  |2 -
>  drivers/base/power/domain_governor.c |   61 
> +--
>  2 files changed, 38 insertions(+), 25 deletions(-)
>
> Index: linux-pm/drivers/base/power/domain.c
> ===
> --- linux-pm.orig/drivers/base/power/domain.c
> +++ linux-pm/drivers/base/power/domain.c
> @@ -1331,7 +1331,7 @@ static struct generic_pm_domain_data *ge
>
> gpd_data->base.dev = dev;
> gpd_data->td.constraint_changed = true;
> -   gpd_data->td.effective_constraint_ns = -1;
> +   gpd_data->td.effective_constraint_ns = 0;
> gpd_data->nb.notifier_call = genpd_dev_pm_qos_notifier;
>
> spin_lock_irq(>power.lock);
> Index: linux-pm/drivers/base/power/domain_governor.c
> ===
> --- linux-pm.orig/drivers/base/power/domain_governor.c
> +++ linux-pm/drivers/base/power/domain_governor.c
> @@ -14,22 +14,22 @@
>  static int dev_update_qos_constraint(struct device *dev, void *data)
>  {
> s64 *constraint_ns_p = data;
> -   s32 constraint_ns = -1;
> -
> -   if (dev->power.subsys_data && dev->power.subsys_data->domain_data)
> -   constraint_ns = dev_gpd_data(dev)->td.effective_constraint_ns;
> +   s64 constraint_ns;
>
> -   if (constraint_ns < 0) {
> -   constraint_ns = dev_pm_qos_read_value(dev);
> -   constraint_ns *= NSEC_PER_USEC;
> -   }
> -   if (constraint_ns == 0)
> +   if (!dev->power.subsys_data || !dev->power.subsys_data->domain_data)
> return 0;
>
> /*
> -* constraint_ns cannot be negative here, because the device has been
> -* suspended.
> +* Only take suspend-time QoS constraints of devices into account,
> +* because constraints updated after the device has been suspended are
> +* not guaranteed to be taken into account anyway.  In order for them
> +* to take effect, the device has to be resumed and suspended again.
>  */

This means a change in behavior, because earlier we took into account
QoS values for child devices that were not attached to a genpd.

Is there any reason you think we should change this, or is it just
something you overlooked here?

I understand, that if the QoS constraint has been updated after such
child device has been suspended, it's tricky to take a correct
decision.

To really solve this, we would either have to register a QoS notifier
for all children devices that has its parent attached to a genpd, or
always runtime resume devices before updating QoS constraints.

Non of these options is perfect, so perhaps we should consider a "best
effort" approach instead? Whatever that may be.

> +   constraint_ns = dev_gpd_data(dev)->td.effective_constraint_ns;
> +   /* 0 means "no constraint" */
> +   if (constraint_ns == 0)
> +   return 0;
> +
> if (constraint_ns < *constraint_ns_p || *constraint_ns_p == 0)
> *constraint_ns_p = constraint_ns;
>
> @@ -76,14 +76,29 @@ static bool default_suspend_ok(struct de
> device_for_each_child(dev, _ns,
>   dev_update_qos_constraint);
>
> -   if (constraint_ns > 

Re: [RFT][PATCH v2 1/2] PM / domains: Rework governor code to be more consistent

[Ramesh Thomas]

On 2017-11-04 at 12:24:15 +0100, Rafael J. Wysocki wrote:
> On Sat, Nov 4, 2017 at 3:34 AM, Ramesh Thomas  wrote:
> > On 2017-11-03 at 12:47:20 +0100, Rafael J. Wysocki wrote:
> >> From: Rafael J. Wysocki 
> >>
> >> The genpd governor currently uses negative PM QoS values to indicate
> >> the "no suspend" condition and 0 as "no restriction", but it doesn't
> >> use them consistently.  Moreover, it tries to refresh QoS values for
> >> already suspended devices in a quite questionable way.
> >>
> >> For the above reasons, rework it to be a bit more consistent.
> >>
> >> First off, note that dev_pm_qos_read_value() in
> >> dev_update_qos_constraint() and __default_power_down_ok() is
> >> evaluated for devices in suspend.  Moreover, that only happens if the
> >> effective_constraint_ns value for them is negative (meaning "no
> >> suspend").  It is not evaluated in any other cases, so effectively
> >> the QoS values are only updated for devices in suspend that should
> >> not have been suspended in the first place.  In all of the other
> >> cases, the QoS values taken into account are the effective ones from
> >> the time before the device has been suspended, so generally devices
> >> need to be resumed and suspended again for new QoS values to take
> >> effect anyway.  Thus evaluating dev_update_qos_constraint() in
> >> those two places doesn't make sense at all, so drop it.
> >>
> >> Second, initialize effective_constraint_ns to 0 ("no constraint")
> >> rather than to (-1) ("no suspend"), which makes more sense in
> >> general and in case effective_constraint_ns is never updated
> >> (the device is in suspend all the time or it is never suspended)
> >> it doesn't affect the device's parent and so on.
> >>
> >> Finally, rework default_suspend_ok() to explicitly handle the
> >> "no restriction" special case.
> >>
> >> Signed-off-by: Rafael J. Wysocki 
> >> ---
> 
> [cut]
> 
> > Looks good to me.
> >
> > Acked-by: Ramesh Thomas 
> 
> Thanks!
> 
> Do you actually mean Reviewed-by?

Yes, it should be Reveiewed-by for both patches!

Thanks,
Ramesh

Re: [RFT][PATCH v2 1/2] PM / domains: Rework governor code to be more consistent

[Ramesh Thomas]

On 2017-11-04 at 12:24:15 +0100, Rafael J. Wysocki wrote:
> On Sat, Nov 4, 2017 at 3:34 AM, Ramesh Thomas  wrote:
> > On 2017-11-03 at 12:47:20 +0100, Rafael J. Wysocki wrote:
> >> From: Rafael J. Wysocki 
> >>
> >> The genpd governor currently uses negative PM QoS values to indicate
> >> the "no suspend" condition and 0 as "no restriction", but it doesn't
> >> use them consistently.  Moreover, it tries to refresh QoS values for
> >> already suspended devices in a quite questionable way.
> >>
> >> For the above reasons, rework it to be a bit more consistent.
> >>
> >> First off, note that dev_pm_qos_read_value() in
> >> dev_update_qos_constraint() and __default_power_down_ok() is
> >> evaluated for devices in suspend.  Moreover, that only happens if the
> >> effective_constraint_ns value for them is negative (meaning "no
> >> suspend").  It is not evaluated in any other cases, so effectively
> >> the QoS values are only updated for devices in suspend that should
> >> not have been suspended in the first place.  In all of the other
> >> cases, the QoS values taken into account are the effective ones from
> >> the time before the device has been suspended, so generally devices
> >> need to be resumed and suspended again for new QoS values to take
> >> effect anyway.  Thus evaluating dev_update_qos_constraint() in
> >> those two places doesn't make sense at all, so drop it.
> >>
> >> Second, initialize effective_constraint_ns to 0 ("no constraint")
> >> rather than to (-1) ("no suspend"), which makes more sense in
> >> general and in case effective_constraint_ns is never updated
> >> (the device is in suspend all the time or it is never suspended)
> >> it doesn't affect the device's parent and so on.
> >>
> >> Finally, rework default_suspend_ok() to explicitly handle the
> >> "no restriction" special case.
> >>
> >> Signed-off-by: Rafael J. Wysocki 
> >> ---
> 
> [cut]
> 
> > Looks good to me.
> >
> > Acked-by: Ramesh Thomas 
> 
> Thanks!
> 
> Do you actually mean Reviewed-by?

Yes, it should be Reveiewed-by for both patches!

Thanks,
Ramesh

Re: [RFT][PATCH v2 1/2] PM / domains: Rework governor code to be more consistent

[Rafael J. Wysocki]

On Sat, Nov 4, 2017 at 3:34 AM, Ramesh Thomas  wrote:
> On 2017-11-03 at 12:47:20 +0100, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki 
>>
>> The genpd governor currently uses negative PM QoS values to indicate
>> the "no suspend" condition and 0 as "no restriction", but it doesn't
>> use them consistently.  Moreover, it tries to refresh QoS values for
>> already suspended devices in a quite questionable way.
>>
>> For the above reasons, rework it to be a bit more consistent.
>>
>> First off, note that dev_pm_qos_read_value() in
>> dev_update_qos_constraint() and __default_power_down_ok() is
>> evaluated for devices in suspend.  Moreover, that only happens if the
>> effective_constraint_ns value for them is negative (meaning "no
>> suspend").  It is not evaluated in any other cases, so effectively
>> the QoS values are only updated for devices in suspend that should
>> not have been suspended in the first place.  In all of the other
>> cases, the QoS values taken into account are the effective ones from
>> the time before the device has been suspended, so generally devices
>> need to be resumed and suspended again for new QoS values to take
>> effect anyway.  Thus evaluating dev_update_qos_constraint() in
>> those two places doesn't make sense at all, so drop it.
>>
>> Second, initialize effective_constraint_ns to 0 ("no constraint")
>> rather than to (-1) ("no suspend"), which makes more sense in
>> general and in case effective_constraint_ns is never updated
>> (the device is in suspend all the time or it is never suspended)
>> it doesn't affect the device's parent and so on.
>>
>> Finally, rework default_suspend_ok() to explicitly handle the
>> "no restriction" special case.
>>
>> Signed-off-by: Rafael J. Wysocki 
>> ---

[cut]

> Looks good to me.
>
> Acked-by: Ramesh Thomas 

Thanks!

Do you actually mean Reviewed-by?

Re: [RFT][PATCH v2 1/2] PM / domains: Rework governor code to be more consistent

[Rafael J. Wysocki]

On Sat, Nov 4, 2017 at 3:34 AM, Ramesh Thomas  wrote:
> On 2017-11-03 at 12:47:20 +0100, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki 
>>
>> The genpd governor currently uses negative PM QoS values to indicate
>> the "no suspend" condition and 0 as "no restriction", but it doesn't
>> use them consistently.  Moreover, it tries to refresh QoS values for
>> already suspended devices in a quite questionable way.
>>
>> For the above reasons, rework it to be a bit more consistent.
>>
>> First off, note that dev_pm_qos_read_value() in
>> dev_update_qos_constraint() and __default_power_down_ok() is
>> evaluated for devices in suspend.  Moreover, that only happens if the
>> effective_constraint_ns value for them is negative (meaning "no
>> suspend").  It is not evaluated in any other cases, so effectively
>> the QoS values are only updated for devices in suspend that should
>> not have been suspended in the first place.  In all of the other
>> cases, the QoS values taken into account are the effective ones from
>> the time before the device has been suspended, so generally devices
>> need to be resumed and suspended again for new QoS values to take
>> effect anyway.  Thus evaluating dev_update_qos_constraint() in
>> those two places doesn't make sense at all, so drop it.
>>
>> Second, initialize effective_constraint_ns to 0 ("no constraint")
>> rather than to (-1) ("no suspend"), which makes more sense in
>> general and in case effective_constraint_ns is never updated
>> (the device is in suspend all the time or it is never suspended)
>> it doesn't affect the device's parent and so on.
>>
>> Finally, rework default_suspend_ok() to explicitly handle the
>> "no restriction" special case.
>>
>> Signed-off-by: Rafael J. Wysocki 
>> ---

[cut]

> Looks good to me.
>
> Acked-by: Ramesh Thomas 

Thanks!

Do you actually mean Reviewed-by?

Re: [RFT][PATCH v2 1/2] PM / domains: Rework governor code to be more consistent

[Ramesh Thomas]

On 2017-11-03 at 12:47:20 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki 
> 
> The genpd governor currently uses negative PM QoS values to indicate
> the "no suspend" condition and 0 as "no restriction", but it doesn't
> use them consistently.  Moreover, it tries to refresh QoS values for
> already suspended devices in a quite questionable way.
> 
> For the above reasons, rework it to be a bit more consistent.
> 
> First off, note that dev_pm_qos_read_value() in
> dev_update_qos_constraint() and __default_power_down_ok() is
> evaluated for devices in suspend.  Moreover, that only happens if the
> effective_constraint_ns value for them is negative (meaning "no
> suspend").  It is not evaluated in any other cases, so effectively
> the QoS values are only updated for devices in suspend that should
> not have been suspended in the first place.  In all of the other
> cases, the QoS values taken into account are the effective ones from
> the time before the device has been suspended, so generally devices
> need to be resumed and suspended again for new QoS values to take
> effect anyway.  Thus evaluating dev_update_qos_constraint() in
> those two places doesn't make sense at all, so drop it.
> 
> Second, initialize effective_constraint_ns to 0 ("no constraint")
> rather than to (-1) ("no suspend"), which makes more sense in
> general and in case effective_constraint_ns is never updated
> (the device is in suspend all the time or it is never suspended)
> it doesn't affect the device's parent and so on.
> 
> Finally, rework default_suspend_ok() to explicitly handle the
> "no restriction" special case.
> 
> Signed-off-by: Rafael J. Wysocki 
> ---
>  drivers/base/power/domain.c  |2 -
>  drivers/base/power/domain_governor.c |   61 
> +--
>  2 files changed, 38 insertions(+), 25 deletions(-)
> 
> Index: linux-pm/drivers/base/power/domain.c
> ===
> --- linux-pm.orig/drivers/base/power/domain.c
> +++ linux-pm/drivers/base/power/domain.c
> @@ -1331,7 +1331,7 @@ static struct generic_pm_domain_data *ge
>  
>   gpd_data->base.dev = dev;
>   gpd_data->td.constraint_changed = true;
> - gpd_data->td.effective_constraint_ns = -1;
> + gpd_data->td.effective_constraint_ns = 0;
>   gpd_data->nb.notifier_call = genpd_dev_pm_qos_notifier;
>  
>   spin_lock_irq(>power.lock);
> Index: linux-pm/drivers/base/power/domain_governor.c
> ===
> --- linux-pm.orig/drivers/base/power/domain_governor.c
> +++ linux-pm/drivers/base/power/domain_governor.c
> @@ -14,22 +14,22 @@
>  static int dev_update_qos_constraint(struct device *dev, void *data)
>  {
>   s64 *constraint_ns_p = data;
> - s32 constraint_ns = -1;
> -
> - if (dev->power.subsys_data && dev->power.subsys_data->domain_data)
> - constraint_ns = dev_gpd_data(dev)->td.effective_constraint_ns;
> + s64 constraint_ns;
>  
> - if (constraint_ns < 0) {
> - constraint_ns = dev_pm_qos_read_value(dev);
> - constraint_ns *= NSEC_PER_USEC;
> - }
> - if (constraint_ns == 0)
> + if (!dev->power.subsys_data || !dev->power.subsys_data->domain_data)
>   return 0;
>  
>   /*
> -  * constraint_ns cannot be negative here, because the device has been
> -  * suspended.
> +  * Only take suspend-time QoS constraints of devices into account,
> +  * because constraints updated after the device has been suspended are
> +  * not guaranteed to be taken into account anyway.  In order for them
> +  * to take effect, the device has to be resumed and suspended again.
>*/
> + constraint_ns = dev_gpd_data(dev)->td.effective_constraint_ns;
> + /* 0 means "no constraint" */
> + if (constraint_ns == 0)
> + return 0;
> +
>   if (constraint_ns < *constraint_ns_p || *constraint_ns_p == 0)
>   *constraint_ns_p = constraint_ns;
>  
> @@ -76,14 +76,29 @@ static bool default_suspend_ok(struct de
>   device_for_each_child(dev, _ns,
> dev_update_qos_constraint);
>  
> - if (constraint_ns > 0) {
> + if (constraint_ns == 0) {
> + /* "No restriction", so the device is allowed to suspend. */
> + td->effective_constraint_ns = 0;
> + td->cached_suspend_ok = true;
> + } else {
> + /*
> +  * constraint_ns must be positive here, because the children
> +  * walked above are all suspended, so effective_constraint_ns
> +  * cannot be negative for them.
> +  */
>   constraint_ns -= td->suspend_latency_ns +
>   td->resume_latency_ns;
> - if (constraint_ns == 0)
> + /*
> +  * effective_constraint_ns is 

Re: [RFT][PATCH v2 1/2] PM / domains: Rework governor code to be more consistent

[Ramesh Thomas]

On 2017-11-03 at 12:47:20 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki 
> 
> The genpd governor currently uses negative PM QoS values to indicate
> the "no suspend" condition and 0 as "no restriction", but it doesn't
> use them consistently.  Moreover, it tries to refresh QoS values for
> already suspended devices in a quite questionable way.
> 
> For the above reasons, rework it to be a bit more consistent.
> 
> First off, note that dev_pm_qos_read_value() in
> dev_update_qos_constraint() and __default_power_down_ok() is
> evaluated for devices in suspend.  Moreover, that only happens if the
> effective_constraint_ns value for them is negative (meaning "no
> suspend").  It is not evaluated in any other cases, so effectively
> the QoS values are only updated for devices in suspend that should
> not have been suspended in the first place.  In all of the other
> cases, the QoS values taken into account are the effective ones from
> the time before the device has been suspended, so generally devices
> need to be resumed and suspended again for new QoS values to take
> effect anyway.  Thus evaluating dev_update_qos_constraint() in
> those two places doesn't make sense at all, so drop it.
> 
> Second, initialize effective_constraint_ns to 0 ("no constraint")
> rather than to (-1) ("no suspend"), which makes more sense in
> general and in case effective_constraint_ns is never updated
> (the device is in suspend all the time or it is never suspended)
> it doesn't affect the device's parent and so on.
> 
> Finally, rework default_suspend_ok() to explicitly handle the
> "no restriction" special case.
> 
> Signed-off-by: Rafael J. Wysocki 
> ---
>  drivers/base/power/domain.c  |2 -
>  drivers/base/power/domain_governor.c |   61 
> +--
>  2 files changed, 38 insertions(+), 25 deletions(-)
> 
> Index: linux-pm/drivers/base/power/domain.c
> ===
> --- linux-pm.orig/drivers/base/power/domain.c
> +++ linux-pm/drivers/base/power/domain.c
> @@ -1331,7 +1331,7 @@ static struct generic_pm_domain_data *ge
>  
>   gpd_data->base.dev = dev;
>   gpd_data->td.constraint_changed = true;
> - gpd_data->td.effective_constraint_ns = -1;
> + gpd_data->td.effective_constraint_ns = 0;
>   gpd_data->nb.notifier_call = genpd_dev_pm_qos_notifier;
>  
>   spin_lock_irq(>power.lock);
> Index: linux-pm/drivers/base/power/domain_governor.c
> ===
> --- linux-pm.orig/drivers/base/power/domain_governor.c
> +++ linux-pm/drivers/base/power/domain_governor.c
> @@ -14,22 +14,22 @@
>  static int dev_update_qos_constraint(struct device *dev, void *data)
>  {
>   s64 *constraint_ns_p = data;
> - s32 constraint_ns = -1;
> -
> - if (dev->power.subsys_data && dev->power.subsys_data->domain_data)
> - constraint_ns = dev_gpd_data(dev)->td.effective_constraint_ns;
> + s64 constraint_ns;
>  
> - if (constraint_ns < 0) {
> - constraint_ns = dev_pm_qos_read_value(dev);
> - constraint_ns *= NSEC_PER_USEC;
> - }
> - if (constraint_ns == 0)
> + if (!dev->power.subsys_data || !dev->power.subsys_data->domain_data)
>   return 0;
>  
>   /*
> -  * constraint_ns cannot be negative here, because the device has been
> -  * suspended.
> +  * Only take suspend-time QoS constraints of devices into account,
> +  * because constraints updated after the device has been suspended are
> +  * not guaranteed to be taken into account anyway.  In order for them
> +  * to take effect, the device has to be resumed and suspended again.
>*/
> + constraint_ns = dev_gpd_data(dev)->td.effective_constraint_ns;
> + /* 0 means "no constraint" */
> + if (constraint_ns == 0)
> + return 0;
> +
>   if (constraint_ns < *constraint_ns_p || *constraint_ns_p == 0)
>   *constraint_ns_p = constraint_ns;
>  
> @@ -76,14 +76,29 @@ static bool default_suspend_ok(struct de
>   device_for_each_child(dev, _ns,
> dev_update_qos_constraint);
>  
> - if (constraint_ns > 0) {
> + if (constraint_ns == 0) {
> + /* "No restriction", so the device is allowed to suspend. */
> + td->effective_constraint_ns = 0;
> + td->cached_suspend_ok = true;
> + } else {
> + /*
> +  * constraint_ns must be positive here, because the children
> +  * walked above are all suspended, so effective_constraint_ns
> +  * cannot be negative for them.
> +  */
>   constraint_ns -= td->suspend_latency_ns +
>   td->resume_latency_ns;
> - if (constraint_ns == 0)
> + /*
> +  * effective_constraint_ns is negative already and
> +  * cached_suspend_ok is 

[RFT][PATCH v2 1/2] PM / domains: Rework governor code to be more consistent

[Rafael J. Wysocki]

From: Rafael J. Wysocki 

The genpd governor currently uses negative PM QoS values to indicate
the "no suspend" condition and 0 as "no restriction", but it doesn't
use them consistently.  Moreover, it tries to refresh QoS values for
already suspended devices in a quite questionable way.

For the above reasons, rework it to be a bit more consistent.

First off, note that dev_pm_qos_read_value() in
dev_update_qos_constraint() and __default_power_down_ok() is
evaluated for devices in suspend.  Moreover, that only happens if the
effective_constraint_ns value for them is negative (meaning "no
suspend").  It is not evaluated in any other cases, so effectively
the QoS values are only updated for devices in suspend that should
not have been suspended in the first place.  In all of the other
cases, the QoS values taken into account are the effective ones from
the time before the device has been suspended, so generally devices
need to be resumed and suspended again for new QoS values to take
effect anyway.  Thus evaluating dev_update_qos_constraint() in
those two places doesn't make sense at all, so drop it.

Second, initialize effective_constraint_ns to 0 ("no constraint")
rather than to (-1) ("no suspend"), which makes more sense in
general and in case effective_constraint_ns is never updated
(the device is in suspend all the time or it is never suspended)
it doesn't affect the device's parent and so on.

Finally, rework default_suspend_ok() to explicitly handle the
"no restriction" special case.

Signed-off-by: Rafael J. Wysocki 
---
 drivers/base/power/domain.c  |2 -
 drivers/base/power/domain_governor.c |   61 +--
 2 files changed, 38 insertions(+), 25 deletions(-)

Index: linux-pm/drivers/base/power/domain.c
===
--- linux-pm.orig/drivers/base/power/domain.c
+++ linux-pm/drivers/base/power/domain.c
@@ -1331,7 +1331,7 @@ static struct generic_pm_domain_data *ge
 
gpd_data->base.dev = dev;
gpd_data->td.constraint_changed = true;
-   gpd_data->td.effective_constraint_ns = -1;
+   gpd_data->td.effective_constraint_ns = 0;
gpd_data->nb.notifier_call = genpd_dev_pm_qos_notifier;
 
spin_lock_irq(>power.lock);
Index: linux-pm/drivers/base/power/domain_governor.c
===
--- linux-pm.orig/drivers/base/power/domain_governor.c
+++ linux-pm/drivers/base/power/domain_governor.c
@@ -14,22 +14,22 @@
 static int dev_update_qos_constraint(struct device *dev, void *data)
 {
s64 *constraint_ns_p = data;
-   s32 constraint_ns = -1;
-
-   if (dev->power.subsys_data && dev->power.subsys_data->domain_data)
-   constraint_ns = dev_gpd_data(dev)->td.effective_constraint_ns;
+   s64 constraint_ns;
 
-   if (constraint_ns < 0) {
-   constraint_ns = dev_pm_qos_read_value(dev);
-   constraint_ns *= NSEC_PER_USEC;
-   }
-   if (constraint_ns == 0)
+   if (!dev->power.subsys_data || !dev->power.subsys_data->domain_data)
return 0;
 
/*
-* constraint_ns cannot be negative here, because the device has been
-* suspended.
+* Only take suspend-time QoS constraints of devices into account,
+* because constraints updated after the device has been suspended are
+* not guaranteed to be taken into account anyway.  In order for them
+* to take effect, the device has to be resumed and suspended again.
 */
+   constraint_ns = dev_gpd_data(dev)->td.effective_constraint_ns;
+   /* 0 means "no constraint" */
+   if (constraint_ns == 0)
+   return 0;
+
if (constraint_ns < *constraint_ns_p || *constraint_ns_p == 0)
*constraint_ns_p = constraint_ns;
 
@@ -76,14 +76,29 @@ static bool default_suspend_ok(struct de
device_for_each_child(dev, _ns,
  dev_update_qos_constraint);
 
-   if (constraint_ns > 0) {
+   if (constraint_ns == 0) {
+   /* "No restriction", so the device is allowed to suspend. */
+   td->effective_constraint_ns = 0;
+   td->cached_suspend_ok = true;
+   } else {
+   /*
+* constraint_ns must be positive here, because the children
+* walked above are all suspended, so effective_constraint_ns
+* cannot be negative for them.
+*/
constraint_ns -= td->suspend_latency_ns +
td->resume_latency_ns;
-   if (constraint_ns == 0)
+   /*
+* effective_constraint_ns is negative already and
+* cached_suspend_ok is false, so if the computed value is not
+* positive, return right away.
+*/
+ 

[RFT][PATCH v2 1/2] PM / domains: Rework governor code to be more consistent

[Rafael J. Wysocki]

From: Rafael J. Wysocki 

The genpd governor currently uses negative PM QoS values to indicate
the "no suspend" condition and 0 as "no restriction", but it doesn't
use them consistently.  Moreover, it tries to refresh QoS values for
already suspended devices in a quite questionable way.

For the above reasons, rework it to be a bit more consistent.

First off, note that dev_pm_qos_read_value() in
dev_update_qos_constraint() and __default_power_down_ok() is
evaluated for devices in suspend.  Moreover, that only happens if the
effective_constraint_ns value for them is negative (meaning "no
suspend").  It is not evaluated in any other cases, so effectively
the QoS values are only updated for devices in suspend that should
not have been suspended in the first place.  In all of the other
cases, the QoS values taken into account are the effective ones from
the time before the device has been suspended, so generally devices
need to be resumed and suspended again for new QoS values to take
effect anyway.  Thus evaluating dev_update_qos_constraint() in
those two places doesn't make sense at all, so drop it.

Second, initialize effective_constraint_ns to 0 ("no constraint")
rather than to (-1) ("no suspend"), which makes more sense in
general and in case effective_constraint_ns is never updated
(the device is in suspend all the time or it is never suspended)
it doesn't affect the device's parent and so on.

Finally, rework default_suspend_ok() to explicitly handle the
"no restriction" special case.

Signed-off-by: Rafael J. Wysocki 
---
 drivers/base/power/domain.c  |2 -
 drivers/base/power/domain_governor.c |   61 +--
 2 files changed, 38 insertions(+), 25 deletions(-)

Index: linux-pm/drivers/base/power/domain.c
===
--- linux-pm.orig/drivers/base/power/domain.c
+++ linux-pm/drivers/base/power/domain.c
@@ -1331,7 +1331,7 @@ static struct generic_pm_domain_data *ge
 
gpd_data->base.dev = dev;
gpd_data->td.constraint_changed = true;
-   gpd_data->td.effective_constraint_ns = -1;
+   gpd_data->td.effective_constraint_ns = 0;
gpd_data->nb.notifier_call = genpd_dev_pm_qos_notifier;
 
spin_lock_irq(>power.lock);
Index: linux-pm/drivers/base/power/domain_governor.c
===
--- linux-pm.orig/drivers/base/power/domain_governor.c
+++ linux-pm/drivers/base/power/domain_governor.c
@@ -14,22 +14,22 @@
 static int dev_update_qos_constraint(struct device *dev, void *data)
 {
s64 *constraint_ns_p = data;
-   s32 constraint_ns = -1;
-
-   if (dev->power.subsys_data && dev->power.subsys_data->domain_data)
-   constraint_ns = dev_gpd_data(dev)->td.effective_constraint_ns;
+   s64 constraint_ns;
 
-   if (constraint_ns < 0) {
-   constraint_ns = dev_pm_qos_read_value(dev);
-   constraint_ns *= NSEC_PER_USEC;
-   }
-   if (constraint_ns == 0)
+   if (!dev->power.subsys_data || !dev->power.subsys_data->domain_data)
return 0;
 
/*
-* constraint_ns cannot be negative here, because the device has been
-* suspended.
+* Only take suspend-time QoS constraints of devices into account,
+* because constraints updated after the device has been suspended are
+* not guaranteed to be taken into account anyway.  In order for them
+* to take effect, the device has to be resumed and suspended again.
 */
+   constraint_ns = dev_gpd_data(dev)->td.effective_constraint_ns;
+   /* 0 means "no constraint" */
+   if (constraint_ns == 0)
+   return 0;
+
if (constraint_ns < *constraint_ns_p || *constraint_ns_p == 0)
*constraint_ns_p = constraint_ns;
 
@@ -76,14 +76,29 @@ static bool default_suspend_ok(struct de
device_for_each_child(dev, _ns,
  dev_update_qos_constraint);
 
-   if (constraint_ns > 0) {
+   if (constraint_ns == 0) {
+   /* "No restriction", so the device is allowed to suspend. */
+   td->effective_constraint_ns = 0;
+   td->cached_suspend_ok = true;
+   } else {
+   /*
+* constraint_ns must be positive here, because the children
+* walked above are all suspended, so effective_constraint_ns
+* cannot be negative for them.
+*/
constraint_ns -= td->suspend_latency_ns +
td->resume_latency_ns;
-   if (constraint_ns == 0)
+   /*
+* effective_constraint_ns is negative already and
+* cached_suspend_ok is false, so if the computed value is not
+* positive, return right away.
+*/
+   if (constraint_ns <= 0)