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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171026020020.GB18264@intel.com>
Date:   Wed, 25 Oct 2017 19:00:20 -0700
From:   Ramesh Thomas <ramesh.thomas@...el.com>
To:     "Rafael J. Wysocki" <rjw@...ysocki.net>
Cc:     Linux PM <linux-pm@...r.kernel.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Reinette Chatre <reinette.chatre@...el.com>,
        Alex Shi <alex.shi@...aro.org>,
        Ulf Hansson <ulf.hansson@...aro.org>
Subject: Re: [PATCH v2] PM / QoS: Fix device resume latency PM QoS

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@...el.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@...el.com>