Dear Tom Rini,
In message 5186723E.2080503@ti.com you wrote:
These changes should be factored out into a separate commit. In any case, the code should be compiled in only when needed, i. e. when btrfs support is selected. Otherwise you just bloat the code size and build time for all systems without need.
We should be able to make lib/crc32_c.o depend on btrfs being set, yes. But non-command stuff should be getting garbage collected out in most cases at last (Need to poke Albert about the patch for ARM).
Yes, it may be garbage collected, but only after compilation - which means we add to the build time for about 1000 boards which do not use this.
Hm... do we really need yet another crc32 implementation?
We talked about this before I believe and the answer is yes :(
Yes, I remember this discussion. But look at the code. It raises a number of questions:
- Why do we have to calculate the crc32c_table[] at runtime? Our regular CRC code uses a pre-calculated table; we should do the same here.
- Compare the code for crc32c_cal() in the patch with the definition of DO_CRC(x) in "lib/crc32.c" - to me, it appears to be the same for little endian code (it is redundant?), but different for big endian systems - which raises the question if this code has ever been tested on a BE machine?
- The code claims to be derived from "Linux kernel crypto/crc32c.c"; but I cannot find such code in that file.
- The implementation of crc32c_cal() suffers from a few other problems (like not triggering the watchdog, which will cause problems on systems that use one).
I think we should re-check this. My feeling is that we may eventually end up with a different CRC table, but without need for new code.
Best regards,
Wolfgang Denk