| Message ID | 20180723162020.6221-1-krzk@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | net: ethernet: fs-enet: Use generic CRC32 implementation | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
| snowpatch_ozlabs/checkpatch | success | Test checkpatch on branch next |
| snowpatch_ozlabs/build-ppc64le | success | Test build-ppc64le on branch next |
| snowpatch_ozlabs/build-ppc64be | success | Test build-ppc64be on branch next |
| snowpatch_ozlabs/build-ppc64e | success | Test build-ppc64e on branch next |
| snowpatch_ozlabs/build-ppc32 | success | Test build-ppc32 on branch next |
From: Krzysztof Kozlowski > Sent: 23 July 2018 17:20 > Use generic kernel CRC32 implementation because it: > 1. Should be faster (uses lookup tables), Are you sure? The lookup tables are unlikely to be in the data cache and the 6 cache misses kill performance. (Not that it particularly matters when setting up multicast hash tables). > 2. Removes duplicated CRC generation code, > 3. Uses well-proven algorithm instead of coding it one more time. ... > > Not tested on hardware. Have you verified that the old and new functions give the same result for a few mac addresses? It is very easy to use the wrong bits in crc calculations or generate the output in the wrong bit order. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 24 July 2018 at 13:05, David Laight <David.Laight@aculab.com> wrote: > From: Krzysztof Kozlowski >> Sent: 23 July 2018 17:20 >> Use generic kernel CRC32 implementation because it: >> 1. Should be faster (uses lookup tables), > > Are you sure? > The lookup tables are unlikely to be in the data cache and > the 6 cache misses kill performance. > (Not that it particularly matters when setting up multicast hash tables). Good point, so this statement should be rather "Could be faster"... I did not run any performance tests so this is not backed up by any data. I think the main benefit is rather easier code maintenance by removing duplicated, custom code. >> 2. Removes duplicated CRC generation code, >> 3. Uses well-proven algorithm instead of coding it one more time. > ... >> >> Not tested on hardware. > > Have you verified that the old and new functions give the > same result for a few mac addresses? > It is very easy to use the wrong bits in crc calculations > or generate the output in the wrong bit order. I copied the original code and new one onto a different driver and run this in a loop for thousands of data input (although not all possible MAC combinations). The output was the same. I agree however that real testing would be important. Best regards, Krzysztof
From: Krzysztof Kozlowski > Sent: 24 July 2018 12:12 ... > >> Not tested on hardware. > > > > Have you verified that the old and new functions give the > > same result for a few mac addresses? > > It is very easy to use the wrong bits in crc calculations > > or generate the output in the wrong bit order. > > I copied the original code and new one onto a different driver and run > this in a loop for thousands of data input (although not all possible > MAC combinations). The output was the same. I agree however that real > testing would be important. Since CRC are linear you only need to check that each input bit generates the correct output. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
From: Krzysztof Kozlowski <krzk@kernel.org> Date: Mon, 23 Jul 2018 18:20:20 +0200 > Use generic kernel CRC32 implementation because it: > 1. Should be faster (uses lookup tables), > 2. Removes duplicated CRC generation code, > 3. Uses well-proven algorithm instead of coding it one more time. > > Suggested-by: Eric Biggers <ebiggers3@gmail.com> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> Applied.
diff --git a/drivers/net/ethernet/freescale/fs_enet/mac-fec.c b/drivers/net/ethernet/freescale/fs_enet/mac-fec.c index 1fc27c97e3b2..99fe2c210d0f 100644 --- a/drivers/net/ethernet/freescale/fs_enet/mac-fec.c +++ b/drivers/net/ethernet/freescale/fs_enet/mac-fec.c @@ -18,6 +18,7 @@ #include <linux/string.h> #include <linux/ptrace.h> #include <linux/errno.h> +#include <linux/crc32.h> #include <linux/ioport.h> #include <linux/interrupt.h> #include <linux/delay.h> @@ -176,21 +177,10 @@ static void set_multicast_start(struct net_device *dev) static void set_multicast_one(struct net_device *dev, const u8 *mac) { struct fs_enet_private *fep = netdev_priv(dev); - int temp, hash_index, i, j; + int temp, hash_index; u32 crc, csrVal; - u8 byte, msb; - - crc = 0xffffffff; - for (i = 0; i < 6; i++) { - byte = mac[i]; - for (j = 0; j < 8; j++) { - msb = crc >> 31; - crc <<= 1; - if (msb ^ (byte & 0x1)) - crc ^= FEC_CRC_POLY; - byte >>= 1; - } - } + + crc = ether_crc(6, mac); temp = (crc & 0x3f) >> 1; hash_index = ((temp & 0x01) << 4) |
Use generic kernel CRC32 implementation because it: 1. Should be faster (uses lookup tables), 2. Removes duplicated CRC generation code, 3. Uses well-proven algorithm instead of coding it one more time. Suggested-by: Eric Biggers <ebiggers3@gmail.com> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> --- Not tested on hardware. --- drivers/net/ethernet/freescale/fs_enet/mac-fec.c | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-)