On Fri, 2005-05-20 at 15:28 -0700, David S.Miller wrote:

From: Jeff Garzik <redacted>
Date: Fri, 20 May 2005 15:42:20 -0400

quoted

9) [additional review]  DaveM, others: is this correct for all arches?

+       if (unlikely((align = (unsigned long) skb->data & 0x7))) {
+               skb_reserve(skb, 8 - align);
+       }

It's probably not even necessary.  dev_alloc_skb() should be returning
an SKB with skb->data at least cache_line_size() aligned (see mm/slab.c)
unless the platform defines an ARCH_KMALLOC_MINALIGN override.

If I remember correctly, I was seeing some SKB with skb->data that is 4-
byte aligned on some Fedora kernels. I don't remember which kernel. This
device has an alignment requirement of at least 8-bytes for the receive
buffers.

quoted

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.

quoted

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.

That's right. We saw rare failures in Anton's PPC environment that
caused the driver to read stale rx buffer descriptors ahead of the rx
index in the status block without the rmb().

quoted

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.

Yes, but in this case, cksum is the checksum calculated over the entire
TCP/UDP packet including the pseudo IP header. Jeff is right, it should
always be 0xffff when the checksum is correct. Checking for zero is a
bug.

quoted

18) you can eliminate one call to request_irq, by lengthening the 'if' test:

Of just assigning a function pointer and set of flags to a local variable.
Then doing on request_irq call.

quoted

20) is there any reason to #ifdef BCM_TSO?  2.4.x kernels I suppose?

Yeah, same for MSI stuff as well.

quoted

27) isn't 'timer_interval == HZ' too rapid a timer?  Does it really need
to fire every second?

The pulse has to be written the the chip at least once every 50 milliseconds,
or else the chip goes into OS absent mode.  Anyways, the timer interval can
probably be extended, although link probing at 1 second intervals does seem
reasonable.

Originally, the driver pulse interval was set at 250 msec, but it's been
extended to a few seconds. So the driver currently will write the pulse
every second and do the serdes related checking at the same time.

David, Do you want me to fix some of these things and send you a new
500K patch or just send incremental patches over the existing driver?