On Fri, 2013-08-30 at 05:39 -0700, Jeff Kirsher wrote: > From: Jesse Brandeburg <jesse.brandeb...@intel.com>
trivial comments only: > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c > b/drivers/net/ethernet/intel/i40e/i40e_main.c > +#define DRV_KERN "-k" > + > +#define DRV_VERSION_MAJOR 0 > +#define DRV_VERSION_MINOR 3 > +#define DRV_VERSION_BUILD 8 > +#define DRV_VERSION __stringify(DRV_VERSION_MAJOR) "." \ > + __stringify(DRV_VERSION_MINOR) "." \ > + __stringify(DRV_VERSION_BUILD) DRV_KERN does "-k" add/signify anything useful? > +/* a bit of forward declarations */ > +static void i40e_vsi_reinit_locked(struct i40e_vsi *vsi); > +static void i40e_handle_reset_warning(struct i40e_pf *pf); > +static s32 i40e_add_vsi(struct i40e_vsi *vsi); > +static s32 i40e_add_veb(struct i40e_veb *veb, struct i40e_vsi *vsi); > +static s32 i40e_setup_pf_switch(struct i40e_pf *pf); > +static int i40e_setup_misc_vector(struct i40e_pf *pf); > +static i40e_status i40e_determine_queue_usage(struct i40e_pf *pf); > +static int i40e_setup_pf_filter_control(struct i40e_pf *pf); pity about these. > +static int debug = -1; > +module_param(debug, int, 0); > +MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)"); Maybe make debug a bitfield instead? > +/** > + * i40e_allocate_dma_mem_d - OS specific memory alloc for shared code > + * @hw: pointer to the HW structure > + * @mem: ptr to mem struct to fill out > + * @size: size of memory requested > + * @alignment: what to align the allocation to > + **/ > +i40e_status i40e_allocate_dma_mem_d(struct i40e_hw *hw, > + struct i40e_dma_mem *mem, > + u64 size, u32 alignment) > +{ > + struct i40e_pf *pf = (struct i40e_pf *)hw->back; > + > + if (!mem) > + return I40E_ERR_PARAM; > + > + mem->size = ALIGN(size, alignment); > + mem->va = dma_zalloc_coherent(&pf->pdev->dev, mem->size, > + &mem->pa, GFP_KERNEL); > + if (mem->va) > + return I40E_SUCCESS; > + else > + return I40E_ERR_NO_MEMORY; > +} I would have written if (!mem->va) return I40E_ERR_NO_MEMORY; return I40E_SUCCESS; } so the error checks have the same style and if ever there's a need to do some other initialization then it can be done in the normal column. > +/** > + * i40e_allocate_virt_mem_d - OS specific memory alloc for shared code > + * @hw: pointer to the HW structure > + * @mem: ptr to mem struct to fill out > + * @size: size of memory requested > + **/ > +i40e_status i40e_allocate_virt_mem_d(struct i40e_hw *hw, struct > i40e_virt_mem *mem, > + u32 size) > +{ > + if (!mem) > + return I40E_ERR_PARAM; > + > + mem->size = size; > + mem->va = kzalloc(size, GFP_KERNEL); > + > + if (mem->va) > + return I40E_SUCCESS; > + else > + return I40E_ERR_NO_MEMORY; > +} Same style comment as above. [] > +/** > + * i40e_get_netdev_stats_struct - Get statistics for netdev interface > + * @netdev: network interface device structure > + * > + * Returns the address of the device statistics structure. > + * The statistics are actually updated from the service task. > + **/ > +static struct rtnl_link_stats64 *i40e_get_netdev_stats_struct( > + struct net_device *netdev, > + struct rtnl_link_stats64 *storage) An alternative bsd declaration style might be more readable for these very long types and names. static struct rtnl_link_stats64 * i40e_get_netdev_stats_struct(struct net_device *netdev, struct rtnl_link_stats64 *storage) > +{ > + memcpy(storage, > + i40e_get_vsi_stats_struct( > + ((struct i40e_netdev_priv *)netdev_priv(netdev))->vsi), temporaries might help. > + sizeof(*storage)); > + > + return storage; > +} > +/** > + * i40e_stat_update48 - read and update a 48 bit stat from the chip > + * @hw: ptr to the hardware info > + * @hireg: the high 32 bit reg to read > + * @loreg: the low 32 bit reg to read > + * @offset_loaded: has the initial offset been loaded yet > + * @offset: ptr to current offset value > + * @stat: ptr to the stat > + * > + * Since the device stats are not reset at PFReset, they likely will not > + * be zeroed when the driver starts. We'll save the first values read > + * and use them as offsets to be subtracted from the raw values in order > + * to report stats that count from zero. In the process, we also manage > + * the potential roll-over. > + **/ > +static void i40e_stat_update48(struct i40e_hw *hw, u32 hireg, u32 loreg, > + bool offset_loaded, u64 *offset, u64 *stat) > +{ > + u64 new_data; > + > + if (hw->device_id == I40E_QEMU_DEVICE_ID) { > + new_data = rd32(hw, loreg); > + new_data |= ((u64)(rd32(hw, hireg) & 0xFFFF)) << 32; > + } else { > + new_data = rd64(hw, loreg); > + } Not that I expect to see this hardware on arm64, but are there any endian issues here? [] > +static int i40e_set_mac(struct net_device *netdev, void *p) > +{ [] > + if (!memcmp(netdev->dev_addr, addr->sa_data, netdev->addr_len)) > + return 0; ether_addr_equal? [] > +int i40e_vsi_add_vlan(struct i40e_vsi *vsi, s16 vid) > +{ > + struct i40e_mac_filter *f, *add_f; > + bool is_netdev, is_vf; > + i40e_status ret; > + > + is_vf = (vsi->type == I40E_VSI_SRIOV); > + is_netdev = !!(vsi->netdev); unnecessary !! [] > + if (vid > 0) { > + if (is_netdev && i40e_find_filter( > + vsi, vsi->netdev->dev_addr, > + I40E_VLAN_ANY, > + is_vf, is_netdev)) { Odd linebreak after i40e_find_filter > + i40e_del_filter(vsi, vsi->netdev->dev_addr, > + I40E_VLAN_ANY, is_vf, is_netdev); > + add_f = i40e_add_filter(vsi, vsi->netdev->dev_addr, 0, > + is_vf, is_netdev); nicer to align to open parenthesis > + if (add_f == NULL) { nicer to test if (!add_f) [] > + list_for_each_entry(f, &vsi->mac_filter_list, list) { > + if (is_netdev) { > + if (f->vlan && > + !memcmp(netdev->dev_addr, f->macaddr, > + netdev->addr_len)) another ether_addr_equal? (last mention) > +static void i40e_vsi_free_rx_resources(struct i40e_vsi *vsi) > +{ > + int i; > + > + for (i = 0; i < vsi->num_queue_pairs; i++) > + if (vsi->rx_rings[i].desc) > + i40e_free_rx_resources(&vsi->rx_rings[i]); > +} maybe add braces to the for loop > + > +/** > + * i40e_configure_tx_ring - Configure a transmit ring context and rest > + * @ring: The Tx ring to configure > + * > + * Configure the Tx descriptor ring in the HMC context. > + **/ > +static s32 i40e_configure_tx_ring(struct i40e_ring *ring) > +{ > + struct i40e_vsi *vsi = ring->vsi; > + u16 pf_q = vsi->base_queue + ring->queue_index; > + struct i40e_hw *hw = &vsi->back->hw; > + struct i40e_hmc_obj_txq tx_ctx; > + i40e_status err = I40E_SUCCESS; > + u32 qtx_ctl = 0; > + > + /* some ATR related tx ring init */ > + if (vsi->back->flags & I40E_FLAG_FDIR_ATR_ENABLED) { > + ring->atr_sample_rate = vsi->back->atr_sample_rate; > + ring->atr_count = 0; > + } else { > + ring->atr_sample_rate = 0; > + } > + > + /* clear the context structure first */ > + memset(&tx_ctx, 0, sizeof(struct i40e_hmc_obj_txq)); memset(&tx_ctx, 0, sizeof(tx_ctx)); (too long, stopped reading) ------------------------------------------------------------------------------ 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=58040911&iu=/4140/ostg.clktrk _______________________________________________ E1000-devel mailing list E1000-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/e1000-devel To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired