[Intel-wired-lan] [PATCH net v3 0/4] iavf: fix reset task deadlock

[Intel-wired-lan] [PATCH net v3 0/4] iavf: fix reset task deadlock

[Kamil Maziarz]

Changing the way we handle resets so that callbacks operating under the RTNL lock will wait for the lock to be released.
This will eliminate circular dependency with the critical lock.

Marcin Szycik (4):
  iavf: Wait for reset in callbacks which trigger it
  iavf: Don't lock rtnl_lock twice in reset
  Revert "iavf: Detach device during reset task"
  Revert "iavf: Do not restart Tx queues after reset task failure"
---
v2: squashed "iavf: Add waiting for reset to finish in iavf_change_mtu" commit
---
v3: changes described under each commit separately
---
 drivers/net/ethernet/intel/iavf/iavf.h        |   3 +
 .../net/ethernet/intel/iavf/iavf_ethtool.c    |  31 +++--
 drivers/net/ethernet/intel/iavf/iavf_main.c   | 115 +++++++++++++-----
 .../net/ethernet/intel/iavf/iavf_virtchnl.c   |   1 +
 4 files changed, 102 insertions(+), 48 deletions(-)

-- 
2.31.1

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [PATCH net v3 1/4] iavf: Wait for reset in callbacks which trigger it

[Kamil Maziarz]

From: Marcin Szycik <marcin.szycik@linux.intel.com>

There was a fail when trying to add the interface to bonding
right after changing the MTU on the interface. It was caused
by bonding interface unable to open the interface due to
interface being in __RESETTING state because of MTU change.

Add new reset_waitqueue to indicate that reset has finished.
Add waiting for reset to finish in .

Add waiting for reset to finish in callbacks which trigger hw reset:
iavf_set_priv_flags(), iavf_change_mtu() and iavf_set_ringparam().
We choose 5 sec for the timeout waiting time because the same interval
is used in oot where waiting

Add a function iavf_wait_for_reset() to reuse waiting for reset code and
use it also in iavf_set_channels(), which already waits for reset.
We don't use error handling in iavf_set_channels() as this could
cause the device to be in incorrect state if the reset was scheduled
but hit timeout or the waitng function was interrupted by a signal.

We use a 5000ms timeout period because on Hyper-V based systems,
this operation takes around 3000-4000ms. In normal circumstances,
it doesn't take more than 500ms to complete.

Fixes: aa626da947e9 ("iavf: Detach device during reset task")
Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
Co-developed-by: Dawid Wesierski <dawidx.wesierski@intel.com>
Signed-off-by: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
Signed-off-by: Dawid Wesierski <dawidx.wesierski@intel.com>
Signed-off-by: Kamil Maziarz <kamil.maziarz@intel.com>
---
v2: commit iavf: add waiting for reset to finish in iavf_change_mtu  was squashed also added the information about the timeout length origin
---
v3: no changes
---
 drivers/net/ethernet/intel/iavf/iavf.h        |  2 +
 .../net/ethernet/intel/iavf/iavf_ethtool.c    | 31 ++++++-----
 drivers/net/ethernet/intel/iavf/iavf_main.c   | 51 ++++++++++++++++++-
 .../net/ethernet/intel/iavf/iavf_virtchnl.c   |  1 +
 4 files changed, 68 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf.h b/drivers/net/ethernet/intel/iavf/iavf.h
index 746ff76f2fb1..47f777674087 100644
--- a/drivers/net/ethernet/intel/iavf/iavf.h
+++ b/drivers/net/ethernet/intel/iavf/iavf.h
@@ -258,6 +258,7 @@ struct iavf_adapter {
 	struct work_struct adminq_task;
 	struct delayed_work client_task;
 	wait_queue_head_t down_waitqueue;
+	wait_queue_head_t reset_waitqueue;
 	wait_queue_head_t vc_waitqueue;
 	struct iavf_q_vector *q_vectors;
 	struct list_head vlan_filter_list;
@@ -592,5 +593,6 @@ void iavf_add_adv_rss_cfg(struct iavf_adapter *adapter);
 void iavf_del_adv_rss_cfg(struct iavf_adapter *adapter);
 struct iavf_mac_filter *iavf_add_filter(struct iavf_adapter *adapter,
 					const u8 *macaddr);
+int iavf_wait_for_reset(struct iavf_adapter *adapter);
 int iavf_lock_timeout(struct mutex *lock, unsigned int msecs);
 #endif /* _IAVF_H_ */
diff --git a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
index 6f171d1d85b7..b7141c2a941d 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
@@ -484,6 +484,7 @@ static int iavf_set_priv_flags(struct net_device *netdev, u32 flags)
 {
 	struct iavf_adapter *adapter = netdev_priv(netdev);
 	u32 orig_flags, new_flags, changed_flags;
+	int ret = 0;
 	u32 i;
 
 	orig_flags = READ_ONCE(adapter->flags);
@@ -533,10 +534,13 @@ static int iavf_set_priv_flags(struct net_device *netdev, u32 flags)
 		if (netif_running(netdev)) {
 			adapter->flags |= IAVF_FLAG_RESET_NEEDED;
 			queue_work(adapter->wq, &adapter->reset_task);
+			ret = iavf_wait_for_reset(adapter);
+			if (ret)
+				netdev_warn(netdev, "Changing private flags timeout or interrupted waiting for reset");
 		}
 	}
 
-	return 0;
+	return ret;
 }
 
 /**
@@ -627,6 +631,7 @@ static int iavf_set_ringparam(struct net_device *netdev,
 {
 	struct iavf_adapter *adapter = netdev_priv(netdev);
 	u32 new_rx_count, new_tx_count;
+	int ret = 0;
 
 	if ((ring->rx_mini_pending) || (ring->rx_jumbo_pending))
 		return -EINVAL;
@@ -673,9 +678,12 @@ static int iavf_set_ringparam(struct net_device *netdev,
 	if (netif_running(netdev)) {
 		adapter->flags |= IAVF_FLAG_RESET_NEEDED;
 		queue_work(adapter->wq, &adapter->reset_task);
+		ret = iavf_wait_for_reset(adapter);
+		if (ret)
+			netdev_warn(netdev, "Changing ring parameters timeout or interrupted waiting for reset");
 	}
 
-	return 0;
+	return ret;
 }
 
 /**
@@ -1830,7 +1838,7 @@ static int iavf_set_channels(struct net_device *netdev,
 {
 	struct iavf_adapter *adapter = netdev_priv(netdev);
 	u32 num_req = ch->combined_count;
-	int i;
+	int ret = 0;
 
 	if ((adapter->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_ADQ) &&
 	    adapter->num_tc) {
@@ -1854,20 +1862,11 @@ static int iavf_set_channels(struct net_device *netdev,
 	adapter->flags |= IAVF_FLAG_REINIT_ITR_NEEDED;
 	iavf_schedule_reset(adapter);
 
-	/* wait for the reset is done */
-	for (i = 0; i < IAVF_RESET_WAIT_COMPLETE_COUNT; i++) {
-		msleep(IAVF_RESET_WAIT_MS);
-		if (adapter->flags & IAVF_FLAG_RESET_PENDING)
-			continue;
-		break;
-	}
-	if (i == IAVF_RESET_WAIT_COMPLETE_COUNT) {
-		adapter->flags &= ~IAVF_FLAG_REINIT_ITR_NEEDED;
-		adapter->num_active_queues = num_req;
-		return -EOPNOTSUPP;
-	}
+	ret = iavf_wait_for_reset(adapter);
+	if (ret)
+		netdev_warn(netdev, "Changing channel count timeout or interrupted waiting for reset");
 
-	return 0;
+	return ret;
 }
 
 /**
diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 2de4baff4c20..dcf4232ba1ca 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -166,6 +166,45 @@ static struct iavf_adapter *iavf_pdev_to_adapter(struct pci_dev *pdev)
 	return netdev_priv(pci_get_drvdata(pdev));
 }
 
+/**
+ * iavf_is_reset_in_progress - Check if a reset is in progress
+ * @adapter: board private structure
+ */
+static bool iavf_is_reset_in_progress(struct iavf_adapter *adapter)
+{
+	if (adapter->state == __IAVF_RESETTING ||
+	    adapter->flags & (IAVF_FLAG_RESET_PENDING |
+			      IAVF_FLAG_RESET_NEEDED))
+		return true;
+
+	return false;
+}
+
+/**
+ * iavf_wait_for_reset - Wait for reset to finish.
+ * @adapter: board private structure
+ *
+ * Returns 0 if reset finished successfully, negative on timeout or interrupt.
+ */
+int iavf_wait_for_reset(struct iavf_adapter *adapter)
+{
+	int ret = wait_event_interruptible_timeout(adapter->reset_waitqueue,
+						   !iavf_is_reset_in_progress(adapter),
+						   msecs_to_jiffies(5000));
+
+	/* If ret < 0 then it means wait was interrupted.
+	 * If ret == 0 then it means we got a timeout while waiting
+	 * for reset to finish.
+	 * If ret > 0 it means reset has finished.
+	 */
+	if (ret > 0)
+		return 0;
+	else if (ret < 0)
+		return -EINTR;
+	else
+		return -EBUSY;
+}
+
 /**
  * iavf_allocate_dma_mem_d - OS specific memory alloc for shared code
  * @hw:   pointer to the HW structure
@@ -3162,6 +3201,7 @@ static void iavf_reset_task(struct work_struct *work)
 	} else {
 		iavf_change_state(adapter, __IAVF_DOWN);
 		wake_up(&adapter->down_waitqueue);
+		wake_up(&adapter->reset_waitqueue);
 	}
 
 	adapter->flags &= ~IAVF_FLAG_REINIT_ITR_NEEDED;
@@ -4330,6 +4370,7 @@ static int iavf_close(struct net_device *netdev)
 static int iavf_change_mtu(struct net_device *netdev, int new_mtu)
 {
 	struct iavf_adapter *adapter = netdev_priv(netdev);
+	int ret = 0;
 
 	netdev_dbg(netdev, "changing MTU from %d to %d\n",
 		   netdev->mtu, new_mtu);
@@ -4342,9 +4383,14 @@ static int iavf_change_mtu(struct net_device *netdev, int new_mtu)
 	if (netif_running(netdev)) {
 		adapter->flags |= IAVF_FLAG_RESET_NEEDED;
 		queue_work(adapter->wq, &adapter->reset_task);
+		ret = iavf_wait_for_reset(adapter);
+		if (ret < 0)
+			netdev_warn(netdev, "MTU change interrupted waiting for reset");
+		else if (ret)
+			netdev_warn(netdev, "MTU change timed out waiting for reset");
 	}
 
-	return 0;
+	return ret;
 }
 
 #define NETIF_VLAN_OFFLOAD_FEATURES	(NETIF_F_HW_VLAN_CTAG_RX | \
@@ -4945,6 +4991,9 @@ static int iavf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	/* Setup the wait queue for indicating transition to down status */
 	init_waitqueue_head(&adapter->down_waitqueue);
 
+	/* Setup the wait queue for indicating transition to running state */
+	init_waitqueue_head(&adapter->reset_waitqueue);
+
 	/* Setup the wait queue for indicating virtchannel events */
 	init_waitqueue_head(&adapter->vc_waitqueue);
 
diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
index 7c0578b5457b..1bab896aaf40 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
@@ -2285,6 +2285,7 @@ void iavf_virtchnl_completion(struct iavf_adapter *adapter,
 	case VIRTCHNL_OP_ENABLE_QUEUES:
 		/* enable transmits */
 		iavf_irq_enable(adapter, true);
+		wake_up(&adapter->reset_waitqueue);
 		adapter->flags &= ~IAVF_FLAG_QUEUES_DISABLED;
 		break;
 	case VIRTCHNL_OP_DISABLE_QUEUES:
-- 
2.31.1

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v3 1/4] iavf: Wait for reset in callbacks which trigger it

[Jacob Keller]

On 4/19/2023 4:50 AM, Kamil Maziarz wrote:
> From: Marcin Szycik <marcin.szycik@linux.intel.com>
> 
> There was a fail when trying to add the interface to bonding
> right after changing the MTU on the interface. It was caused
> by bonding interface unable to open the interface due to
> interface being in __RESETTING state because of MTU change.
> 
> Add new reset_waitqueue to indicate that reset has finished.
> Add waiting for reset to finish in .
> 
> Add waiting for reset to finish in callbacks which trigger hw reset:
> iavf_set_priv_flags(), iavf_change_mtu() and iavf_set_ringparam().
> We choose 5 sec for the timeout waiting time because the same interval
> is used in oot where waiting
> 
> Add a function iavf_wait_for_reset() to reuse waiting for reset code and
> use it also in iavf_set_channels(), which already waits for reset.
> We don't use error handling in iavf_set_channels() as this could
> cause the device to be in incorrect state if the reset was scheduled
> but hit timeout or the waitng function was interrupted by a signal.
> 
> We use a 5000ms timeout period because on Hyper-V based systems,
> this operation takes around 3000-4000ms. In normal circumstances,
> it doesn't take more than 500ms to complete.
> 
> Fixes: aa626da947e9 ("iavf: Detach device during reset task")
> Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
> Co-developed-by: Dawid Wesierski <dawidx.wesierski@intel.com>
> Signed-off-by: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
> Signed-off-by: Dawid Wesierski <dawidx.wesierski@intel.com>
> Signed-off-by: Kamil Maziarz <kamil.maziarz@intel.com>

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

> ---
> v2: commit iavf: add waiting for reset to finish in iavf_change_mtu  was squashed also added the information about the timeout length origin
> ---
> v3: no changes
> ---
>  drivers/net/ethernet/intel/iavf/iavf.h        |  2 +
>  .../net/ethernet/intel/iavf/iavf_ethtool.c    | 31 ++++++-----
>  drivers/net/ethernet/intel/iavf/iavf_main.c   | 51 ++++++++++++++++++-
>  .../net/ethernet/intel/iavf/iavf_virtchnl.c   |  1 +
>  4 files changed, 68 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/iavf/iavf.h b/drivers/net/ethernet/intel/iavf/iavf.h
> index 746ff76f2fb1..47f777674087 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf.h
> +++ b/drivers/net/ethernet/intel/iavf/iavf.h
> @@ -258,6 +258,7 @@ struct iavf_adapter {
>  	struct work_struct adminq_task;
>  	struct delayed_work client_task;
>  	wait_queue_head_t down_waitqueue;
> +	wait_queue_head_t reset_waitqueue;
>  	wait_queue_head_t vc_waitqueue;
>  	struct iavf_q_vector *q_vectors;
>  	struct list_head vlan_filter_list;
> @@ -592,5 +593,6 @@ void iavf_add_adv_rss_cfg(struct iavf_adapter *adapter);
>  void iavf_del_adv_rss_cfg(struct iavf_adapter *adapter);
>  struct iavf_mac_filter *iavf_add_filter(struct iavf_adapter *adapter,
>  					const u8 *macaddr);
> +int iavf_wait_for_reset(struct iavf_adapter *adapter);
>  int iavf_lock_timeout(struct mutex *lock, unsigned int msecs);
>  #endif /* _IAVF_H_ */
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> index 6f171d1d85b7..b7141c2a941d 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> @@ -484,6 +484,7 @@ static int iavf_set_priv_flags(struct net_device *netdev, u32 flags)
>  {
>  	struct iavf_adapter *adapter = netdev_priv(netdev);
>  	u32 orig_flags, new_flags, changed_flags;
> +	int ret = 0;
>  	u32 i;
>  
>  	orig_flags = READ_ONCE(adapter->flags);
> @@ -533,10 +534,13 @@ static int iavf_set_priv_flags(struct net_device *netdev, u32 flags)
>  		if (netif_running(netdev)) {
>  			adapter->flags |= IAVF_FLAG_RESET_NEEDED;
>  			queue_work(adapter->wq, &adapter->reset_task);
> +			ret = iavf_wait_for_reset(adapter);
> +			if (ret)
> +				netdev_warn(netdev, "Changing private flags timeout or interrupted waiting for reset");
>  		}
>  	}
>  
> -	return 0;
> +	return ret;
>  }
>  
>  /**
> @@ -627,6 +631,7 @@ static int iavf_set_ringparam(struct net_device *netdev,
>  {
>  	struct iavf_adapter *adapter = netdev_priv(netdev);
>  	u32 new_rx_count, new_tx_count;
> +	int ret = 0;
>  
>  	if ((ring->rx_mini_pending) || (ring->rx_jumbo_pending))
>  		return -EINVAL;
> @@ -673,9 +678,12 @@ static int iavf_set_ringparam(struct net_device *netdev,
>  	if (netif_running(netdev)) {
>  		adapter->flags |= IAVF_FLAG_RESET_NEEDED;
>  		queue_work(adapter->wq, &adapter->reset_task);
> +		ret = iavf_wait_for_reset(adapter);
> +		if (ret)
> +			netdev_warn(netdev, "Changing ring parameters timeout or interrupted waiting for reset");
>  	}
>  
> -	return 0;
> +	return ret;
>  }
>  
>  /**
> @@ -1830,7 +1838,7 @@ static int iavf_set_channels(struct net_device *netdev,
>  {
>  	struct iavf_adapter *adapter = netdev_priv(netdev);
>  	u32 num_req = ch->combined_count;
> -	int i;
> +	int ret = 0;
>  
>  	if ((adapter->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_ADQ) &&
>  	    adapter->num_tc) {
> @@ -1854,20 +1862,11 @@ static int iavf_set_channels(struct net_device *netdev,
>  	adapter->flags |= IAVF_FLAG_REINIT_ITR_NEEDED;
>  	iavf_schedule_reset(adapter);
>  
> -	/* wait for the reset is done */
> -	for (i = 0; i < IAVF_RESET_WAIT_COMPLETE_COUNT; i++) {
> -		msleep(IAVF_RESET_WAIT_MS);
> -		if (adapter->flags & IAVF_FLAG_RESET_PENDING)
> -			continue;
> -		break;
> -	}
> -	if (i == IAVF_RESET_WAIT_COMPLETE_COUNT) {
> -		adapter->flags &= ~IAVF_FLAG_REINIT_ITR_NEEDED;
> -		adapter->num_active_queues = num_req;
> -		return -EOPNOTSUPP;
> -	}
> +	ret = iavf_wait_for_reset(adapter);
> +	if (ret)
> +		netdev_warn(netdev, "Changing channel count timeout or interrupted waiting for reset");
>  
> -	return 0;
> +	return ret;
>  }
>  
>  /**
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 2de4baff4c20..dcf4232ba1ca 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -166,6 +166,45 @@ static struct iavf_adapter *iavf_pdev_to_adapter(struct pci_dev *pdev)
>  	return netdev_priv(pci_get_drvdata(pdev));
>  }
>  
> +/**
> + * iavf_is_reset_in_progress - Check if a reset is in progress
> + * @adapter: board private structure
> + */
> +static bool iavf_is_reset_in_progress(struct iavf_adapter *adapter)
> +{
> +	if (adapter->state == __IAVF_RESETTING ||
> +	    adapter->flags & (IAVF_FLAG_RESET_PENDING |
> +			      IAVF_FLAG_RESET_NEEDED))
> +		return true;
> +
> +	return false;
> +}
> +
> +/**
> + * iavf_wait_for_reset - Wait for reset to finish.
> + * @adapter: board private structure
> + *
> + * Returns 0 if reset finished successfully, negative on timeout or interrupt.
> + */
> +int iavf_wait_for_reset(struct iavf_adapter *adapter)
> +{
> +	int ret = wait_event_interruptible_timeout(adapter->reset_waitqueue,
> +						   !iavf_is_reset_in_progress(adapter),
> +						   msecs_to_jiffies(5000));
> +
> +	/* If ret < 0 then it means wait was interrupted.
> +	 * If ret == 0 then it means we got a timeout while waiting
> +	 * for reset to finish.
> +	 * If ret > 0 it means reset has finished.
> +	 */
> +	if (ret > 0)
> +		return 0;
> +	else if (ret < 0)
> +		return -EINTR;
> +	else
> +		return -EBUSY;
> +}
> +
>  /**
>   * iavf_allocate_dma_mem_d - OS specific memory alloc for shared code
>   * @hw:   pointer to the HW structure
> @@ -3162,6 +3201,7 @@ static void iavf_reset_task(struct work_struct *work)
>  	} else {
>  		iavf_change_state(adapter, __IAVF_DOWN);
>  		wake_up(&adapter->down_waitqueue);
> +		wake_up(&adapter->reset_waitqueue);
>  	}
>  
>  	adapter->flags &= ~IAVF_FLAG_REINIT_ITR_NEEDED;
> @@ -4330,6 +4370,7 @@ static int iavf_close(struct net_device *netdev)
>  static int iavf_change_mtu(struct net_device *netdev, int new_mtu)
>  {
>  	struct iavf_adapter *adapter = netdev_priv(netdev);
> +	int ret = 0;
>  
>  	netdev_dbg(netdev, "changing MTU from %d to %d\n",
>  		   netdev->mtu, new_mtu);
> @@ -4342,9 +4383,14 @@ static int iavf_change_mtu(struct net_device *netdev, int new_mtu)
>  	if (netif_running(netdev)) {
>  		adapter->flags |= IAVF_FLAG_RESET_NEEDED;
>  		queue_work(adapter->wq, &adapter->reset_task);
> +		ret = iavf_wait_for_reset(adapter);
> +		if (ret < 0)
> +			netdev_warn(netdev, "MTU change interrupted waiting for reset");
> +		else if (ret)
> +			netdev_warn(netdev, "MTU change timed out waiting for reset");
>  	}
>  
> -	return 0;
> +	return ret;
>  }
>  
>  #define NETIF_VLAN_OFFLOAD_FEATURES	(NETIF_F_HW_VLAN_CTAG_RX | \
> @@ -4945,6 +4991,9 @@ static int iavf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	/* Setup the wait queue for indicating transition to down status */
>  	init_waitqueue_head(&adapter->down_waitqueue);
>  
> +	/* Setup the wait queue for indicating transition to running state */
> +	init_waitqueue_head(&adapter->reset_waitqueue);
> +
>  	/* Setup the wait queue for indicating virtchannel events */
>  	init_waitqueue_head(&adapter->vc_waitqueue);
>  
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> index 7c0578b5457b..1bab896aaf40 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> @@ -2285,6 +2285,7 @@ void iavf_virtchnl_completion(struct iavf_adapter *adapter,
>  	case VIRTCHNL_OP_ENABLE_QUEUES:
>  		/* enable transmits */
>  		iavf_irq_enable(adapter, true);
> +		wake_up(&adapter->reset_waitqueue);
>  		adapter->flags &= ~IAVF_FLAG_QUEUES_DISABLED;
>  		break;
>  	case VIRTCHNL_OP_DISABLE_QUEUES:
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v3 1/4] iavf: Wait for reset in callbacks which trigger it

[Tony Nguyen]

On 4/19/2023 4:50 AM, Kamil Maziarz wrote:
> From: Marcin Szycik <marcin.szycik@linux.intel.com>
> 
> There was a fail when trying to add the interface to bonding
> right after changing the MTU on the interface. It was caused
> by bonding interface unable to open the interface due to
> interface being in __RESETTING state because of MTU change.
> 
> Add new reset_waitqueue to indicate that reset has finished.
> Add waiting for reset to finish in .
> 
> Add waiting for reset to finish in callbacks which trigger hw reset:
> iavf_set_priv_flags(), iavf_change_mtu() and iavf_set_ringparam().
> We choose 5 sec for the timeout waiting time because the same interval
> is used in oot where waiting
> 
> Add a function iavf_wait_for_reset() to reuse waiting for reset code and
> use it also in iavf_set_channels(), which already waits for reset.
> We don't use error handling in iavf_set_channels() as this could
> cause the device to be in incorrect state if the reset was scheduled
> but hit timeout or the waitng function was interrupted by a signal.
> 
> We use a 5000ms timeout period because on Hyper-V based systems,
> this operation takes around 3000-4000ms. In normal circumstances,
> it doesn't take more than 500ms to complete.
> 
> Fixes: aa626da947e9 ("iavf: Detach device during reset task")
> Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
> Co-developed-by: Dawid Wesierski <dawidx.wesierski@intel.com>

The sign-off directly following co-developed needs to be the same.

WARNING: Co-developed-by and Signed-off-by: name/email do not match

> Signed-off-by: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
> Signed-off-by: Dawid Wesierski <dawidx.wesierski@intel.com>
> Signed-off-by: Kamil Maziarz <kamil.maziarz@intel.com>

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [PATCH net v3 2/4] iavf: Don't lock rtnl_lock twice in reset

[Kamil Maziarz]

From: Marcin Szycik <marcin.szycik@linux.intel.com>

Some ndo/ethtool callbacks are called under rtnl_lock. If such callback
then triggers a reset, the reset task might try to take the rtnl_lock
again, causing a deadlock.

Callbacks that are sensitive to rtnl_lock are scheduled when the drivers
are unable to obtain the rtnl_lock in the reset flow. This ensures that
the reset task does not attempt to double lock rtnl_lock and avoids
the deadlock.

Before this patch, a deadlock could be caused by e.g.:

echo 1 > /sys/class/net/$PF1/device/sriov_numvfs
while :; do
ip l s $VF1 up
ethtool --set-channels $VF1 combined 8
ip l s $VF1 down
ip l s $VF1 up
ethtool --set-channels $VF1 combined 16
ip l s $VF1 down
done

Fixes: aa626da947e9 ("iavf: Detach device during reset task")
Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
Co-developed-by: Dawid Wesierski <dawidx.wesierski@intel.com>
Signed-off-by: Dawid Wesierski <dawidx.wesierski@intel.com>
Signed-off-by: Kamil Maziarz <kamil.maziarz@intel.com>
---
v2: no changes
---
v3: Instead of the flag, we are using the rtnl_trylock in the rtnl_lock sensitive functions in the iavf_reset to avoid deadlock.
    Adding the scheduling functionality so we can update the netdev later in that case.
---
 drivers/net/ethernet/intel/iavf/iavf.h      |  1 +
 drivers/net/ethernet/intel/iavf/iavf_main.c | 35 ++++++++++++++++++---
 2 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf.h b/drivers/net/ethernet/intel/iavf/iavf.h
index 47f777674087..8f48b5354025 100644
--- a/drivers/net/ethernet/intel/iavf/iavf.h
+++ b/drivers/net/ethernet/intel/iavf/iavf.h
@@ -256,6 +256,7 @@ struct iavf_adapter {
 	struct workqueue_struct *wq;
 	struct work_struct reset_task;
 	struct work_struct adminq_task;
+	struct work_struct set_interrupt_capability;
 	struct delayed_work client_task;
 	wait_queue_head_t down_waitqueue;
 	wait_queue_head_t reset_waitqueue;
diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index dcf4232ba1ca..7bcf422c0b5f 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -1707,11 +1707,36 @@ static int iavf_set_interrupt_capability(struct iavf_adapter *adapter)
 	err = iavf_acquire_msix_vectors(adapter, v_budget);
 
 out:
-	netif_set_real_num_rx_queues(adapter->netdev, pairs);
-	netif_set_real_num_tx_queues(adapter->netdev, pairs);
+	if (rtnl_trylock()) {
+		netif_set_real_num_rx_queues(adapter->netdev, pairs);
+		netif_set_real_num_tx_queues(adapter->netdev, pairs);
+		rtnl_unlock();
+	} else {
+		queue_work(adapter->wq, &adapter->set_interrupt_capability);
+	}
+
 	return err;
 }
 
+/**
+ * iavf_delayed_set_interrupt_capability - schedule the update of the netdev
+ * @work: pointer to work_struct containing our data
+ **/
+static void iavf_delayed_set_interrupt_capability(struct work_struct *work)
+{
+	struct iavf_adapter *adapter = container_of(work, struct iavf_adapter,
+                                                   set_interrupt_capability);
+	int pairs = adapter->num_active_queues;
+
+	if(rtnl_trylock()) {
+		netif_set_real_num_rx_queues(adapter->netdev, pairs);
+		netif_set_real_num_tx_queues(adapter->netdev, pairs);
+		rtnl_unlock();
+	} else {
+		queue_work(adapter->wq, &adapter->set_interrupt_capability);
+	}
+}
+
 /**
  * iavf_config_rss_aq - Configure RSS keys and lut by using AQ commands
  * @adapter: board private structure
@@ -1930,10 +1955,8 @@ int iavf_init_interrupt_scheme(struct iavf_adapter *adapter)
 			"Unable to allocate memory for queues\n");
 		goto err_alloc_queues;
 	}
-
-	rtnl_lock();
 	err = iavf_set_interrupt_capability(adapter);
-	rtnl_unlock();
+
 	if (err) {
 		dev_err(&adapter->pdev->dev,
 			"Unable to setup interrupt capabilities\n");
@@ -4983,6 +5006,7 @@ static int iavf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	INIT_WORK(&adapter->reset_task, iavf_reset_task);
 	INIT_WORK(&adapter->adminq_task, iavf_adminq_task);
+	INIT_WORK(&adapter->set_interrupt_capability, iavf_delayed_set_interrupt_capability);
 	INIT_DELAYED_WORK(&adapter->watchdog_task, iavf_watchdog_task);
 	INIT_DELAYED_WORK(&adapter->client_task, iavf_client_task);
 	queue_delayed_work(adapter->wq, &adapter->watchdog_task,
@@ -5156,6 +5180,7 @@ static void iavf_remove(struct pci_dev *pdev)
 	cancel_work_sync(&adapter->reset_task);
 	cancel_delayed_work_sync(&adapter->watchdog_task);
 	cancel_work_sync(&adapter->adminq_task);
+	cancel_work_sync(&adapter->set_interrupt_capability);
 	cancel_delayed_work_sync(&adapter->client_task);
 
 	adapter->aq_required = 0;
-- 
2.31.1

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v3 2/4] iavf: Don't lock rtnl_lock twice in reset

[Jacob Keller]

On 4/19/2023 4:50 AM, Kamil Maziarz wrote:
> From: Marcin Szycik <marcin.szycik@linux.intel.com>
> 
> Some ndo/ethtool callbacks are called under rtnl_lock. If such callback
> then triggers a reset, the reset task might try to take the rtnl_lock
> again, causing a deadlock.
> 
> Callbacks that are sensitive to rtnl_lock are scheduled when the drivers
> are unable to obtain the rtnl_lock in the reset flow. This ensures that
> the reset task does not attempt to double lock rtnl_lock and avoids
> the deadlock.
> 
> Before this patch, a deadlock could be caused by e.g.:
> 
> echo 1 > /sys/class/net/$PF1/device/sriov_numvfs
> while :; do
> ip l s $VF1 up
> ethtool --set-channels $VF1 combined 8
> ip l s $VF1 down
> ip l s $VF1 up
> ethtool --set-channels $VF1 combined 16
> ip l s $VF1 down
> done
> 
> Fixes: aa626da947e9 ("iavf: Detach device during reset task")
> Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
> Co-developed-by: Dawid Wesierski <dawidx.wesierski@intel.com>
> Signed-off-by: Dawid Wesierski <dawidx.wesierski@intel.com>
> Signed-off-by: Kamil Maziarz <kamil.maziarz@intel.com>
> ---
> v2: no changes
> ---
> v3: Instead of the flag, we are using the rtnl_trylock in the rtnl_lock sensitive functions in the iavf_reset to avoid deadlock.
>     Adding the scheduling functionality so we can update the netdev later in that case.
>  out:
> -	netif_set_real_num_rx_queues(adapter->netdev, pairs);
> -	netif_set_real_num_tx_queues(adapter->netdev, pairs);
> +	if (rtnl_trylock()) {
> +		netif_set_real_num_rx_queues(adapter->netdev, pairs);
> +		netif_set_real_num_tx_queues(adapter->netdev, pairs);
> +		rtnl_unlock();
> +	} else {
> +		queue_work(adapter->wq, &adapter->set_interrupt_capability);
> +	}
> +

I guess there's some merit to doing it inline when possible, but I'd
argue for simplicity of just always scheduling the work item. The
trylock is at least safe. I'm ok with either approach.

>  	return err;
>  }
>  
> +/**
> + * iavf_delayed_set_interrupt_capability - schedule the update of the netdev
> + * @work: pointer to work_struct containing our data
> + **/
> +static void iavf_delayed_set_interrupt_capability(struct work_struct *work)
> +{
> +	struct iavf_adapter *adapter = container_of(work, struct iavf_adapter,
> +                                                   set_interrupt_capability);
> +	int pairs = adapter->num_active_queues;
> +
> +	if(rtnl_trylock()) {
> +		netif_set_real_num_rx_queues(adapter->netdev, pairs);
> +		netif_set_real_num_tx_queues(adapter->netdev, pairs);
> +		rtnl_unlock();
> +	} else {
> +		queue_work(adapter->wq, &adapter->set_interrupt_capability);
> +	}
> +}

The delayed function is now in a thread where its safe to take RTNL
lock, (since no one is waiting on a different lock/lock bit/etc) so this
should just be "rtnl_lock() / *set_real_num* / rtnl_unlock(). I don't
see the benefit of re-queueing here. The update won't take long so I
don't think we're going to be starving the adapter work queue by locking
on it..
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v3 2/4] iavf: Don't lock rtnl_lock twice in reset

[Tony Nguyen]

On 4/19/2023 4:50 AM, Kamil Maziarz wrote:
> From: Marcin Szycik <marcin.szycik@linux.intel.com>
> 
> Some ndo/ethtool callbacks are called under rtnl_lock. If such callback
> then triggers a reset, the reset task might try to take the rtnl_lock
> again, causing a deadlock.
> 
> Callbacks that are sensitive to rtnl_lock are scheduled when the drivers
> are unable to obtain the rtnl_lock in the reset flow. This ensures that
> the reset task does not attempt to double lock rtnl_lock and avoids
> the deadlock.
> 
> Before this patch, a deadlock could be caused by e.g.:
> 
> echo 1 > /sys/class/net/$PF1/device/sriov_numvfs
> while :; do
> ip l s $VF1 up
> ethtool --set-channels $VF1 combined 8
> ip l s $VF1 down
> ip l s $VF1 up
> ethtool --set-channels $VF1 combined 16
> ip l s $VF1 down
> done
> 
> Fixes: aa626da947e9 ("iavf: Detach device during reset task")
> Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
> Co-developed-by: Dawid Wesierski <dawidx.wesierski@intel.com>
> Signed-off-by: Dawid Wesierski <dawidx.wesierski@intel.com>
> Signed-off-by: Kamil Maziarz <kamil.maziarz@intel.com>

Please be sure to check checkpatch before sending your patches:

ERROR: code indent should use tabs where possible
ERROR: space required before the open parenthesis '('
WARNING: please, no spaces at the start of a line
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [PATCH net v3 3/4] Revert "iavf: Detach device during reset task"

[Kamil Maziarz]

From: Marcin Szycik <marcin.szycik@linux.intel.com>

This reverts commit aa626da947e9cd30c4cf727493903e1adbb2c0a0.

Detaching device during reset was not fully fixing the rtnl locking issue,
as there could be a situation where callback was already in progress before
detaching netdev.

Furthermore, detaching netdevice causes TX timeouts if traffic is running.
To reproduce:

ip netns exec ns1 iperf3 -c $PEER_IP -t 600 --logfile /dev/null &
while :; do
        for i in 200 7000 400 5000 300 3000 ; do
		ip netns exec ns1 ip link set $VF1 mtu $i
                sleep 2
        done
        sleep 10
done

Currently callbacks such as iavf_change_mtu() wait for reset and don't try
to take rtnl_lock if they already run under rtnl_lock (flag
IAVF_FLAG_RTNL_LOCK_TAKEN), therefore rtnl_lock will be released when reset
finishes, and only then another callback which uses rtnl_lock will be able
to start.

Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
Signed-off-by: Kamil Maziarz <kamil.maziarz@intel.com>
---
v2: no changes
---
v3: no changes
---
 drivers/net/ethernet/intel/iavf/iavf_main.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 7bcf422c0b5f..8dd488158961 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -3017,11 +3017,6 @@ static void iavf_reset_task(struct work_struct *work)
 	int i = 0, err;
 	bool running;
 
-	/* Detach interface to avoid subsequent NDO callbacks */
-	rtnl_lock();
-	netif_device_detach(netdev);
-	rtnl_unlock();
-
 	/* When device is being removed it doesn't make sense to run the reset
 	 * task, just return in such a case.
 	 */
@@ -3029,7 +3024,7 @@ static void iavf_reset_task(struct work_struct *work)
 		if (adapter->state != __IAVF_REMOVE)
 			queue_work(adapter->wq, &adapter->reset_task);
 
-		goto reset_finish;
+		return;
 	}
 
 	while (!mutex_trylock(&adapter->client_lock))
@@ -3232,7 +3227,7 @@ static void iavf_reset_task(struct work_struct *work)
 	mutex_unlock(&adapter->client_lock);
 	mutex_unlock(&adapter->crit_lock);
 
-	goto reset_finish;
+	return;
 reset_err:
 	if (running) {
 		set_bit(__IAVF_VSI_DOWN, adapter->vsi.state);
@@ -3253,10 +3248,6 @@ static void iavf_reset_task(struct work_struct *work)
 	}
 
 	dev_err(&adapter->pdev->dev, "failed to allocate resources during reinit\n");
-reset_finish:
-	rtnl_lock();
-	netif_device_attach(netdev);
-	rtnl_unlock();
 }
 
 /**
-- 
2.31.1

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v3 3/4] Revert "iavf: Detach device during reset task"

[Jacob Keller]

On 4/19/2023 4:50 AM, Kamil Maziarz wrote:
> From: Marcin Szycik <marcin.szycik@linux.intel.com>
> 
> This reverts commit aa626da947e9cd30c4cf727493903e1adbb2c0a0.
> 
> Detaching device during reset was not fully fixing the rtnl locking issue,
> as there could be a situation where callback was already in progress before
> detaching netdev.
> 
> Furthermore, detaching netdevice causes TX timeouts if traffic is running.
> To reproduce:
> 
> ip netns exec ns1 iperf3 -c $PEER_IP -t 600 --logfile /dev/null &
> while :; do
>         for i in 200 7000 400 5000 300 3000 ; do
> 		ip netns exec ns1 ip link set $VF1 mtu $i
>                 sleep 2
>         done
>         sleep 10
> done
> 
> Currently callbacks such as iavf_change_mtu() wait for reset and don't try
> to take rtnl_lock if they already run under rtnl_lock (flag
> IAVF_FLAG_RTNL_LOCK_TAKEN), therefore rtnl_lock will be released when reset
> finishes, and only then another callback which uses rtnl_lock will be able
> to start.

This comment is no longer correct since we stopped using this flag, but
otherwise the patch content is fine.

> 
> Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
> Signed-off-by: Kamil Maziarz <kamil.maziarz@intel.com>
> ---
> v2: no changes
> ---
> v3: no changes
> ---
>  drivers/net/ethernet/intel/iavf/iavf_main.c | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 7bcf422c0b5f..8dd488158961 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -3017,11 +3017,6 @@ static void iavf_reset_task(struct work_struct *work)
>  	int i = 0, err;
>  	bool running;
>  
> -	/* Detach interface to avoid subsequent NDO callbacks */
> -	rtnl_lock();
> -	netif_device_detach(netdev);
> -	rtnl_unlock();
> -
>  	/* When device is being removed it doesn't make sense to run the reset
>  	 * task, just return in such a case.
>  	 */
> @@ -3029,7 +3024,7 @@ static void iavf_reset_task(struct work_struct *work)
>  		if (adapter->state != __IAVF_REMOVE)
>  			queue_work(adapter->wq, &adapter->reset_task);
>  
> -		goto reset_finish;
> +		return;
>  	}
>  
>  	while (!mutex_trylock(&adapter->client_lock))
> @@ -3232,7 +3227,7 @@ static void iavf_reset_task(struct work_struct *work)
>  	mutex_unlock(&adapter->client_lock);
>  	mutex_unlock(&adapter->crit_lock);
>  
> -	goto reset_finish;
> +	return;
>  reset_err:
>  	if (running) {
>  		set_bit(__IAVF_VSI_DOWN, adapter->vsi.state);
> @@ -3253,10 +3248,6 @@ static void iavf_reset_task(struct work_struct *work)
>  	}
>  
>  	dev_err(&adapter->pdev->dev, "failed to allocate resources during reinit\n");
> -reset_finish:
> -	rtnl_lock();
> -	netif_device_attach(netdev);
> -	rtnl_unlock();
>  }
>  
>  /**
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [PATCH net v3 4/4] Revert "iavf: Do not restart Tx queues after reset task failure"

[Kamil Maziarz]

From: Marcin Szycik <marcin.szycik@linux.intel.com>

This reverts commit 08f1c147b7265245d67321585c68a27e990e0c4b.

Netdev is no longer being detached during reset, so this fix can be
reverted.

Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
Signed-off-by: Kamil Maziarz <kamil.maziarz@intel.com>
---
v2: no changes
---
v3: no changes
---
 drivers/net/ethernet/intel/iavf/iavf_main.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 8dd488158961..8f13685ed2fe 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -2988,6 +2988,7 @@ static void iavf_disable_vf(struct iavf_adapter *adapter)
 	iavf_free_queues(adapter);
 	memset(adapter->vf_res, 0, IAVF_VIRTCHNL_VF_RESOURCE_SIZE);
 	iavf_shutdown_adminq(&adapter->hw);
+	adapter->netdev->flags &= ~IFF_UP;
 	adapter->flags &= ~IAVF_FLAG_RESET_PENDING;
 	iavf_change_state(adapter, __IAVF_DOWN);
 	wake_up(&adapter->down_waitqueue);
@@ -3082,11 +3083,6 @@ static void iavf_reset_task(struct work_struct *work)
 		iavf_disable_vf(adapter);
 		mutex_unlock(&adapter->client_lock);
 		mutex_unlock(&adapter->crit_lock);
-		if (netif_running(netdev)) {
-			rtnl_lock();
-			dev_close(netdev);
-			rtnl_unlock();
-		}
 		return; /* Do not attempt to reinit. It's dead, Jim. */
 	}
 
@@ -3237,16 +3233,6 @@ static void iavf_reset_task(struct work_struct *work)
 
 	mutex_unlock(&adapter->client_lock);
 	mutex_unlock(&adapter->crit_lock);
-
-	if (netif_running(netdev)) {
-		/* Close device to ensure that Tx queues will not be started
-		 * during netif_device_attach() at the end of the reset task.
-		 */
-		rtnl_lock();
-		dev_close(netdev);
-		rtnl_unlock();
-	}
-
 	dev_err(&adapter->pdev->dev, "failed to allocate resources during reinit\n");
 }
 
-- 
2.31.1

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v3 4/4] Revert "iavf: Do not restart Tx queues after reset task failure"

[Jacob Keller]

On 4/19/2023 4:50 AM, Kamil Maziarz wrote:
> From: Marcin Szycik <marcin.szycik@linux.intel.com>
> 
> This reverts commit 08f1c147b7265245d67321585c68a27e990e0c4b.
> 
> Netdev is no longer being detached during reset, so this fix can be
> reverted.
> 
> Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
> Signed-off-by: Kamil Maziarz <kamil.maziarz@intel.com>

With the other fixes, this is good.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

> ---
> v2: no changes
> ---
> v3: no changes
> ---
>  drivers/net/ethernet/intel/iavf/iavf_main.c | 16 +---------------
>  1 file changed, 1 insertion(+), 15 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 8dd488158961..8f13685ed2fe 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -2988,6 +2988,7 @@ static void iavf_disable_vf(struct iavf_adapter *adapter)
>  	iavf_free_queues(adapter);
>  	memset(adapter->vf_res, 0, IAVF_VIRTCHNL_VF_RESOURCE_SIZE);
>  	iavf_shutdown_adminq(&adapter->hw);
> +	adapter->netdev->flags &= ~IFF_UP;
>  	adapter->flags &= ~IAVF_FLAG_RESET_PENDING;
>  	iavf_change_state(adapter, __IAVF_DOWN);
>  	wake_up(&adapter->down_waitqueue);
> @@ -3082,11 +3083,6 @@ static void iavf_reset_task(struct work_struct *work)
>  		iavf_disable_vf(adapter);
>  		mutex_unlock(&adapter->client_lock);
>  		mutex_unlock(&adapter->crit_lock);
> -		if (netif_running(netdev)) {
> -			rtnl_lock();
> -			dev_close(netdev);
> -			rtnl_unlock();
> -		}
>  		return; /* Do not attempt to reinit. It's dead, Jim. */
>  	}
>  
> @@ -3237,16 +3233,6 @@ static void iavf_reset_task(struct work_struct *work)
>  
>  	mutex_unlock(&adapter->client_lock);
>  	mutex_unlock(&adapter->crit_lock);
> -
> -	if (netif_running(netdev)) {
> -		/* Close device to ensure that Tx queues will not be started
> -		 * during netif_device_attach() at the end of the reset task.
> -		 */
> -		rtnl_lock();
> -		dev_close(netdev);
> -		rtnl_unlock();
> -	}
> -
>  	dev_err(&adapter->pdev->dev, "failed to allocate resources during reinit\n");
>  }
>  
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v3 4/4] Revert "iavf: Do not restart Tx queues after reset task failure"

[Jacob Keller]

On 4/19/2023 11:42 AM, Jacob Keller wrote:
> 
> 
> On 4/19/2023 4:50 AM, Kamil Maziarz wrote:
>> From: Marcin Szycik <marcin.szycik@linux.intel.com>
>>
>> This reverts commit 08f1c147b7265245d67321585c68a27e990e0c4b.
>>
>> Netdev is no longer being detached during reset, so this fix can be
>> reverted.
>>
>> Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
>> Signed-off-by: Kamil Maziarz <kamil.maziarz@intel.com>
> 
> With the other fixes, this is good.
> 
> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
> 
>> ---
>> v2: no changes
>> ---
>> v3: no changes
>> ---
>>  drivers/net/ethernet/intel/iavf/iavf_main.c | 16 +---------------
>>  1 file changed, 1 insertion(+), 15 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
>> index 8dd488158961..8f13685ed2fe 100644
>> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
>> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
>> @@ -2988,6 +2988,7 @@ static void iavf_disable_vf(struct iavf_adapter *adapter)
>>  	iavf_free_queues(adapter);
>>  	memset(adapter->vf_res, 0, IAVF_VIRTCHNL_VF_RESOURCE_SIZE);
>>  	iavf_shutdown_adminq(&adapter->hw);
>> +	adapter->netdev->flags &= ~IFF_UP;

Wait. We must not modify netdev->flags like this. Please do not restore
this part of the commit. Perhaps a new commit message with a partial
revert explanation. Modification of the netdev flags like this is incorrect.

See the original commit message which said:

    Also replace the hacky manipulation with IFF_UP flag by device close
    that clears properly both IFF_UP and __LINK_STATE_START flags.
    In these case iavf_close() does not do anything because the adapter
    state is already __IAVF_DOWN.

NAK this change as-is.

It may be no longer required to perform the other parts of the patch,
but I do not accept re-adding the incorrect IFF_UP modification.

Thanks,
Jake
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v3 0/4] iavf: fix reset task deadlock

[Jacob Keller]

On 4/19/2023 4:50 AM, Kamil Maziarz wrote:
> Changing the way we handle resets so that callbacks operating under the RTNL lock will wait for the lock to be released.
> This will eliminate circular dependency with the critical lock.
> 
> Marcin Szycik (4):
>   iavf: Wait for reset in callbacks which trigger it
>   iavf: Don't lock rtnl_lock twice in reset
>   Revert "iavf: Detach device during reset task"
>   Revert "iavf: Do not restart Tx queues after reset task failure"
> ---

Thanks, this is the better direction I think. I have one request but
otherwise this looks good.

> v2: squashed "iavf: Add waiting for reset to finish in iavf_change_mtu" commit
> ---
> v3: changes described under each commit separately
> ---
>  drivers/net/ethernet/intel/iavf/iavf.h        |   3 +
>  .../net/ethernet/intel/iavf/iavf_ethtool.c    |  31 +++--
>  drivers/net/ethernet/intel/iavf/iavf_main.c   | 115 +++++++++++++-----
>  .../net/ethernet/intel/iavf/iavf_virtchnl.c   |   1 +
>  4 files changed, 102 insertions(+), 48 deletions(-)
> 
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan