| 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 | expand |
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.
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
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
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. */ >
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.
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.
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. */
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> --- hw/net/eepro100.c | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-)