Re: [Bug fix]mips 64bits checksum error -- csum_tcpudp_nofold

[Mark Zhang]

Hi Alex,

    Thanks for your reply.
    I tested your correction. The result is correct.
    The C language will cause this function(csum_tcpudp_nofold) become
176 MIPS instructions. The assemble code is 150 MIPS instruction.
    If the MIPS CPU is running as 1GHz, C language cause more 60 nano
seconds during send/receive each tcp/udp packet. I'm not sure whether
it will cause any negative result if the frequency of CPU was lower.
MIPS arch is usually used in networking equipments.
    I think Ralf's correction is better.

    - Mark

Signed-off-by: Ralf Baechle <ralf@xxxxxxxxxxxxxx>

 arch/mips/include/asm/checksum.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/mips/include/asm/checksum.h b/arch/mips/include/asm/checksum.h
index 5c585c5..08b6ba3 100644
--- a/arch/mips/include/asm/checksum.h
+++ b/arch/mips/include/asm/checksum.h
@@ -186,7 +186,9 @@ static inline __wsum csum_tcpudp_nofold(__be32 saddr,
        "       daddu   %0, %4          \n"
        "       dsll32  $1, %0, 0       \n"
        "       daddu   %0, $1          \n"
+       "       sltu    $1, %0, $1      \n"
        "       dsra32  %0, %0, 0       \n"
+       "       daddu   %0, $1          \n"
 #endif
        "       .set    pop"
        : "=r" (sum)


2017-01-26 2:13 GMT+08:00 Alexander Duyck <alexander.duyck@xxxxxxxxx>:
> On Tue, Jan 24, 2017 at 8:35 PM, Mark Zhang <bomb.zhang@xxxxxxxxx> wrote:
>> If the input parameters as saddr = 0xc0a8fd60,daddr = 0xc0a8fda1,len =
>> 80, proto = 17, sum =0x7eae049d.
>> The correct result should be 1, but original function return 0.
>>
>> Attached the correction patch.
>
> I've copied your patch here:
>
> From 52e265f7fe0acf9a6e9c4346e1fe6fa994aa00b6 Mon Sep 17 00:00:00 2001
> From: qzhang <qin.2.zhang@xxxxxxx>
> Date: Wed, 25 Jan 2017 12:25:25 +0800
> Subject: [PATCH] Fixed the mips 64bits checksum error -- csum_tcpudp_nofold
>
> ---
>  arch/mips/include/asm/checksum.h |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/arch/mips/include/asm/checksum.h b/arch/mips/include/asm/checksum.h
> index 7749daf..0e351c5 100644
> --- a/arch/mips/include/asm/checksum.h
> +++ b/arch/mips/include/asm/checksum.h
> @@ -184,6 +184,10 @@ static inline __wsum csum_tcpudp_nofold(__be32
> saddr, __be32 daddr,
>   " daddu %0, %2 \n"
>   " daddu %0, %3 \n"
>   " daddu %0, %4 \n"
> + " dsrl32  $1, %0, 0 \n"
> + " dsll32 %0, %0, 0 \n"
> + " dsrl32 %0, %0, 0 \n"
> + " daddu   %0, $1 \n"
>   " dsll32 $1, %0, 0 \n"
>   " daddu %0, $1 \n"
>   " dsra32 %0, %0, 0 \n"
>
> I agree there does appear to be a bug in the code, and my
> understanding of MIPS assembly is limited, but I don't think your
> patch truly fixes it.  From what I can understand it seems like you
> would just be shifting the register called out at %0 past 64 bits
> which would just zero it out.
>
> Below is the snippet you are updating:
>
>     #ifdef CONFIG_64BIT
>             "       daddu   %0, %2          \n"
>             "       daddu   %0, %3          \n"
>             "       daddu   %0, %4          \n"
>             "       dsll32  $1, %0, 0       \n"
>             "       daddu   %0, $1          \n"
>             "       dsra32  %0, %0, 0       \n"
>     #endif
>
> From what I can tell the issue is that the dsll32 really needs to be
> replaced with some sort of rotation type call instead of a shift, but
> it looks like MIPS doesn't have a rotation instruction.  We need to
> add the combination of a shift left by 32, to a shift right 32, and
> then add that value back to the original register.  Then we will get
> the correct result in the upper 32 bits.
>
> I'm not even sure it is necessary to use inline assembler.  You could
> probably just use the inline assembler for the 32b and change the 64b
> approach over to using C.  The code for it would be pretty simple:
>     unsigned long res = (__force unsigned long)sum;
>
>     res += (__force unsigned long)daddr;
>     res += (__force unsigned long)saddr;
> #ifdef __MIPSEL__
>     res += (proto + len) << 8;
> #else
>     res += proto + len;
> #endif
>     res += (res << 32) | (res >> 32);
>
>     return (__force __wsum)(res >> 32);
>
> That would probably be enough to fix the issue and odds are it should
> generate assembly code very similar to what was already there.
>
> - Alex