Re: Patch "PM / QoS: Fix device resume latency PM QoS" has been added to the 4.13-stable tree

["Rafael J. Wysocki" <rafael.j.wysocki@intel.com>]

On 11/5/2017 3:44 PM, gregkh@linuxfoundation.org wrote:
> This is a note to let you know that I've just added the patch titled
>
>      PM / QoS: Fix device resume latency PM QoS
>
> to the 4.13-stable tree which can be found at:
>      http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
>
> The filename of the patch is:
>       pm-qos-fix-device-resume-latency-pm-qos.patch
> and it can be found in the queue-4.13 subdirectory.
>
> If you, or anyone else, feels it should not be added to the stable tree,
> please let <stable@vger.kernel.org> know about it.
>
>
>  From 0cc2b4e5a020fc7f4d1795741c116c983e9467d7 Mon Sep 17 00:00:00 2001
> From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> Date: Tue, 24 Oct 2017 15:20:45 +0200
> Subject: PM / QoS: Fix device resume latency PM QoS
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> commit 0cc2b4e5a020fc7f4d1795741c116c983e9467d7 upstream.
>
> 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.
>
> Fixes: 85dc0b8a4019 (PM / QoS: Make it possible to expose PM QoS latency constraints)
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=197323
> Reported-by: Reinette Chatre <reinette.chatre@intel.com>
> Tested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Acked-by: Alex Shi <alex.shi@linaro.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> ---
>   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, 63 insertions(+), 35 deletions(-)
>
> --- a/Documentation/ABI/testing/sysfs-devices-power
> +++ b/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.
> --- a/drivers/base/cpu.c
> +++ b/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;
>   }
> --- a/drivers/base/power/domain_governor.c
> +++ b/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.
> --- a/drivers/base/power/qos.c
> +++ b/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;
>   
> --- a/drivers/base/power/runtime.c
> +++ b/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;
> --- a/drivers/base/power/sysfs.c
> +++ b/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,21 @@ 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)
> +		if (value == 0)
> +			value = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
> +	} else if (!strcmp(buf, "n/a") || !strcmp(buf, "n/a\n")) {
> +		value = 0;
> +	} else {
>   		return -EINVAL;
> +	}
>   
>   	ret = dev_pm_qos_update_request(dev->power.qos->resume_latency_req,
>   					value);
> --- a/drivers/cpuidle/governors/menu.c
> +++ b/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 */
> --- a/include/linux/pm_qos.h
> +++ b/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)
>   #define PM_QOS_FLAG_REMOTE_WAKEUP	(1 << 1)
>
>
> Patches currently in stable-queue which might be from rafael.j.wysocki@intel.com are
>
> queue-4.13/pm-qos-fix-default-runtime_pm-device-resume-latency.patch
> queue-4.13/pm-qos-fix-device-resume-latency-pm-qos.patch

Please drop this from the -stable queue, it has been reverted from the 
mainline.

Thanks,

Rafael