[PATCHv2,4/5] eepro100: switch e100_compute_mcast_idx() over to use net_crc32()

Message ID 20171205081744.6563-5-mark.cave-ayland@ilande.co.uk
State New
Headers show
Series
  • net: introduce common net_crc32() and net_crc32_le() functions
Related show

Commit Message

Mark Cave-Ayland Dec. 5, 2017, 8:17 a.m.
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/net/eepro100.c | 19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

Comments

Eric Blake Dec. 5, 2017, 2:28 p.m. | #1
On 12/05/2017 02:17 AM, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/net/eepro100.c | 19 +------------------
>  1 file changed, 1 insertion(+), 18 deletions(-)
> 

> -            if (carry) {
> -                crc = ((crc ^ POLYNOMIAL) | carry);

How does this compile after 1/5 renames POLYNOMIAL to POLYNOMIAL_BE in
net.h?

/me looks

Oh, you have a redundant definition in the .c file, which is now a dead
define.  Patch 1 should be updated to remove the duplicate definitions,
and fix code to uniformly use POLYNOMIAL_BE.

But overall, I like what the series is doing.
Stefan Weil Dec. 5, 2017, 3:13 p.m. | #2
Am 05.12.2017 um 09:17 schrieb Mark Cave-Ayland:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/net/eepro100.c | 19 +------------------
>  1 file changed, 1 insertion(+), 18 deletions(-)
>
> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
> index 1c0def555b..4fe94b7471 100644
> --- a/hw/net/eepro100.c
> +++ b/hw/net/eepro100.c
> @@ -327,26 +327,9 @@ static const uint16_t eepro100_mdi_mask[] = {
>  
>  static E100PCIDeviceInfo *eepro100_get_class(EEPRO100State *s);
>  
> -/* From FreeBSD (locally modified). */
>  static unsigned e100_compute_mcast_idx(const uint8_t *ep)
>  {
> -    uint32_t crc;
> -    int carry, i, j;
> -    uint8_t b;
> -
> -    crc = 0xffffffff;
> -    for (i = 0; i < 6; i++) {
> -        b = *ep++;
> -        for (j = 0; j < 8; j++) {
> -            carry = ((crc & 0x80000000L) ? 1 : 0) ^ (b & 0x01);
> -            crc <<= 1;
> -            b >>= 1;
> -            if (carry) {
> -                crc = ((crc ^ POLYNOMIAL) | carry);
> -            }
> -        }
> -    }
> -    return (crc & BITS(7, 2)) >> 2;
> +    return (net_crc32(ep, 6) & BITS(7, 2)) >> 2;
>  }
>  
>  /* Read a 16 bit control/status (CSR) register. */


What about eliminating the intermediate function e100_compute_mcast_idx (and function lnc_mchash, too)?
You did that for lnc_mchash, and I think that is cleaner and saves some lines of code.

With or without that minor change:

Reviewed-by: Stefan Weil <sw@weilnetz.de>

Regards
Stefan
Stefan Weil Dec. 5, 2017, 3:16 p.m. | #3
Am 05.12.2017 um 16:13 schrieb Stefan Weil:
> Am 05.12.2017 um 09:17 schrieb Mark Cave-Ayland:
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>  hw/net/eepro100.c | 19 +------------------
>>  1 file changed, 1 insertion(+), 18 deletions(-)
>>
>> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
>> index 1c0def555b..4fe94b7471 100644
>> --- a/hw/net/eepro100.c
>> +++ b/hw/net/eepro100.c
>> @@ -327,26 +327,9 @@ static const uint16_t eepro100_mdi_mask[] = {
>>  
>>  static E100PCIDeviceInfo *eepro100_get_class(EEPRO100State *s);
>>  
>> -/* From FreeBSD (locally modified). */
>>  static unsigned e100_compute_mcast_idx(const uint8_t *ep)
>>  {
>> -    uint32_t crc;
>> -    int carry, i, j;
>> -    uint8_t b;
>> -
>> -    crc = 0xffffffff;
>> -    for (i = 0; i < 6; i++) {
>> -        b = *ep++;
>> -        for (j = 0; j < 8; j++) {
>> -            carry = ((crc & 0x80000000L) ? 1 : 0) ^ (b & 0x01);
>> -            crc <<= 1;
>> -            b >>= 1;
>> -            if (carry) {
>> -                crc = ((crc ^ POLYNOMIAL) | carry);
>> -            }
>> -        }
>> -    }
>> -    return (crc & BITS(7, 2)) >> 2;
>> +    return (net_crc32(ep, 6) & BITS(7, 2)) >> 2;
>>  }
>>  
>>  /* Read a 16 bit control/status (CSR) register. */
> 
> 
> What about eliminating the intermediate function e100_compute_mcast_idx (and function lnc_mchash, too)?
> You did that for lnc_mchash, and I think that is cleaner and saves some lines of code.

This should be "You did that for sunhme_crc32_le" ...

Stefan
Philippe Mathieu-Daudé Dec. 6, 2017, 3:28 a.m. | #4
On 12/05/2017 05:17 AM, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/net/eepro100.c | 19 +------------------
>  1 file changed, 1 insertion(+), 18 deletions(-)
> 
> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
> index 1c0def555b..4fe94b7471 100644
> --- a/hw/net/eepro100.c
> +++ b/hw/net/eepro100.c
> @@ -327,26 +327,9 @@ static const uint16_t eepro100_mdi_mask[] = {
>  
>  static E100PCIDeviceInfo *eepro100_get_class(EEPRO100State *s);
>  
> -/* From FreeBSD (locally modified). */
>  static unsigned e100_compute_mcast_idx(const uint8_t *ep)
>  {
> -    uint32_t crc;
> -    int carry, i, j;
> -    uint8_t b;
> -
> -    crc = 0xffffffff;
> -    for (i = 0; i < 6; i++) {
> -        b = *ep++;
> -        for (j = 0; j < 8; j++) {
> -            carry = ((crc & 0x80000000L) ? 1 : 0) ^ (b & 0x01);
> -            crc <<= 1;
> -            b >>= 1;
> -            if (carry) {
> -                crc = ((crc ^ POLYNOMIAL) | carry);
> -            }
> -        }
> -    }
> -    return (crc & BITS(7, 2)) >> 2;
> +    return (net_crc32(ep, 6) & BITS(7, 2)) >> 2;
>  }
>  
>  /* Read a 16 bit control/status (CSR) register. */
>
Mark Cave-Ayland Dec. 7, 2017, 5:08 a.m. | #5
On 05/12/17 14:28, Eric Blake wrote:

> On 12/05/2017 02:17 AM, Mark Cave-Ayland wrote:
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/net/eepro100.c | 19 +------------------
>>   1 file changed, 1 insertion(+), 18 deletions(-)
>>
> 
>> -            if (carry) {
>> -                crc = ((crc ^ POLYNOMIAL) | carry);
> 
> How does this compile after 1/5 renames POLYNOMIAL to POLYNOMIAL_BE in
> net.h?
> 
> /me looks
> 
> Oh, you have a redundant definition in the .c file, which is now a dead
> define.  Patch 1 should be updated to remove the duplicate definitions,
> and fix code to uniformly use POLYNOMIAL_BE.

Ah yes, I can fix that up on a v3.

> But overall, I like what the series is doing.

Great stuff, in that case I'll fix it up based upon all the comments and 
continue. It has been lying around in a local branch for months now...

BTW one thing I did notice is that sungem.c calls zlib's crc32 function 
directly which doesn't seem right, so I'll probably add that into the 
next version too. Once this has been done, switching the new 
net_crc32()/net_crc32_le() functions over to use a LUT or zlib or 
something else as the underlying implementation should be trivial.


ATB,

Mark.
Mark Cave-Ayland Dec. 7, 2017, 5:15 a.m. | #6
On 05/12/17 15:13, Stefan Weil wrote:

> Am 05.12.2017 um 09:17 schrieb Mark Cave-Ayland:
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/net/eepro100.c | 19 +------------------
>>   1 file changed, 1 insertion(+), 18 deletions(-)
>>
>> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
>> index 1c0def555b..4fe94b7471 100644
>> --- a/hw/net/eepro100.c
>> +++ b/hw/net/eepro100.c
>> @@ -327,26 +327,9 @@ static const uint16_t eepro100_mdi_mask[] = {
>>   
>>   static E100PCIDeviceInfo *eepro100_get_class(EEPRO100State *s);
>>   
>> -/* From FreeBSD (locally modified). */
>>   static unsigned e100_compute_mcast_idx(const uint8_t *ep)
>>   {
>> -    uint32_t crc;
>> -    int carry, i, j;
>> -    uint8_t b;
>> -
>> -    crc = 0xffffffff;
>> -    for (i = 0; i < 6; i++) {
>> -        b = *ep++;
>> -        for (j = 0; j < 8; j++) {
>> -            carry = ((crc & 0x80000000L) ? 1 : 0) ^ (b & 0x01);
>> -            crc <<= 1;
>> -            b >>= 1;
>> -            if (carry) {
>> -                crc = ((crc ^ POLYNOMIAL) | carry);
>> -            }
>> -        }
>> -    }
>> -    return (crc & BITS(7, 2)) >> 2;
>> +    return (net_crc32(ep, 6) & BITS(7, 2)) >> 2;
>>   }
>>   
>>   /* Read a 16 bit control/status (CSR) register. */
> 
> 
> What about eliminating the intermediate function e100_compute_mcast_idx (and function lnc_mchash, too)?
> You did that for lnc_mchash, and I think that is cleaner and saves some lines of code.

Yes, I can do if you like. The reason I've left these as they are for 
the moment is that I don't have something readily available to test 
multicast for eepro100 post-conversion (my SPARC/PPC images cover 
pcnet/sunhme) but if you are happy the functionality is the same during 
review then I can go ahead and do it.

I don't really mind exactly how we do the conversion as long as we aim 
for consistency.

> With or without that minor change:
> 
> Reviewed-by: Stefan Weil <sw@weilnetz.de>
> 
> Regards
> Stefan

ATB,

Mark.

Patch

diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index 1c0def555b..4fe94b7471 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -327,26 +327,9 @@  static const uint16_t eepro100_mdi_mask[] = {
 
 static E100PCIDeviceInfo *eepro100_get_class(EEPRO100State *s);
 
-/* From FreeBSD (locally modified). */
 static unsigned e100_compute_mcast_idx(const uint8_t *ep)
 {
-    uint32_t crc;
-    int carry, i, j;
-    uint8_t b;
-
-    crc = 0xffffffff;
-    for (i = 0; i < 6; i++) {
-        b = *ep++;
-        for (j = 0; j < 8; j++) {
-            carry = ((crc & 0x80000000L) ? 1 : 0) ^ (b & 0x01);
-            crc <<= 1;
-            b >>= 1;
-            if (carry) {
-                crc = ((crc ^ POLYNOMIAL) | carry);
-            }
-        }
-    }
-    return (crc & BITS(7, 2)) >> 2;
+    return (net_crc32(ep, 6) & BITS(7, 2)) >> 2;
 }
 
 /* Read a 16 bit control/status (CSR) register. */