Bug 5553

Summary: speedstep-smi fails to load if CONFIG_CPU_FREQ_DEBUG is not set
Product: Power Management Reporter: Theodor Milkov (tm)
Component: cpufreqAssignee: cpufreq (cpufreq)
Status: CLOSED CODE_FIX    
Severity: normal CC: akpm, lenb
Priority: P2    
Hardware: i386   
OS: Linux   
Kernel Version: 2.6.14 Subsystem:
Regression: No Bisected commit-id:
Attachments: wait a bit before trying transitions
change dprintk to printk in speedstep_smi_ownership()

Description Theodor Milkov 2005-11-04 16:14:50 UTC
Most recent kernel where this bug did not occur:
 2.6.14
Distribution:
 Debian GNU/Linux Etch
Hardware Environment:
 IBM ThinkPad T22, Pentium III (Coppermine), i440BX
Software Environment:
Problem Description:

cpufreq used to work with 2.6.8 and 2.6.11 kernels, but doesn't work with 2.6.12
and 2.6.14 for me. I've found that it actually works with 2.6.14 but only if
kernel is compiled with CONFIG_CPU_FREQ_DEBUG option set.

Steps to reproduce:

Compile kernel with all allmodconfig and clear the CONFIG_CPU_FREQ_DEBUG.

Then the following happens (otherwise it works):

# modprobe speedstep-lib relaxed_check=1
# modprobe speedstep-smi
FATAL: Error inserting speedstep_smi
(/lib/modules/2.6.14/kernel/arch/i386/kernel/cpu/cpufreq/speedstep-smi.ko): No
such device
Comment 1 Andrew Morton 2005-11-11 02:04:14 UTC
bugme-daemon@bugzilla.kernel.org wrote:
>
> http://bugzilla.kernel.org/show_bug.cgi?id=5553
> 
>             Summary: speedstep-smi fails to load if CONFIG_CPU_FREQ_DEBUG is
>                      not set

Perhaps you could try adding this, it might tell us a bit more about why
it's failing?

--- devel/arch/i386/kernel/cpu/cpufreq/speedstep-smi.c~a	2005-11-11 01:42:00.000000000 -0800
+++ devel-akpm/arch/i386/kernel/cpu/cpufreq/speedstep-smi.c	2005-11-11 01:43:06.000000000 -0800
@@ -373,6 +373,9 @@ static int __init speedstep_init(void)
 		ist_info.signature, ist_info.command, ist_info.event, ist_info.perf_level);
 
 
+	printk("sig=%lx, smi_port=%d, smi_cmd=%d\n",
+		ist_info.signature, smi_port, smi_cmd);
+
 	/* Error if no IST-SMI BIOS or no PARM 
 		 sig= 'ISGE' aka 'Intel Speedstep Gate E' */
 	if ((ist_info.signature !=  0x47534943) && ( 
_

Comment 2 Venkatesh Pallipadi 2005-11-11 19:37:11 UTC
Also. Try modprobe'ing multiple times. There is something strange about
speedstep-smi that it works sometimes and doesn;t work at others...

Look at this bug #5082 as well

Comment 3 Theodor Milkov 2005-11-13 06:08:09 UTC
With the patch provided by Andrew Morton:

CONFIG_CPU_FREQ_DEBUG=n

# modprobe speedstep-smi
FATAL: Error inserting speedstep_smi (/.../speedstep-smi.ko): No such device
kernel: sig=47534943, smi_port=0, smi_cmd=0

# modprobe speedstep-smi smi_port=0xb2 smi_cmd=0x82
FATAL: Error inserting speedstep_smi (/.../speedstep-smi.ko): No such device
kernel: sig=47534943, smi_port=178, smi_cmd=130

As per venkatesh.pallipadi at intel comments I've tried:

# for i in `seq 1 100`; do modprobe speedstep-smi ;done

But still no go.

---

CONFIG_CPU_FREQ_DEBUG=y

# modprobe speedstep-smi
kernel: sig=47534943, smi_port=0, smi_cmd=0

# rmmod speedstep-smi

# modprobe speedstep-smi smi_port=0xb2 smi_cmd=0x82
kernel: sig=47534943, smi_port=178, smi_cmd=130

With debug enabled it just works.
Comment 4 Dominik Brodowski 2006-01-08 14:24:10 UTC
Created attachment 6973 [details]
wait a bit before trying transitions

This is one of the strangest bugs I've ever seen... As there is no code change,
just a few lines of printk() added -- which might slow down things a bit, which
is what might be needed in this case... does this patch help, by chance?
Comment 5 Theodor Milkov 2006-01-21 13:21:33 UTC
Unfortunately the patch in attachment id 6973 did not solve the problem. I've 
tryed to nail down the time critical place by changing dprintk()s to printk() 
one by one. speedstep-smi is working for me with the changes in the attached 
patch. 
Comment 6 Theodor Milkov 2006-01-21 13:24:08 UTC
Created attachment 7093 [details]
change dprintk to printk in speedstep_smi_ownership()
Comment 7 Hiroshi Miura 2006-01-31 20:37:05 UTC
It's strange!

Theodor wrote;
>cpufreq used to work with 2.6.8 and 2.6.11 kernels, but doesn't work >with
2.6.12 and 2.6.14 for me. 

and inserting printk before calling SMI BIOS solves a problem.  

But in 2.6.8, speedstep-smi driver DON'T have printk() there.
Do you use same compiler version as compiling 2.6.8 or 2.6.11? 
If the problem is this timing issue, 2.6.8 should have same problem, too.  





 
Comment 8 Hiroshi Miura 2006-02-05 06:08:32 UTC
Uhmm, it seems some cache coherency problem.
Touching 'command' variable in printk() flashes cache including 'magic_data',
doesn't it?

How about moving definition of magic_data[] to global?
The valiable will be initialized at load time. 
--- speedstep-smi.c.orig        2005-09-17 10:02:12.000000000 +0900
+++ speedstep-smi.c     2006-02-05 23:52:16.000000000 +0900
@@ -61,11 +61,13 @@
 /**
  * speedstep_smi_ownership
  */
+
+static unsigned char magic_data[] = "Copyright (c) 1999 Intel Corporation";
+
 static int speedstep_smi_ownership (void)
 {
        u32 command, result, magic;
        u32 function = GET_SPEEDSTEP_OWNER;
-       unsigned char magic_data[] = "Copyright (c) 1999 Intel Corporation";

        command = (smi_sig & 0xffffff00) | (smi_cmd & 0xff);
        magic = virt_to_phys(magic_data);
Comment 9 Antonio Ospite 2006-03-23 11:24:12 UTC
Hallo, I also have this problem on linux 2.6.16, and the patch in
http://bugzilla.kernel.org/show_bug.cgi?id=5553#c8 does not help.

I found that any printk() statement before the asm code seems to fix the symptom
(hence the driver can be loaded without problems).

Just for istance, the following patch make the module loadable without touching
explicitly the 'command' variable or anything else.

--- speedstep-smi.c.orig	2006-03-23 19:22:05.000000000 +0100
+++ speedstep-smi.c	2006-03-23 20:05:28.000000000 +0100
@@ -70,6 +70,7 @@ static int speedstep_smi_ownership (void
 	command = (smi_sig & 0xffffff00) | (smi_cmd & 0xff);
 	magic = virt_to_phys(magic_data);
 
+	printk("Loading speedstep-smi.\n");
 	dprintk("trying to obtain ownership with command %x at port %x\n", command,
smi_port);
 
 	__asm__ __volatile__(


Maybe a workaround like this can be used in a future kernel version to let users
load the module, while thinking to a real fix (it sounds a bit 'redmondish', I
know :) )

Thanks,
   Antonio Ospite.
Comment 10 Andrew Morton 2006-03-23 16:17:31 UTC
Linus Torvalds <torvalds@osdl.org> wrote:
>
> 
> 
> On Thu, 23 Mar 2006, Andrew Morton wrote:
> > 
> > hm.  "any printk before the asm seems to fix the symptom".  That makes me
> > think that the asm might have incorrect constraints or clobbers.
> > 
> > Linus, you're the man.  Does this look OK?
> 
> The thing looks fine, so I suspect it is just that the printk() changes 
> timings.
> 
> On the other hand, it does seem to depend on the _memory_ contents of that 
> "magic_data[]", so maybe adding a
> 
> 	: "memory"
> 
> at the end of the asm would actually help some?
> 
> 		Linus

Can someone please test this?


--- 25/arch/i386/kernel/cpu/cpufreq/speedstep-smi.c~a	Thu Mar 23 16:18:19 2006
+++ 25-akpm/arch/i386/kernel/cpu/cpufreq/speedstep-smi.c	Thu Mar 23 16:19:07 2006
@@ -75,7 +75,9 @@ static int speedstep_smi_ownership (void
 	__asm__ __volatile__(
 		"out %%al, (%%dx)\n"
 		: "=D" (result)
-		: "a" (command), "b" (function), "c" (0), "d" (smi_port), "D" (0), "S" (magic)
+		: "a" (command), "b" (function), "c" (0), "d" (smi_port),
+			"D" (0), "S" (magic)
+		: "memory"
 	);
 
 	dprintk("result is %x\n", result);
_

Comment 11 Antonio Ospite 2006-03-24 05:59:56 UTC
Great!
The latest patch from Andrew worked for me. 
Hope to see this change in the next version.

Many thanks to Linus... again.

Regards,
   Antonio
Comment 12 Andrew Morton 2006-03-24 10:45:02 UTC
Thanks for confirming.  I'll send this in for 2.6.16.1 as well.
Comment 13 Len Brown 2011-07-30 05:19:16 UTC
closed by:


commit f081a529f808ed450c22553de7b3275e0ffde9a0
Author: Andrew Morton <akpm@osdl.org>
Date:   Sat Mar 25 01:51:51 2006 -0800

    [PATCH] cpufreq: speedstep-smi asm fix
    
    Fix bug identified by Linus Torvalds <torvalds@osdl.org>: the `out'
    instruction depends upon the state of memory_data[], so we need to tell gcc
    that before executing it. (The opcode, not gcc).
    
    Fixes http://bugzilla.kernel.org/show_bug.cgi?id=5553
    
    Thanks to Antonio Ospite <ospite@studenti.unina.it> for testing.
    
    Cc: Dave Jones <davej@codemonkey.org.uk>
    Signed-off-by: Andrew Morton <akpm@osdl.org>
    Signed-off-by: Linus Torvalds <torvalds@osdl.org>