Patch submission for S2io Xframe driver to 2.6 kernel

Hi Jeff et al,
We gained immensely from your code review comments last time when we submitted the S2io driver.
Attached is a patch(use with -p1 option from /usr/src/<kernel_name> directory) on the current driver
in the 2.6 kernel. Please review the code changes and reply with your valuable comments.

Briefly, some of the main features in this patch:

1. Support for IBM PowerPC platform.
2. Support for NAPI.
3. More efficient allocation of Transmit blocks.
4. Multi-buffer mode for improved Receive performance.
4. Following bug fixes:
a. Race condition when trying to down interface during traffic. b. Fix for bringup problems with SuSE9 on IA64 system.

Thanks,
Ravi

Hi all,
Apologies for the wide distribution, hopefully this is the right forum...

We are finalizing design for the next version of our 10GbE ASIC; in a nutshell the device supports both TOE/RDMA interface as well as standard Ethernet interface with number of "Stateless" TCP assists like TSO, offload for large UDP segments, etc. (The non-offload path will be software compatible with the current Xframe 10GbE NIC).

We are going to further enhance the latter "partial offload" (aka "stateless offload", aka "fast path acceleration" :-)) interface by adding some additional features.

As an example, some time ago Andi put forward an idea of socket demultiplex in hardware, to improve receive side performance.
This is already being designed in, and we are willing to consider other TCP hardware assists as well.

I'm looking for volunteers who would collaborate with us on this project and implement kernel support for the socket demultiplex (and possibly other assists), with S2io providing the necessary driver support and access to the (likely, remote) 10GbE hardware platform.

Most if not all the work could be done on a current S2io 10GbE hardware; main limitation for now will be the number of sockets since shipping NIC has only 8 independent rx hw queues but the next version will support tens of thousands.

Please contact me if there is an interest in the project. I'm not sure about the legal framework yet, access to our hw specs may require an NDA but the driver code will be released under GPL of course, much like the current S2io Linux driver.

Regards, Leonid

Leonid Grossman
Software Development
www.s2io.com

On Fri, 13 Aug 2004 17:19:36 -0700 Ravinandan Arakali wrote:

| Hi Jeff et al,
| We gained immensely from your code review comments last time when we
| submitted the S2io driver.
| Attached is a patch(use with -p1 option from /usr/src/<kernel_name>
| directory) on the current driver
| in the 2.6 kernel. Please review the code changes and reply with your
| valuable comments.
| 
| Briefly, some of the main features in this patch:
| 
| 1. Support for IBM PowerPC platform.
| 2. Support for NAPI.
| 3. More efficient allocation of Transmit blocks.
| 4. Multi-buffer mode for improved Receive performance.
| 4. Following bug fixes:
|    a. Race condition when trying to down interface during traffic.
|    b. Fix for bringup problems with SuSE9 on IA64 system.

for s2io.c::::::::::

0. Renames a bunch of functions. It would be helpful (for patch reviewers) to separate this into functional patch sets. That way, things like function renames (and kerneldoc style function comment blocks -- which are much better, thanks -- wouldn't get in the way of reviewing the meat of the patch.

a. It needs some fixups for coding style.
Current s2io.c has these right, but the patch messes them up:

use a space after "if" and "for", not "if(" or "for("

b. This code fragment (done 2 times):

+ int tmp = config->TxCfg[i].FifoLen / lst_per_page;
+ int tmp1 = config->TxCfg[i].FifoLen % lst_per_page;
+ int page_num = tmp1? (tmp+1) : tmp;

looks like a familiar idiom to me:

#define container_index(len, per_each) ((len + per_each - 1) / per_each)

		page_num = container_index(config->TxCfg[i].FifoLen, lst_per_page);

c. in this code fragment:

+ int k = 0;
+ dma_addr_t tmp_p;
+ void *tmp_v;
+ tmp_v = pci_alloc_consistent(nic->pdev,
+ PAGE_SIZE, &tmp_p);
+ if(!tmp_v) {
+ DBG_PRINT(ERR_DBG,"Malloc failed for TxDL\n");
Using "Malloc" here would be confusing to me, but it's DBG_PRINT(), so....

+ return -ENOMEM;
+ }
+ memset(tmp_v, 0, PAGE_SIZE);
memset() is done by the alloc call.

d. long comment format in Linux is:

/*
* begin here
* more comment
* still more
* end here
*/

e. s/chachelines/cachelines/ (or /cache lines/)

f. There are several function description blocks where the function name in the comment block does not match the actual coded function name.

g. check the return type and comment:

+ * Return value :
+ * void.
*/
+
int s2io_ethtool_gset(

h. read_eeprom():
+ * Return value:
+ * -1 on failure and the value read from the Eeprom if successful.

so read_eeprom() doesn't distinguish between reading 0xff ff ff ff and an error?

i. write_eeprom():
probably returns 0 on success, not '0':
+ * Return value:
+ * '0' on success, -1 on failure.
Just remove the ' marks. (in multiple places)

j. Please use tabs instead of spaces (e.g., 8 spaces at beginning of some lines).

k. in check parameter values:

+ if ((shared_splits > 31)) {
+ printk("shared_splits can exceed 31\n");
? cannot ??

+ if (rmac_util_period > 0xF) {
+ printk("rmac_util_period can exceed 15\n");
? cannot ??

+ if (tmac_util_period > 0xF) {
+ printk("tmac_util_period can exceed 15\n");
? cannot ??

for s2io.h::::::::::::::::;

1. It would be nice (desirable, preferred) to remove all of those typedefs, similar to the way that you changed FuncTionNames and comments.

2. +#define SIZE_OF_BLOCK 4096

Is this size strictly device specific, and not architecture related?

--
~Randy

diff -Naur orig/drivers/net/s2io.c new/drivers/net/s2io.c

[...]
@@ -432,14 +507,29 @@
mac_control = &nic->mac_control;
config = &nic->config;

- if (mac_control->txd_list_mem) {
- pci_free_consistent(nic->pdev,

-				    mac_control->txd_list_mem_sz,
-				    mac_control->txd_list_mem,
-				    mac_control->txd_list_mem_phy);

+ lst_size = (sizeof(TxD_t) * config->MaxTxDs);
+ lst_per_page = PAGE_SIZE / lst_size;
+
+ for(i=0; i<config->TxFIFONum; i++) {
+ int tmp = config->TxCfg[i].FifoLen / lst_per_page;
+ int tmp1 = config->TxCfg[i].FifoLen % lst_per_page;
+ int page_num = tmp1?(tmp+1) : tmp;
+ for(j=0; j<page_num; j++) {
+ int mem_blks = (j * lst_per_page);
+ if(!nic->list_info[i][mem_blks].list_virt_addr)
+ break;
+ pci_free_consistent(nic->pdev, PAGE_SIZE,
+ nic->list_info[i][mem_blks].list_virt_addr,
+ nic->list_info[i][mem_blks].list_phy_addr);
+ }
+ kfree(nic->list_info[i]);
}

-> Give it more room and avoid some arithmetic:

	for (i = 0; i < config->TxFIFONum; i++) {
		int blk, blk_max = config->TxCfg[i].FifoLen;

		for (blk = 0; blk < blk_max; blk += lst_per_page) {
			struct list_info_hold *lih = &nic->list_info[i][blk];

			if (!lih->list_virt_addr)
				break;
			pci_free_consistent(nic->pdev, PAGE_SIZE,
				lih->list_virt_addr, lih->list_phy_addr);
		}
		kfree(nic->list_info[i]);
	}

[...]
@@ -858,22 +963,57 @@
writel((u32) (val64 >> 32), (add + 4));
val64 = readq(&bar0->mac_cfg);

+ /*Set the time value to be inserted in the pause frame generated by xena */
+ val64 = readq(&bar0->rmac_pause_cfg);
+ val64 &= ~(RMAC_PAUSE_HG_PTIME(0xffff));
+ val64 |= RMAC_PAUSE_HG_PTIME(nic->mac_control.rmac_pause_time);
+ writeq(val64, &bar0->rmac_pause_cfg);
+
+ /* Set the Threshold Limit for Generating the pause frame
+ * If the amount of data in any Queue exceeds ratio of
+ * (mac_control.mc_pause_threshold_q0q3 or q4q7)/256
+ * pause frame is generated
+ */
+ val64 = 0;
+ for (i = 0; i < 4; i++) {
+ val64 |=
+ (((u64) 0xFF00 | nic->mac_control.
+ mc_pause_threshold_q0q3)
+ << (i * 2 * 8));
+ }

-> (internal parser panic) keep 'mc_pause_threshold_q0q3' on its containing
struct line ?

-> May be it's not worth it but you can save an iteration:
val64 = (u64) 0xFF00 | nic->mac_control.mc_pause_threshold_q0q3; val64 |= val64 << 16;
val64 |= val64 << 32;

You can wrap the whole thing behind an inlined function if it tends to appear several times.

--
Ueimor

is there a follow-up patch addressing the comments ?

Hi Ravi,
Please try to address the comments in real time, it's probably better to submit a follow-up rather than waiting for more comments... Also - are Koushik and Sriram in Cupertino next week?
Cheers , Leonid

The comments are being addressed. But since Koushik is in transition from Bangalore to Cupertino and the associated travel preparation, there may be some delay in the next submission.
In addition to the comments, we are trying to include additional code required for 32-bit systems. Currently we are targeting middle of next week for the next submission.

Ravi

-----Original Message-----
From: Leonid Grossman [mailto:[email protected]]
Sent: Saturday, August 28, 2004 10:53 PM
To: 'Jeff Garzik'; [email protected]
Cc: [email protected]; [email protected]
Subject: RE: Patch submission for S2io Xframe driver to 2.6 kernel

Hi Ravi,
Please try to address the comments in real time, it's probably better to submit a follow-up rather than waiting for more comments... Also - are Koushik and Sriram in Cupertino next week?
Cheers , Leonid

Hi Jeff,
Attached is the patch with the first round comments incorporated. In addition, this patch contains
Some fixes related to 32-bit systems.
Few fixes related to Rx path in NAPI.

Thanks to all for the comments.

Pls review this patch as well and come back with your comments.

Regards,
Ravi

-----Original Message-----
From: Jeff Garzik [mailto:[email protected]]
Sent: Saturday, August 28, 2004 4:37 PM
To: [email protected]
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: Patch submission for S2io Xframe driver to 2.6 kernel

is there a follow-up patch addressing the comments ?

hmmm... it's a big undescribed patch that doesn't apply anymore :/

drivers/net/Kconfig 1.91 -> 1.92: 2657 lines
1 out of 1 hunk FAILED -- saving rejects to file drivers/net/Kconfig.rej drivers/net/s2io-regs.h 1.1 -> 1.2: 775 lines drivers/net/s2io.c 1.6 -> 1.7: 4393 lines
1 out of 148 hunks FAILED -- saving rejects to file drivers/net/s2io.c.rej drivers/net/s2io.h 1.5 -> 1.6: 854 lines

Can you please split it up into individual changes, or at least smaller chunks?

Jeff

Jeff,
We tried it out with 2.6.7. Can you pls specify the kernel version on which you tried and we will send a patch which works for that version. We will also send a more detailed description of the changes along with that.

Thanks,
Ravi

-----Original Message-----
From: Jeff Garzik [mailto:[email protected]]
Sent: Thursday, September 16, 2004 5:39 PM
To: [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: Patch submission for S2io Xframe driver to 2.6 kernel

hmmm... it's a big undescribed patch that doesn't apply anymore :/

drivers/net/Kconfig 1.91 -> 1.92: 2657 lines
1 out of 1 hunk FAILED -- saving rejects to file drivers/net/Kconfig.rej drivers/net/s2io-regs.h 1.1 -> 1.2: 775 lines drivers/net/s2io.c 1.6 -> 1.7: 4393 lines
1 out of 148 hunks FAILED -- saving rejects to file drivers/net/s2io.c.rej drivers/net/s2io.h 1.5 -> 1.6: 854 lines

Can you please split it up into individual changes, or at least smaller chunks?

Jeff

Always diff against the latest version of the kernel. You can find out the latest version by going to http://www.kernel.org/ You'll typically want the latest snapshot (preferably), or the latest pre-patch.

Standard patch submission format is described at http://linux.yyz.us/patch-format.html and in Documentation/SubmittingPatches.

When submitting a series of patches, it is normal patches to depend on preceding patches.

Jeff

On Thu, 16 Sep 2004 20:52:50 -0400 Jeff Garzik wrote:

| Ravinandan Arakali wrote:
| > Jeff,
| > We tried it out with 2.6.7. Can you pls specify the kernel version
| > on which you tried and we will send a patch which works for that version.
| > We will also send a more detailed description of the changes along with
| > that.

For splitting it up :), e.g.:

- 1 patch could be for renaming only, no code/logic changes (like TxCfg to tx_cfg); maybe including comments/whitespace changes;

- 1 could be to eliminate (most/all) typedefs

- 1 could be for errata/workarounds

- 1 could be for ethtool support (changes)

- 1 for NAPI support (changes)

- 1 for 2BUFF mode

etc...

Q1: why is this change needed?

static inline u64 readq(void *addr)
{
u64 ret = 0;
ret = readl(addr + 4);
- ret <<= 32;
- ret |= readl(addr);
+ (u64) ret <<= 32;
+ (u64) ret |= readl(addr);

| 
| Always diff against the latest version of the kernel.  You can find out 
| the latest version by going to http://www.kernel.org/  You'll typically 
| want the latest snapshot (preferably), or the latest pre-patch.
| 
| Standard patch submission format is described at 
| http://linux.yyz.us/patch-format.html and in 
| Documentation/SubmittingPatches.
| 
| When submitting a series of patches, it is normal patches to depend on 
| preceding patches.

--
~Randy

Hi,
Attached are the updated patches. It is designed to work on the latest kernel. Also, this time the submission is split to 3 patches(they need to applied in the order mentioned below).

s2io_styling_level1 - Contains
a. styling related, function name changes. b. fix for 32-bit systems.
c. modified Transmit descriptor allocation
strategy.
d. miscellaneous fixes.
with_napi_level2 - Fixes/tunes NAPI feature
with_2buff_level3 - Adds support for 2-buffer mode.

Please review and get back with your comments.

Thanks,
Ravi

-----Original Message-----
From: Jeff Garzik [mailto:[email protected]]
Sent: Thursday, September 16, 2004 5:53 PM
To: [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: Patch submission for S2io Xframe driver to 2.6 kernel

Always diff against the latest version of the kernel. You can find out the latest version by going to http://www.kernel.org/ You'll typically want the latest snapshot (preferably), or the latest pre-patch.

Standard patch submission format is described at
http://linux.yyz.us/patch-format.html and in
Documentation/SubmittingPatches.

When submitting a series of patches, it is normal patches to depend on preceding patches.

Jeff

On Mon, 27 Sep 2004 17:42:51 -0700 Ravinandan Arakali wrote:

| Hi,
| Attached are the updated patches. It is designed to work on the latest
| kernel. Also, this time the submission is split to 3 patches(they need
| to applied in the order mentioned below).
| 
| s2io_styling_level1  - Contains
|                              a. styling related, function name changes.
|                              b. fix for 32-bit systems.
|                              c. modified Transmit descriptor allocation
| strategy.
|                              d. miscellaneous fixes.
| with_napi_level2     - Fixes/tunes NAPI feature
| with_2buff_level3    - Adds support for 2-buffer mode.
| 
| Please review and get back with your comments.

The focused nature of the NAPI and 2BUFF patches are good IMO.

It would be very helpful if the styling patch had all comments/whitespace/style changes split out from it (maybe 3000 of the 4000 lines of the patch). Then we (reviewers) could spend time focusing on the important parts of your patches, and the styling patch could go with some lighter review.

I'd like to help Jeff with some review here, but it's become too painful & time-consuming for me to wade thru the patches looking for the more-important parts of such a large patch, so I'll have to leave it to Jeff and other netdev regulars.

| Thanks,
| Ravi
| 
| -----Original Message-----
| From: Jeff Garzik [mailto:[email protected]]
| Sent: Thursday, September 16, 2004 5:53 PM
| To: [email protected]
| Cc: [email protected]; [email protected];
| [email protected]; [email protected]
| Subject: Re: Patch submission for S2io Xframe driver to 2.6 kernel
| 
| 
| Ravinandan Arakali wrote:
| > Jeff,
| > We tried it out with 2.6.7. Can you pls specify the kernel version
| > on which you tried and we will send a patch which works for that version.
| > We will also send a more detailed description of the changes along with
| > that.
| 
| 
| Always diff against the latest version of the kernel.  You can find out
| the latest version by going to http://www.kernel.org/  You'll typically
| want the latest snapshot (preferably), or the latest pre-patch.
| 
| Standard patch submission format is described at
| http://linux.yyz.us/patch-format.html and in
| Documentation/SubmittingPatches.
| 
| When submitting a series of patches, it is normal patches to depend on
| preceding patches.

--
~Randy

When we say "separate logical changes", we mean it :)

This means that your level1 patch should be split into at least 4 patches.

Second, send each patch in a separate email, as described in
http://linux.yyz.us/patch-format.html (and Documentation/SubmittingPatches) rather than sending as a tarball.

Jeff

Hi Jeff, Randy,
We are sorry about that. We will split the first patch into multiple smaller patches and resubmit them one-by-one.
We appreciate all your patience.

Regards,
Ravi

-----Original Message-----
From: Jeff Garzik [mailto:[email protected]]
Sent: Tuesday, September 28, 2004 8:05 AM
To: [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: Patch submission for S2io Xframe driver to 2.6 kernel

When we say "separate logical changes", we mean it :)

This means that your level1 patch should be split into at least 4 patches.

Second, send each patch in a separate email, as described in
http://linux.yyz.us/patch-format.html (and Documentation/SubmittingPatches) rather than sending as a tarball.

Jeff

Hi Jeff and others,
We have now split the submission into 8 patches. Attached is the first patch. It contains mostly cosmetic changes such as style of comments, indentation, structure name changes as per linux convention etc.

Please let us know if you want us to send the subsequent patches one-after-another right now, or do you want us to send them only after you have completed review and accepted this first patch.

Thanks,
Ravi

-----Original Message-----
From: Jeff Garzik [mailto:[email protected]]
Sent: Tuesday, September 28, 2004 8:05 AM
To: [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: Patch submission for S2io Xframe driver to 2.6 kernel

When we say "separate logical changes", we mean it :)

This means that your level1 patch should be split into at least 4 patches.

Second, send each patch in a separate email, as described in
http://linux.yyz.us/patch-format.html (and Documentation/SubmittingPatches) rather than sending as a tarball.

Jeff

Please send all patches at once, as described in the URLs I mentioned previously.

Example:

[PATCH 2.6.9-rc3 1/8] whitespace changes [PATCH 2.6.9-rc3 2/8] fix 32bit issue [PATCH 2.6.9-rc3 3/8] add a feature [PATCH 2.6.9-rc3 4/8] etc...

and I will review and apply in the order that you specify.

Jeff

Jeff Garzik <[email protected]> :
[...]

<nit>
[PATCH 2.6.9-rc3 1/8] S2io: whitespace changes [PATCH 2.6.9-rc3 2/8] S2io: fix 32bit issue [PATCH 2.6.9-rc3 3/8] S2io: add a feature [PATCH 2.6.9-rc3 4/8] S2io: etc... </nit>

--
Ueimor

Yes, thanks for the correction :)

Jeff

Hi,
I'm resending the first patch which has cosmetic changes such as indentation, change in comment styles, variable name changes. I'll follow up with subsequent patches.

Thanks,
Ravi

On Mon, 13 Sep 2004 10:09:53 -0700 Ravinandan Arakali wrote:

| Hi Jeff,
| Attached is the patch with the first round comments incorporated.
| In addition, this patch contains
| Some fixes related to 32-bit systems.
| Few fixes related to Rx path in NAPI.
| 
| Thanks to all for the comments.
| 
| Pls review this patch as well and come back with your comments.

1. typo? first name is still used in source code:

-static char s2io_driver_version[] = "Version 1.0"; +static char s2iO_driver_version[] = "Version 1.1";

2. don't init. globals to 0 -- it's done automatically for you.

(more this weekend....)

--
~Randy