Jeff,
Thanks for your comments. We are incorporating these suggestions and will
resubmit an updated driver asap.
--
pradeep
On Tue, 12 Sep 2006, Jeff Garzik wrote:
More updates needed:
1) diff against 2.6.18-rcX (currently -rc6)
2) remove ifdefs around NETIF_F_TSO
3) remove ifdefs around CONFIG_PCI_MSI
4) during initial allocation of port struct, the following line is completely
superfluous:
port->flags &= ~NETXEN_NETDEV_STATUS
5) netxen_nic_set_multi() does not have any multi-cast filter update code
6) remove impossible checks such as the following in nic_set_promise_mode:
+ if ((phy < 0) || (phy > 3))
+ return -1;
7) the following line in nic_set_mtu requires explanation, or fixing:
+ case NETXEN_NIC_XGBE:
+ new_mtu += 100; /* so that MAC accepts frames > MTU */
8) never call udelay() with a number >= 1000. use mdelay()
9) [major] a great many functions simply do
netxen_nic_do_blah()
case NETXEN_NIC_GBE:
netxen_nic_do_blah_gbe()
break;
case NETXEN_NIC_XGBE:
netxen_nic_do_blah_xgbe()
break;
This is silly and makes the code needlessly larger and needlessly slower.
Modularize the driver to avoid all such functions.
10) don't cast iounmap argument to u8*
11) don't case to/from void*, particularly where you accidentally drop the
__iomem marker:
+ addr = (void *)(adapter->ahw.pci_base + off);
12) make sure the driver passes sparse checks. Read Documentation/sparse.txt
13) __iomem markers missing in NETXEN_NIC_HW_BLOCK_WRITE_64,
NETXEN_NIC_HW_BLOCK_READ_64
14) If you need a 'void __iomem *' cast in the following macros, there is a
type definition bug somewhere:
+#define NETXEN_NIC_LOCKED_READ_REG(X, Y) \
+ addr = (void __iomem *)(adapter->ahw.pci_base + X); \
+ *(u32 *)Y = readl(addr);
+
+#define NETXEN_NIC_LOCKED_WRITE_REG(X, Y) \
+ addr = (void __iomem *)(adapter->ahw.pci_base + X); \
+ writel(*(u32 *)Y, addr);
+
15) implement NETXEN_NIC_LOCKED_READ_REG, NETXEN_NIC_LOCKED_WRITE_REG,
NETXEN_NIC_HW_BLOCK_WRITE_64, NETXEN_NIC_HW_BLOCK_READ_64 as static inline
functions rather than macros for greater type safety
16) don't invent your own set_bit, clear_bit, etc: _netxen_crb_set_bit
17) don't needlessly invent new types such as
+typedef __le32 netxen_crbword_t; /* single word in CRB space */
18) The strings in netxen_nic_gstrings_test[] should be easily
computer-parseable. Eliminate parens, spaces
19) eliminate magic numbers (replace with named constants) in, e.g.
+ if ((netxen_rom_fast_read(adapter, 0, &n) == 0) && (n & 0x80000000))
{
+ n &= ~0x80000000;
+ if (n < 1024)
20) use the short driver name (commonly DRV_NAME constant in other drivers)
in ETHTOOL_GDRVINFO:
+ strncpy(drvinfo->driver, "NetXen NIC Driver", 32);
21) export the real firmware version in ETHTOOL_GDRVINFO:
+ strncpy(drvinfo->fw_version, NETXEN_NIC_FW_VERSIONID, 32);
22) netxen_nic_set_settings() simply returns success if the NIC is XGBE,
which is obviously wrong
23) bogus return value -1 in netxen_nic_get_eeprom()
24) -EOPNOTSUPP would seem to be a much better return value for XGBE in
netxen_nic_set_pauseparam()
25) netxen_nic_change_mtu() should check minimum MTU too
26) long delays should be sleeping, not spinning the CPU and locking out
other tasks:
+ udelay(10000);
I stopped reviewing here. That should keep you busy...
Jeff
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
