[PATCHES] cpufreq_get(), cpufreq->get()

[PATCHES] cpufreq_get(), cpufreq->get()

[Dominik Brodowski]

[-- Attachment #1.1: Type: text/plain, Size: 9918 bytes --]

Dear all,

The following patchset is based on the current cpufreq bitkeeper patch
and one important speedstep-centrino patch 
[see http://www.linux.org.uk/mailman/private/cpufreq/2004-April/003499.html ].

Its aim is to provide
a) better user and kernel information about current frequency,
b) handle frequency changes "behind cpufreq's back" (e.g. by BIOS)
   better, and
c) fix up suspend/resume.

I suggest that this patchset is considered for the 2.6.7 timeframe; 
while the x86 parts seem to be quite straightforward, the arm 
architecture might be affected more heavily: it already relied on 
cpufreq_get(), and I can't verify my changes do even compile. Russell, 
I hope I didn't break anything this time...

The patches described below [02, 04, 05 and 06 will also be sent to the
cpufreq list, the others are only available online] are partly based on
the suggestion by Bruno Ducrot 
[see http://www.linux.org.uk/mailman/private/cpufreq/2004-January/003076.html ]. 

In future, maybe even still in time for 2.6.7, verification inside 
cpufreq_notify_transition (cpu_policy->cur != freqs.old), and a call 
to cpufreq_get() before the "lost tick detection" mechanism kicks in, 
might be useful work on top of these patches.

The x86 patches are split up now for reviewal / bugfixing reasons now,
but could be merged into one cset for bk before merging. Same is true
for patches 05 and 06; all others are different "steps" and should stay
different csets, IMO.

WRT other architectures than arm and x86: I couldn't find an easy way
to add a hardware-based "get-frequency()" function to the sh, ppc and
sparc64 cpufreq drivers, but I didn't look all that hard... so maybe 
the respective driver maintainers can take a look at this? Thanks.





http://www.brodo.de/patches/2004-04-11/01-export_scaling_cur_freq

Many users want to know the current cpu freqeuncy, even if not using
the userspace frequency. On ->target cpufreq drivers (if they do their
calls to cpufreq_notify_transition correctly) this just means reading
out cpufreq_policy->cur.

 drivers/cpufreq/cpufreq.c |    4 ++++
 1 files changed, 4 insertions(+)

http://www.brodo.de/patches/2004-04-11/02-cpufreq_get

Move cpufreq_get() from the userspace governor to the core. Contrary to the
previous implementation, it now calls the cpufreq driver, and reads out the
_actual_ current frequency, and not the frequency the CPUfreq core _thinks_
the CPU is running at. Most cpufreq drivers do provide such a "hw get"
function (only ACPI-io can definitely not be supported, I'm not sure
about sh, sparc64 and powermac) anyway, and it is useful for other issues
(see following patches).

 drivers/cpufreq/cpufreq.c           |   32 +++++++++++++++++++++++++++
 drivers/cpufreq/cpufreq_userspace.c |   42 ++----------------------------------
 include/linux/cpufreq.h             |    9 +++++--
 3 files changed, 41 insertions(+), 42 deletions(-)

http://www.brodo.de/patches/2004-04-11/03-cpuinfo_cur_freq

Export cpufreq_get() to userspace. As it involves calls to hardware which might
take some time, only let the super-user read out this value.

 drivers/cpufreq/cpufreq.c |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+)

http://www.brodo.de/patches/2004-04-11/04-out_of_sync

Sometimes we might discover during a call to cpufreq_get() that we're "out of sync",
meaning the actual CPU frequency changed "behind our back". If this happens, the
flag CPUFREQ_PANIC_OUTOFSYNC decides what can be done: if it is set, the kernel panic's,
it it is not set, the cpufreq transition notifiers are informed of this change, and
a call to cpufreq_update_policy() is scheduled [using the default workqueue] so that
the user-defined values override BIOS / external interaction.

 drivers/cpufreq/cpufreq.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/cpufreq.h   |   15 +++++++++++--
 2 files changed, 63 insertions(+), 2 deletions(-)

http://www.brodo.de/patches/2004-04-11/05-resume

(Hopefully) fix cpufreq resume support.

Upon resuming, first CPUfreq hardware support needs to be re-enabled in ceratin cases
(call to cpufreq_driver->resume()).

Then, two different paths may need to be taken:
a) frequency during suspend equals frequency during resume ==> everything is fine,

b) frequency differ ==> either we can't handle it, then panic (see flag 
   CPUFREQ_PANIC_RESUME_OUTOFSYNC). Or we can handle it, then notify all
   transition notifiers of this frequency change (CPUFREQ_RESUMECHANGE so that they
   know IRQs are disabled).

At last, a call to cpufreq_update_policy() is scheduled. This asserts that
that the policy can be updated soon, in case platform limit, thermal and/or other 
issues make an adjustment necessary.

 Documentation/cpu-freq/core.txt |    4 ++
 drivers/cpufreq/cpufreq.c       |   63 ++++++++++++++++++++++++----------------
 include/linux/cpufreq.h         |    5 +++
 3 files changed, 47 insertions(+), 25 deletions(-)

http://www.brodo.de/patches/2004-04-11/06-notifieres

Handle CPUFREQ_RESUMECHANGE notifications in i386, sparc64, x86_64, sh-sci
and sa11xx-pcmcia notifiers. sa1100-framebuffer doesn't seem to be able
to handle frequency transitions behind its back well. So, sa11xx will be marked
CPUFREQ_PANIC_OUTOFSYNC | CPUFREQ_PANIC_RESUME_OUTOFSYNC in patch 19. Russell,
do you think sa1100fb will be able to handle such an out of sync situation?

 arch/i386/kernel/timers/timer_tsc.c |    3 ++-
 arch/sparc64/kernel/time.c          |    3 ++-
 arch/x86_64/kernel/time.c           |    3 ++-
 drivers/char/sh-sci.c               |    3 ++-
 drivers/pcmcia/sa11xx_core.c        |    2 ++
 drivers/serial/sh-sci.c             |    3 ++-
 6 files changed, 12 insertions(+), 5 deletions(-)

http://www.brodo.de/patches/2004-04-11/07-elanfreq-get

use elanfreq's internal get function as ->get()

 arch/i386/kernel/cpu/cpufreq/elanfreq.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

http://www.brodo.de/patches/2004-04-11/08-gx-suspmod-get

use gx-suspmod's internal get function as ->get()

 arch/i386/kernel/cpu/cpufreq/gx-suspmod.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

http://www.brodo.de/patches/2004-04-11/09-longhaul-get

Add a longhaul_get function. It was easy to add as all necessary
parts were already there...

 arch/i386/kernel/cpu/cpufreq/longhaul.c |    8 ++++++++
 1 files changed, 8 insertions(+)

http://www.brodo.de/patches/2004-04-11/10-longrun-get

Longrun users might be interested in their CPU's current frequency as
well, so use a longrun-specific cpuid-call in longrun_get().

 arch/i386/kernel/cpu/cpufreq/longrun.c |   13 +++++++++++++
 1 files changed, 13 insertions(+)

http://www.brodo.de/patches/2004-04-11/11-p4-clockmod-get

p4-clockmod is a bit more complicated as it might run on SMP, HT, and 
the instructions need to run on the specific (physical) CPU.

 arch/i386/kernel/cpu/cpufreq/p4-clockmod.c |   48 +++++++++++++++++++++++++++--
 1 files changed, 46 insertions(+), 2 deletions(-)

http://www.brodo.de/patches/2004-04-11/12-powernow-k6-get

powernow_k6 has almost all pieces in place for its own ->get() function.
Add the rest.

 arch/i386/kernel/cpu/cpufreq/powernow-k6.c |    6 ++++++
 1 files changed, 6 insertions(+)

http://www.brodo.de/patches/2004-04-11/13-powernow-k7-get

powernow-k7 has almost all pieces necessary for ->get() in place. Add
the rest, and use it in powernow_cpu_init().

 arch/i386/kernel/cpu/cpufreq/powernow-k7.c |   17 ++++++++++++++++-
 1 files changed, 16 insertions(+), 1 deletion(-)

http://www.brodo.de/patches/2004-04-11/14-powernow-k8-get

Add powernowk8_get, but be careful as some code needs to run on
specified CPU only.

 arch/i386/kernel/cpu/cpufreq/powernow-k8.c |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+)

http://www.brodo.de/patches/2004-04-11/15-speedstep_centrino_get

use speedstep_centrino's internal get function as ->get()

 arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

http://www.brodo.de/patches/2004-04-11/16-speedstep-ich-get

Use speedstep_lib's capabilites for ->get() in speedstep-ich.c

 arch/i386/kernel/cpu/cpufreq/speedstep-ich.c |    5 +++++
 1 files changed, 5 insertions(+)

http://www.brodo.de/patches/2004-04-11/17-speedstep-smi-get

Use speedstep_lib's capabilites for ->get() in speedstep-smi.c

 arch/i386/kernel/cpu/cpufreq/speedstep-smi.c |   20 +++++++++++++++++---
 1 files changed, 17 insertions(+), 3 deletions(-)

http://www.brodo.de/patches/2004-04-11/18-arm-integrator-get

arm-integrator had its ->get() implementation inside 
integratior_cpufreq_init(). Move it to an extra function,
and add it as ->get() function.

 arch/arm/mach-integrator/cpu.c |   20 ++++++++++++++------
 1 files changed, 14 insertions(+), 6 deletions(-)

http://www.brodo.de/patches/2004-04-11/19-arm-sa11xx-get

sa11x0_getspeed can be used by both cpu-sa1100.c and cpu-sa1110.c as
->get() function. Update calling conventions, and un-export it as
we fixed the handling of cpufreq_get in the cpufreq core. Also,
remove special call to userspace-governor init as it isn't needed
any longer.

 arch/arm/mach-sa1100/cpu-sa1100.c |    8 +++++---
 arch/arm/mach-sa1100/cpu-sa1110.c |    9 +++++----
 arch/arm/mach-sa1100/generic.c    |    6 ++++--
 arch/arm/mach-sa1100/generic.h    |    2 +-
 4 files changed, 15 insertions(+), 10 deletions(-)

http://www.brodo.de/patches/2004-04-11/20-arm-Kconfig

As cpufreq_get() is no longer a part of cpufreq_userspace.c, SA11x0 doesn't
need to be handled specially any longer.

 arch/arm/Kconfig |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 143 bytes --]

_______________________________________________
Cpufreq mailing list
Cpufreq@www.linux.org.uk
http://www.linux.org.uk/mailman/listinfo/cpufreq

patch-02 [Re: [PATCHES] cpufreq_get(), cpufreq->get()]

[Dominik Brodowski]

Move cpufreq_get() from the userspace governor to the core. Contrary to the
previous implementation, it now calls the cpufreq driver, and reads out the
_actual_ current frequency, and not the frequency the CPUfreq core _thinks_
the CPU is running at. Most cpufreq drivers do provide such a "hw get"
function (only ACPI-io can definitely not be supported, I'm not sure
about sh, sparc64 and powermac) anyway, and it is useful for other issues
(see following patches).

 drivers/cpufreq/cpufreq.c           |   32 +++++++++++++++++++++++++++
 drivers/cpufreq/cpufreq_userspace.c |   42 ++----------------------------------
 include/linux/cpufreq.h             |    9 +++++--
 3 files changed, 41 insertions(+), 42 deletions(-)

diff -ruN linux-original/drivers/cpufreq/cpufreq.c linux/drivers/cpufreq/cpufreq.c
--- linux-original/drivers/cpufreq/cpufreq.c	2004-04-11 16:59:29.868192392 +0200
+++ linux/drivers/cpufreq/cpufreq.c	2004-04-11 17:01:30.297884296 +0200
@@ -474,6 +474,38 @@
 	return 0;
 }
 
+
+/** 
+ * cpufreq_get - get the current CPU frequency (in kHz)
+ * @cpu: CPU number
+ *
+ * Get the CPU current (static) CPU frequency
+ */
+unsigned int cpufreq_get(unsigned int cpu)
+{
+	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
+	unsigned int ret = 0;
+
+	if (!policy)
+		return 0;
+
+	if (!cpufreq_driver->get)
+		goto out;
+
+	down(&policy->lock);
+
+	ret = cpufreq_driver->get(cpu);
+
+	up(&policy->lock);
+
+ out:
+	cpufreq_cpu_put(policy);
+
+	return (ret);
+}
+EXPORT_SYMBOL(cpufreq_get);
+
+
 /**
  *	cpufreq_resume - restore the CPU clock frequency after resume
  *
diff -ruN linux-original/drivers/cpufreq/cpufreq_userspace.c linux/drivers/cpufreq/cpufreq_userspace.c
--- linux-original/drivers/cpufreq/cpufreq_userspace.c	2004-04-11 14:48:15.000000000 +0200
+++ linux/drivers/cpufreq/cpufreq_userspace.c	2004-04-11 17:04:23.642531904 +0200
@@ -145,19 +145,6 @@
 EXPORT_SYMBOL_GPL(cpufreq_setmax);
 
 
-/** 
- * cpufreq_get - get the current CPU frequency (in kHz)
- * @cpu: CPU number
- *
- * Get the CPU current (static) CPU frequency
- */
-unsigned int cpufreq_get(unsigned int cpu)
-{
-	return cpu_cur_freq[cpu];
-}
-EXPORT_SYMBOL(cpufreq_get);
-
-
 #ifdef CONFIG_CPU_FREQ_24_API
 
 
@@ -542,20 +529,6 @@
 	return 0;
 }
 
-/* on ARM SA1100 we need to rely on the values of cpufreq_get() - because 
- * of this, cpu_cur_freq[] needs to be set early.
- */
-#if defined(CONFIG_ARM) && defined(CONFIG_ARCH_SA1100)
-extern unsigned int sa11x0_getspeed(void);
-
-static void cpufreq_sa11x0_compat(void)
-{
-	cpu_cur_freq[0] = sa11x0_getspeed();
-}
-#else
-#define cpufreq_sa11x0_compat() do {} while(0)
-#endif
-
 
 struct cpufreq_governor cpufreq_gov_userspace = {
 	.name		= "userspace",
@@ -564,21 +537,12 @@
 };
 EXPORT_SYMBOL(cpufreq_gov_userspace);
 
-static int already_init = 0;
-
-int cpufreq_gov_userspace_init(void)
+static int __init cpufreq_gov_userspace_init(void)
 {
-	if (!already_init) {
-		down(&userspace_sem);
-		cpufreq_sa11x0_compat();
-		cpufreq_sysctl_init();
-		cpufreq_register_notifier(&userspace_cpufreq_notifier_block, CPUFREQ_TRANSITION_NOTIFIER);
-		already_init = 1;
-		up(&userspace_sem);
-	}
+	cpufreq_sysctl_init();
+	cpufreq_register_notifier(&userspace_cpufreq_notifier_block, CPUFREQ_TRANSITION_NOTIFIER);
 	return cpufreq_register_governor(&cpufreq_gov_userspace);
 }
-EXPORT_SYMBOL(cpufreq_gov_userspace_init);
 
 
 static void __exit cpufreq_gov_userspace_exit(void)
diff -ruN linux-original/include/linux/cpufreq.h linux/include/linux/cpufreq.h
--- linux-original/include/linux/cpufreq.h	2004-04-11 14:48:29.000000000 +0200
+++ linux/include/linux/cpufreq.h	2004-04-11 17:04:47.436914608 +0200
@@ -187,6 +187,9 @@
 				 unsigned int target_freq,
 				 unsigned int relation);
 
+	/* should be defined, if possible */
+	unsigned int	(*get)	(unsigned int cpu);
+
 	/* optional */
 	int	(*exit)		(struct cpufreq_policy *policy);
 	int	(*resume)	(struct cpufreq_policy *policy);
@@ -234,6 +237,9 @@
 int cpufreq_get_policy(struct cpufreq_policy *policy, unsigned int cpu);
 int cpufreq_update_policy(unsigned int cpu);
 
+/* query the current CPU frequency (in kHz). If zero, cpufreq couldn't detect it */
+unsigned int cpufreq_get(unsigned int cpu);
+
 /* the proc_intf.c needs this */
 int cpufreq_parse_governor (char *str_governor, unsigned int *policy, struct cpufreq_governor **governor);
 
@@ -241,13 +247,10 @@
 /*********************************************************************
  *                      CPUFREQ USERSPACE GOVERNOR                   *
  *********************************************************************/
-int cpufreq_gov_userspace_init(void);
-
 #ifdef CONFIG_CPU_FREQ_24_API
 
 int cpufreq_setmax(unsigned int cpu);
 int cpufreq_set(unsigned int kHz, unsigned int cpu);
-unsigned int cpufreq_get(unsigned int cpu);
 
 
 /* /proc/sys/cpu */

patch-04 [Re: [PATCHES] cpufreq_get(), cpufreq->get()]

[Dominik Brodowski]

Sometimes we might discover during a call to cpufreq_get() that we're "out of sync",
meaning the actual CPU frequency changed "behind our back". If this happens, the
flag CPUFREQ_PANIC_OUTOFSYNC decides what can be done: if it is set, the kernel panic's,
it it is not set, the cpufreq transition notifiers are informed of this change, and
a call to cpufreq_update_policy() is scheduled [using the default workqueue] so that
the user-defined values override BIOS / external interaction.

 drivers/cpufreq/cpufreq.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/cpufreq.h   |   15 +++++++++++--
 2 files changed, 63 insertions(+), 2 deletions(-)

diff -ruN linux-original/drivers/cpufreq/cpufreq.c linux/drivers/cpufreq/cpufreq.c
--- linux-original/drivers/cpufreq/cpufreq.c	2004-04-11 17:08:00.752526168 +0200
+++ linux/drivers/cpufreq/cpufreq.c	2004-04-11 17:09:54.233274472 +0200
@@ -362,6 +362,7 @@
 	.release	= cpufreq_sysfs_release,
 };
 
+static void handle_update(void *data);
 
 /**
  * cpufreq_add_dev - add a CPU device
@@ -388,6 +389,7 @@
 	policy->cpu = cpu;
 	init_MUTEX_LOCKED(&policy->lock);
 	init_completion(&policy->kobj_unregister);
+	INIT_WORK(&policy->update, handle_update, (void *) cpu);
 
 	/* call driver. From then on the cpufreq must be able
 	 * to accept all calls to ->verify and ->setpolicy for this CPU
@@ -496,6 +498,39 @@
 }
 
 
+static void handle_update(void *data)
+{
+	unsigned int cpu = (unsigned int) data;
+	cpufreq_update_policy(cpu);
+}
+
+/**
+ *	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
+ *	@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, unsigned int new_freq)
+{
+	struct cpufreq_freqs freqs;
+
+	if (cpufreq_driver->flags & CPUFREQ_PANIC_OUTOFSYNC)
+		panic("CPU Frequency is out of sync.");
+
+	printk(KERN_WARNING "Warning: CPU frequency out of sync: cpufreq and timing "
+	       "core thinks of %u, is %u kHz.\n", old_freq, new_freq);
+
+	freqs.cpu = cpu;
+	freqs.old = old_freq;
+	freqs.new = new_freq;
+	cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
+	cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
+}
+
+
 /** 
  * cpufreq_get - get the current CPU frequency (in kHz)
  * @cpu: CPU number
@@ -517,6 +552,15 @@
 
 	ret = cpufreq_driver->get(cpu);
 
+	if (ret && policy->cur && !(cpufreq_driver->flags & CPUFREQ_CONST_LOOPS)) 
+	{
+		/* verify no discrepancy between actual and saved value exists */
+		if (unlikely(ret != policy->cur)) {
+			cpufreq_out_of_sync(cpu, policy->cur, ret);
+			schedule_work(&policy->update);
+		}
+	}
+
 	up(&policy->lock);
 
  out:
@@ -957,6 +1001,9 @@
 
 static inline void adjust_jiffies(unsigned long val, struct cpufreq_freqs *ci)
 {
+	if (cpufreq_driver->flags & CPUFREQ_CONST_LOOPS)
+		return;
+
 	if (!l_p_j_ref_freq) {
 		l_p_j_ref = loops_per_jiffy;
 		l_p_j_ref_freq = ci->old;
@@ -1023,6 +1070,9 @@
 	    ((!driver_data->setpolicy) && (!driver_data->target)))
 		return -EINVAL;
 
+	if (driver_data->setpolicy)
+		driver_data->flags |= CPUFREQ_CONST_LOOPS;
+
 	spin_lock_irqsave(&cpufreq_driver_lock, flags);
 	if (cpufreq_driver) {
 		spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
diff -ruN linux-original/include/linux/cpufreq.h linux/include/linux/cpufreq.h
--- linux-original/include/linux/cpufreq.h	2004-04-11 17:07:17.095163096 +0200
+++ linux/include/linux/cpufreq.h	2004-04-11 17:10:41.321116032 +0200
@@ -21,6 +21,7 @@
 #include <linux/kobject.h>
 #include <linux/sysfs.h>
 #include <linux/completion.h>
+#include <linux/workqueue.h>
 
 #define CPUFREQ_NAME_LEN 16
 
@@ -81,6 +82,9 @@
  	struct semaphore	lock;   /* CPU ->setpolicy or ->target may
 					   only be called once a time */
 
+	struct work_struct	update; /* if update_policy() needs to be
+					 * called, but you're in IRQ context */
+
 	struct cpufreq_real_policy	user_policy;
 
 	struct kobject		kobj;
@@ -198,8 +202,15 @@
 
 /* flags */
 
-#define CPUFREQ_STICKY	0x01	/* the driver isn't removed even if 
-				   all ->init() calls failed */
+#define CPUFREQ_STICKY		0x01	/* the driver isn't removed even if 
+					 * all ->init() calls failed */
+#define CPUFREQ_CONST_LOOPS 	0x02	/* loops_per_jiffy or other kernel
+					 * "constants" aren't affected by
+					 * frequency transitions */
+#define CPUFREQ_PANIC_OUTOFSYNC	0x04	/* panic if cpufreq's opinion of
+					 * current frequency differs from
+					 * actual frequency */
+
 
 int cpufreq_register_driver(struct cpufreq_driver *driver_data);
 int cpufreq_unregister_driver(struct cpufreq_driver *driver_data);

patch-05 [Re: [PATCHES] cpufreq_get(), cpufreq->get()]

[Dominik Brodowski]

(Hopefully) fix cpufreq resume support.

Upon resuming, first CPUfreq hardware support needs to be re-enabled in ceratin cases
(call to cpufreq_driver->resume()).

Then, two different paths may need to be taken:
a) frequency during suspend equals frequency during resume ==> everything is fine,

b) frequency differ ==> either we can't handle it, then panic (see flag 
   CPUFREQ_PANIC_RESUME_OUTOFSYNC). Or we can handle it, then notify all
   transition notifiers of this frequency change (CPUFREQ_RESUMECHANGE so that they
   know IRQs are disabled).

At last, a call to cpufreq_update_policy() is scheduled. This asserts that
that the policy can be updated soon, in case platform limit, thermal and/or other 
issues make an adjustment necessary.

 Documentation/cpu-freq/core.txt |    4 ++
 drivers/cpufreq/cpufreq.c       |   63 ++++++++++++++++++++++++----------------
 include/linux/cpufreq.h         |    5 +++
 3 files changed, 47 insertions(+), 25 deletions(-)

diff -ruN linux-original/Documentation/cpu-freq/core.txt linux/Documentation/cpu-freq/core.txt
--- linux-original/Documentation/cpu-freq/core.txt	2004-04-11 14:47:24.000000000 +0200
+++ linux/Documentation/cpu-freq/core.txt	2004-04-11 17:13:16.571514384 +0200
@@ -92,3 +92,7 @@
 cpu	- number of the affected CPU
 old	- old frequency
 new	- new frequency
+
+If the cpufreq core detects the frequency has changed while the system
+was suspended, these notifiers are called with CPUFREQ_RESUMECHANGE as
+second argument.
diff -ruN linux-original/drivers/cpufreq/cpufreq.c linux/drivers/cpufreq/cpufreq.c
--- linux-original/drivers/cpufreq/cpufreq.c	2004-04-11 17:14:34.836616280 +0200
+++ linux/drivers/cpufreq/cpufreq.c	2004-04-11 17:13:16.572514232 +0200
@@ -33,8 +33,10 @@
 static struct cpufreq_policy	*cpufreq_cpu_data[NR_CPUS];
 static spinlock_t		cpufreq_driver_lock = SPIN_LOCK_UNLOCKED;
 
-/* internal prototype */
+/* internal prototypes */
 static int __cpufreq_governor(struct cpufreq_policy *policy, unsigned int event);
+static void handle_update(void *data);
+static inline void adjust_jiffies(unsigned long val, struct cpufreq_freqs *ci);
 
 
 /**
@@ -362,7 +364,6 @@
 	.release	= cpufreq_sysfs_release,
 };
 
-static void handle_update(void *data);
 
 /**
  * cpufreq_add_dev - add a CPU device
@@ -572,10 +573,11 @@
 
 
 /**
- *	cpufreq_resume - restore the CPU clock frequency after resume
+ *	cpufreq_resume -  restore proper CPU frequency handling after resume
  *
- *	Restore the CPU clock frequency so that our idea of the current
- *	frequency reflects the actual hardware.
+ *	1.) resume CPUfreq hardware support (cpufreq_driver->resume())
+ *	2.) if ->target and !CPUFREQ_CONST_LOOPS: verify we're in sync
+ *	3.) schedule call cpufreq_update_policy() ASAP as interrupts are restored.
  */
 static int cpufreq_resume(struct sys_device * sysdev)
 {
@@ -595,25 +597,37 @@
 	if (!cpu_policy)
 		return -EINVAL;
 
-	if (cpufreq_driver->resume)
-		ret = cpufreq_driver->resume(cpu_policy);
-	if (ret) {
-		printk(KERN_ERR "cpufreq: resume failed in ->resume step on CPU %u\n", cpu_policy->cpu);
-		goto out;
-	}
+	if (!cpufreq_driver->flags & CPUFREQ_CONST_LOOPS) {
+		unsigned int cur_freq = 0;
 
-	if (cpufreq_driver->setpolicy)
-		ret = cpufreq_driver->setpolicy(cpu_policy);
-	else
-		/* CPUFREQ_RELATION_H or CPUFREQ_RELATION_L have the same effect here, as cpu_policy->cur is known
-		 * to be a valid and exact target frequency
-		 */
-		ret = cpufreq_driver->target(cpu_policy, cpu_policy->cur, CPUFREQ_RELATION_H);
+		if (cpufreq_driver->get)
+			cur_freq = cpufreq_driver->get(cpu_policy->cpu);
 
-	if (ret)
-		printk(KERN_ERR "cpufreq: resume failed in ->setpolicy/target step on CPU %u\n", cpu_policy->cpu);
+		if (!cur_freq || !cpu_policy->cur) {
+			printk(KERN_ERR "cpufreq: resume failed to assert current frequency is what timing core thinks it is.\n");
+			goto out;
+		}
+
+		if (unlikely(cur_freq != cpu_policy->cur)) {
+			struct cpufreq_freqs freqs;
+
+			if (cpufreq_driver->flags & CPUFREQ_PANIC_RESUME_OUTOFSYNC)
+				panic("CPU Frequency is out of sync.");
+
+			printk(KERN_WARNING "Warning: CPU frequency out of sync: cpufreq and timing"
+			       "core thinks of %u, is %u kHz.\n", cpu_policy->cur, cur_freq);
+
+			freqs.cpu = cpu;
+			freqs.old = cpu_policy->cur;
+			freqs.new = cur_freq;
+
+			notifier_call_chain(&cpufreq_transition_notifier_list, CPUFREQ_RESUMECHANGE, &freqs);
+			adjust_jiffies(CPUFREQ_RESUMECHANGE, &freqs);
+		}
+	}
 
 out:
+	schedule_work(&cpu_policy->update);
 	cpufreq_cpu_put(cpu_policy);
 	return ret;
 }
@@ -1009,11 +1023,12 @@
 		l_p_j_ref_freq = ci->old;
 	}
 	if ((val == CPUFREQ_PRECHANGE  && ci->old < ci->new) ||
-	    (val == CPUFREQ_POSTCHANGE && ci->old > ci->new))
+	    (val == CPUFREQ_POSTCHANGE && ci->old > ci->new) ||
+	    (val == CPUFREQ_RESUMECHANGE))
 		loops_per_jiffy = cpufreq_scale(l_p_j_ref, l_p_j_ref_freq, ci->new);
 }
 #else
-#define adjust_jiffies(x...) do {} while (0)
+static inline void adjust_jiffies(unsigned long val, struct cpufreq_freqs *ci) { return; }
 #endif
 
 
@@ -1025,9 +1040,7 @@
  */
 void cpufreq_notify_transition(struct cpufreq_freqs *freqs, unsigned int state)
 {
-	if (irqs_disabled())
-		return;   /* Only valid if we're in the resume process where
-			   * everyone knows what CPU frequency we are at */
+	BUG_ON(irqs_disabled());
 
 	down_read(&cpufreq_notifier_rwsem);
 	switch (state) {
diff -ruN linux-original/include/linux/cpufreq.h linux/include/linux/cpufreq.h
--- linux-original/include/linux/cpufreq.h	2004-04-11 17:14:34.837616128 +0200
+++ linux/include/linux/cpufreq.h	2004-04-11 17:13:49.038578640 +0200
@@ -100,6 +100,7 @@
 
 #define CPUFREQ_PRECHANGE	(0)
 #define CPUFREQ_POSTCHANGE	(1)
+#define CPUFREQ_RESUMECHANGE	(8)
 
 struct cpufreq_freqs {
 	unsigned int cpu;	/* cpu nr */
@@ -210,6 +211,10 @@
 #define CPUFREQ_PANIC_OUTOFSYNC	0x04	/* panic if cpufreq's opinion of
 					 * current frequency differs from
 					 * actual frequency */
+#define CPUFREQ_PANIC_RESUME_OUTOFSYNC 0x08 /* panic if cpufreq's opinion of
+					 * current frequency differs from
+					 * actual frequency on resume
+					 * from sleep. */
 
 
 int cpufreq_register_driver(struct cpufreq_driver *driver_data);

patch-06 [Re: [PATCHES] cpufreq_get(), cpufreq->get()]

[Dominik Brodowski]

Handle CPUFREQ_RESUMECHANGE notifications in i386, sparc64, x86_64, sh-sci
and sa11xx-pcmcia notifiers. sa1100-framebuffer doesn't seem to be able
to handle frequency transitions behind its back well. So, sa11xx will be marked
CPUFREQ_PANIC_OUTOFSYNC | CPUFREQ_PANIC_RESUME_OUTOFSYNC in patch 19. Russell,
do you think sa1100fb will be able to handle such an out of sync situation?

 arch/i386/kernel/timers/timer_tsc.c |    3 ++-
 arch/sparc64/kernel/time.c          |    3 ++-
 arch/x86_64/kernel/time.c           |    3 ++-
 drivers/char/sh-sci.c               |    3 ++-
 drivers/pcmcia/sa11xx_core.c        |    2 ++
 drivers/serial/sh-sci.c             |    3 ++-
 6 files changed, 12 insertions(+), 5 deletions(-)


diff -ruN linux-original/arch/i386/kernel/timers/timer_tsc.c linux/arch/i386/kernel/timers/timer_tsc.c
--- linux-original/arch/i386/kernel/timers/timer_tsc.c	2004-04-04 14:13:49.000000000 +0200
+++ linux/arch/i386/kernel/timers/timer_tsc.c	2004-04-11 13:28:50.560219864 +0200
@@ -356,7 +356,8 @@
 	}
 
 	if ((val == CPUFREQ_PRECHANGE  && freq->old < freq->new) ||
-	    (val == CPUFREQ_POSTCHANGE && freq->old > freq->new)) {
+	    (val == CPUFREQ_POSTCHANGE && freq->old > freq->new) ||
+	    (val == CPUFREQ_RESUMECHANGE)) {
 		if (variable_tsc)
 			cpu_data[freq->cpu].loops_per_jiffy = cpufreq_scale(loops_per_jiffy_ref, ref_freq, freq->new);
 #ifndef CONFIG_SMP
diff -ruN linux-original/arch/sparc64/kernel/time.c linux/arch/sparc64/kernel/time.c
--- linux-original/arch/sparc64/kernel/time.c	2004-03-12 22:26:56.000000000 +0100
+++ linux/arch/sparc64/kernel/time.c	2004-04-11 13:29:27.000000000 +0200
@@ -1035,7 +1035,8 @@
 		ft->clock_tick_ref = cpu_data(cpu).clock_tick;
 	}
 	if ((val == CPUFREQ_PRECHANGE  && freq->old < freq->new) ||
-	    (val == CPUFREQ_POSTCHANGE && freq->old > freq->new)) {
+	    (val == CPUFREQ_POSTCHANGE && freq->old > freq->new) ||
+	    (val == CPUFREQ_RESUMECHANGE)) {
 		cpu_data(cpu).udelay_val =
 			cpufreq_scale(ft->udelay_val_ref,
 				      ft->ref_freq,
diff -ruN linux-original/arch/x86_64/kernel/time.c linux/arch/x86_64/kernel/time.c
--- linux-original/arch/x86_64/kernel/time.c	2004-03-12 22:26:57.000000000 +0100
+++ linux/arch/x86_64/kernel/time.c	2004-04-11 13:30:00.000000000 +0200
@@ -531,7 +531,8 @@
 		cpu_khz_ref = cpu_khz;
 	}
         if ((val == CPUFREQ_PRECHANGE  && freq->old < freq->new) ||
-            (val == CPUFREQ_POSTCHANGE && freq->old > freq->new)) {
+            (val == CPUFREQ_POSTCHANGE && freq->old > freq->new) ||
+	    (val == CPUFREQ_RESUMECHANGE)) {
                 *lpj =
 		cpufreq_scale(loops_per_jiffy_ref, ref_freq, freq->new);
 
diff -ruN linux-original/drivers/char/sh-sci.c linux/drivers/char/sh-sci.c
--- linux-original/drivers/char/sh-sci.c	2004-03-12 22:26:59.000000000 +0100
+++ linux/drivers/char/sh-sci.c	2004-04-11 13:30:57.000000000 +0200
@@ -1239,7 +1239,8 @@
 	struct cpufreq_freqs *freqs = p;
 	int i;
 
-	if (phase == CPUFREQ_POSTCHANGE) {
+	if ((phase == CPUFREQ_POSTCHANGE) ||
+	    (phase == CPUFREQ_RESUMECHANGE)) {
 		for (i = 0; i < SCI_NPORTS; i++) {
 			/*
 			 * This will force a baud rate change in hardware.
diff -ruN linux-original/drivers/pcmcia/sa11xx_core.c linux/drivers/pcmcia/sa11xx_core.c
--- linux-original/drivers/pcmcia/sa11xx_core.c	2004-04-04 14:13:52.000000000 +0200
+++ linux/drivers/pcmcia/sa11xx_core.c	2004-04-11 13:32:27.000000000 +0200
@@ -933,6 +933,8 @@
 		if (freqs->new < freqs->old)
 			sa1100_pcmcia_update_mecr(freqs->new);
 		break;
+	case CPUFREQ_RESUMECHANGE:
+		sa1100_pcmcia_update_mecr(freqs->new);
 	}
 
 	return 0;
diff -ruN linux-original/drivers/serial/sh-sci.c linux/drivers/serial/sh-sci.c
--- linux-original/drivers/serial/sh-sci.c	2004-04-04 14:13:54.000000000 +0200
+++ linux/drivers/serial/sh-sci.c	2004-04-11 13:33:19.000000000 +0200
@@ -748,7 +748,8 @@
 	struct cpufreq_freqs *freqs = p;
 	int i;
 
-	if (phase == CPUFREQ_POSTCHANGE) {
+	if ((phase == CPUFREQ_POSTCHANGE) ||
+	    (phase == CPUFREQ_RESUMECHANGE)){
 		for (i = 0; i < SCI_NPORTS; i++) {
 			struct uart_port *port = &sci_ports[i];

give the TSC a fair chance [Was: [PATCHES] cpufreq_get(), cpufreq->get()]

[Dominik Brodowski]

Dear John, 

Based on a patchset I submitted to the cpufreq list yesterday [description
and patches: http://www.brodo.de/patches/2004-04-11/00-announcement ]
I'd like to discuss the following patch:

If many lost ticks are detected, a call to "cpufreq_get()" for each online
CPU is scheduled as "work" for the "event" thread. This needs to be delayed
as cpufreq_get() may sleep. Inside cpufreq_get(), the cpufreq core tries to
determine the actual (hardware) CPU frequency, and if it differs from the
state the CPU was last set to, it will adjust to this inconsistency by
calling cpufreq notifiers. This means that the relevant frequency-affected
"constants" in timer_tsc.c will be updated, and no more lost ticks should
occur.

I've just verified that this patch does what it intends to, removing and
inserting the AC plug caused these messages in dmesg:
Warning: CPU frequency out of sync: cpufreq and timing core thinks of 1400000, is 600000 kHz.
Warning: CPU frequency out of sync: cpufreq and timing core thinks of 600000, is 1400000 kHz.

Please note that this patchset only will function on SMP as the cpufreq
workqueue interface was designed so well: even though the "kevent" thread
runs normally on one CPU, the actual work can be moved to other CPUs by
set_cpus_allowed(); one kevent "worker" can schedule additional work.

However, this patch has one slight chance for a bug: if lost_count reaches
50, a call to handle_cpufreq_delayed_work is scheduled, but does not execute
until lost_count reaches 50 again [e.g. when no lost ticks are
detected at the next call, but on the next 50 consecutive calls...]. As
keventd runs at -10 priority I doubt this will happen under normal
circumstances, though.

diff -ruN linux-original/arch/i386/kernel/timers/timer_tsc.c linux/arch/i386/kernel/timers/timer_tsc.c
--- linux-original/arch/i386/kernel/timers/timer_tsc.c	2004-04-12 13:38:59.541593040 +0200
+++ linux/arch/i386/kernel/timers/timer_tsc.c	2004-04-12 10:54:15.000000000 +0200
@@ -27,6 +27,8 @@
 struct timer_opts timer_tsc;
 #endif
 
+static inline void cpufreq_delayed_get(void);
+
 int tsc_disable __initdata = 0;
 
 extern spinlock_t i8253_lock;
@@ -241,6 +243,9 @@
 
 			clock_fallback();
 		}
+		/* ... but give it a fair chance to compensate for BIOS cpu frequency transitions */
+		if (lost_count == 50)
+			cpufreq_delayed_get();
 	} else
 		lost_count = 0;
 	/* update the monotonic base value */
@@ -324,6 +329,29 @@
 
 
 #ifdef CONFIG_CPU_FREQ
+#include <linux/workqueue.h>
+
+static unsigned int cpufreq_init = 0;
+static struct work_struct cpufreq_delayed_get_work;
+
+static void handle_cpufreq_delayed_get(void *v)
+{
+	unsigned int cpu;
+	for_each_online_cpu(cpu) {
+		cpufreq_get(cpu);
+	}
+}
+
+/* if we notice lost ticks, schedule a call to cpufreq_get() as it tries
+ * to verify the CPU frequency the timing core thinks the CPU is running
+ * at is still correct.
+ */
+static inline void cpufreq_delayed_get(void) 
+{
+	if (cpufreq_init)
+		schedule_work(&cpufreq_delayed_get_work);
+}
+
 /* If the CPU frequency is scaled, TSC-based delays will need a different
  * loops_per_jiffy value to function properly. An exception to this
  * are modern Intel Pentium 4 processors, where the TSC runs at a constant
@@ -383,6 +411,8 @@
 
 static int __init cpufreq_tsc(void)
 {
+	INIT_WORK(&cpufreq_delayed_get_work, handle_cpufreq_delayed_get, NULL);
+	cpufreq_init = 1;
 	/* P4 and above CPU TSC freq doesn't change when CPU frequency changes*/
 	if ((boot_cpu_data.x86 >= 15) && (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL))
 		variable_tsc = 0;
@@ -391,6 +421,8 @@
 }
 core_initcall(cpufreq_tsc);
 
+#else /* CONFIG_CPU_FREQ */
+static inline void cpufreq_delayed_get(void) { return; }
 #endif 

Re: [PATCHES] cpufreq_get(), cpufreq->get()

[Dave Jones]

On Sun, Apr 11, 2004 at 11:59:12PM +0200, Dominik Brodowski wrote:

 > The following patchset is based on the current cpufreq bitkeeper patch
 > and one important speedstep-centrino patch 
 > [see http://www.linux.org.uk/mailman/private/cpufreq/2004-April/003499.html ].
 > 
 > I suggest that this patchset is considered for the 2.6.7 timeframe; 
 > while the x86 parts seem to be quite straightforward, the arm 
 > architecture might be affected more heavily: it already relied on 
 > cpufreq_get(), and I can't verify my changes do even compile. Russell, 
 > I hope I didn't break anything this time...

Lets roll these in after 2.6.6 is out, and they can sit in -mm for a while.

		Dave

Re: give the TSC a fair chance [Was: [PATCHES] cpufreq_get(), cpufreq->get()]

[john stultz]

On Mon, 2004-04-12 at 05:04, Dominik Brodowski wrote:
> However, this patch has one slight chance for a bug: if lost_count reaches
> 50, a call to handle_cpufreq_delayed_work is scheduled, but does not execute
> until lost_count reaches 50 again [e.g. when no lost ticks are
> detected at the next call, but on the next 50 consecutive calls...]. As
> keventd runs at -10 priority I doubt this will happen under normal
> circumstances, though.

I'm not sure I follow this (lost_count == 50) bit. You're trying to
schedule work only when we've noticed we're missing exactly 50 ticks?
I'm not sure I understand the significance of that value.

thanks
-john

Re: give the TSC a fair chance [Was: [PATCHES] cpufreq_get(), cpufreq->get()]

[Dominik Brodowski]

[-- Attachment #1.1: Type: text/plain, Size: 1291 bytes --]

On Wed, Apr 14, 2004 at 11:24:21AM -0700, john stultz wrote:
> On Mon, 2004-04-12 at 05:04, Dominik Brodowski wrote:
> > However, this patch has one slight chance for a bug: if lost_count reaches
> > 50, a call to handle_cpufreq_delayed_work is scheduled, but does not execute
> > until lost_count reaches 50 again [e.g. when no lost ticks are
> > detected at the next call, but on the next 50 consecutive calls...]. As
> > keventd runs at -10 priority I doubt this will happen under normal
> > circumstances, though.
> 
> I'm not sure I follow this (lost_count == 50) bit. You're trying to
> schedule work only when we've noticed we're missing exactly 50 ticks?

No, not when we're missing exactly 50 ticks, but when we've detected 50
consecutive times we're missing ticks. That's half of the amount needed
before TSC is disabled and should mean enough time for cpufreq even under
heavy load to execute a cpufreq_get() call. 

> I'm not sure I understand the significance of that value.

It's not really significant, it should be less than what triggers
clock_fallback();
though. Note that it needs to be _one_ value instead of (lost_count > 50)
so that it doesn't get scheduled more than once. As lost_count is only
inc'ed, this shouldn't be a problem.

	Dominik

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 143 bytes --]

_______________________________________________
Cpufreq mailing list
Cpufreq@www.linux.org.uk
http://www.linux.org.uk/mailman/listinfo/cpufreq

Re: give the TSC a fair chance [Was: [PATCHES] cpufreq_get(), cpufreq->get()]

[john stultz]

On Wed, 2004-04-14 at 11:54, Dominik Brodowski wrote:
> On Wed, Apr 14, 2004 at 11:24:21AM -0700, john stultz wrote:
> > On Mon, 2004-04-12 at 05:04, Dominik Brodowski wrote:
> > > However, this patch has one slight chance for a bug: if lost_count reaches
> > > 50, a call to handle_cpufreq_delayed_work is scheduled, but does not execute
> > > until lost_count reaches 50 again [e.g. when no lost ticks are
> > > detected at the next call, but on the next 50 consecutive calls...]. As
> > > keventd runs at -10 priority I doubt this will happen under normal
> > > circumstances, though.
> > 
> > I'm not sure I follow this (lost_count == 50) bit. You're trying to
> > schedule work only when we've noticed we're missing exactly 50 ticks?
> 
> No, not when we're missing exactly 50 ticks, but when we've detected 50
> consecutive times we're missing ticks. That's half of the amount needed
> before TSC is disabled and should mean enough time for cpufreq even under
> heavy load to execute a cpufreq_get() call. 

Ok! Got it. Yea, in that case the patch looks fine. As for the bug you
mentioned, I'm not sure I understand how after you've scheduled 
cpufreq_delayed_get_work() to run it wouldn't run? 

So 50 consecutive interrupts occur where we notice we missed ticks, at
this point we schedule the function. If the next tick we don't notice
lost interrupts, why would the function not execute?

> > I'm not sure I understand the significance of that value.
> 
> It's not really significant, it should be less than what triggers
> clock_fallback();
> though. Note that it needs to be _one_ value instead of (lost_count > 50)
> so that it doesn't get scheduled more than once. As lost_count is only
> inc'ed, this shouldn't be a problem.

Ah! Indeed! Sorry, my context was a bit off. 

thanks
-john

Re: give the TSC a fair chance [Was: [PATCHES] cpufreq_get(), cpufreq->get()]

[Dominik Brodowski]

[-- Attachment #1.1: Type: text/plain, Size: 2208 bytes --]

On Wed, Apr 14, 2004 at 01:03:27PM -0700, john stultz wrote:
> On Wed, 2004-04-14 at 11:54, Dominik Brodowski wrote:
> > On Wed, Apr 14, 2004 at 11:24:21AM -0700, john stultz wrote:
> > > On Mon, 2004-04-12 at 05:04, Dominik Brodowski wrote:
> > > > However, this patch has one slight chance for a bug: if lost_count reaches
> > > > 50, a call to handle_cpufreq_delayed_work is scheduled, but does not execute
> > > > until lost_count reaches 50 again [e.g. when no lost ticks are
> > > > detected at the next call, but on the next 50 consecutive calls...]. As
> > > > keventd runs at -10 priority I doubt this will happen under normal
> > > > circumstances, though.
> > > 
> > > I'm not sure I follow this (lost_count == 50) bit. You're trying to
> > > schedule work only when we've noticed we're missing exactly 50 ticks?
> > 
> > No, not when we're missing exactly 50 ticks, but when we've detected 50
> > consecutive times we're missing ticks. That's half of the amount needed
> > before TSC is disabled and should mean enough time for cpufreq even under
> > heavy load to execute a cpufreq_get() call. 
> 
> Ok! Got it. Yea, in that case the patch looks fine.

Once 2.6.6 is out, I'll try to push it upstream (Dave -> Andrew -> Linus)

> As for the bug you
> mentioned, I'm not sure I understand how after you've scheduled 
> cpufreq_delayed_get_work() to run it wouldn't run? 

It would run, but too late. See below.

> So 50 consecutive interrupts occur where we notice we missed ticks, at
> this point we schedule the function. If the next tick we don't notice
> lost interrupts, why would the function not execute?

The following must happen:
50 consecutive interrupts with missed ticks occur
-> handle_cpufreq_get_delayed_work() is scheduled
0 to 50 more consecutive interrupts with missed ticks must occur,
then one or more interrupts with zero or one missed ticks so that lost_count
is reset to zero, and then
50 consecutive interrupts with missed ticks again.
_AND_ since the scheduling of handle_cpufreq_get_delayed_work() above
this handler running at -10 priority must not have been called ==> so 
it's really a very unlikely situation.

	Dominik

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 143 bytes --]

_______________________________________________
Cpufreq mailing list
Cpufreq@www.linux.org.uk
http://www.linux.org.uk/mailman/listinfo/cpufreq

Re: give the TSC a fair chance [Was: [PATCHES] cpufreq_get(), cpufreq->get()]

[john stultz]

On Wed, 2004-04-14 at 13:57, Dominik Brodowski wrote:
> On Wed, Apr 14, 2004 at 01:03:27PM -0700, john stultz wrote:
> > As for the bug you
> > mentioned, I'm not sure I understand how after you've scheduled 
> > cpufreq_delayed_get_work() to run it wouldn't run? 
> 
> It would run, but too late. See below.
> 
> > So 50 consecutive interrupts occur where we notice we missed ticks, at
> > this point we schedule the function. If the next tick we don't notice
> > lost interrupts, why would the function not execute?
> 
> The following must happen:
> 50 consecutive interrupts with missed ticks occur
> -> handle_cpufreq_get_delayed_work() is scheduled
> 0 to 50 more consecutive interrupts with missed ticks must occur,
> then one or more interrupts with zero or one missed ticks so that lost_count
> is reset to zero, and then
> 50 consecutive interrupts with missed ticks again.
> _AND_ since the scheduling of handle_cpufreq_get_delayed_work() above
> this handler running at -10 priority must not have been called ==> so 
> it's really a very unlikely situation.

Hmm. That's still not parsing easily, but I think I get it. Might we
want to turn off the consecutive lost tick accounting after we schedule
the delayed_work() function, and once that code has executed start
again? 

And how likely is it that we end up hitting 100 consecutive lost ticks
and disable the timesource before the delayed_work function gets to run?
Should that watermark get bumped up some?

thanks
-john

Re: give the TSC a fair chance [Was: [PATCHES] cpufreq_get(), cpufreq->get()]

[Dominik Brodowski]

On Wed, Apr 14, 2004 at 02:24:36PM -0700, john stultz wrote:
> On Wed, 2004-04-14 at 13:57, Dominik Brodowski wrote:
> > On Wed, Apr 14, 2004 at 01:03:27PM -0700, john stultz wrote:
> > > As for the bug you
> > > mentioned, I'm not sure I understand how after you've scheduled 
> > > cpufreq_delayed_get_work() to run it wouldn't run? 
> > 
> > It would run, but too late. See below.
> > 
> > > So 50 consecutive interrupts occur where we notice we missed ticks, at
> > > this point we schedule the function. If the next tick we don't notice
> > > lost interrupts, why would the function not execute?
> > 
> > The following must happen:
> > 50 consecutive interrupts with missed ticks occur
> > -> handle_cpufreq_get_delayed_work() is scheduled
> > 0 to 50 more consecutive interrupts with missed ticks must occur,
> > then one or more interrupts with zero or one missed ticks so that lost_count
> > is reset to zero, and then
> > 50 consecutive interrupts with missed ticks again.
> > _AND_ since the scheduling of handle_cpufreq_get_delayed_work() above
> > this handler running at -10 priority must not have been called ==> so 
> > it's really a very unlikely situation.
> 
> Hmm. That's still not parsing easily, but I think I get it. Might we
> want to turn off the consecutive lost tick accounting after we schedule
> the delayed_work() function, and once that code has executed start
> again? 

We could do it, but it wouldn't save us much. And it would delay falling
back to a "sane" timesource even more...

> And how likely is it that we end up hitting 100 consecutive lost ticks
> and disable the timesource before the delayed_work function gets to run?
> Should that watermark get bumped up some?

... IMO, no. there's already the need for >= 300 ms[*] of consecutive errors in
the TSC handling, and that's really much.

	Dominik

[*] 100 ticks which reach the code and detect >= 2 * 100 lost ticks ==> 
>= 300 ticks    ==>     >= 300 ms