[PATCH V5 0/7] cpufreq: suspend early/resume late: dpm_{suspend|resume}()

[PATCH V5 0/7] cpufreq: suspend early/resume late: dpm_{suspend|resume}()

[Viresh Kumar]

This patchset creates/calls cpufreq suspend/resume callbacks from dpm_{suspend|resume}()
for handling suspend/resume of cpufreq governors and core.

There are multiple problems that are fixed by this patch:
- Nishanth Menon (TI) found an interesting problem on his platform, OMAP. His board
  wasn't working well with suspend/resume as calls for removing non-boot CPUs
  was turning out into a call to drivers ->target() which then tries to play
  with regulators. But regulators and their I2C bus were already suspended and
  this resulted in a failure. Many platforms have such problems, samsung, tegra,
  etc.. They solved it with driver specific PM notifiers where they used to
  disable their driver's ->target() routine.
- Lan Tianyu (Intel) & Jinhyuk Choi (Broadcom) found an issue where tunables
  configuration for clusters/sockets with non-boot CPUs was getting lost after
  suspend/resume, as we were notifying governors with CPUFREQ_GOV_POLICY_EXIT on
  removal of the last cpu for that policy and so deallocating memory for
  tunables.  This is fixed by this patch as we don't allow any operation on
  governors after device suspend and before device resume now.

This is already tested by few people and so incorporating their Tested-by as
well.

I have tested this again on latest stuff on my thinkpad for several
suspend/resume cycles.

Viresh Kumar (7):
  cpufreq: suspend governors on system suspend/hibernate
  cpufreq: suspend governors from dpm_{suspend|resume}()
  cpufreq: call driver's suspend/resume for each policy
  cpufreq: Implement cpufreq_generic_suspend()
  cpufreq: exynos: Use cpufreq_generic_suspend()
  cpufreq: s5pv210: Use cpufreq_generic_suspend()
  cpufreq: Tegra: Use cpufreq_generic_suspend()

 drivers/base/power/main.c         |   5 ++
 drivers/cpufreq/cpufreq.c         | 133 ++++++++++++++++++++++----------------
 drivers/cpufreq/exynos-cpufreq.c  |  96 ++-------------------------
 drivers/cpufreq/s5pv210-cpufreq.c |  49 +-------------
 drivers/cpufreq/tegra-cpufreq.c   |  46 ++-----------
 include/linux/cpufreq.h           |  11 ++++
 6 files changed, 108 insertions(+), 232 deletions(-)

-- 
1.7.12.rc2.18.g61b472e

[PATCH V5 1/7] cpufreq: suspend governors on system suspend/hibernate

[Viresh Kumar]

This patch adds cpufreq suspend/resume calls to dpm_{suspend|resume}_noirq() for
handling suspend/resume of cpufreq governors.

Lan Tianyu (Intel) & Jinhyuk Choi (Broadcom) found an issue where tunables
configuration for clusters/sockets with non-boot CPUs was getting lost after
suspend/resume, as we were notifying governors with CPUFREQ_GOV_POLICY_EXIT on
removal of the last cpu for that policy and so deallocating memory for tunables.
This is fixed by this patch as we don't allow any operation on governors after
device suspend and before device resume now.

Reported-and-tested-by: Lan Tianyu <tianyu.lan@intel.com>
Reported-by: Jinhyuk Choi <jinchoi@broadcom.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/main.c |  3 +++
 drivers/cpufreq/cpufreq.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/cpufreq.h   |  8 ++++++++
 3 files changed, 54 insertions(+)

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index deb4e02..da49882 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -29,6 +29,7 @@
 #include <linux/async.h>
 #include <linux/suspend.h>
 #include <trace/events/power.h>
+#include <linux/cpufreq.h>
 #include <linux/cpuidle.h>
 #include <linux/timer.h>
 
@@ -581,6 +582,7 @@ static void dpm_resume_noirq(pm_message_t state)
 	dpm_show_time(starttime, state, "noirq");
 	resume_device_irqs();
 	cpuidle_resume();
+	cpufreq_resume();
 }
 
 /**
@@ -1061,6 +1063,7 @@ static int dpm_suspend_noirq(pm_message_t state)
 	ktime_t starttime = ktime_get();
 	int error = 0;
 
+	cpufreq_suspend();
 	cpuidle_pause();
 	suspend_device_irqs();
 	mutex_lock(&dpm_list_mtx);
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 08ca8c9..cb23858 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -26,6 +26,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
+#include <linux/suspend.h>
 #include <linux/syscore_ops.h>
 #include <linux/tick.h>
 #include <trace/events/power.h>
@@ -47,6 +48,9 @@ static LIST_HEAD(cpufreq_policy_list);
 static DEFINE_PER_CPU(char[CPUFREQ_NAME_LEN], cpufreq_cpu_governor);
 #endif
 
+/* Flag to suspend/resume CPUFreq governors */
+static bool cpufreq_suspended;
+
 static inline bool has_target(void)
 {
 	return cpufreq_driver->target_index || cpufreq_driver->target;
@@ -1576,6 +1580,41 @@ static struct subsys_interface cpufreq_interface = {
 	.remove_dev	= cpufreq_remove_dev,
 };
 
+void cpufreq_suspend(void)
+{
+	struct cpufreq_policy *policy;
+
+	if (!has_target())
+		return;
+
+	pr_debug("%s: Suspending Governors\n", __func__);
+
+	list_for_each_entry(policy, &cpufreq_policy_list, policy_list)
+		if (__cpufreq_governor(policy, CPUFREQ_GOV_STOP))
+			pr_err("%s: Failed to stop governor for policy: %p\n",
+				__func__, policy);
+
+	cpufreq_suspended = true;
+}
+
+void cpufreq_resume(void)
+{
+	struct cpufreq_policy *policy;
+
+	if (!has_target())
+		return;
+
+	pr_debug("%s: Resuming Governors\n", __func__);
+
+	cpufreq_suspended = false;
+
+	list_for_each_entry(policy, &cpufreq_policy_list, policy_list)
+		if (__cpufreq_governor(policy, CPUFREQ_GOV_START)
+		    || __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS))
+			pr_err("%s: Failed to start governor for policy: %p\n",
+				__func__, policy);
+}
+
 /**
  * cpufreq_bp_suspend - Prepare the boot CPU for system suspend.
  *
@@ -1869,6 +1908,10 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
 	struct cpufreq_governor *gov = NULL;
 #endif
 
+	/* Don't start any governor operations if we are entering suspend */
+	if (cpufreq_suspended)
+		return 0;
+
 	if (policy->governor->max_transition_latency &&
 	    policy->cpuinfo.transition_latency >
 	    policy->governor->max_transition_latency) {
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 4d89e0e..94ed907 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -296,6 +296,14 @@ cpufreq_verify_within_cpu_limits(struct cpufreq_policy *policy)
 			policy->cpuinfo.max_freq);
 }
 
+#ifdef CONFIG_CPU_FREQ
+void cpufreq_suspend(void);
+void cpufreq_resume(void);
+#else
+static inline void cpufreq_suspend(void) {}
+static inline void cpufreq_resume(void) {}
+#endif
+
 /*********************************************************************
  *                     CPUFREQ NOTIFIER INTERFACE                    *
  *********************************************************************/
-- 
1.7.12.rc2.18.g61b472e

[PATCH V5 2/7] cpufreq: suspend governors from dpm_{suspend|resume}()

[Viresh Kumar]

Previous patch added support for suspending governors, with callbacks being
called from dpm_{suspend|resume}_noirq(). The problem here is that most of the
devices (i.e. devices with ->suspend() callbacks) have already been suspended by
now and so if drivers want to change frequency before suspending, then it might
not be possible for many platforms (which depend on other peripherals like i2c,
regulators, etc).

So, we actually need to do this from dpm_{suspend|resume}() instead. This patch
does it.

Tested-by: Lan Tianyu <tianyu.lan@intel.com>
Tested-by: Nishanth Menon <nm@ti.com>
Tested-by: Stephen Warren <swarren@nvidia.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/main.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index da49882..0f82012 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -582,7 +582,6 @@ static void dpm_resume_noirq(pm_message_t state)
 	dpm_show_time(starttime, state, "noirq");
 	resume_device_irqs();
 	cpuidle_resume();
-	cpufreq_resume();
 }
 
 /**
@@ -858,6 +857,8 @@ void dpm_resume(pm_message_t state)
 	mutex_unlock(&dpm_list_mtx);
 	async_synchronize_full();
 	dpm_show_time(starttime, state, NULL);
+
+	cpufreq_resume();
 }
 
 /**
@@ -1063,7 +1064,6 @@ static int dpm_suspend_noirq(pm_message_t state)
 	ktime_t starttime = ktime_get();
 	int error = 0;
 
-	cpufreq_suspend();
 	cpuidle_pause();
 	suspend_device_irqs();
 	mutex_lock(&dpm_list_mtx);
@@ -1415,6 +1415,8 @@ int dpm_suspend(pm_message_t state)
 
 	might_sleep();
 
+	cpufreq_suspend();
+
 	mutex_lock(&dpm_list_mtx);
 	pm_transition = state;
 	async_error = 0;
-- 
1.7.12.rc2.18.g61b472e

[PATCH V5 3/7] cpufreq: call driver's suspend/resume for each policy

[Viresh Kumar]

Earlier cpufreq suspend/resume callbacks into drivers were getting called only
for the boot CPU, as by the time callbacks were called non-boot CPUs were
already removed. Because we might still need driver specific actions on
suspend/resume, its better to use earlier infrastructure from the early
suspend/late resume calls.

In effect, we call suspend/resume for each policy. The resume part also takes
care of synchronising frequency for boot CPU, which might turn out be different
than what cpufreq core believes.

Hence, the earlier syscore infrastructure is getting removed now.

Tested-by: Lan Tianyu <tianyu.lan@intel.com>
Tested-by: Nishanth Menon <nm@ti.com>
Tested-by: Stephen Warren <swarren@nvidia.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 112 +++++++++++++---------------------------------
 1 file changed, 32 insertions(+), 80 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index cb23858..87a4b96 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -27,7 +27,6 @@
 #include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/suspend.h>
-#include <linux/syscore_ops.h>
 #include <linux/tick.h>
 #include <trace/events/power.h>
 
@@ -1580,6 +1579,14 @@ static struct subsys_interface cpufreq_interface = {
 	.remove_dev	= cpufreq_remove_dev,
 };
 
+/**
+ * cpufreq_suspend() - Suspend CPUFreq governors
+ *
+ * Called during system wide Suspend/Hibernate cycles for suspending governors
+ * as some platforms can't change frequency after this point in suspend cycle.
+ * Because some of the devices (like: i2c, regulators, etc) they use for
+ * changing frequency are suspended quickly after this point.
+ */
 void cpufreq_suspend(void)
 {
 	struct cpufreq_policy *policy;
@@ -1589,14 +1596,25 @@ void cpufreq_suspend(void)
 
 	pr_debug("%s: Suspending Governors\n", __func__);
 
-	list_for_each_entry(policy, &cpufreq_policy_list, policy_list)
+	list_for_each_entry(policy, &cpufreq_policy_list, policy_list) {
 		if (__cpufreq_governor(policy, CPUFREQ_GOV_STOP))
 			pr_err("%s: Failed to stop governor for policy: %p\n",
 				__func__, policy);
+		else if (cpufreq_driver->suspend
+		    && cpufreq_driver->suspend(policy))
+			pr_err("%s: Failed to suspend driver: %p\n", __func__,
+				policy);
+	}
 
 	cpufreq_suspended = true;
 }
 
+/**
+ * cpufreq_resume() - Resume CPUFreq governors
+ *
+ * Called during system wide Suspend/Hibernate cycle for resuming governors that
+ * are suspended with cpufreq_suspend().
+ */
 void cpufreq_resume(void)
 {
 	struct cpufreq_policy *policy;
@@ -1608,91 +1626,26 @@ void cpufreq_resume(void)
 
 	cpufreq_suspended = false;
 
-	list_for_each_entry(policy, &cpufreq_policy_list, policy_list)
+	list_for_each_entry(policy, &cpufreq_policy_list, policy_list) {
 		if (__cpufreq_governor(policy, CPUFREQ_GOV_START)
 		    || __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS))
 			pr_err("%s: Failed to start governor for policy: %p\n",
 				__func__, policy);
-}
-
-/**
- * cpufreq_bp_suspend - Prepare the boot CPU for system suspend.
- *
- * This function is only executed for the boot processor.  The other CPUs
- * have been put offline by means of CPU hotplug.
- */
-static int cpufreq_bp_suspend(void)
-{
-	int ret = 0;
-
-	int cpu = smp_processor_id();
-	struct cpufreq_policy *policy;
-
-	pr_debug("suspending cpu %u\n", cpu);
-
-	/* If there's no policy for the boot CPU, we have nothing to do. */
-	policy = cpufreq_cpu_get(cpu);
-	if (!policy)
-		return 0;
-
-	if (cpufreq_driver->suspend) {
-		ret = cpufreq_driver->suspend(policy);
-		if (ret)
-			printk(KERN_ERR "cpufreq: suspend failed in ->suspend "
-					"step on CPU %u\n", policy->cpu);
-	}
-
-	cpufreq_cpu_put(policy);
-	return ret;
-}
-
-/**
- * cpufreq_bp_resume - Restore proper frequency handling of the boot CPU.
- *
- *	1.) resume CPUfreq hardware support (cpufreq_driver->resume())
- *	2.) schedule call cpufreq_update_policy() ASAP as interrupts are
- *	    restored. It will verify that the current freq is in sync with
- *	    what we believe it to be. This is a bit later than when it
- *	    should be, but nonethteless it's better than calling
- *	    cpufreq_driver->get() here which might re-enable interrupts...
- *
- * This function is only executed for the boot CPU.  The other CPUs have not
- * been turned on yet.
- */
-static void cpufreq_bp_resume(void)
-{
-	int ret = 0;
+		else if (cpufreq_driver->resume
+		    && cpufreq_driver->resume(policy))
+			pr_err("%s: Failed to resume driver: %p\n", __func__,
+				policy);
 
-	int cpu = smp_processor_id();
-	struct cpufreq_policy *policy;
-
-	pr_debug("resuming cpu %u\n", cpu);
-
-	/* If there's no policy for the boot CPU, we have nothing to do. */
-	policy = cpufreq_cpu_get(cpu);
-	if (!policy)
-		return;
-
-	if (cpufreq_driver->resume) {
-		ret = cpufreq_driver->resume(policy);
-		if (ret) {
-			printk(KERN_ERR "cpufreq: resume failed in ->resume "
-					"step on CPU %u\n", policy->cpu);
-			goto fail;
-		}
+		/*
+		 * schedule call cpufreq_update_policy() for boot CPU, i.e. last
+		 * policy in list. It will verify that the current freq is in
+		 * sync with what we believe it to be.
+		 */
+		if (list_is_last(&policy->policy_list, &cpufreq_policy_list))
+			schedule_work(&policy->update);
 	}
-
-	schedule_work(&policy->update);
-
-fail:
-	cpufreq_cpu_put(policy);
 }
 
-static struct syscore_ops cpufreq_syscore_ops = {
-	.suspend	= cpufreq_bp_suspend,
-	.resume		= cpufreq_bp_resume,
-};
-
 /**
  *	cpufreq_get_current_driver - return current driver's name
  *
@@ -2458,7 +2411,6 @@ static int __init cpufreq_core_init(void)
 
 	cpufreq_global_kobject = kobject_create();
 	BUG_ON(!cpufreq_global_kobject);
-	register_syscore_ops(&cpufreq_syscore_ops);
 
 	return 0;
 }
-- 
1.7.12.rc2.18.g61b472e

[PATCH V5 4/7] cpufreq: Implement cpufreq_generic_suspend()

[Viresh Kumar]

Multiple platforms need to set CPU to a particular frequency before suspending
system. And so they need a common infrastructure which is provided by this
patch. Those platforms just need to initialize their ->suspend() pointers with
the generic routine.

Tested-by: Stephen Warren <swarren@nvidia.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 26 ++++++++++++++++++++++++++
 include/linux/cpufreq.h   |  3 +++
 2 files changed, 29 insertions(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 87a4b96..c13c100 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1579,6 +1579,32 @@ static struct subsys_interface cpufreq_interface = {
 	.remove_dev	= cpufreq_remove_dev,
 };
 
+/*
+ * In case platform wants some specific frequency to be configured
+ * during suspend..
+ */
+int cpufreq_generic_suspend(struct cpufreq_policy *policy)
+{
+	int ret;
+
+	if (!policy->suspend_freq) {
+		pr_err("%s: suspend_freq can't be zero\n", __func__);
+		return -EINVAL;
+	}
+
+	pr_debug("%s: Setting suspend-freq: %u\n", __func__,
+			policy->suspend_freq);
+
+	ret = __cpufreq_driver_target(policy, policy->suspend_freq,
+			CPUFREQ_RELATION_H);
+	if (ret)
+		pr_err("%s: unable to set suspend-freq: %u. err: %d\n",
+				__func__, policy->suspend_freq, ret);
+
+	return ret;
+}
+EXPORT_SYMBOL(cpufreq_generic_suspend);
+
 /**
  * cpufreq_suspend() - Suspend CPUFreq governors
  *
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 94ed907..325bab0 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -74,6 +74,8 @@ struct cpufreq_policy {
 	unsigned int		max;    /* in kHz */
 	unsigned int		cur;    /* in kHz, only needed if cpufreq
 					 * governors are used */
+	unsigned int		suspend_freq; /* freq to set during suspend */
+
 	unsigned int		policy; /* see above */
 	struct cpufreq_governor	*governor; /* see below */
 	void			*governor_data;
@@ -299,6 +301,7 @@ cpufreq_verify_within_cpu_limits(struct cpufreq_policy *policy)
 #ifdef CONFIG_CPU_FREQ
 void cpufreq_suspend(void);
 void cpufreq_resume(void);
+int cpufreq_generic_suspend(struct cpufreq_policy *policy);
 #else
 static inline void cpufreq_suspend(void) {}
 static inline void cpufreq_resume(void) {}
-- 
1.7.12.rc2.18.g61b472e

[PATCH V5 5/7] cpufreq: exynos: Use cpufreq_generic_suspend()

[Viresh Kumar]

Currently we have implemented PM notifiers to disable/enable ->target() routines
functionality during suspend/resume.

Now we have support present in cpufreq core, lets use it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/exynos-cpufreq.c | 96 +++-------------------------------------
 1 file changed, 7 insertions(+), 89 deletions(-)

diff --git a/drivers/cpufreq/exynos-cpufreq.c b/drivers/cpufreq/exynos-cpufreq.c
index fcd2914..307f02e 100644
--- a/drivers/cpufreq/exynos-cpufreq.c
+++ b/drivers/cpufreq/exynos-cpufreq.c
@@ -16,7 +16,6 @@
 #include <linux/slab.h>
 #include <linux/regulator/consumer.h>
 #include <linux/cpufreq.h>
-#include <linux/suspend.h>
 #include <linux/platform_device.h>
 
 #include <plat/cpu.h>
@@ -24,12 +23,8 @@
 #include "exynos-cpufreq.h"
 
 static struct exynos_dvfs_info *exynos_info;
-
 static struct regulator *arm_regulator;
-
 static unsigned int locking_frequency;
-static bool frequency_locked;
-static DEFINE_MUTEX(cpufreq_lock);
 
 static int exynos_cpufreq_get_index(unsigned int freq)
 {
@@ -134,83 +129,13 @@ out:
 
 static int exynos_target(struct cpufreq_policy *policy, unsigned int index)
 {
-	struct cpufreq_frequency_table *freq_table = exynos_info->freq_table;
-	int ret = 0;
-
-	mutex_lock(&cpufreq_lock);
-
-	if (frequency_locked)
-		goto out;
-
-	ret = exynos_cpufreq_scale(freq_table[index].frequency);
-
-out:
-	mutex_unlock(&cpufreq_lock);
-
-	return ret;
-}
-
-#ifdef CONFIG_PM
-static int exynos_cpufreq_suspend(struct cpufreq_policy *policy)
-{
-	return 0;
-}
-
-static int exynos_cpufreq_resume(struct cpufreq_policy *policy)
-{
-	return 0;
-}
-#endif
-
-/**
- * exynos_cpufreq_pm_notifier - block CPUFREQ's activities in suspend-resume
- *			context
- * @notifier
- * @pm_event
- * @v
- *
- * While frequency_locked == true, target() ignores every frequency but
- * locking_frequency. The locking_frequency value is the initial frequency,
- * which is set by the bootloader. In order to eliminate possible
- * inconsistency in clock values, we save and restore frequencies during
- * suspend and resume and block CPUFREQ activities. Note that the standard
- * suspend/resume cannot be used as they are too deep (syscore_ops) for
- * regulator actions.
- */
-static int exynos_cpufreq_pm_notifier(struct notifier_block *notifier,
-				       unsigned long pm_event, void *v)
-{
-	int ret;
-
-	switch (pm_event) {
-	case PM_SUSPEND_PREPARE:
-		mutex_lock(&cpufreq_lock);
-		frequency_locked = true;
-		mutex_unlock(&cpufreq_lock);
-
-		ret = exynos_cpufreq_scale(locking_frequency);
-		if (ret < 0)
-			return NOTIFY_BAD;
-
-		break;
-
-	case PM_POST_SUSPEND:
-		mutex_lock(&cpufreq_lock);
-		frequency_locked = false;
-		mutex_unlock(&cpufreq_lock);
-		break;
-	}
-
-	return NOTIFY_OK;
+	return exynos_cpufreq_scale(exynos_info->freq_table[index].frequency);
 }
 
-static struct notifier_block exynos_cpufreq_nb = {
-	.notifier_call = exynos_cpufreq_pm_notifier,
-};
-
 static int exynos_cpufreq_cpu_init(struct cpufreq_policy *policy)
 {
 	policy->clk = exynos_info->cpu_clk;
+	policy->suspend_freq = locking_frequency;
 	return cpufreq_generic_init(policy, exynos_info->freq_table, 100000);
 }
 
@@ -227,8 +152,7 @@ static struct cpufreq_driver exynos_driver = {
 	.boost_supported = true,
 #endif
 #ifdef CONFIG_PM
-	.suspend	= exynos_cpufreq_suspend,
-	.resume		= exynos_cpufreq_resume,
+	.suspend	= cpufreq_generic_suspend,
 #endif
 };
 
@@ -263,19 +187,13 @@ static int exynos_cpufreq_probe(struct platform_device *pdev)
 		goto err_vdd_arm;
 	}
 
+	/* Done here as we want to capture boot frequency */
 	locking_frequency = clk_get_rate(exynos_info->cpu_clk) / 1000;
 
-	register_pm_notifier(&exynos_cpufreq_nb);
-
-	if (cpufreq_register_driver(&exynos_driver)) {
-		pr_err("%s: failed to register cpufreq driver\n", __func__);
-		goto err_cpufreq;
-	}
-
-	return 0;
-err_cpufreq:
-	unregister_pm_notifier(&exynos_cpufreq_nb);
+	if (!cpufreq_register_driver(&exynos_driver))
+		return 0;
 
+	pr_err("%s: failed to register cpufreq driver\n", __func__);
 	regulator_put(arm_regulator);
 err_vdd_arm:
 	kfree(exynos_info);
-- 
1.7.12.rc2.18.g61b472e

[PATCH V5 6/7] cpufreq: s5pv210: Use cpufreq_generic_suspend()

[Viresh Kumar]

Currently we have implemented PM notifiers to disable/enable ->target() routines
functionality during suspend/resume.

Now we have support present in cpufreq core, lets use it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/s5pv210-cpufreq.c | 49 +++------------------------------------
 1 file changed, 3 insertions(+), 46 deletions(-)

diff --git a/drivers/cpufreq/s5pv210-cpufreq.c b/drivers/cpufreq/s5pv210-cpufreq.c
index 55a8e9f..7242153 100644
--- a/drivers/cpufreq/s5pv210-cpufreq.c
+++ b/drivers/cpufreq/s5pv210-cpufreq.c
@@ -18,7 +18,6 @@
 #include <linux/cpufreq.h>
 #include <linux/reboot.h>
 #include <linux/regulator/consumer.h>
-#include <linux/suspend.h>
 
 #include <mach/map.h>
 #include <mach/regs-clock.h>
@@ -435,18 +434,6 @@ exit:
 	return ret;
 }
 
-#ifdef CONFIG_PM
-static int s5pv210_cpufreq_suspend(struct cpufreq_policy *policy)
-{
-	return 0;
-}
-
-static int s5pv210_cpufreq_resume(struct cpufreq_policy *policy)
-{
-	return 0;
-}
-#endif
-
 static int check_mem_type(void __iomem *dmc_reg)
 {
 	unsigned long val;
@@ -502,6 +489,7 @@ static int __init s5pv210_cpu_init(struct cpufreq_policy *policy)
 	s5pv210_dram_conf[1].refresh = (__raw_readl(S5P_VA_DMC1 + 0x30) * 1000);
 	s5pv210_dram_conf[1].freq = clk_get_rate(dmc1_clk);
 
+	policy->suspend_freq = SLEEP_FREQ;
 	return cpufreq_generic_init(policy, s5pv210_freq_table, 40000);
 
 out_dmc1:
@@ -511,32 +499,6 @@ out_dmc0:
 	return ret;
 }
 
-static int s5pv210_cpufreq_notifier_event(struct notifier_block *this,
-					  unsigned long event, void *ptr)
-{
-	int ret;
-
-	switch (event) {
-	case PM_SUSPEND_PREPARE:
-		ret = cpufreq_driver_target(cpufreq_cpu_get(0), SLEEP_FREQ, 0);
-		if (ret < 0)
-			return NOTIFY_BAD;
-
-		/* Disable updation of cpu frequency */
-		no_cpufreq_access = true;
-		return NOTIFY_OK;
-	case PM_POST_RESTORE:
-	case PM_POST_SUSPEND:
-		/* Enable updation of cpu frequency */
-		no_cpufreq_access = false;
-		cpufreq_driver_target(cpufreq_cpu_get(0), SLEEP_FREQ, 0);
-
-		return NOTIFY_OK;
-	}
-
-	return NOTIFY_DONE;
-}
-
 static int s5pv210_cpufreq_reboot_notifier_event(struct notifier_block *this,
 						 unsigned long event, void *ptr)
 {
@@ -558,15 +520,11 @@ static struct cpufreq_driver s5pv210_driver = {
 	.init		= s5pv210_cpu_init,
 	.name		= "s5pv210",
 #ifdef CONFIG_PM
-	.suspend	= s5pv210_cpufreq_suspend,
-	.resume		= s5pv210_cpufreq_resume,
+	.suspend	= cpufreq_generic_suspend,
+	.resume		= cpufreq_generic_suspend, /* We need to set SLEEP FREQ again */
 #endif
 };
 
-static struct notifier_block s5pv210_cpufreq_notifier = {
-	.notifier_call = s5pv210_cpufreq_notifier_event,
-};
-
 static struct notifier_block s5pv210_cpufreq_reboot_notifier = {
 	.notifier_call = s5pv210_cpufreq_reboot_notifier_event,
 };
@@ -586,7 +544,6 @@ static int __init s5pv210_cpufreq_init(void)
 		return PTR_ERR(int_regulator);
 	}
 
-	register_pm_notifier(&s5pv210_cpufreq_notifier);
 	register_reboot_notifier(&s5pv210_cpufreq_reboot_notifier);
 
 	return cpufreq_register_driver(&s5pv210_driver);
-- 
1.7.12.rc2.18.g61b472e

[PATCH V5 7/7] cpufreq: Tegra: Use cpufreq_generic_suspend()

[Viresh Kumar]

Currently we have implemented PM notifiers to disable/enable ->target() routines
functionality during suspend/resume.

Now we have support present in cpufreq core, lets use it.

Tested-by: Stephen Warren <swarren@nvidia.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/tegra-cpufreq.c | 46 +++++------------------------------------
 1 file changed, 5 insertions(+), 41 deletions(-)

diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c
index e652c1b..bcfed77 100644
--- a/drivers/cpufreq/tegra-cpufreq.c
+++ b/drivers/cpufreq/tegra-cpufreq.c
@@ -26,7 +26,6 @@
 #include <linux/err.h>
 #include <linux/clk.h>
 #include <linux/io.h>
-#include <linux/suspend.h>
 
 static struct cpufreq_frequency_table freq_table[] = {
 	{ .frequency = 216000 },
@@ -47,9 +46,6 @@ static struct clk *pll_x_clk;
 static struct clk *pll_p_clk;
 static struct clk *emc_clk;
 
-static DEFINE_MUTEX(tegra_cpu_lock);
-static bool is_suspended;
-
 static int tegra_cpu_clk_set_rate(unsigned long rate)
 {
 	int ret;
@@ -112,42 +108,9 @@ static int tegra_update_cpu_speed(struct cpufreq_policy *policy,
 
 static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
 {
-	int ret = -EBUSY;
-
-	mutex_lock(&tegra_cpu_lock);
-
-	if (!is_suspended)
-		ret = tegra_update_cpu_speed(policy,
-				freq_table[index].frequency);
-
-	mutex_unlock(&tegra_cpu_lock);
-	return ret;
+	return tegra_update_cpu_speed(policy, freq_table[index].frequency);
 }
 
-static int tegra_pm_notify(struct notifier_block *nb, unsigned long event,
-	void *dummy)
-{
-	mutex_lock(&tegra_cpu_lock);
-	if (event == PM_SUSPEND_PREPARE) {
-		struct cpufreq_policy *policy = cpufreq_cpu_get(0);
-		is_suspended = true;
-		pr_info("Tegra cpufreq suspend: setting frequency to %d kHz\n",
-			freq_table[0].frequency);
-		if (clk_get_rate(cpu_clk) / 1000 != freq_table[0].frequency)
-			tegra_update_cpu_speed(policy, freq_table[0].frequency);
-		cpufreq_cpu_put(policy);
-	} else if (event == PM_POST_SUSPEND) {
-		is_suspended = false;
-	}
-	mutex_unlock(&tegra_cpu_lock);
-
-	return NOTIFY_OK;
-}
-
-static struct notifier_block tegra_cpu_pm_notifier = {
-	.notifier_call = tegra_pm_notify,
-};
-
 static int tegra_cpu_init(struct cpufreq_policy *policy)
 {
 	int ret;
@@ -166,10 +129,8 @@ static int tegra_cpu_init(struct cpufreq_policy *policy)
 		return ret;
 	}
 
-	if (policy->cpu == 0)
-		register_pm_notifier(&tegra_cpu_pm_notifier);
-
 	policy->clk = cpu_clk;
+	policy->suspend_freq = freq_table[0].frequency;
 	return 0;
 }
 
@@ -190,6 +151,9 @@ static struct cpufreq_driver tegra_cpufreq_driver = {
 	.exit		= tegra_cpu_exit,
 	.name		= "tegra",
 	.attr		= cpufreq_generic_attr,
+#ifdef CONFIG_PM
+	.suspend	= cpufreq_generic_suspend,
+#endif
 };
 
 static int __init tegra_cpufreq_init(void)
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH V5 0/7] cpufreq: suspend early/resume late: dpm_{suspend|resume}()

[Stephen Warren]

On 02/12/2014 11:50 PM, Viresh Kumar wrote:
> This patchset creates/calls cpufreq suspend/resume callbacks from dpm_{suspend|resume}()
> for handling suspend/resume of cpufreq governors and core.

Are these patches for 3.14 or 3.15?

I ask because I just tested Linus's master from a few days back (some
point after v3.14-rc2; commit 9398a10cd964 Merge tag
'regulator-v3.14-rc2'), and I see a lot of the following messages during
suspend and/or resume (about 2-7 times, perhaps more of them from the
resume path, but it's hard to tell):

cpufreq: __cpufreq_driver_target: Failed to change cpu frequency: -16

This series does appear to solve those, so I think at least part of it
needs to be applied for 3.14.

Also, I sometimes see the following during resume. I saw it twice with
Linus's tree, but then I did 10 more reboot+suspend+resume cycles and
couldn't repro it, and I saw it once with Linus's tree plus this series
applied, then couldn't reproduce it in 5 more tries.

> [ 48.500410] ------------[ cut here ]------------
> [ 48.505252] WARNING: CPU: 0 PID: 877 at fs/sysfs/dir.c:52 sysfs_warn_dup+0x68/0x84()
> [ 48.513005] sysfs: cannot create duplicate filename '/devices/system/cpu/cpu1/cpufreq'
> [ 48.520995] Modules linked in: brcmfmac brcmutil
> [ 48.525740] CPU: 0 PID: 877 Comm: test-rtc-resume Not tainted 3.14.0-rc2-00259-g9398a10cd964 #12
> [ 48.534645] [<c0015bac>] (unwind_backtrace) from [<c0011850>] (show_stack+0x10/0x14)
> [ 48.542440] [<c0011850>] (show_stack) from [<c056e018>] (dump_stack+0x80/0xcc)
> [ 48.549757] [<c056e018>] (dump_stack) from [<c0025e44>] (warn_slowpath_common+0x64/0x88)
> [ 48.557964] [<c0025e44>] (warn_slowpath_common) from [<c0025efc>] (warn_slowpath_fmt+0x30/0x40)
> [ 48.566756] [<c0025efc>] (warn_slowpath_fmt) from [<c012776c>] (sysfs_warn_dup+0x68/0x84)
> [ 48.575024] [<c012776c>] (sysfs_warn_dup) from [<c0127a54>] (sysfs_do_create_link_sd+0xb0/0xb8)
> [ 48.583772] [<c0127a54>] (sysfs_do_create_link_sd) from [<c038ef64>] (__cpufreq_add_dev.isra.27+0x2a8/0x814)
> [ 48.593701] [<c038ef64>] (__cpufreq_add_dev.isra.27) from [<c038f548>] (cpufreq_cpu_callback+0x70/0x8c)
> [ 48.603198] [<c038f548>] (cpufreq_cpu_callback) from [<c0043864>] (notifier_call_chain+0x44/0x84)
> [ 48.612166] [<c0043864>] (notifier_call_chain) from [<c0025f60>] (__cpu_notify+0x28/0x44)
> [ 48.620424] [<c0025f60>] (__cpu_notify) from [<c00261e8>] (_cpu_up+0xf0/0x140)
> [ 48.627748] [<c00261e8>] (_cpu_up) from [<c0569eb8>] (enable_nonboot_cpus+0x68/0xb0)
> [ 48.635579] [<c0569eb8>] (enable_nonboot_cpus) from [<c006339c>] (suspend_devices_and_enter+0x198/0x2dc)
> [ 48.645136] [<c006339c>] (suspend_devices_and_enter) from [<c0063654>] (pm_suspend+0x174/0x1e8)
> [ 48.653862] [<c0063654>] (pm_suspend) from [<c00624e0>] (state_store+0x6c/0xbc)
> [ 48.661258] [<c00624e0>] (state_store) from [<c01fc200>] (kobj_attr_store+0x14/0x20)
> [ 48.669083] [<c01fc200>] (kobj_attr_store) from [<c0126e50>] (sysfs_kf_write+0x44/0x48)
> [ 48.677163] [<c0126e50>] (sysfs_kf_write) from [<c012a274>] (kernfs_fop_write+0xb4/0x14c)
> [ 48.685432] [<c012a274>] (kernfs_fop_write) from [<c00d4818>] (vfs_write+0xa8/0x180)
> [ 48.693214] [<c00d4818>] (vfs_write) from [<c00d4bb8>] (SyS_write+0x3c/0x70)
> [ 48.700349] [<c00d4bb8>] (SyS_write) from [<c000e620>] (ret_fast_syscall+0x0/0x30)
> [ 48.708053] ---[ end trace 76969904b614c18f ]--- 

Do you have any idea what the problem might be, and how to solve it?

Re: [PATCH V5 0/7] cpufreq: suspend early/resume late: dpm_{suspend|resume}()

[Stephen Warren]

On 02/14/2014 12:42 PM, Stephen Warren wrote:
> On 02/12/2014 11:50 PM, Viresh Kumar wrote:
>> This patchset creates/calls cpufreq suspend/resume callbacks from dpm_{suspend|resume}()
>> for handling suspend/resume of cpufreq governors and core.

BTW, I also happened to test these on a Tegra114 system, on which there
is no cpufreq driver, and this series (applied on top of commit
9398a10cd964 Merge tag 'regulator-v3.14-rc2') causes the following
during suspend:

> [  196.666799] PM: Syncing filesystems ... done.
> [  196.692824] Freezing user space processes ... (elapsed 0.002 seconds) done.
> [  196.694265] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
> [  196.695377] Unable to handle kernel NULL pointer dereference at virtual address 00000024
> [  196.695380] pgd = ec454000
> [  196.695390] [00000024] *pgd=acb0c831, *pte=00000000, *ppte=00000000
> [  196.695398] Internal error: Oops: 17 [#1] PREEMPT SMP ARM
> [  196.695405] Modules linked in:
> [  196.695413] CPU: 0 PID: 892 Comm: test-rtc-resume Not tainted 3.14.0-rc2-00266-gca91b201c563-dirty #41
> [  196.695418] task: ecb86e00 ti: ec692000 task.ti: ec692000
> [  196.695432] PC is at cpufreq_suspend+0xc/0xc8
> [  196.695440] LR is at dpm_suspend+0x30/0x214
> [  196.695447] pc : [<c038f6c0>]    lr : [<c02c3208>]    psr: 60000113
> [  196.695447] sp : ec693e60  ip : 00000018  fp : 000aa2b8
> [  196.695451] r10: c07dcfec  r9 : ec975808  r8 : c07f4a28
> [  196.695455] r7 : c08642c8  r6 : c08658a0  r5 : 00000003  r4 : c081f690
> [  196.695459] r3 : 00000000  r2 : cbf0c52d  r1 : 3b9aca00  r0 : ec693e80
> [  196.695465] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
> [  196.695470] Control: 10c5387d  Table: ac45406a  DAC: 00000015
> [  196.695475] Process test-rtc-resume (pid: 892, stack limit = 0xec692240)
> [  196.695479] Stack: (0xec693e60 to 0xec694000)
> [  196.695488] 3e60: c081f690 00000003 c07f49ec c02c3208 c07f4a20 00000002 cbf0c52d 0000002d
> [  196.695495] 3e80: cbf0c52d 0000002d 00000003 c081f690 00000003 00000003 00000003 ec88bcc0
> [  196.695503] 3ea0: ec975808 00000004 000aa2b8 c006324c 000aa2b8 c056cb70 c070a0a4 ec693ed4
> [  196.695509] 3ec0: 00000003 ec693ed4 00000003 00000000 00000003 c0577bc4 00000003 ec88bcc0
> [  196.695516] 3ee0: 00000004 c0063654 0000006d 00000003 c06b8040 c00624e0 00000004 ed13b4c8
> [  196.695523] 3f00: ec88bcc0 00000004 ec693f80 ec88bcc0 ec975800 c01fc200 00000004 c0126e50
> [  196.695530] 3f20: c0126e0c 00000000 00000000 c012a274 00000000 00000000 ec4f3ec0 00000004
> [  196.695536] 3f40: 000ac408 ec693f80 00000000 ec692000 00000004 c00d4818 ec9c6d40 00000001
> [  196.695543] 3f60: 0000000a 00000000 00000000 ec4f3ec0 000ac408 00000000 00000004 c00d4bb8
> [  196.695550] 3f80: 00000000 00000000 ec4f3ec0 b6f17a78 00000004 000ac408 00000004 c000e7a4
> [  196.695556] 3fa0: 00000000 c000e620 b6f17a78 00000004 00000001 000ac408 00000004 00000000
> [  196.695563] 3fc0: b6f17a78 00000004 000ac408 00000004 beca645c 000a6094 00000000 000aa2b8
> [  196.695570] 3fe0: 00000000 beca63dc b6e86747 b6ebe60c 400f0010 00000001 941e9002 3000f8d9
> [  196.695585] [<c038f6c0>] (cpufreq_suspend) from [<c02c3208>] (dpm_suspend+0x30/0x214)
> [  196.695601] [<c02c3208>] (dpm_suspend) from [<c006324c>] (suspend_devices_and_enter+0x48/0x2dc)
> [  196.695613] [<c006324c>] (suspend_devices_and_enter) from [<c0063654>] (pm_suspend+0x174/0x1e8)
> [  196.695623] [<c0063654>] (pm_suspend) from [<c00624e0>] (state_store+0x6c/0xbc)
> [  196.695634] [<c00624e0>] (state_store) from [<c01fc200>] (kobj_attr_store+0x14/0x20)
> [  196.695650] [<c01fc200>] (kobj_attr_store) from [<c0126e50>] (sysfs_kf_write+0x44/0x48)
> [  196.695663] [<c0126e50>] (sysfs_kf_write) from [<c012a274>] (kernfs_fop_write+0xb4/0x14c)
> [  196.695677] [<c012a274>] (kernfs_fop_write) from [<c00d4818>] (vfs_write+0xa8/0x180)
> [  196.695691] [<c00d4818>] (vfs_write) from [<c00d4bb8>] (SyS_write+0x3c/0x70)
> [  196.695705] [<c00d4bb8>] (SyS_write) from [<c000e620>] (ret_fast_syscall+0x0/0x30)
> [  196.695716] Code: e12fff1e e92d4070 e59f60a8 e5963000 (e5932024) 
> [  196.695722] ---[ end trace 606f168631f0e32f ]---

Re: [PATCH V5 0/7] cpufreq: suspend early/resume late: dpm_{suspend|resume}()

[Viresh Kumar]

On 15 February 2014 01:52, Stephen Warren <swarren@wwwdotorg.org> wrote:
> BTW, I also happened to test these on a Tegra114 system, on which there
> is no cpufreq driver, and this series (applied on top of commit
> 9398a10cd964 Merge tag 'regulator-v3.14-rc2') causes the following
> during suspend:

Ahh, that's a obvious bug. Following would fix it, thanks:

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index c13c100..2dfbb7e 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1617,6 +1617,9 @@ void cpufreq_suspend(void)
 {
        struct cpufreq_policy *policy;

+       if (!cpufreq_driver)
+               return;
+
        if (!has_target())
                return;

@@ -1645,6 +1648,9 @@ void cpufreq_resume(void)
 {
        struct cpufreq_policy *policy;

+       if (!cpufreq_driver)
+               return;
+
        if (!has_target())
                return;

Re: [PATCH V5 0/7] cpufreq: suspend early/resume late: dpm_{suspend|resume}()

[Stephen Warren]

On 02/16/2014 11:02 PM, Viresh Kumar wrote:
> On 15 February 2014 01:52, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> BTW, I also happened to test these on a Tegra114 system, on which there
>> is no cpufreq driver, and this series (applied on top of commit
>> 9398a10cd964 Merge tag 'regulator-v3.14-rc2') causes the following
>> during suspend:
> 
> Ahh, that's a obvious bug. Following would fix it, thanks:

Yes, I tested V6 of the patch series, which incorporates that change,
and it's OK.

Re: [PATCH V5 0/7] cpufreq: suspend early/resume late: dpm_{suspend|resume}()

[Rafael J. Wysocki]

On Friday, February 14, 2014 12:42:53 PM Stephen Warren wrote:
> On 02/12/2014 11:50 PM, Viresh Kumar wrote:
> > This patchset creates/calls cpufreq suspend/resume callbacks from dpm_{suspend|resume}()
> > for handling suspend/resume of cpufreq governors and core.
> 
> Are these patches for 3.14 or 3.15?

I don't think they are suitable for 3.14, honestly.

> I ask because I just tested Linus's master from a few days back (some
> point after v3.14-rc2; commit 9398a10cd964 Merge tag
> 'regulator-v3.14-rc2'), and I see a lot of the following messages during
> suspend and/or resume (about 2-7 times, perhaps more of them from the
> resume path, but it's hard to tell):
> 
> cpufreq: __cpufreq_driver_target: Failed to change cpu frequency: -16
> 
> This series does appear to solve those, so I think at least part of it
> needs to be applied for 3.14.

Well, it would be good to verify which part, then.

> Also, I sometimes see the following during resume. I saw it twice with
> Linus's tree, but then I did 10 more reboot+suspend+resume cycles and
> couldn't repro it, and I saw it once with Linus's tree plus this series
> applied, then couldn't reproduce it in 5 more tries.
> 
> > [ 48.500410] ------------[ cut here ]------------
> > [ 48.505252] WARNING: CPU: 0 PID: 877 at fs/sysfs/dir.c:52 sysfs_warn_dup+0x68/0x84()
> > [ 48.513005] sysfs: cannot create duplicate filename '/devices/system/cpu/cpu1/cpufreq'
> > [ 48.520995] Modules linked in: brcmfmac brcmutil
> > [ 48.525740] CPU: 0 PID: 877 Comm: test-rtc-resume Not tainted 3.14.0-rc2-00259-g9398a10cd964 #12
> > [ 48.534645] [<c0015bac>] (unwind_backtrace) from [<c0011850>] (show_stack+0x10/0x14)
> > [ 48.542440] [<c0011850>] (show_stack) from [<c056e018>] (dump_stack+0x80/0xcc)
> > [ 48.549757] [<c056e018>] (dump_stack) from [<c0025e44>] (warn_slowpath_common+0x64/0x88)
> > [ 48.557964] [<c0025e44>] (warn_slowpath_common) from [<c0025efc>] (warn_slowpath_fmt+0x30/0x40)
> > [ 48.566756] [<c0025efc>] (warn_slowpath_fmt) from [<c012776c>] (sysfs_warn_dup+0x68/0x84)
> > [ 48.575024] [<c012776c>] (sysfs_warn_dup) from [<c0127a54>] (sysfs_do_create_link_sd+0xb0/0xb8)
> > [ 48.583772] [<c0127a54>] (sysfs_do_create_link_sd) from [<c038ef64>] (__cpufreq_add_dev.isra.27+0x2a8/0x814)
> > [ 48.593701] [<c038ef64>] (__cpufreq_add_dev.isra.27) from [<c038f548>] (cpufreq_cpu_callback+0x70/0x8c)
> > [ 48.603198] [<c038f548>] (cpufreq_cpu_callback) from [<c0043864>] (notifier_call_chain+0x44/0x84)
> > [ 48.612166] [<c0043864>] (notifier_call_chain) from [<c0025f60>] (__cpu_notify+0x28/0x44)
> > [ 48.620424] [<c0025f60>] (__cpu_notify) from [<c00261e8>] (_cpu_up+0xf0/0x140)
> > [ 48.627748] [<c00261e8>] (_cpu_up) from [<c0569eb8>] (enable_nonboot_cpus+0x68/0xb0)
> > [ 48.635579] [<c0569eb8>] (enable_nonboot_cpus) from [<c006339c>] (suspend_devices_and_enter+0x198/0x2dc)
> > [ 48.645136] [<c006339c>] (suspend_devices_and_enter) from [<c0063654>] (pm_suspend+0x174/0x1e8)
> > [ 48.653862] [<c0063654>] (pm_suspend) from [<c00624e0>] (state_store+0x6c/0xbc)
> > [ 48.661258] [<c00624e0>] (state_store) from [<c01fc200>] (kobj_attr_store+0x14/0x20)
> > [ 48.669083] [<c01fc200>] (kobj_attr_store) from [<c0126e50>] (sysfs_kf_write+0x44/0x48)
> > [ 48.677163] [<c0126e50>] (sysfs_kf_write) from [<c012a274>] (kernfs_fop_write+0xb4/0x14c)
> > [ 48.685432] [<c012a274>] (kernfs_fop_write) from [<c00d4818>] (vfs_write+0xa8/0x180)
> > [ 48.693214] [<c00d4818>] (vfs_write) from [<c00d4bb8>] (SyS_write+0x3c/0x70)
> > [ 48.700349] [<c00d4bb8>] (SyS_write) from [<c000e620>] (ret_fast_syscall+0x0/0x30)
> > [ 48.708053] ---[ end trace 76969904b614c18f ]--- 
> 
> Do you have any idea what the problem might be, and how to solve it?

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

Re: [PATCH V5 0/7] cpufreq: suspend early/resume late: dpm_{suspend|resume}()

[Stephen Warren]

On 02/14/2014 03:23 PM, Rafael J. Wysocki wrote:
> On Friday, February 14, 2014 12:42:53 PM Stephen Warren wrote:
>> On 02/12/2014 11:50 PM, Viresh Kumar wrote:
>>> This patchset creates/calls cpufreq suspend/resume callbacks from dpm_{suspend|resume}()
>>> for handling suspend/resume of cpufreq governors and core.
>>
>> Are these patches for 3.14 or 3.15?
> 
> I don't think they are suitable for 3.14, honestly.
> 
>> I ask because I just tested Linus's master from a few days back (some
>> point after v3.14-rc2; commit 9398a10cd964 Merge tag
>> 'regulator-v3.14-rc2'), and I see a lot of the following messages during
>> suspend and/or resume (about 2-7 times, perhaps more of them from the
>> resume path, but it's hard to tell):
>>
>> cpufreq: __cpufreq_driver_target: Failed to change cpu frequency: -16
>>
>> This series does appear to solve those, so I think at least part of it
>> needs to be applied for 3.14.
> 
> Well, it would be good to verify which part, then.

Patch 2/7 appears to stop that message from being printed during
suspend, and perhaps reduce the number of times it's printed during
resume. Patch 7/7 stops the message being printed at all.

Looking at patch 7, I wonder if it's simply because tegra_target() was
modified never to return -EBUSY, so the bug is still there, but it's
just been hidden.

>> Also, I sometimes see the following during resume. I saw it twice with
>> Linus's tree, but then I did 10 more reboot+suspend+resume cycles and
>> couldn't repro it, and I saw it once with Linus's tree plus this series
>> applied, then couldn't reproduce it in 5 more tries.

Oops. I screwed up my re-testing (tested on the wrong board, without
cpufreq active:-/). The message below is reproducible 100% of the time
with or without this series.

>>> [ 48.500410] ------------[ cut here ]------------
>>> [ 48.505252] WARNING: CPU: 0 PID: 877 at fs/sysfs/dir.c:52 sysfs_warn_dup+0x68/0x84()
>>> [ 48.513005] sysfs: cannot create duplicate filename '/devices/system/cpu/cpu1/cpufreq'
>>> [ 48.520995] Modules linked in: brcmfmac brcmutil
>>> [ 48.525740] CPU: 0 PID: 877 Comm: test-rtc-resume Not tainted 3.14.0-rc2-00259-g9398a10cd964 #12
...
>>> [ 48.575024] [<c012776c>] (sysfs_warn_dup) from [<c0127a54>] (sysfs_do_create_link_sd+0xb0/0xb8)
>>> [ 48.583772] [<c0127a54>] (sysfs_do_create_link_sd) from [<c038ef64>] (__cpufreq_add_dev.isra.27+0x2a8/0x814)
>>> [ 48.593701] [<c038ef64>] (__cpufreq_add_dev.isra.27) from [<c038f548>] (cpufreq_cpu_callback+0x70/0x8c)
>>> [ 48.603198] [<c038f548>] (cpufreq_cpu_callback) from [<c0043864>] (notifier_call_chain+0x44/0x84)
>>> [ 48.612166] [<c0043864>] (notifier_call_chain) from [<c0025f60>] (__cpu_notify+0x28/0x44)
>>> [ 48.620424] [<c0025f60>] (__cpu_notify) from [<c00261e8>] (_cpu_up+0xf0/0x140)
>>> [ 48.627748] [<c00261e8>] (_cpu_up) from [<c0569eb8>] (enable_nonboot_cpus+0x68/0xb0)
>>> [ 48.635579] [<c0569eb8>] (enable_nonboot_cpus) from [<c006339c>] (suspend_devices_and_enter+0x198/0x2dc)

Re: [PATCH V5 0/7] cpufreq: suspend early/resume late: dpm_{suspend|resume}()

[Viresh Kumar]

On 15 February 2014 05:33, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 02/14/2014 03:23 PM, Rafael J. Wysocki wrote:

>> Well, it would be good to verify which part, then.
>
> Patch 2/7 appears to stop that message from being printed during
> suspend, and perhaps reduce the number of times it's printed during
> resume. Patch 7/7 stops the message being printed at all.
>
> Looking at patch 7, I wonder if it's simply because tegra_target() was
> modified never to return -EBUSY, so the bug is still there, but it's
> just been hidden.

No, the bug is removed now. Its hidden in current linus/master :)

>>> Also, I sometimes see the following during resume. I saw it twice with
>>> Linus's tree, but then I did 10 more reboot+suspend+resume cycles and
>>> couldn't repro it, and I saw it once with Linus's tree plus this series
>>> applied, then couldn't reproduce it in 5 more tries.
>
> Oops. I screwed up my re-testing (tested on the wrong board, without
> cpufreq active:-/). The message below is reproducible 100% of the time
> with or without this series.

Somehow I missed it. Following will fix it, sending a separate patch for it as
well:

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 2dfbb7e..48315e0 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1326,8 +1326,7 @@ static int __cpufreq_remove_dev_prepare(struct
device *dev,
        up_read(&policy->rwsem);

        if (cpu != policy->cpu) {
-               if (!frozen)
-                       sysfs_remove_link(&dev->kobj, "cpufreq");
+               sysfs_remove_link(&dev->kobj, "cpufreq");
        } else if (cpus > 1) {
                new_cpu = cpufreq_nominate_new_policy_cpu(policy, cpu);
                if (new_cpu >= 0) {

Re: [PATCH V5 0/7] cpufreq: suspend early/resume late: dpm_{suspend|resume}()

[Stephen Warren]

On 02/17/2014 02:20 AM, Viresh Kumar wrote:
> On 15 February 2014 05:33, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 02/14/2014 03:23 PM, Rafael J. Wysocki wrote:
> 
>>> Well, it would be good to verify which part, then.
>>
>> Patch 2/7 appears to stop that message from being printed during
>> suspend, and perhaps reduce the number of times it's printed during
>> resume. Patch 7/7 stops the message being printed at all.
>>
>> Looking at patch 7, I wonder if it's simply because tegra_target() was
>> modified never to return -EBUSY, so the bug is still there, but it's
>> just been hidden.
> 
> No, the bug is removed now. Its hidden in current linus/master :)

I'm not sure what that means; I still see the message:

cpufreq: __cpufreq_driver_target: Failed to change cpu frequency: -16

... in Linus's tree (805937cf45f9 "Merge tag 'ext4_for_linus_stable' of
git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4").

Re: [PATCH V5 0/7] cpufreq: suspend early/resume late: dpm_{suspend|resume}()

[Viresh Kumar]

On 19-Feb-2014 1:48 AM, "Stephen Warren" <swarren@wwwdotorg.org> wrote:
>
> On 02/17/2014 02:20 AM, Viresh Kumar wrote:
> > On 15 February 2014 05:33, Stephen Warren <swarren@wwwdotorg.org> wrote:
> >> On 02/14/2014 03:23 PM, Rafael J. Wysocki wrote:
> >
> >>> Well, it would be good to verify which part, then.
> >>
> >> Patch 2/7 appears to stop that message from being printed during
> >> suspend, and perhaps reduce the number of times it's printed during
> >> resume. Patch 7/7 stops the message being printed at all.
> >>
> >> Looking at patch 7, I wonder if it's simply because tegra_target() was
> >> modified never to return -EBUSY, so the bug is still there, but it's
> >> just been hidden.
> >
> > No, the bug is removed now. Its hidden in current linus/master :)
>
> I'm not sure what that means; I still see the message:

I have given a better reply in one of the earlier mails in this thread.
And skipped a more elaborative reply now.

So this failure was always there since long time, as you disable your
target() fn early in suspend. But the message wasn't printed earlier.

A recently added core patch started printing this, so not a new bug.
But this series fixes suspend resume completely and you wouldn't see it
anymore.

Re: [PATCH V5 0/7] cpufreq: suspend early/resume late: dpm_{suspend|resume}()

[Stephen Warren]

On 02/18/2014 09:15 PM, Viresh Kumar wrote:
> On 19-Feb-2014 1:48 AM, "Stephen Warren" <swarren@wwwdotorg.org> wrote:
>>
>> On 02/17/2014 02:20 AM, Viresh Kumar wrote:
>>> On 15 February 2014 05:33, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>> On 02/14/2014 03:23 PM, Rafael J. Wysocki wrote:
>>>
>>>>> Well, it would be good to verify which part, then.
>>>>
>>>> Patch 2/7 appears to stop that message from being printed during
>>>> suspend, and perhaps reduce the number of times it's printed during
>>>> resume. Patch 7/7 stops the message being printed at all.
>>>>
>>>> Looking at patch 7, I wonder if it's simply because tegra_target() was
>>>> modified never to return -EBUSY, so the bug is still there, but it's
>>>> just been hidden.
>>>
>>> No, the bug is removed now. Its hidden in current linus/master :)
>>
>> I'm not sure what that means; I still see the message:
> 
> I have given a better reply in one of the earlier mails in this thread.
> And skipped a more elaborative reply now.
> 
> So this failure was always there since long time, as you disable your
> target() fn early in suspend. But the message wasn't printed earlier.
> 
> A recently added core patch started printing this, so not a new bug.
> But this series fixes suspend resume completely and you wouldn't see it
> anymore.

OK, so I suppose we have two options:

a) Just ignore the kernel error spew since it's a known issue.

b) If I make the Tegra driver return 0 rather than -EBUSY, would that
work? It would certainly silence the error. However, I wonder if it
would cause the cpufreq core to get out of sync with HW; the core would
think that it'd set some frequency, which the driver ignored, and if it
later wanted to switch frequency, the call might get skipped because the
core thought the HW was already set to that frequency?

Re: [PATCH V5 0/7] cpufreq: suspend early/resume late: dpm_{suspend|resume}()

[Linaro]

> On 19-Feb-2014, at 10:56 pm, Stephen Warren <swarren@wwwdotorg.org> wrote:
> 
>> On 02/18/2014 09:15 PM, Viresh Kumar wrote:
>>> On 19-Feb-2014 1:48 AM, "Stephen Warren" <swarren@wwwdotorg.org> wrote:
>>> 
>>>> On 02/17/2014 02:20 AM, Viresh Kumar wrote:
>>>>> On 15 February 2014 05:33, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>>> On 02/14/2014 03:23 PM, Rafael J. Wysocki wrote:
>>>> 
>>>>>> Well, it would be good to verify which part, then.
>>>>> 
>>>>> Patch 2/7 appears to stop that message from being printed during
>>>>> suspend, and perhaps reduce the number of times it's printed during
>>>>> resume. Patch 7/7 stops the message being printed at all.
>>>>> 
>>>>> Looking at patch 7, I wonder if it's simply because tegra_target() was
>>>>> modified never to return -EBUSY, so the bug is still there, but it's
>>>>> just been hidden.
>>>> 
>>>> No, the bug is removed now. Its hidden in current linus/master :)
>>> 
>>> I'm not sure what that means; I still see the message:
>> 
>> I have given a better reply in one of the earlier mails in this thread.
>> And skipped a more elaborative reply now.
>> 
>> So this failure was always there since long time, as you disable your
>> target() fn early in suspend. But the message wasn't printed earlier.
>> 
>> A recently added core patch started printing this, so not a new bug.
>> But this series fixes suspend resume completely and you wouldn't see it
>> anymore.
> 
> OK, so I suppose we have two options:
> 
> a) Just ignore the kernel error spew since it's a known issue.
> 
> b) If I make the Tegra driver return 0 rather than -EBUSY, would that
> work? It would certainly silence the error. However, I wonder if it
> would cause the cpufreq core to get out of sync with HW; the core would
> think that it'd set some frequency, which the driver ignored, and if it
> later wanted to switch frequency, the call might get skipped because the
> core thought the HW was already set to that frequency?

Option is the one you need.

Re: [PATCH V5 0/7] cpufreq: suspend early/resume late: dpm_{suspend|resume}()

[Linaro]

> On 20-Feb-2014, at 7:19 am, Linaro <viresh.kumar@linaro.org> wrote:
> 
> 
> 
>>> On 19-Feb-2014, at 10:56 pm, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> 
>>>> On 02/18/2014 09:15 PM, Viresh Kumar wrote:
>>>>> On 19-Feb-2014 1:48 AM, "Stephen Warren" <swarren@wwwdotorg.org> wrote:
>>>>> 
>>>>>> On 02/17/2014 02:20 AM, Viresh Kumar wrote:
>>>>>> On 15 February 2014 05:33, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>>>> On 02/14/2014 03:23 PM, Rafael J. Wysocki wrote:
>>>>> 
>>>>>>> Well, it would be good to verify which part, then.
>>>>>> 
>>>>>> Patch 2/7 appears to stop that message from being printed during
>>>>>> suspend, and perhaps reduce the number of times it's printed during
>>>>>> resume. Patch 7/7 stops the message being printed at all.
>>>>>> 
>>>>>> Looking at patch 7, I wonder if it's simply because tegra_target() was
>>>>>> modified never to return -EBUSY, so the bug is still there, but it's
>>>>>> just been hidden.
>>>>> 
>>>>> No, the bug is removed now. Its hidden in current linus/master :)
>>>> 
>>>> I'm not sure what that means; I still see the message:
>>> 
>>> I have given a better reply in one of the earlier mails in this thread.
>>> And skipped a more elaborative reply now.
>>> 
>>> So this failure was always there since long time, as you disable your
>>> target() fn early in suspend. But the message wasn't printed earlier.
>>> 
>>> A recently added core patch started printing this, so not a new bug.
>>> But this series fixes suspend resume completely and you wouldn't see it
>>> anymore.
>> 
>> OK, so I suppose we have two options:
>> 
>> a) Just ignore the kernel error spew since it's a known issue.
>> 
>> b) If I make the Tegra driver return 0 rather than -EBUSY, would that
>> work? It would certainly silence the error. However, I wonder if it
>> would cause the cpufreq core to get out of sync with HW; the core would
>> think that it'd set some frequency, which the driver ignored, and if it
>> later wanted to switch frequency, the call might get skipped because the
>> core thought the HW was already set to that frequency?
> 
> Option is the one you need.

Option a..

Re: [PATCH V5 0/7] cpufreq: suspend early/resume late: dpm_{suspend|resume}()

[Stephen Warren]

On 02/19/2014 06:50 PM, Linaro wrote:
> 
> 
>> On 20-Feb-2014, at 7:19 am, Linaro <viresh.kumar@linaro.org> wrote:
>>
>>
>>
>>>> On 19-Feb-2014, at 10:56 pm, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>>
>>>>> On 02/18/2014 09:15 PM, Viresh Kumar wrote:
>>>>>> On 19-Feb-2014 1:48 AM, "Stephen Warren" <swarren@wwwdotorg.org> wrote:
>>>>>>
>>>>>>> On 02/17/2014 02:20 AM, Viresh Kumar wrote:
>>>>>>> On 15 February 2014 05:33, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>>>>> On 02/14/2014 03:23 PM, Rafael J. Wysocki wrote:
>>>>>>
>>>>>>>> Well, it would be good to verify which part, then.
>>>>>>>
>>>>>>> Patch 2/7 appears to stop that message from being printed during
>>>>>>> suspend, and perhaps reduce the number of times it's printed during
>>>>>>> resume. Patch 7/7 stops the message being printed at all.
>>>>>>>
>>>>>>> Looking at patch 7, I wonder if it's simply because tegra_target() was
>>>>>>> modified never to return -EBUSY, so the bug is still there, but it's
>>>>>>> just been hidden.
>>>>>>
>>>>>> No, the bug is removed now. Its hidden in current linus/master :)
>>>>>
>>>>> I'm not sure what that means; I still see the message:
>>>>
>>>> I have given a better reply in one of the earlier mails in this thread.
>>>> And skipped a more elaborative reply now.
>>>>
>>>> So this failure was always there since long time, as you disable your
>>>> target() fn early in suspend. But the message wasn't printed earlier.
>>>>
>>>> A recently added core patch started printing this, so not a new bug.
>>>> But this series fixes suspend resume completely and you wouldn't see it
>>>> anymore.
>>>
>>> OK, so I suppose we have two options:
>>>
>>> a) Just ignore the kernel error spew since it's a known issue.
>>>
>>> b) If I make the Tegra driver return 0 rather than -EBUSY, would that
>>> work? It would certainly silence the error. However, I wonder if it
>>> would cause the cpufreq core to get out of sync with HW; the core would
>>> think that it'd set some frequency, which the driver ignored, and if it
>>> later wanted to switch frequency, the call might get skipped because the
>>> core thought the HW was already set to that frequency?
>>
>> Option is the one you need.
> 
> Option a..

Well, except that still leaves a bunch of errors in the kernel log, and
I have to remember to ignore them:-/

It'd be nice if the cpufreq core didn't keep changing its behaviour and
adding new error prints. It really should be up to the cpufreq drivers
to log the errors if they experience any.

Re: [PATCH V5 0/7] cpufreq: suspend early/resume late: dpm_{suspend|resume}()

[Viresh Kumar]

On 20 February 2014 23:10, Stephen Warren <swarren@wwwdotorg.org> wrote:
> Well, except that still leaves a bunch of errors in the kernel log, and
> I have to remember to ignore them:-/

Just for few releases, before this patchset goes in.

> It'd be nice if the cpufreq core didn't keep changing its behaviour and
> adding new error prints. It really should be up to the cpufreq drivers
> to log the errors if they experience any.

Hmm... not sure.. Its better to do error prints at a single place, i.e. cpufreq
core on behalf of all drivers. If there is a error being returned from some
routine, we better print a message for that. Rather than living in the illusion
that everything is fine :)

Re: [PATCH V5 0/7] cpufreq: suspend early/resume late: dpm_{suspend|resume}()

[Stephen Warren]

On 02/23/2014 11:43 PM, Viresh Kumar wrote:
> On 20 February 2014 23:10, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> Well, except that still leaves a bunch of errors in the kernel log, and
>> I have to remember to ignore them:-/
> 
> Just for few releases, before this patchset goes in.
> 
>> It'd be nice if the cpufreq core didn't keep changing its behaviour and
>> adding new error prints. It really should be up to the cpufreq drivers
>> to log the errors if they experience any.
> 
> Hmm... not sure.. Its better to do error prints at a single place, i.e. cpufreq
> core on behalf of all drivers. If there is a error being returned from some
> routine, we better print a message for that. Rather than living in the illusion
> that everything is fine :)

As a general rule, I think it's better to report the error right where
it happened. That's the only place where the code knows exactly what the
error was. Further up the call stack, we have to guess why there was an
error from the return code, which isn't very much information.

Re: [PATCH V5 0/7] cpufreq: suspend early/resume late: dpm_{suspend|resume}()

[Viresh Kumar]

On 15 February 2014 01:12, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 02/12/2014 11:50 PM, Viresh Kumar wrote:
>> This patchset creates/calls cpufreq suspend/resume callbacks from dpm_{suspend|resume}()
>> for handling suspend/resume of cpufreq governors and core.
>
> Are these patches for 3.14 or 3.15?

3.15

> I ask because I just tested Linus's master from a few days back (some
> point after v3.14-rc2; commit 9398a10cd964 Merge tag
> 'regulator-v3.14-rc2'), and I see a lot of the following messages during
> suspend and/or resume (about 2-7 times, perhaps more of them from the
> resume path, but it's hard to tell):
>
> cpufreq: __cpufreq_driver_target: Failed to change cpu frequency: -16

That's because you return -EBUSY from your tegra_cpu_init() once
suspend/resume has started. Probably the print message is new here,
and so you are able to see it failing.. Nothing else should have changed
otherwise.

> This series does appear to solve those, so I think at least part of it
> needs to be applied for 3.14.

Because we disallow calls to __cpufreq_driver_target() after suspend/
resume have started.

> Also, I sometimes see the following during resume. I saw it twice with
> Linus's tree, but then I did 10 more reboot+suspend+resume cycles and
> couldn't repro it, and I saw it once with Linus's tree plus this series
> applied, then couldn't reproduce it in 5 more tries.

> Do you have any idea what the problem might be, and how to solve it?

Not yet, will check..