| Message ID | 20240324231804.841099-1-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Revert "sh: Handle calling csum_partial with misaligned data" | expand |
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
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
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
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
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
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
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
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
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
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
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
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
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(-)