Re: [net-2.6 PATCH 6/10] Neterion: New driver: Hardware init & configuration

Previous thread: [net-2.6 PATCH 3/10] Neterion: New driver: Kconfig and Makefile by Ramkrishna Vepa on Saturday, March 14, 2009 - 1:20 am. (6 messages)

Next thread: [net-2.6 PATCH 7/10] Neterion: New driver: Traffic & alarm handler by Ramkrishna Vepa on Saturday, March 14, 2009 - 1:21 am. (1 message)
From: Ramkrishna Vepa
Date: Saturday, March 14, 2009 - 1:21 am

This patch takes care of Initialization and configuration steps of
Neterion Inc's X3100 Series 10GbE PCIe I/O Virtualized Server Adapter.
- Device Initialization.
- Verification and setting of device config parameters.
- Allocation of Tx FIFO and Rx Ring descriptors (DTR).
- APIs to get various type of hw stats
- APIs to configure RTS (Receive Traffic Steering)

Signed-off-by: Sivakumar Subramani <sivakumar.subramani@neterion.com>
Signed-off-by: Rastapur Santosh <santosh.rastapur@neterion.com>
Signed-off-by: Ramkrishna Vepa <ram.vepa@neterion.com>
---
diff -urpN patch_5/drivers/net/vxge/vxge-config.c patch_6/drivers/net/vxge/vxge-config.c
--- patch_5/drivers/net/vxge/vxge-config.c	1969-12-31 16:00:00.000000000 -0800
+++ patch_6/drivers/net/vxge/vxge-config.c	2009-03-13 00:11:27.000000000 -0700
@@ -0,0 +1,7576 @@
+/******************************************************************************
+ * This software may be used and distributed according to the terms of
+ * the GNU General Public License (GPL), incorporated herein by reference.
+ * Drivers based on or derived from this code fall under the GPL and must
+ * retain the authorship, copyright and license notice.  This file is not
+ * a complete program and may only be used when the entire operating
+ * system is licensed under the GPL.
+ * See the file COPYING in this distribution for more information.
+ ******************************************************************************/
+/*******************************************************************************
+ * vxge-config.c: Driver for Neterion Inc's X3100 Series 10GbE PCIe I/O
+ *                Virtualized Server Adapter.
+ * Copyright(c) 2002-2009 Neterion Inc.
+ ******************************************************************************/
+#include <linux/version.h>
+#include <linux/netdevice.h>
+#include <linux/etherdevice.h>
+#include <linux/pci.h>
+#include <linux/delay.h>
+
+#include "vxge-traffic.h"
+#include "vxge-config.h"
+
+/*
+ * ...
From: Ben Hutchings
Date: Monday, March 16, 2009 - 2:42 pm

Drivers in the kernel tree are not necessarily compiled as modules, so
their external symbols must be globally unique.  You should use a
driver- specific prefix, presumably "vxge" (possibly with "__" in front
of that).


Why bother clearing members of *channel just before you free it?


Why are you using backslashes in multi-line statements?  This is only
necessary in macro definitions.


This is redundant with pci_find_capability().



Since you only change two bits in the command register, maybe you should

You should use pci_save_state() and pci_restore_state() around resets


You need to reset i to 0 or 1 after this, since you've waited 910 us not



Based on the function call below I'm guessing "sapper" should be

This function

might have 

too much

vertical whitespace.


Use pci_read_vpd() instead of this loop.

[...]

Why 176?  This value should have at least a comment, but preferably a
name or explicit formula.

Also should this variable be called "calendar"?

This is as far as I got.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--

From: Andi Kleen
Date: Monday, March 16, 2009 - 6:26 pm

Having so many arguments in a function is usually a clear sign that it needs

You seem to use vmalloc nearly everywhere. First that has a lot of 
overhead (rounds up to pages, flushes TLBs) and also will cause more

That assert is pretyt pointless because you'll just get a NULL pointer
reference next. Same true all over. asserts (or rather BUG_ON) only
make sense when they check for something that's not nearly obvious


Such magic numbers are frowned upon.

Also in general you should use the pci capabilities accessor functions

Especially the patch orgies are scary. Does that really use readl() et.al.

Such wrappers are not encouraged in Linux code, which aims to do with


Is that readl/writel clean again?

In general your driver looks like it could use a pass with sparse's

Again far too many arguments.


Stopped reading for now, but that file needs a lot of work in general.

-Andi


--

From: Ramkrishna Vepa
Date: Monday, March 16, 2009 - 8:25 pm

Andi,

Thanks for the comments. We are looking into them.

How do I enable this checking?

--

From: Ben Hutchings
Date: Monday, March 16, 2009 - 8:34 pm

What Andi said, only without the typo: run "make C=1 drivers/net/vxge/"

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
--

From: Ramkrishna Vepa
Date: Monday, March 16, 2009 - 10:57 pm

Ben,
Got it.

Thanks,
--

From: Andi Kleen
Date: Tuesday, March 17, 2009 - 3:52 am

Install sparse and then

make V=1

and add __iomem annotations as needed.

There will be a lot of warnings, it's likely enough to concentrate on
iomem first pass.

-Andi
--

Previous thread: [net-2.6 PATCH 3/10] Neterion: New driver: Kconfig and Makefile by Ramkrishna Vepa on Saturday, March 14, 2009 - 1:20 am. (6 messages)

Next thread: [net-2.6 PATCH 7/10] Neterion: New driver: Traffic & alarm handler by Ramkrishna Vepa on Saturday, March 14, 2009 - 1:21 am. (1 message)