PM / QoS: Fix device resume latency PM QoS

Commit Message

Rafael J. Wysocki Oct. 20, 2017, 11:27 a.m. UTC

From: Rafael J. Wysocki <[e-mail]>

The special value of 0 for device resume latency PM QoS means
"no restriction", but there are two problems with that.

First, device resume latency PM QoS requests with 0 as the
value are always put in front of requests with positive
values in the priority lists used internally by the PM QoS
framework, causing 0 to be chosen as an effective constraint
value.  However, that 0 is then interpreted as "no restriction"
effectively overriding the other requests with specific
restrictions which is incorrect.

Second, the users of device resume latency PM QoS have no
way to specify that *any* resume latency at all should be
avoided, which is an artificial limitation in general.

To address these issues, modify device resume latency PM QoS to
use S32_MAX as the "no constraint" value and 0 as the "no
latency at all" one and rework its users (the cpuidle menu
governor, the genpd QoS governor and the runtime PM framework)
to follow these changes.

Also add a special "n/a" value to the corresponding user space I/F
to allow user space to indicate that it cannot accept any resume
latencies at all for the given device.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=197323
Signed-off-by: Rafael J. Wysocki <[e-mail]>
Reported-by: Reinette Chatre <[e-mail]>
---
 Documentation/ABI/testing/sysfs-devices-power |    4 +
 drivers/base/cpu.c                            |    3 -
 drivers/base/power/domain_governor.c          |   53 ++++++++++++++------------
 drivers/base/power/qos.c                      |    2 
 drivers/base/power/runtime.c                  |    2 
 drivers/base/power/sysfs.c                    |   25 +++++++++---
 drivers/cpuidle/governors/menu.c              |    4 -
 include/linux/pm_qos.h                        |    5 +-
 8 files changed, 62 insertions(+), 36 deletions(-)

Comments

Reinette Chatre Oct. 20, 2017, 10:04 p.m. UTC | #1

Hi Rafael,

Thank you very much!

With this patch I am now able to dynamically modify the latency requirements via the kernel API.

Below I provide the details of what I tested.

On a four core system I wanted to dynamically constrain two cores from entering deeper C-states.
# grep . /sys/devices/system/cpu/cpuidle/current_*
/sys/devices/system/cpu/cpuidle/current_driver:acpi_idle
/sys/devices/system/cpu/cpuidle/current_governor_ro:menu

Stage1:
On boot I now see (from the cpu online code):
       swapper/0-1     [000] ....     0.346148: dev_pm_qos_add_request: device=cpu0 type=DEV_PM_QOS_RESUME_LATENCY new_value=2147483647
       swapper/0-1     [000] ....     0.346247: pm_qos_update_target: action=ADD_REQ prev_value=2147483647 curr_value=2147483647
       swapper/0-1     [000] ....     0.346519: dev_pm_qos_add_request: device=cpu1 type=DEV_PM_QOS_RESUME_LATENCY new_value=2147483647
       swapper/0-1     [000] ....     0.346593: pm_qos_update_target: action=ADD_REQ prev_value=2147483647 curr_value=2147483647
       swapper/0-1     [000] ....     0.346794: dev_pm_qos_add_request: device=cpu2 type=DEV_PM_QOS_RESUME_LATENCY new_value=2147483647
       swapper/0-1     [000] ....     0.346867: pm_qos_update_target: action=ADD_REQ prev_value=2147483647 curr_value=2147483647
       swapper/0-1     [000] ....     0.347080: dev_pm_qos_add_request: device=cpu3 type=DEV_PM_QOS_RESUME_LATENCY new_value=2147483647
       swapper/0-1     [000] ....     0.347153: pm_qos_update_target: action=ADD_REQ prev_value=2147483647 curr_value=2147483647

At this time turbostat shows me that all four of the cores are in C6 more than 99% of the time.

Stage2:
Starting my code I request a 2us constraint on the resume latency of core #2 and #3 and this goes smoothly:
           runit-700   [002] ....   933.722548: dev_pm_qos_add_request: device=cpu2 type=DEV_PM_QOS_RESUME_LATENCY new_value=2
           runit-700   [002] ....   933.722956: pm_qos_update_target: action=ADD_REQ prev_value=2147483647 curr_value=2
           runit-700   [002] ....   933.723161: dev_pm_qos_add_request: device=cpu3 type=DEV_PM_QOS_RESUME_LATENCY new_value=2
           runit-700   [002] ....   933.723407: pm_qos_update_target: action=ADD_REQ prev_value=2147483647 curr_value=2

Running turbostat in parallel I can immediately observe that the cores are indeed not entering deeper C-states (they stay in C1). I also tried it by requesting a 0us constraint where turbostat showed that only C1 is entered minimally (0.09%).

Stage3:
Removing the constraint went just as smoothly, previous "no constraint" was restored and turbostat shows we are spending most time in C6 again.
           rmdir-757   [002] ....  1050.146021: dev_pm_qos_remove_request: device=cpu3 type=DEV_PM_QOS_RESUME_LATENCY new_value=-1
           rmdir-757   [002] ....  1050.146383: pm_qos_update_target: action=REMOVE_REQ prev_value=2 curr_value=2147483647
           rmdir-757   [002] ....  1050.146512: dev_pm_qos_remove_request: device=cpu2 type=DEV_PM_QOS_RESUME_LATENCY new_value=-1
           rmdir-757   [002] ....  1050.146812: pm_qos_update_target: action=REMOVE_REQ prev_value=2 curr_value=2147483647

Through all three stages every core kept reporting a resume_latency requirement of zero. Though not blocking anything from my side it is still of concern to me that there is no way to query the effective constraint from user space. 
# grep . /sys/devices/system/cpu/cpu?/power/pm_qos_resume_latency_us
/sys/devices/system/cpu/cpu0/power/pm_qos_resume_latency_us:0
/sys/devices/system/cpu/cpu1/power/pm_qos_resume_latency_us:0
/sys/devices/system/cpu/cpu2/power/pm_qos_resume_latency_us:0
/sys/devices/system/cpu/cpu3/power/pm_qos_resume_latency_us:0

I am keeping this patch in my local repo to continue developing against it. 

Thank you very much.

Tested-by: Reinette Chatre <[e-mail]>

Reinette

On 10/20/2017 4:27 AM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[e-mail]>
> 
> The special value of 0 for device resume latency PM QoS means
> "no restriction", but there are two problems with that.
> 
> First, device resume latency PM QoS requests with 0 as the
> value are always put in front of requests with positive
> values in the priority lists used internally by the PM QoS
> framework, causing 0 to be chosen as an effective constraint
> value.  However, that 0 is then interpreted as "no restriction"
> effectively overriding the other requests with specific
> restrictions which is incorrect.
> 
> Second, the users of device resume latency PM QoS have no
> way to specify that *any* resume latency at all should be
> avoided, which is an artificial limitation in general.
> 
> To address these issues, modify device resume latency PM QoS to
> use S32_MAX as the "no constraint" value and 0 as the "no
> latency at all" one and rework its users (the cpuidle menu
> governor, the genpd QoS governor and the runtime PM framework)
> to follow these changes.
> 
> Also add a special "n/a" value to the corresponding user space I/F
> to allow user space to indicate that it cannot accept any resume
> latencies at all for the given device.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=197323
> Signed-off-by: Rafael J. Wysocki <[e-mail]>
> Reported-by: Reinette Chatre <[e-mail]>
> ---
>  Documentation/ABI/testing/sysfs-devices-power |    4 +
>  drivers/base/cpu.c                            |    3 -
>  drivers/base/power/domain_governor.c          |   53 ++++++++++++++------------
>  drivers/base/power/qos.c                      |    2 
>  drivers/base/power/runtime.c                  |    2 
>  drivers/base/power/sysfs.c                    |   25 +++++++++---
>  drivers/cpuidle/governors/menu.c              |    4 -
>  include/linux/pm_qos.h                        |    5 +-
>  8 files changed, 62 insertions(+), 36 deletions(-)
> 
> Index: linux-pm/drivers/base/power/sysfs.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/sysfs.c
> +++ linux-pm/drivers/base/power/sysfs.c
> @@ -218,7 +218,14 @@ static ssize_t pm_qos_resume_latency_sho
>  					  struct device_attribute *attr,
>  					  char *buf)
>  {
> -	return sprintf(buf, "%d\n", dev_pm_qos_requested_resume_latency(dev));
> +	s32 value = dev_pm_qos_requested_resume_latency(dev);
> +
> +	if (value == 0)
> +		return sprintf(buf, "n/a\n");
> +	else if (value == PM_QOS_RESUME_LATENCY_NO_CONSTRAINT)
> +		value = 0;
> +
> +	return sprintf(buf, "%d\n", value);
>  }
>  
>  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")) {
> +		value = 0;
> +	}
>  
>  	ret = dev_pm_qos_update_request(dev->power.qos->resume_latency_req,
>  					value);
> Index: linux-pm/include/linux/pm_qos.h
> ===================================================================
> --- linux-pm.orig/include/linux/pm_qos.h
> +++ linux-pm/include/linux/pm_qos.h
> @@ -27,16 +27,17 @@ enum pm_qos_flags_status {
>  	PM_QOS_FLAGS_ALL,
>  };
>  
> -#define PM_QOS_DEFAULT_VALUE -1
> +#define PM_QOS_DEFAULT_VALUE	(-1)
> +#define PM_QOS_LATENCY_ANY	S32_MAX
>  
>  #define PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE	(2000 * USEC_PER_SEC)
>  #define PM_QOS_NETWORK_LAT_DEFAULT_VALUE	(2000 * USEC_PER_SEC)
>  #define PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE	0
>  #define PM_QOS_MEMORY_BANDWIDTH_DEFAULT_VALUE	0
>  #define PM_QOS_RESUME_LATENCY_DEFAULT_VALUE	0
> +#define PM_QOS_RESUME_LATENCY_NO_CONSTRAINT	PM_QOS_LATENCY_ANY
>  #define PM_QOS_LATENCY_TOLERANCE_DEFAULT_VALUE	0
>  #define PM_QOS_LATENCY_TOLERANCE_NO_CONSTRAINT	(-1)
> -#define PM_QOS_LATENCY_ANY			((s32)(~(__u32)0 >> 1))
>  
>  #define PM_QOS_FLAG_NO_POWER_OFF	(1 << 0)
>  
> Index: linux-pm/drivers/cpuidle/governors/menu.c
> ===================================================================
> --- linux-pm.orig/drivers/cpuidle/governors/menu.c
> +++ linux-pm/drivers/cpuidle/governors/menu.c
> @@ -298,8 +298,8 @@ static int menu_select(struct cpuidle_dr
>  		data->needs_update = 0;
>  	}
>  
> -	/* resume_latency is 0 means no restriction */
> -	if (resume_latency && resume_latency < latency_req)
> +	if (resume_latency < latency_req &&
> +	    resume_latency != PM_QOS_RESUME_LATENCY_NO_CONSTRAINT)
>  		latency_req = resume_latency;
>  
>  	/* Special case when user has set very strict latency requirement */
> 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;
> +		}
>  	}
> -	td->effective_constraint_ns = constraint_ns;
> -	td->cached_suspend_ok = constraint_ns >= 0;
>  
>  	/*
>  	 * The children have been suspended already, so we don't need to take
> @@ -145,13 +151,14 @@ static bool __default_power_down_ok(stru
>  		td = &to_gpd_data(pdd)->td;
>  		constraint_ns = td->effective_constraint_ns;
>  		/* default_suspend_ok() need not be called before us. */
> -		if (constraint_ns < 0) {
> +		if (constraint_ns < 0)
>  			constraint_ns = dev_pm_qos_read_value(pdd->dev);
> -			constraint_ns *= NSEC_PER_USEC;
> -		}
> -		if (constraint_ns == 0)
> +
> +		if (constraint_ns == PM_QOS_RESUME_LATENCY_NO_CONSTRAINT)
>  			continue;
>  
> +		constraint_ns *= NSEC_PER_USEC;
> +
>  		/*
>  		 * constraint_ns cannot be negative here, because the device has
>  		 * been suspended.
> Index: linux-pm/drivers/base/power/runtime.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/runtime.c
> +++ linux-pm/drivers/base/power/runtime.c
> @@ -253,7 +253,7 @@ static int rpm_check_suspend_allowed(str
>  	    || (dev->power.request_pending
>  			&& dev->power.request == RPM_REQ_RESUME))
>  		retval = -EAGAIN;
> -	else if (__dev_pm_qos_read_value(dev) < 0)
> +	else if (__dev_pm_qos_read_value(dev) == 0)
>  		retval = -EPERM;
>  	else if (dev->power.runtime_status == RPM_SUSPENDED)
>  		retval = 1;
> Index: linux-pm/drivers/base/cpu.c
> ===================================================================
> --- linux-pm.orig/drivers/base/cpu.c
> +++ linux-pm/drivers/base/cpu.c
> @@ -377,7 +377,8 @@ int register_cpu(struct cpu *cpu, int nu
>  
>  	per_cpu(cpu_sys_devices, num) = &cpu->dev;
>  	register_cpu_under_node(num, cpu_to_node(num));
> -	dev_pm_qos_expose_latency_limit(&cpu->dev, 0);
> +	dev_pm_qos_expose_latency_limit(&cpu->dev,
> +					PM_QOS_RESUME_LATENCY_NO_CONSTRAINT);
>  
>  	return 0;
>  }
> Index: linux-pm/drivers/base/power/qos.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/qos.c
> +++ linux-pm/drivers/base/power/qos.c
> @@ -189,7 +189,7 @@ static int dev_pm_qos_constraints_alloca
>  	plist_head_init(&c->list);
>  	c->target_value = PM_QOS_RESUME_LATENCY_DEFAULT_VALUE;
>  	c->default_value = PM_QOS_RESUME_LATENCY_DEFAULT_VALUE;
> -	c->no_constraint_value = PM_QOS_RESUME_LATENCY_DEFAULT_VALUE;
> +	c->no_constraint_value = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
>  	c->type = PM_QOS_MIN;
>  	c->notifiers = n;
>  
> Index: linux-pm/Documentation/ABI/testing/sysfs-devices-power
> ===================================================================
> --- linux-pm.orig/Documentation/ABI/testing/sysfs-devices-power
> +++ linux-pm/Documentation/ABI/testing/sysfs-devices-power
> @@ -211,7 +211,9 @@ Description:
>  		device, after it has been suspended at run time, from a resume
>  		request to the moment the device will be ready to process I/O,
>  		in microseconds.  If it is equal to 0, however, this means that
> -		the PM QoS resume latency may be arbitrary.
> +		the PM QoS resume latency may be arbitrary and the special value
> +		"n/a" means that user space cannot accept any resume latency at
> +		all for the given device.
>  
>  		Not all drivers support this attribute.  If it isn't supported,
>  		it is not present.
>

Alex Shi Oct. 23, 2017, 5:12 a.m. UTC | #2

On 10/20/2017 07:27 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[e-mail]>
> 
> The special value of 0 for device resume latency PM QoS means
> "no restriction", but there are two problems with that.
> 
> First, device resume latency PM QoS requests with 0 as the
> value are always put in front of requests with positive
> values in the priority lists used internally by the PM QoS
> framework, causing 0 to be chosen as an effective constraint
> value.  However, that 0 is then interpreted as "no restriction"
> effectively overriding the other requests with specific
> restrictions which is incorrect.
> 
> Second, the users of device resume latency PM QoS have no
> way to specify that *any* resume latency at all should be
> avoided, which is an artificial limitation in general.
> 
> To address these issues, modify device resume latency PM QoS to
> use S32_MAX as the "no constraint" value and 0 as the "no
> latency at all" one and rework its users (the cpuidle menu
> governor, the genpd QoS governor and the runtime PM framework)
> to follow these changes.
> 
> Also add a special "n/a" value to the corresponding user space I/F
> to allow user space to indicate that it cannot accept any resume
> latencies at all for the given device.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=197323
> Signed-off-by: Rafael J. Wysocki <[e-mail]>
> Reported-by: Reinette Chatre <[e-mail]>


Acked-by: Alex Shi <[e-mail]>

Ramesh Thomas Oct. 24, 2017, 5:54 a.m. UTC | #3

On 2017-10-20 at 13:27:34 +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[e-mail]>
> 
>  
>  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.

Rafael J. Wysocki Oct. 24, 2017, 8:49 a.m. UTC | #4

On Tuesday, October 24, 2017 7:54:09 AM CEST Ramesh Thomas wrote:
> On 2017-10-20 at 13:27:34 +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[e-mail]>
> > 
> >  
> >  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?

No, because "n/asomething" would then match too.

> > +		value = 0;
> > +	}
> 
> Should there be a check for kstrtos32 failure and return -EINVAL?

No, but there should be an "else" branch here.

> >  
> >  	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.

Yes, it is.

> I think at dev_update_qos_constraint, this can cause to skip call to
> dev_pm_qos_read_value. 

I need to double check that.

Thanks,
Rafael

Rafael J. Wysocki Oct. 24, 2017, 11:23 a.m. UTC | #5

On Tue, Oct 24, 2017 at 10:49 AM, Rafael J. Wysocki <[e-mail]> wrote:
> On Tuesday, October 24, 2017 7:54:09 AM CEST Ramesh Thomas wrote:
>> On 2017-10-20 at 13:27:34 +0200, Rafael J. Wysocki wrote:
>> > From: Rafael J. Wysocki <[e-mail]>
>> >

[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;
>>
>> Previously effective_constraint_ns was left as -1 if constraint_ns becomes 0
>> Not sure if this change is intentional.
>
> Yes, it is.
>
>> I think at dev_update_qos_constraint, this can cause to skip call to
>> dev_pm_qos_read_value.
>
> I need to double check that.

If constraint_ns becomes 0 (or less) here, power cannot be removed
from the device, because it would add an unacceptable latency.

Thus effective_constraint_ns has to be 0 for it to indicate that
situation.  If it was left at -1, it would mean "no requirement", but
that wouldn't be correct.

Thanks,
Rafael

Ramesh Thomas Oct. 25, 2017, 7:27 a.m. UTC | #6

On 2017-10-24 at 13:23:23 +0200, Rafael J. Wysocki wrote:
> On Tue, Oct 24, 2017 at 10:49 AM, Rafael J. Wysocki <[e-mail]> wrote:
> > On Tuesday, October 24, 2017 7:54:09 AM CEST Ramesh Thomas wrote:
> >> On 2017-10-20 at 13:27:34 +0200, Rafael J. Wysocki wrote:
> >> > From: Rafael J. Wysocki <[e-mail]>
> >> >
> 
> [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;
> >>
> >> Previously effective_constraint_ns was left as -1 if constraint_ns becomes 0
> >> Not sure if this change is intentional.
> >
> > Yes, it is.
> >
> >> I think at dev_update_qos_constraint, this can cause to skip call to
> >> dev_pm_qos_read_value.
> >
> > I need to double check that.
> 
> If constraint_ns becomes 0 (or less) here, power cannot be removed
> from the device, because it would add an unacceptable latency.
> 
> Thus effective_constraint_ns has to be 0 for it to indicate that
> situation.  If it was left at -1, it would mean "no requirement", but
> that wouldn't be correct.
> 

A negative value in effective_constraint_ns is used as trigger to read new
resume latency constraints.

Here the parent of this device will not use this value when this functions
returns false for this device as that means it is not going to suspend. Only
other function that will use it is __default_power_down_ok. That function also
needs to consider a changed new resume latency constraints which it won't if
this does not have a negative value or if __default_power_down_ok does not
itself initialize it to -1 before starting its calculation like
default_suspend_ok does.

Thanks,
Ramesh

Rafael J. Wysocki Oct. 25, 2017, 4:28 p.m. UTC | #7

On Wed, Oct 25, 2017 at 9:27 AM, Ramesh Thomas <[e-mail]> wrote:
> On 2017-10-24 at 13:23:23 +0200, Rafael J. Wysocki wrote:
>> On Tue, Oct 24, 2017 at 10:49 AM, Rafael J. Wysocki <[e-mail]> wrote:
>> > On Tuesday, October 24, 2017 7:54:09 AM CEST Ramesh Thomas wrote:
>> >> On 2017-10-20 at 13:27:34 +0200, Rafael J. Wysocki wrote:
>> >> > From: Rafael J. Wysocki <[e-mail]>
>> >> >
>>
>> [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;
>> >>
>> >> Previously effective_constraint_ns was left as -1 if constraint_ns becomes 0
>> >> Not sure if this change is intentional.
>> >
>> > Yes, it is.
>> >
>> >> I think at dev_update_qos_constraint, this can cause to skip call to
>> >> dev_pm_qos_read_value.
>> >
>> > I need to double check that.
>>
>> If constraint_ns becomes 0 (or less) here, power cannot be removed
>> from the device, because it would add an unacceptable latency.
>>
>> Thus effective_constraint_ns has to be 0 for it to indicate that
>> situation.  If it was left at -1, it would mean "no requirement", but
>> that wouldn't be correct.
>>
>
> A negative value in effective_constraint_ns is used as trigger to read new
> resume latency constraints.

I guess you mean in __default_power_down_ok(), right?

That doesn't matter, because it covers the case when the device has
never been runtime-suspended: it started in the "suspended" state and
has never been made "active".

The case we are talking about is when default_suspend_ok() *was* run
and it returned "true", or the device would not have been suspended,
so __default_power_down_ok() would not have run for that domain at
all.  In that case effective_constraint has to be positive anyway,
because that is the only case when default_suspend_ok() returns
"true".

It matters in default_suspend_ok() itself, however, where the
constraints for the children are checked and -1 means "no
restriction".  So it still looks like the patch needs to be improved,
but that's because effective_constraint should not remain -1 if
constraint_ns is 0 (which it still does in one case).

Thanks,
Rafael

Andy Shevchenko Oct. 25, 2017, 8:06 p.m. UTC | #8

On Tue, Oct 24, 2017 at 11:49 AM, Rafael J. Wysocki <[e-mail]> wrote:
> On Tuesday, October 24, 2017 7:54:09 AM CEST Ramesh Thomas wrote:
>> On 2017-10-20 at 13:27:34 +0200, Rafael J. Wysocki wrote:

>> >  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)) {
>> > +           /*
>> > +            * 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)
>> > +                   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?
>
> No, because "n/asomething" would then match too.

If I don't missed anything, kernfs is aware of \n which means the
first check is enough.
Am I correct?

Ramesh Thomas Oct. 26, 2017, 1:41 a.m. UTC | #9

On 2017-10-25 at 18:28:01 +0200, Rafael J. Wysocki wrote:
> On Wed, Oct 25, 2017 at 9:27 AM, Ramesh Thomas <[e-mail]> wrote:
> > On 2017-10-24 at 13:23:23 +0200, Rafael J. Wysocki wrote:
> >> On Tue, Oct 24, 2017 at 10:49 AM, Rafael J. Wysocki <[e-mail]> wrote:
> >> > On Tuesday, October 24, 2017 7:54:09 AM CEST Ramesh Thomas wrote:
> >> >> On 2017-10-20 at 13:27:34 +0200, Rafael J. Wysocki wrote:
> >> >> > From: Rafael J. Wysocki <[e-mail]>
> >> >> >
> >>
> >> [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;
> >> >>
> >> >> Previously effective_constraint_ns was left as -1 if constraint_ns becomes 0
> >> >> Not sure if this change is intentional.
> >> >
> >> > Yes, it is.
> >> >
> >> >> I think at dev_update_qos_constraint, this can cause to skip call to
> >> >> dev_pm_qos_read_value.
> >> >
> >> > I need to double check that.
> >>
> >> If constraint_ns becomes 0 (or less) here, power cannot be removed
> >> from the device, because it would add an unacceptable latency.
> >>
> >> Thus effective_constraint_ns has to be 0 for it to indicate that
> >> situation.  If it was left at -1, it would mean "no requirement", but
> >> that wouldn't be correct.
> >>
> >
> > A negative value in effective_constraint_ns is used as trigger to read new
> > resume latency constraints.
> 
> I guess you mean in __default_power_down_ok(), right?

Yes.

> 
> That doesn't matter, because it covers the case when the device has
> never been runtime-suspended: it started in the "suspended" state and
> has never been made "active".
> 
> The case we are talking about is when default_suspend_ok() *was* run
> and it returned "true", or the device would not have been suspended,
> so __default_power_down_ok() would not have run for that domain at
> all.  In that case effective_constraint has to be positive anyway,
> because that is the only case when default_suspend_ok() returns
> "true".
> 
> It matters in default_suspend_ok() itself, however, where the
> constraints for the children are checked and -1 means "no
> restriction".  So it still looks like the patch needs to be improved,
> but that's because effective_constraint should not remain -1 if
> constraint_ns is 0 (which it still does in one case).

If you are referring to the place where it exits when constraint_ns == 0,
then I think it should be ok because it returns false there. Unless I
am missing something, the device would not suspend and neither the parent
nor __default_power_down_ok() would be referring to that value in that case.

Thanks,
Ramesh

Rafael J. Wysocki Oct. 26, 2017, 8:38 a.m. UTC | #10

On Wed, Oct 25, 2017 at 10:06 PM, Andy Shevchenko
<[e-mail]> wrote:
> On Tue, Oct 24, 2017 at 11:49 AM, Rafael J. Wysocki <[e-mail]> wrote:
>> On Tuesday, October 24, 2017 7:54:09 AM CEST Ramesh Thomas wrote:
>>> On 2017-10-20 at 13:27:34 +0200, Rafael J. Wysocki wrote:
>
>>> >  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)) {
>>> > +           /*
>>> > +            * 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)
>>> > +                   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?
>>
>> No, because "n/asomething" would then match too.
>
> If I don't missed anything, kernfs is aware of \n which means the
> first check is enough.
> Am I correct?

I'm not sure, honestly. :-)

Anyway, that can be fixed up later and the bug in question is rather urgent.

Thanks,
Rafael

Andy Shevchenko Oct. 27, 2017, 6:52 p.m. UTC | #11

On Thu, Oct 26, 2017 at 11:38 AM, Rafael J. Wysocki <[e-mail]> wrote:
> On Wed, Oct 25, 2017 at 10:06 PM, Andy Shevchenko
> <[e-mail]> wrote:
>> On Tue, Oct 24, 2017 at 11:49 AM, Rafael J. Wysocki <[e-mail]> wrote:
>>> On Tuesday, October 24, 2017 7:54:09 AM CEST Ramesh Thomas wrote:
>>>> On 2017-10-20 at 13:27:34 +0200, Rafael J. Wysocki wrote:
>>
>>>> >  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)) {
>>>> > +           /*
>>>> > +            * 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)
>>>> > +                   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?
>>>
>>> No, because "n/asomething" would then match too.
>>
>> If I don't missed anything, kernfs is aware of \n which means the
>> first check is enough.
>> Am I correct?
>
> I'm not sure, honestly. :-)

Okay, just a summary:
1. kernfs guarantees that buffer is NULL terminated
2. sysfs guarantees that the buffer is not empty
3. kstrto* are aware of '\n'
4. sysfs_streq() and __sysfs_match_string() are aware of '\n'

Thus, we just may use sysfs_streq() for that.

I will prepare a clean up patch on top of this fix if you are okay with it.

> Anyway, that can be fixed up later and the bug in question is rather urgent.

Sure.

Rafael J. Wysocki Oct. 27, 2017, 7:16 p.m. UTC | #12

On Fri, Oct 27, 2017 at 8:52 PM, Andy Shevchenko
<[e-mail]> wrote:
> On Thu, Oct 26, 2017 at 11:38 AM, Rafael J. Wysocki <[e-mail]> wrote:
>> On Wed, Oct 25, 2017 at 10:06 PM, Andy Shevchenko
>> <[e-mail]> wrote:
>>> On Tue, Oct 24, 2017 at 11:49 AM, Rafael J. Wysocki <[e-mail]> wrote:
>>>> On Tuesday, October 24, 2017 7:54:09 AM CEST Ramesh Thomas wrote:
>>>>> On 2017-10-20 at 13:27:34 +0200, Rafael J. Wysocki wrote:
>>>
>>>>> >  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)) {
>>>>> > +           /*
>>>>> > +            * 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)
>>>>> > +                   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?
>>>>
>>>> No, because "n/asomething" would then match too.
>>>
>>> If I don't missed anything, kernfs is aware of \n which means the
>>> first check is enough.
>>> Am I correct?
>>
>> I'm not sure, honestly. :-)
>
> Okay, just a summary:
> 1. kernfs guarantees that buffer is NULL terminated
> 2. sysfs guarantees that the buffer is not empty
> 3. kstrto* are aware of '\n'
> 4. sysfs_streq() and __sysfs_match_string() are aware of '\n'
>
> Thus, we just may use sysfs_streq() for that.
>
> I will prepare a clean up patch on top of this fix if you are okay with it.

OK, but then please cover all similar cases in this file.

Thanks,
Rafael

Patch

Index: linux-pm/drivers/base/power/sysfs.c
===================================================================
--- linux-pm.orig/drivers/base/power/sysfs.c
+++ linux-pm/drivers/base/power/sysfs.c
@@ -218,7 +218,14 @@  static ssize_t pm_qos_resume_latency_sho
 					  struct device_attribute *attr,
 					  char *buf)
 {
-	return sprintf(buf, "%d\n", dev_pm_qos_requested_resume_latency(dev));
+	s32 value = dev_pm_qos_requested_resume_latency(dev);
+
+	if (value == 0)
+		return sprintf(buf, "n/a\n");
+	else if (value == PM_QOS_RESUME_LATENCY_NO_CONSTRAINT)
+		value = 0;
+
+	return sprintf(buf, "%d\n", value);
 }
 
 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")) {
+		value = 0;
+	}
 
 	ret = dev_pm_qos_update_request(dev->power.qos->resume_latency_req,
 					value);
Index: linux-pm/include/linux/pm_qos.h
===================================================================
--- linux-pm.orig/include/linux/pm_qos.h
+++ linux-pm/include/linux/pm_qos.h
@@ -27,16 +27,17 @@  enum pm_qos_flags_status {
 	PM_QOS_FLAGS_ALL,
 };
 
-#define PM_QOS_DEFAULT_VALUE -1
+#define PM_QOS_DEFAULT_VALUE	(-1)
+#define PM_QOS_LATENCY_ANY	S32_MAX
 
 #define PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE	(2000 * USEC_PER_SEC)
 #define PM_QOS_NETWORK_LAT_DEFAULT_VALUE	(2000 * USEC_PER_SEC)
 #define PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE	0
 #define PM_QOS_MEMORY_BANDWIDTH_DEFAULT_VALUE	0
 #define PM_QOS_RESUME_LATENCY_DEFAULT_VALUE	0
+#define PM_QOS_RESUME_LATENCY_NO_CONSTRAINT	PM_QOS_LATENCY_ANY
 #define PM_QOS_LATENCY_TOLERANCE_DEFAULT_VALUE	0
 #define PM_QOS_LATENCY_TOLERANCE_NO_CONSTRAINT	(-1)
-#define PM_QOS_LATENCY_ANY			((s32)(~(__u32)0 >> 1))
 
 #define PM_QOS_FLAG_NO_POWER_OFF	(1 << 0)
 
Index: linux-pm/drivers/cpuidle/governors/menu.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/governors/menu.c
+++ linux-pm/drivers/cpuidle/governors/menu.c
@@ -298,8 +298,8 @@  static int menu_select(struct cpuidle_dr
 		data->needs_update = 0;
 	}
 
-	/* resume_latency is 0 means no restriction */
-	if (resume_latency && resume_latency < latency_req)
+	if (resume_latency < latency_req &&
+	    resume_latency != PM_QOS_RESUME_LATENCY_NO_CONSTRAINT)
 		latency_req = resume_latency;
 
 	/* Special case when user has set very strict latency requirement */
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;
+		}
 	}
-	td->effective_constraint_ns = constraint_ns;
-	td->cached_suspend_ok = constraint_ns >= 0;
 
 	/*
 	 * The children have been suspended already, so we don't need to take
@@ -145,13 +151,14 @@  static bool __default_power_down_ok(stru
 		td = &to_gpd_data(pdd)->td;
 		constraint_ns = td->effective_constraint_ns;
 		/* default_suspend_ok() need not be called before us. */
-		if (constraint_ns < 0) {
+		if (constraint_ns < 0)
 			constraint_ns = dev_pm_qos_read_value(pdd->dev);
-			constraint_ns *= NSEC_PER_USEC;
-		}
-		if (constraint_ns == 0)
+
+		if (constraint_ns == PM_QOS_RESUME_LATENCY_NO_CONSTRAINT)
 			continue;
 
+		constraint_ns *= NSEC_PER_USEC;
+
 		/*
 		 * constraint_ns cannot be negative here, because the device has
 		 * been suspended.
Index: linux-pm/drivers/base/power/runtime.c
===================================================================
--- linux-pm.orig/drivers/base/power/runtime.c
+++ linux-pm/drivers/base/power/runtime.c
@@ -253,7 +253,7 @@  static int rpm_check_suspend_allowed(str
 	    || (dev->power.request_pending
 			&& dev->power.request == RPM_REQ_RESUME))
 		retval = -EAGAIN;
-	else if (__dev_pm_qos_read_value(dev) < 0)
+	else if (__dev_pm_qos_read_value(dev) == 0)
 		retval = -EPERM;
 	else if (dev->power.runtime_status == RPM_SUSPENDED)
 		retval = 1;
Index: linux-pm/drivers/base/cpu.c
===================================================================
--- linux-pm.orig/drivers/base/cpu.c
+++ linux-pm/drivers/base/cpu.c
@@ -377,7 +377,8 @@  int register_cpu(struct cpu *cpu, int nu
 
 	per_cpu(cpu_sys_devices, num) = &cpu->dev;
 	register_cpu_under_node(num, cpu_to_node(num));
-	dev_pm_qos_expose_latency_limit(&cpu->dev, 0);
+	dev_pm_qos_expose_latency_limit(&cpu->dev,
+					PM_QOS_RESUME_LATENCY_NO_CONSTRAINT);
 
 	return 0;
 }
Index: linux-pm/drivers/base/power/qos.c
===================================================================
--- linux-pm.orig/drivers/base/power/qos.c
+++ linux-pm/drivers/base/power/qos.c
@@ -189,7 +189,7 @@  static int dev_pm_qos_constraints_alloca
 	plist_head_init(&c->list);
 	c->target_value = PM_QOS_RESUME_LATENCY_DEFAULT_VALUE;
 	c->default_value = PM_QOS_RESUME_LATENCY_DEFAULT_VALUE;
-	c->no_constraint_value = PM_QOS_RESUME_LATENCY_DEFAULT_VALUE;
+	c->no_constraint_value = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
 	c->type = PM_QOS_MIN;
 	c->notifiers = n;
 
Index: linux-pm/Documentation/ABI/testing/sysfs-devices-power
===================================================================
--- linux-pm.orig/Documentation/ABI/testing/sysfs-devices-power
+++ linux-pm/Documentation/ABI/testing/sysfs-devices-power
@@ -211,7 +211,9 @@  Description:
 		device, after it has been suspended at run time, from a resume
 		request to the moment the device will be ready to process I/O,
 		in microseconds.  If it is equal to 0, however, this means that
-		the PM QoS resume latency may be arbitrary.
+		the PM QoS resume latency may be arbitrary and the special value
+		"n/a" means that user space cannot accept any resume latency at
+		all for the given device.
 
 		Not all drivers support this attribute.  If it isn't supported,
 		it is not present.