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