[PATCH] Revert "sh: Handle calling csum_partial with misaligned data"

[PATCH] Revert "sh: Handle calling csum_partial with misaligned data"

[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 <linux@roeck-us.net>
---
 arch/sh/lib/checksum.S | 67 ++++++++++++------------------------------
 1 file changed, 18 insertions(+), 49 deletions(-)

diff --git a/arch/sh/lib/checksum.S b/arch/sh/lib/checksum.S
index 3e07074e0098..06fed5a21e8b 100644
--- a/arch/sh/lib/checksum.S
+++ b/arch/sh/lib/checksum.S
@@ -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
 
-- 
2.39.2

Re: [PATCH] Revert "sh: Handle calling csum_partial with misaligned data"

[John Paul Adrian Glaubitz]

Hi Guenter,

On Sun, 2024-03-24 at 16:18 -0700, Guenter Roeck wrote:
> 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

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

Adrian

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

Re: [PATCH] Revert "sh: Handle calling csum_partial with misaligned data"

[Guenter Roeck]

On 3/25/24 00:39, John Paul Adrian Glaubitz wrote:
> Hi Guenter,
> 
> On Sun, 2024-03-24 at 16:18 -0700, Guenter Roeck wrote:
>> 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
> 
> 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

Re: [PATCH] Revert "sh: Handle calling csum_partial with misaligned data"

[Guenter Roeck]

On Mon, Mar 25, 2024 at 08:39:39AM +0100, John Paul Adrian Glaubitz wrote:
> Hi Guenter,
> 
> On Sun, 2024-03-24 at 16:18 -0700, Guenter Roeck wrote:
> > 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
> 
> 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/0a0fbbd8-17dd-4f4c-9513-f3ac9749890b@roeck-us.net/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

Re: [PATCH] Revert "sh: Handle calling csum_partial with misaligned data"

[Guenter Roeck]

Hi,

On Mon, Mar 25, 2024 at 07:34:00AM -0700, Guenter Roeck wrote:
> On 3/25/24 00:39, John Paul Adrian Glaubitz wrote:
> > Hi Guenter,
> > 
> > On Sun, 2024-03-24 at 16:18 -0700, Guenter Roeck wrote:
> > > 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
> > 
> > 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

Re: [PATCH] Revert "sh: Handle calling csum_partial with misaligned data"

[John Paul Adrian Glaubitz]

On Tue, 2024-04-02 at 07:06 -0700, Guenter Roeck wrote:
> Hi,
> 
> On Mon, Mar 25, 2024 at 07:34:00AM -0700, Guenter Roeck wrote:
> > On 3/25/24 00:39, John Paul Adrian Glaubitz wrote:
> > > Hi Guenter,
> > > 
> > > On Sun, 2024-03-24 at 16:18 -0700, Guenter Roeck wrote:
> > > > 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
> > > 
> > > 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

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

Re: [PATCH] Revert "sh: Handle calling csum_partial with misaligned data"

[John Paul Adrian Glaubitz]

Hi Guenter,

On Tue, 2024-04-02 at 16:09 +0200, John Paul Adrian Glaubitz wrote:
> On Tue, 2024-04-02 at 07:06 -0700, Guenter Roeck wrote:
> > Hi,
> > 
> > On Mon, Mar 25, 2024 at 07:34:00AM -0700, Guenter Roeck wrote:
> > > On 3/25/24 00:39, John Paul Adrian Glaubitz wrote:
> > > > Hi Guenter,
> > > > 
> > > > On Sun, 2024-03-24 at 16:18 -0700, Guenter Roeck wrote:
> > > > > 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
> > > > 
> > > > 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.

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

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

Re: [PATCH] Revert "sh: Handle calling csum_partial with misaligned data"

[Guenter Roeck]

On Wed, May 01, 2024 at 10:28:11AM +0200, John Paul Adrian Glaubitz wrote:
> > > 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

Re: [PATCH] Revert "sh: Handle calling csum_partial with misaligned data"

[John Paul Adrian Glaubitz]

On Wed, 2024-05-01 at 07:44 -0700, Guenter Roeck wrote:
> On Wed, May 01, 2024 at 10:28:11AM +0200, John Paul Adrian Glaubitz wrote:
> > > > 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.

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

Adrian

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

Re: [PATCH] Revert "sh: Handle calling csum_partial with misaligned data"

[Geert Uytterhoeven]

On Wed, May 1, 2024 at 10:28 AM John Paul Adrian Glaubitz
<glaubitz@physik.fu-berlin.de> wrote:
> On Tue, 2024-04-02 at 16:09 +0200, John Paul Adrian Glaubitz wrote:
> > On Tue, 2024-04-02 at 07:06 -0700, Guenter Roeck wrote:
> > > On Mon, Mar 25, 2024 at 07:34:00AM -0700, Guenter Roeck wrote:
> > > > On 3/25/24 00:39, John Paul Adrian Glaubitz wrote:
> > > > > Hi Guenter,
> > > > >
> > > > > On Sun, 2024-03-24 at 16:18 -0700, Guenter Roeck wrote:
> > > > > > 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
> > > > >
> > > > > 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.
>
> 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 <geert+renesas@glider.be>

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH] Revert "sh: Handle calling csum_partial with misaligned data"

[John Paul Adrian Glaubitz]

Hi Geert,

On Thu, 2024-05-02 at 09:21 +0200, Geert Uytterhoeven wrote:
> 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 <geert+renesas@glider.be>
> 
> 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 <glaubitz@physik.fu-berlin.de>

Adrian

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

Re: [PATCH] Revert "sh: Handle calling csum_partial with misaligned data"

[John Paul Adrian Glaubitz]

On Sun, 2024-03-24 at 16:18 -0700, Guenter Roeck wrote:
> 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 <linux@roeck-us.net>
> ---
>  arch/sh/lib/checksum.S | 67 ++++++++++++------------------------------
>  1 file changed, 18 insertions(+), 49 deletions(-)
> 
> diff --git a/arch/sh/lib/checksum.S b/arch/sh/lib/checksum.S
> index 3e07074e0098..06fed5a21e8b 100644
> --- a/arch/sh/lib/checksum.S
> +++ b/arch/sh/lib/checksum.S
> @@ -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

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

Thanks,
Adrian

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