Re: [net-next v2 1/8] i40e: main driver core

[Stefan Assmann <sassmann@kpanic.de>]

On 23.08.2013 04:15, Jeff Kirsher wrote:
> From: Jesse Brandeburg <jesse.brandeburg@intel.com>
> 
> This is the driver for the Intel(R) Ethernet Controller XL710 Family.
> 
> This driver is targeted at basic ethernet functionality only, and will be
> improved upon further as time goes on.
> 
> This patch mail contains the driver entry points but does not include transmit
> and receive (see the next patch in the series) routines.

[...]

I see the term VSI a lot in the code, what exactly does it mean?

> ---
>  drivers/net/ethernet/intel/i40e/i40e_main.c | 7520 +++++++++++++++++++++++++++
>  1 file changed, 7520 insertions(+)
>  create mode 100644 drivers/net/ethernet/intel/i40e/i40e_main.c
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> new file mode 100644
> index 0000000..c2a79b5
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c

[...]

> +/**
> + * 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
> + **/
> +enum i40e_status_code i40e_allocate_dma_mem_d(struct i40e_hw *hw,

If you want to use an enum I suggest you shorten the name to something
like i40e_sc, otherwise the function declaration lines will become
longer than needed and we'll have to split the arguments across multiple
lines more than necessary.

> +					      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);
> +	/* GFP_ZERO zeros the memory */
> +	mem->va = dma_alloc_coherent(&pf->pdev->dev, mem->size,
> +				     &mem->pa, GFP_ATOMIC | __GFP_ZERO);
> +	if (mem->va)
> +		return I40E_SUCCESS;
> +	else
> +		return I40E_ERR_NO_MEMORY;

Just wondering why you don't use the standard error codes like ENOMEM?

> +}
> +
> +/**
> + * i40e_free_dma_mem_d - OS specific memory free for shared code
> + * @hw:   pointer to the HW structure
> + * @mem:  ptr to mem struct to free
> + **/
> +enum i40e_status_code i40e_free_dma_mem_d(struct i40e_hw *hw,
> +					  struct i40e_dma_mem *mem)
> +{
> +	struct i40e_pf *pf = (struct i40e_pf *)hw->back;
> +
> +	if (!mem || !mem->va)
> +		return I40E_ERR_PARAM;
> +	dma_free_coherent(&pf->pdev->dev, mem->size, mem->va, mem->pa);
> +	mem->va = NULL;
> +	mem->pa = (dma_addr_t)NULL;

Missing a blank line here.

[...]

> +/**
> + * i40e_get_lump - find a lump of free generic resource
> + * @pile: the pile of resource to search
> + * @needed: the number of items needed
> + * @id: an owner id to stick on the items assigned
> + *
> + * Returns the base item index of the lump, or negative for error
> + *
> + * The search_hint trick and lack of advanced fit-finding only work
> + * because we're highly likely to have all the same size lump requests.
> + * Linear search time and any fragmentation should be minimal.
> + **/
> +static int i40e_get_lump(struct i40e_lump_tracking *pile, u16 needed, u16 id)
> +{
> +	int i = 0, j = 0;
> +	int ret = I40E_ERR_NO_MEMORY;
> +
> +	if (pile == NULL || needed == 0 || id >= I40E_PILE_VALID_BIT) {
> +		pr_info("%s: param err: pile=%p needed=%d id=0x%04x\n",
> +		       __func__, pile, needed, id);

Shouldn't this be indented by another tab instead of the spaces? Maybe
you could use netdev_info() instead of pr_info() so it's easier to
understand which device is meant.

> +		return I40E_ERR_PARAM;
> +	}
> +
> +	/* 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;
> +		} else {
> +			/* not enough, so skip over it and continue looking */
> +			i += j;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +/**
> + * i40e_put_lump - return a lump of generic resource
> + * @pile: the pile of resource to search
> + * @index: the base item index
> + * @id: the owner id of the items assigned
> + *
> + * Returns the count of items in the lump
> + **/
> +static int i40e_put_lump(struct i40e_lump_tracking *pile, u16 index, u16 id)
> +{
> +	int i = index;
> +	int count = 0;
> +
> +	if (pile == NULL || index >= pile->num_entries)
> +		return I40E_ERR_PARAM;
> +
> +	for (i = index;
> +	     i < pile->num_entries && pile->list[i] == (id|I40E_PILE_VALID_BIT);

Missing spaces around |.

[...]

> +static void i40e_tx_timeout(struct net_device *netdev)
> +{
> +	struct i40e_netdev_priv *np = netdev_priv(netdev);
> +	struct i40e_vsi *vsi = np->vsi;
> +	struct i40e_pf *pf = vsi->back;
> +
> +	pf->tx_timeout_count++;
> +
> +	if (time_after(jiffies, (pf->tx_timeout_last_recovery + HZ*20)))
> +		pf->tx_timeout_recovery_level = 0;
> +	pf->tx_timeout_last_recovery = jiffies;
> +	netdev_info(netdev, "%s: recovery level %d\n",
> +		    __func__, pf->tx_timeout_recovery_level);
> +
> +	switch (pf->tx_timeout_recovery_level) {
> +	case 0:
> +		/* disable and re-enable queues for the VSI */
> +		if (in_interrupt()) {
> +			set_bit(__I40E_REINIT_REQUESTED, &pf->state);
> +			set_bit(__I40E_REINIT_REQUESTED, &vsi->state);
> +		} else {
> +			i40e_vsi_reinit_locked(vsi);
> +		}
> +		break;
> +	case 1:
> +		set_bit(__I40E_PF_RESET_REQUESTED, &pf->state);
> +		break;
> +	case 2:
> +		set_bit(__I40E_CORE_RESET_REQUESTED, &pf->state);
> +		break;
> +	case 3:
> +		set_bit(__I40E_GLOBAL_RESET_REQUESTED, &pf->state);
> +		break;
> +	default:
> +		netdev_err(netdev, "%s: recovery unsuccessful\n", __func__);
> +		i40e_down(vsi);
> +		break;
> +	}
> +	i40e_service_event_schedule(pf);
> +	pf->tx_timeout_recovery_level++;
> +
> +	return;

Function is void, no need for return.

> +}
> +
> +/**
> + * i40e_release_rx_desc - Store the new tail and head values
> + * @rx_ring: ring to bump
> + * @val: new head index
> + **/
> +static inline void i40e_release_rx_desc(struct i40e_ring *rx_ring, u32 val)
> +{
> +	rx_ring->next_to_use = val;
> +	/* Force memory writes to complete before letting h/w
> +	 * know there are new descriptors to fetch.  (Only
> +	 * applicable for weak-ordered memory model archs,
> +	 * such as IA-64).
> +	 */
> +	wmb();
> +	writel(val, rx_ring->tail);
> +}
> +
> +/**
> + * i40e_get_vsi_stats_struct - Get System Network Statistics
> + * @vsi: the VSI we care about
> + *
> + * Returns the address of the device statistics structure.
> + * The statistics are actually updated from the service task.
> + **/
> +struct rtnl_link_stats64 *i40e_get_vsi_stats_struct(struct i40e_vsi *vsi)
> +{
> +	return &vsi->net_stats;
> +}
> +
> +/**
> + * 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)
> +{
> +	memcpy(storage,
> +	       i40e_get_vsi_stats_struct(
> +			((struct i40e_netdev_priv *)netdev_priv(netdev))->vsi),
> +	       sizeof(*storage));

Missing a blank line.

> +	return storage;
> +}
> +
> +/**
> + * i40e_vsi_reset_stats - Resets all stats of the given vsi
> + * @vsi: the VSI to have its stats reset
> + **/
> +void i40e_vsi_reset_stats(struct i40e_vsi *vsi)
> +{
> +	int i;
> +	struct rtnl_link_stats64 *ns;
> +
> +	if (!vsi)
> +		return;
> +
> +	ns = i40e_get_vsi_stats_struct(vsi);
> +	memset(ns, 0, sizeof(struct net_device_stats));
> +	memset(&vsi->net_stats_offsets, 0, sizeof(struct net_device_stats));
> +	memset(&vsi->eth_stats, 0, sizeof(struct i40e_eth_stats));
> +	memset(&vsi->eth_stats_offsets, 0, sizeof(struct i40e_eth_stats));
> +	if (vsi->rx_rings)
> +		for (i = 0; i < vsi->num_queue_pairs; i++) {
> +			memset(&vsi->rx_rings[i].rx_stats, 0 ,
> +				sizeof(struct i40e_rx_queue_stats));
> +			memset(&vsi->tx_rings[i].tx_stats, 0,
> +				sizeof(struct i40e_tx_queue_stats));
> +		}
> +	vsi->stat_offsets_loaded = false;
> +}
> +
> +/**
> + * i40e_pf_reset_stats - Reset all of the stats for the given pf
> + * @pf: the PF to be reset
> + **/
> +void i40e_pf_reset_stats(struct i40e_pf *pf)
> +{
> +	memset(&pf->stats, 0, sizeof(struct i40e_hw_port_stats));
> +	memset(&pf->stats_offsets, 0, sizeof(struct i40e_hw_port_stats));
> +	pf->stat_offsets_loaded = false;
> +

Remove blank line above.

> +}
> +
> +/**
> + * 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;

Missing a blank line.

> +	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);
> +	}
> +	if (!offset_loaded)
> +		*offset = new_data;
> +	if (likely(new_data >= *offset))
> +		*stat = new_data - *offset;
> +	else
> +		*stat = (new_data + ((u64)1 << 48)) - *offset;
> +	*stat &= 0xFFFFFFFFFFFFULL;
> +}
> +
> +/**
> + * i40e_stat_update32 - read and update a 32 bit stat from the chip
> + * @hw: ptr to the hardware info
> + * @reg: the hw reg to read
> + * @offset_loaded: has the initial offset been loaded yet
> + * @offset: ptr to current offset value
> + * @stat: ptr to the stat
> + **/
> +static void i40e_stat_update32(struct i40e_hw *hw, u32 reg,
> +			       bool offset_loaded, u64 *offset, u64 *stat)
> +{
> +	u32 new_data;

Missing a blank line.

> +	new_data = rd32(hw, reg);
> +	if (!offset_loaded)
> +		*offset = new_data;
> +	if (likely(new_data >= *offset))
> +		*stat = (u32)(new_data - *offset);
> +	else
> +		*stat = (u32)((new_data + ((u64)1 << 32)) - *offset);
> +}

[...]

> +/**
> + * i40e_is_vsi_in_vlan - Check if VSI is in vlan mode
> + * @vsi: the VSI to be searched
> + *
> + * Returns true if VSI is in vlan mode or false otherwise
> + **/
> +bool i40e_is_vsi_in_vlan(struct i40e_vsi *vsi)
> +{
> +	struct i40e_mac_filter *f;
> +
> +	/* Only -1 for all the filters denotes not in vlan mode
> +	 * so we have to go through all the list in order to make sure
> +	 */
> +	list_for_each_entry(f, &vsi->mac_filter_list, list) {
> +		if (f->vlan < 0)
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
> +/**
> + * i40e_put_mac_in_vlan - Goes through all the macvlan filters and adds a
> + *  macvlan filter for each unique vlan that already exists

Superfluous space at the beginning.

> + * @vsi: the VSI to be searched
> + * @macaddr: the mac address to be filtered
> + * @is_vf: true if it is a vf
> + * @is_netdev: true if it is a netdev
> + *
> + * Returns I40E_SUCCESS on success or -ENOMEM if it could not add a filter
> + **/
> +enum i40e_status_code i40e_put_mac_in_vlan(struct i40e_vsi *vsi,
> +						  u8 *macaddr,
> +						  bool is_vf, bool is_netdev)

This could be better indented, aligned with the struct i40e_vsi.

> +{
> +	struct i40e_mac_filter *f, *add_f;
> +
> +	list_for_each_entry(f, &vsi->mac_filter_list, list) {
> +		if (!i40e_find_filter(vsi, macaddr, f->vlan,
> +				      is_vf, is_netdev)) {
> +			add_f = i40e_add_filter(vsi, macaddr, f->vlan,
> +						is_vf, is_netdev);
> +
> +			if (NULL == add_f) {
> +				dev_info(&vsi->back->pdev->dev, "%s: Could not add filter %d for %pM\n",
> +					 __func__, f->vlan, f->macaddr);
> +				return -ENOMEM;
> +			}
> +		}
> +	}
> +	return I40E_SUCCESS;
> +}
> +
> +/**
> + * i40e_add_filter - Add a mac/vlan filter to the VSI
> + * @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 when no memory available.
> + **/
> +struct i40e_mac_filter *i40e_add_filter(struct i40e_vsi *vsi,
> +					u8 *macaddr, s16 vlan,
> +					bool is_vf, bool is_netdev)
> +{
> +	struct i40e_mac_filter *f;

Just f seems to be a bad naming, can we come up with a more fitting
name for this?

> +
> +	if (!vsi || !macaddr)
> +		return NULL;
> +
> +	f = i40e_find_filter(vsi, macaddr, vlan, is_vf, is_netdev);
> +	if (NULL == f) {
> +		f = kzalloc(sizeof(*f), GFP_ATOMIC);
> +		if (NULL == f)
> +			goto add_filter_out;
> +
> +		memcpy(f->macaddr, macaddr, ETH_ALEN);
> +		f->vlan = vlan;
> +		f->changed = true;
> +
> +		INIT_LIST_HEAD(&f->list);
> +		list_add(&f->list, &vsi->mac_filter_list);
> +	}
> +
> +	/* increment counter and add a new flag if needed */
> +	if (is_vf) {
> +		if (!f->is_vf) {
> +			f->is_vf = true;
> +			f->counter++;
> +		}
> +	} else if (is_netdev) {
> +		if (!f->is_netdev) {
> +			f->is_netdev = true;
> +			f->counter++;
> +		}
> +	} else {
> +		f->counter++;
> +	}
> +
> +	/* changed tells sync_filters_subtask to
> +	 * push the filter down to the firmware
> +	 */
> +	if (f->changed) {
> +		vsi->flags |= I40E_VSI_FLAG_FILTER_CHANGED;
> +		vsi->back->flags |= I40E_FLAG_FILTER_SYNC;
> +	}
> +
> +add_filter_out:
> +	return f;
> +}
> +
> +/**
> + * i40e_del_filter - Remove a mac/vlan filter from the VSI
> + * @vsi: the VSI to be searched
> + * @macaddr: the MAC address
> + * @vlan: the vlan
> + * @is_vf: make sure it's a vf filter, else doesn't matter
> + * @is_netdev: make sure it's a netdev filter, else doesn't matter
> + **/
> +void i40e_del_filter(struct i40e_vsi *vsi,
> +		     u8 *macaddr, s16 vlan,
> +		     bool is_vf, bool is_netdev)

Again, please indent properly. I'm not going to make further comments
about this.

> +{
> +	struct i40e_mac_filter *f;
> +
> +	if (!vsi || !macaddr)
> +		return;
> +
> +	f = i40e_find_filter(vsi, macaddr, vlan, is_vf, is_netdev);
> +	if (NULL == f || f->counter == 0)
> +		goto del_filter_out;
> +
> +	if (is_vf) {
> +		if (f->is_vf) {
> +			f->is_vf = false;
> +			f->counter--;
> +		}
> +	} else if (is_netdev) {
> +		if (f->is_netdev) {
> +			f->is_netdev = false;
> +			f->counter--;
> +		}
> +	} else {
> +		/* make sure we don't remove a filter in use by vf or netdev */
> +		int min_f = 0;
> +		min_f += (f->is_vf ? 1 : 0);
> +		min_f += (f->is_netdev ? 1 : 0);
> +
> +		if (f->counter > min_f)
> +			f->counter--;
> +	}
> +
> +	/* counter == 0 tells sync_filters_subtask to
> +	 * remove the filter from the firmware's list
> +	 */
> +	if (f->counter == 0) {
> +		f->changed = true;
> +		vsi->flags |= I40E_VSI_FLAG_FILTER_CHANGED;
> +		vsi->back->flags |= I40E_FLAG_FILTER_SYNC;
> +	}
> +
> +del_filter_out:
> +	return;

[...]

> +/**
> + * i40e_sync_vsi_filters - Update the VSI filter list to the HW
> + * @vsi: ptr to the VSI
> + *
> + * Push any outstanding VSI filter changes through the AdminQ.
> + *
> + * Returns I40E_SUCCESS or error value
> + **/
> +enum i40e_status_code i40e_sync_vsi_filters(struct i40e_vsi *vsi)
> +{
> +	struct i40e_mac_filter *f, *ftmp;
> +	struct i40e_pf *pf;
> +	int num_add = 0;
> +	int num_del = 0;
> +	u32 changed_flags = 0;
> +	bool add_happened = false;
> +	bool promisc_forced_on = false;
> +	enum i40e_status_code ret = I40E_SUCCESS;
> +	u16 cmd_flags;
> +
> +#define FILTER_LIST_LEN 30

Having the defines in one place at the top of the file would be nice,
unless there's a special reason to have it here.

> +	/* empty array typed pointers, kcalloc later */
> +	struct i40e_aqc_add_macvlan_element_data *add_list;
> +	struct i40e_aqc_remove_macvlan_element_data *del_list;
> +
> +	if (!vsi)
> +		return I40E_ERR_PARAM;
> +	while (test_and_set_bit(__I40E_CONFIG_BUSY, &vsi->state))
> +		usleep_range(1000, 2000);
> +	pf = vsi->back;
> +
> +	if (vsi->netdev) {
> +		changed_flags = vsi->current_netdev_flags ^ vsi->netdev->flags;
> +		vsi->current_netdev_flags = vsi->netdev->flags;
> +	}
> +
> +	if (vsi->flags & I40E_VSI_FLAG_FILTER_CHANGED) {
> +		vsi->flags &= ~I40E_VSI_FLAG_FILTER_CHANGED;
> +
> +		del_list = kcalloc(FILTER_LIST_LEN,
> +			    sizeof(struct i40e_aqc_remove_macvlan_element_data),
> +			    GFP_KERNEL);
> +		if (!del_list)
> +			return I40E_ERR_NO_MEMORY;
> +
> +		list_for_each_entry_safe(f, ftmp, &vsi->mac_filter_list, list) {
> +			if (!f->changed)
> +				continue;
> +
> +			if (f->counter != 0)
> +				continue;
> +			f->changed = false;
> +			cmd_flags = 0;
> +
> +			/* add to delete list */
> +			memcpy(del_list[num_del].mac_addr,
> +			       f->macaddr, ETH_ALEN);
> +			del_list[num_del].vlan_tag =
> +				cpu_to_le16((u16)(f->vlan ==
> +					    I40E_VLAN_ANY ? 0 : f->vlan));
> +
> +			/* vlan0 as wild card to allow packets from all vlans */
> +			if (f->vlan == I40E_VLAN_ANY ||
> +			    (vsi->netdev && !(vsi->netdev->features &
> +					      NETIF_F_HW_VLAN_CTAG_FILTER)))
> +				cmd_flags |= I40E_AQC_MACVLAN_DEL_IGNORE_VLAN;
> +			cmd_flags |= I40E_AQC_MACVLAN_DEL_PERFECT_MATCH;
> +			del_list[num_del].flags = cpu_to_le16(cmd_flags);
> +			num_del++;
> +
> +			/* unlink from filter list */
> +			list_del(&f->list);
> +			kfree(f);
> +
> +			/* flush a full buffer */
> +			if (num_del == FILTER_LIST_LEN) {
> +				ret = i40e_aq_remove_macvlan(&pf->hw,
> +					    vsi->seid, del_list, num_del,
> +					    NULL);
> +				num_del = 0;
> +				memset(del_list, 0, sizeof(*del_list));
> +
> +				if (ret != I40E_SUCCESS)
> +					dev_info(&pf->pdev->dev,

Maybe use netdev_info() instead of dev_info() in the whole driver? Would
be nice if this was consistent.

[...]

> +/**
> + * i40e_change_mtu - NDO callback to change the Maximum Transfer Unit
> + * @netdev: network interface device structure
> + * @new_mtu: new value for maximum frame size
> + *
> + * Returns 0 on success, negative on failure
> + **/
> +static int i40e_change_mtu(struct net_device *netdev, int new_mtu)
> +{
> +	struct i40e_netdev_priv *np = netdev_priv(netdev);
> +	struct i40e_vsi *vsi = np->vsi;
> +	int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN;
> +
> +	/* MTU < 68 is an error and causes problems on some kernels */
> +	if ((new_mtu < 68) || (max_frame > I40E_MAX_RXBUFFER))

Maybe add another netdev_info that MTU < 68 is not supported? Not sure
if it makes sense to set it that low. Never mind if it's just a corner
case.

> +		return -EINVAL;
> +
> +	netdev_info(netdev, "%s: changing MTU from %d to %d\n",
> +		    __func__, netdev->mtu, new_mtu);
> +	netdev->mtu = new_mtu;
> +	if (netif_running(netdev))
> +		i40e_vsi_reinit_locked(vsi);
> +
> +	return 0;
> +}

[...]

> +/**
> + * i40e_enable_misc_int_causes - enable the non-queue interrupts
> + * @hw: ptr to the hardware info
> + **/
> +static void i40e_enable_misc_int_causes(struct i40e_hw *hw)
> +{
> +	u32 val;
> +
> +	/* clear things first */
> +	wr32(hw, I40E_PFINT_ICR0_ENA, 0);  /* disable all */
> +	rd32(hw, I40E_PFINT_ICR0);         /* read to clear */
> +
> +	val = I40E_PFINT_ICR0_ENA_ECC_ERR_MASK	     |
> +	      I40E_PFINT_ICR0_ENA_MAL_DETECT_MASK    |
> +	      I40E_PFINT_ICR0_ENA_GRST_MASK	     |
> +	      I40E_PFINT_ICR0_ENA_PCI_EXCEPTION_MASK |
> +	      I40E_PFINT_ICR0_ENA_GPIO_MASK	     |
> +	      I40E_PFINT_ICR0_ENA_STORM_DETECT_MASK  |
> +	      I40E_PFINT_ICR0_ENA_HMC_ERR_MASK	     |
> +	      I40E_PFINT_ICR0_ENA_VFLR_MASK          |
> +	      I40E_PFINT_ICR0_ENA_ADMINQ_MASK;

Inconsistent mix of tabs and whitespaces in the lines above. I guess you
just missed the tab at the end of I40E_PFINT_ICR0_ENA_VFLR_MASK.

> +
> +	wr32(hw, I40E_PFINT_ICR0_ENA, val);
> +
> +	/* SW_ITR_IDX = 0, but don't change INTENA */
> +	wr32(hw, I40E_PFINT_DYN_CTL0, I40E_PFINT_DYN_CTLN_SW_ITR_INDX_MASK |
> +					I40E_PFINT_DYN_CTLN_INTENA_MSK_MASK);
> +
> +	/* OTHER_ITR_IDX = 0 */
> +	wr32(hw, I40E_PFINT_STAT_CTL0, 0);
> +}

[...]

> +/**
> + * i40e_vsi_configure_bw_alloc - Configure VSI BW allocation per TC
> + * @vsi: the VSI being configured
> + * @enabled_tc: TC bitmap
> + * @bw_credits: BW shared credits per TC
> + *
> + * Returns 0 on success, negative value on failure
> + **/
> +static s32 i40e_vsi_configure_bw_alloc(struct i40e_vsi *vsi,
> +				       u8 enabled_tc,
> +				       u8 *bw_share)
> +{
> +	int i, ret = 0;
> +	struct i40e_aqc_configure_vsi_tc_bw_data bw_data;
> +
> +	bw_data.tc_valid_bits = enabled_tc;
> +	for (i = 0; i < I40E_MAX_TRAFFIC_CLASS; i++)
> +		bw_data.tc_bw_credits[i] = bw_share[i];
> +
> +	ret = i40e_aq_config_vsi_tc_bw(&vsi->back->hw, vsi->seid,
> +				       &bw_data, NULL);
> +	if (ret != I40E_SUCCESS) {
> +		dev_info(&vsi->back->pdev->dev,
> +			 "%s: AQ command Config VSI BW allocation per TC failed = %d\n",
> +			  __func__, vsi->back->hw.aq.asq_last_status);
> +		return ret;
> +	}
> +
> +	for (i = 0; i < I40E_MAX_TRAFFIC_CLASS; i++)
> +		vsi->info.qs_handle[i] = bw_data.qs_handles[i];
> +
> +	return ret;
> +

superfluous blank line.

> +}

[...]

> +/**
> + * i40e_do_reset - Start a PF or Core Reset sequence
> + * @pf: board private structure
> + * @reset_flags: which reset is requested
> + *
> + * The essential difference in resets is that the PF Reset
> + * doesn't clear the packet buffers, doesn't reset the PE
> + * firmware, and doesn't bother the other PFs on the chip.
> + **/
> +void i40e_do_reset(struct i40e_pf *pf, u32 reset_flags)
> +{
> +	u32 val;
> +
> +	WARN_ON(in_interrupt());
> +
> +	/* do the biggest reset indicated */
> +	if (reset_flags & (1 << __I40E_GLOBAL_RESET_REQUESTED)) {
> +
> +		/* Request a Global Reset
> +		 *
> +		 * This will start the chip's countdown to the actual full
> +		 * chip reset event, and a warning interrupt to be sent
> +		 * to all PFs, including the requestor.  Our handler
> +		 * for the warning interrupt will deal with the shutdown
> +		 * and recovery of the switch setup.
> +		 *
> +		 * GlobR includes the MAC/PHY in the reset.
> +		 */
> +		dev_info(&pf->pdev->dev, "%s: GlobalR requested\n", __func__);
> +		val = rd32(&pf->hw, I40E_GLGEN_RTRIG);
> +		val |= I40E_GLGEN_RTRIG_GLOBR_MASK;
> +		wr32(&pf->hw, I40E_GLGEN_RTRIG, val);
> +
> +	} else if (reset_flags & (1 << __I40E_CORE_RESET_REQUESTED)) {
> +
> +		/* Request a Core Reset
> +		 *
> +		 * This will start the chip's countdown to the actual full
> +		 * chip reset event, and a warning interrupt to be sent
> +		 * to all PFs, including the requestor.  Our handler
> +		 * for the warning interrupt will deal with the shutdown
> +		 * and recovery of the switch setup.
> +		 */

Both comments for global and core reset are identical except for one
more line. Could you change the core reset comment to something like:
"Same as Global Reset excluding MAC/PHY" ?

> +		dev_info(&pf->pdev->dev, "%s: CoreR requested\n", __func__);
> +		val = rd32(&pf->hw, I40E_GLGEN_RTRIG);
> +		val |= I40E_GLGEN_RTRIG_CORER_MASK;
> +		wr32(&pf->hw, I40E_GLGEN_RTRIG, val);
> +		flush(&pf->hw);
> +
> +	} else if (reset_flags & (1 << __I40E_PF_RESET_REQUESTED)) {
> +
> +		/* Request a PF Reset
> +		 *
> +		 * This goes directly to the tear-down and rebuild of
> +		 * the switch, since we need to do the same recovery as
> +		 * for the Core Reset.
> +		 */
> +		dev_info(&pf->pdev->dev, "%s: PFR requested\n", __func__);
> +		i40e_handle_reset_warning(pf);
> +
> +	} else if (reset_flags & (1 << __I40E_REINIT_REQUESTED)) {
> +		int v;
> +
> +		/* Find the VSI(s) that requested a re-init */
> +		dev_info(&pf->pdev->dev,
> +			 "%s: VSI reinit requested\n", __func__);
> +		for (v = 0; v < pf->hw.func_caps.num_vsis; v++) {
> +			struct i40e_vsi *vsi = pf->vsi[v];
> +			if (vsi != NULL &&
> +			    test_bit(__I40E_REINIT_REQUESTED, &vsi->state)) {
> +				i40e_vsi_reinit_locked(pf->vsi[v]);
> +				clear_bit(__I40E_REINIT_REQUESTED, &vsi->state);
> +			}
> +		}
> +
> +		/* no further action needed, so return now */
> +		return;
> +	} else {
> +		dev_info(&pf->pdev->dev,
> +			 "%s: bad reset request 0x%08x\n",
> +			 __func__, reset_flags);
> +		return;
> +	}
> +

superfluous blank line.

> +}

[...]

> +/**
> + * i40e_link_event - Update netif_carrier status
> + * @pf: board private structure
> + **/
> +static void i40e_link_event(struct i40e_pf *pf)
> +{
> +	bool new_link, old_link;
> +
> +	new_link = (pf->hw.phy.link_info.link_info & I40E_AQ_LINK_UP);
> +	old_link = (pf->hw.phy.link_info_old.link_info & I40E_AQ_LINK_UP);
> +
> +	if (new_link == old_link)
> +		return;
> +
> +	netdev_info(pf->vsi[pf->lan_vsi]->netdev,
> +		    "%s: NIC Link is %s\n",
> +		    __func__, (new_link ? "Up" : "Down"));
> +
> +	/* Notify the base of the switch tree connected to
> +	 * the link.  Floating VEBs are not notified.

What's a floating VEB?

> +	 */
> +	if (pf->lan_veb != I40E_NO_VEB && pf->veb[pf->lan_veb])
> +		i40e_veb_link_event(pf->veb[pf->lan_veb], new_link);
> +	else
> +		i40e_vsi_link_event(pf->vsi[pf->lan_vsi], new_link);
> +
> +	if (pf->vf)
> +		i40e_vc_notify_link_state(pf);
> +}

[...]

> +/**
> + * i40e_watchdog_subtask - Check and bring link up
> + * @pf: board private structure
> + **/
> +static void i40e_watchdog_subtask(struct i40e_pf *pf)
> +{
> +	int v;

Why did you call the variable v and not i?

> +
> +	/* if interface is down do nothing */
> +	if (test_bit(__I40E_DOWN, &pf->state) ||
> +	    test_bit(__I40E_CONFIG_BUSY, &pf->state))
> +		return;
> +
> +	/* Update the stats for active netdevs so the network stack
> +	 * can look at updated numbers whenever it cares to
> +	 */
> +	for (v = 0; v < pf->hw.func_caps.num_vsis; v++)
> +		if (pf->vsi[v] && pf->vsi[v]->netdev)
> +			i40e_update_stats(pf->vsi[v]);
> +
> +	/* Update the stats for the active switching components */
> +	for (v = 0; v < I40E_MAX_VEB; v++)
> +		if (pf->veb[v])
> +			i40e_update_veb_stats(pf->veb[v]);
> +}

[...]

> +/**
> + * i40e_handle_reset_warning - prep for the core to reset
> + * @pf: board private structure
> + *
> + * Close up the VFs and other things in prep for a Core Reset,
> + * then get ready to rebuild the world.
> + **/
> +static void i40e_handle_reset_warning(struct i40e_pf *pf)
> +{

[...]

> +	/* reinit the misc interrupt */
> +	if (pf->flags & I40E_FLAG_MSIX_ENABLED)

Did I understand this right, the misc interrupt is now handled by the
legacy IRQ handler?

[...]

> +/**
> + * i40e_service_task - Run the driver's async subtasks
> + * @work: pointer to work_struct containing our data
> + **/
> +static void i40e_service_task(struct work_struct *work)
> +{
> +	struct i40e_pf *pf = container_of(work,
> +					  struct i40e_pf,
> +					  service_task);
> +	unsigned long start_time = jiffies;
> +
> +	i40e_reset_subtask(pf);
> +	i40e_handle_mdd_event(pf);
> +	i40e_vc_process_vflr_event(pf);
> +	i40e_watchdog_subtask(pf);
> +	i40e_fdir_reinit_subtask(pf);
> +	i40e_check_hang_subtask(pf);
> +	i40e_sync_filters_subtask(pf);
> +	i40e_clean_adminq_subtask(pf);
> +

This blank line could probably removed as well or did you add it for a
special reason?

> +	i40e_service_event_complete(pf);
> +
> +	/* If the tasks have taken longer than one timer cycle or there
> +	 * is more work to be done, reschedule the service task now
> +	 * rather than wait for the timer to tick again.
> +	 */
> +	if (time_after(jiffies, (start_time + pf->service_timer_period)) ||
> +	    test_bit(__I40E_ADMINQ_EVENT_PENDING, &pf->state)		 ||
> +	    test_bit(__I40E_MDD_EVENT_PENDING, &pf->state)		 ||
> +	    test_bit(__I40E_VFLR_EVENT_PENDING, &pf->state))
> +		i40e_service_event_schedule(pf);
> +}

[...]

> +	/* prep for VF support */
> +	if ((pf->flags & I40E_FLAG_SRIOV_ENABLED) &&
> +	    (pf->flags & I40E_FLAG_MSIX_ENABLED)) {
> +		u32 val;
> +
> +		/* disable link interrupts for VFs */
> +		val = rd32(hw, I40E_PFGEN_PORTMDIO_NUM);
> +		val &= ~I40E_PFGEN_PORTMDIO_NUM_VFLINK_STAT_ENA_MASK;
> +		wr32(hw, I40E_PFGEN_PORTMDIO_NUM, val);
> +		flush(hw);
> +

superfluous blank line.

> +	}

  Stefan