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