diff mbox series

[net,2/2] iavf: avoid taking rtnl_lock in adminq_task

Message ID 20221215225049.508812-3-mschmidt@redhat.com (mailing list archive)
State Awaiting Upstream
Delegated to: Netdev Maintainers
Headers show
Series iavf: fix temporary deadlock and failure to set MAC address | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers fail 2 blamed authors not CCed: sylwesterx.dziedziuch@intel.com anthony.l.nguyen@intel.com; 7 maintainers not CCed: edumazet@google.com sylwesterx.dziedziuch@intel.com davem@davemloft.net kuba@kernel.org pabeni@redhat.com anthony.l.nguyen@intel.com jesse.brandeburg@intel.com
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 105 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Michal Schmidt Dec. 15, 2022, 10:50 p.m. UTC
adminq_task processes virtchnl completions. iavf_set_mac() needs
virtchnl communication to progress while it holds rtnl_lock.
So adminq_task must not take rtnl_lock.

Do the handling of netdev features updates in a new work, features_task.
The new work cannot run on the same ordered workqueue as adminq_task.
The system-wide system_unbound_wq will do.

iavf_set_queue_vlan_tag_loc(), which iterates through queues, must run
under crit_lock to prevent a concurrent iavf_free_queues() possibly
called from watchdog_task or reset_task.

IAVF_FLAG_SETUP_NETDEV_FEATURES becomes unnecessary. features_task can
be queued directly from iavf_virtchnl_completion().

Fixes: 35a2443d0910 ("iavf: Add waiting for response from PF in set mac")
Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
 drivers/net/ethernet/intel/iavf/iavf.h        |  2 +-
 drivers/net/ethernet/intel/iavf/iavf_main.c   | 49 ++++++++++++-------
 .../net/ethernet/intel/iavf/iavf_virtchnl.c   |  6 ++-
 3 files changed, 37 insertions(+), 20 deletions(-)
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/iavf/iavf.h b/drivers/net/ethernet/intel/iavf/iavf.h
index 2a9f1eeeb701..7dfd6dac74e4 100644
--- a/drivers/net/ethernet/intel/iavf/iavf.h
+++ b/drivers/net/ethernet/intel/iavf/iavf.h
@@ -252,6 +252,7 @@  struct iavf_adapter {
 	struct workqueue_struct *wq;
 	struct work_struct reset_task;
 	struct work_struct adminq_task;
+	struct work_struct features_task;
 	struct delayed_work client_task;
 	wait_queue_head_t down_waitqueue;
 	wait_queue_head_t vc_waitqueue;
@@ -297,7 +298,6 @@  struct iavf_adapter {
 #define IAVF_FLAG_LEGACY_RX			BIT(15)
 #define IAVF_FLAG_REINIT_ITR_NEEDED		BIT(16)
 #define IAVF_FLAG_QUEUES_DISABLED		BIT(17)
-#define IAVF_FLAG_SETUP_NETDEV_FEATURES		BIT(18)
 #define IAVF_FLAG_REINIT_MSIX_NEEDED		BIT(20)
 /* duplicates for common code */
 #define IAVF_FLAG_DCB_ENABLED			0
diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index e7380f1b4acc..e53f5262c047 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -3187,6 +3187,35 @@  static void iavf_reset_task(struct work_struct *work)
 	rtnl_unlock();
 }
 
+/**
+ * iavf_features_task - update netdev features after caps negotiation
+ * @work: pointer to work_struct
+ *
+ * After negotiating VLAN caps with the PF, we need to update our netdev
+ * features, but this requires rtnl_lock. We cannot take it directly in
+ * adminq_task - we might deadlock with iavf_set_mac, which waits on
+ * virtchnl communication while holding rtnl_lock.
+ * So the features are updated here, using a different workqueue.
+ **/
+static void iavf_features_task(struct work_struct *work)
+{
+	struct iavf_adapter *adapter = container_of(work,
+						    struct iavf_adapter,
+						    features_task);
+	struct net_device *netdev = adapter->netdev;
+
+	rtnl_lock();
+	netdev_update_features(netdev);
+	rtnl_unlock();
+	/* Request VLAN offload settings */
+	if (VLAN_V2_ALLOWED(adapter))
+		iavf_set_vlan_offload_features(adapter, 0, netdev->features);
+
+	mutex_lock(&adapter->crit_lock);
+	iavf_set_queue_vlan_tag_loc(adapter);
+	mutex_unlock(&adapter->crit_lock);
+}
+
 /**
  * iavf_adminq_task - worker thread to clean the admin queue
  * @work: pointer to work_struct containing our data
@@ -3233,24 +3262,6 @@  static void iavf_adminq_task(struct work_struct *work)
 	} while (pending);
 	mutex_unlock(&adapter->crit_lock);
 
-	if ((adapter->flags & IAVF_FLAG_SETUP_NETDEV_FEATURES)) {
-		if (adapter->netdev_registered ||
-		    !test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section)) {
-			struct net_device *netdev = adapter->netdev;
-
-			rtnl_lock();
-			netdev_update_features(netdev);
-			rtnl_unlock();
-			/* Request VLAN offload settings */
-			if (VLAN_V2_ALLOWED(adapter))
-				iavf_set_vlan_offload_features
-					(adapter, 0, netdev->features);
-
-			iavf_set_queue_vlan_tag_loc(adapter);
-		}
-
-		adapter->flags &= ~IAVF_FLAG_SETUP_NETDEV_FEATURES;
-	}
 	if ((adapter->flags &
 	     (IAVF_FLAG_RESET_PENDING | IAVF_FLAG_RESET_NEEDED)) ||
 	    adapter->state == __IAVF_RESETTING)
@@ -4948,6 +4959,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->features_task, iavf_features_task);
 	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,
@@ -5115,6 +5127,7 @@  static void iavf_remove(struct pci_dev *pdev)
 	cancel_delayed_work_sync(&adapter->watchdog_task);
 	cancel_work_sync(&adapter->adminq_task);
 	cancel_delayed_work_sync(&adapter->client_task);
+	cancel_work_sync(&adapter->features_task);
 
 	adapter->aq_required = 0;
 	adapter->flags &= ~IAVF_FLAG_REINIT_ITR_NEEDED;
diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
index 0752fd67c96e..a644ab3804de 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
@@ -2225,7 +2225,6 @@  void iavf_virtchnl_completion(struct iavf_adapter *adapter,
 				     sizeof(adapter->vlan_v2_caps)));
 
 		iavf_process_config(adapter);
-		adapter->flags |= IAVF_FLAG_SETUP_NETDEV_FEATURES;
 		was_mac_changed = !ether_addr_equal(netdev->dev_addr,
 						    adapter->hw.mac.addr);
 
@@ -2266,6 +2265,11 @@  void iavf_virtchnl_completion(struct iavf_adapter *adapter,
 
 		adapter->aq_required |= IAVF_FLAG_AQ_ADD_MAC_FILTER |
 			aq_required;
+
+		if (adapter->netdev_registered ||
+		    !test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section))
+			queue_work(system_unbound_wq,
+				   &adapter->features_task);
 		}
 		break;
 	case VIRTCHNL_OP_ENABLE_QUEUES: