i40e: display priority_xon and priority_xoff stats

PATCH New Alice Michael 2 participants 19 messages Oct 27, 2017 – Oct 31, 2017

Subsystems (2)

Network drivers Intel Ethernet

lore

Share & tools

lore

Alice Michael

author

2017-10-27 15:06 UTC #0 lore raw

PATCH 1/9 i40e: display priority_xon and priority_xoff stats

Display some more stats that were already being counted, to help users understand when priority xon/xoff packets are being sent/received

Signed-off-by Alice Michael <[email protected]>

---
 drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/net/ethernet/intel/i40e/i40e_ethtool.c +4 -0

index dc9b8dc..2e5ccdd 100644
@@ -126,6 +126,10 @@ static const struct i40e_stats i40e_gstrings_stats[] = {
 	I40E_PF_STAT("link_xoff_rx", stats.link_xoff_rx),
 	I40E_PF_STAT("link_xon_tx", stats.link_xon_tx),
 	I40E_PF_STAT("link_xoff_tx", stats.link_xoff_tx),
+	I40E_PF_STAT("priority_xon_rx", stats.priority_xon_rx),
+	I40E_PF_STAT("priority_xoff_rx", stats.priority_xoff_rx),
+	I40E_PF_STAT("priority_xon_tx", stats.priority_xon_tx),
+	I40E_PF_STAT("priority_xoff_tx", stats.priority_xoff_tx),
 	I40E_PF_STAT("rx_size_64", stats.rx_size_64),
 	I40E_PF_STAT("rx_size_127", stats.rx_size_127),
 	I40E_PF_STAT("rx_size_255", stats.rx_size_255),

Signature · 1 line

2.9.5

Alice Michael

author

2017-10-27 15:06 UTC #1 lore raw

PATCH 2/9 i40evf: don't rely on netif_running() outside rtnl_lock()

From: Jacob Keller <[email protected]>

In i40evf_reset_task we use netif_running() to determine whether or not the device is currently up. This allows us to properly free queue memory and shut down things before we request the hardware reset.

It turns out that we cannot be guaranteed of netif_running() returning false until the device is fully up, as the kernel core code sets __LINK_STATE_START prior to calling .ndo_open. Since we're not holding the rtnl_lock(), it's possible that the driver's i40evf_open handler function is currently being called while we're resetting.

We can't simply hold the rtnl_lock() while checking netif_running() as this could cause a deadlock with the i40evf_open() function. Additionally, we can't avoid the deadlock by holding the rtnl_lock() over the whole reset path, as this essentially serializes all resets, and can cause massive delays if we have multiple VFs on a system.

Instead, lets just check our own internal state __I40EVF_RUNNING state field. This allows us to ensure that the state is correct and is only set after we've finished bringing the device up.

Without this change we might free data structures about device queues and other memory before they've been fully allocated.

Signed-off-by Jacob Keller <[email protected]>

---
 drivers/net/ethernet/intel/i40evf/i40evf_main.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

drivers/net/ethernet/intel/i40evf/i40evf_main.c +17 -3

index ca2ebdb..320408f 100644
@@ -1796,7 +1796,11 @@ static void i40evf_disable_vf(struct i40evf_adapter *adapter)
 
 	adapter->flags |= I40EVF_FLAG_PF_COMMS_FAILED;
 
-	if (netif_running(adapter->netdev)) {
+	/* We don't use netif_running() because it may be true prior to
+	 * ndo_open() returning, so we can't assume it means all our open
+	 * tasks have finished, since we're not holding the rtnl_lock here.
+	 */
+	if (adapter->state == __I40EVF_RUNNING) {
 		set_bit(__I40E_VSI_DOWN, adapter->vsi.state);
 		netif_carrier_off(adapter->netdev);
 		netif_tx_disable(adapter->netdev);

@@ -1854,6 +1858,7 @@ static void i40evf_reset_task(struct work_struct *work)
 	struct i40evf_mac_filter *f;
 	u32 reg_val;
 	int i = 0, err;
+	bool running;
 
 	while (test_and_set_bit(__I40EVF_IN_CLIENT_TASK,
 				&adapter->crit_section))

@@ -1913,7 +1918,13 @@ static void i40evf_reset_task(struct work_struct *work)
 	}
 
 continue_reset:
-	if (netif_running(netdev)) {
+	/* We don't use netif_running() because it may be true prior to
+	 * ndo_open() returning, so we can't assume it means all our open
+	 * tasks have finished, since we're not holding the rtnl_lock here.
+	 */
+	running = (adapter->state == __I40EVF_RUNNING);
+
+	if (running) {
 		netif_carrier_off(netdev);
 		netif_tx_stop_all_queues(netdev);
 		adapter->link_up = false;

@@ -1964,7 +1975,10 @@ static void i40evf_reset_task(struct work_struct *work)
 
 	mod_timer(&adapter->watchdog_timer, jiffies + 2);
 
-	if (netif_running(adapter->netdev)) {
+	/* We were running when the reset started, so we need to restore some
+	 * state here.
+	 */
+	if (running) {
 		/* allocate transmit descriptors */
 		err = i40evf_setup_all_tx_resources(adapter);
 		if (err)

Signature · 1 line

2.9.5

Bowers

2017-10-30 22:02 UTC #2 lore raw

PATCH 2/9 i40evf: don't rely on netif_running() outside rtnl_lock()

Quoted 31 lines

Quoted 8 lines

Tested-by Andrew Bowers <[email protected]>

Alice Michael

author

2017-10-27 15:06 UTC #3 lore raw

PATCH 3/9 i40evf: use spinlock to protect (mac|vlan)_filter_list

In reply to Alice Michael

From: Jacob Keller <[email protected]>

Stop overloading the __I40EVF_IN_CRITICAL_TASK bit lock to protect the mac_filter_list and vlan_filter_list. Instead, implement a spinlock to protect these two lists, similar to how we protect the hash in the i40e PF code.

Ensure that every place where we access the list uses the spinlock to ensure consistency, and stop holding the critical section around blocks of code which only need access to the macvlan filter lists.

This refactor helps simplify the locking behavior, and is necessary as a future refactor to the __I40EVF_IN_CRITICAL_TASK would cause a deadlock otherwise.

Signed-off-by Jacob Keller <[email protected]>

---
 drivers/net/ethernet/intel/i40evf/i40evf.h         |  4 +-
 drivers/net/ethernet/intel/i40evf/i40evf_main.c    | 84 +++++++++++-----------
 .../net/ethernet/intel/i40evf/i40evf_virtchnl.c    | 42 +++++++++--
 3 files changed, 81 insertions(+), 49 deletions(-)

drivers/net/ethernet/intel/i40evf/i40evf.h +3 -1

index de0af52..47040ab 100644
@@ -199,6 +199,9 @@ struct i40evf_adapter {
 	wait_queue_head_t down_waitqueue;
 	struct i40e_q_vector *q_vectors;
 	struct list_head vlan_filter_list;
+	struct list_head mac_filter_list;
+	/* Lock to protect accesses to MAC and VLAN lists */
+	spinlock_t mac_vlan_list_lock;
 	char misc_vector_name[IFNAMSIZ + 9];
 	int num_active_queues;
 	int num_req_queues;

@@ -206,7 +209,6 @@ struct i40evf_adapter {
 	/* TX */
 	struct i40e_ring *tx_rings;
 	u32 tx_timeout_count;
-	struct list_head mac_filter_list;
 	u32 tx_desc_count;
 
 	/* RX */

drivers/net/ethernet/intel/i40evf/i40evf_main.c +40 ~~-44~~

226 lines not on this page · raw message

drivers/net/ethernet/intel/i40evf/i40evf_virtchnl.c +38 -4

142 lines not on this page · raw message

Signature · 1 line

2.9.5

Bowers

2017-10-31 19:04 UTC #4 lore raw

PATCH 3/9 i40evf: use spinlock to protect (mac|vlan)_filter_list

Quoted 23 lines

Quoted 7 lines

Signed-off-by Jacob Keller <[email protected]>

---
 drivers/net/ethernet/intel/i40evf/i40evf.h         |  4 +-
 drivers/net/ethernet/intel/i40evf/i40evf_main.c    | 84 +++++++++++--------
---
 .../net/ethernet/intel/i40evf/i40evf_virtchnl.c    | 42 +++++++++--
 3 files changed, 81 insertions(+), 49 deletions(-)

Tested-by Andrew Bowers <[email protected]>

Alice Michael

author

2017-10-27 15:06 UTC #5 lore raw

PATCH 4/9 i40evf: release bit locks in reverse order

In reply to Alice Michael

From: Jacob Keller <[email protected]>

Although not strictly necessary, it is customary to reverse the order in which we release locks that we acquire. This helps preserve lock ordering during future refactors, which can help avoid potential deadlock situations.

Signed-off-by Jacob Keller <[email protected]>

---
 drivers/net/ethernet/intel/i40evf/i40evf_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/net/ethernet/intel/i40evf/i40evf_main.c +1 -1

index 2b135dc..9a945ff 100644
@@ -1960,8 +1960,8 @@ static void i40evf_reset_task(struct work_struct *work)
 
 	adapter->aq_required |= I40EVF_FLAG_AQ_ADD_MAC_FILTER;
 	adapter->aq_required |= I40EVF_FLAG_AQ_ADD_VLAN_FILTER;
-	clear_bit(__I40EVF_IN_CRITICAL_TASK, &adapter->crit_section);
 	clear_bit(__I40EVF_IN_CLIENT_TASK, &adapter->crit_section);
+	clear_bit(__I40EVF_IN_CRITICAL_TASK, &adapter->crit_section);
 	i40evf_misc_irq_enable(adapter);
 
 	mod_timer(&adapter->watchdog_timer, jiffies + 2);

Signature · 1 line

2.9.5

Bowers

2017-10-31 19:05 UTC #6 lore raw

PATCH 4/9 i40evf: release bit locks in reverse order

Quoted 11 lines

Quoted 8 lines

Tested-by Andrew Bowers <[email protected]>

Alice Michael

author

2017-10-27 15:06 UTC #7 lore raw

PATCH 5/9 i40evf: hold the critical task bit lock while opening

In reply to Alice Michael

From: Jacob Keller <[email protected]>

If i40evf_open() is called quickly at the same time as a reset occurs (such as via ethtool) it is possible for the device to attempt to open while a reset is in progress. This occurs because the driver was not holding the critical task bit lock during i40evf_open, nor was it holding it around the call to i40evf_up_complete() in i40evf_reset_task().

We didn't hold the lock previously because calls to i40evf_down() would take the bit lock directly, and this would have caused a deadlock.

To avoid this, we'll move the bit lock handling out of i40evf_down() and into the callers of this function. Additionally, we'll now hold the bit lock over the entire set of steps when going up or down, to ensure that we remain consistent.

Ultimately this causes us to serialize the transitions between down and up properly, and avoid changing status while we're resetting.

Signed-off-by Jacob Keller <[email protected]>

---
 drivers/net/ethernet/intel/i40evf/i40evf_main.c | 40 +++++++++++++++++++------
 1 file changed, 31 insertions(+), 9 deletions(-)

drivers/net/ethernet/intel/i40evf/i40evf_main.c +31 -9

index 9a945ff..45cff72 100644
@@ -1035,6 +1035,8 @@ static void i40evf_configure(struct i40evf_adapter *adapter)
 /**
  * i40evf_up_complete - Finish the last steps of bringing up a connection
  * @adapter: board private structure
+ *
+ * Expects to be called while holding the __I40EVF_IN_CRITICAL_TASK bit lock.
  **/
 static void i40evf_up_complete(struct i40evf_adapter *adapter)
 {

@@ -1052,6 +1054,8 @@ static void i40evf_up_complete(struct i40evf_adapter *adapter)
 /**
  * i40e_down - Shutdown the connection processing
  * @adapter: board private structure
+ *
+ * Expects to be called while holding the __I40EVF_IN_CRITICAL_TASK bit lock.
  **/
 void i40evf_down(struct i40evf_adapter *adapter)
 {

@@ -1061,10 +1065,6 @@ void i40evf_down(struct i40evf_adapter *adapter)
 	if (adapter->state <= __I40EVF_DOWN_PENDING)
 		return;
 
-	while (test_and_set_bit(__I40EVF_IN_CRITICAL_TASK,
-				&adapter->crit_section))
-		usleep_range(500, 1000);
-
 	netif_carrier_off(netdev);
 	netif_tx_disable(netdev);
 	adapter->link_up = false;

@@ -1097,7 +1097,6 @@ void i40evf_down(struct i40evf_adapter *adapter)
 		adapter->aq_required |= I40EVF_FLAG_AQ_DISABLE_QUEUES;
 	}
 
-	clear_bit(__I40EVF_IN_CRITICAL_TASK, &adapter->crit_section);
 	mod_timer_pending(&adapter->watchdog_timer, jiffies + 1);
 }

@@ -1960,8 +1959,6 @@ static void i40evf_reset_task(struct work_struct *work)
 
 	adapter->aq_required |= I40EVF_FLAG_AQ_ADD_MAC_FILTER;
 	adapter->aq_required |= I40EVF_FLAG_AQ_ADD_VLAN_FILTER;
-	clear_bit(__I40EVF_IN_CLIENT_TASK, &adapter->crit_section);
-	clear_bit(__I40EVF_IN_CRITICAL_TASK, &adapter->crit_section);
 	i40evf_misc_irq_enable(adapter);
 
 	mod_timer(&adapter->watchdog_timer, jiffies + 2);

@@ -1998,9 +1995,13 @@ static void i40evf_reset_task(struct work_struct *work)
 		adapter->state = __I40EVF_DOWN;
 		wake_up(&adapter->down_waitqueue);
 	}
+	clear_bit(__I40EVF_IN_CLIENT_TASK, &adapter->crit_section);
+	clear_bit(__I40EVF_IN_CRITICAL_TASK, &adapter->crit_section);
 
 	return;
 reset_err:
+	clear_bit(__I40EVF_IN_CLIENT_TASK, &adapter->crit_section);
+	clear_bit(__I40EVF_IN_CRITICAL_TASK, &adapter->crit_section);
 	dev_err(&adapter->pdev->dev, "failed to allocate resources during reinit\n");
 	i40evf_close(netdev);
 }

@@ -2244,8 +2245,14 @@ static int i40evf_open(struct net_device *netdev)
 		return -EIO;
 	}
 
-	if (adapter->state != __I40EVF_DOWN)
-		return -EBUSY;
+	while (test_and_set_bit(__I40EVF_IN_CRITICAL_TASK,
+				&adapter->crit_section))
+		usleep_range(500, 1000);
+
+	if (adapter->state != __I40EVF_DOWN) {
+		err = -EBUSY;
+		goto err_unlock;
+	}
 
 	/* allocate transmit descriptors */
 	err = i40evf_setup_all_tx_resources(adapter);

@@ -2269,6 +2276,8 @@ static int i40evf_open(struct net_device *netdev)
 
 	i40evf_irq_enable(adapter, true);
 
+	clear_bit(__I40EVF_IN_CRITICAL_TASK, &adapter->crit_section);
+
 	return 0;
 
 err_req_irq:

@@ -2278,6 +2287,8 @@ static int i40evf_open(struct net_device *netdev)
 	i40evf_free_all_rx_resources(adapter);
 err_setup_tx:
 	i40evf_free_all_tx_resources(adapter);
+err_unlock:
+	clear_bit(__I40EVF_IN_CRITICAL_TASK, &adapter->crit_section);
 
 	return err;
 }

@@ -2301,6 +2312,9 @@ static int i40evf_close(struct net_device *netdev)
 	if (adapter->state <= __I40EVF_DOWN_PENDING)
 		return 0;
 
+	while (test_and_set_bit(__I40EVF_IN_CRITICAL_TASK,
+				&adapter->crit_section))
+		usleep_range(500, 1000);
 
 	set_bit(__I40E_VSI_DOWN, adapter->vsi.state);
 	if (CLIENT_ENABLED(adapter))

@@ -2310,6 +2324,8 @@ static int i40evf_close(struct net_device *netdev)
 	adapter->state = __I40EVF_DOWN_PENDING;
 	i40evf_free_traffic_irqs(adapter);
 
+	clear_bit(__I40EVF_IN_CRITICAL_TASK, &adapter->crit_section);
+
 	/* We explicitly don't free resources here because the hardware is
 	 * still active and can DMA into memory. Resources are cleared in
 	 * i40evf_virtchnl_completion() after we get confirmation from the PF

@@ -2992,6 +3008,10 @@ static int i40evf_suspend(struct pci_dev *pdev, pm_message_t state)
 
 	netif_device_detach(netdev);
 
+	while (test_and_set_bit(__I40EVF_IN_CRITICAL_TASK,
+				&adapter->crit_section))
+		usleep_range(500, 1000);
+
 	if (netif_running(netdev)) {
 		rtnl_lock();
 		i40evf_down(adapter);

@@ -3000,6 +3020,8 @@ static int i40evf_suspend(struct pci_dev *pdev, pm_message_t state)
 	i40evf_free_misc_irq(adapter);
 	i40evf_reset_interrupt_capability(adapter);
 
+	clear_bit(__I40EVF_IN_CRITICAL_TASK, &adapter->crit_section);
+
 	retval = pci_save_state(pdev);
 	if (retval)
 		return retval;

Signature · 1 line

2.9.5

Bowers

2017-10-31 19:05 UTC #8 lore raw

PATCH 5/9 i40evf: hold the critical task bit lock while opening

Quoted 25 lines

Quoted 8 lines

Tested-by Andrew Bowers <[email protected]>

Alice Michael

author

2017-10-27 15:06 UTC #9 lore raw

PATCH 6/9 i40e: update VFs of link state after GET_VF_RESOURCES

In reply to Alice Michael

From: Jacob Keller <[email protected]>

We currently notify a VF of the link state after ENABLE_QUEUES, which is the last thing a VF does after being configured. Guests may not actually ENABLE_QUEUES until they get configured, and thus between driver load and device configuration the VF may show inaccurate link status.

Fix this by also sending the link state after GET_VF_RESOURCES. Although we could remove the message following ENABLE_QUEUES, it's not that significant of a loss, so this patch just keeps both to ensure maximum compatibility with guests on various OSes.

Specifically, without this patch guests running FreeBSD will display inaccurate link state until the device is brought up. This is mostly a cosmetic issue but can be confusing to system administrators.

Signed-off-by Jacob Keller <[email protected]>

---
 drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 1 +
 1 file changed, 1 insertion(+)

drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c +1 -0

index 56dd1e8..5c5113c 100644
@@ -2748,6 +2748,7 @@ int i40e_vc_process_vf_msg(struct i40e_pf *pf, s16 vf_id, u32 v_opcode,
 		break;
 	case VIRTCHNL_OP_GET_VF_RESOURCES:
 		ret = i40e_vc_get_vf_resources_msg(vf, msg);
+		i40e_vc_notify_vf_link_state(vf);
 		break;
 	case VIRTCHNL_OP_RESET_VF:
 		i40e_vc_reset_vf_msg(vf);

Signature · 1 line

2.9.5

Bowers

2017-10-31 19:06 UTC #10 lore raw

PATCH 6/9 i40e: update VFs of link state after GET_VF_RESOURCES

Quoted 21 lines

Quoted 8 lines

Tested-by Andrew Bowers <[email protected]>

Alice Michael

author

2017-10-27 15:06 UTC #11 lore raw

PATCH 7/9 i40e: add helper conversion function for link_speed

In reply to Alice Michael

From: Jacob Keller <[email protected]>

We introduced the virtchnl interface in order to have an interface for talking to a virtual device driver which was host-driver agnostic. This interface has its own definitions, including one for link speed.

The host driver has to talk to the virtchnl interface using these new definitions in order to remain compatible. Today, the i40e link_speed enumerations are value-exact matches for the virtchnl interface, so it was originally decided to simply use a typecast.

However, this is unsafe, and makes it easier for future drivers to continue this unsafe practice. There is nothing guaranteeing these values are exact, and the type-cast would hide any compiler warning which indicates the problem.

Rather than rely on this type cast, introduce a helper function which can convert the AdminQ link speed definition into a virtchnl definition. This can then be used by host driver implementations in order to safely convert to the interface recognized by the virtual functions.

If the link speed is not able to be represented by the virtchnl definitions we'll report UNKNOWN which is the safest result.

This will ensure that should the driver specific link_speeds actual bit definitions change, we do not report them incorrectly according to the VF.

Additionally, this provides a better pattern for future drivers to copy, as it is more likely a future device may not use the exact same bit-wise definition as the current virtchnl interface.

Signed-off-by Jacob Keller <[email protected]>

---
 drivers/net/ethernet/intel/i40e/i40e_prototype.h   | 31 ++++++++++++++++++++++
 drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c |  4 +--
 2 files changed, 33 insertions(+), 2 deletions(-)

drivers/net/ethernet/intel/i40e/i40e_prototype.h +31 -0

index 3bb6659..e70bebc 100644
@@ -343,6 +343,37 @@ static inline struct i40e_rx_ptype_decoded decode_rx_desc_ptype(u8 ptype)
 	return i40e_ptype_lookup[ptype];
 }
 
+/**
+ * i40e_virtchnl_link_speed - Convert AdminQ link_speed to virtchnl definition
+ * @link_speed: the speed to convert
+ *
+ * Returns the link_speed in terms of the virtchnl interface, for use in
+ * converting link_speed as reported by the AdminQ into the format used for
+ * talking to virtchnl devices. If we can't represent the link speed properly,
+ * report LINK_SPEED_UNKNOWN.
+ **/
+static inline enum virtchnl_link_speed
+i40e_virtchnl_link_speed(enum i40e_aq_link_speed link_speed)
+{
+	switch (link_speed) {
+	case I40E_LINK_SPEED_100MB:
+		return VIRTCHNL_LINK_SPEED_100MB;
+	case I40E_LINK_SPEED_1GB:
+		return VIRTCHNL_LINK_SPEED_1GB;
+	case I40E_LINK_SPEED_10GB:
+		return VIRTCHNL_LINK_SPEED_10GB;
+	case I40E_LINK_SPEED_40GB:
+		return VIRTCHNL_LINK_SPEED_40GB;
+	case I40E_LINK_SPEED_20GB:
+		return VIRTCHNL_LINK_SPEED_20GB;
+	case I40E_LINK_SPEED_25GB:
+		return VIRTCHNL_LINK_SPEED_25GB;
+	case I40E_LINK_SPEED_UNKNOWN:
+	default:
+		return VIRTCHNL_LINK_SPEED_UNKNOWN;
+	}
+}
+
 /* prototype for functions used for SW locks */
 
 /* i40e_common for VF drivers*/

drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c +2 -2

index 5c5113c..bfba60f 100644
@@ -81,12 +81,12 @@ static void i40e_vc_notify_vf_link_state(struct i40e_vf *vf)
 	if (vf->link_forced) {
 		pfe.event_data.link_event.link_status = vf->link_up;
 		pfe.event_data.link_event.link_speed =
-			(vf->link_up ? I40E_LINK_SPEED_40GB : 0);
+			(vf->link_up ? VIRTCHNL_LINK_SPEED_40GB : 0);
 	} else {
 		pfe.event_data.link_event.link_status =
 			ls->link_info & I40E_AQ_LINK_UP;
 		pfe.event_data.link_event.link_speed =
-			(enum virtchnl_link_speed)ls->link_speed;
+			i40e_virtchnl_link_speed(ls->link_speed);
 	}
 	i40e_aq_send_msg_to_vf(hw, abs_vf_id, VIRTCHNL_OP_EVENT,
 			       0, (u8 *)&pfe, sizeof(pfe), NULL);

Signature · 1 line

2.9.5

Bowers

2017-10-31 19:06 UTC #12 lore raw

PATCH 7/9 i40e: add helper conversion function for link_speed

Quoted 40 lines

Quoted 6 lines

Signed-off-by Jacob Keller <[email protected]>

---
 drivers/net/ethernet/intel/i40e/i40e_prototype.h   | 31
++++++++++++++++++++++
 drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c |  4 +--
 2 files changed, 33 insertions(+), 2 deletions(-)

Tested-by Andrew Bowers <[email protected]>

Alice Michael

author

2017-10-27 15:06 UTC #13 lore raw

PATCH 8/9 i40e: Fix for NUP NVM image downgrade failure

In reply to Alice Michael

From: Jacob Keller <[email protected]>

Since commit 96a39aed25e6 ("i40e: Acquire NVM lock before reads on all devices") we've used the NVM lock
to synchronize NVM reads even on devices which don't strictly need the lock.

Doing so can cause a regression on older firmware prior to 1.5, especially when downgrading the firmware.

Fix this by only grabbing the lock if we're running on an X722 device (which requires the lock as it uses the AdminQ to read the NVM), or if we're currently running 1.5 or newer firmware.

Signed-off-by Jacob Keller <[email protected]>

---
 drivers/net/ethernet/intel/i40e/i40e_adminq.c | 6 ++++++
 drivers/net/ethernet/intel/i40e/i40e_common.c | 3 ++-
 drivers/net/ethernet/intel/i40e/i40e_nvm.c    | 8 +++++---
 drivers/net/ethernet/intel/i40e/i40e_type.h   | 1 +
 4 files changed, 14 insertions(+), 4 deletions(-)

drivers/net/ethernet/intel/i40e/i40e_adminq.c +6 -0

index 9dcb2a9..9af7425 100644
@@ -613,6 +613,12 @@ i40e_status i40e_init_adminq(struct i40e_hw *hw)
 		hw->flags |= I40E_HW_FLAG_AQ_PHY_ACCESS_CAPABLE;
 	}
 
+	/* Newer versions of firmware require lock when reading the NVM */
+	if (hw->aq.api_maj_ver > 1 ||
+	    (hw->aq.api_maj_ver == 1 &&
+	     hw->aq.api_min_ver >= 5))
+		hw->flags |= I40E_HW_FLAG_NVM_READ_REQUIRES_LOCK;
+
 	/* The ability to RX (not drop) 802.1ad frames was added in API 1.7 */
 	if (hw->aq.api_maj_ver > 1 ||
 	    (hw->aq.api_maj_ver == 1 &&

drivers/net/ethernet/intel/i40e/i40e_common.c +2 -1

index 0203665..13c7946 100644
@@ -948,7 +948,8 @@ i40e_status i40e_init_shared_code(struct i40e_hw *hw)
 		hw->pf_id = (u8)(func_rid & 0x7);
 
 	if (hw->mac.type == I40E_MAC_X722)
-		hw->flags |= I40E_HW_FLAG_AQ_SRCTL_ACCESS_ENABLE;
+		hw->flags |= I40E_HW_FLAG_AQ_SRCTL_ACCESS_ENABLE |
+			     I40E_HW_FLAG_NVM_READ_REQUIRES_LOCK;
 
 	status = i40e_init_nvm(hw);
 	return status;

drivers/net/ethernet/intel/i40e/i40e_nvm.c +5 -3

index 0ccab0a..7689c2e 100644
@@ -328,15 +328,17 @@ static i40e_status __i40e_read_nvm_word(struct i40e_hw *hw,
 i40e_status i40e_read_nvm_word(struct i40e_hw *hw, u16 offset,
 			       u16 *data)
 {
-	i40e_status ret_code;
+	i40e_status ret_code = 0;
 
-	ret_code = i40e_acquire_nvm(hw, I40E_RESOURCE_READ);
+	if (hw->flags & I40E_HW_FLAG_NVM_READ_REQUIRES_LOCK)
+		ret_code = i40e_acquire_nvm(hw, I40E_RESOURCE_READ);
 	if (ret_code)
 		return ret_code;
 
 	ret_code = __i40e_read_nvm_word(hw, offset, data);
 
-	i40e_release_nvm(hw);
+	if (hw->flags & I40E_HW_FLAG_NVM_READ_REQUIRES_LOCK)
+		i40e_release_nvm(hw);
 
 	return ret_code;
 }

drivers/net/ethernet/intel/i40e/i40e_type.h +1 -0

index 00d4833..0e85687 100644
@@ -629,6 +629,7 @@ struct i40e_hw {
 #define I40E_HW_FLAG_AQ_SRCTL_ACCESS_ENABLE BIT_ULL(0)
 #define I40E_HW_FLAG_802_1AD_CAPABLE        BIT_ULL(1)
 #define I40E_HW_FLAG_AQ_PHY_ACCESS_CAPABLE  BIT_ULL(2)
+#define I40E_HW_FLAG_NVM_READ_REQUIRES_LOCK BIT_ULL(3)
 	u64 flags;
 
 	/* Used in set switch config AQ command */

Signature · 1 line

2.9.5

Keller

2017-10-30 22:44 UTC #14 lore raw

PATCH 8/9 i40e: Fix for NUP NVM image downgrade failure

ACK

Quoted 105 lines

-----Original Message-----
From: Michael, Alice
Sent: Friday, October 27, 2017 8:07 AM
To: Michael, Alice <[email protected]>; intel-wired-lan at lists.osuosl.org
Cc: Keller, Jacob E <[email protected]>
Subject: [next PATCH S81-V3 8/9] i40e: Fix for NUP NVM image downgrade failure

From: Jacob Keller <[email protected]>

Since commit 96a39aed25e6 ("i40e: Acquire NVM lock before reads on all devices") we've used the NVM lock
to synchronize NVM reads even on devices which don't strictly need the lock.

Doing so can cause a regression on older firmware prior to 1.5, especially when downgrading the firmware.

Fix this by only grabbing the lock if we're running on an X722 device (which requires the lock as it uses the AdminQ to read the NVM), or if we're currently running 1.5 or newer firmware.

Signed-off-by Jacob Keller <[email protected]>

---
 drivers/net/ethernet/intel/i40e/i40e_adminq.c | 6 ++++++
 drivers/net/ethernet/intel/i40e/i40e_common.c | 3 ++-
 drivers/net/ethernet/intel/i40e/i40e_nvm.c    | 8 +++++---
 drivers/net/ethernet/intel/i40e/i40e_type.h   | 1 +
 4 files changed, 14 insertions(+), 4 deletions(-)

drivers/net/ethernet/intel/i40e/i40e_adminq.c +6 -0

index 9dcb2a9..9af7425 100644
@@ -613,6 +613,12 @@ i40e_status i40e_init_adminq(struct i40e_hw *hw)
 		hw->flags |= I40E_HW_FLAG_AQ_PHY_ACCESS_CAPABLE;
 	}
 
+	/* Newer versions of firmware require lock when reading the NVM */
+	if (hw->aq.api_maj_ver > 1 ||
+	    (hw->aq.api_maj_ver == 1 &&
+	     hw->aq.api_min_ver >= 5))
+		hw->flags |= I40E_HW_FLAG_NVM_READ_REQUIRES_LOCK;
+
 	/* The ability to RX (not drop) 802.1ad frames was added in API 1.7 */
 	if (hw->aq.api_maj_ver > 1 ||
 	    (hw->aq.api_maj_ver == 1 &&

drivers/net/ethernet/intel/i40e/i40e_common.c +2 -1

index 0203665..13c7946 100644
@@ -948,7 +948,8 @@ i40e_status i40e_init_shared_code(struct i40e_hw *hw)
 		hw->pf_id = (u8)(func_rid & 0x7);
 
 	if (hw->mac.type == I40E_MAC_X722)
-		hw->flags |= I40E_HW_FLAG_AQ_SRCTL_ACCESS_ENABLE;
+		hw->flags |= I40E_HW_FLAG_AQ_SRCTL_ACCESS_ENABLE |
+			     I40E_HW_FLAG_NVM_READ_REQUIRES_LOCK;
 
 	status = i40e_init_nvm(hw);
 	return status;

drivers/net/ethernet/intel/i40e/i40e_nvm.c +5 -3

index 0ccab0a..7689c2e 100644
@@ -328,15 +328,17 @@ static i40e_status __i40e_read_nvm_word(struct
 i40e_hw *hw,
 i40e_status i40e_read_nvm_word(struct i40e_hw *hw, u16 offset,
 			       u16 *data)
 {
-	i40e_status ret_code;
+	i40e_status ret_code = 0;
 
-	ret_code = i40e_acquire_nvm(hw, I40E_RESOURCE_READ);
+	if (hw->flags & I40E_HW_FLAG_NVM_READ_REQUIRES_LOCK)
+		ret_code = i40e_acquire_nvm(hw, I40E_RESOURCE_READ);
 	if (ret_code)
 		return ret_code;
 
 	ret_code = __i40e_read_nvm_word(hw, offset, data);
 
-	i40e_release_nvm(hw);
+	if (hw->flags & I40E_HW_FLAG_NVM_READ_REQUIRES_LOCK)
+		i40e_release_nvm(hw);
 
 	return ret_code;

}

drivers/net/ethernet/intel/i40e/i40e_type.h +1 -1

index 00d4833..0e85687 100644
@@ -629,6 +629,7 @@ struct i40e_hw {
 #define I40E_HW_FLAG_AQ_SRCTL_ACCESS_ENABLE BIT_ULL(0)
 #define I40E_HW_FLAG_802_1AD_CAPABLE        BIT_ULL(1)
 #define I40E_HW_FLAG_AQ_PHY_ACCESS_CAPABLE  BIT_ULL(2)
+#define I40E_HW_FLAG_NVM_READ_REQUIRES_LOCK BIT_ULL(3)
 	u64 flags;
 
 	/* Used in set switch config AQ command */
--

2.9.5

Bowers

2017-10-31 19:07 UTC #15 lore raw

PATCH 8/9 i40e: Fix for NUP NVM image downgrade failure

In reply to Alice Michael

Quoted 22 lines

Quoted 7 lines

Signed-off-by Jacob Keller <[email protected]>

---
 drivers/net/ethernet/intel/i40e/i40e_adminq.c | 6 ++++++
drivers/net/ethernet/intel/i40e/i40e_common.c | 3 ++-
 drivers/net/ethernet/intel/i40e/i40e_nvm.c    | 8 +++++---
 drivers/net/ethernet/intel/i40e/i40e_type.h   | 1 +
 4 files changed, 14 insertions(+), 4 deletions(-)

Tested-by Andrew Bowers <[email protected]>

Alice Michael

author

2017-10-27 15:06 UTC #16 lore raw

PATCH 9/9 i40e/i40evf: Bump driver versions

In reply to Alice Michael

Bump the i40e driver from 2.1.14 to 2.3.2.

Bump the i40evf driver from 3.0.1 to 3.2.2

Signed-off-by Alice Michael <[email protected]>

---
 drivers/net/ethernet/intel/i40e/i40e_main.c     | 4 ++--
 drivers/net/ethernet/intel/i40evf/i40evf_main.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

drivers/net/ethernet/intel/i40e/i40e_main.c +2 -2

index 5b1bd3c..70ba48a 100644
@@ -47,8 +47,8 @@ static const char i40e_driver_string[] =
 #define DRV_KERN "-k"
 
 #define DRV_VERSION_MAJOR 2
-#define DRV_VERSION_MINOR 1
-#define DRV_VERSION_BUILD 14
+#define DRV_VERSION_MINOR 3
+#define DRV_VERSION_BUILD 2
 #define DRV_VERSION __stringify(DRV_VERSION_MAJOR) "." \
 	     __stringify(DRV_VERSION_MINOR) "." \
 	     __stringify(DRV_VERSION_BUILD)    DRV_KERN

drivers/net/ethernet/intel/i40evf/i40evf_main.c +2 -2

index 45cff72..75dc94f 100644
@@ -45,8 +45,8 @@ static const char i40evf_driver_string[] =
 #define DRV_KERN "-k"
 
 #define DRV_VERSION_MAJOR 3
-#define DRV_VERSION_MINOR 0
-#define DRV_VERSION_BUILD 1
+#define DRV_VERSION_MINOR 2
+#define DRV_VERSION_BUILD 2
 #define DRV_VERSION __stringify(DRV_VERSION_MAJOR) "." \
 	     __stringify(DRV_VERSION_MINOR) "." \
 	     __stringify(DRV_VERSION_BUILD) \

Signature · 1 line

2.9.5

Bowers

2017-10-30 22:04 UTC #17 lore raw

PATCH 9/9 i40e/i40evf: Bump driver versions

Quoted 11 lines

Quoted 7 lines

Tested-by Andrew Bowers <[email protected]>

Bowers

2017-10-30 21:52 UTC #18 lore raw

PATCH 1/9 i40e: display priority_xon and priority_xoff stats

In reply to Alice Michael

Quoted 9 lines

Quoted 7 lines

Tested-by Andrew Bowers <[email protected]>