Re: [PATCH] Re: cpufreq limits avilable frequencies to 800MHz on git kernel

From: Thomas Renninger
Date: Thu Jul 17 2008 - 23:20:28 EST

Next message: jie.yang: "[PATCH net-next] atl1e: Atheros L1E Gigabit Ethernet driver"
Previous message: Ian Kent: "[PATCH] autofs4 - fix direct mount pending expire race - correction"
In reply to: Andrew Morton: "Re: [PATCH] Re: cpufreq limits avilable frequencies to 800MHz ongit kernel"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thursday 17 July 2008 11:40:00 pm Andrew Morton wrote:
> On Thu, 17 Jul 2008 15:48:02 +0200
>
> Thomas Renninger <trenn@xxxxxxx> wrote:
> > Hi,
> >
> > maybe I found something..., can someone review/test this.
> >
> > Thanks,
> >
> > Thomas
> >
> > ------------
> > CPUFREQ ACPI: Only call _PPC after cpufreq ACPI init funcs got called
> > already
> >
> > Ingo Molnar provided a fix to not call _PPC at processor driver
> > initialization time.
> > Git commit #e4233dec749a3519069d9390561b5636a75c7579
> >
> > But it can still happen that _PPC is called at processor driver
> > initialization time.
> >
> > This patch should make sure that this is not possible anymore.
>
> There is no actual description of what this fixes, is there? Do
> machines go oops, or what?
ThinkPads get stuck at lowest frequency.
A very nasty bug, reported several times recently.
This seem to show up rarely, not 100% reproducable (after every X boot...).
I do not know whether this one really fixes it. I saw an interesting debug
output on the cpufreq list recently pointing in this direction.
Even if it's not this, only calling _PPC after other initial cpufreq ACPI
functions like _PDC or _PSS have been called (means cpufreq is really active)
is a good idea. _PPC functions often have a complex logic. They might depend
on variables initialized in e.g. in _PDC or _PSS...
>
> How do we proceed from here with this patch? Who should review it, who
> should test it, who should ack it and who should merge it?
I hope that Dave or someone on cpufreq list is having a look at it.
Dave should take it if it's fine.
People seeing this are in CC. After a quick review (patches are short..), I
can provide an rpm for SUSE people (there is a SUSE and a kernel bug open)
https://bugzilla.novell.com/show_bug.cgi?id=374099

>
> e4233dec749a3519069d9390561b5636a75c7579 was in January so this patch
> is applicable to 2.6.25.x and to 2.6.26.x. But is it needed there?
Probably should go in through .27 after review/testing.

> Insufficient info.
>
> Ho hum. I queued it, tagged as needed-in-2.6.25.x and 2.6.26.x. But I
> am unsure about that.
>
> Please help to clarify these things.
>
> > Signed-off-by: Thomas Renninger <trenn@xxxxxxx>
> > ---
> > arch/powerpc/platforms/cell/cbe_cpufreq_pmi.c | 6 ++++++
> > drivers/acpi/processor_perflib.c | 13 ++++++++++++-
> > drivers/cpufreq/cpufreq.c | 3 +++
> > include/linux/cpufreq.h | 1 +
> > 4 files changed, 22 insertions(+), 1 deletions(-)
> >
> > diff --git a/arch/powerpc/platforms/cell/cbe_cpufreq_pmi.c
> > b/arch/powerpc/platforms/cell/cbe_cpufreq_pmi.c
> > index 69288f6..3233fe8 100644
> > --- a/arch/powerpc/platforms/cell/cbe_cpufreq_pmi.c
> > +++ b/arch/powerpc/platforms/cell/cbe_cpufreq_pmi.c
> > @@ -96,6 +96,12 @@ static int pmi_notifier(struct notifier_block *nb,
> > struct cpufreq_frequency_table *cbe_freqs;
> > u8 node;
> >
> > + /* Should this really be called for CPUFREQ_ADJUST,
> > CPUFREQ_INCOMPATIBLE + * and CPUFREQ_NOTIFY policy events?)
> > + */
> > + if (event == CPUFREQ_START)
> > + return 0;
> > +
> > cbe_freqs = cpufreq_frequency_get_table(policy->cpu);
> > node = cbe_cpu_to_node(policy->cpu);
> >
> > diff --git a/drivers/acpi/processor_perflib.c
> > b/drivers/acpi/processor_perflib.c
> > index b474996..63ccf80 100644
> > --- a/drivers/acpi/processor_perflib.c
> > +++ b/drivers/acpi/processor_perflib.c
> > @@ -72,7 +72,13 @@ MODULE_PARM_DESC(ignore_ppc, "If the frequency of your
> > machine gets wrongly" \
> > #define PPC_REGISTERED 1
> > #define PPC_IN_USE 2
> >
> > -static int acpi_processor_ppc_status = 0;
> > +/* ignore_ppc:
> > + * -1 -> cpufreq low level drivers not initialized -> _PSS, etc. not
> > called yet
>
> I am going to start hanging around in dark alleys in the hope of
> meeting the person who invented wordwrapping.
Sorry about that, I moved to kmail recently, still do not know how this could
happen...

Thomas
>
>
> Here it is again, fixed:
>
>
> From: Thomas Renninger <trenn@xxxxxxx>
>
> Ingo Molnar provided a fix to not call _PPC at processor driver
> initialization time. Git commit #e4233dec749a3519069d9390561b5636a75c7579
>
> But it can still happen that _PPC is called at processor driver
> initialization time.
>
> This patch should make sure that this is not possible anymore.
>
> Signed-off-by: Thomas Renninger <trenn@xxxxxxx>
> Cc: Dave Jones <davej@xxxxxxxxxx>
> Cc: Len Brown <len.brown@xxxxxxxxx>
> Cc: Venkatesh Pallipadi <venkatesh.pallipadi@xxxxxxxxx>
> Cc: Chandra Seetharaman <sekharan@xxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxx>
> Cc: Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx>
> Cc: Arkadiusz Miskiewicz <arekm@xxxxxxxx>
> Cc: <gnorton@xxxxxxxxxx>
> Cc: <miguel@xxxxxxxxxx>
> Cc: <stable@xxxxxxxxxx> [2.6.25.x, 2.6.26.x]
> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> ---
>
> arch/powerpc/platforms/cell/cbe_cpufreq_pmi.c | 6 ++++++
> drivers/acpi/processor_perflib.c | 13 ++++++++++++-
> drivers/cpufreq/cpufreq.c | 3 +++
> include/linux/cpufreq.h | 1 +
> 4 files changed, 22 insertions(+), 1 deletion(-)
>
> diff -puN
> arch/powerpc/platforms/cell/cbe_cpufreq_pmi.c~cpufreq-acpi-only-call-_ppc-a
>fter-cpufreq-acpi-init-funcs-got-called-already
> arch/powerpc/platforms/cell/cbe_cpufreq_pmi.c ---
> a/arch/powerpc/platforms/cell/cbe_cpufreq_pmi.c~cpufreq-acpi-only-call-_ppc
>-after-cpufreq-acpi-init-funcs-got-called-already +++
> a/arch/powerpc/platforms/cell/cbe_cpufreq_pmi.c
> @@ -96,6 +96,12 @@ static int pmi_notifier(struct notifier_
> struct cpufreq_frequency_table *cbe_freqs;
> u8 node;
>
> + /* Should this really be called for CPUFREQ_ADJUST, CPUFREQ_INCOMPATIBLE
> + * and CPUFREQ_NOTIFY policy events?)
> + */
> + if (event == CPUFREQ_START)
> + return 0;
> +
> cbe_freqs = cpufreq_frequency_get_table(policy->cpu);
> node = cbe_cpu_to_node(policy->cpu);
>
> diff -puN
> drivers/acpi/processor_perflib.c~cpufreq-acpi-only-call-_ppc-after-cpufreq-
>acpi-init-funcs-got-called-already drivers/acpi/processor_perflib.c ---
> a/drivers/acpi/processor_perflib.c~cpufreq-acpi-only-call-_ppc-after-cpufre
>q-acpi-init-funcs-got-called-already +++ a/drivers/acpi/processor_perflib.c
> @@ -72,7 +72,13 @@ MODULE_PARM_DESC(ignore_ppc, "If the fre
> #define PPC_REGISTERED 1
> #define PPC_IN_USE 2
>
> -static int acpi_processor_ppc_status = 0;
> +/* ignore_ppc:
> + * -1 -> cpufreq low level drivers not initialized -> _PSS, etc. not
> called yet + * ignore _PPC
> + * 0 -> cpufreq low level drivers initialized -> consider _PPC values
> + * 1 -> ignore _PPC totally -> forced by user through boot param
> + */
> +static int acpi_processor_ppc_status = -1;
>
> static int acpi_processor_ppc_notifier(struct notifier_block *nb,
> unsigned long event, void *data)
> @@ -81,6 +87,11 @@ static int acpi_processor_ppc_notifier(s
> struct acpi_processor *pr;
> unsigned int ppc = 0;
>
> + if (event == CPUFREQ_START && ignore_ppc <= 0) {
> + ignore_ppc = 0;
> + return 0;
> + }
> +
> if (ignore_ppc)
> return 0;
>
> diff -puN
> drivers/cpufreq/cpufreq.c~cpufreq-acpi-only-call-_ppc-after-cpufreq-acpi-in
>it-funcs-got-called-already drivers/cpufreq/cpufreq.c ---
> a/drivers/cpufreq/cpufreq.c~cpufreq-acpi-only-call-_ppc-after-cpufreq-acpi-
>init-funcs-got-called-already +++ a/drivers/cpufreq/cpufreq.c
> @@ -825,6 +825,9 @@ static int cpufreq_add_dev(struct sys_de
> policy->user_policy.min = policy->cpuinfo.min_freq;
> policy->user_policy.max = policy->cpuinfo.max_freq;
>
> + blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
> + CPUFREQ_START, policy);
> +
> #ifdef CONFIG_SMP
>
> #ifdef CONFIG_HOTPLUG_CPU
> diff -puN
> include/linux/cpufreq.h~cpufreq-acpi-only-call-_ppc-after-cpufreq-acpi-init
>-funcs-got-called-already include/linux/cpufreq.h ---
> a/include/linux/cpufreq.h~cpufreq-acpi-only-call-_ppc-after-cpufreq-acpi-in
>it-funcs-got-called-already +++ a/include/linux/cpufreq.h
> @@ -106,6 +106,7 @@ struct cpufreq_policy {
> #define CPUFREQ_ADJUST (0)
> #define CPUFREQ_INCOMPATIBLE (1)
> #define CPUFREQ_NOTIFY (2)
> +#define CPUFREQ_START (3)
>
> #define CPUFREQ_SHARED_TYPE_NONE (0) /* None */
> #define CPUFREQ_SHARED_TYPE_HW (1) /* HW does needed coordination */
> _

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: jie.yang: "[PATCH net-next] atl1e: Atheros L1E Gigabit Ethernet driver"
Previous message: Ian Kent: "[PATCH] autofs4 - fix direct mount pending expire race - correction"
In reply to: Andrew Morton: "Re: [PATCH] Re: cpufreq limits avilable frequencies to 800MHz ongit kernel"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]