> -----Original Message-----
> From: Stephen Hemminger [mailto:[email protected]]
> Sent: Thursday, September 05, 2013 9:27 AM
>
> More review comments, these are non-blocking, and can be fixed later.
Thanks, Stephen, we'll roll these updates in soon.
sln
>
> Minor
> -----
> * style
> not a fan of doing own error codes and handling (ie. I40E_SUCCESS)
> which probably is side effect of OS abstraction.
>
> * extra parens aren't needed on return
> return (budget > 0);
>
> * i40e_config_netdev
> etherdev_size = (sizeof(struct i40e_netdev_priv));
> Extra paren, only used once, unsigned vs signed, just
> use sizeof()
> strlcpy(netdev->name, "eth%d", IFNAMSIZ-1);
> That is already done by alloc_etherdev
> memcpy(netdev->dev_addr, mac_addr, netdev->addr_len);
> netdev->addr_len is ETH_ALEN, mac_addr is ETH_ALEN
> just use constant size copy.
> * i40e_init_module
> ret = pci_register_driver(&i40e_driver);
> return ret;
> assignment is unneeded, just do return pci_register_driver
>
> * service timer
> since it is 1 HZ, you should use round_jiffies to align
> on clock boundary to save power.
>
> * Why is Kbuild file used instead of Makefile like other drivers?
>
> * Ethtool
> Need to use ethtool_cmd_speed_set othewise only lower bit set.
>
> * Extension I40E_DEBUG_USER is non-standard
>
> * What is is point of using snprintf() in i40e_get_drvinfo
> snprintf(drvinfo->fw_version, sizeof(drvinfo->fw_version),
> "%s", i40e_fw_version_str(&pf->hw));
> Already using strlcpy() for other fields.
>
> * Ethtool ops can be const
>
> * isn't vsi->netdev = NULL needed here.
> s32 i40e_vsi_release(struct i40e_vsi *vsi)
> if (vsi->netdev_registered) {
> vsi->netdev_registered = false;
> if (vsi->netdev) {
> /* results in a call to i40e_close() */
> unregister_netdev(vsi->netdev);
> free_netdev(vsi->netdev);
> >>>> vsi->netdev = NULL;
>
>
> Enhancement
> -----------
> * Support Byte Queue Limits
> - might be tougher with VBE
>
------------------------------------------------------------------------------
Learn the latest--Visual Studio 2012, SharePoint 2013, SQL 2012, more!
Discover the easy way to master current and previous Microsoft technologies
and advance your career. Get an incredible 1,500+ hours of step-by-step
tutorial videos with LearnDevNow. Subscribe today and save!
http://pubads.g.doubleclick.net/gampad/clk?id=58041391&iu=/4140/ostg.clktrk
_______________________________________________
E1000-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit
http://communities.intel.com/community/wired
