Commit ce6c3997 authored by Dominik Brodowski's avatar Dominik Brodowski Committed by Dave Jones
Browse files

[CPUFREQ] Re-enable cpufreq suspend and resume code 



Commit 4bc5d341 is broken and causes regressions:

(1) cpufreq_driver->resume() and ->suspend() were only called on
__powerpc__, but you could set them on all architectures. In fact,
->resume() was defined and used before the PPC-related commit
42d4dc3f complained about in 4bc5d341.

(2) Therfore, the resume functions in acpi_cpufreq and speedstep-smi
would never be called.

(3) This means speedstep-smi would be unusuable after suspend or resume.

The _real_ problem was calling cpufreq_driver->get() with interrupts
off, but it re-enabling interrupts on some platforms. Why is ->get()
necessary?

Some systems like to change the CPU frequency behind our
back, especially during BIOS-intensive operations like suspend or
resume. If such systems also use a CPU frequency-dependant timing loop,
delays might be off by large factors. Therefore, we need to ascertain
as soon as possible that the CPU frequency is indeed at the speed we
think it is. You can do this two ways: either setting it anew, or trying
to get it. The latter is what was done, the former also has the same IRQ
issue.

So, let's try something different: defer the checking to after interrupts
are re-enabled, by calling cpufreq_update_policy() (via schedule_work()).
Timings may be off until this later stage, so let's watch out for
resume regressions caused by the deferred handling of frequency changes
behind the kernel's back.

Signed-off-by: default avatarDominik Brodowski <linux@dominikbrodowski.net>
Signed-off-by: default avatarDave Jones <davej@redhat.com>
parent 37d0892c

drivers/cpufreq/cpufreq.c

+7 −88
Original line number Diff line number Diff line
@@ -1250,20 +1250,11 @@ static int cpufreq_suspend(struct sys_device *sysdev, pm_message_t pmsg) 
{

	int ret = 0;



#ifdef __powerpc__

	int cpu = sysdev->id;

	unsigned int cur_freq = 0;

	struct cpufreq_policy *cpu_policy;



	dprintk("suspending cpu %u\n", cpu);



	/*

	 * This whole bogosity is here because Powerbooks are made of fail.

	 * No sane platform should need any of the code below to be run.

	 * (it's entirely the wrong thing to do, as driver->get may

	 *  reenable interrupts on some architectures).

	 */



	if (!cpu_online(cpu))

		return 0;
@@ -1282,47 +1273,13 @@ static int cpufreq_suspend(struct sys_device *sysdev, pm_message_t pmsg) 


	if (cpufreq_driver->suspend) {

		ret = cpufreq_driver->suspend(cpu_policy, pmsg);

		if (ret) {

		if (ret)

			printk(KERN_ERR "cpufreq: suspend failed in ->suspend "

					"step on CPU %u\n", cpu_policy->cpu);

			goto out;

		}

	}



	if (cpufreq_driver->flags & CPUFREQ_CONST_LOOPS)

		goto out;



	if (cpufreq_driver->get)

		cur_freq = cpufreq_driver->get(cpu_policy->cpu);



	if (!cur_freq || !cpu_policy->cur) {

		printk(KERN_ERR "cpufreq: suspend 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_PM_NO_WARN))

			dprintk("Warning: CPU frequency is %u, "

			       "cpufreq assumed %u kHz.\n",

			       cur_freq, cpu_policy->cur);



		freqs.cpu = cpu;

		freqs.old = cpu_policy->cur;

		freqs.new = cur_freq;



		srcu_notifier_call_chain(&cpufreq_transition_notifier_list,

				    CPUFREQ_SUSPENDCHANGE, &freqs);

		adjust_jiffies(CPUFREQ_SUSPENDCHANGE, &freqs);



		cpu_policy->cur = cur_freq;

	}



out:

	cpufreq_cpu_put(cpu_policy);

#endif	/* __powerpc__ */

	return ret;

}
@@ -1330,24 +1287,21 @@ out: 
 *	cpufreq_resume -  restore proper CPU frequency handling after resume

 *

 *	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.

 *	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...

 */

static int cpufreq_resume(struct sys_device *sysdev)

{

	int ret = 0;



#ifdef __powerpc__

	int cpu = sysdev->id;

	struct cpufreq_policy *cpu_policy;



	dprintk("resuming cpu %u\n", cpu);



	/* As with the ->suspend method, all the code below is

	 * only necessary because Powerbooks suck.

	 * See commit 42d4dc3f4e1e for jokes. */



	if (!cpu_online(cpu))

		return 0;
@@ -1373,45 +1327,10 @@ static int cpufreq_resume(struct sys_device *sysdev) 
		}

	}



	if (!(cpufreq_driver->flags & CPUFREQ_CONST_LOOPS)) {

		unsigned int cur_freq = 0;



		if (cpufreq_driver->get)

			cur_freq = cpufreq_driver->get(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_PM_NO_WARN))

				dprintk("Warning: CPU frequency "

				       "is %u, cpufreq assumed %u kHz.\n",

				       cur_freq, cpu_policy->cur);



			freqs.cpu = cpu;

			freqs.old = cpu_policy->cur;

			freqs.new = cur_freq;



			srcu_notifier_call_chain(

					&cpufreq_transition_notifier_list,

					CPUFREQ_RESUMECHANGE, &freqs);

			adjust_jiffies(CPUFREQ_RESUMECHANGE, &freqs);



			cpu_policy->cur = cur_freq;

		}

	}



out:

	schedule_work(&cpu_policy->update);



fail:

	cpufreq_cpu_put(cpu_policy);

#endif	/* __powerpc__ */

	return ret;

}

CRA Git | Maintained and supported by SUSTech CRA and CCSE