From: Michal Schmidt <mschmidt@redhat.com>
Date: Thu, 15 Dec 2022 23:50:48 +0100
Subject: iavf: fix temporary deadlock and failure to set MAC address
Patch-mainline: v6.2-rc6
Git-commit: 4411a608f7c8df000cb1a9f7881982dd8e10839a
References: jsc#PED-835
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>
Acked-by: Thomas Bogendoerfer <tbogendoerfer@suse.de>
---
drivers/net/ethernet/intel/iavf/iavf.h | 2
drivers/net/ethernet/intel/iavf/iavf_ethtool.c | 10 +-
drivers/net/ethernet/intel/iavf/iavf_main.c | 86 +++++++++++-------------
drivers/net/ethernet/intel/iavf/iavf_virtchnl.c | 2
4 files changed, 49 insertions(+), 51 deletions(-)
--- 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)
{
--- 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 ne
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
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
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
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_ad
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);
--- 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 Ad
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_ada
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_ada
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,
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
/* 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->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 *adap
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 ia
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 wo
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 wo
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 wo
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 wo
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 wo
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 wo
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_
*/
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
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_de
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 *pd
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 *pd
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 *pd
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(st
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 *
}
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);
--- 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 iav
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: