New driver for Broadcom 10Gb Ethernet, take two.

Message-ID: <[email protected]>
Date: Mon, 08 Oct 2007 14:34:45 +0200
From: Eliezer Tamir <[email protected]>
User-Agent: Thunderbird 2.0.0.6 (X11/20070728)
MIME-Version: 1.0
To: "[email protected]" <[email protected]> (...)
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]

Please consider applying to 2.6.24

Thanks
Eliezer

Quoted 10 lines · Eliezer Tamir

Message-ID: <[email protected]>
Date: Mon, 08 Oct 2007 14:34:45 +0200
From: Eliezer Tamir <[email protected]>
User-Agent: Thunderbird 2.0.0.6 (X11/20070728)
MIME-Version: 1.0
To: "[email protected]" <[email protected]> (...)
Subject: [BNX2X RESUBMIT][PATCH 0/8] New driver for Broadcom 10Gb
Ethernet, take two.
X-WSS-ID: 6B14FC184VK13380170-01-01
Content-Type: text/plain;

Quoted 5 lines

	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.

[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?

Quoted 4 lines · Stephen Hemminger

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

Quoted 2 lines

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

Quoted 3 lines

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

Quoted 7 lines · On Mon, 08 Oct 2007 20:34:41 +0200 "Eliezer" <[email protected]> wrote:

Quoted 4 lines · Stephen Hemminger

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

Quoted 5 lines

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)

(quoted diff) +20 -0

+{
+	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

Quoted 4 lines

Quoted 3 lines

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

Quoted 6 lines

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

Quoted 10 lines

Quoted 4 lines

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

Quoted 6 lines

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.

Quoted 3 lines · On Mon, 08 Oct 2007 20:34:41 +0200 "Eliezer" <[email protected]> wrote:

Quoted 2 lines

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

Quoted 6 lines

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

Quoted 6 lines · Stephen Hemminger

Quoted 3 lines · On Mon, 08 Oct 2007 20:34:41 +0200 "Eliezer" <[email protected]> wrote:

Quoted 2 lines

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

Quoted 9 lines

Quoted 6 lines

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.

Quoted 17 lines · On Mon, 08 Oct 2007 14:40:22 -0700 "Michael Chan" <[email protected]> wrote:

Quoted 6 lines · Stephen Hemminger

Quoted 3 lines · On Mon, 08 Oct 2007 20:34:41 +0200 "Eliezer" <[email protected]> wrote:

Quoted 2 lines

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

Quoted 9 lines

Quoted 6 lines

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

Quoted 8 lines

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.