cpufreq: trivial cleanups

PATCH In review Viresh Kumar 2 participants 22 messages Jan 2, 2015 – Jan 27, 2015 CPU frequency scaling framework

lore

Share & tools

lore

Viresh Kumar

author maintainer

2015-01-02 07:04 UTC #0 lore raw

cover cpufreq: trivial cleanups

Hi Rafael et al

Along with the big changes I have for cpufreq core simplification, I also accumulated these trivial cleanups. In order to not confuse the much dedicated series on solving the real problems, I am sending these separately.

These are independent of the stats cleanups sent earlier and so it doesn't matter which one you apply first.

Pushed here:
git://git.linaro.org/people/viresh.kumar/linux.git cpufreq/core/trivial-cleanups

@Prarit/Saravana: It would be helpful if you guys can go through this as well :)

Viresh Kumar (17):
cpufreq: remove dangling comment
cpufreq: remove extra parenthesis
cpufreq: don't need line break in show_scaling_cur_freq()
cpufreq: merge 'if' blocks in __cpufreq_remove_dev_prepare()
cpufreq: s/__find_governor/find_governor
cpufreq: No need to check for has_target()
cpufreq: pass policy to cpufreq_out_of_sync
cpufreq: pass policy to __cpufreq_get()
cpufreq: update driver->flags only if we are registering driver
cpufreq: get rid of CONFIG_{HOTPLUG_CPU|SMP} mess
cpufreq: get rid of 'tpolicy' from __cpufreq_add_dev()
cpufreq: use light-weight cpufreq_cpu_get_raw() in __cpufreq_add_dev
cpufreq: limit the scope of l_p_j variables
cpufreq: check cpufreq_policy_list instead of getting policies for all
CPUs
cpufreq: don't check if cpu > nr_cpu_ids
cpufreq: remove check for cpufreq_disabled() from

    cpufreq_cpu_{get|put}()

cpufreq: move some initialization stuff to cpufreq_policy_alloc()

 drivers/cpufreq/cpufreq.c | 129 ++++++++++++++--------------------------------
 1 file changed, 40 insertions(+), 89 deletions(-)

Signature · 1 line

2.2.0

Viresh Kumar

author maintainer

2015-01-02 07:04 UTC #1 lore raw

PATCH 1/17 cpufreq: remove dangling comment

It doesn't make any sense at all and is a leftover of some earlier commit. Remove it.

Signed-off-by Viresh Kumar <[email protected]>

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

drivers/cpufreq/cpufreq.c +0 -4

index 6d97dffeaaf7..891672b913c8 100644
@@ -2000,10 +2000,6 @@ int cpufreq_driver_target(struct cpufreq_policy *policy,
 }
 EXPORT_SYMBOL_GPL(cpufreq_driver_target);
 
-/*
- * when "event" is CPUFREQ_GOV_LIMITS
- */
-
 static int __cpufreq_governor(struct cpufreq_policy *policy,
 					unsigned int event)
 {

Signature · 1 line

2.2.0

Viresh Kumar

author maintainer

2015-01-02 07:05 UTC #2 lore raw

PATCH 2/17 cpufreq: remove extra parenthesis

In reply to Viresh Kumar

We can live without it and so we should.

Signed-off-by Viresh Kumar <[email protected]>

---
 drivers/cpufreq/cpufreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/cpufreq/cpufreq.c +1 -1

index 891672b913c8..8417313943d5 100644
@@ -902,7 +902,7 @@ static int cpufreq_add_dev_interface(struct cpufreq_policy *policy,
 
 	/* set up files for this cpu device */
 	drv_attr = cpufreq_driver->attr;
-	while ((drv_attr) && (*drv_attr)) {
+	while (drv_attr && *drv_attr) {
 		ret = sysfs_create_file(&policy->kobj, &((*drv_attr)->attr));
 		if (ret)
 			return ret;

Signature · 1 line

2.2.0

Viresh Kumar

author maintainer

2015-01-02 07:05 UTC #3 lore raw

PATCH 3/17 cpufreq: don't need line break in show_scaling_cur_freq()

In reply to Viresh Kumar

No need of an unnecessary line break.

Signed-off-by Viresh Kumar <[email protected]>

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

drivers/cpufreq/cpufreq.c +1 -2

index 8417313943d5..4271d68aec41 100644
@@ -513,8 +513,7 @@ show_one(cpuinfo_transition_latency, cpuinfo.transition_latency);
 show_one(scaling_min_freq, min);
 show_one(scaling_max_freq, max);
 
-static ssize_t show_scaling_cur_freq(
-	struct cpufreq_policy *policy, char *buf)
+static ssize_t show_scaling_cur_freq(struct cpufreq_policy *policy, char *buf)
 {
 	ssize_t ret;

Signature · 1 line

2.2.0

Viresh Kumar

author maintainer

2015-01-02 07:05 UTC #4 lore raw

PATCH 4/17 cpufreq: merge 'if' blocks in __cpufreq_remove_dev_prepare()

In reply to Viresh Kumar

There are two 'if' blocks here, checking for !cpufreq_driver->setpolicy and has_target(). Both are actually doing the same thing, merge them.

Signed-off-by Viresh Kumar <[email protected]>

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

drivers/cpufreq/cpufreq.c +1 -2

index 4271d68aec41..4309b075df06 100644
@@ -1364,11 +1364,10 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,
 			pr_err("%s: Failed to stop governor\n", __func__);
 			return ret;
 		}
-	}
 
-	if (!cpufreq_driver->setpolicy)
 		strncpy(per_cpu(cpufreq_cpu_governor, cpu),
 			policy->governor->name, CPUFREQ_NAME_LEN);
+	}
 
 	down_read(&policy->rwsem);
 	cpus = cpumask_weight(policy->cpus);

Signature · 1 line

2.2.0

Viresh Kumar

author maintainer

2015-01-02 07:05 UTC #5 lore raw

PATCH 5/17 cpufreq: s/__find_governor/find_governor

In reply to Viresh Kumar

Remove unnecessary __ present in find_governor's name.

Signed-off-by Viresh Kumar <[email protected]>

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

drivers/cpufreq/cpufreq.c +5 -5

index 4309b075df06..c9ba1d64015c 100644
@@ -432,7 +432,7 @@ static ssize_t store_boost(struct kobject *kobj, struct attribute *attr,
 }
 define_one_global_rw(boost);
 
-static struct cpufreq_governor *__find_governor(const char *str_governor)
+static struct cpufreq_governor *find_governor(const char *str_governor)
 {
 	struct cpufreq_governor *t;

@@ -468,7 +468,7 @@ static int cpufreq_parse_governor(char *str_governor, unsigned int *policy,
 
 		mutex_lock(&cpufreq_governor_mutex);
 
-		t = __find_governor(str_governor);
+		t = find_governor(str_governor);
 
 		if (t == NULL) {
 			int ret;

@@ -478,7 +478,7 @@ static int cpufreq_parse_governor(char *str_governor, unsigned int *policy,
 			mutex_lock(&cpufreq_governor_mutex);
 
 			if (ret == 0)
-				t = __find_governor(str_governor);
+				t = find_governor(str_governor);
 		}
 
 		if (t != NULL) {

@@ -935,7 +935,7 @@ static void cpufreq_init_policy(struct cpufreq_policy *policy)
 	memcpy(&new_policy, policy, sizeof(*policy));
 
 	/* Update governor of new_policy to the governor used before hotplug */
-	gov = __find_governor(per_cpu(cpufreq_cpu_governor, policy->cpu));
+	gov = find_governor(per_cpu(cpufreq_cpu_governor, policy->cpu));
 	if (gov)
 		pr_debug("Restoring governor %s for cpu %d\n",
 				policy->governor->name, policy->cpu);

@@ -2089,7 +2089,7 @@ int cpufreq_register_governor(struct cpufreq_governor *governor)
 
 	governor->initialized = 0;
 	err = -EBUSY;
-	if (__find_governor(governor->name) == NULL) {
+	if (!find_governor(governor->name)) {
 		err = 0;
 		list_add(&governor->governor_list, &cpufreq_governor_list);
 	}

Signature · 1 line

2.2.0

Viresh Kumar

author maintainer

2015-01-02 07:05 UTC #6 lore raw

PATCH 6/17 cpufreq: No need to check for has_target()

In reply to Viresh Kumar

Either we can be setpolicy or target type, nothing else. And so the else part of setpolicy will automatically be of has_target() type. And so we don't need to check it again.

Signed-off-by Viresh Kumar <[email protected]>

---
 drivers/cpufreq/cpufreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/cpufreq/cpufreq.c +1 -1

index c9ba1d64015c..5e60b3259cf5 100644
@@ -463,7 +463,7 @@ static int cpufreq_parse_governor(char *str_governor, unsigned int *policy,
 			*policy = CPUFREQ_POLICY_POWERSAVE;
 			err = 0;
 		}
-	} else if (has_target()) {
+	} else {
 		struct cpufreq_governor *t;
 
 		mutex_lock(&cpufreq_governor_mutex);

Signature · 1 line

2.2.0

Viresh Kumar

author maintainer

2015-01-02 07:05 UTC #7 lore raw

PATCH 7/17 cpufreq: pass policy to cpufreq_out_of_sync

In reply to Viresh Kumar

There is no point finding out the 'policy' again within cpufreq_out_of_sync() when all the callers already have it. Just make them pass policy instead.

Signed-off-by Viresh Kumar <[email protected]>

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

drivers/cpufreq/cpufreq.c +6 ~~-14~~

index 5e60b3259cf5..3e80b97d64b8 100644
@@ -1502,30 +1502,23 @@ static void handle_update(struct work_struct *work)
 /**
  *	cpufreq_out_of_sync - If actual and saved CPU frequency differs, we're
  *	in deep trouble.
- *	@cpu: cpu number
- *	@old_freq: CPU frequency the kernel thinks the CPU runs at
+ *	@policy: policy managing CPUs
  *	@new_freq: CPU frequency the CPU actually runs at
  *
  *	We adjust to current frequency first, and need to clean up later.
  *	So either call to cpufreq_update_policy() or schedule handle_update()).
  */
-static void cpufreq_out_of_sync(unsigned int cpu, unsigned int old_freq,
+static void cpufreq_out_of_sync(struct cpufreq_policy *policy,
 				unsigned int new_freq)
 {
-	struct cpufreq_policy *policy;
 	struct cpufreq_freqs freqs;
-	unsigned long flags;
 
 	pr_debug("Warning: CPU frequency out of sync: cpufreq and timing core thinks of %u, is %u kHz\n",
-		 old_freq, new_freq);
+		 policy->cur, new_freq);
 
-	freqs.old = old_freq;
+	freqs.old = policy->cur;
 	freqs.new = new_freq;
 
-	read_lock_irqsave(&cpufreq_driver_lock, flags);
-	policy = per_cpu(cpufreq_cpu_data, cpu);
-	read_unlock_irqrestore(&cpufreq_driver_lock, flags);
-
 	cpufreq_freq_transition_begin(policy, &freqs);
 	cpufreq_freq_transition_end(policy, &freqs, 0);
 }

@@ -1590,7 +1583,7 @@ static unsigned int __cpufreq_get(unsigned int cpu)
 		/* verify no discrepancy between actual and
 					saved value exists */
 		if (unlikely(ret_freq != policy->cur)) {
-			cpufreq_out_of_sync(cpu, policy->cur, ret_freq);
+			cpufreq_out_of_sync(policy, ret_freq);
 			schedule_work(&policy->update);
 		}
 	}

@@ -2289,8 +2282,7 @@ int cpufreq_update_policy(unsigned int cpu)
 			policy->cur = new_policy.cur;
 		} else {
 			if (policy->cur != new_policy.cur && has_target())
-				cpufreq_out_of_sync(cpu, policy->cur,
-								new_policy.cur);
+				cpufreq_out_of_sync(policy, new_policy.cur);
 		}
 	}

Signature · 1 line

2.2.0

Viresh Kumar

author maintainer

2015-01-02 07:05 UTC #8 lore raw

PATCH 8/17 cpufreq: pass policy to __cpufreq_get()

In reply to Viresh Kumar

There is no point finding out the 'policy' again within __cpufreq_get() when all the callers already have it. Just make them pass policy instead.

Signed-off-by Viresh Kumar <[email protected]>

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

drivers/cpufreq/cpufreq.c +5 -6

index 3e80b97d64b8..35d4734ce856 100644
@@ -62,7 +62,7 @@ static DECLARE_RWSEM(cpufreq_rwsem);
 /* internal prototypes */
 static int __cpufreq_governor(struct cpufreq_policy *policy,
 		unsigned int event);
-static unsigned int __cpufreq_get(unsigned int cpu);
+static unsigned int __cpufreq_get(struct cpufreq_policy *policy);
 static void handle_update(struct work_struct *work);
 
 /**

@@ -562,7 +562,7 @@ store_one(scaling_max_freq, max);
 static ssize_t show_cpuinfo_cur_freq(struct cpufreq_policy *policy,
 					char *buf)
 {
-	unsigned int cur_freq = __cpufreq_get(policy->cpu);
+	unsigned int cur_freq = __cpufreq_get(policy);
 	if (!cur_freq)
 		return sprintf(buf, "<unknown>");
 	return sprintf(buf, "%u\n", cur_freq);

@@ -1568,15 +1568,14 @@ unsigned int cpufreq_quick_get_max(unsigned int cpu)
 }
 EXPORT_SYMBOL(cpufreq_quick_get_max);
 
-static unsigned int __cpufreq_get(unsigned int cpu)
+static unsigned int __cpufreq_get(struct cpufreq_policy *policy)
 {
-	struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
 	unsigned int ret_freq = 0;
 
 	if (!cpufreq_driver->get)
 		return ret_freq;
 
-	ret_freq = cpufreq_driver->get(cpu);
+	ret_freq = cpufreq_driver->get(policy->cpu);
 
 	if (ret_freq && policy->cur &&
 		!(cpufreq_driver->flags & CPUFREQ_CONST_LOOPS)) {

@@ -1604,7 +1603,7 @@ unsigned int cpufreq_get(unsigned int cpu)
 
 	if (policy) {
 		down_read(&policy->rwsem);
-		ret_freq = __cpufreq_get(cpu);
+		ret_freq = __cpufreq_get(policy);
 		up_read(&policy->rwsem);
 
 		cpufreq_cpu_put(policy);

Signature · 1 line

2.2.0

Viresh Kumar

author maintainer

2015-01-02 07:05 UTC #9 lore raw

PATCH 9/17 cpufreq: update driver_data->flags only if we are registering driver

In reply to Viresh Kumar

We should first check if a cpufreq driver is already registered or not before updating driver_data->flags.

Signed-off-by Viresh Kumar <[email protected]>

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

drivers/cpufreq/cpufreq.c +3 -3

index 35d4734ce856..9595107477a6 100644
@@ -2427,9 +2427,6 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
 
 	pr_debug("trying to register driver %s\n", driver_data->name);
 
-	if (driver_data->setpolicy)
-		driver_data->flags |= CPUFREQ_CONST_LOOPS;
-
 	write_lock_irqsave(&cpufreq_driver_lock, flags);
 	if (cpufreq_driver) {
 		write_unlock_irqrestore(&cpufreq_driver_lock, flags);

@@ -2438,6 +2435,9 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
 	cpufreq_driver = driver_data;
 	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
+	if (driver_data->setpolicy)
+		driver_data->flags |= CPUFREQ_CONST_LOOPS;
+
 	if (cpufreq_boost_supported()) {
 		/*
 		 * Check if driver provides function to enable boost -

Signature · 1 line

2.2.0

Viresh Kumar

author maintainer

2015-01-02 07:05 UTC #10 lore raw

PATCH 10/17 cpufreq: get rid of CONFIG_{HOTPLUG_CPU|SMP} mess

In reply to Viresh Kumar

These are messing up more than the benefit they provide. It isn't a lot of code anyway, that we will compile without them.

Kill them.

Signed-off-by Viresh Kumar <[email protected]>

---
 drivers/cpufreq/cpufreq.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

drivers/cpufreq/cpufreq.c +1 ~~-10~~

index 9595107477a6..3893c87739d1 100644
@@ -957,7 +957,6 @@ static void cpufreq_init_policy(struct cpufreq_policy *policy)
 	}
 }
 
-#ifdef CONFIG_HOTPLUG_CPU
 static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy,
 				  unsigned int cpu, struct device *dev)
 {

@@ -995,7 +994,6 @@ static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy,
 
 	return sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
 }
-#endif
 
 static struct cpufreq_policy *cpufreq_policy_restore(unsigned int cpu)
 {

@@ -1100,19 +1098,15 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 {
 	unsigned int j, cpu = dev->id;
 	int ret = -ENOMEM;
-	struct cpufreq_policy *policy;
+	struct cpufreq_policy *policy, *tpolicy;
 	unsigned long flags;
 	bool recover_policy = cpufreq_suspended;
-#ifdef CONFIG_HOTPLUG_CPU
-	struct cpufreq_policy *tpolicy;
-#endif
 
 	if (cpu_is_offline(cpu))
 		return 0;
 
 	pr_debug("adding CPU %u\n", cpu);
 
-#ifdef CONFIG_SMP
 	/* check whether a different CPU already registered this
 	 * CPU because it is in the same boat. */
 	policy = cpufreq_cpu_get(cpu);

@@ -1120,12 +1114,10 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 		cpufreq_cpu_put(policy);
 		return 0;
 	}
-#endif
 
 	if (!down_read_trylock(&cpufreq_rwsem))
 		return 0;
 
-#ifdef CONFIG_HOTPLUG_CPU
 	/* Check if this cpu was hot-unplugged earlier and has siblings */
 	read_lock_irqsave(&cpufreq_driver_lock, flags);
 	list_for_each_entry(tpolicy, &cpufreq_policy_list, policy_list) {

@@ -1137,7 +1129,6 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 		}
 	}
 	read_unlock_irqrestore(&cpufreq_driver_lock, flags);
-#endif
 
 	/*
 	 * Restore the saved policy when doing light-weight init and fall back

Signature · 1 line

2.2.0

Viresh Kumar

author maintainer

2015-01-02 07:05 UTC #11 lore raw

PATCH 11/17 cpufreq: get rid of 'tpolicy' from __cpufreq_add_dev()

In reply to Viresh Kumar

There is no need of this separate variable, use 'policy' instead.

Signed-off-by Viresh Kumar <[email protected]>

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

drivers/cpufreq/cpufreq.c +4 -4

index 3893c87739d1..3061a5823e3c 100644
@@ -1098,7 +1098,7 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 {
 	unsigned int j, cpu = dev->id;
 	int ret = -ENOMEM;
-	struct cpufreq_policy *policy, *tpolicy;
+	struct cpufreq_policy *policy;
 	unsigned long flags;
 	bool recover_policy = cpufreq_suspended;

@@ -1120,10 +1120,10 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 
 	/* Check if this cpu was hot-unplugged earlier and has siblings */
 	read_lock_irqsave(&cpufreq_driver_lock, flags);
-	list_for_each_entry(tpolicy, &cpufreq_policy_list, policy_list) {
-		if (cpumask_test_cpu(cpu, tpolicy->related_cpus)) {
+	list_for_each_entry(policy, &cpufreq_policy_list, policy_list) {
+		if (cpumask_test_cpu(cpu, policy->related_cpus)) {
 			read_unlock_irqrestore(&cpufreq_driver_lock, flags);
-			ret = cpufreq_add_policy_cpu(tpolicy, cpu, dev);
+			ret = cpufreq_add_policy_cpu(policy, cpu, dev);
 			up_read(&cpufreq_rwsem);
 			return ret;
 		}

Signature · 1 line

2.2.0

Viresh Kumar

author maintainer

2015-01-02 07:05 UTC #12 lore raw

PATCH 12/17 cpufreq: use light-weight cpufreq_cpu_get_raw() in __cpufreq_add_dev

In reply to Viresh Kumar

We just need to check if a 'policy' is already present for the cpu we are adding. We don't need to take all the locks and do kobject usage updates. Use the light-weight cpufreq_cpu_get_raw() routine instead.

Signed-off-by Viresh Kumar <[email protected]>

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

drivers/cpufreq/cpufreq.c +2 -4

index 3061a5823e3c..788016cded33 100644
@@ -1109,11 +1109,9 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 
 	/* check whether a different CPU already registered this
 	 * CPU because it is in the same boat. */
-	policy = cpufreq_cpu_get(cpu);
-	if (unlikely(policy)) {
-		cpufreq_cpu_put(policy);
+	policy = cpufreq_cpu_get_raw(cpu);
+	if (unlikely(policy))
 		return 0;
-	}
 
 	if (!down_read_trylock(&cpufreq_rwsem))
 		return 0;

Signature · 1 line

2.2.0

Viresh Kumar

author maintainer

2015-01-02 07:05 UTC #13 lore raw

PATCH 13/17 cpufreq: limit the scope of l_p_j variables

In reply to Viresh Kumar

These variables are just used within adjust_jiffies() and so must be local to it. Also there is no need of a dummy routine for CONFIG_SMP case as we can take care of all that with help of macros in the same routine. It doesn't look that ugly.

Signed-off-by Viresh Kumar <[email protected]>

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

drivers/cpufreq/cpufreq.c +5 ~~-10~~

index 788016cded33..74169ad47546 100644
@@ -249,12 +249,12 @@ EXPORT_SYMBOL_GPL(cpufreq_cpu_put);
  * systems as each CPU might be scaled differently. So, use the arch
  * per-CPU loops_per_jiffy value wherever possible.
  */
-#ifndef CONFIG_SMP
-static unsigned long l_p_j_ref;
-static unsigned int l_p_j_ref_freq;
-
 static void adjust_jiffies(unsigned long val, struct cpufreq_freqs *ci)
 {
+#ifndef CONFIG_SMP
+	static unsigned long l_p_j_ref;
+	static unsigned int l_p_j_ref_freq;
+
 	if (ci->flags & CPUFREQ_CONST_LOOPS)
 		return;

@@ -270,13 +270,8 @@ static void adjust_jiffies(unsigned long val, struct cpufreq_freqs *ci)
 		pr_debug("scaling loops_per_jiffy to %lu for frequency %u kHz\n",
 			 loops_per_jiffy, ci->new);
 	}
-}
-#else
-static inline void adjust_jiffies(unsigned long val, struct cpufreq_freqs *ci)
-{
-	return;
-}
 #endif
+}
 
 static void __cpufreq_notify_transition(struct cpufreq_policy *policy,
 		struct cpufreq_freqs *freqs, unsigned int state)

Signature · 1 line

2.2.0

Viresh Kumar

author maintainer

2015-01-02 07:05 UTC #14 lore raw

PATCH 14/17 cpufreq: check cpufreq_policy_list instead of scanning policies for all CPUs

In reply to Viresh Kumar

CPUFREQ_STICKY flag is set by drivers which don't want to get unregistered even if cpufreq-core isn't able to initialize policy for any CPU.

When this flag isn't set, we try to unregister the driver. To find out which CPUs are registered and which are not, we try to check per_cpu cpufreq_cpu_data for all CPUs. Because we have a list of valid policies available now, we better check if the list is empty or not instead of the 'for' loop. That will be much more efficient.

Signed-off-by Viresh Kumar <[email protected]>

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

drivers/cpufreq/cpufreq.c +5 ~~-16~~

index 74169ad47546..09426671eb79 100644
@@ -2442,23 +2442,12 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
 	if (ret)
 		goto err_boost_unreg;
 
-	if (!(cpufreq_driver->flags & CPUFREQ_STICKY)) {
-		int i;
-		ret = -ENODEV;
-
-		/* check for at least one working CPU */
-		for (i = 0; i < nr_cpu_ids; i++)
-			if (cpu_possible(i) && per_cpu(cpufreq_cpu_data, i)) {
-				ret = 0;
-				break;
-			}
-
+	if (!(cpufreq_driver->flags & CPUFREQ_STICKY) &&
+	    list_empty(&cpufreq_policy_list)) {
 		/* if all ->init() calls failed, unregister */
-		if (ret) {
-			pr_debug("no CPU initialized for driver %s\n",
-				 driver_data->name);
-			goto err_if_unreg;
-		}
+		pr_debug("%s: No CPU initialized for driver %s\n", __func__,
+			 driver_data->name);
+		goto err_if_unreg;
 	}
 
 	register_hotcpu_notifier(&cpufreq_cpu_notifier);

Signature · 1 line

2.2.0

Viresh Kumar

author maintainer

2015-01-02 07:05 UTC #15 lore raw

PATCH 15/17 cpufreq: don't check if cpu > nr_cpu_ids

In reply to Viresh Kumar

I don't know for what kind of callers do we have this check for. The problem is that such checks introduce an unnecessary delay for the good users.

So, I am dropping it for now, let me know if this is a bad change.

Signed-off-by Viresh Kumar <[email protected]>

---
 drivers/cpufreq/cpufreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/cpufreq/cpufreq.c +1 -1

index 09426671eb79..14d637a28dd8 100644
@@ -202,7 +202,7 @@ struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu)
 	struct cpufreq_policy *policy = NULL;
 	unsigned long flags;
 
-	if (cpufreq_disabled() || (cpu >= nr_cpu_ids))
+	if (cpufreq_disabled())
 		return NULL;
 
 	if (!down_read_trylock(&cpufreq_rwsem))

Signature · 1 line

2.2.0

Viresh Kumar

author maintainer

2015-01-02 07:05 UTC #16 lore raw

PATCH 16/17 cpufreq: remove check for cpufreq_disabled() from cpufreq_cpu_{get|put}()

In reply to Viresh Kumar

In most of the cases cpufreq wouldn't be disabled and this adds unnecessary delay for its users. In the case if cpufreq is really disabled, then the per-cpu variable will also return NULL and things will continue working as is. Remove this unnecessary check.

Signed-off-by Viresh Kumar <[email protected]>

---
 drivers/cpufreq/cpufreq.c | 6 ------
 1 file changed, 6 deletions(-)

drivers/cpufreq/cpufreq.c +0 -6

index 14d637a28dd8..62c6a0b8b0d1 100644
@@ -202,9 +202,6 @@ struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu)
 	struct cpufreq_policy *policy = NULL;
 	unsigned long flags;
 
-	if (cpufreq_disabled())
-		return NULL;
-
 	if (!down_read_trylock(&cpufreq_rwsem))
 		return NULL;

@@ -229,9 +226,6 @@ EXPORT_SYMBOL_GPL(cpufreq_cpu_get);
 
 void cpufreq_cpu_put(struct cpufreq_policy *policy)
 {
-	if (cpufreq_disabled())
-		return;
-
 	kobject_put(&policy->kobj);
 	up_read(&cpufreq_rwsem);
 }

Signature · 1 line

2.2.0

Viresh Kumar

author maintainer

2015-01-25 13:24 UTC #17 lore raw

Re: [PATCH 16/17] cpufreq: remove check for cpufreq_disabled() from cpufreq_cpu_{get|put}()

Quoted 25 lines · Viresh Kumar

In most of the cases cpufreq wouldn't be disabled and this adds unnecessary delay for its users. In the case if cpufreq is really disabled, then the per-cpu variable will also return NULL and things will continue working as is. Remove this unnecessary check.
Signed-off-by Viresh Kumar <[email protected]>
---
 drivers/cpufreq/cpufreq.c | 6 ------
 1 file changed, 6 deletions(-)
drivers/cpufreq/cpufreq.c +0 -3
index 14d637a28dd8..62c6a0b8b0d1 100644
@@ -202,9 +202,6 @@ struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu)
        struct cpufreq_policy *policy = NULL;
        unsigned long flags;
 
-       if (cpufreq_disabled())
-               return NULL;
-
        if (!down_read_trylock(&cpufreq_rwsem))
                return NULL;
 

drivers/cpufreq/cpufreq.c EXPORT_SYMBOL_GPL(cpufreq_cpu_get); · line 231

Quoted 9 lines

drivers/cpufreq/cpufreq.c +0 -3

@@ -229,9 +226,6 @@ EXPORT_SYMBOL_GPL(cpufreq_cpu_get);
 
 void cpufreq_cpu_put(struct cpufreq_policy *policy)
 {
-       if (cpufreq_disabled())
-               return;
-
        kobject_put(&policy->kobj);
        up_read(&cpufreq_rwsem);
 }

@Rafael: Are you fine with this patch as is? I meant, you just left it for rebase conflict or because you want it to be part of the WARN() you suggested?

The WARN here might not be a good idea as this can be called by users while cpufreq is disabled. But we don't need this check because of the reasons I mentioned in commit log.

Rafael J. Wysocki

2015-01-26 00:16 UTC #18 lore raw

Re: [PATCH 16/17] cpufreq: remove check for cpufreq_disabled() from cpufreq_cpu_{get|put}()

Quoted 27 lines · Viresh Kumar

Quoted 26 lines · Viresh Kumar
In most of the cases cpufreq wouldn't be disabled and this adds unnecessary delay for its users. In the case if cpufreq is really disabled, then the per-cpu variable will also return NULL and things will continue working as is. Remove this unnecessary check.
Signed-off-by Viresh Kumar <[email protected]>
---
 drivers/cpufreq/cpufreq.c | 6 ------
 1 file changed, 6 deletions(-)
drivers/cpufreq/cpufreq.c +0 -3
index 14d637a28dd8..62c6a0b8b0d1 100644
@@ -202,9 +202,6 @@ struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu)
        struct cpufreq_policy *policy = NULL;
        unsigned long flags;
 
-       if (cpufreq_disabled())
-               return NULL;
-
        if (!down_read_trylock(&cpufreq_rwsem))
                return NULL;
 
@@ -229,9 +226,6 @@ EXPORT_SYMBOL_GPL(cpufreq_cpu_get);
 

Quoted 12 lines

drivers/cpufreq/cpufreq.c EXPORT_SYMBOL_GPL(cpufreq_cpu_get); · line 231
Quoted 8 lines
drivers/cpufreq/cpufreq.c +0 -3
 void cpufreq_cpu_put(struct cpufreq_policy *policy)
 {
-       if (cpufreq_disabled())
-               return;
-
        kobject_put(&policy->kobj);
        up_read(&cpufreq_rwsem);
 }
@Rafael: Are you fine with this patch as is? I meant, you just left it for rebase conflict or because you want it to be part of the WARN() you suggested?

It depended on [15/17] that I didn't apply.

Quoted 3 lines

The WARN here might not be a good idea as this can be called by users while cpufreq is disabled. But we don't need this check because of the reasons I mentioned in commit log.

I'm not sure which commit log are you talking about, this one or the [15/17] one?

Signature · 2 lines

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Viresh Kumar

author maintainer

2015-01-27 03:47 UTC #19 lore raw

Re: [PATCH 16/17] cpufreq: remove check for cpufreq_disabled() from cpufreq_cpu_{get|put}()

Quoted 1 line · Rafael J. Wysocki

It depended on [15/17] that I didn't apply.

That's all I wanted to know.

Quoted 5 lines

Quoted 3 lines
The WARN here might not be a good idea as this can be called by users while cpufreq is disabled. But we don't need this check because of the reasons I mentioned in commit log.
I'm not sure which commit log are you talking about, this one or the [15/17] one?

This one.

Viresh Kumar

author maintainer

2015-01-02 07:06 UTC #20 lore raw

PATCH 17/17 cpufreq: move some initialization stuff to cpufreq_policy_alloc()

In reply to Viresh Kumar

We need to initialize completion and work only on policy allocation and not really on the policy restore side and so we better move this piece of code to cpufreq_policy_alloc().

Signed-off-by Viresh Kumar <[email protected]>

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

drivers/cpufreq/cpufreq.c +2 -3

index 62c6a0b8b0d1..ace5dd4cd271 100644
@@ -1019,6 +1019,8 @@ static struct cpufreq_policy *cpufreq_policy_alloc(void)
 	init_rwsem(&policy->rwsem);
 	spin_lock_init(&policy->transition_lock);
 	init_waitqueue_head(&policy->transition_wait);
+	init_completion(&policy->kobj_unregister);
+	INIT_WORK(&policy->update, handle_update);
 
 	return policy;

@@ -1142,9 +1144,6 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 
 	cpumask_copy(policy->cpus, cpumask_of(cpu));
 
-	init_completion(&policy->kobj_unregister);
-	INIT_WORK(&policy->update, handle_update);
-
 	/* call driver. From then on the cpufreq must be able
 	 * to accept all calls to ->verify and ->setpolicy for this CPU
 	 */

Signature · 1 line

2.2.0

Viresh Kumar

author maintainer

2015-01-12 06:11 UTC #21 lore raw

In reply to Viresh Kumar

Quoted 6 lines · Viresh Kumar

Along with the big changes I have for cpufreq core simplification, I also accumulated these trivial cleanups. In order to not confuse the much dedicated series on solving the real problems, I am sending these separately.
These are independent of the stats cleanups sent earlier and so it doesn't matter which one you apply first.

Ping for some reviews :)