RE: 2.6.19-rc5-mm2

RE: 2.6.19-rc5-mm2

[Pallipadi, Venkatesh]

 

>-----Original Message-----
>From: Mattia Dongili [mailto:malattia@linux.it] 
>Sent: Wednesday, November 15, 2006 9:47 AM
>To: Pallipadi, Venkatesh
>Cc: ego@in.ibm.com; Reuben Farrelly; Andrew Morton; 
>davej@redhat.com; linux-kernel@vger.kernel.org; CPUFreq 
>Mailing List; Sadykov, Denis M
>Subject: Re: 2.6.19-rc5-mm2
>
>On Wed, Nov 15, 2006 at 06:09:47AM -0800, Pallipadi, Venkatesh wrote:
>[...]
>> >data->cpu_feature == ACPI_ADR_SPACE_FIXED_HARDWARE.
>> 
>> Yes. Patch from Mattia is indeed required for acpi-cpufreq. 
>> Mattia: Can you please send the patch towards Andrew (With signed off
>> etc) for inclusion.
>
>Venki, I'm a little worried about the switch/case in question (line
>702): the data->cpu_feature is set either to SYSTEM_IO_CAPABLE or
>SYSTEM_INTEL_MSR_CAPABLE just a few lines above so it seems the switch
>variable is wrong and none of the 2 cases will ever get a chance to
>execute.
>

The variable is set few lines before. So, it should be OK to switch on
that 
variable set and one of the two cases will execute. isn't it? Or am 
I missing something?

>Unfortunately I don't have enough knowledge to tell if it's simply
>necessary to fix the switch variable as 

Get_cur_freq_on_cpu will not work on SYSTEM_IO space as ACPI does not
have an interface to get the current frequency. It only has a interface
to say whether the last transitions tried was successful or not.
So, if indeed a change in switch is required, first option should be
good...

Thanks,
Venki

RE: 2.6.19-rc5-mm2

[Pallipadi, Venkatesh]

 

>-----Original Message-----
>From: Mattia Dongili [mailto:malattia@linux.it] 
>Sent: Wednesday, November 15, 2006 9:47 AM
>To: Pallipadi, Venkatesh
>Cc: ego@in.ibm.com; Reuben Farrelly; Andrew Morton; 
>davej@redhat.com; linux-kernel@vger.kernel.org; CPUFreq 
>Mailing List; Sadykov, Denis M
>Subject: Re: 2.6.19-rc5-mm2
>
>On Wed, Nov 15, 2006 at 06:09:47AM -0800, Pallipadi, Venkatesh wrote:
>[...]
>> >data->cpu_feature == ACPI_ADR_SPACE_FIXED_HARDWARE.
>> 
>> Yes. Patch from Mattia is indeed required for acpi-cpufreq. 
>> Mattia: Can you please send the patch towards Andrew (With signed off
>> etc) for inclusion.
>
>Venki, I'm a little worried about the switch/case in question (line
>702): the data->cpu_feature is set either to SYSTEM_IO_CAPABLE or
>SYSTEM_INTEL_MSR_CAPABLE just a few lines above so it seems the switch
>variable is wrong and none of the 2 cases will ever get a chance to
>execute.
>

The variable is set few lines before. So, it should be OK to switch on
that 
variable set and one of the two cases will execute. isn't it? Or am 
I missing something?

>Unfortunately I don't have enough knowledge to tell if it's simply
>necessary to fix the switch variable as 

Get_cur_freq_on_cpu will not work on SYSTEM_IO space as ACPI does not
have an interface to get the current frequency. It only has a interface
to say whether the last transitions tried was successful or not.
So, if indeed a change in switch is required, first option should be
good...

Thanks,
Venki

Re: 2.6.19-rc5-mm2

[Mattia Dongili]

On Wed, Nov 15, 2006 at 10:06:23AM -0800, Pallipadi, Venkatesh wrote:
[...]
> >Venki, I'm a little worried about the switch/case in question (line
> >702): the data->cpu_feature is set either to SYSTEM_IO_CAPABLE or
> >SYSTEM_INTEL_MSR_CAPABLE just a few lines above so it seems the switch
> >variable is wrong and none of the 2 cases will ever get a chance to
> >execute.
> >
> 
> The variable is set few lines before. So, it should be OK to switch on
> that 
> variable set and one of the two cases will execute. isn't it? Or am 
> I missing something?

Yes, patch is coming.

> >Unfortunately I don't have enough knowledge to tell if it's simply
> >necessary to fix the switch variable as 
> 
> Get_cur_freq_on_cpu will not work on SYSTEM_IO space as ACPI does not
> have an interface to get the current frequency. It only has a interface
> to say whether the last transitions tried was successful or not.
> So, if indeed a change in switch is required, first option should be
> good...

Hmmmm... I see, the following path fooled me, but re-reading it seems
more obvious what it is doing :)

	get_cur_freq_on_cpu
		extract_freq
			switch (data->cpu_feature) {
			case SYSTEM_INTEL_MSR_CAPABLE:
				return extract_msr(val, data);
			case SYSTEM_IO_CAPABLE:
				return extract_io(val, data);
			...

-- 
mattia
:wq!

[PATCH 2.6.19-rc5-mm2] cpufreq: set policy->curfreq on initialization

[Mattia Dongili]

Check the correct variable and set policy->cur upon acpi-cpufreq
initialization to allow the userspace governor to be used as default.

Signed-off-by: Mattia Dongili <malattia@linux.it>

---

Reuben, could you also try if this patch fixes the BUG()?
Thanks

diff --git a/arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c b/arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c
index 18f4715..a630f94 100644
--- a/arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c
+++ b/arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c
@@ -699,14 +699,14 @@ static int acpi_cpufreq_cpu_init(struct
 	if (result)
 		goto err_freqfree;
 
-	switch (data->cpu_feature) {
+	switch (perf->control_register.space_id) {
 	case ACPI_ADR_SPACE_SYSTEM_IO:
 		/* Current speed is unknown and not detectable by IO port */
 		policy->cur = acpi_cpufreq_guess_freq(data, policy->cpu);
 		break;
 	case ACPI_ADR_SPACE_FIXED_HARDWARE:
 		acpi_cpufreq_driver.get = get_cur_freq_on_cpu;
-		get_cur_freq_on_cpu(cpu);
+		policy->cur = get_cur_freq_on_cpu(cpu);
 		break;
 	default:
 		break;

Re: [PATCH 2.6.19-rc5-mm2] cpufreq: set policy->curfreq on initialization

[Reuben Farrelly]

On 16/11/2006 6:05 AM, Mattia Dongili wrote:
> Check the correct variable and set policy->cur upon acpi-cpufreq
> initialization to allow the userspace governor to be used as default.
> 
> Signed-off-by: Mattia Dongili <malattia@linux.it>
> 
> ---
> 
> Reuben, could you also try if this patch fixes the BUG()?
> Thanks

It does, and all looks fine now, thanks.  Sorry for not getting back about it a 
little earlier.

Reuben


> diff --git a/arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c b/arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c
> index 18f4715..a630f94 100644
> --- a/arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c
> +++ b/arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c
> @@ -699,14 +699,14 @@ static int acpi_cpufreq_cpu_init(struct
>  	if (result)
>  		goto err_freqfree;
>  
> -	switch (data->cpu_feature) {
> +	switch (perf->control_register.space_id) {
>  	case ACPI_ADR_SPACE_SYSTEM_IO:
>  		/* Current speed is unknown and not detectable by IO port */
>  		policy->cur = acpi_cpufreq_guess_freq(data, policy->cpu);
>  		break;
>  	case ACPI_ADR_SPACE_FIXED_HARDWARE:
>  		acpi_cpufreq_driver.get = get_cur_freq_on_cpu;
> -		get_cur_freq_on_cpu(cpu);
> +		policy->cur = get_cur_freq_on_cpu(cpu);
>  		break;
>  	default:
>  		break;

RE: [PATCH 2.6.19-rc5-mm2] cpufreq: set policy->curfreq on initialization

[Pallipadi, Venkatesh]

Acked.
Andrew: Please include this patch in mm.

Thanks,
Venki 

>-----Original Message-----
>From: Mattia Dongili [mailto:malattia@linux.it] 
>Sent: Wednesday, November 15, 2006 11:05 AM
>To: Pallipadi, Venkatesh; ego@in.ibm.com; Reuben Farrelly; 
>Andrew Morton; davej@redhat.com; linux-kernel@vger.kernel.org; 
>CPUFreq Mailing List; Sadykov, Denis M
>Subject: [PATCH 2.6.19-rc5-mm2] cpufreq: set policy->curfreq 
>on initialization
>
>
>Check the correct variable and set policy->cur upon acpi-cpufreq
>initialization to allow the userspace governor to be used as default.
>
>Signed-off-by: Mattia Dongili <malattia@linux.it>
>
>---
>
>Reuben, could you also try if this patch fixes the BUG()?
>Thanks
>
>diff --git a/arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c 
>b/arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c
>index 18f4715..a630f94 100644
>--- a/arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c
>+++ b/arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c
>@@ -699,14 +699,14 @@ static int acpi_cpufreq_cpu_init(struct
> 	if (result)
> 		goto err_freqfree;
> 
>-	switch (data->cpu_feature) {
>+	switch (perf->control_register.space_id) {
> 	case ACPI_ADR_SPACE_SYSTEM_IO:
> 		/* Current speed is unknown and not detectable 
>by IO port */
> 		policy->cur = acpi_cpufreq_guess_freq(data, 
>policy->cpu);
> 		break;
> 	case ACPI_ADR_SPACE_FIXED_HARDWARE:
> 		acpi_cpufreq_driver.get = get_cur_freq_on_cpu;
>-		get_cur_freq_on_cpu(cpu);
>+		policy->cur = get_cur_freq_on_cpu(cpu);
> 		break;
> 	default:
> 		break;
>

RE: [PATCH 2.6.19-rc5-mm2] cpufreq: set policy->curfreq on initialization

[Pallipadi, Venkatesh]

Acked.
Andrew: Please include this patch in mm.

Thanks,
Venki 

>-----Original Message-----
>From: Mattia Dongili [mailto:malattia@linux.it] 
>Sent: Wednesday, November 15, 2006 11:05 AM
>To: Pallipadi, Venkatesh; ego@in.ibm.com; Reuben Farrelly; 
>Andrew Morton; davej@redhat.com; linux-kernel@vger.kernel.org; 
>CPUFreq Mailing List; Sadykov, Denis M
>Subject: [PATCH 2.6.19-rc5-mm2] cpufreq: set policy->curfreq 
>on initialization
>
>
>Check the correct variable and set policy->cur upon acpi-cpufreq
>initialization to allow the userspace governor to be used as default.
>
>Signed-off-by: Mattia Dongili <malattia@linux.it>
>
>---
>
>Reuben, could you also try if this patch fixes the BUG()?
>Thanks
>
>diff --git a/arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c 
>b/arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c
>index 18f4715..a630f94 100644
>--- a/arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c
>+++ b/arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c
>@@ -699,14 +699,14 @@ static int acpi_cpufreq_cpu_init(struct
> 	if (result)
> 		goto err_freqfree;
> 
>-	switch (data->cpu_feature) {
>+	switch (perf->control_register.space_id) {
> 	case ACPI_ADR_SPACE_SYSTEM_IO:
> 		/* Current speed is unknown and not detectable 
>by IO port */
> 		policy->cur = acpi_cpufreq_guess_freq(data, 
>policy->cpu);
> 		break;
> 	case ACPI_ADR_SPACE_FIXED_HARDWARE:
> 		acpi_cpufreq_driver.get = get_cur_freq_on_cpu;
>-		get_cur_freq_on_cpu(cpu);
>+		policy->cur = get_cur_freq_on_cpu(cpu);
> 		break;
> 	default:
> 		break;
>