On Tue, Nov 07, 2017 at 01:49:13AM +0100, Rafael J. Wysocki wrote: > On 11/5/2017 3:44 PM, gregkh@xxxxxxxxxxxxxxxxxxx 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@xxxxxxxxxxxxxxx> know about it. > > > > > > From 0cc2b4e5a020fc7f4d1795741c116c983e9467d7 Mon Sep 17 00:00:00 2001 > > From: "Rafael J. Wysocki" <rafael.j.wysocki@xxxxxxxxx> > > 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@xxxxxxxxx> > > > > 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@xxxxxxxxx> > > Tested-by: Reinette Chatre <reinette.chatre@xxxxxxxxx> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx> > > Acked-by: Alex Shi <alex.shi@xxxxxxxxxx> > > Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> > > > > --- > > 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@xxxxxxxxx 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. Now dropped, thanks. greg k-h