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

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

On 2017-10-20 at 13:27:34 +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
>  
>  static ssize_t pm_qos_resume_latency_store(struct device *dev,
> @@ -228,11 +235,19 @@ static ssize_t pm_qos_resume_latency_sto
>  	s32 value;
>  	int ret;
>  
> -	if (kstrtos32(buf, 0, &value))
> -		return -EINVAL;
> +	if (!kstrtos32(buf, 0, &value)) {
> +		/*
> +		 * Prevent users from writing negative or "no constraint" values
> +		 * directly.
> +		 */
> +		if (value < 0 || value == PM_QOS_RESUME_LATENCY_NO_CONSTRAINT)
> +			return -EINVAL;
>  
> -	if (value < 0)
> -		return -EINVAL;
> +		if (value == 0)
> +			value = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
> +	} else if (!strcmp(buf, "n/a") || !strcmp(buf, "n/a\n")) {

Can the 2 checks for "n/a" be combined by checking first 3 characters?

> +		value = 0;
> +	}

Should there be a check for kstrtos32 failure and return -EINVAL?

>  
>  	ret = dev_pm_qos_update_request(dev->power.qos->resume_latency_req,
>  					value);


> 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,23 +14,20 @@
>  static int dev_update_qos_constraint(struct device *dev, void *data)
>  {
>  	s64 *constraint_ns_p = data;
> -	s32 constraint_ns = -1;
> +	s64 constraint_ns = -1;
>  
>  	if (dev->power.subsys_data && dev->power.subsys_data->domain_data)
>  		constraint_ns = dev_gpd_data(dev)->td.effective_constraint_ns;
>  
> -	if (constraint_ns < 0) {
> +	if (constraint_ns < 0)
>  		constraint_ns = dev_pm_qos_read_value(dev);
> -		constraint_ns *= NSEC_PER_USEC;
> -	}
> -	if (constraint_ns == 0)
> +
> +	if (constraint_ns == PM_QOS_RESUME_LATENCY_NO_CONSTRAINT)
>  		return 0;
>  
> -	/*
> -	 * constraint_ns cannot be negative here, because the device has been
> -	 * suspended.
> -	 */
> -	if (constraint_ns < *constraint_ns_p || *constraint_ns_p == 0)
> +	constraint_ns *= NSEC_PER_USEC;
> +
> +	if (constraint_ns < *constraint_ns_p || *constraint_ns_p < 0)
>  		*constraint_ns_p = constraint_ns;
>  
>  	return 0;
> @@ -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;

Previously effective_constraint_ns was left as -1 if constraint_ns becomes 0
Not sure if this change is intentional. I think at dev_update_qos_constraint,
this can cause to skip call to dev_pm_qos_read_value.