[BNX2X RESUBMIT][PATCH 0/8] New driver for Broadcom 10Gb Ethernet, take two.

[BNX2X RESUBMIT][PATCH 0/8] New driver for Broadcom 10Gb Ethernet, take two.

[Eliezer Tamir]

[resubmitting, this time without line breaks, sorry]

This is an initial version of the BNX2X, the Linux driver for the
BCM5771X 10Gb Ethernet controller family.
 
Although the chip is very different from the 5706-8 family we based the
driver code on the BNX2 driver.
 
Since the hardware is supposed to be generally available soon I have
posted an initial version and after hearing all the comments I am 
reposting with changes to address them.
 
Some planned feature are still under development, but we want to get
whatever we have out now so people can start using the HW.

Main changes from first version.

* Fixed most issues raised by Michael Buesch. (Thanks Michael!)
* Some slow path bug fixes.
* Yitchak Gertner re-grouped the code in a more logical manner.
* A lot of work was done to get the generated code to comply with coding 
style requirements.

Known issues/TODO.
* Move slowpath event handling from tasklet to workqueue context. This 
will allow replacing the busy waits in the link management code with sleeps.

Please consider applying to 2.6.24

Thanks
Eliezer

Re: [BNX2X RESUBMIT][PATCH 0/8] New driver for Broadcom 10Gb Ethernet, take two.

[Matti Aarnio]

On Mon, Oct 08, 2007 at 02:34:45PM +0200, Eliezer Tamir wrote:
> Message-ID: <470A23E5.6030102@broadcom.com>
> Date: Mon, 08 Oct 2007 14:34:45 +0200
> From: Eliezer Tamir <eliezert@broadcom.com>
> User-Agent: Thunderbird 2.0.0.6 (X11/20070728)
> MIME-Version: 1.0
> To: "netdev@vger.kernel.org" <netdev@vger.kernel.org> (...)
> Subject: [BNX2X RESUBMIT][PATCH 0/8] New driver for Broadcom 10Gb
> 	Ethernet, take two.
> X-WSS-ID: 6B14FC184VK13380170-01-01
> Content-Type: text/plain;
> 	charset=utf-8;
> 	format=flowed
> Content-Transfer-Encoding: 7bit
> 
> [resubmitting, this time without line breaks, sorry]

I am sorry, but you had no success in this round either.

Thunderbird is "somewhat"(*) difficult when you try to include the source
(diff-) file into message body text.

(*: like "serious pain in the rear section..)


The classical instruction is that "thy shall post source as diffs against
baseline in email message textbody"  ---  but now your only real choice is
in between using genuine attachments, and placing it into some public
downloadable file, which the rest of the world can download - or to use
the ATTACH -button.

Two failures with preferred method is (IMO) a good excuse to resort on
MIME attachments.

....
> Please consider applying to 2.6.24
>
> Thanks
> Eliezer

Matti Aarnio

Re: [BNX2X RESUBMIT][PATCH 0/8] New driver for Broadcom 10Gb Ethernet, take two.

[Randy Dunlap]

On Mon, 8 Oct 2007 16:02:48 +0300 Matti Aarnio wrote:

> On Mon, Oct 08, 2007 at 02:34:45PM +0200, Eliezer Tamir wrote:
> > Message-ID: <470A23E5.6030102@broadcom.com>
> > Date: Mon, 08 Oct 2007 14:34:45 +0200
> > From: Eliezer Tamir <eliezert@broadcom.com>
> > User-Agent: Thunderbird 2.0.0.6 (X11/20070728)
> > MIME-Version: 1.0
> > To: "netdev@vger.kernel.org" <netdev@vger.kernel.org> (...)
> > Subject: [BNX2X RESUBMIT][PATCH 0/8] New driver for Broadcom 10Gb
> > 	Ethernet, take two.
> > X-WSS-ID: 6B14FC184VK13380170-01-01
> > Content-Type: text/plain;
> > 	charset=utf-8;
> > 	format=flowed
> > Content-Transfer-Encoding: 7bit
> > 
> > [resubmitting, this time without line breaks, sorry]
> 
> I am sorry, but you had no success in this round either.
> 
> Thunderbird is "somewhat"(*) difficult when you try to include the source
> (diff-) file into message body text.
> 
> (*: like "serious pain in the rear section..)
> 
> 
> The classical instruction is that "thy shall post source as diffs against
> baseline in email message textbody"  ---  but now your only real choice is
> in between using genuine attachments, and placing it into some public
> downloadable file, which the rest of the world can download - or to use
> the ATTACH -button.
> 
> Two failures with preferred method is (IMO) a good excuse to resort on
> MIME attachments.

In tbird v1.5x at least, the External Editor option works fine for me
to send patches inline.

---
~Randy

Re: [BNX2X RESUBMIT][PATCH 0/8] New driver for Broadcom 10Gb Ethernet, take two.

[Stephen Hemminger]

On Mon, 08 Oct 2007 14:34:45 +0200
"Eliezer Tamir" <eliezert@broadcom.com> wrote:

> [resubmitting, this time without line breaks, sorry]
> 
> This is an initial version of the BNX2X, the Linux driver for the
> BCM5771X 10Gb Ethernet controller family.
>  
> Although the chip is very different from the 5706-8 family we based the
> driver code on the BNX2 driver.
>  
> Since the hardware is supposed to be generally available soon I have
> posted an initial version and after hearing all the comments I am 
> reposting with changes to address them.
>  
> Some planned feature are still under development, but we want to get
> whatever we have out now so people can start using the HW.
> 
> Main changes from first version.
> 
> * Fixed most issues raised by Michael Buesch. (Thanks Michael!)
> * Some slow path bug fixes.
> * Yitchak Gertner re-grouped the code in a more logical manner.
> * A lot of work was done to get the generated code to comply with coding 
> style requirements.
> 
> Known issues/TODO.
> * Move slowpath event handling from tasklet to workqueue context. This 
> will allow replacing the busy waits in the link management code with sleeps.
> 
> Please consider applying to 2.6.24
> 
> Thanks
> Eliezer


Looks good.  Some minor stuff:
* You can use network device stats in network device structure and
  no longer need the copy in bp
* The MACRO's for 64 bit stats look like they could be done with 
  u64 and/or turned into inline's.
* RCS/CVS tags for date and version # are kind of ugly. They git system
  doesn't use them, perhaps you just want to track your internal version
  control information, but what happens when changes come from outside?


-- 
Stephen Hemminger <shemminger@linux-foundation.org>

Re: [BNX2X RESUBMIT][PATCH 0/8] New driver for Broadcom 10Gb Ethernet, take two.

[Eliezer]

On Mon, 2007-10-08 at 10:29 -0700, Stephen Hemminger wrote:

> 
> Looks good.  Some minor stuff:
> * You can use network device stats in network device structure and
>   no longer need the copy in bp

We can't use net device stats in the bp because the elements are also
used by HW and we can't rearrange them. 
struct bmac_stats and struct emac_stats are written to by the HW 
struct nig_stats and struct bnx2x_eth_stats are read by HW
(I would have loved to get rid of at least one of these long structs.)


> * The MACRO's for 64 bit stats look like they could be done with 
>   u64 and/or turned into inline's.

The MACRO's modify some of their arguments, plus they need to work on 32
bit machines (are 64 bit counters always available on 32 bit machines?).
so using an inline would allow the inline to be more readable but
calling it would get ugly.
I'm open to suggestions.

> * RCS/CVS tags for date and version # are kind of ugly. They git system
>   doesn't use them, perhaps you just want to track your internal version
>   control information, but what happens when changes come from outside?

We need some way to correlate cvs to opensource to bug tracking tool.
Note that driver and Microcode code are very tightly coupled on this
device, so if I want to debug a microcode related problem I would need
both CVS version and microcode version.

I agree that tags are ugly but can't think of a better way to do it.
Changes coming from the outside are a problem, but using the cvs version
as an anchor I can track them using git.
Again, I'm open to suggestions and if someone can think of an elegant
way of doing it, I will gladly use it.

thanks
Eliezer

Re: [BNX2X RESUBMIT][PATCH 0/8] New driver for Broadcom 10Gb Ethernet, take two.

[Stephen Hemminger]

On Mon, 08 Oct 2007 20:34:41 +0200
"Eliezer" <eliezert@broadcom.com> wrote:

> On Mon, 2007-10-08 at 10:29 -0700, Stephen Hemminger wrote:
> 
> > 
> > Looks good.  Some minor stuff:
> > * You can use network device stats in network device structure and
> >   no longer need the copy in bp
> 
> We can't use net device stats in the bp because the elements are also
> used by HW and we can't rearrange them. 
> struct bmac_stats and struct emac_stats are written to by the HW 
> struct nig_stats and struct bnx2x_eth_stats are read by HW
> (I would have loved to get rid of at least one of these long structs.)

Here:
static void bnx2x_update_net_stats(struct bnx2x *bp)
+{
+	struct bnx2x_eth_stats *estats = bnx2x_sp(bp, eth_stats);
+
+	bp->net_stats.rx_packets =
+		bnx2x_hilo(&estats->total_unicast_packets_received_hi) +
+		bnx2x_hilo(&estats->total_multicast_packets_received_hi) +
+		bnx2x_hilo(&estats->total_broadcast_packets_received_hi);
+
+	bp->net_stats.tx_packets =
+		bnx2x_hilo(&estats->total_unicast_packets_transmitted_hi) +
+		bnx2x_hilo(&estats->total_multicast_packets_transmitted_hi) +
+		bnx2x_hilo(&estats->total_broadcast_packets_transmitted_hi);
+
+	bp->net_stats.rx_bytes = bnx2x_hilo(&estats->total_bytes_received_hi);
+
+	bp->net_stats.tx_bytes =
+		bnx2x_hilo(&estats->total_bytes_transmitted_hi);
+
+	bp->net_stats.rx_dropped = estats->no_buff_discard;
+	bp->net_stats.tx_dropped = 0;

bp->net_stats could be replaced by dev->net_stats


> > * The MACRO's for 64 bit stats look like they could be done with 
> >   u64 and/or turned into inline's.
> 
> The MACRO's modify some of their arguments, plus they need to work on 32
> bit machines (are 64 bit counters always available on 32 bit machines?).
> so using an inline would allow the inline to be more readable but
> calling it would get ugly.
> I'm open to suggestions.
> 

u64 exists on all platforms (including 32 bit).

> > * RCS/CVS tags for date and version # are kind of ugly. They git system
> >   doesn't use them, perhaps you just want to track your internal version
> >   control information, but what happens when changes come from outside?
> 
> We need some way to correlate cvs to opensource to bug tracking tool.
> Note that driver and Microcode code are very tightly coupled on this
> device, so if I want to debug a microcode related problem I would need
> both CVS version and microcode version.
> 
> I agree that tags are ugly but can't think of a better way to do it.
> Changes coming from the outside are a problem, but using the cvs version
> as an anchor I can track them using git.
> Again, I'm open to suggestions and if someone can think of an elegant
> way of doing it, I will gladly use it.
> 

Leave them for now, but be careful about relying too strongly on
the version having any real meaning.

-- 
Stephen Hemminger <shemminger@linux-foundation.org>

Re: [BNX2X RESUBMIT][PATCH 0/8] New driver for Broadcom 10Gb Ethernet, take two.

[Eliezer Tamir]

On Mon, 2007-10-08 at 12:08 -0700, Stephen Hemminger wrote:
> On Mon, 08 Oct 2007 20:34:41 +0200
> "Eliezer" <eliezert@broadcom.com> wrote:
> 
> > On Mon, 2007-10-08 at 10:29 -0700, Stephen Hemminger wrote:
> > 
> > > 
> > > Looks good.  Some minor stuff:
> > > * You can use network device stats in network device structure and
> > >   no longer need the copy in bp
> > 
> > We can't use net device stats in the bp because the elements are also
> > used by HW and we can't rearrange them. 
> > struct bmac_stats and struct emac_stats are written to by the HW 
> > struct nig_stats and struct bnx2x_eth_stats are read by HW
> > (I would have loved to get rid of at least one of these long structs.)
> 
> Here:
> static void bnx2x_update_net_stats(struct bnx2x *bp)
> +{
> +	struct bnx2x_eth_stats *estats = bnx2x_sp(bp, eth_stats);
> +
> +	bp->net_stats.rx_packets =
> +		bnx2x_hilo(&estats->total_unicast_packets_received_hi) +
> +		bnx2x_hilo(&estats->total_multicast_packets_received_hi) +
> +		bnx2x_hilo(&estats->total_broadcast_packets_received_hi);
> +
> +	bp->net_stats.tx_packets =
> +		bnx2x_hilo(&estats->total_unicast_packets_transmitted_hi) +
> +		bnx2x_hilo(&estats->total_multicast_packets_transmitted_hi) +
> +		bnx2x_hilo(&estats->total_broadcast_packets_transmitted_hi);
> +
> +	bp->net_stats.rx_bytes = bnx2x_hilo(&estats->total_bytes_received_hi);
> +
> +	bp->net_stats.tx_bytes =
> +		bnx2x_hilo(&estats->total_bytes_transmitted_hi);
> +
> +	bp->net_stats.rx_dropped = estats->no_buff_discard;
> +	bp->net_stats.tx_dropped = 0;
> 
> bp->net_stats could be replaced by dev->net_stats
> 
OK, I will do that on the next version.

> 
> > > * The MACRO's for 64 bit stats look like they could be done with 
> > >   u64 and/or turned into inline's.
> > 
> > The MACRO's modify some of their arguments, plus they need to work on 32
> > bit machines (are 64 bit counters always available on 32 bit machines?).
> > so using an inline would allow the inline to be more readable but
> > calling it would get ugly.
> > I'm open to suggestions.
> > 
> 
> u64 exists on all platforms (including 32 bit).
I will see if I can use that to simplify the MACROs.

> 
> > > * RCS/CVS tags for date and version # are kind of ugly. They git system
> > >   doesn't use them, perhaps you just want to track your internal version
> > >   control information, but what happens when changes come from outside?
> > 
> > We need some way to correlate cvs to opensource to bug tracking tool.
> > Note that driver and Microcode code are very tightly coupled on this
> > device, so if I want to debug a microcode related problem I would need
> > both CVS version and microcode version.
> > 
> > I agree that tags are ugly but can't think of a better way to do it.
> > Changes coming from the outside are a problem, but using the cvs version
> > as an anchor I can track them using git.
> > Again, I'm open to suggestions and if someone can think of an elegant
> > way of doing it, I will gladly use it.
> > 
> 
> Leave them for now, but be careful about relying too strongly on
> the version having any real meaning.
> 

Re: [BNX2X RESUBMIT][PATCH 0/8] New driver for Broadcom 10Gb Ethernet, take two.

[Michael Chan]

On Mon, 2007-10-08 at 12:08 -0700, Stephen Hemminger wrote:
> On Mon, 08 Oct 2007 20:34:41 +0200
> "Eliezer" <eliezert@broadcom.com> wrote:
> 
> > > * The MACRO's for 64 bit stats look like they could be done with 
> > >   u64 and/or turned into inline's.
> > 
> > The MACRO's modify some of their arguments, plus they need to work on 32
> > bit machines (are 64 bit counters always available on 32 bit machines?).
> > so using an inline would allow the inline to be more readable but
> > calling it would get ugly.
> > I'm open to suggestions.
> > 
> 
> u64 exists on all platforms (including 32 bit).
> 

I think the biggest problem with these 64-bits counters (and 64-bit
addresses) is that the hardware treats them as big endian and they get
DMA'ed in big endian format.  We control the byte swap so that 32-bit
quantities will have the correct endianness, but the high and low 32-bit
words will be in the wrong spots on little endian machines.  That's why
we need to separate the high and the low words and convert them back and
forth.

Re: [BNX2X RESUBMIT][PATCH 0/8] New driver for Broadcom 10Gb Ethernet, take two.

[Stephen Hemminger]

On Mon, 08 Oct 2007 14:40:22 -0700
"Michael Chan" <mchan@broadcom.com> wrote:

> On Mon, 2007-10-08 at 12:08 -0700, Stephen Hemminger wrote:
> > On Mon, 08 Oct 2007 20:34:41 +0200
> > "Eliezer" <eliezert@broadcom.com> wrote:
> > 
> > > > * The MACRO's for 64 bit stats look like they could be done with 
> > > >   u64 and/or turned into inline's.
> > > 
> > > The MACRO's modify some of their arguments, plus they need to work on 32
> > > bit machines (are 64 bit counters always available on 32 bit machines?).
> > > so using an inline would allow the inline to be more readable but
> > > calling it would get ugly.
> > > I'm open to suggestions.
> > > 
> > 
> > u64 exists on all platforms (including 32 bit).
> > 
> 
> I think the biggest problem with these 64-bits counters (and 64-bit
> addresses) is that the hardware treats them as big endian and they get
> DMA'ed in big endian format.  We control the byte swap so that 32-bit
> quantities will have the correct endianness, but the high and low 32-bit
> words will be in the wrong spots on little endian machines.  That's why
> we need to separate the high and the low words and convert them back and
> forth.
>
There are types and tools for checking endianness see be64, etc.

-- 
Stephen Hemminger <shemminger@linux-foundation.org>

Re: [BNX2X RESUBMIT][PATCH 0/8] New driver for Broadcom 10Gb Ethernet, take two.

[David Miller]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Mon, 8 Oct 2007 14:03:01 -0700

> On Mon, 08 Oct 2007 14:40:22 -0700
> "Michael Chan" <mchan@broadcom.com> wrote:
> 
> > On Mon, 2007-10-08 at 12:08 -0700, Stephen Hemminger wrote:
> > > On Mon, 08 Oct 2007 20:34:41 +0200
> > > "Eliezer" <eliezert@broadcom.com> wrote:
> > > 
> > > > > * The MACRO's for 64 bit stats look like they could be done with 
> > > > >   u64 and/or turned into inline's.
> > > > 
> > > > The MACRO's modify some of their arguments, plus they need to work on 32
> > > > bit machines (are 64 bit counters always available on 32 bit machines?).
> > > > so using an inline would allow the inline to be more readable but
> > > > calling it would get ugly.
> > > > I'm open to suggestions.
> > > > 
> > > 
> > > u64 exists on all platforms (including 32 bit).
> > > 
> > 
> > I think the biggest problem with these 64-bits counters (and 64-bit
> > addresses) is that the hardware treats them as big endian and they get
> > DMA'ed in big endian format.  We control the byte swap so that 32-bit
> > quantities will have the correct endianness, but the high and low 32-bit
> > words will be in the wrong spots on little endian machines.  That's why
> > we need to separate the high and the low words and convert them back and
> > forth.
> >
> There are types and tools for checking endianness see be64, etc.

It's not a be64 Stephen, Michael is trying to explain this.

They configure the hardware to swap the bytes within a 32-bit word
into cpu endianness, but the chip doesn't swap the 32-bit words within
a 64-bit word properly, which is why the macros are necessary.