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. */
--
2.11.0
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.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
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.
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 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.
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. */
>
© 2016 - 2026 Red Hat, Inc.