Messages in this thread | | | From | Mike Frysinger <> | Date | Tue, 23 Jun 2009 17:53:44 -0400 | Subject | Re: [PATCH 05/17] Blackfin: convert to generic checksum code |
| |
On Tue, Jun 23, 2009 at 17:14, Arnd Bergmann wrote: > --- a/lib/checksum.c > +++ b/lib/checksum.c > @@ -57,9 +57,9 @@ static unsigned int do_csum(const unsigned char *buff, int len) > odd = 1 & (unsigned long) buff; > if (odd) { > #ifdef __LITTLE_ENDIAN > - result = *buff; > -#else > result += (*buff << 8); > +#else > + result = *buff; > #endif > len--; > buff++; > ---
thanks, my tests seem to work now ... i was doing telnet/rsh stuff as well as icmp/udp/tcp flooding. i'll convert Blackfin to generic checksum post 2.6.31 so we can test it some more in the meantime.
>>extern unsigned short do_csum(const unsigned char *buff, int len); > > do_csum is really an internal function. IMHO we should better check > csum_partial(), ip_fast_csum(), csum_fold(), csum_tcpudp_magic() > and ip_compute_csum(), or at least a subset of them.
and this is an internal tester ... if we're going to let arches override it, then its function signature are pretty much set in stone. i agree we should test every checksum function, but i think only testing do_csum indirectly would be detrimental to the people who would want to use this -- arch maintainers looking to implement do_csum() themselves. otherwise they need to step through the surrounding functions a bit to find the exact values given to do_csum() and the exact value expected back such that the calling function still works. and every arch guy is going to do this same thing.
>>static unsigned char __initdata do_csum_data2[] = { >> 0x0d, 0x0a, >>}; >>static unsigned char __initdata do_csum_data3[] = { >> 0xff, 0xfb, 0x01, >>}; >> ... >>static struct do_csum_data __initdata do_csum_data[] = { >> DO_CSUM_DATA(1, 0x0020), >> DO_CSUM_DATA(2, 0xfc00), >> DO_CSUM_DATA(3, 0x0a0d), >> DO_CSUM_DATA(5, 0x7fc4), >> DO_CSUM_DATA(7, 0x7597), >> DO_CSUM_DATA(255, 0x4f96), >>}; > > You mixed up do_csum_data2 and do_csum_data3, so they will always > show up as incorrect.
i was going through different ways of formatting the data and looks like i messed that up, thanks
> Also, the expected checksum is endian-dependent. > The test module should either be modified to expect 0xffff to be > returned in every case, or should use le16_to_cpu(0x0020) etc
i'm a bit of a nub here wrt checksuming. the test module i came up with by simply flooding my board and copying & pasting mismatched buffers ;). are you saying the following should be OK ? - if (tret != do_csum_data[i].ret) { + if (tret != le16_to_cpu(do_csum_data[i].ret)) {
unless there is a macro i could use that'd do the expansion at CPP time so i'd write the test data as: static struct do_csum_data __initdata do_csum_data[] = { DO_CSUM_DATA(1, le16_to_cpu(0x0020)), ... -mike -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |