[PATCH] net: ethernet: fs-enet: Use generic CRC32 implementation

[PATCH] net: ethernet: fs-enet: Use generic CRC32 implementation

[Krzysztof Kozlowski]

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(-)

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) |
-- 
2.14.1

RE: [PATCH] net: ethernet: fs-enet: Use generic CRC32 implementation

[David Laight]

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.
...
>=20
> 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.

=09David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1=
PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] net: ethernet: fs-enet: Use generic CRC32 implementation

[Krzysztof Kozlowski]

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

RE: [PATCH] net: ethernet: fs-enet: Use generic CRC32 implementation

[David Laight]

RnJvbTogS3J6eXN6dG9mIEtvemxvd3NraQ0KPiBTZW50OiAyNCBKdWx5IDIwMTggMTI6MTINCi4u
Lg0KPiA+PiBOb3QgdGVzdGVkIG9uIGhhcmR3YXJlLg0KPiA+DQo+ID4gSGF2ZSB5b3UgdmVyaWZp
ZWQgdGhhdCB0aGUgb2xkIGFuZCBuZXcgZnVuY3Rpb25zIGdpdmUgdGhlDQo+ID4gc2FtZSByZXN1
bHQgZm9yIGEgZmV3IG1hYyBhZGRyZXNzZXM/DQo+ID4gSXQgaXMgdmVyeSBlYXN5IHRvIHVzZSB0
aGUgd3JvbmcgYml0cyBpbiBjcmMgY2FsY3VsYXRpb25zDQo+ID4gb3IgZ2VuZXJhdGUgdGhlIG91
dHB1dCBpbiB0aGUgd3JvbmcgYml0IG9yZGVyLg0KPiANCj4gSSBjb3BpZWQgdGhlIG9yaWdpbmFs
IGNvZGUgYW5kIG5ldyBvbmUgb250byBhIGRpZmZlcmVudCBkcml2ZXIgYW5kIHJ1bg0KPiB0aGlz
IGluIGEgbG9vcCBmb3IgdGhvdXNhbmRzIG9mIGRhdGEgaW5wdXQgKGFsdGhvdWdoIG5vdCBhbGwg
cG9zc2libGUNCj4gTUFDIGNvbWJpbmF0aW9ucykuIFRoZSBvdXRwdXQgd2FzIHRoZSBzYW1lLiBJ
IGFncmVlIGhvd2V2ZXIgdGhhdCByZWFsDQo+IHRlc3Rpbmcgd291bGQgYmUgaW1wb3J0YW50Lg0K
DQpTaW5jZSBDUkMgYXJlIGxpbmVhciB5b3Ugb25seSBuZWVkIHRvIGNoZWNrIHRoYXQgZWFjaCBp
bnB1dA0KYml0IGdlbmVyYXRlcyB0aGUgY29ycmVjdCBvdXRwdXQuDQoNCglEYXZpZA0KDQotDQpS
ZWdpc3RlcmVkIEFkZHJlc3MgTGFrZXNpZGUsIEJyYW1sZXkgUm9hZCwgTW91bnQgRmFybSwgTWls
dG9uIEtleW5lcywgTUsxIDFQVCwgVUsNClJlZ2lzdHJhdGlvbiBObzogMTM5NzM4NiAoV2FsZXMp
DQo=

Re: [PATCH] net: ethernet: fs-enet: Use generic CRC32 implementation

[David Miller]

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.