Re: [PATCH] add checksum selftest

[Mike Frysinger <vapier.adi@gmail.com>]

On Wed, Jun 24, 2009 at 10:14, Arnd Bergmann wrote:
> On Wednesday 24 June 2009, Mike Frysinger wrote:
>> +/*
>> + * The do_csum() interface is "internal" to the generic checksum code.
>> + * Do not require it if the arch has not switched over.
>> + */
>> +extern unsigned short do_csum(const unsigned char *buff, int len);
>
> Just nitpicking: this prototype should to into asm-generic/checksum.h,
> extern declarations have no place in .c files.

yeah, i meant to move that after the last discussion, but forgot

>> +static unsigned char __initdata do_csum_data1[] = {
>> +     0x20,
>> +};
>> +static unsigned char __initdata do_csum_data2[] = {
>> +     0x0d, 0x0a,
>> +};
>> +static unsigned char __initdata do_csum_data3[] = {
>> +     0xff, 0xfb, 0x01,
>> +};
>
> You define separate test vectors for each of the three
> cases, which looks like it could be optimized by reusing
> the same test vectors for each case.

i'm not really familiar with the interfaces to figure out how to do
this ... i just added some printks to dump arguments/buffers and then
copied & pasted ones that looked pretty different

>> +static struct csum_partial_data __initdata csum_partial_data[] = {
>> +     CSUM_PARTIAL_DATA(1,  0x00000074, 0x0),
>> +     CSUM_PARTIAL_DATA(2,  0x00000a0d, 0x0),
>> +     CSUM_PARTIAL_DATA(3,  0x0000fe00, 0x0),
>> +     CSUM_PARTIAL_DATA(5,  0x00005084, 0x0),
>> +     CSUM_PARTIAL_DATA(8,  0x1101eefe, 0x11016a80),
>> +     CSUM_PARTIAL_DATA(8b, 0x00008781, 0x847e),
>> +     CSUM_PARTIAL_DATA(9,  0x1101eefe, 0x11016b80),
>> +};
>
> For partial checksums, the result has to be folded into a 16-bit
> number using csum_fold(), because csum_partial and other functions
> return a 32-bit __wsum that can take many equivalent values taht
> are all correct.

i hear your words, but i understand them not ;)

>> +static int __init csum_tcpudp_nofold_selftest(void)
>> +{
>> +     int i, ret;
>> +     unsigned short tret, eret;
>> +
>> +     ret = 0;
>> +     for (i = 0; i < ARRAY_SIZE(csum_tcpudp_nofold_data); ++i) {
>> +             eret = le16_to_cpu(csum_tcpudp_nofold_data[i].ret);
>> +             tret = csum_tcpudp_nofold(
>> +                     csum_tcpudp_nofold_data[i].saddr,
>> +                     csum_tcpudp_nofold_data[i].daddr,
>> +                     csum_tcpudp_nofold_data[i].len,
>> +                     csum_tcpudp_nofold_data[i].proto,
>> +                     csum_tcpudp_nofold_data[i].sum);
>> +             if (tret != eret) {
>> +                     pr_err("%s: test %i: %#x != %#x: FAIL\n",
>> +                             __func__, i, tret, eret);
>> +                     ret = 1;
>> +             }
>> +     }
>> +
>> +     return ret;
>> +}
>
> same here, but you can easily use csum_tcpudp_magic() instead of
> csum_tcpudp_nofold here.

this introduces redirection that would be annoying for people who
implement just csum_tcpudp_nofold() (like the Blackfin arch).  i dont
have a problem extending the test to also cover csum_tcpudp_magic(),
but i'd like to keep the csum_tcpudp_nofold() invocation as well.
-mike