On 09/12/2013 10:55 AM, Viresh Kumar wrote:

As a rule its better not to break string (quoted inside "") in a print statement even if it crosses 80 column boundary as that may introduce unwanted bugs and so this patch rewrites one of the print statements..

Ok, if that is the convention, then so be it.

Signed-off-by: Viresh Kumar viresh.kumar@linaro.org

Reviewed-by: Srivatsa S. Bhat srivatsa.bhat@linux.vnet.ibm.com

Regards, Srivatsa S. Bhat

---

drivers/cpufreq/cpufreq.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 28477eb..31f7845 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1192,8 +1192,8 @@ static int __cpufreq_remove_dev_prepare(struct device *dev, unlock_policy_rwsem_write(cpu);

	if (!frozen) {

• 		pr_debug("%s: policy Kobject moved to cpu: %d "
  

• 			 "from: %d\n",__func__, new_cpu, cpu);
  

• 		pr_debug("%s: policy Kobject moved to cpu: %d from: %d\n",
  

• 				__func__, new_cpu, cpu);
  }
  

  } }

On 09/12/2013 10:55 AM, Viresh Kumar wrote:

Nobody except cpufreq_remove_dev() is calling __cpufreq_remove_dev() and so we don't need separate routines here. Lets merge code from __cpufreq_remove_dev() to cpufreq_remove_dev() and get rid of __cpufreq_remove_dev().

Signed-off-by: Viresh Kumar viresh.kumar@linaro.org

Reviewed-by: Srivatsa S. Bhat srivatsa.bhat@linux.vnet.ibm.com

Regards, Srivatsa S. Bhat

---

drivers/cpufreq/cpufreq.c | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 31f7845..5e0a82e 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1285,36 +1285,26 @@ static int __cpufreq_remove_dev_finish(struct device *dev, }

/**

• • __cpufreq_remove_dev - remove a CPU device

• • cpufreq_remove_dev - remove a CPU device
  • Removes the cpufreq interface for a CPU device.
  • Caller should already have policy_rwsem in write mode for this CPU.
  • This routine frees the rwsem before returning.
  */

-static inline int __cpufreq_remove_dev(struct device *dev,

• 		       struct subsys_interface *sif,
  

• 		       bool frozen)
  

-{

• int ret;
• ret = __cpufreq_remove_dev_prepare(dev, sif, frozen);
• if (!ret)

• ret = __cpufreq_remove_dev_finish(dev, sif, frozen);
  

• return ret;

-}

static int cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif) { unsigned int cpu = dev->id;

• int retval;

• int ret;

  if (cpu_is_offline(cpu)) return 0;

• retval = __cpufreq_remove_dev(dev, sif, false);
• return retval;

• ret = __cpufreq_remove_dev_prepare(dev, sif, false);
• if (!ret)

• ret = __cpufreq_remove_dev_finish(dev, sif, false);
  

• return ret;

}

static void handle_update(struct work_struct *work)

On 09/12/2013 10:55 AM, Viresh Kumar wrote:

With a recent change the logic here is changed a bit and I just figured out it is something we don't want.

Consider we have four CPUs (0,1,2,3) managed by a policy and policy->cpu is set to 0. Now we are suspending and hence we call __cpufreq_remove_dev_prepare() for cpu 1, 2 & 3..

With the current code we always call cpufreq_nominate_new_policy_cpu() for cpu 1, 2 & 3 whereas we should skipped most of __cpufreq_remove_dev_prepare() routine.

Lets fix it by moving the check for !frozen inside the first if block.

As you noted in the other thread, Rafael already applied my patch[1] which does the same thing. So I guess you'll drop this patch from your series.

[1].http://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/commit/?h=bl...

Regards, Srivatsa S. Bhat

Signed-off-by: Viresh Kumar viresh.kumar@linaro.org

drivers/cpufreq/cpufreq.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 5e0a82e..0e11fcb 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1182,8 +1182,9 @@ static int __cpufreq_remove_dev_prepare(struct device *dev, cpumask_clear_cpu(cpu, policy->cpus); unlock_policy_rwsem_write(cpu);

• if (cpu != policy->cpu && !frozen) {

• sysfs_remove_link(&dev->kobj, "cpufreq");
  

• if (cpu != policy->cpu) {

• if (!frozen)
  

• 	sysfs_remove_link(&dev->kobj, "cpufreq");
  

  } else if (cpus > 1) { new_cpu = cpufreq_nominate_new_policy_cpu(policy, cpu, frozen); if (new_cpu >= 0) {

On 09/12/2013 10:55 AM, Viresh Kumar wrote:

This broke after a recent change "cedb70a cpufreq: Split __cpufreq_remove_dev() into two parts" from Srivatsa..

Consider a scenario where we have two CPUs in a policy (0 & 1) and we are removing cpu 1. On the call to __cpufreq_remove_dev_prepare() we have cleared 1 from policy->cpus and now on a call to __cpufreq_remove_dev_finish() we read cpumask_weight of policy->cpus, which will come as 1 and this code will behave as if we are removing the last cpu from policy :)

Fix it by clearing cpu mask in __cpufreq_remove_dev_finish() instead of __cpufreq_remove_dev_prepare().

Oops! Good catch!

That said, your fix doesn't look correct. See below.

Signed-off-by: Viresh Kumar viresh.kumar@linaro.org

drivers/cpufreq/cpufreq.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 0e11fcb..b556d46 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1175,12 +1175,9 @@ static int __cpufreq_remove_dev_prepare(struct device *dev, policy->governor->name, CPUFREQ_NAME_LEN); #endif

• WARN_ON(lock_policy_rwsem_write(cpu));

• lock_policy_rwsem_read(cpu); cpus = cpumask_weight(policy->cpus);

• if (cpus > 1)

• cpumask_clear_cpu(cpu, policy->cpus);
  

• unlock_policy_rwsem_write(cpu);

• unlock_policy_rwsem_read(cpu);

  if (cpu != policy->cpu) { if (!frozen)

Around here, we call cpufreq_nominate_new_policy_cpu(), and if we haven't cleared the CPU by then, there is a chance that it will nominate the same CPU that we are taking offline. So its important to clear the CPU before that point.

@@ -1222,9 +1219,12 @@ static int __cpufreq_remove_dev_finish(struct device *dev, return -EINVAL; }

• lock_policy_rwsem_read(cpu);

• WARN_ON(lock_policy_rwsem_write(cpu)); cpus = cpumask_weight(policy->cpus);

• unlock_policy_rwsem_read(cpu);

• if (cpus > 1)

• cpumask_clear_cpu(cpu, policy->cpus);
  

• unlock_policy_rwsem_write(cpu);

Perhaps we can retain the above as a read operation, ...

/* If cpu is last user of policy, free policy */ if (cpus == 1) {

... and change this suitably (from 1 to 0 etc..) ? To add to it, it will look more clear as well:

if (cpus == 0) { /* No cpus in policy, so free it */ } else { /* Restart governor */ }

Regards, Srivatsa S. Bhat

On 12 September 2013 12:46, Srivatsa S. Bhat srivatsa.bhat@linux.vnet.ibm.com wrote:

That sounds good! Even the naming is much better, it conveys the intent clearly.

Folded below change in my patch (attached):

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index b556d46..23f5845 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1111,7 +1111,7 @@ static int cpufreq_nominate_new_policy_cpu(struct cpufreq_policy *policy, int ret;

/* first sibling now owns the new sysfs dir */ - cpu_dev = get_cpu_device(cpumask_first(policy->cpus)); + cpu_dev = get_cpu_device(cpumask_any_but(policy->cpus, old_cpu));

/* Don't touch sysfs files during light-weight tear-down */ if (frozen)

-- thanks