Re: removal of cpufreq_set_policy()

[Thomas Renninger <trenn@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 4566 bytes --]

On Mon, 2007-05-07 at 19:20 +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2007-05-07 at 10:54 +0200, Thomas Renninger wrote:
> 
> > set_policy is buggy:
> >   It calls verify_within_limits (where max_freq might get reduced)
> >   then sets
> >   data->user_policy.max = data->max;
> >   which is used to restore to highest freq with update_policy, but
> >   an already lowered freq might get assigned (e.g. if initialized
> >   on battery with limited freq, you will never ever reach higher
> >   freqs again).
> > 
> > I grepped the whole kernel and couldn't find cpufreq_set_policy to be
> > used anywhere else.
> 
> Looks like cbe_cpufreq driver was merged at the same time as you were
> removing set_polcy :-)
Yes.
> 
> > The cell cpufreq driver also does not match?:
> > linux-2.6.21_tests/arch/powerpc/platforms/cell/cbe_cpufreq.c
> > 
> > Can you give me a pointer to the code, is this an external project?
> 
> It should be in that cbe_cpufreq.c file in current -git. I'm not the
> author of the driver though, I'm just reporting the build failure :-) 
> 
> Arnd and Christian, can you check what's up ?
> 
> I think that driver uses set_policy to enforce new limits upon request
> from the system management.
> 
> I know I did something similar, though slightly different, for powermac,
> in drivers/macintosh/windfarm_cpufreq_clamp.c which is a module in my
> thermal control infrastructure that clamps down the cpufreq when an
> overtemp condition is detected.

I can reproduce this with -rc2.
cpufreq_set_policy came in with this one:
2007-04-23 Christian Krafft [POWERPC] cell: use pmi in cpufreq driver

Attached patch is setting the limit like it's done for x86 in
driver/acpi/processor_perflib.c
IMO the interface is more complicated as it needs to be (maybe I am
wrong),
but it seems that this is the way it's intended to work.

Please review, ehh, I don't have HW to test and don't even know what pmi
is,
can someone give this patch a test and try whether freq is limited and
increased again correctly if "pmi" kicks in (compile tested...).

       Thomas

Limit frequency for cell powerpc via cpufreq notifier chain

and get rid of cpufreq_set_policy call that caused a build
failure due interfering commits

Signed-off-by: Thomas Renninger <trenn@suse.de>


---
 arch/powerpc/platforms/cell/cbe_cpufreq.c |   28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

Index: linux-2.6.22-rc2/arch/powerpc/platforms/cell/cbe_cpufreq.c
===================================================================
--- linux-2.6.22-rc2.orig/arch/powerpc/platforms/cell/cbe_cpufreq.c
+++ linux-2.6.22-rc2/arch/powerpc/platforms/cell/cbe_cpufreq.c
@@ -67,6 +67,7 @@ static u64 MIC_Slow_Next_Timer_table[] =
 	0x00003FC000000000ull,
 };
 
+static unsigned int pmi_frequency_limit = 0;
 /*
  * hardware specific functions
  */
@@ -164,7 +165,6 @@ static int set_pmode(int cpu, unsigned i
 
 static void cbe_cpufreq_handle_pmi(struct of_device *dev, pmi_message_t pmi_msg)
 {
-	struct cpufreq_policy policy;
 	u8 cpu;
 	u8 cbe_pmode_new;
 
@@ -173,15 +173,27 @@ static void cbe_cpufreq_handle_pmi(struc
 	cpu = cbe_node_to_cpu(pmi_msg.data1);
 	cbe_pmode_new = pmi_msg.data2;
 
-	cpufreq_get_policy(&policy, cpu);
+	pmi_frequency_limit = cbe_freqs[cbe_pmode_new].frequency;
 
-	policy.max = min(policy.max, cbe_freqs[cbe_pmode_new].frequency);
-	policy.min = min(policy.min, policy.max);
+	pr_debug("cbe_handle_pmi: max freq=%d\n", pmi_frequency_limit);
+}
+
+static int pmi_notifier(struct notifier_block *nb,
+				       unsigned long event, void *data)
+{
+	struct cpufreq_policy *policy = data;
+
+	if (event != CPUFREQ_INCOMPATIBLE)
+		return 0;
 
-	pr_debug("cbe_handle_pmi: new policy.min=%d policy.max=%d\n", policy.min, policy.max);
-	cpufreq_set_policy(&policy);
+	cpufreq_verify_within_limits(policy, 0, pmi_frequency_limit);
+	return 0;
 }
 
+static struct notifier_block pmi_notifier_block = {
+	.notifier_call = pmi_notifier,
+};
+
 static struct pmi_handler cbe_pmi_handler = {
 	.type			= PMI_TYPE_FREQ_CHANGE,
 	.handle_pmi_message	= cbe_cpufreq_handle_pmi,
@@ -238,6 +250,10 @@ static int cbe_cpufreq_cpu_init(struct c
 
 	cpufreq_frequency_table_get_attr(cbe_freqs, policy->cpu);
 
+	/* frequency might get limited later, initialize limit with max_freq */
+	pmi_frequency_limit = max_freq;
+	cpufreq_register_notifier(&pmi_notifier_block, CPUFREQ_POLICY_NOTIFIER);
+
 	/* this ensures that policy->cpuinfo_min and policy->cpuinfo_max are set correctly */
 	return cpufreq_frequency_table_cpuinfo(policy, cbe_freqs);
 }


[-- Attachment #2: ppc_cpufreq_pmi_notifier.patch --]
[-- Type: text/x-patch, Size: 2489 bytes --]

Limit frequency for cell powerpc via cpufreq notifier chain

and get rid of cpufreq_set_policy call that caused a build
failure due interfering commits

Signed-off-by: Thomas Renninger <trenn@suse.de>


---
 arch/powerpc/platforms/cell/cbe_cpufreq.c |   28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

Index: linux-2.6.22-rc2/arch/powerpc/platforms/cell/cbe_cpufreq.c
===================================================================
--- linux-2.6.22-rc2.orig/arch/powerpc/platforms/cell/cbe_cpufreq.c
+++ linux-2.6.22-rc2/arch/powerpc/platforms/cell/cbe_cpufreq.c
@@ -67,6 +67,7 @@ static u64 MIC_Slow_Next_Timer_table[] =
 	0x00003FC000000000ull,
 };
 
+static unsigned int pmi_frequency_limit = 0;
 /*
  * hardware specific functions
  */
@@ -164,7 +165,6 @@ static int set_pmode(int cpu, unsigned i
 
 static void cbe_cpufreq_handle_pmi(struct of_device *dev, pmi_message_t pmi_msg)
 {
-	struct cpufreq_policy policy;
 	u8 cpu;
 	u8 cbe_pmode_new;
 
@@ -173,15 +173,27 @@ static void cbe_cpufreq_handle_pmi(struc
 	cpu = cbe_node_to_cpu(pmi_msg.data1);
 	cbe_pmode_new = pmi_msg.data2;
 
-	cpufreq_get_policy(&policy, cpu);
+	pmi_frequency_limit = cbe_freqs[cbe_pmode_new].frequency;
 
-	policy.max = min(policy.max, cbe_freqs[cbe_pmode_new].frequency);
-	policy.min = min(policy.min, policy.max);
+	pr_debug("cbe_handle_pmi: max freq=%d\n", pmi_frequency_limit);
+}
+
+static int pmi_notifier(struct notifier_block *nb,
+				       unsigned long event, void *data)
+{
+	struct cpufreq_policy *policy = data;
+
+	if (event != CPUFREQ_INCOMPATIBLE)
+		return 0;
 
-	pr_debug("cbe_handle_pmi: new policy.min=%d policy.max=%d\n", policy.min, policy.max);
-	cpufreq_set_policy(&policy);
+	cpufreq_verify_within_limits(policy, 0, pmi_frequency_limit);
+	return 0;
 }
 
+static struct notifier_block pmi_notifier_block = {
+	.notifier_call = pmi_notifier,
+};
+
 static struct pmi_handler cbe_pmi_handler = {
 	.type			= PMI_TYPE_FREQ_CHANGE,
 	.handle_pmi_message	= cbe_cpufreq_handle_pmi,
@@ -238,6 +250,10 @@ static int cbe_cpufreq_cpu_init(struct c
 
 	cpufreq_frequency_table_get_attr(cbe_freqs, policy->cpu);
 
+	/* frequency might get limited later, initialize limit with max_freq */
+	pmi_frequency_limit = max_freq;
+	cpufreq_register_notifier(&pmi_notifier_block, CPUFREQ_POLICY_NOTIFIER);
+
 	/* this ensures that policy->cpuinfo_min and policy->cpuinfo_max are set correctly */
 	return cpufreq_frequency_table_cpuinfo(policy, cbe_freqs);
 }

[-- Attachment #3: Type: text/plain, Size: 147 bytes --]

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