Patch to stop i386 longhaul from deadlocking the kernel

Patch to stop i386 longhaul from deadlocking the kernel

[Ashley Pittman]

Hi,

I've had some problems with the 'longhaul' driver deadlocking my kernel
when the module is inserted.  I tracked this down to the module failing
to discover the fsb speed.

This patch stops the deadlock in the case where the speed of the fsb is
undetected.  I've also made the guess function more vague so it can
detect the correct speed for my cpu, it might need to be even more vague
for other cpu's.

I am still unable to change the CPU speed even with this patch however. 

The problem arrived in about the -test6 timeframe and still exists in
the latest test9 I've tested.  The motherboard is a mini-itx from via
running at 800.261 Mhz.

The relevent kernel output is:

Before:

longhaul: VIA C3 'Ezra' [C5C] CPU detected. Longhaul v1 supported.
longhaul: MinMult=3.0x MaxMult=6.0x
longhaul: FSB: 0MHz Lowestspeed=0MHz Highestspeed=0MHz

At this point the whole system would lock solid.

After:

longhaul: VIA C3 'Ezra' [C5C] CPU detected. Longhaul v1 supported.
longhaul: MinMult=3.0x MaxMult=6.0x
longhaul: FSB: 133MHz Lowestspeed=399MHz Highestspeed=798MHz

The module is now inserted and appears in '/proc/modules'

Ashley,


diff -u linux-2.6.0-test9-dist/arch/i386/kernel/cpu/cpufreq/longhaul.c linux-2.6.0-test9/arch/i386/kernel/cpu/cpufreq/longhaul.c
--- linux-2.6.0-test9-dist/arch/i386/kernel/cpu/cpufreq/longhaul.c      2003-10-29 18:50:54.000000000 +0000
+++ linux-2.6.0-test9/arch/i386/kernel/cpu/cpufreq/longhaul.c   2003-11-04 11:36:43.000000000 +0000
@@ -30,6 +30,9 @@
 
 #include "longhaul.h"
 
+#define SPEEDKLUDGE 0xbf
+
+
 #define DEBUG
 
 #ifdef DEBUG
@@ -177,13 +180,13 @@
        target = ((maxmult/10)*guess);
        if (maxmult%10 != 0)
                target += (guess/2);
-       target &= ~0xf;
+       target &= ~SPEEDKLUDGE;
        return target;
 }
 
 static int guess_fsb(int maxmult)
 {
-       int speed = (cpu_khz/1000) & ~0xf;
+       int speed = (cpu_khz/1000) & ~SPEEDKLUDGE;
        int i;
        int speeds[3] = { 66, 100, 133 };
 
@@ -248,6 +251,10 @@
        dprintk (KERN_INFO PFX "FSB: %dMHz Lowestspeed=%dMHz Highestspeed=%dMHz\n",
                 fsb, lowest_speed, highest_speed);
 
+       if ( fsb == 0 ) {
+               return -EINVAL;
+       }
+
        longhaul_table = kmalloc((numscales + 1) * sizeof(struct cpufreq_frequency_table), GFP_KERNEL);
        if(!longhaul_table)
                return -ENOMEM;



-- 
Ashley Pittman <ashley@quadrics.com>

Re: Patch to stop i386 longhaul from deadlocking the kernel

[Dave Jones]

On Wed, Nov 05, 2003 at 03:19:30PM +0000, Ashley Pittman wrote:

 > This patch stops the deadlock in the case where the speed of the fsb is
 > undetected.  I've also made the guess function more vague so it can
 > detect the correct speed for my cpu, it might need to be even more vague
 > for other cpu's.

0xbf seems bizarre. Something went wrong somewhere.
Can you put some printk's in there, so I can see what values _guess()
and guess_fsb() are being called with ?

 > I am still unable to change the CPU speed even with this patch however. 

You're not the first report I've had of this. I'm not sure what I broke.
It was a case of 'fix for some, break others'.

 > longhaul: VIA C3 'Ezra' [C5C] CPU detected. Longhaul v1 supported.
 > longhaul: MinMult=3.0x MaxMult=6.0x
 > longhaul: FSB: 0MHz Lowestspeed=0MHz Highestspeed=0MHz
 > 
 > At this point the whole system would lock solid.

Why it locks up is also odd.

		Dave

Re: Patch to stop i386 longhaul from deadlocking the kernel

[Ashley Pittman]

On Thu, 2003-11-20 at 12:46, Dave Jones wrote:
> On Wed, Nov 05, 2003 at 03:19:30PM +0000, Ashley Pittman wrote:
> 
>  > This patch stops the deadlock in the case where the speed of the fsb is
>  > undetected.  I've also made the guess function more vague so it can
>  > detect the correct speed for my cpu, it might need to be even more vague
>  > for other cpu's.
> 
> 0xbf seems bizarre. Something went wrong somewhere.
> Can you put some printk's in there, so I can see what values _guess()
> and guess_fsb() are being called with ?

The computer is at home today but it's one of the 800Mhz boards, the fsb
is 133, multiplier of 60.  The speed should therefore be 798 but cpu_khz
is reporting it as 800.  Then it's just a case of how many bits you need
to remove of before 798==800.  The result is that they both need to
equal 768.  I've attached a user-space program which demonstrates this
with output.

>  > I am still unable to change the CPU speed even with this patch however. 
> 
> You're not the first report I've had of this. I'm not sure what I broke.
> It was a case of 'fix for some, break others'.
> 
>  > longhaul: VIA C3 'Ezra' [C5C] CPU detected. Longhaul v1 supported.
>  > longhaul: MinMult=3.0x MaxMult=6.0x
>  > longhaul: FSB: 0MHz Lowestspeed=0MHz Highestspeed=0MHz
>  > 
>  > At this point the whole system would lock solid.
> 
> Why it locks up is also odd.

I didn't get anywhere with that, I took the route of stopping the code
when a error condition was detected, returning -EFAULT avoided the
problem nicely.

Ashley,

optput:
ashley:cpufreq> ./a.out 
Kludge is 15 0xf
Speed is 800 0x320 (800 0x320)
Working... guess 66 maxmult 60 target 384 0x180
Working... guess 100 maxmult 60 target 592 0x250
Working... guess 133 maxmult 60 target 784 0x310
The answer is 0
Kludge is 191 0xbf
Speed is 768 0x300 (800 0x320)
Working... guess 66 maxmult 60 target 256 0x100
Working... guess 100 maxmult 60 target 576 0x240
Working... guess 133 maxmult 60 target 768 0x300
The second answer is 133
ashley:cpufreq> 


test.c:

int SPEEDKLUDGE = 0;
int cpu_khz = 800000;

static int _guess (int guess, int maxmult)
{
	int target;
	
	target = ((maxmult/10)*guess);
	if (maxmult%10 != 0)
		target += (guess/2);
	target &= ~SPEEDKLUDGE;
	printf("Working... guess %d maxmult %d target %d %#x\n",
	       guess,maxmult,target,target);
	return target;
}

static int guess_fsb(int maxmult)
{
	int speed = (cpu_khz/1000) & ~SPEEDKLUDGE;
	int i;
	int speeds[3] = { 66, 100, 133 };
	
	printf("Kludge is %d %#x\n",SPEEDKLUDGE,SPEEDKLUDGE);
	
	printf("Speed is %d %#x (%d %#x)\n",
	       speed,speed,
	       (cpu_khz/1000),(cpu_khz/1000));
	
	for (i=0; i<3; i++) {
		if (_guess(speeds[i],maxmult) == speed)
			return speeds[i];
	}
	return 0;
}

main () {
	SPEEDKLUDGE = 0xf;
	printf("The answer is %d\n",guess_fsb(60));
	SPEEDKLUDGE = 0xbf;
	printf("The second answer is %d\n",guess_fsb(60));
}

Re: Patch to stop i386 longhaul from deadlocking the kernel

[Dave Jones]

On Thu, Nov 20, 2003 at 01:24:04PM +0000, Ashley Pittman wrote:
 > The computer is at home today but it's one of the 800Mhz boards, the fsb
 > is 133, multiplier of 60.  The speed should therefore be 798 but cpu_khz
 > is reporting it as 800.  Then it's just a case of how many bits you need
 > to remove of before 798==800.  The result is that they both need to
 > equal 768.  I've attached a user-space program which demonstrates this
 > with output.

Right, so the anding is catching the case of overflow, but neglecting
underflow. It seems to do the right thing if I change guess_fsb to do this..

    for (i=0; i<3; i++) {
        ret = _guess(speeds[i],maxmult);
        if (ret == speed)
            return speeds[i];
        if (ret < speed) {
            if ((ret+16) == speed)
                return speeds[i];
        }
    }
    return 0;
}

I need to think about this some more. I'm not entirely comfortable with
that, it looks somewhat fragile, though I can see why it works.
133 * 6 = 798
798 & ~0xf = 784
784 + 16 = 800

I wonder how well this kludge works for other frequencies.

		Dave

Re: Patch to stop i386 longhaul from deadlocking the kernel

[Ashley Pittman]

On Thu, 2003-11-20 at 13:40, Dave Jones wrote:
> On Thu, Nov 20, 2003 at 01:24:04PM +0000, Ashley Pittman wrote:
>  > The computer is at home today but it's one of the 800Mhz boards, the fsb
>  > is 133, multiplier of 60.  The speed should therefore be 798 but cpu_khz
>  > is reporting it as 800.  Then it's just a case of how many bits you need
>  > to remove of before 798==800.  The result is that they both need to
>  > equal 768.  I've attached a user-space program which demonstrates this
>  > with output.
> 
> Right, so the anding is catching the case of overflow, but neglecting
> underflow. It seems to do the right thing if I change guess_fsb to do this..

> I need to think about this some more. I'm not entirely comfortable with
> that, it looks somewhat fragile, though I can see why it works.

Isn't the correct thing to do to increase the speed before anding it? 
That way it will always end up in the right bin.  The following code
works for me and does feel right.  A value of 0x7 is all thats needed to
get the right answer on my machine in this case.

static int _guess (int guess, int maxmult)
{
	int target;
	
	target = ((maxmult/10)*guess);
	if (maxmult%10 != 0)
		target += (guess/2);
	target += SPEEDKLUDGE/2;
	target &= ~SPEEDKLUDGE;
	printf("Working... guess %d maxmult %d target %d %#x\n",
	       guess,maxmult,target,target);
	return target;
}

static int guess_fsb(int maxmult)
{
	int speed = (cpu_khz/1000);
	int i;
	int speeds[3] = { 66, 100, 133 };
	
	speed += SPEEDKLUDGE/2;
	speed &= ~SPEEDKLUDGE;
	
	printf("Kludge is %d %#x\n",(int)SPEEDKLUDGE,(int)SPEEDKLUDGE);
	
	printf("Speed is %d %#x (%d %#x)\n",
	       speed,speed,
	       (cpu_khz/1000),(cpu_khz/1000));
	
	for (i=0; i<3; i++) {
		if (_guess(speeds[i],maxmult) == speed)
			return speeds[i];
	}
	return 0;
}