Revert "sh: Handle calling csum_partial with misaligned data"

PATCH Applied Guenter Roeck 3 participants 12 messages Mar 24, 2024 – May 2, 2024 SuperH

lore

Share & tools

lore

Guenter Roeck

author

2024-03-24 23:18 UTC #0 lore raw

PATCH 1/1 Revert "sh: Handle calling csum_partial with misaligned data"

This reverts commit cadc4e1a2b4d20d0cc0e81f2c6ba0588775e54e5.

Commit cadc4e1a2b4d ("sh: Handle calling csum_partial with misaligned data") causes bad checksum calculations on unaligned data. Reverting it fixes the problem.

# Subtest: checksum
# module: checksum_kunit
1..5
# test_csum_fixed_random_inputs: ASSERTION FAILED at lib/checksum_kunit.c:500 Expected ( u64)result == ( u64)expec, but
( u64)result == 53378 (0xd082)
( u64)expec == 33488 (0x82d0)
# test_csum_fixed_random_inputs: pass:0 fail:1 skip:0 total:1 not ok 1 test_csum_fixed_random_inputs
# test_csum_all_carry_inputs: ASSERTION FAILED at lib/checksum_kunit.c:525 Expected ( u64)result == ( u64)expec, but
( u64)result == 65281 (0xff01)
( u64)expec == 65280 (0xff00)
# test_csum_all_carry_inputs: pass:0 fail:1 skip:0 total:1
not ok 2 test_csum_all_carry_inputs
# test_csum_no_carry_inputs: ASSERTION FAILED at lib/checksum_kunit.c:573 Expected ( u64)result == ( u64)expec, but
( u64)result == 65535 (0xffff)
( u64)expec == 65534 (0xfffe)
# test_csum_no_carry_inputs: pass:0 fail:1 skip:0 total:1
not ok 3 test_csum_no_carry_inputs
# test_ip_fast_csum: pass:1 fail:0 skip:0 total:1
ok 4 test_ip_fast_csum
# test_csum_ipv6_magic: pass:1 fail:0 skip:0 total:1
ok 5 test_csum_ipv6_magic
# checksum: pass:2 fail:3 skip:0 total:5
# Totals: pass:2 fail:3 skip:0 total:5
not ok 22 checksum

Fixes cadc4e1a2b4d ("sh: Handle calling csum_partial with misaligned data")

Signed-off-by Guenter Roeck <[email protected]>

---
 arch/sh/lib/checksum.S | 67 ++++++++++++------------------------------
 1 file changed, 18 insertions(+), 49 deletions(-)

arch/sh/lib/checksum.S +18 ~~-49~~

index 3e07074e0098..06fed5a21e8b 100644
@@ -33,7 +33,8 @@
  */
 
 /*	
- * asmlinkage __wsum csum_partial(const void *buf, int len, __wsum sum);
+ * unsigned int csum_partial(const unsigned char *buf, int len,
+ *                           unsigned int sum);
  */
 
 .text

@@ -45,31 +46,11 @@ ENTRY(csum_partial)
 	   * Fortunately, it is easy to convert 2-byte alignment to 4-byte
 	   * alignment for the unrolled loop.
 	   */
+	mov	r5, r1
 	mov	r4, r0
-	tst	#3, r0		! Check alignment.
-	bt/s	2f		! Jump if alignment is ok.
-	 mov	r4, r7		! Keep a copy to check for alignment
+	tst	#2, r0		! Check alignment.
+	bt	2f		! Jump if alignment is ok.
 	!
-	tst	#1, r0		! Check alignment.
-	bt	21f		! Jump if alignment is boundary of 2bytes.
-
-	! buf is odd
-	tst	r5, r5
-	add	#-1, r5
-	bt	9f
-	mov.b	@r4+, r0
-	extu.b	r0, r0
-	addc	r0, r6		! t=0 from previous tst
-	mov	r6, r0
-	shll8	r6
-	shlr16	r0
-	shlr8	r0
-	or	r0, r6
-	mov	r4, r0
-	tst	#2, r0
-	bt	2f
-21:
-	! buf is 2 byte aligned (len could be 0)
 	add	#-2, r5		! Alignment uses up two bytes.
 	cmp/pz	r5		!
 	bt/s	1f		! Jump if we had at least two bytes.

@@ -77,17 +58,16 @@ ENTRY(csum_partial)
 	bra	6f
 	 add	#2, r5		! r5 was < 2.  Deal with it.
 1:
+	mov	r5, r1		! Save new len for later use.
 	mov.w	@r4+, r0
 	extu.w	r0, r0
 	addc	r0, r6
 	bf	2f
 	add	#1, r6
 2:
-	! buf is 4 byte aligned (len could be 0)
-	mov	r5, r1
 	mov	#-5, r0
-	shld	r0, r1
-	tst	r1, r1
+	shld	r0, r5
+	tst	r5, r5
 	bt/s	4f		! if it's =0, go to 4f
 	 clrt
 	.align	2

@@ -109,31 +89,30 @@ ENTRY(csum_partial)
 	addc	r0, r6
 	addc	r2, r6
 	movt	r0
-	dt	r1
+	dt	r5
 	bf/s	3b
 	 cmp/eq	#1, r0
-	! here, we know r1==0
-	addc	r1, r6			! add carry to r6
+	! here, we know r5==0
+	addc	r5, r6			! add carry to r6
 4:
-	mov	r5, r0
+	mov	r1, r0
 	and	#0x1c, r0
 	tst	r0, r0
-	bt	6f
-	! 4 bytes or more remaining
-	mov	r0, r1
-	shlr2	r1
+	bt/s	6f
+	 mov	r0, r5
+	shlr2	r5
 	mov	#0, r2
 5:
 	addc	r2, r6
 	mov.l	@r4+, r2
 	movt	r0
-	dt	r1
+	dt	r5
 	bf/s	5b
 	 cmp/eq	#1, r0
 	addc	r2, r6
-	addc	r1, r6		! r1==0 here, so it means add carry-bit
+	addc	r5, r6		! r5==0 here, so it means add carry-bit
 6:
-	! 3 bytes or less remaining
+	mov	r1, r5
 	mov	#3, r0
 	and	r0, r5
 	tst	r5, r5

@@ -159,16 +138,6 @@ ENTRY(csum_partial)
 	mov	#0, r0
 	addc	r0, r6
 9:
-	! Check if the buffer was misaligned, if so realign sum
-	mov	r7, r0
-	tst	#1, r0
-	bt	10f
-	mov	r6, r0
-	shll8	r6
-	shlr16	r0
-	shlr8	r0
-	or	r0, r6
-10:
 	rts
 	 mov	r6, r0

Signature · 1 line

2.39.2

John Paul Adrian Glaubitz

maintainer

2024-03-25 07:39 UTC #1 lore raw

Hi Guenter,

Quoted 26 lines · Guenter Roeck

This reverts commit cadc4e1a2b4d20d0cc0e81f2c6ba0588775e54e5.
Commit cadc4e1a2b4d ("sh: Handle calling csum_partial with misaligned data") causes bad checksum calculations on unaligned data. Reverting it fixes the problem.
# Subtest: checksum
# module: checksum_kunit
1..5
# test_csum_fixed_random_inputs: ASSERTION FAILED at lib/checksum_kunit.c:500 Expected ( u64)result == ( u64)expec, but
( u64)result == 53378 (0xd082)
( u64)expec == 33488 (0x82d0)
# test_csum_fixed_random_inputs: pass:0 fail:1 skip:0 total:1 not ok 1 test_csum_fixed_random_inputs
# test_csum_all_carry_inputs: ASSERTION FAILED at lib/checksum_kunit.c:525 Expected ( u64)result == ( u64)expec, but
( u64)result == 65281 (0xff01)
( u64)expec == 65280 (0xff00)
# test_csum_all_carry_inputs: pass:0 fail:1 skip:0 total:1
not ok 2 test_csum_all_carry_inputs
# test_csum_no_carry_inputs: ASSERTION FAILED at lib/checksum_kunit.c:573 Expected ( u64)result == ( u64)expec, but
( u64)result == 65535 (0xffff)
( u64)expec == 65534 (0xfffe)
# test_csum_no_carry_inputs: pass:0 fail:1 skip:0 total:1

Quoted 8 lines

not ok 3 test_csum_no_carry_inputs
# test_ip_fast_csum: pass:1 fail:0 skip:0 total:1 ok 4 test_ip_fast_csum
# test_csum_ipv6_magic: pass:1 fail:0 skip:0 total:1 ok 5 test_csum_ipv6_magic
# checksum: pass:2 fail:3 skip:0 total:5
# Totals: pass:2 fail:3 skip:0 total:5
not ok 22 checksum

Can you tell me how the tests are run so I can try to verify this on real hardware?

Adrian

Signature · 4 lines

 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Guenter Roeck

author

2024-03-25 14:34 UTC #2 lore raw

Quoted 38 lines · John Paul Adrian Glaubitz

Hi Guenter,
Quoted 26 lines · Guenter Roeck
This reverts commit cadc4e1a2b4d20d0cc0e81f2c6ba0588775e54e5.
Commit cadc4e1a2b4d ("sh: Handle calling csum_partial with misaligned data") causes bad checksum calculations on unaligned data. Reverting it fixes the problem.
# Subtest: checksum
# module: checksum_kunit
1..5
# test_csum_fixed_random_inputs: ASSERTION FAILED at lib/checksum_kunit.c:500 Expected ( u64)result == ( u64)expec, but
( u64)result == 53378 (0xd082)
( u64)expec == 33488 (0x82d0)
# test_csum_fixed_random_inputs: pass:0 fail:1 skip:0 total:1 not ok 1 test_csum_fixed_random_inputs
# test_csum_all_carry_inputs: ASSERTION FAILED at lib/checksum_kunit.c:525 Expected ( u64)result == ( u64)expec, but
( u64)result == 65281 (0xff01)
( u64)expec == 65280 (0xff00)
# test_csum_all_carry_inputs: pass:0 fail:1 skip:0 total:1
not ok 2 test_csum_all_carry_inputs
# test_csum_no_carry_inputs: ASSERTION FAILED at lib/checksum_kunit.c:573 Expected ( u64)result == ( u64)expec, but
( u64)result == 65535 (0xffff)
( u64)expec == 65534 (0xfffe)
# test_csum_no_carry_inputs: pass:0 fail:1 skip:0 total:1
Quoted 8 lines
not ok 3 test_csum_no_carry_inputs
# test_ip_fast_csum: pass:1 fail:0 skip:0 total:1 ok 4 test_ip_fast_csum
# test_csum_ipv6_magic: pass:1 fail:0 skip:0 total:1 ok 5 test_csum_ipv6_magic
# checksum: pass:2 fail:3 skip:0 total:5
# Totals: pass:2 fail:3 skip:0 total:5
not ok 22 checksum

Quoted 2 lines

Can you tell me how the tests are run so I can try to verify this on real hardware?

Enabling CONFIG_KUNIT and CHECKSUM_KUNIT and booting with those tests enabled should do it.

Thanks,
Guenter

Guenter Roeck

author

2024-04-02 14:06 UTC #3 lore raw

Hi,

Quoted 39 lines · Guenter Roeck

Quoted 38 lines · John Paul Adrian Glaubitz
Hi Guenter,
Quoted 34 lines · Guenter Roeck
This reverts commit cadc4e1a2b4d20d0cc0e81f2c6ba0588775e54e5.
Commit cadc4e1a2b4d ("sh: Handle calling csum_partial with misaligned data") causes bad checksum calculations on unaligned data. Reverting it fixes the problem.
# Subtest: checksum
# module: checksum_kunit
1..5
# test_csum_fixed_random_inputs: ASSERTION FAILED at lib/checksum_kunit.c:500 Expected ( u64)result == ( u64)expec, but
( u64)result == 53378 (0xd082)
( u64)expec == 33488 (0x82d0)
# test_csum_fixed_random_inputs: pass:0 fail:1 skip:0 total:1 not ok 1 test_csum_fixed_random_inputs
# test_csum_all_carry_inputs: ASSERTION FAILED at lib/checksum_kunit.c:525 Expected ( u64)result == ( u64)expec, but
( u64)result == 65281 (0xff01)
( u64)expec == 65280 (0xff00)
# test_csum_all_carry_inputs: pass:0 fail:1 skip:0 total:1
not ok 2 test_csum_all_carry_inputs
# test_csum_no_carry_inputs: ASSERTION FAILED at lib/checksum_kunit.c:573 Expected ( u64)result == ( u64)expec, but
( u64)result == 65535 (0xffff)
( u64)expec == 65534 (0xfffe)
# test_csum_no_carry_inputs: pass:0 fail:1 skip:0 total:1
not ok 3 test_csum_no_carry_inputs
# test_ip_fast_csum: pass:1 fail:0 skip:0 total:1
ok 4 test_ip_fast_csum
# test_csum_ipv6_magic: pass:1 fail:0 skip:0 total:1
ok 5 test_csum_ipv6_magic
# checksum: pass:2 fail:3 skip:0 total:5
# Totals: pass:2 fail:3 skip:0 total:5
not ok 22 checksum

Quoted 6 lines

Quoted 2 lines
Can you tell me how the tests are run so I can try to verify this on real hardware?
Enabling CONFIG_KUNIT and CHECKSUM_KUNIT and booting with those tests enabled should do it.

Did you have time to test this on real hardware ?

Thanks,
Guenter

John Paul Adrian Glaubitz

maintainer

2024-04-02 14:14 UTC #4 lore raw

Quoted 42 lines · Guenter Roeck

Hi,
Quoted 39 lines · Guenter Roeck
Quoted 38 lines · John Paul Adrian Glaubitz
Hi Guenter,
Quoted 34 lines · Guenter Roeck
This reverts commit cadc4e1a2b4d20d0cc0e81f2c6ba0588775e54e5.
Commit cadc4e1a2b4d ("sh: Handle calling csum_partial with misaligned data") causes bad checksum calculations on unaligned data. Reverting it fixes the problem.
# Subtest: checksum
# module: checksum_kunit
1..5
# test_csum_fixed_random_inputs: ASSERTION FAILED at lib/checksum_kunit.c:500 Expected ( u64)result == ( u64)expec, but
( u64)result == 53378 (0xd082)
( u64)expec == 33488 (0x82d0)
# test_csum_fixed_random_inputs: pass:0 fail:1 skip:0 total:1 not ok 1 test_csum_fixed_random_inputs
# test_csum_all_carry_inputs: ASSERTION FAILED at lib/checksum_kunit.c:525 Expected ( u64)result == ( u64)expec, but
( u64)result == 65281 (0xff01)
( u64)expec == 65280 (0xff00)
# test_csum_all_carry_inputs: pass:0 fail:1 skip:0 total:1
not ok 2 test_csum_all_carry_inputs
# test_csum_no_carry_inputs: ASSERTION FAILED at lib/checksum_kunit.c:573 Expected ( u64)result == ( u64)expec, but
( u64)result == 65535 (0xffff)
( u64)expec == 65534 (0xfffe)
# test_csum_no_carry_inputs: pass:0 fail:1 skip:0 total:1
not ok 3 test_csum_no_carry_inputs
# test_ip_fast_csum: pass:1 fail:0 skip:0 total:1
ok 4 test_ip_fast_csum
# test_csum_ipv6_magic: pass:1 fail:0 skip:0 total:1
ok 5 test_csum_ipv6_magic
# checksum: pass:2 fail:3 skip:0 total:5
# Totals: pass:2 fail:3 skip:0 total:5
not ok 22 checksum

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
Can you tell me how the tests are run so I can try to verify this on real hardware?
Enabling CONFIG_KUNIT and CHECKSUM_KUNIT and booting with those tests enabled should do it.
Did you have time to test this on real hardware ?

Not yet. I just returned from Easter holidays and need to get synced with work first.

Adrian

Signature · 4 lines

 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

John Paul Adrian Glaubitz

maintainer

2024-05-01 08:28 UTC #5 lore raw

Hi Guenter,

Quoted 46 lines · John Paul Adrian Glaubitz

Quoted 45 lines · Guenter Roeck
Hi,
Quoted 42 lines · Guenter Roeck
Quoted 40 lines · John Paul Adrian Glaubitz
Hi Guenter,
Quoted 26 lines · Guenter Roeck
This reverts commit cadc4e1a2b4d20d0cc0e81f2c6ba0588775e54e5.
Commit cadc4e1a2b4d ("sh: Handle calling csum_partial with misaligned data") causes bad checksum calculations on unaligned data. Reverting it fixes the problem.
# Subtest: checksum
# module: checksum_kunit
1..5
# test_csum_fixed_random_inputs: ASSERTION FAILED at lib/checksum_kunit.c:500 Expected ( u64)result == ( u64)expec, but
( u64)result == 53378 (0xd082)
( u64)expec == 33488 (0x82d0)
# test_csum_fixed_random_inputs: pass:0 fail:1 skip:0 total:1 not ok 1 test_csum_fixed_random_inputs
# test_csum_all_carry_inputs: ASSERTION FAILED at lib/checksum_kunit.c:525 Expected ( u64)result == ( u64)expec, but
( u64)result == 65281 (0xff01)
( u64)expec == 65280 (0xff00)
# test_csum_all_carry_inputs: pass:0 fail:1 skip:0 total:1
not ok 2 test_csum_all_carry_inputs
# test_csum_no_carry_inputs: ASSERTION FAILED at lib/checksum_kunit.c:573 Expected ( u64)result == ( u64)expec, but
( u64)result == 65535 (0xffff)
( u64)expec == 65534 (0xfffe)
# test_csum_no_carry_inputs: pass:0 fail:1 skip:0 total:1
Quoted 8 lines
not ok 3 test_csum_no_carry_inputs
# test_ip_fast_csum: pass:1 fail:0 skip:0 total:1 ok 4 test_ip_fast_csum
# test_csum_ipv6_magic: pass:1 fail:0 skip:0 total:1 ok 5 test_csum_ipv6_magic
# checksum: pass:2 fail:3 skip:0 total:5
# Totals: pass:2 fail:3 skip:0 total:5
not ok 22 checksum
Can you tell me how the tests are run so I can try to verify this on real hardware?

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
Enabling CONFIG_KUNIT and CHECKSUM_KUNIT and booting with those tests enabled should do it.
Did you have time to test this on real hardware ?
Not yet. I just returned from Easter holidays and need to get synced with work first.

I might have to skip this for v6.10 as I haven't been able to test this yet.

I agree with the change in general, but I want to make sure I can reproduce this on real hardware.

Adrian

Signature · 4 lines

 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Guenter Roeck

author

2024-05-01 14:44 UTC #6 lore raw

Quoted 9 lines · John Paul Adrian Glaubitz

Quoted 3 lines
Quoted 1 line
Did you have time to test this on real hardware ?
Not yet. I just returned from Easter holidays and need to get synced with work first.
I might have to skip this for v6.10 as I haven't been able to test this yet.
I agree with the change in general, but I want to make sure I can reproduce this on real hardware.

No worries, This is not a new problem, after all, so it doesn't really make much of a difference.

Guenter

John Paul Adrian Glaubitz

maintainer

2024-05-01 14:50 UTC #7 lore raw

Quoted 5 lines · Guenter Roeck

Quoted 4 lines · John Paul Adrian Glaubitz
Quoted 3 lines
Quoted 1 line
Did you have time to test this on real hardware ?
Not yet. I just returned from Easter holidays and need to get synced with work first.

Quoted 8 lines

Quoted 5 lines
I might have to skip this for v6.10 as I haven't been able to test this yet.
I agree with the change in general, but I want to make sure I can reproduce this on real hardware.
No worries, This is not a new problem, after all, so it doesn't really make much of a difference.

OK, thanks. I'll be happy when I'm though with Geert's two series which take quite some time to review.

Adrian

Signature · 4 lines

 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Geert Uytterhoeven

Tested-by

2024-05-02 07:21 UTC #8 lore raw

In reply to John Paul Adrian Glaubitz

Quoted 49 lines · John Paul Adrian Glaubitz

Quoted 48 lines · John Paul Adrian Glaubitz
Quoted 47 lines · Guenter Roeck
Quoted 45 lines · Guenter Roeck
Quoted 38 lines · John Paul Adrian Glaubitz
Hi Guenter,
Quoted 26 lines · Guenter Roeck
This reverts commit cadc4e1a2b4d20d0cc0e81f2c6ba0588775e54e5.
Commit cadc4e1a2b4d ("sh: Handle calling csum_partial with misaligned data") causes bad checksum calculations on unaligned data. Reverting it fixes the problem.
# Subtest: checksum
# module: checksum_kunit
1..5
# test_csum_fixed_random_inputs: ASSERTION FAILED at lib/checksum_kunit.c:500 Expected ( u64)result == ( u64)expec, but
( u64)result == 53378 (0xd082)
( u64)expec == 33488 (0x82d0)
# test_csum_fixed_random_inputs: pass:0 fail:1 skip:0 total:1 not ok 1 test_csum_fixed_random_inputs
# test_csum_all_carry_inputs: ASSERTION FAILED at lib/checksum_kunit.c:525 Expected ( u64)result == ( u64)expec, but
( u64)result == 65281 (0xff01)
( u64)expec == 65280 (0xff00)
# test_csum_all_carry_inputs: pass:0 fail:1 skip:0 total:1
not ok 2 test_csum_all_carry_inputs
# test_csum_no_carry_inputs: ASSERTION FAILED at lib/checksum_kunit.c:573 Expected ( u64)result == ( u64)expec, but
( u64)result == 65535 (0xffff)
( u64)expec == 65534 (0xfffe)
# test_csum_no_carry_inputs: pass:0 fail:1 skip:0 total:1
Quoted 8 lines
not ok 3 test_csum_no_carry_inputs
# test_ip_fast_csum: pass:1 fail:0 skip:0 total:1 ok 4 test_ip_fast_csum
# test_csum_ipv6_magic: pass:1 fail:0 skip:0 total:1 ok 5 test_csum_ipv6_magic
# checksum: pass:2 fail:3 skip:0 total:5
# Totals: pass:2 fail:3 skip:0 total:5
not ok 22 checksum
Quoted 2 lines
Can you tell me how the tests are run so I can try to verify this on real hardware?
Enabling CONFIG_KUNIT and CHECKSUM_KUNIT and booting with those tests enabled should do it.

Quoted 8 lines

Quoted 3 lines
Quoted 1 line
Did you have time to test this on real hardware ?
Not yet. I just returned from Easter holidays and need to get synced with work first.
I might have to skip this for v6.10 as I haven't been able to test this yet.
I agree with the change in general, but I want to make sure I can reproduce this on real hardware.

On landisk:

KTAP version 1
1..1
KTAP version 1
# Subtest: checksum
# module: checksum_kunit
1..5
- # test_csum_fixed_random_inputs: ASSERTION FAILED at
lib/checksum_kunit.c:500
- Expected ( u64)result == ( u64)expec, but
- ( u64)result == 53378 (0xd082)
- ( u64)expec == 33488 (0x82d0)

-    not ok 1 test_csum_fixed_random_inputs
-    # test_csum_all_carry_inputs: ASSERTION FAILED at lib/checksum_kunit.c:525
-    Expected ( u64)result == ( u64)expec, but
-        ( u64)result == 65281 (0xff01)
-        ( u64)expec == 65280 (0xff00)
-    not ok 2 test_csum_all_carry_inputs
-    # test_csum_no_carry_inputs: ASSERTION FAILED at lib/checksum_kunit.c:573
-    Expected ( u64)result == ( u64)expec, but

- ( u64)result == 65535 (0xffff)
- ( u64)expec == 65534 (0xfffe)
- not ok 3 test_csum_no_carry_inputs
+ # test_csum_fixed_random_inputs: Test should be marked slow
(runtime: 9.814991070s)
+ ok 1 test_csum_fixed_random_inputs
+ # test_csum_all_carry_inputs: Test should be marked slow
(runtime: 19.621274580s)
+ ok 2 test_csum_all_carry_inputs
+ # test_csum_no_carry_inputs: Test should be marked slow (runtime:
19.614096540s)
+ ok 3 test_csum_no_carry_inputs
ok 4 test_ip_fast_csum
ok 5 test_csum_ipv6_magic
-# checksum: pass:2 fail:3 skip:0 total:5
-# Totals: pass:2 fail:3 skip:0 total:5
-not ok 1 checksum
+# checksum: pass:5 fail:0 skip:0 total:5
+# Totals: pass:5 fail:0 skip:0 total:5
+ok 1 checksum

As we aim for correctness over performance:

Tested-by Geert Uytterhoeven <[email protected]>

However, given the big impact on performance, it would be great if someone could find out what's wrong with the optimized version.

Gr{oetje,eeting}s,

Geert

Signature · 5 lines

Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

John Paul Adrian Glaubitz

maintainer Reviewed-by

2024-05-02 07:23 UTC #9 lore raw

Hi Geert,

Quoted 42 lines · Geert Uytterhoeven

On landisk:

KTAP version 1
1..1
KTAP version 1
# Subtest: checksum
# module: checksum_kunit
1..5
- # test_csum_fixed_random_inputs: ASSERTION FAILED at
lib/checksum_kunit.c:500

(quoted diff) +1 ~~-14~~

-    Expected ( u64)result == ( u64)expec, but
-        ( u64)result == 53378 (0xd082)
-        ( u64)expec == 33488 (0x82d0)
-    not ok 1 test_csum_fixed_random_inputs
-    # test_csum_all_carry_inputs: ASSERTION FAILED at lib/checksum_kunit.c:525
-    Expected ( u64)result == ( u64)expec, but
-        ( u64)result == 65281 (0xff01)
-        ( u64)expec == 65280 (0xff00)
-    not ok 2 test_csum_all_carry_inputs
-    # test_csum_no_carry_inputs: ASSERTION FAILED at lib/checksum_kunit.c:573
-    Expected ( u64)result == ( u64)expec, but
-        ( u64)result == 65535 (0xffff)
-        ( u64)expec == 65534 (0xfffe)
-    not ok 3 test_csum_no_carry_inputs
+    # test_csum_fixed_random_inputs: Test should be marked slow

(runtime: 9.814991070s)

(quoted diff) +2 -0

+    ok 1 test_csum_fixed_random_inputs
+    # test_csum_all_carry_inputs: Test should be marked slow

(runtime: 19.621274580s)

(quoted diff) +2 -0

+    ok 2 test_csum_all_carry_inputs
+    # test_csum_no_carry_inputs: Test should be marked slow (runtime:

19.614096540s)

(quoted diff) +4 -3

+    ok 3 test_csum_no_carry_inputs
     ok 4 test_ip_fast_csum
     ok 5 test_csum_ipv6_magic
-# checksum: pass:2 fail:3 skip:0 total:5
-# Totals: pass:2 fail:3 skip:0 total:5
-not ok 1 checksum
+# checksum: pass:5 fail:0 skip:0 total:5
+# Totals: pass:5 fail:0 skip:0 total:5
+ok 1 checksum

Quoted 5 lines

As we aim for correctness over performance:
Tested-by Geert Uytterhoeven <[email protected]>
However, given the big impact on performance, it would be great if someone could find out what's wrong with the optimized version.

Thanks for testing this. I will pick this up then since it actually fixes a bug.

Reviewed-by John Paul Adrian Glaubitz <[email protected]>

Adrian

Signature · 4 lines

 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Guenter Roeck

author

2024-03-25 15:35 UTC #10 lore raw

In reply to John Paul Adrian Glaubitz

Quoted 38 lines · John Paul Adrian Glaubitz

Hi Guenter,
Quoted 26 lines · Guenter Roeck
This reverts commit cadc4e1a2b4d20d0cc0e81f2c6ba0588775e54e5.
Commit cadc4e1a2b4d ("sh: Handle calling csum_partial with misaligned data") causes bad checksum calculations on unaligned data. Reverting it fixes the problem.
# Subtest: checksum
# module: checksum_kunit
1..5
# test_csum_fixed_random_inputs: ASSERTION FAILED at lib/checksum_kunit.c:500 Expected ( u64)result == ( u64)expec, but
( u64)result == 53378 (0xd082)
( u64)expec == 33488 (0x82d0)
# test_csum_fixed_random_inputs: pass:0 fail:1 skip:0 total:1 not ok 1 test_csum_fixed_random_inputs
# test_csum_all_carry_inputs: ASSERTION FAILED at lib/checksum_kunit.c:525 Expected ( u64)result == ( u64)expec, but
( u64)result == 65281 (0xff01)
( u64)expec == 65280 (0xff00)
# test_csum_all_carry_inputs: pass:0 fail:1 skip:0 total:1
not ok 2 test_csum_all_carry_inputs
# test_csum_no_carry_inputs: ASSERTION FAILED at lib/checksum_kunit.c:573 Expected ( u64)result == ( u64)expec, but
( u64)result == 65535 (0xffff)
( u64)expec == 65534 (0xfffe)
# test_csum_no_carry_inputs: pass:0 fail:1 skip:0 total:1
Quoted 8 lines
not ok 3 test_csum_no_carry_inputs
# test_ip_fast_csum: pass:1 fail:0 skip:0 total:1 ok 4 test_ip_fast_csum
# test_csum_ipv6_magic: pass:1 fail:0 skip:0 total:1 ok 5 test_csum_ipv6_magic
# checksum: pass:2 fail:3 skip:0 total:5
# Totals: pass:2 fail:3 skip:0 total:5
not ok 22 checksum

Quoted 2 lines

Can you tell me how the tests are run so I can try to verify this on real hardware?

Please also see
https://lore.kernel.org/lkml/[email protected]/t/
where I reported the problem a while ago. I didn't see a fix, so I figured I'd submit a revert, following the logic that non-optimized code is better than buggy code. Obviously a real fix would be preferrable, but I don't understand sh4 assembler well enough to understand what is happening.

Thanks,
Guenter

John Paul Adrian Glaubitz

maintainer Applied

2024-05-02 10:29 UTC #11 lore raw

In reply to Guenter Roeck

Quoted 159 lines · Guenter Roeck

This reverts commit cadc4e1a2b4d20d0cc0e81f2c6ba0588775e54e5.
Commit cadc4e1a2b4d ("sh: Handle calling csum_partial with misaligned data") causes bad checksum calculations on unaligned data. Reverting it fixes the problem.
# Subtest: checksum
# module: checksum_kunit
1..5
# test_csum_fixed_random_inputs: ASSERTION FAILED at lib/checksum_kunit.c:500 Expected ( u64)result == ( u64)expec, but
( u64)result == 53378 (0xd082)
( u64)expec == 33488 (0x82d0)
# test_csum_fixed_random_inputs: pass:0 fail:1 skip:0 total:1 not ok 1 test_csum_fixed_random_inputs
# test_csum_all_carry_inputs: ASSERTION FAILED at lib/checksum_kunit.c:525 Expected ( u64)result == ( u64)expec, but
( u64)result == 65281 (0xff01)
( u64)expec == 65280 (0xff00)
# test_csum_all_carry_inputs: pass:0 fail:1 skip:0 total:1
not ok 2 test_csum_all_carry_inputs
# test_csum_no_carry_inputs: ASSERTION FAILED at lib/checksum_kunit.c:573 Expected ( u64)result == ( u64)expec, but
( u64)result == 65535 (0xffff)
( u64)expec == 65534 (0xfffe)
# test_csum_no_carry_inputs: pass:0 fail:1 skip:0 total:1
not ok 3 test_csum_no_carry_inputs
# test_ip_fast_csum: pass:1 fail:0 skip:0 total:1
ok 4 test_ip_fast_csum
# test_csum_ipv6_magic: pass:1 fail:0 skip:0 total:1
ok 5 test_csum_ipv6_magic
# checksum: pass:2 fail:3 skip:0 total:5
# Totals: pass:2 fail:3 skip:0 total:5
not ok 22 checksum
Fixes cadc4e1a2b4d ("sh: Handle calling csum_partial with misaligned data")
Signed-off-by Guenter Roeck <[email protected]>
---
 arch/sh/lib/checksum.S | 67 ++++++++++++------------------------------
 1 file changed, 18 insertions(+), 49 deletions(-)
arch/sh/lib/checksum.S +18 ~~-41~~
index 3e07074e0098..06fed5a21e8b 100644
@@ -33,7 +33,8 @@
  */
 
 /*	
- * asmlinkage __wsum csum_partial(const void *buf, int len, __wsum sum);
+ * unsigned int csum_partial(const unsigned char *buf, int len,
+ *                           unsigned int sum);
  */
 
 .text
@@ -45,31 +46,11 @@ ENTRY(csum_partial)
 	   * Fortunately, it is easy to convert 2-byte alignment to 4-byte
 	   * alignment for the unrolled loop.
 	   */
+	mov	r5, r1
 	mov	r4, r0
-	tst	#3, r0		! Check alignment.
-	bt/s	2f		! Jump if alignment is ok.
-	 mov	r4, r7		! Keep a copy to check for alignment
+	tst	#2, r0		! Check alignment.
+	bt	2f		! Jump if alignment is ok.
 	!
-	tst	#1, r0		! Check alignment.
-	bt	21f		! Jump if alignment is boundary of 2bytes.
-
-	! buf is odd
-	tst	r5, r5
-	add	#-1, r5
-	bt	9f
-	mov.b	@r4+, r0
-	extu.b	r0, r0
-	addc	r0, r6		! t=0 from previous tst
-	mov	r6, r0
-	shll8	r6
-	shlr16	r0
-	shlr8	r0
-	or	r0, r6
-	mov	r4, r0
-	tst	#2, r0
-	bt	2f
-21:
-	! buf is 2 byte aligned (len could be 0)
 	add	#-2, r5		! Alignment uses up two bytes.
 	cmp/pz	r5		!
 	bt/s	1f		! Jump if we had at least two bytes.
@@ -77,17 +58,16 @@ ENTRY(csum_partial)
 	bra	6f
 	 add	#2, r5		! r5 was < 2.  Deal with it.
 1:
+	mov	r5, r1		! Save new len for later use.
 	mov.w	@r4+, r0
 	extu.w	r0, r0
 	addc	r0, r6
 	bf	2f
 	add	#1, r6
 2:
-	! buf is 4 byte aligned (len could be 0)
-	mov	r5, r1
 	mov	#-5, r0
-	shld	r0, r1
-	tst	r1, r1
+	shld	r0, r5
+	tst	r5, r5
 	bt/s	4f		! if it's =0, go to 4f
 	 clrt
 	.align	2
@@ -109,31 +89,30 @@ ENTRY(csum_partial)
 	addc	r0, r6
 	addc	r2, r6
 	movt	r0
-	dt	r1
+	dt	r5
 	bf/s	3b
 	 cmp/eq	#1, r0
-	! here, we know r1==0
-	addc	r1, r6			! add carry to r6
+	! here, we know r5==0
+	addc	r5, r6			! add carry to r6
 4:
-	mov	r5, r0
+	mov	r1, r0
 	and	#0x1c, r0
 	tst	r0, r0
-	bt	6f
-	! 4 bytes or more remaining
-	mov	r0, r1
-	shlr2	r1
+	bt/s	6f
+	 mov	r0, r5
+	shlr2	r5
 	mov	#0, r2
 5:
 	addc	r2, r6
 	mov.l	@r4+, r2
 	movt	r0
-	dt	r1
+	dt	r5
 	bf/s	5b
 	 cmp/eq	#1, r0
 	addc	r2, r6
-	addc	r1, r6		! r1==0 here, so it means add carry-bit
+	addc	r5, r6		! r5==0 here, so it means add carry-bit
 6:
-	! 3 bytes or less remaining
+	mov	r1, r5
 	mov	#3, r0
 	and	r0, r5
 	tst	r5, r5
@@ -159,16 +138,6 @@ ENTRY(csum_partial)
 	mov	#0, r0
 	addc	r0, r6
 9:
-	! Check if the buffer was misaligned, if so realign sum
-	mov	r7, r0

arch/sh/lib/checksum.S ENTRY(csum_partial) · line 142

Quoted 10 lines

arch/sh/lib/checksum.S +0 -8

-	tst	#1, r0
-	bt	10f
-	mov	r6, r0
-	shll8	r6
-	shlr16	r0
-	shlr8	r0
-	or	r0, r6
-10:
 	rts
 	 mov	r6, r0

Applied to my sh-linux tree in the for-next branch.

Thanks,
Adrian

Signature · 4 lines

 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913