Re: [Qemu-devel] [PATCHv2 5/5] sunhme: switch sunhme_receive() over to use net_crc32_le()

[Mark Cave-Ayland]

On 06/12/17 03:34, Philippe Mathieu-Daudé wrote:

> Hi Mark,
>
> On 12/05/2017 05:17 AM, Mark Cave-Ayland wrote:
> > Signed-off-by: Mark Cave-Ayland <address@hidden>
> > ---
> >   hw/net/sunhme.c | 25 +------------------------
> >   1 file changed, 1 insertion(+), 24 deletions(-)
> >
> > diff --git a/hw/net/sunhme.c b/hw/net/sunhme.c
> > index b1efa1b88d..df66e2630c 100644
> > --- a/hw/net/sunhme.c
> > +++ b/hw/net/sunhme.c
> > @@ -698,29 +698,6 @@ static inline void sunhme_set_rx_ring_nr(SunHMEState *s, 
> > int i)
> >       s->erxregs[HME_ERXI_RING >> 2] = ring;
> >   }
> >   
> > -#define POLYNOMIAL_LE 0xedb88320
> > -static uint32_t sunhme_crc32_le(const uint8_t *p, int len)
> > -{
> > -    uint32_t crc;
> > -    int carry, i, j;
> > -    uint8_t b;
> > -
> > -    crc = 0xffffffff;
> > -    for (i = 0; i < len; i++) {
> > -        b = *p++;
> > -        for (j = 0; j < 8; j++) {
> > -            carry = (crc & 0x1) ^ (b & 0x01);
> > -            crc >>= 1;
> > -            b >>= 1;
> > -            if (carry) {
> > -                crc = crc ^ POLYNOMIAL_LE;
> > -            }
> > -        }
> > -    }
> > -
> > -    return crc;
> > -}
> > -
> >   #define MIN_BUF_SIZE 60
> >   
> >   static ssize_t sunhme_receive(NetClientState *nc, const uint8_t *buf,
> > @@ -761,7 +738,7 @@ static ssize_t sunhme_receive(NetClientState *nc, const 
> > uint8_t *buf,
> >               trace_sunhme_rx_filter_bcast_match();
> >           } else if (s->macregs[HME_MACI_RXCFG >> 2] & HME_MAC_RXCFG_HENABLE) {
> >               /* Didn't match local address, check hash filter */
> > -            int mcast_idx = sunhme_crc32_le(buf, 6) >> 26;
>
> This could be:
>
>                 int mcast_idx = compute_mcast_idx_le(buf);
>
> With:
>
> unsigned compute_mcast_idx_le(const uint8_t *ep)
> {
>      return net_crc32(ep, ETH_ALEN) >> 26;
> }

It looks like Stefan is suggesting in his comments that the inline 
approach is better, so I'll try to keep that style for the moment.

I do like the use of ETH_ALEN though so I'll add that in to the next 
version.

> Anyway:
> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
>
> > +            int mcast_idx = net_crc32_le(buf, 6) >> 26;
> >               if (!(s->macregs[(HME_MACI_HASHTAB0 >> 2) - (mcast_idx >> 4)] &
> >                       (1 << (mcast_idx & 0xf)))) {
> >                   /* Didn't match hash filter */

ATB,

Mark.