On Wed, 2013-09-11 at 02:50 -0700, Jeff Kirsher wrote:
> From: Jesse Brandeburg <[email protected]>
>
> This is the driver for the Intel(R) Ethernet Controller XL710 Family.
trivial:
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c
> b/drivers/net/ethernet/intel/i40e/i40e_main.c
[]
> +int 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;
> +
> + mem->size = ALIGN(size, alignment);
> + mem->va = dma_zalloc_coherent(&pf->pdev->dev, mem->size,
> + &mem->pa, GFP_KERNEL);
> + if (mem->va)
> + return 0;
> +
> + return -ENOMEM;
> +}
It's much more common to use
foo = alloc(...)
if (!foo)
return -ENOMEM;
...
return 0;
[]
> +int i40e_allocate_virt_mem_d(struct i40e_hw *hw, struct i40e_virt_mem *mem,
> + u32 size)
> +{
> + mem->size = size;
> + mem->va = kzalloc(size, GFP_KERNEL);
> +
> + if (mem->va)
> + return 0;
> +
> + return -ENOMEM;
> +}
here too.
> +static int i40e_get_lump(struct i40e_pf *pf, struct i40e_lump_tracking *pile,
> + u16 needed, u16 id)
> +{
> + int ret = -ENOMEM;
> + int i = 0;
> + int j = 0;
> +
> + if (!pile || needed == 0 || id >= I40E_PILE_VALID_BIT) {
> + dev_info(&pf->pdev->dev,
> + "param err: pile=%p needed=%d id=0x%04x\n",
> + pile, needed, id);
> + return -EINVAL;
> + }
> +
> + /* start the linear search with an imperfect hint */
> + i = pile->search_hint;
> + while (i < pile->num_entries && ret < 0) {
> + /* skip already allocated entries */
> + if (pile->list[i] & I40E_PILE_VALID_BIT) {
> + i++;
> + continue;
> + }
> +
> + /* do we have enough in this lump? */
> + for (j = 0; (j < needed) && ((i+j) < pile->num_entries); j++) {
> + if (pile->list[i+j] & I40E_PILE_VALID_BIT)
> + break;
> + }
> +
> + if (j == needed) {
> + /* there was enough, so assign it to the requestor */
> + for (j = 0; j < needed; j++)
> + pile->list[i+j] = id | I40E_PILE_VALID_BIT;
> + ret = i;
> + pile->search_hint = i + j;
I think it'd read better with a break;
removing the ret < 0 test from the while loop above
> + } else {
> + /* not enough, so skip over it and continue looking */
> + i += j;
> + }
> + }
> +
> + return ret;
> +}
> +
[]
> +int i40e_sync_vsi_filters(struct i40e_vsi *vsi)
> +{
[]
> + while (test_and_set_bit(__I40E_CONFIG_BUSY, &vsi->state))
> + usleep_range(1000, 2000);
Possible hardware fault sleeps forever?
[]
> +/**
> + * i40e_find_filter - Search VSI filter list for specific mac/vlan filter
> + * @vsi: the VSI to be searched
> + * @macaddr: the MAC address
> + * @vlan: the vlan
> + * @is_vf: make sure its a vf filter, else doesn't matter
> + * @is_netdev: make sure its a netdev filter, else doesn't matter
> + *
> + * Returns ptr to the filter object or NULL
kernel-doc uses a "* Return: " style keyword.
(there are lots of these)
from Documentation/kernel-doc-nano-HOWTO.ext
Example kernel-doc function comment:
/**
* foobar() - short function description of foobar
* @arg1: Describe the first argument to foobar.
* @arg2: Describe the second argument to foobar.
* One can provide multiple line descriptions
* for arguments.
*
* A longer description, with more discussion of the function foobar()
* that might be useful to those using or modifying it. Begins with
* empty comment line, and may include additional embedded empty
* comment lines.
*
* The longer description can have multiple paragraphs.
*
* Return: Describe the return value of foobar.
*/
[]
> +/**
> + * i40e_vsi_kill_vlan - Remove vsi membership for given vlan
> + * @vsi: the vsi being configured
> + * @vid: vlan id to be removed (0 = untagged only , -1 = any)
* Return: 0 on success or negative error code otherwise
[]
> +/**
> + * i40e_vlan_rx_add_vid - Add a vlan id filter to HW offload
> + * @netdev: network interface to be adjusted
> + * @vid: vlan id to be added
> + **/
> +static int i40e_vlan_rx_add_vid(struct net_device *netdev,
> + __always_unused __be16 proto, u16 vid)
> +{
> + struct i40e_netdev_priv *np = netdev_priv(netdev);
> + struct i40e_vsi *vsi = np->vsi;
> + int ret;
> +
> + if (vid > 4095)
> + return 0;
return -err or...
> +
> + netdev_info(vsi->netdev, "adding %pM vid=%d\n",
> + netdev->dev_addr, vid);
> + /* If the network stack called us with vid = 0, we should
> + * indicate to i40e_vsi_add_vlan() that we want to receive
> + * any traffic (i.e. with any vlan tag, or untagged)
> + */
> + ret = i40e_vsi_add_vlan(vsi, vid ? vid : I40E_VLAN_ANY);
> +
> + if (!ret) {
> + if (vid < VLAN_N_VID)
> + set_bit(vid, vsi->active_vlans);
> + }
> +
> + return 0;
make it a void return instead?
> +}
> +
> +/**
> + * i40e_vlan_rx_kill_vid - Remove a vlan id filter from HW offload
> + * @netdev: network interface to be adjusted
> + * @vid: vlan id to be removed
> + **/
> +static int i40e_vlan_rx_kill_vid(struct net_device *netdev,
> + __always_unused __be16 proto, u16 vid)
> +{
> + struct i40e_netdev_priv *np = netdev_priv(netdev);
> + struct i40e_vsi *vsi = np->vsi;
> +
> + netdev_info(vsi->netdev, "removing %pM vid=%d\n",
> + netdev->dev_addr, vid);
> + /* return code is ignored as there is nothing a user
> + * can do about failure to remove and a log message was
> + * already printed from another function
> + */
> + i40e_vsi_kill_vlan(vsi, vid);
> +
> + clear_bit(vid, vsi->active_vlans);
> + return 0;
here too? There are others below.
Maybe change all the functions that have
a fixed return 0 to void?
> +/**
> + * i40e_dcb_get_num_tc - Get the number of TCs from DCBx config
> + * @dcbcfg: the corresponding DCBx configuration structure
> + *
> + * Return the number of TCs from given DCBx configuration
> + **/
> +static u8 i40e_dcb_get_num_tc(struct i40e_dcbx_config *dcbcfg)
> +{
> + int num_tc = 0, i;
> +
> + /* Scan the ETS Config Priority Table to find
> + * traffic class enabled for a given priority
> + * and use the traffic class index to get the
> + * number of traffic classes enabled
> + */
> + for (i = 0; i < I40E_MAX_USER_PRIORITY; i++) {
> + if (dcbcfg->etscfg.prioritytable[i] > num_tc)
> + num_tc = dcbcfg->etscfg.prioritytable[i];
> + }
> +
> + /* Traffic class index starts from zero so
> + * increment to return the actual count
> + */
> + num_tc++;
> +
> + return num_tc;
There's a possible loss of precision here.
why isn't num_tc u8 like the function below?
> +}
> +
> +/**
> + * i40e_dcb_get_enabled_tc - Get enabled traffic classes
> + * @dcbcfg: the corresponding DCBx configuration structure
> + *
> + * Query the current DCB configuration and return the number of
> + * traffic classes enabled from the given DCBX config
> + **/
> +static u8 i40e_dcb_get_enabled_tc(struct i40e_dcbx_config *dcbcfg)
> +{
> + u8 num_tc = i40e_dcb_get_num_tc(dcbcfg);
> + u8 enabled_tc = 1;
> + u8 i;
> +
> + for (i = 0; i < num_tc; i++)
> + enabled_tc |= 1 << i;
> +
> + return enabled_tc;
> +}
------------------------------------------------------------------------------
How ServiceNow helps IT people transform IT departments:
1. Consolidate legacy IT systems to a single system of record for IT
2. Standardize and globalize service processes across IT
3. Implement zero-touch automation to replace manual, redundant tasks
http://pubads.g.doubleclick.net/gampad/clk?id=51271111&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
