On 2017-10-20 at 13:27:34 +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wyso...@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.
