cpufreq_set_policy removed

cpufreq_set_policy removed

[Tony Breeds]

Hi All,
	The git commit  632786ce9ff6206951ee4c84fe5c0d5c1d12f4cc reomved
the "deprecated and buggy" cpufreq_set_policy().  In the linux powerpc
tree we have a driver that's using it.

Offending function:
---
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;

	BUG_ON(pmi_msg.type != PMI_TYPE_FREQ_CHANGE);

	cpu = cbe_node_to_cpu(pmi_msg.data1);
	cbe_pmode_new = pmi_msg.data2;

	cpufreq_get_policy(&policy, cpu);

	policy.max = min(policy.max, cbe_freqs[cbe_pmode_new].frequency);
	policy.min = min(policy.min, policy.max);

	pr_debug("cbe_handle_pmi: new policy.min=%d policy.max=%d\n", policy.min, policy.max);
	cpufreq_set_policy(&policy);
}
---

What is the correct API to use now?

Yours Tony

  linux.conf.au        http://linux.conf.au/ || http://lca2008.linux.org.au/
  Jan 28 - Feb 02 2008 The Australian Linux Technical Conference!

Re: cpufreq_set_policy removed

[Benjamin Herrenschmidt]

On Thu, 2007-06-07 at 07:42 +1000, Tony Breeds wrote:
> Hi All,
> 	The git commit  632786ce9ff6206951ee4c84fe5c0d5c1d12f4cc reomved
> the "deprecated and buggy" cpufreq_set_policy().  In the linux powerpc
> tree we have a driver that's using it.

Ugh... we still use it ? I though we had fixed that ...

CC'ing Christian who maintains the CBE cpufreq driver.

Ben.

Re: cpufreq_set_policy removed

[Dave Jones]

On Thu, Jun 07, 2007 at 09:01:18AM +1000, Ben Herrenschmidt wrote:
 > On Thu, 2007-06-07 at 07:42 +1000, Tony Breeds wrote:
 > > Hi All,
 > > 	The git commit  632786ce9ff6206951ee4c84fe5c0d5c1d12f4cc reomved
 > > the "deprecated and buggy" cpufreq_set_policy().  In the linux powerpc
 > > tree we have a driver that's using it.
 > 
 > Ugh... we still use it ? I though we had fixed that ...
 > 
 > CC'ing Christian who maintains the CBE cpufreq driver.

Thomas mailed the following patch a few weeks ago..

	Dave

Subject: Re: removal of cpufreq_set_policy()
From: Thomas Renninger <trenn@suse.de>
Reply-To: trenn@suse.de
To: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: davej@redhat.com, Arnd Bergmann <arnd@arndb.de>,
        Christian Krafft <krafft@de.ibm.com>, cpufreq@lists.linux.org.uk
In-Reply-To: <1178529617.14928.4.camel@localhost.localdomain>
References: <1178522195.6353.246.camel@localhost.localdomain>
	 <1178528071.1231.942.camel@queen.suse.de>
	 <1178529617.14928.4.camel@localhost.localdomain>
Content-Type: multipart/mixed; boundary="=-Hzx+Pg+lBncYZX+nEfyQ"
Date: Mon, 21 May 2007 06:54:13 -0500
Message-Id: <1179748453.20299.68.camel@sublime.suse.de>


--=-Hzx+Pg+lBncYZX+nEfyQ
Content-Type: text/plain
Content-Transfer-Encoding: 7bit

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);
 }


--=-Hzx+Pg+lBncYZX+nEfyQ
Content-Disposition: attachment; filename=ppc_cpufreq_pmi_notifier.patch
Content-Type: text/x-patch; name=ppc_cpufreq_pmi_notifier.patch; charset=UTF-8
Content-Transfer-Encoding: 7bit

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);
 }

--=-Hzx+Pg+lBncYZX+nEfyQ--


-- 
http://www.codemonkey.org.uk

Re: cpufreq_set_policy removed

[Tony Breeds]

On Wed, Jun 06, 2007 at 07:13:43PM -0400, Dave Jones wrote:
> On Thu, Jun 07, 2007 at 09:01:18AM +1000, Ben Herrenschmidt wrote:
>  > On Thu, 2007-06-07 at 07:42 +1000, Tony Breeds wrote:
>  > > Hi All,
>  > > 	The git commit  632786ce9ff6206951ee4c84fe5c0d5c1d12f4cc reomved
>  > > the "deprecated and buggy" cpufreq_set_policy().  In the linux powerpc
>  > > tree we have a driver that's using it.
>  > 
>  > Ugh... we still use it ? I though we had fixed that ...
>  > 
>  > CC'ing Christian who maintains the CBE cpufreq driver.
> 
> Thomas mailed the following patch a few weeks ago..

Thanks Dave and Thomas.

We'll see what we can do about getting this tested and pushed.

Yours Tony

  linux.conf.au        http://linux.conf.au/ || http://lca2008.linux.org.au/
  Jan 28 - Feb 02 2008 The Australian Linux Technical Conference!

Re: cpufreq_set_policy removed

[Klaus.K Pedersen (Nokia-M/Helsinki)]

I currently use set_policy() to get around some peculiar features 
of OMAP2420, so at first I was a little scared. But now after fixing
my code - I agree that it is much better to only rely on the
CPUFREQ_POLICY_NOTIFIER chain.

I have some comments to patch in-line:

On Thu, 2007-06-07 at 00:14 +0000, an unknown sender wrote:
... 
> On Mon, 2007-05-07 at 19:20 +1000, Benjamin Herrenschmidt wrote:
... 
> 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
...
> @@ -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);
> +}

Shouldn't cpufreq_update_policy() be called from this function? 

> +
> +static int pmi_notifier(struct notifier_block *nb,
> +				       unsigned long event, void *data)
...
> +	cpufreq_verify_within_limits(policy, 0, pmi_frequency_limit);
> +	return 0;
>  }

... because pmi_notifier() is updating the policy...

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

... and pmi_notifier() is registered on the CPUFREQ_POLICY_NOTIFIER
chain.

So, how is the CPUFREQ_POLICY_NOTIFIER chain executed with no calls to
cpufreq_update_policy() (or __cpufreq_set_policy())?

And if pmi_notifier() is not called, how is the policy updated?


BR, Klaus Pedersen

Re: cpufreq_set_policy removed

[Thomas Renninger]

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

On Fri, 2007-06-15 at 20:45 +0300, Klaus.K Pedersen (Nokia-M/Helsinki)
wrote:
> I currently use set_policy() to get around some peculiar features 
> of OMAP2420, so at first I was a little scared. But now after fixing
> my code - I agree that it is much better to only rely on the
> CPUFREQ_POLICY_NOTIFIER chain.
> 
> I have some comments to patch in-line:
> 
> On Thu, 2007-06-07 at 00:14 +0000, an unknown sender wrote:
> ... 
> > On Mon, 2007-05-07 at 19:20 +1000, Benjamin Herrenschmidt wrote:
> ... 
> > 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
> ...
> > @@ -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);
> > +}
> 
> Shouldn't cpufreq_update_policy() be called from this function? 

Yes, I think you are right, is that better?
Still a real-life test whether all that pmi things are working as
expected would be great...

Thanks for the detailed info and review,

    Thomas


Update the policy on pmi frequency changes

Also fixes a compile dependency, PPC_PMI is needed for cell cpufreq.

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

---
 arch/powerpc/platforms/cell/Kconfig       |    2 +-
 arch/powerpc/platforms/cell/cbe_cpufreq.c |    2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

Index: linux-2.6.22-rc5/arch/powerpc/platforms/cell/cbe_cpufreq.c
===================================================================
--- linux-2.6.22-rc5.orig/arch/powerpc/platforms/cell/cbe_cpufreq.c
+++ linux-2.6.22-rc5/arch/powerpc/platforms/cell/cbe_cpufreq.c
@@ -175,6 +175,8 @@ static void cbe_cpufreq_handle_pmi(struc
 
 	pmi_frequency_limit = cbe_freqs[cbe_pmode_new].frequency;
 
+	cpufreq_update_policy(cpu);
+
 	pr_debug("cbe_handle_pmi: max freq=%d\n", pmi_frequency_limit);
 }
 
Index: linux-2.6.22-rc5/arch/powerpc/platforms/cell/Kconfig
===================================================================
--- linux-2.6.22-rc5.orig/arch/powerpc/platforms/cell/Kconfig
+++ linux-2.6.22-rc5/arch/powerpc/platforms/cell/Kconfig
@@ -66,7 +66,7 @@ config CBE_THERM
 
 config CBE_CPUFREQ
 	tristate "CBE frequency scaling"
-	depends on CBE_RAS && CPU_FREQ
+	depends on PPC_PMI && CBE_RAS && CPU_FREQ
 	default m
 	help
 	  This adds the cpufreq driver for Cell BE processors.


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

Update the policy on pmi frequency changes

Also fixes a compile dependency, PPC_PMI is needed for cell cpufreq.

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

---
 arch/powerpc/platforms/cell/Kconfig       |    2 +-
 arch/powerpc/platforms/cell/cbe_cpufreq.c |    2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

Index: linux-2.6.22-rc5/arch/powerpc/platforms/cell/cbe_cpufreq.c
===================================================================
--- linux-2.6.22-rc5.orig/arch/powerpc/platforms/cell/cbe_cpufreq.c
+++ linux-2.6.22-rc5/arch/powerpc/platforms/cell/cbe_cpufreq.c
@@ -175,6 +175,8 @@ static void cbe_cpufreq_handle_pmi(struc
 
 	pmi_frequency_limit = cbe_freqs[cbe_pmode_new].frequency;
 
+	cpufreq_update_policy(cpu);
+
 	pr_debug("cbe_handle_pmi: max freq=%d\n", pmi_frequency_limit);
 }
 
Index: linux-2.6.22-rc5/arch/powerpc/platforms/cell/Kconfig
===================================================================
--- linux-2.6.22-rc5.orig/arch/powerpc/platforms/cell/Kconfig
+++ linux-2.6.22-rc5/arch/powerpc/platforms/cell/Kconfig
@@ -66,7 +66,7 @@ config CBE_THERM
 
 config CBE_CPUFREQ
 	tristate "CBE frequency scaling"
-	depends on CBE_RAS && CPU_FREQ
+	depends on PPC_PMI && CBE_RAS && CPU_FREQ
 	default m
 	help
 	  This adds the cpufreq driver for Cell BE processors.

[-- 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: cpufreq_set_policy removed

[Myron Stowe]

Thomas, Dave:

Do either of you have a problem with the existence of
cpufreq_set_policy() or was it only removed because there were no
remaining callers in the tree and it had bugs?

The reason why I ask is that I have been working on a module that
changes cpufreq governors and I don't see any other way for a module to
be able to change governors without this interface.

If there is another way please let me know.  Otherwise I would like to
fix the cpufreq_set_policy() related bugs and make it available for
modules to use.

Myron

-- 
Myron Stowe                             HP Open Source & Linux Org

Re: cpufreq_set_policy removed

[Thomas Renninger]

On Wed, 2007-06-20 at 15:20 -0600, Myron Stowe wrote:
> Thomas, Dave:
> 
> Do either of you have a problem with the existence of
> cpufreq_set_policy() or was it only removed because there were no
> remaining callers in the tree and it had bugs?
Both.

In cpufreq_set_policy:

	/* this one invokes verify and data->max could get lowered
           because of temperature or BIOS limitations
	*/
        ret = __cpufreq_set_policy(data, policy);
        data->user_policy.min = data->min;
        data->user_policy.max = data->max;
        /* here user_policy.max is getting overridden with a possible
	limited value. But it is used in update_policy to restore the
	unlimited values again */
        data->user_policy.policy = data->policy;
        data->user_policy.governor = data->governor;

I think in update_policy:
policy->cpuinfo.min_freq
policy->cpuinfo.max_freq
should get used anyway...

If really needed it could get exported again, but just use
cpu_get/cpu_put, set/unset a lock and invoke __cpufreq_set_policy in
between?

> 
> The reason why I ask is that I have been working on a module that
> changes cpufreq governors and I don't see any other way for a module to
> be able to change governors without this interface.
But governor changing is userspace job?
IMO this is not the best argument, why do you need switching governors
inside the kernel?

   Thomas