m68k: Patch for broken lsl64() "old code" with incorrect shift

[1.] One line summary of the problem:

[PATCH] m68k: Patch for broken lsl64() "old code" with incorrect shift

[2.] Full description of the problem/report:

In arch/m68k/math-emu/multi_arith.h, lsl64() doesn't calculate a correct HI_WORD(*dest) when count < 32, due to an incorrect shift operation. This line:

    HI_WORD(*dest) = (HI_WORD(*dest) << count) | (LO_WORD(*dest) >> count);

...should be...

    HI_WORD(*dest) = (HI_WORD(*dest) << count) | (LO_WORD(*dest) >>

(32 - count));

This function is ifdef'd out, marked as "old code". However, I fell into the same trap into which I suspect others have fallen (or will fall): On rare occasion when I need to implement my own library functions, I depend on the Linux kernel to provide solid code. This bug, albeit in old code, led me astray.

I'm asking that this old code (lsl64) be corrected or removed.

[3.] Keywords (i.e., modules, networking, kernel):

kernel, arch, m68k, math-emu, multi_arith.h, lsl64

[4.] Kernel information

2.6.39.1

[5.] Most recent kernel version which did not have the bug:

Unknown

[6.] Output of Oops.. message (if applicable) with symbolic information resolved (see Documentation/oops-tracing.txt)

N/A

[7.] A small shell script or example program which triggers the problem (if possible)

-----------------------------------------------------------------
#include <stdio.h>

#define LO_WORD(ll) (((unsigned int *) &ll)[1])
#define HI_WORD(ll) (((unsigned int *) &ll)[0])

static inline void lsl64_broken(int count, unsigned long long *dest)

{
	if (count < 32) {
		HI_WORD(*dest) = (HI_WORD(*dest) << count)
		    | (LO_WORD(*dest) >> count);
		LO_WORD(*dest) <<= count;
		return;
	}
	count -= 32;
	HI_WORD(*dest) = LO_WORD(*dest) << count;
	LO_WORD(*dest) = 0;
}

static inline void lsl64_fixed(int count, unsigned long long *dest)

{
	if (count < 32) {
		HI_WORD(*dest) = (HI_WORD(*dest) << count)
		    | (LO_WORD(*dest) >> (32 - count));
		LO_WORD(*dest) <<= count;
		return;
	}
	count -= 32;
	HI_WORD(*dest) = LO_WORD(*dest) << count;
	LO_WORD(*dest) = 0;
}

static inline void lsr64(int count, unsigned long long *dest)

{
	if (count < 32) {
		LO_WORD(*dest) = (LO_WORD(*dest) >> count)
		    | (HI_WORD(*dest) << (32 - count));
		HI_WORD(*dest) >>= count;
		return;
	}
	count -= 32;
	LO_WORD(*dest) = HI_WORD(*dest) >> count;
	HI_WORD(*dest) = 0;
}

int main( void )

{
	long long testVal = 0x0000100000001000L;
	long long testVal_broken = testVal;
	long long testVal_fixed = testVal;
	
	printf( "begin testVal_broken =\t\t0x%016llx\n", testVal_broken );
	lsr64( 1, &testVal_broken );
	printf( "intermediate testVal_broken =\t0x%016llx\n", testVal_broken );
	lsl64_broken( 1, &testVal_broken );
	printf( "final testVal_broken =\t\t0x%016llx\n", testVal_broken );
	printf( "final matches begin?\t\t%s\n", testVal_broken == testVal ?

"TRUE" : "FALSE" );

	printf( "\nbegin testVal_fixed =\t\t0x%016llx\n", testVal_fixed );
	lsr64( 1, &testVal_fixed );
	printf( "intermediate testVal_fixed =\t0x%016llx\n", testVal_fixed );
	lsl64_fixed( 1, &testVal_fixed );
	printf( "final testVal_fixed =\t\t0x%016llx\n", testVal_fixed );
	printf( "final matches begin?\t\t%s\n", testVal_fixed == testVal ?

"TRUE" : "FALSE" );

	return 0;
}

-----------------------------------------------------------------

[8.] Environment

Found via code inspection

[9.] Patch

-----------------------------------------------------------------
diff -up linux-2.6/arch/m68k/math-emu/multi_arith.h linux-2.6_orig/arch/m68k/math-emu/multi_arith.h
--- linux-2.6/arch/m68k/math-emu/multi_arith.h 2011-06-10
22:50:32.538711320 -0400
+++ linux-2.6_orig/arch/m68k/math-emu/multi_arith.h 2011-06-10
22:47:43.407285452 -0400
@@ -236,7 +236,7 @@ static inline void lsl64(int count, unsi
{
if (count < 32) {
HI_WORD(*dest) = (HI_WORD(*dest) << count)
- | (LO_WORD(*dest) >> (32 - count));
+ | (LO_WORD(*dest) >> count);
LO_WORD(*dest) <<= count;
return;
}
-----------------------------------------------------------------

[10.] Responses

Please personally CC me on responses to this post.

--
Jonathan Elchison
[email protected]

I think it's better to just remove all #ifdefed out parts in the code.

I'm good either way, as long as the broken "old code" is no longer present.

My apologies, you're correct. In case anyone is interested in the correct patch:

-----------------------------------------------------------------
diff -up linux-2.6_orig/arch/m68k/math-emu/multi_arith.h linux-2.6/arch/m68k/math-emu/multi_arith.h
--- linux-2.6_orig/arch/m68k/math-emu/multi_arith.h 2011-06-10
22:47:43.407285452 -0400
+++ linux-2.6/arch/m68k/math-emu/multi_arith.h 2011-06-10
22:50:32.538711320 -0400
@@ -236,7 +236,7 @@ static inline void lsl64(int count, unsi
{
if (count < 32) {
HI_WORD(*dest) = (HI_WORD(*dest) << count)
- | (LO_WORD(*dest) >> count);
+ | (LO_WORD(*dest) >> (32 - count));
LO_WORD(*dest) <<= count;
return;
}
-----------------------------------------------------------------

Please personally CC me on responses to this post.

v/r,

--
Jonathan Elchison
[email protected]