Re: removal of cpufreq_set_policy()

Re: removal of cpufreq_set_policy()

[Thomas Renninger]

[-- 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

Re: removal of cpufreq_set_policy()

[Christian Krafft]

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

Hi Thomas,

On Mon, 21 May 2007 06:54:13 -0500
Thomas Renninger <trenn@suse.de> wrote:

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

Thanks for fixing the compile issue.

Unfortunately the whole PMI stuff is broken by hardware till now.
I also don't have working hardware and the whole interface is under discussion atm.
Therefor I disabled cpufreq and pmi in cell_defconfig for now.

Until the hardware guys can fix the problem, it should not hurt that the code is in.
If they don't manage to provide that interface the whole pmi code will have to be removed again.

The code should just not get called, if no pmi device can be found.

This is where I have two comments below.

I checked patch (including the two comments) on hardware without pmi. 
At least that looks still good.



> 
>        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 just needs to be done, if pmi_dev is not NULL.
Also in that case the notifier needs to be unregistered in cbe_cpufreq_cpu_exit().

>  	/* this ensures that policy->cpuinfo_min and policy->cpuinfo_max are set correctly */
>  	return cpufreq_frequency_table_cpuinfo(policy, cbe_freqs);
>  }
> 




-- 
Mit freundlichen Gruessen,
kind regards,

Christian Krafft
IBM Systems & Technology Group,
Linux Kernel Development
IT Specialist


Vorsitzender des Aufsichtsrats:	Martin Jetter
Geschaeftsfuehrung:		Herbert Kircher
Sitz der Gesellschaft:		Boeblingen
Registriergericht:		Amtsgericht Stuttgart, HRB 243294


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

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

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