Re: [PATCH 2.6.17 0/9] NetXen: 1G/10G Ethernet Driver

[Pradeep Dalvi <pradeep@netxen.com>]

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