A new driver for Broadcom bcm5706

A new driver for Broadcom bcm5706

[Michael Chan]

A new driver bnx2 for Broadcom bcm5706 is available. Since the patch is
over 500K, I've put it on the ftp server:

ftp://Net_sys_anon@ftp1.broadcom.com/bnx2-2.patch

The patch also includes new 1000BASE-X advertisement bit definitions in
mii.h

Thanks to David Miller and Jeff Garzik for reviewing and their valuable
feedback.

Signed-off-by: Michael Chan <mchan@broadcom.com>

Re: A new driver for Broadcom bcm5706

[Jeff Garzik]

On Fri, May 20, 2005 at 10:15:29AM -0700, Michael Chan wrote:
> A new driver bnx2 for Broadcom bcm5706 is available. Since the patch is
> over 500K, I've put it on the ftp server:
> 
> ftp://Net_sys_anon@ftp1.broadcom.com/bnx2-2.patch
> 
> The patch also includes new 1000BASE-X advertisement bit definitions in
> mii.h


Here's my in-depth review:


0) Please use 'jgarzik@pobox.com' for open source stuff, and
'jgarzik@redhat.com' only for private Red Hat (NDA'd) stuff.  I know all
these email addresses can be confusing; I apologize.


1) [style] don't use braces, when a single statement follows an 'if',
'for', etc.

+               bp->duplex = DUPLEX_FULL;
+       }
+       else {
+               bp->duplex = DUPLEX_HALF;
+       }
+
+       if (!(bmcr & BMCR_ANENABLE)) {
+               return 0;
+       }

[several areas of code need this]


2) bnx2_read_phy() returns a value, but the code quite often ignores its
return value.


3) ditto for bnx2_write_phy()


4) ditto for bnx2_reset_phy()


5) ditto for bnx2_init_phy()


6) too many magic numbers in phy init.  Please define constants for the
BCM-specific phy registers, and register bits, where reasonable.

+       if (bp->dev->mtu > 1500) {
+               u32 val;
+
+               /* Set extended packet length bit */
+               bnx2_write_phy(bp, 0x18, 0x7);
+               bnx2_read_phy(bp, 0x18, &val);
+               bnx2_write_phy(bp, 0x18, (val & 0xfff8) | 0x4000);
+
+               bnx2_write_phy(bp, 0x1c, 0x6c00);
+               bnx2_read_phy(bp, 0x1c, &val);
+               bnx2_write_phy(bp, 0x1c, (val & 0x3ff) | 0xec02);
+       }
+       else {
+               u32 val;
+
+               bnx2_write_phy(bp, 0x18, 0x7);
+               bnx2_read_phy(bp, 0x18, &val);
+               bnx2_write_phy(bp, 0x18, val & ~0x4007);
+
+               bnx2_write_phy(bp, 0x1c, 0x6c00);
+               bnx2_read_phy(bp, 0x1c, &val);
+               bnx2_write_phy(bp, 0x1c, (val & 0x3fd) | 0xec00);
+       }


7) the delays in bnx2_fw_sync(), bnx2_reset_chip() and bnx2_init_chip()
might work better as sleeps, rather than udelay, since you are always
in process context, and its not a performance critical path.

As a general rule for all driver programming, msleep() is preferred over
udelay() / mdelay().  udelay() spins, while msleep() allows other
[possibly high priority real-time] processes to continue.

Note that tg3 needs fixing for this too, but nobody has been motivated
enough to care.


8) excessive stack size in bnx2_alloc_bad_rbuf():

+       u16 good_mbuf[512];

a 1024-byte object is simply unacceptable, given that that is fully 1/4
of a 4K stack, for users that use the 4K stack option.

Typical solution is kmalloc'ing a temporary buffer.


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

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


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?


10.1) [additional review] is the rmb() even needed in bnx2_rx_int(),
since its caller also uses rmb()?


11) Don't use IRQ_RETVAL(1), use IRQ_NONE / IRQ_HANDLED for the constant
cases.


12) use a named constant rather than magic number (aka numeric constant):

+               if ((len > (bp->dev->mtu + ETH_HLEN)) &&
+                       (htons(skb->protocol) != 0x8100)) {


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;
+                       }


14) is D0 guaranteed after 50 usec?

+               /* No more memory access after this point until
+                * device is brought back to D0.
+                */
+               udelay(50);
+               break;


15) the following loop is just silly.  use mdelay or (preferably)
msleep.

+       if ((CHIP_ID(bp) == CHIP_ID_5706_A0) ||
+           (CHIP_ID(bp) == CHIP_ID_5706_A1)) {
+
+               for (i = 0; i < 500; i++) {
+                       udelay(30);
+               }
+       }


16) [long term; grunt work] it would be nice if somebody would submit
patches to ethtool, for verbose register dump of bnx2 and tg3


17) you may prefer msleep_interruptible() ?

+       for (i = 0; i < 10; i++) {
+               if ((REG_RD(bp, BNX2_PCICFG_INT_ACK_CMD) & 0xffff) !=
+                       status_idx) {
+
+                       break;
+               }
+
+               current->state = TASK_INTERRUPTIBLE;
+               schedule_timeout(1);
+       }



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

+       if ((CHIP_ID(bp) != CHIP_ID_5706_A0) &&
+               (CHIP_ID(bp) != CHIP_ID_5706_A1) &&
+               !disable_msi) {
+
+               if (pci_enable_msi(bp->pdev) == 0) {
+                       bp->flags |= USING_MSI_FLAG;
+                       rc = request_irq(bp->pdev->irq, bnx2_msi, 0, dev->name,
+                                       dev);
+               }
+               else {
+                       rc = request_irq(bp->pdev->irq, bnx2_interrupt,
+                                       SA_SHIRQ, dev->name, dev);
+               }
+       }
+       else {
+               rc = request_irq(bp->pdev->irq, bnx2_interrupt, SA_SHIRQ,
+                               dev->name, dev);
+       }



19) [additional review] need flush_scheduled_work(), if using work queues?


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


21) need to call bnx2_netif_stop() in bnx2_close()


22) [namespace] s/ETH_NUM_STATS/BNX2_NUM_STATS/


23) use msleep_interruptible() in bnx2_phys_id()

+               current->state = TASK_INTERRUPTIBLE;
+               schedule_timeout(HZ / 2);
+               if (signal_pending(current))
+                       break;


24) consider using pci_enable_msi() in bnx2_init_board(), rather than
the request_irq() callsite.


25) don't assign dev->mem_{start,end}, it's pointless.  Some drivers
use it to pass _information_ these days, even.


26) Note that some PCI buses have speeds other than what is listed
in bnx2_init_board().  Some weirdo platforms even vary the PCI bus
speed (which is legal).


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


28) use SET_ETHTOOL_OPS(), if you are backporting to 2.4.x as item #20
indicates.


29) do not use NETIF_F_SG independent of NETIF_F_xx_CSUM.

+       dev->features |= NETIF_F_SG;
+       if (bp->flags & USING_DAC_FLAG)
+               dev->features |= NETIF_F_HIGHDMA;
+       dev->features |= NETIF_F_IP_CSUM;


30) as David mentioned in a previous email, mark everything 'static':

+int COM_b06FwReleaseMajor = 0x0;
+int COM_b06FwReleaseMinor = 0x0;
+int COM_b06FwReleaseFix = 0x0;
+u32 COM_b06FwStartAddr = 0x080004a0;
+u32 COM_b06FwTextAddr = 0x08000000;
+int COM_b06FwTextLen = 0x4594;
+u32 COM_b06FwDataAddr = 0x080045e0;
+int COM_b06FwDataLen = 0x0;
+u32 COM_b06FwRodataAddr = 0x08004598;
+int COM_b06FwRodataLen = 0x18;
+u32 COM_b06FwBssAddr = 0x08004600;
+int COM_b06FwBssLen = 0x88;
+u32 COM_b06FwSbssAddr = 0x080045e0;
+int COM_b06FwSbssLen = 0x1c;
+u32 COM_b06FwText[(0x4594/4) + 1] = {


31) use arrays, as requested in a previous email:

+       u16 status_tx_quick_consumer_index0;
+       u16 status_tx_quick_consumer_index1;
+       u16 status_tx_quick_consumer_index2;
+       u16 status_tx_quick_consumer_index3;
+       u16 status_rx_quick_consumer_index0;
+       u16 status_rx_quick_consumer_index1;
+       u16 status_rx_quick_consumer_index2;
+       u16 status_rx_quick_consumer_index3;
+       u16 status_rx_quick_consumer_index4;
+       u16 status_rx_quick_consumer_index5;
+       u16 status_rx_quick_consumer_index6;
+       u16 status_rx_quick_consumer_index7;
+       u16 status_rx_quick_consumer_index8;
+       u16 status_rx_quick_consumer_index9;
+       u16 status_rx_quick_consumer_index10;
+       u16 status_rx_quick_consumer_index11;
+       u16 status_rx_quick_consumer_index12;
+       u16 status_rx_quick_consumer_index13;
+       u16 status_rx_quick_consumer_index14;
+       u16 status_rx_quick_consumer_index15;


32) [speling] s/fileds/fields/

 /* End of fileds used in the performance code paths. */


33) [ack]  I ACK the changes to linux/mii.h [I am the maintainer]

Re: A new driver for Broadcom bcm5706

[Michael Chan]

On Fri, 2005-05-20 at 17:09 -0400, Jeff Garzik wrote:
> On Fri, May 20, 2005 at 02:07:17PM -0700, Ben Greear wrote:
> > I dig having the chipset manufacturer
> > solidly behind the driver, like e1000!
> 
> Clearly you have been asleep :)  Broadcom has been contributing actively
> to tg3.
> 

Yes, we are solidly behind tg3 and bnx2.

Re: A new driver for Broadcom bcm5706

[Jeff Garzik]

Note that I only consider a very few of these items, highlighted below, 
to be merge-stoppers.  The rest are minor things that can be fixed up at 
leisure.

> 8) excessive stack size in bnx2_alloc_bad_rbuf():

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

> 13) [additional review] why is CHECKSUM_UNNECESSARY used when
> cksum==0xffff or cksum==0 ?

> 15) the following loop is just silly.  use mdelay or (preferably)
> msleep.

> 19) [additional review] need flush_scheduled_work(), if using work queues?

> 21) need to call bnx2_netif_stop() in bnx2_close()

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

Re: A new driver for Broadcom bcm5706

[Ben Greear]

Jeff Garzik wrote:
> 
> Note that I only consider a very few of these items, highlighted below, 
> to be merge-stoppers.  The rest are minor things that can be fixed up at 
> leisure.

Out of curiousity:  Is the intended goal to replace tg3 with
this new driver eventually?  I dig having the chipset manufacturer
solidly behind the driver, like e1000!

Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: A new driver for Broadcom bcm5706

[Jeff Garzik]

On Fri, May 20, 2005 at 02:07:17PM -0700, Ben Greear wrote:
> Jeff Garzik wrote:
> >
> >Note that I only consider a very few of these items, highlighted below, 
> >to be merge-stoppers.  The rest are minor things that can be fixed up at 
> >leisure.
> 
> Out of curiousity:  Is the intended goal to replace tg3 with
> this new driver eventually?

No, different hardware.


> I dig having the chipset manufacturer
> solidly behind the driver, like e1000!

Clearly you have been asleep :)  Broadcom has been contributing actively
to tg3.

	Jeff

Re: A new driver for Broadcom bcm5706

[Jeff Garzik]

Michael Chan wrote:
> A new driver bnx2 for Broadcom bcm5706 is available. Since the patch is
> over 500K, I've put it on the ftp server:
> 
> ftp://Net_sys_anon@ftp1.broadcom.com/bnx2-2.patch
> 
> The patch also includes new 1000BASE-X advertisement bit definitions in
> mii.h
> 
> Thanks to David Miller and Jeff Garzik for reviewing and their valuable
> feedback.

For what it's worth, DaveM and I decided that the patch submission 
procedure will be the same for bnx2 and tg3:  DaveM will merge the bnx2 
driver and patches, and send them upstream.

(this deviates from the normal procedure of me merging all net driver 
changes)

	Jeff

Re: A new driver for Broadcom bcm5706

[Ben Greear]

Michael Chan wrote:
> On Fri, 2005-05-20 at 17:09 -0400, Jeff Garzik wrote:
> 
>>On Fri, May 20, 2005 at 02:07:17PM -0700, Ben Greear wrote:
>>
>>>I dig having the chipset manufacturer
>>>solidly behind the driver, like e1000!
>>
>>Clearly you have been asleep :)  Broadcom has been contributing actively
>>to tg3.
>>
> 
> 
> Yes, we are solidly behind tg3 and bnx2.

Excellent...sorry for my sleepiness :)

With regard to tg3, I (with the gracious help of the Intel driver folks)
have added the ability to receive the ethernet checksum on the end of the
skb, as well as tell the e1000 chipset to receive packets with bad checksums,
etc....

Assuming the tg3 chipset can do this, are there documents or code snippets
available to explain how to enable the feature?

I would like to add generic support for this to the ethtool API and allow
drivers to support it as they wish....  Of all my hacks, this one actually
doesn't require new ioctls or /proc file API..so maybe it has a chance of
getting accepted? :)

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: A new driver for Broadcom bcm5706

[David S. Miller]

From: Jeff Garzik <jgarzik@pobox.com>
Date: Fri, 20 May 2005 15:42:20 -0400

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

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

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

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

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

Yeah, same for MSI stuff as well.

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

Re: A new driver for Broadcom bcm5706

[Michael Chan]

On Fri, 2005-05-20 at 15:28 -0700, David S.Miller wrote:
> From: Jeff Garzik <jgarzik@pobox.com>
> Date: Fri, 20 May 2005 15:42:20 -0400
> 
> > 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.

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

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().

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

> 
> > 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.
> 
> > 20) is there any reason to #ifdef BCM_TSO?  2.4.x kernels I suppose?
> 
> Yeah, same for MSI stuff as well.
> 
> > 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?

Re: A new driver for Broadcom bcm5706

[Michael Chan]

On Fri, 2005-05-20 at 20:01 -0400, Jeff Garzik wrote:
> David S.Miller wrote:
> > From: Jeff Garzik <jgarzik@pobox.com>
> > Date: Fri, 20 May 2005 19:30:01 -0400
> > 
> > 
> >>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.
> > 
> > 
> > Sure, both ways are fine.
> 
> huh?  They are pretty different...  one says "Checksum all good, dude" 
> and the other says "I didn't checksum, do it in software for me."
> 
> right?
> 

Yes, if the UDP checksum field in the UDP header is zero - meaning
checksum is not calculated for this packet, the calculated checksum done
by the chip will almost always be something other than 0xffff, and so it
will end up with CHECKSUM_NONE.

Re: A new driver for Broadcom bcm5706

[Jeff Garzik]

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

Re: A new driver for Broadcom bcm5706

[David S. Miller]

From: Jeff Garzik <jgarzik@pobox.com>
Date: Fri, 20 May 2005 19:30:01 -0400

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

Sure, both ways are fine.

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

CHECKSUM_HW is for a different calculation than what 5706 and tg3 are
providing here.  CHECKSUM_HW is for when the chip provides a raw 2's
complement 16-bit sum starting at a fixed offset from the beginning of
the packet.  The network stack then "undoes" or subtracts the header
2's complement checksum from the device provided sum to arrive at the
real checksum result.

5706 and tg3 are actually interpreting the headers and running the
checksum algorithm over the proper parts of the packet headers.

Re: A new driver for Broadcom bcm5706

[Jeff Garzik]

David S.Miller wrote:
> From: Jeff Garzik <jgarzik@pobox.com>
> Date: Fri, 20 May 2005 19:30:01 -0400
> 
> 
>>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.
> 
> 
> Sure, both ways are fine.

huh?  They are pretty different...  one says "Checksum all good, dude" 
and the other says "I didn't checksum, do it in software for me."

right?

	Jeff

Re: A new driver for Broadcom bcm5706

[David S. Miller]

From: Jeff Garzik <jgarzik@pobox.com>
Subject: Re: A new driver for Broadcom bcm5706
Date: Fri, 20 May 2005 20:01:23 -0400

> David S.Miller wrote:
> > From: Jeff Garzik <jgarzik@pobox.com>
> > Date: Fri, 20 May 2005 19:30:01 -0400
> > 
> > 
> >>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.
> > 
> > 
> > Sure, both ways are fine.
> 
> huh?  They are pretty different...  one says "Checksum all good, dude" 
> and the other says "I didn't checksum, do it in software for me."
> 
> right?

0x0000 is the UDP "no checksum" case, so if we say "in software"
for that UDP will just let it pass through still, so the effect
is that same.

Re: A new driver for Broadcom bcm5706

[David S. Miller]

From: "Michael Chan" <mchan@broadcom.com>
Date: Fri, 20 May 2005 16:04:21 -0700

> On Fri, 2005-05-20 at 15:28 -0700, David S.Miller wrote:
> 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?

Please send incremental patches.  That also makes the
changes easier to review as well.

Re: A new driver for Broadcom bcm5706

[David S. Miller]

From: "Michael Chan" <mchan@broadcom.com>
Date: Fri, 20 May 2005 16:04:21 -0700

> On Fri, 2005-05-20 at 15:28 -0700, David S.Miller wrote:
> > From: Jeff Garzik <jgarzik@pobox.com>
> > Date: Fri, 20 May 2005 15:42:20 -0400
> > 
> > > 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.

Just keep it in for now then.  I wouldn't be surprised if some 2.4.x
kernels let this happen.

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

Ok, thanks for verifying.

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

I think the current timer stuff is fine, at least for now.

Re: A new driver for Broadcom bcm5706

[Christoph Hellwig]

On Fri, May 20, 2005 at 10:15:29AM -0700, Michael Chan wrote:
> A new driver bnx2 for Broadcom bcm5706 is available. Since the patch is
> over 500K, I've put it on the ftp server:
> 
> ftp://Net_sys_anon@ftp1.broadcom.com/bnx2-2.patch
> 
> The patch also includes new 1000BASE-X advertisement bit definitions in
> mii.h

These defintions overlap older 10MB/s defintions.  I don't think the number
space is scare enough to need this hack.  If we absolutely want to keep it
you should at least add some big comments explaining it.

Re: A new driver for Broadcom bcm5706

[Michael Chan]

On Fri, 2005-05-27 at 08:41 +0100, Christoph Hellwig wrote:
> On Fri, May 20, 2005 at 10:15:29AM -0700, Michael Chan wrote:
> > A new driver bnx2 for Broadcom bcm5706 is available. Since the patch is
> > over 500K, I've put it on the ftp server:
> > 
> > ftp://Net_sys_anon@ftp1.broadcom.com/bnx2-2.patch
> > 
> > The patch also includes new 1000BASE-X advertisement bit definitions in
> > mii.h
> 
> These defintions overlap older 10MB/s defintions.  I don't think the number
> space is scare enough to need this hack.  If we absolutely want to keep it
> you should at least add some big comments explaining it.
> 
> 
Yes they do. But these overlapping bit definitions are defined by the
802.3 standard for 1000Base-X. These are not Broadcom proprietary
definitions.

Re: A new driver for Broadcom bcm5706

[Christoph Hellwig]

On Fri, May 27, 2005 at 08:58:23AM -0700, Michael Chan wrote:
> > These defintions overlap older 10MB/s defintions.  I don't think the number
> > space is scare enough to need this hack.  If we absolutely want to keep it
> > you should at least add some big comments explaining it.
> > 
> > 
> Yes they do. But these overlapping bit definitions are defined by the
> 802.3 standard for 1000Base-X. These are not Broadcom proprietary
> definitions.

Ok, makes a lot of sense.  Now just mention that in a nice comment in
ethtool.h :)