Whilst trying to debug a CRC32 endian issue for NIC multicast hash lookups, it
struck me that it would make sense to have a common set of standard ethernet
CRC32 functions (both little and big endian variants) in net.c.

Patches 1 and 2 introduce the new net_crc32() and net_crc32_le() functions for
use by NIC multicast hash lookups.

Patches 3 to 5 switch the pcnet, eepro100 and sunhme drivers over to use the
new functions instead of having their own private implementations.

Patch 6 fixes the sungem multicast filter CRC calculation, since we can see from
the Linux sungem driver that the CRC is calculated using ether_crc32_le() and so
the direct use of the zlib crc32() function is incorrect. The solution here is to
simply use the corresponding net_crc32_le() function.

Patches 7 to 12 switch the remaining users of compute_mcast_idx() over to use
the new net_crc32() function as it becomes much easier to spot errors in the
multicast hash calculations (see below).

Finally patch 13 removes the now unused compute_mcast_idx() function.

One of the motivations for removing compute_mcast_idx() and using the CRC and
bitshift inline is because it makes it much easier to spot potential errors
when comparing with the corresponding driver source code. This led to the following
curiosities whilst developing the patchset:

1) The sungem multicast filter CRC calculation was incorrect (fixed by patch 6 as
   I was able to test locally)

2) After conversion we can see in eepro100.c line 1682 that there is one single
   remaining multicast hash calculation that doesn't apply a BITS(7, 2) mask to
   the filter index when compared with all the others. This looks incorrect, but
   I don't have an easy way to verify this.

3) In ftgmac100.c we see a comment next to the multicast hash filter calculation
   that states "TODO: this does not seem to work for ftgmac100". Upon consulting the
   Linux driver source we can see that ether_crc32_le() is used in the driver
   suggesting that changing net_crc32() to net_crc32_le() should fix the calculation.
   Again I've left this as I don't have an easy way to verify the fix.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

v3:
- Add R-B and S-o-B tags
- Fix sungem multicast hash filter
- Use ETH_ALEN to specift MAC address length as suggested by Phillipe
- Switch to using inline CRC + bitshift as suggested by Stefan (i.e. remove
  what's left of the remaining hash function)
- Convert all remaining users of compute_mcast_idx() to inline CRC + bitshift
  (and then remove it) to make it easier to validate the multicast hash index
  calculation with the corresponding driver source

v2:
- Add sumhme net_crc32_le() conversion

Mark Cave-Ayland (13):
  net: move CRC32 calculation from compute_mcast_idx() into its own
    net_crc32() function
  net: introduce net_crc32_le() function
  pcnet: switch pcnet over to use net_crc32_le()
  eepro100: switch eepro100 e100_compute_mcast_idx() over to use
    net_crc32()
  sunhme: switch sunhme over to use net_crc32_le()
  sungem: fix multicast filter CRC calculation
  eepro100: use inline net_crc32() and bitshift instead of
    compute_mcast_idx()
  opencores_eth: use inline net_crc32() and bitshift instead of
    compute_mcast_idx()
  lan9118: use inline net_crc32() and bitshift instead of
    compute_mcast_idx()
  ftgmac100: use inline net_crc32() and bitshift instead of
    compute_mcast_idx()
  ne2000: use inline net_crc32() and bitshift instead of
    compute_mcast_idx()
  rtl8139: use inline net_crc32() and bitshift instead of
    compute_mcast_idx()
  net: remove unused compute_mcast_idx() function

 hw/net/eepro100.c      | 32 +++++---------------------------
 hw/net/ftgmac100.c     |  2 +-
 hw/net/lan9118.c       |  3 ++-
 hw/net/ne2000.c        |  3 ++-
 hw/net/opencores_eth.c |  3 ++-
 hw/net/pcnet.c         | 22 ++--------------------
 hw/net/rtl8139.c       |  2 +-
 hw/net/sungem.c        |  5 ++---
 hw/net/sunhme.c        | 25 +------------------------
 include/net/net.h      |  5 ++++-
 net/net.c              | 33 ++++++++++++++++++++++++++++-----
 11 files changed, 50 insertions(+), 85 deletions(-)

-- 
2.11.0

On 2017年12月16日 02:41, Mark Cave-Ayland wrote:
> Whilst trying to debug a CRC32 endian issue for NIC multicast hash lookups, it
> struck me that it would make sense to have a common set of standard ethernet
> CRC32 functions (both little and big endian variants) in net.c.
>
> Patches 1 and 2 introduce the new net_crc32() and net_crc32_le() functions for
> use by NIC multicast hash lookups.
>
> Patches 3 to 5 switch the pcnet, eepro100 and sunhme drivers over to use the
> new functions instead of having their own private implementations.
>
> Patch 6 fixes the sungem multicast filter CRC calculation, since we can see from
> the Linux sungem driver that the CRC is calculated using ether_crc32_le() and so
> the direct use of the zlib crc32() function is incorrect. The solution here is to
> simply use the corresponding net_crc32_le() function.
>
> Patches 7 to 12 switch the remaining users of compute_mcast_idx() over to use
> the new net_crc32() function as it becomes much easier to spot errors in the
> multicast hash calculations (see below).
>
> Finally patch 13 removes the now unused compute_mcast_idx() function.
>
> One of the motivations for removing compute_mcast_idx() and using the CRC and
> bitshift inline is because it makes it much easier to spot potential errors
> when comparing with the corresponding driver source code. This led to the following
> curiosities whilst developing the patchset:
>
> 1) The sungem multicast filter CRC calculation was incorrect (fixed by patch 6 as
>     I was able to test locally)
>
> 2) After conversion we can see in eepro100.c line 1682 that there is one single
>     remaining multicast hash calculation that doesn't apply a BITS(7, 2) mask to
>     the filter index when compared with all the others. This looks incorrect, but
>     I don't have an easy way to verify this.
>
> 3) In ftgmac100.c we see a comment next to the multicast hash filter calculation
>     that states "TODO: this does not seem to work for ftgmac100". Upon consulting the
>     Linux driver source we can see that ether_crc32_le() is used in the driver
>     suggesting that changing net_crc32() to net_crc32_le() should fix the calculation.
>     Again I've left this as I don't have an easy way to verify the fix.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Series looks good to me.

A small question is that, is this better to keep compute_mcast_idx()?

Thanks

>
> v3:
> - Add R-B and S-o-B tags
> - Fix sungem multicast hash filter
> - Use ETH_ALEN to specift MAC address length as suggested by Phillipe
> - Switch to using inline CRC + bitshift as suggested by Stefan (i.e. remove
>    what's left of the remaining hash function)
> - Convert all remaining users of compute_mcast_idx() to inline CRC + bitshift
>    (and then remove it) to make it easier to validate the multicast hash index
>    calculation with the corresponding driver source
>
> v2:
> - Add sumhme net_crc32_le() conversion
>
> Mark Cave-Ayland (13):
>    net: move CRC32 calculation from compute_mcast_idx() into its own
>      net_crc32() function
>    net: introduce net_crc32_le() function
>    pcnet: switch pcnet over to use net_crc32_le()
>    eepro100: switch eepro100 e100_compute_mcast_idx() over to use
>      net_crc32()
>    sunhme: switch sunhme over to use net_crc32_le()
>    sungem: fix multicast filter CRC calculation
>    eepro100: use inline net_crc32() and bitshift instead of
>      compute_mcast_idx()
>    opencores_eth: use inline net_crc32() and bitshift instead of
>      compute_mcast_idx()
>    lan9118: use inline net_crc32() and bitshift instead of
>      compute_mcast_idx()
>    ftgmac100: use inline net_crc32() and bitshift instead of
>      compute_mcast_idx()
>    ne2000: use inline net_crc32() and bitshift instead of
>      compute_mcast_idx()
>    rtl8139: use inline net_crc32() and bitshift instead of
>      compute_mcast_idx()
>    net: remove unused compute_mcast_idx() function
>
>   hw/net/eepro100.c      | 32 +++++---------------------------
>   hw/net/ftgmac100.c     |  2 +-
>   hw/net/lan9118.c       |  3 ++-
>   hw/net/ne2000.c        |  3 ++-
>   hw/net/opencores_eth.c |  3 ++-
>   hw/net/pcnet.c         | 22 ++--------------------
>   hw/net/rtl8139.c       |  2 +-
>   hw/net/sungem.c        |  5 ++---
>   hw/net/sunhme.c        | 25 +------------------------
>   include/net/net.h      |  5 ++++-
>   net/net.c              | 33 ++++++++++++++++++++++++++++-----
>   11 files changed, 50 insertions(+), 85 deletions(-)
>

On 20/12/17 08:35, Jason Wang wrote:
> 
> 
> On 2017年12月16日 02:41, Mark Cave-Ayland wrote:
>> Whilst trying to debug a CRC32 endian issue for NIC multicast hash 
>> lookups, it
>> struck me that it would make sense to have a common set of standard 
>> ethernet
>> CRC32 functions (both little and big endian variants) in net.c.
>>
>> Patches 1 and 2 introduce the new net_crc32() and net_crc32_le() 
>> functions for
>> use by NIC multicast hash lookups.
>>
>> Patches 3 to 5 switch the pcnet, eepro100 and sunhme drivers over to 
>> use the
>> new functions instead of having their own private implementations.
>>
>> Patch 6 fixes the sungem multicast filter CRC calculation, since we 
>> can see from
>> the Linux sungem driver that the CRC is calculated using 
>> ether_crc32_le() and so
>> the direct use of the zlib crc32() function is incorrect. The solution 
>> here is to
>> simply use the corresponding net_crc32_le() function.
>>
>> Patches 7 to 12 switch the remaining users of compute_mcast_idx() over 
>> to use
>> the new net_crc32() function as it becomes much easier to spot errors 
>> in the
>> multicast hash calculations (see below).
>>
>> Finally patch 13 removes the now unused compute_mcast_idx() function.
>>
>> One of the motivations for removing compute_mcast_idx() and using the 
>> CRC and
>> bitshift inline is because it makes it much easier to spot potential 
>> errors
>> when comparing with the corresponding driver source code. This led to 
>> the following
>> curiosities whilst developing the patchset:
>>
>> 1) The sungem multicast filter CRC calculation was incorrect (fixed by 
>> patch 6 as
>>     I was able to test locally)
>>
>> 2) After conversion we can see in eepro100.c line 1682 that there is 
>> one single
>>     remaining multicast hash calculation that doesn't apply a BITS(7, 
>> 2) mask to
>>     the filter index when compared with all the others. This looks 
>> incorrect, but
>>     I don't have an easy way to verify this.
>>
>> 3) In ftgmac100.c we see a comment next to the multicast hash filter 
>> calculation
>>     that states "TODO: this does not seem to work for ftgmac100". Upon 
>> consulting the
>>     Linux driver source we can see that ether_crc32_le() is used in 
>> the driver
>>     suggesting that changing net_crc32() to net_crc32_le() should fix 
>> the calculation.
>>     Again I've left this as I don't have an easy way to verify the fix.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> Series looks good to me.
> 
> A small question is that, is this better to keep compute_mcast_idx()?
> 
> Thanks

Hi Jason,

I did think about this, however at the very minimum you'd need 
big-endian and little-endian variants of compute_mcast_idx(), and then 
you see that eepro100 applies a different bitmask/shift which is yet 
another variant...

For this reason I moved them all inline to the QEMU driver and that made 
it possible to compare the hash calculation directly with the 
corresponding Linux driver which found the 3 potential bugs above. So I 
think this is a net win (pardon the pun) on all sides :)


ATB,

Mark.

On 2017年12月20日 16:58, Mark Cave-Ayland wrote:
>> Series looks good to me.
>>
>> A small question is that, is this better to keep compute_mcast_idx()?
>>
>> Thanks
>
> Hi Jason,
>
> I did think about this, however at the very minimum you'd need 
> big-endian and little-endian variants of compute_mcast_idx(), and then 
> you see that eepro100 applies a different bitmask/shift which is yet 
> another variant...
>
> For this reason I moved them all inline to the QEMU driver and that 
> made it possible to compare the hash calculation directly with the 
> corresponding Linux driver which found the 3 potential bugs above. So 
> I think this is a net win (pardon the pun) on all sides :)
>
>
> ATB,

Ok, applied.

Thanks