Skip to content

Commit

Permalink
iavf: Rework mutexes for better synchronisation
Browse files Browse the repository at this point in the history 
The driver used to crash in multiple spots when put to stress testing
of the init, reset and remove paths.

The user would experience call traces or hangs when creating,
resetting, removing VFs. Depending on the machines, the call traces
are happening in random spots, like reset restoring resources racing
with driver remove.

Make adapter->crit_lock mutex a mandatory lock for guarding the
operations performed on all workqueues and functions dealing with
resource allocation and disposal.

Make __IAVF_REMOVE a final state of the driver respected by
workqueues that shall not requeue, when they fail to obtain the
crit_lock.

Make the IRQ handler not to queue the new work for adminq_task
when the __IAVF_REMOVE state is set.

Fixes: 5ac49f3 ("iavf: use mutexes for locking of critical sections")
Signed-off-by: Slawomir Laba <slawomirx.laba@intel.com>
Signed-off-by: Phani Burra <phani.r.burra@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
Tested-by: Konrad Jankowski <konrad0.jankowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
  • Loading branch information
@SlawomirLaba @anguy11
SlawomirLaba authored and anguy11 committed Feb 25, 2022
1 parent e01b042 commit fc2e6b3
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 31 deletions.
1 change: 0 additions & 1 deletion drivers/net/ethernet/intel/iavf/iavf.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,6 @@ struct iavf_adapter {
struct list_head mac_filter_list;
struct mutex crit_lock;
struct mutex client_lock;
struct mutex remove_lock;
/* Lock to protect accesses to MAC and VLAN lists */
spinlock_t mac_vlan_list_lock;
char misc_vector_name[IFNAMSIZ + 9];
Expand Down
66 changes: 36 additions & 30 deletions drivers/net/ethernet/intel/iavf/iavf_main.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -302,8 +302,9 @@ static irqreturn_t iavf_msix_aq(int irq, void *data)
rd32(hw, IAVF_VFINT_ICR01);
rd32(hw, IAVF_VFINT_ICR0_ENA1);

/* schedule work on the private workqueue */
queue_work(iavf_wq, &adapter->adminq_task);
if (adapter->state != __IAVF_REMOVE)
/* schedule work on the private workqueue */
queue_work(iavf_wq, &adapter->adminq_task);

return IRQ_HANDLED;
}
Expand Down Expand Up @@ -2374,8 +2375,12 @@ static void iavf_watchdog_task(struct work_struct *work)
struct iavf_hw *hw = &adapter->hw;
u32 reg_val;

if (!mutex_trylock(&adapter->crit_lock))
if (!mutex_trylock(&adapter->crit_lock)) {
if (adapter->state == __IAVF_REMOVE)
return;

goto restart_watchdog;
}

if (adapter->flags & IAVF_FLAG_PF_COMMS_FAILED)
iavf_change_state(adapter, __IAVF_COMM_FAILED);
Expand Down Expand Up @@ -2601,13 +2606,13 @@ static void iavf_reset_task(struct work_struct *work)
/* When device is being removed it doesn't make sense to run the reset
* task, just return in such a case.
*/
if (mutex_is_locked(&adapter->remove_lock))
return;
if (!mutex_trylock(&adapter->crit_lock)) {
if (adapter->state != __IAVF_REMOVE)
queue_work(iavf_wq, &adapter->reset_task);

if (iavf_lock_timeout(&adapter->crit_lock, 200)) {
schedule_work(&adapter->reset_task);
return;
}

while (!mutex_trylock(&adapter->client_lock))
usleep_range(500, 1000);
if (CLIENT_ENABLED(adapter)) {
Expand Down Expand Up @@ -2826,13 +2831,19 @@ static void iavf_adminq_task(struct work_struct *work)
if (adapter->flags & IAVF_FLAG_PF_COMMS_FAILED)
goto out;

if (!mutex_trylock(&adapter->crit_lock)) {
if (adapter->state == __IAVF_REMOVE)
return;

queue_work(iavf_wq, &adapter->adminq_task);
goto out;
}

event.buf_len = IAVF_MAX_AQ_BUF_SIZE;
event.msg_buf = kzalloc(event.buf_len, GFP_KERNEL);
if (!event.msg_buf)
goto out;

if (iavf_lock_timeout(&adapter->crit_lock, 200))
goto freedom;
do {
ret = iavf_clean_arq_element(hw, &event, &pending);
v_op = (enum virtchnl_ops)le32_to_cpu(event.desc.cookie_high);
Expand Down Expand Up @@ -3800,11 +3811,12 @@ static int iavf_close(struct net_device *netdev)
struct iavf_adapter *adapter = netdev_priv(netdev);
int status;

if (adapter->state <= __IAVF_DOWN_PENDING)
return 0;
mutex_lock(&adapter->crit_lock);

while (!mutex_trylock(&adapter->crit_lock))
usleep_range(500, 1000);
if (adapter->state <= __IAVF_DOWN_PENDING) {
mutex_unlock(&adapter->crit_lock);
return 0;
}

set_bit(__IAVF_VSI_DOWN, adapter->vsi.state);
if (CLIENT_ENABLED(adapter))
Expand Down Expand Up @@ -4431,7 +4443,6 @@ static int iavf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
*/
mutex_init(&adapter->crit_lock);
mutex_init(&adapter->client_lock);
mutex_init(&adapter->remove_lock);
mutex_init(&hw->aq.asq_mutex);
mutex_init(&hw->aq.arq_mutex);

Expand Down Expand Up @@ -4556,11 +4567,7 @@ static void iavf_remove(struct pci_dev *pdev)
struct iavf_cloud_filter *cf, *cftmp;
struct iavf_hw *hw = &adapter->hw;
int err;
/* Indicate we are in remove and not to run reset_task */
mutex_lock(&adapter->remove_lock);
cancel_work_sync(&adapter->reset_task);
cancel_delayed_work_sync(&adapter->watchdog_task);
cancel_delayed_work_sync(&adapter->client_task);

if (adapter->netdev_registered) {
unregister_netdev(netdev);
adapter->netdev_registered = false;
Expand All @@ -4572,25 +4579,30 @@ static void iavf_remove(struct pci_dev *pdev)
err);
}

mutex_lock(&adapter->crit_lock);
dev_info(&adapter->pdev->dev, "Remove device\n");
iavf_change_state(adapter, __IAVF_REMOVE);

iavf_request_reset(adapter);
msleep(50);
/* If the FW isn't responding, kick it once, but only once. */
if (!iavf_asq_done(hw)) {
iavf_request_reset(adapter);
msleep(50);
}
if (iavf_lock_timeout(&adapter->crit_lock, 5000))
dev_warn(&adapter->pdev->dev, "failed to acquire crit_lock in %s\n", __FUNCTION__);

dev_info(&adapter->pdev->dev, "Removing device\n");
iavf_misc_irq_disable(adapter);
/* Shut down all the garbage mashers on the detention level */
iavf_change_state(adapter, __IAVF_REMOVE);
cancel_work_sync(&adapter->reset_task);
cancel_delayed_work_sync(&adapter->watchdog_task);
cancel_work_sync(&adapter->adminq_task);
cancel_delayed_work_sync(&adapter->client_task);

adapter->aq_required = 0;
adapter->flags &= ~IAVF_FLAG_REINIT_ITR_NEEDED;

iavf_free_all_tx_resources(adapter);
iavf_free_all_rx_resources(adapter);
iavf_misc_irq_disable(adapter);
iavf_free_misc_irq(adapter);

/* In case we enter iavf_remove from erroneous state, free traffic irqs
Expand All @@ -4606,10 +4618,6 @@ static void iavf_remove(struct pci_dev *pdev)
iavf_reset_interrupt_capability(adapter);
iavf_free_q_vectors(adapter);

cancel_delayed_work_sync(&adapter->watchdog_task);

cancel_work_sync(&adapter->adminq_task);

iavf_free_rss(adapter);

if (hw->aq.asq.count)
Expand All @@ -4621,8 +4629,6 @@ static void iavf_remove(struct pci_dev *pdev)
mutex_destroy(&adapter->client_lock);
mutex_unlock(&adapter->crit_lock);
mutex_destroy(&adapter->crit_lock);
mutex_unlock(&adapter->remove_lock);
mutex_destroy(&adapter->remove_lock);

iounmap(hw->hw_addr);
pci_release_regions(pdev);
Expand Down

0 comments on commit fc2e6b3

Please sign in to comment.