Skip to content

Commit

Permalink
iavf: fix temporary deadlock and failure to set MAC address
Browse files Browse the repository at this point in the history 
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: 35a2443 ("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>
  • Loading branch information
@michich @intel-lab-lkp
michich authored and intel-lab-lkp committed Dec 14, 2022
1 parent 7e68dd7 commit 82f371e
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 49 deletions.
2 changes: 1 addition & 1 deletion drivers/net/ethernet/intel/iavf/iavf.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)
{
Expand Down
10 changes: 5 additions & 5 deletions drivers/net/ethernet/intel/iavf/iavf_ethtool.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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);

Expand Down
84 changes: 42 additions & 42 deletions drivers/net/ethernet/intel/iavf/iavf_main.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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);
}
}

Expand All @@ -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);
}

/**
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}

/**
Expand Down Expand Up @@ -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);
}

/**
Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -2700,39 +2699,39 @@ 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;
}

switch (adapter->state) {
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:
Expand All @@ -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,
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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;
}
Expand All @@ -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);
}

/**
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -3116,7 +3117,7 @@ static void iavf_reset_task(struct work_struct *work)
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.
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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 */
Expand All @@ -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);
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -5202,18 +5214,7 @@ static int __init iavf_init_module(void)

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);
Expand All @@ -5227,7 +5228,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);
Expand Down
2 changes: 1 addition & 1 deletion drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down

0 comments on commit 82f371e

Please sign in to comment.