lkml.org 
[lkml]   [2009]   [Jun]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRe: [PATCH 05/17] Blackfin: convert to generic checksum code
    Date
    On Friday 19 June 2009, Mike Frysinger wrote:
    > On Mon, Jun 15, 2009 at 07:04, Arnd Bergmann wrote:
    > > On Sunday 14 June 2009, Mike Frysinger wrote:
    > >> The Blackfin port only implemented an optimized version of the
    > >> csum_tcpudp_nofold function, so convert everything else to the new
    > >> generic code.
    > >>
    > >> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
    > >
    > > Have you tested this one well? I was as careful as possible with the
    > > version I added, but it was basically only tested on microblaze, which
    > > has a different endianess from blackfin. Some areas of the code may be
    > > sensitive to this.
    >
    > i did some tests and it looks like do_csum() is broken :(

    Thanks for testing. Indeed it turns out to be an endian-problem with
    the handling of the last byte:

    > here's the input:
    > do_csum({ 0xff 0xfb 0x01 }, 3)
    >
    > and here's the output:
    > Blackfin: 0xfc00
    > generic: 0xfcff
    >
    > if i run the two funcs on my x86, i see similar behavior. the
    > attached csum-test.c contains the csum code from lib/checksum.c and
    > arch/blackfin/lib/checksum.c and shows the problem.

    x86 and blackfin are both little-endian, so your variant is correct
    there. They add the 0x01 to the low byte of the 16-bit word, while
    on big-endian machines, you have to add it to the high byte.

    > -mike
    > csum-test.c
    > #include <stdio.h>
    >
    > static inline unsigned short gen_from32to16(unsigned long x)
    > {
    > /* add up 16-bit and 16-bit for 16+c bit */
    > x = (x & 0xffff) + (x >> 16);
    > /* add up carry.. */
    > x = (x & 0xffff) + (x >> 16);
    > return x;
    > }
    >
    > static unsigned int gen_do_csum(const unsigned char *buff, int len)
    > {
    > ...
    > if (len & 1)
    > result += (*buff << 8);

    On little-endian, this needs to be

    if (len & 1)
    result += *buff;

    > static unsigned short bf_do_csum(const unsigned char *buff, int len)
    > {
    > ...
    > if (len > 0)
    > sum += *buff;

    Same problem, on big-endian this would need to be
    if (len > 0)
    sum += (*buff << 8);

    The bug potentially also exists in arch/sh/lib64/c-checksum.c, which only
    works on little-endian machines, while the sh architecture code appears
    dual-endian. Paul, is your lib64/c-checksum.c implementation potentially
    run on big-endian machines, or is sh64 guaranteed to be little-endian?

    I've committed the patch below now.

    Arnd <><

    ---
    From e01fed86629737809c46dcb8b807347e84640b70 Mon Sep 17 00:00:00 2001
    From: Arnd Bergmann <arnd@arndb.de>
    Date: Fri, 19 Jun 2009 10:41:19 +0200
    Subject: [PATCH] lib/checksum.c: fix endianess bug

    The new generic checksum code has a small dependency on endianess and
    worked only on big-endian systems. I could not find a nice efficient
    way to express this, so I added an #ifdef. Using
    'result += le16_to_cpu(*buff);' would have worked as well, but
    would be slightly less efficient on big-endian systems and IMHO
    would not be clearer.

    Also fix a bug that prevents this from working on 64-bit machines.
    If you have a 64-bit CPU and want to use the generic checksum
    code, you should probably do some more optimizations anyway, but
    at least the code should not break.

    Reported-by: Mike Frysinger <vapier@gentoo.org>
    Signed-off-by: Arnd Bergmann <arnd@arndb.de>
    ---
    lib/checksum.c | 6 +++++-
    1 files changed, 5 insertions(+), 1 deletions(-)

    diff --git a/lib/checksum.c b/lib/checksum.c
    index 12e5a1c..4a1c84a 100644
    --- a/lib/checksum.c
    +++ b/lib/checksum.c
    @@ -71,7 +71,7 @@ static unsigned int do_csum(const unsigned char *buff, int len)
    if (count) {
    unsigned long carry = 0;
    do {
    - unsigned long w = *(unsigned long *) buff;
    + unsigned long w = *(unsigned int *) buff;
    count--;
    buff += 4;
    result += carry;
    @@ -87,7 +87,11 @@ static unsigned int do_csum(const unsigned char *buff, int len)
    }
    }
    if (len & 1)
    +#ifdef __LITTLE_ENDIAN
    + result += *buff;
    +#else
    result += (*buff << 8);
    +#endif
    result = from32to16(result);
    if (odd)
    result = ((result >> 8) & 0xff) | ((result & 0xff) << 8);
    --
    1.6.3.1


    \
     
     \ /
      Last update: 2009-06-19 11:09    [W:0.036 / U:274.680 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site