Re: [PATCH 05/17] Blackfin: convert to generic checksum code

[Arnd Bergmann <arnd@arndb.de>]

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++;
 	}