On 2018/7/17 11:43 PM, Andy Shevchenko wrote: > On Tue, 2018-07-17 at 22:55 +0800, Coly Li wrote: >> This patch adds the re-write crc64 calculation routines for Linux >> kernel. >> The CRC64 polynomical arithmetic follows ECMA-182 specification, >> inspired >> by CRC paper of Dr. Ross N. Williams >> (see http://www.ross.net/crc/download/crc_v3.txt) and other public >> domain >> implementations. >> >> All the changes work in this way, >> - When Linux kernel is built, host program lib/gen_crc64table.c will >> be >> compiled to lib/gen_crc64table and executed. >> - The output of gen_crc64table execution is an array called as lookup >> table (a.k.a POLY 0x42f0e1eba9ea369) which contain 256 64bits-long >> numbers, this talbe is dumped into header file lib/crc64table.h. >> - Then the header file is included by lib/crc64.c for normal 64bit crc >> calculation. >> - Function declaration of the crc64 calculation routines is placed in >> include/linux/crc64.h > > Thanks for an update! My comments below. > >> Co-developed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> > > As required by coding style this tag should be accompanied with SoB of > co-developer(s). > Now I change the lines as following in commit log, Co-developed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> >> +u64 __pure crc64_update(u64 crc, const void *_p, size_t len); > > For sake of consistency I would name _len as well. > Hmm, I will use 'p', and use '_p' for internal argument. >> + * Normal 64bit CRC calculation. > > I think 64-bit form is slightly better and more often > > $ git grep -n -w 64bit | wc -l > 809 > > $ git grep -n -w 64-bit | wc -l > 2957 > >> + * crc64table[256] is the lookup table of a table-driver 64bit CRC > > Ditto. > Sure, I will change them to 64-bit. >> + * Copyright 2018 SUSE Linux. >> + * Author: Coly Li <colyli@xxxxxxx> > >> + * > > This (blank comment) line is not needed. > >> +u64 __pure crc64_update(u64 crc, const void *_p, size_t len) > > _len ? > I will rename '_p' to 'p' here too. >> + * Copyright 2018 SUSE Linux. >> + * Author: Coly Li <colyli@xxxxxxx> > >> + * > > Not needed line. > Fixed in v4. >> +#include <inttypes.h> >> +#include <stdio.h> > > + blank line? Would separate groups of headers logically. > >> +#include <linux/swab.h> > >> +static int64_t crc64_table[256] = {0,}; > > I guess {0} would work as well (no comma). Fixed them in v4. > >> + printf("#include <uapi/linux/types.h>\n"); >> + printf("#include <linux/cache.h>\n\n"); > > Do wee need these? CRC32 case seems fine without. > I renamed uapi/linux/types.h to linux/types.h, this header is for 'u64'. Another header linux/cache.h is for '____cacheline_aligned'. They should be necessary if these two headers are not included before lib/crc64table.h is included. Thanks for your review :-) Coly Li
