Re: A new driver for Broadcom bcm5706

[Jeff Garzik <jgarzik@pobox.com>]

David S.Miller wrote:
> From: Jeff Garzik <jgarzik@pobox.com>
> Date: Fri, 20 May 2005 15:42:20 -0400
>>10) [additional review] doesn't bnx2_rx_int() need to move the rmb()
>>inside the loop?  Are you protecting against the compiler
>>reordering/caching loads/stores, or against SMP CPUs?
> 
> 
> This rmb() is needed to strongly order the status block consumer
> index read, from that of the descriptor data.  I'm pretty sure it's
> in the right spot.
> 
> 
>>10.1) [additional review] is the rmb() even needed in bnx2_rx_int(),
>>since its caller also uses rmb()?
> 
> 
> No, it's guarding status block consumer index read to the first
> RX descriptor read, as explained above.

OK


>>13) [additional review] why is CHECKSUM_UNNECESSARY used when
>>cksum==0xffff or cksum==0 ?
>>
>>+                       u16 cksum = rx_hdr->l2_fhdr_tcp_udp_xsum;
>>+
>>+                       if ((cksum == 0xffff) || (cksum == 0)) {
>>+                               skb->ip_summed = CHECKSUM_UNNECESSARY;
>>+                       }
> 
> 
> For UDP, a checksum value of zero means no checksum at all.

Sure.  What I'm driving at is that a checksum of zero seems to imply 
CHECKSUM_NONE not CHECKSUM_UNNECESSARY.  tg3 only does the 0xffff check.

I am also a bit surprised that, if the actual checksum value is 
available, why not use CHECKSUM_HW like sunhme?

	Jeff