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 Fri, Jun 19, 2009 at 05:05, Arnd Bergmann wrote:
    > > 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.
    >
    > can we think of enough simple examples to through together an optional
    > boot-time self test ?

    sounds like a good idea.

    > > I've committed the patch below now.
    >
    > closer, but not quite just yet ...
    > do_csum: mismatch 0x1700 != 0xa0d len:2
    > do_csum: { 0xd, 0xa }

    I've compared the code again with Paul's sh version and found another
    difference in the handling of unaligned data.

    > when do_csum does return correct values, csum_partial sometimes does not:
    > csum_partial: mismatch 0x1101eefe != 0xffff len:32
    > csum_partial: { 0x92, 0x3b, 0x0, 0x17, 0xcf, 0xc1, 0x90, 0xec, 0x1c,
    > 0x3f, 0xff, 0x99, 0x80, 0x10, 0x0, 0x5c, 0x22, 0xfa, 0x0, 0x0, 0x1,
    > 0x1, 0x8, 0xa, 0x6, 0x83, 0xdd, 0x50, 0xff, 0xfe, 0xdf, 0x58 }

    That looks like it's both valid output. csum_partial returns a 32 bit
    value that always needs to get passed to csum_fold in order to get
    checked. 0x1101eefe and 0xffff both evaluate to 0xffff in csum_fold,
    so this would just be an implementation detail, not a bug.

    > btw, would it make sense to change do_csum like so:
    > -static unsigned int do_csum(const unsigned char *buff, int len)
    > +__weak unsigned int do_csum(const unsigned char *buff, int len)
    >
    > after all, it looks like that's really the function most arches would
    > want to hand optimize. all the rest are simple standard wrappers are
    > do_csum(). forcing an arch to copy & paste all the other functions
    > just so they can implement an optimized do_csum() seems pretty
    > unfriendly. plus, it isnt like being static here makes any optimized
    > difference -- it isnt going to get inlined anywhere in this file.

    Agreed, this should be overridable by the architecture, I'll submit a patch
    once we found out what caused the bugs.

    > or rather than weaks (in case some toolchains force indirect pointer
    > loading all the time), use the same interface as the rest of the
    > generic checksum code:
    > #ifndef do_csum
    > static inline unsigned short from32to16(unsigned long x)
    > {
    > ...
    > }
    > static unsigned int do_csum(const unsigned char *buff, int len)
    > {
    > ...
    > }
    > #endif

    I'd prefer the second version, I've done it that way all over
    include/asm-generic, and weak functions tend to cause more trouble.

    ---
    lib/checksum.c: Fix another endianess bug

    will fold this patch into the previous one.

    Signed-off-by: Arnd Bergmann <arnd@arndb.de>
    --- a/lib/checksum.c
    +++ b/lib/checksum.c
    @@ -55,7 +55,11 @@ static unsigned int do_csum(const unsigned char *buff, int len)
    goto out;
    odd = 1 & (unsigned long) buff;
    if (odd) {
    +#ifdef __LITTLE_ENDIAN
    result = *buff;
    +#else
    + result += (*buff << 8);
    +#endif
    len--;
    buff++;
    }

    \
     
     \ /
      Last update: 2009-06-19 14:37    [W:4.196 / U:0.428 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site