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

[Mike Frysinger]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/