Re: [PATCH v2] PM / QoS: Fix device resume latency PM QoS

[Ramesh Thomas <ramesh.thomas@intel.com>]

On 2017-10-25 at 00:16:25 -0700, Ramesh Thomas wrote:
> On 2017-10-24 at 13:35:05 +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> 
> [cut]
> 
> > @@ -63,10 +60,14 @@ static bool default_suspend_ok(struct de
> >  
> >  	spin_unlock_irqrestore(&dev->power.lock, flags);
> >  
> > -	if (constraint_ns < 0)
> > +	if (constraint_ns == 0)
> >  		return false;
> >  
> > -	constraint_ns *= NSEC_PER_USEC;
> > +	if (constraint_ns == PM_QOS_RESUME_LATENCY_NO_CONSTRAINT)
> > +		constraint_ns = -1;
> > +	else
> > +		constraint_ns *= NSEC_PER_USEC;
> > +
> >  	/*
> >  	 * We can walk the children without any additional locking, because
> >  	 * they all have been suspended at this point and their
> > @@ -76,14 +77,19 @@ static bool default_suspend_ok(struct de
> >  		device_for_each_child(dev, &constraint_ns,
> >  				      dev_update_qos_constraint);
> >  
> > -	if (constraint_ns > 0) {
> > -		constraint_ns -= td->suspend_latency_ns +
> > -				td->resume_latency_ns;
> > -		if (constraint_ns == 0)
> > -			return false;
> > +	if (constraint_ns < 0) {
> > +		/* The children have no constraints. */
> > +		td->effective_constraint_ns = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
> > +		td->cached_suspend_ok = true;
> > +	} else {
> > +		constraint_ns -= td->suspend_latency_ns + td->resume_latency_ns;
> > +		if (constraint_ns > 0) {
> > +			td->effective_constraint_ns = constraint_ns;
> > +			td->cached_suspend_ok = true;
> > +		} else {
> > +			td->effective_constraint_ns = 0;
> 
> If the resume latency constraint was increased after this,
> default_power_down_ok may not consider the new value. default_suspend_ok needs
> to get called first if the new value is to be read.
> 
> This is because dev_pm_qos_read_value will get called only if
> effective_constraint_ns has a negative value. default_suspend_ok initializes
> effective_constraint_ns with -1 before doing the calculations.
> default_power_down_ok does not initialize it to -1 and uses
> the existing value.
> 
> A comment in default_power_down_ok implies it is not necessary to call
> default_suspend_ok before calling default_power_down_ok. In that case,
> default_power_down_ok should be able to get the new latency constraint value.
> 

The design expects default_suspend_ok would always be called before
default_power_down_ok if the device was made "active" after start. Changes
to resume latency constraint will not be considered if it happened between
suspend and power down of a device. However, that is the design and not a
behavior introduced by this patch.

Acked-by: Ramesh Thomas <ramesh.thomas@intel.com>