iavf: fix temporary deadlock and failure to set MAC address

[ Upstream commit 4411a608f7c8df000cb1a9f7881982dd8e10839a ]

We are seeing an issue where setting the MAC address on iavf fails with
EAGAIN after the 2.5s timeout expires in iavf_set_mac().

There is the following deadlock scenario:

iavf_set_mac(), holding rtnl_lock, waits on:
  iavf_watchdog_task (within iavf_wq) to send a message to the PF,
 and
  iavf_adminq_task (within iavf_wq) to receive a response from the PF.
In this adapter state (>=__IAVF_DOWN), these tasks do not need to take
rtnl_lock, but iavf_wq is a global single-threaded workqueue, so they
may get stuck waiting for another adapter's iavf_watchdog_task to run
iavf_init_config_adapter(), which does take rtnl_lock.

The deadlock resolves itself by the timeout in iavf_set_mac(),
which results in EAGAIN returned to userspace.

Let's break the deadlock loop by changing iavf_wq into a per-adapter
workqueue, so that one adapter's tasks are not blocked by another's.

Fixes: 35a2443d0910 ("iavf: Add waiting for response from PF in set mac")
Co-developed-by: Ivan Vecera <ivecera@redhat.com>
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>

diff --git a/drivers/net/ethernet/intel/iavf/iavf.h b/drivers/net/ethernet/intel/iavf/iavf.h
index 0d1bab4..2a9f1ee 100644 (file)
--- a/drivers/net/ethernet/intel/iavf/iavf.h
+++ b/drivers/net/ethernet/intel/iavf/iavf.h
@@ -249,6 +249,7 @@ struct iavf_cloud_filter {
 
 /* board specific private data structure */
 struct iavf_adapter {
+       struct workqueue_struct *wq;
        struct work_struct reset_task;
        struct work_struct adminq_task;
        struct delayed_work client_task;
@@ -459,7 +460,6 @@ struct iavf_device {
 
 /* needed by iavf_ethtool.c */
 extern char iavf_driver_name[];
-extern struct workqueue_struct *iavf_wq;
 
 static inline const char *iavf_state_str(enum iavf_state_t state)
 {

diff --git a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
index a056e15..83cfc54 100644 (file)
--- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
@@ -532,7 +532,7 @@ static int iavf_set_priv_flags(struct net_device *netdev, u32 flags)
        if (changed_flags & IAVF_FLAG_LEGACY_RX) {
                if (netif_running(netdev)) {
                        adapter->flags |= IAVF_FLAG_RESET_NEEDED;
-                       queue_work(iavf_wq, &adapter->reset_task);
+                       queue_work(adapter->wq, &adapter->reset_task);
                }
        }
 
@@ -672,7 +672,7 @@ static int iavf_set_ringparam(struct net_device *netdev,
 
        if (netif_running(netdev)) {
                adapter->flags |= IAVF_FLAG_RESET_NEEDED;
-               queue_work(iavf_wq, &adapter->reset_task);
+               queue_work(adapter->wq, &adapter->reset_task);
        }
 
        return 0;
@@ -1433,7 +1433,7 @@ static int iavf_add_fdir_ethtool(struct iavf_adapter *adapter, struct ethtool_rx
        adapter->aq_required |= IAVF_FLAG_AQ_ADD_FDIR_FILTER;
        spin_unlock_bh(&adapter->fdir_fltr_lock);
 
-       mod_delayed_work(iavf_wq, &adapter->watchdog_task, 0);
+       mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0);
 
 ret:
        if (err && fltr)
@@ -1474,7 +1474,7 @@ static int iavf_del_fdir_ethtool(struct iavf_adapter *adapter, struct ethtool_rx
        spin_unlock_bh(&adapter->fdir_fltr_lock);
 
        if (fltr && fltr->state == IAVF_FDIR_FLTR_DEL_REQUEST)
-               mod_delayed_work(iavf_wq, &adapter->watchdog_task, 0);
+               mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0);
 
        return err;
 }
@@ -1658,7 +1658,7 @@ iavf_set_adv_rss_hash_opt(struct iavf_adapter *adapter,
        spin_unlock_bh(&adapter->adv_rss_lock);
 
        if (!err)
-               mod_delayed_work(iavf_wq, &adapter->watchdog_task, 0);
+               mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0);
 
        mutex_unlock(&adapter->crit_lock);
 

diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 260c559..efc7735 100644 (file)
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -49,7 +49,6 @@ MODULE_DESCRIPTION("Intel(R) Ethernet Adaptive Virtual Function Network Driver")
 MODULE_LICENSE("GPL v2");
 
 static const struct net_device_ops iavf_netdev_ops;
-struct workqueue_struct *iavf_wq;
 
 int iavf_status_to_errno(enum iavf_status status)
 {
@@ -277,7 +276,7 @@ void iavf_schedule_reset(struct iavf_adapter *adapter)
        if (!(adapter->flags &
              (IAVF_FLAG_RESET_PENDING | IAVF_FLAG_RESET_NEEDED))) {
                adapter->flags |= IAVF_FLAG_RESET_NEEDED;
-               queue_work(iavf_wq, &adapter->reset_task);
+               queue_work(adapter->wq, &adapter->reset_task);
        }
 }
 
@@ -291,7 +290,7 @@ void iavf_schedule_reset(struct iavf_adapter *adapter)
 void iavf_schedule_request_stats(struct iavf_adapter *adapter)
 {
        adapter->aq_required |= IAVF_FLAG_AQ_REQUEST_STATS;
-       mod_delayed_work(iavf_wq, &adapter->watchdog_task, 0);
+       mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0);
 }
 
 /**
@@ -411,7 +410,7 @@ static irqreturn_t iavf_msix_aq(int irq, void *data)
 
        if (adapter->state != __IAVF_REMOVE)
                /* schedule work on the private workqueue */
-               queue_work(iavf_wq, &adapter->adminq_task);
+               queue_work(adapter->wq, &adapter->adminq_task);
 
        return IRQ_HANDLED;
 }
@@ -1034,7 +1033,7 @@ int iavf_replace_primary_mac(struct iavf_adapter *adapter,
 
        /* schedule the watchdog task to immediately process the request */
        if (f) {
-               queue_work(iavf_wq, &adapter->watchdog_task.work);
+               queue_work(adapter->wq, &adapter->watchdog_task.work);
                return 0;
        }
        return -ENOMEM;
@@ -1257,7 +1256,7 @@ static void iavf_up_complete(struct iavf_adapter *adapter)
        adapter->aq_required |= IAVF_FLAG_AQ_ENABLE_QUEUES;
        if (CLIENT_ENABLED(adapter))
                adapter->flags |= IAVF_FLAG_CLIENT_NEEDS_OPEN;
-       mod_delayed_work(iavf_wq, &adapter->watchdog_task, 0);
+       mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0);
 }
 
 /**
@@ -1414,7 +1413,7 @@ void iavf_down(struct iavf_adapter *adapter)
                adapter->aq_required |= IAVF_FLAG_AQ_DISABLE_QUEUES;
        }
 
-       mod_delayed_work(iavf_wq, &adapter->watchdog_task, 0);
+       mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0);
 }
 
 /**
@@ -2248,7 +2247,7 @@ iavf_set_vlan_offload_features(struct iavf_adapter *adapter,
 
        if (aq_required) {
                adapter->aq_required |= aq_required;
-               mod_delayed_work(iavf_wq, &adapter->watchdog_task, 0);
+               mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0);
        }
 }
 
@@ -2700,7 +2699,7 @@ static void iavf_watchdog_task(struct work_struct *work)
                adapter->aq_required = 0;
                adapter->current_op = VIRTCHNL_OP_UNKNOWN;
                mutex_unlock(&adapter->crit_lock);
-               queue_work(iavf_wq, &adapter->reset_task);
+               queue_work(adapter->wq, &adapter->reset_task);
                return;
        }
 
@@ -2708,31 +2707,31 @@ static void iavf_watchdog_task(struct work_struct *work)
        case __IAVF_STARTUP:
                iavf_startup(adapter);
                mutex_unlock(&adapter->crit_lock);
-               queue_delayed_work(iavf_wq, &adapter->watchdog_task,
+               queue_delayed_work(adapter->wq, &adapter->watchdog_task,
                                   msecs_to_jiffies(30));
                return;
        case __IAVF_INIT_VERSION_CHECK:
                iavf_init_version_check(adapter);
                mutex_unlock(&adapter->crit_lock);
-               queue_delayed_work(iavf_wq, &adapter->watchdog_task,
+               queue_delayed_work(adapter->wq, &adapter->watchdog_task,
                                   msecs_to_jiffies(30));
                return;
        case __IAVF_INIT_GET_RESOURCES:
                iavf_init_get_resources(adapter);
                mutex_unlock(&adapter->crit_lock);
-               queue_delayed_work(iavf_wq, &adapter->watchdog_task,
+               queue_delayed_work(adapter->wq, &adapter->watchdog_task,
                                   msecs_to_jiffies(1));
                return;
        case __IAVF_INIT_EXTENDED_CAPS:
                iavf_init_process_extended_caps(adapter);
                mutex_unlock(&adapter->crit_lock);
-               queue_delayed_work(iavf_wq, &adapter->watchdog_task,
+               queue_delayed_work(adapter->wq, &adapter->watchdog_task,
                                   msecs_to_jiffies(1));
                return;
        case __IAVF_INIT_CONFIG_ADAPTER:
                iavf_init_config_adapter(adapter);
                mutex_unlock(&adapter->crit_lock);
-               queue_delayed_work(iavf_wq, &adapter->watchdog_task,
+               queue_delayed_work(adapter->wq, &adapter->watchdog_task,
                                   msecs_to_jiffies(1));
                return;
        case __IAVF_INIT_FAILED:
@@ -2751,14 +2750,14 @@ static void iavf_watchdog_task(struct work_struct *work)
                        adapter->flags |= IAVF_FLAG_PF_COMMS_FAILED;
                        iavf_shutdown_adminq(hw);
                        mutex_unlock(&adapter->crit_lock);
-                       queue_delayed_work(iavf_wq,
+                       queue_delayed_work(adapter->wq,
                                           &adapter->watchdog_task, (5 * HZ));
                        return;
                }
                /* Try again from failed step*/
                iavf_change_state(adapter, adapter->last_state);
                mutex_unlock(&adapter->crit_lock);
-               queue_delayed_work(iavf_wq, &adapter->watchdog_task, HZ);
+               queue_delayed_work(adapter->wq, &adapter->watchdog_task, HZ);
                return;
        case __IAVF_COMM_FAILED:
                if (test_bit(__IAVF_IN_REMOVE_TASK,
@@ -2789,13 +2788,14 @@ static void iavf_watchdog_task(struct work_struct *work)
                adapter->aq_required = 0;
                adapter->current_op = VIRTCHNL_OP_UNKNOWN;
                mutex_unlock(&adapter->crit_lock);
-               queue_delayed_work(iavf_wq,
+               queue_delayed_work(adapter->wq,
                                   &adapter->watchdog_task,
                                   msecs_to_jiffies(10));
                return;
        case __IAVF_RESETTING:
                mutex_unlock(&adapter->crit_lock);
-               queue_delayed_work(iavf_wq, &adapter->watchdog_task, HZ * 2);
+               queue_delayed_work(adapter->wq, &adapter->watchdog_task,
+                                  HZ * 2);
                return;
        case __IAVF_DOWN:
        case __IAVF_DOWN_PENDING:
@@ -2834,9 +2834,9 @@ static void iavf_watchdog_task(struct work_struct *work)
                adapter->aq_required = 0;
                adapter->current_op = VIRTCHNL_OP_UNKNOWN;
                dev_err(&adapter->pdev->dev, "Hardware reset detected\n");
-               queue_work(iavf_wq, &adapter->reset_task);
+               queue_work(adapter->wq, &adapter->reset_task);
                mutex_unlock(&adapter->crit_lock);
-               queue_delayed_work(iavf_wq,
+               queue_delayed_work(adapter->wq,
                                   &adapter->watchdog_task, HZ * 2);
                return;
        }
@@ -2845,12 +2845,13 @@ static void iavf_watchdog_task(struct work_struct *work)
        mutex_unlock(&adapter->crit_lock);
 restart_watchdog:
        if (adapter->state >= __IAVF_DOWN)
-               queue_work(iavf_wq, &adapter->adminq_task);
+               queue_work(adapter->wq, &adapter->adminq_task);
        if (adapter->aq_required)
-               queue_delayed_work(iavf_wq, &adapter->watchdog_task,
+               queue_delayed_work(adapter->wq, &adapter->watchdog_task,
                                   msecs_to_jiffies(20));
        else
-               queue_delayed_work(iavf_wq, &adapter->watchdog_task, HZ * 2);
+               queue_delayed_work(adapter->wq, &adapter->watchdog_task,
+                                  HZ * 2);
 }
 
 /**
@@ -2952,7 +2953,7 @@ static void iavf_reset_task(struct work_struct *work)
         */
        if (!mutex_trylock(&adapter->crit_lock)) {
                if (adapter->state != __IAVF_REMOVE)
-                       queue_work(iavf_wq, &adapter->reset_task);
+                       queue_work(adapter->wq, &adapter->reset_task);
 
                goto reset_finish;
        }
@@ -3116,7 +3117,7 @@ continue_reset:
        bitmap_clear(adapter->vsi.active_cvlans, 0, VLAN_N_VID);
        bitmap_clear(adapter->vsi.active_svlans, 0, VLAN_N_VID);
 
-       mod_delayed_work(iavf_wq, &adapter->watchdog_task, 2);
+       mod_delayed_work(adapter->wq, &adapter->watchdog_task, 2);
 
        /* We were running when the reset started, so we need to restore some
         * state here.
@@ -3208,7 +3209,7 @@ static void iavf_adminq_task(struct work_struct *work)
                if (adapter->state == __IAVF_REMOVE)
                        return;
 
-               queue_work(iavf_wq, &adapter->adminq_task);
+               queue_work(adapter->wq, &adapter->adminq_task);
                goto out;
        }
 
@@ -4349,7 +4350,7 @@ static int iavf_change_mtu(struct net_device *netdev, int new_mtu)
 
        if (netif_running(netdev)) {
                adapter->flags |= IAVF_FLAG_RESET_NEEDED;
-               queue_work(iavf_wq, &adapter->reset_task);
+               queue_work(adapter->wq, &adapter->reset_task);
        }
 
        return 0;
@@ -4898,6 +4899,13 @@ static int iavf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
        hw = &adapter->hw;
        hw->back = adapter;
 
+       adapter->wq = alloc_ordered_workqueue("%s", WQ_MEM_RECLAIM,
+                                             iavf_driver_name);
+       if (!adapter->wq) {
+               err = -ENOMEM;
+               goto err_alloc_wq;
+       }
+
        adapter->msg_enable = BIT(DEFAULT_DEBUG_LEVEL_SHIFT) - 1;
        iavf_change_state(adapter, __IAVF_STARTUP);
 
@@ -4942,7 +4950,7 @@ static int iavf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
        INIT_WORK(&adapter->adminq_task, iavf_adminq_task);
        INIT_DELAYED_WORK(&adapter->watchdog_task, iavf_watchdog_task);
        INIT_DELAYED_WORK(&adapter->client_task, iavf_client_task);
-       queue_delayed_work(iavf_wq, &adapter->watchdog_task,
+       queue_delayed_work(adapter->wq, &adapter->watchdog_task,
                           msecs_to_jiffies(5 * (pdev->devfn & 0x07)));
 
        /* Setup the wait queue for indicating transition to down status */
@@ -4954,6 +4962,8 @@ static int iavf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
        return 0;
 
 err_ioremap:
+       destroy_workqueue(adapter->wq);
+err_alloc_wq:
        free_netdev(netdev);
 err_alloc_etherdev:
        pci_disable_pcie_error_reporting(pdev);
@@ -5023,7 +5033,7 @@ static int __maybe_unused iavf_resume(struct device *dev_d)
                return err;
        }
 
-       queue_work(iavf_wq, &adapter->reset_task);
+       queue_work(adapter->wq, &adapter->reset_task);
 
        netif_device_attach(adapter->netdev);
 
@@ -5170,6 +5180,8 @@ static void iavf_remove(struct pci_dev *pdev)
        }
        spin_unlock_bh(&adapter->adv_rss_lock);
 
+       destroy_workqueue(adapter->wq);
+
        free_netdev(netdev);
 
        pci_disable_pcie_error_reporting(pdev);
@@ -5196,24 +5208,11 @@ static struct pci_driver iavf_driver = {
  **/
 static int __init iavf_init_module(void)
 {
-       int ret;
-
        pr_info("iavf: %s\n", iavf_driver_string);
 
        pr_info("%s\n", iavf_copyright);
 
-       iavf_wq = alloc_workqueue("%s", WQ_UNBOUND | WQ_MEM_RECLAIM, 1,
-                                 iavf_driver_name);
-       if (!iavf_wq) {
-               pr_err("%s: Failed to create workqueue\n", iavf_driver_name);
-               return -ENOMEM;
-       }
-
-       ret = pci_register_driver(&iavf_driver);
-       if (ret)
-               destroy_workqueue(iavf_wq);
-
-       return ret;
+       return pci_register_driver(&iavf_driver);
 }
 
 module_init(iavf_init_module);
@@ -5227,7 +5226,6 @@ module_init(iavf_init_module);
 static void __exit iavf_exit_module(void)
 {
        pci_unregister_driver(&iavf_driver);
-       destroy_workqueue(iavf_wq);
 }
 
 module_exit(iavf_exit_module);

diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
index 24a701f..0752fd6 100644 (file)
--- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
@@ -1952,7 +1952,7 @@ void iavf_virtchnl_completion(struct iavf_adapter *adapter,
                        if (!(adapter->flags & IAVF_FLAG_RESET_PENDING)) {
                                adapter->flags |= IAVF_FLAG_RESET_PENDING;
                                dev_info(&adapter->pdev->dev, "Scheduling reset task\n");
-                               queue_work(iavf_wq, &adapter->reset_task);
+                               queue_work(adapter->wq, &adapter->reset_task);
                        }
                        break;
                default: