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-ayl...@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.
