Skip to content

Commit 5ac49f3

Browse files
sassmannanguy11
sassmann
authored and
anguy11
committed
iavf: use mutexes for locking of critical sections
As follow-up to the discussion with Jakub Kicinski about iavf locking being insufficient [1] convert iavf to use mutexes instead of bitops. The locking logic is kept as is, just a drop-in replacement of enum iavf_critical_section_t with separate mutexes. The only difference is that the mutexes will be destroyed before the module is unloaded. [1] https://lwn.net/ml/netdev/20210316150210.00007249%40intel.com/ Signed-off-by: Stefan Assmann <sassmann@kpanic.de> Tested-by: Marek Szlosek <marek.szlosek@intel.com> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
1 parent 752be29 commit 5ac49f3

File tree

3 files changed

+56
-63
lines changed

3 files changed

+56
-63
lines changed

‎drivers/net/ethernet/intel/iavf/iavf.h‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -185,12 +185,6 @@ enum iavf_state_t {
185185
__IAVF_RUNNING, /* opened, working */
186186
};
187187

188-
enum iavf_critical_section_t {
189-
__IAVF_IN_CRITICAL_TASK, /* cannot be interrupted */
190-
__IAVF_IN_CLIENT_TASK,
191-
__IAVF_IN_REMOVE_TASK, /* device being removed */
192-
};
193-
194188
#define IAVF_CLOUD_FIELD_OMAC 0x01
195189
#define IAVF_CLOUD_FIELD_IMAC 0x02
196190
#define IAVF_CLOUD_FIELD_IVLAN 0x04
@@ -235,6 +229,9 @@ struct iavf_adapter {
235229
struct iavf_q_vector *q_vectors;
236230
struct list_head vlan_filter_list;
237231
struct list_head mac_filter_list;
232+
struct mutex crit_lock;
233+
struct mutex client_lock;
234+
struct mutex remove_lock;
238235
/* Lock to protect accesses to MAC and VLAN lists */
239236
spinlock_t mac_vlan_list_lock;
240237
char misc_vector_name[IFNAMSIZ + 9];

‎drivers/net/ethernet/intel/iavf/iavf_ethtool.c‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1352,8 +1352,7 @@ static int iavf_add_fdir_ethtool(struct iavf_adapter *adapter, struct ethtool_rx
13521352
if (!fltr)
13531353
return -ENOMEM;
13541354

1355-
while (test_and_set_bit(__IAVF_IN_CRITICAL_TASK,
1356-
&adapter->crit_section)) {
1355+
while (!mutex_trylock(&adapter->crit_lock)) {
13571356
if (--count == 0) {
13581357
kfree(fltr);
13591358
return -EINVAL;
@@ -1378,7 +1377,7 @@ static int iavf_add_fdir_ethtool(struct iavf_adapter *adapter, struct ethtool_rx
13781377
if (err && fltr)
13791378
kfree(fltr);
13801379

1381-
clear_bit(__IAVF_IN_CRITICAL_TASK, &adapter->crit_section);
1380+
mutex_unlock(&adapter->crit_lock);
13821381
return err;
13831382
}
13841383

@@ -1563,8 +1562,7 @@ iavf_set_adv_rss_hash_opt(struct iavf_adapter *adapter,
15631562
return -EINVAL;
15641563
}
15651564

1566-
while (test_and_set_bit(__IAVF_IN_CRITICAL_TASK,
1567-
&adapter->crit_section)) {
1565+
while (!mutex_trylock(&adapter->crit_lock)) {
15681566
if (--count == 0) {
15691567
kfree(rss_new);
15701568
return -EINVAL;
@@ -1600,7 +1598,7 @@ iavf_set_adv_rss_hash_opt(struct iavf_adapter *adapter,
16001598
if (!err)
16011599
mod_delayed_work(iavf_wq, &adapter->watchdog_task, 0);
16021600

1603-
clear_bit(__IAVF_IN_CRITICAL_TASK, &adapter->crit_section);
1601+
mutex_unlock(&adapter->crit_lock);
16041602

16051603
if (!rss_new_add)
16061604
kfree(rss_new);

‎drivers/net/ethernet/intel/iavf/iavf_main.c‎

Lines changed: 49 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -132,21 +132,18 @@ enum iavf_status iavf_free_virt_mem_d(struct iavf_hw *hw,
132132
}
133133

134134
/**
135-
* iavf_lock_timeout - try to set bit but give up after timeout
136-
* @adapter: board private structure
137-
* @bit: bit to set
135+
* iavf_lock_timeout - try to lock mutex but give up after timeout
136+
* @lock: mutex that should be locked
138137
* @msecs: timeout in msecs
139138
*
140139
* Returns 0 on success, negative on failure
141140
**/
142-
static int iavf_lock_timeout(struct iavf_adapter *adapter,
143-
enum iavf_critical_section_t bit,
144-
unsigned int msecs)
141+
static int iavf_lock_timeout(struct mutex *lock, unsigned int msecs)
145142
{
146143
unsigned int wait, delay = 10;
147144

148145
for (wait = 0; wait < msecs; wait += delay) {
149-
if (!test_and_set_bit(bit, &adapter->crit_section))
146+
if (mutex_trylock(lock))
150147
return 0;
151148

152149
msleep(delay);
@@ -1939,7 +1936,7 @@ static void iavf_watchdog_task(struct work_struct *work)
19391936
struct iavf_hw *hw = &adapter->hw;
19401937
u32 reg_val;
19411938

1942-
if (test_and_set_bit(__IAVF_IN_CRITICAL_TASK, &adapter->crit_section))
1939+
if (!mutex_trylock(&adapter->crit_lock))
19431940
goto restart_watchdog;
19441941

19451942
if (adapter->flags & IAVF_FLAG_PF_COMMS_FAILED)
@@ -1957,8 +1954,7 @@ static void iavf_watchdog_task(struct work_struct *work)
19571954
adapter->state = __IAVF_STARTUP;
19581955
adapter->flags &= ~IAVF_FLAG_PF_COMMS_FAILED;
19591956
queue_delayed_work(iavf_wq, &adapter->init_task, 10);
1960-
clear_bit(__IAVF_IN_CRITICAL_TASK,
1961-
&adapter->crit_section);
1957+
mutex_unlock(&adapter->crit_lock);
19621958
/* Don't reschedule the watchdog, since we've restarted
19631959
* the init task. When init_task contacts the PF and
19641960
* gets everything set up again, it'll restart the
@@ -1968,14 +1964,13 @@ static void iavf_watchdog_task(struct work_struct *work)
19681964
}
19691965
adapter->aq_required = 0;
19701966
adapter->current_op = VIRTCHNL_OP_UNKNOWN;
1971-
clear_bit(__IAVF_IN_CRITICAL_TASK,
1972-
&adapter->crit_section);
1967+
mutex_unlock(&adapter->crit_lock);
19731968
queue_delayed_work(iavf_wq,
19741969
&adapter->watchdog_task,
19751970
msecs_to_jiffies(10));
19761971
goto watchdog_done;
19771972
case __IAVF_RESETTING:
1978-
clear_bit(__IAVF_IN_CRITICAL_TASK, &adapter->crit_section);
1973+
mutex_unlock(&adapter->crit_lock);
19791974
queue_delayed_work(iavf_wq, &adapter->watchdog_task, HZ * 2);
19801975
return;
19811976
case __IAVF_DOWN:
@@ -1998,7 +1993,7 @@ static void iavf_watchdog_task(struct work_struct *work)
19981993
}
19991994
break;
20001995
case __IAVF_REMOVE:
2001-
clear_bit(__IAVF_IN_CRITICAL_TASK, &adapter->crit_section);
1996+
mutex_unlock(&adapter->crit_lock);
20021997
return;
20031998
default:
20041999
goto restart_watchdog;
@@ -2020,7 +2015,7 @@ static void iavf_watchdog_task(struct work_struct *work)
20202015
if (adapter->state == __IAVF_RUNNING ||
20212016
adapter->state == __IAVF_COMM_FAILED)
20222017
iavf_detect_recover_hung(&adapter->vsi);
2023-
clear_bit(__IAVF_IN_CRITICAL_TASK, &adapter->crit_section);
2018+
mutex_unlock(&adapter->crit_lock);
20242019
restart_watchdog:
20252020
if (adapter->aq_required)
20262021
queue_delayed_work(iavf_wq, &adapter->watchdog_task,
@@ -2084,7 +2079,7 @@ static void iavf_disable_vf(struct iavf_adapter *adapter)
20842079
memset(adapter->vf_res, 0, IAVF_VIRTCHNL_VF_RESOURCE_SIZE);
20852080
iavf_shutdown_adminq(&adapter->hw);
20862081
adapter->netdev->flags &= ~IFF_UP;
2087-
clear_bit(__IAVF_IN_CRITICAL_TASK, &adapter->crit_section);
2082+
mutex_unlock(&adapter->crit_lock);
20882083
adapter->flags &= ~IAVF_FLAG_RESET_PENDING;
20892084
adapter->state = __IAVF_DOWN;
20902085
wake_up(&adapter->down_waitqueue);
@@ -2117,15 +2112,14 @@ static void iavf_reset_task(struct work_struct *work)
21172112
/* When device is being removed it doesn't make sense to run the reset
21182113
* task, just return in such a case.
21192114
*/
2120-
if (test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section))
2115+
if (mutex_is_locked(&adapter->remove_lock))
21212116
return;
21222117

2123-
if (iavf_lock_timeout(adapter, __IAVF_IN_CRITICAL_TASK, 200)) {
2118+
if (iavf_lock_timeout(&adapter->crit_lock, 200)) {
21242119
schedule_work(&adapter->reset_task);
21252120
return;
21262121
}
2127-
while (test_and_set_bit(__IAVF_IN_CLIENT_TASK,
2128-
&adapter->crit_section))
2122+
while (!mutex_trylock(&adapter->client_lock))
21292123
usleep_range(500, 1000);
21302124
if (CLIENT_ENABLED(adapter)) {
21312125
adapter->flags &= ~(IAVF_FLAG_CLIENT_NEEDS_OPEN |
@@ -2177,7 +2171,7 @@ static void iavf_reset_task(struct work_struct *work)
21772171
dev_err(&adapter->pdev->dev, "Reset never finished (%x)\n",
21782172
reg_val);
21792173
iavf_disable_vf(adapter);
2180-
clear_bit(__IAVF_IN_CLIENT_TASK, &adapter->crit_section);
2174+
mutex_unlock(&adapter->client_lock);
21812175
return; /* Do not attempt to reinit. It's dead, Jim. */
21822176
}
21832177

@@ -2304,13 +2298,13 @@ static void iavf_reset_task(struct work_struct *work)
23042298
adapter->state = __IAVF_DOWN;
23052299
wake_up(&adapter->down_waitqueue);
23062300
}
2307-
clear_bit(__IAVF_IN_CLIENT_TASK, &adapter->crit_section);
2308-
clear_bit(__IAVF_IN_CRITICAL_TASK, &adapter->crit_section);
2301+
mutex_unlock(&adapter->client_lock);
2302+
mutex_unlock(&adapter->crit_lock);
23092303

23102304
return;
23112305
reset_err:
2312-
clear_bit(__IAVF_IN_CLIENT_TASK, &adapter->crit_section);
2313-
clear_bit(__IAVF_IN_CRITICAL_TASK, &adapter->crit_section);
2306+
mutex_unlock(&adapter->client_lock);
2307+
mutex_unlock(&adapter->crit_lock);
23142308
dev_err(&adapter->pdev->dev, "failed to allocate resources during reinit\n");
23152309
iavf_close(netdev);
23162310
}
@@ -2338,7 +2332,7 @@ static void iavf_adminq_task(struct work_struct *work)
23382332
if (!event.msg_buf)
23392333
goto out;
23402334

2341-
if (iavf_lock_timeout(adapter, __IAVF_IN_CRITICAL_TASK, 200))
2335+
if (iavf_lock_timeout(&adapter->crit_lock, 200))
23422336
goto freedom;
23432337
do {
23442338
ret = iavf_clean_arq_element(hw, &event, &pending);
@@ -2353,7 +2347,7 @@ static void iavf_adminq_task(struct work_struct *work)
23532347
if (pending != 0)
23542348
memset(event.msg_buf, 0, IAVF_MAX_AQ_BUF_SIZE);
23552349
} while (pending);
2356-
clear_bit(__IAVF_IN_CRITICAL_TASK, &adapter->crit_section);
2350+
mutex_unlock(&adapter->crit_lock);
23572351

23582352
if ((adapter->flags &
23592353
(IAVF_FLAG_RESET_PENDING | IAVF_FLAG_RESET_NEEDED)) ||
@@ -2420,7 +2414,7 @@ static void iavf_client_task(struct work_struct *work)
24202414
* later.
24212415
*/
24222416

2423-
if (test_and_set_bit(__IAVF_IN_CLIENT_TASK, &adapter->crit_section))
2417+
if (!mutex_trylock(&adapter->client_lock))
24242418
return;
24252419

24262420
if (adapter->flags & IAVF_FLAG_SERVICE_CLIENT_REQUESTED) {
@@ -2443,7 +2437,7 @@ static void iavf_client_task(struct work_struct *work)
24432437
adapter->flags &= ~IAVF_FLAG_CLIENT_NEEDS_OPEN;
24442438
}
24452439
out:
2446-
clear_bit(__IAVF_IN_CLIENT_TASK, &adapter->crit_section);
2440+
mutex_unlock(&adapter->client_lock);
24472441
}
24482442

24492443
/**
@@ -3046,8 +3040,7 @@ static int iavf_configure_clsflower(struct iavf_adapter *adapter,
30463040
if (!filter)
30473041
return -ENOMEM;
30483042

3049-
while (test_and_set_bit(__IAVF_IN_CRITICAL_TASK,
3050-
&adapter->crit_section)) {
3043+
while (!mutex_trylock(&adapter->crit_lock)) {
30513044
if (--count == 0)
30523045
goto err;
30533046
udelay(1);
@@ -3078,7 +3071,7 @@ static int iavf_configure_clsflower(struct iavf_adapter *adapter,
30783071
if (err)
30793072
kfree(filter);
30803073

3081-
clear_bit(__IAVF_IN_CRITICAL_TASK, &adapter->crit_section);
3074+
mutex_unlock(&adapter->crit_lock);
30823075
return err;
30833076
}
30843077

@@ -3225,8 +3218,7 @@ static int iavf_open(struct net_device *netdev)
32253218
return -EIO;
32263219
}
32273220

3228-
while (test_and_set_bit(__IAVF_IN_CRITICAL_TASK,
3229-
&adapter->crit_section))
3221+
while (!mutex_trylock(&adapter->crit_lock))
32303222
usleep_range(500, 1000);
32313223

32323224
if (adapter->state != __IAVF_DOWN) {
@@ -3261,7 +3253,7 @@ static int iavf_open(struct net_device *netdev)
32613253

32623254
iavf_irq_enable(adapter, true);
32633255

3264-
clear_bit(__IAVF_IN_CRITICAL_TASK, &adapter->crit_section);
3256+
mutex_unlock(&adapter->crit_lock);
32653257

32663258
return 0;
32673259

@@ -3273,7 +3265,7 @@ static int iavf_open(struct net_device *netdev)
32733265
err_setup_tx:
32743266
iavf_free_all_tx_resources(adapter);
32753267
err_unlock:
3276-
clear_bit(__IAVF_IN_CRITICAL_TASK, &adapter->crit_section);
3268+
mutex_unlock(&adapter->crit_lock);
32773269

32783270
return err;
32793271
}
@@ -3297,8 +3289,7 @@ static int iavf_close(struct net_device *netdev)
32973289
if (adapter->state <= __IAVF_DOWN_PENDING)
32983290
return 0;
32993291

3300-
while (test_and_set_bit(__IAVF_IN_CRITICAL_TASK,
3301-
&adapter->crit_section))
3292+
while (!mutex_trylock(&adapter->crit_lock))
33023293
usleep_range(500, 1000);
33033294

33043295
set_bit(__IAVF_VSI_DOWN, adapter->vsi.state);
@@ -3309,7 +3300,7 @@ static int iavf_close(struct net_device *netdev)
33093300
adapter->state = __IAVF_DOWN_PENDING;
33103301
iavf_free_traffic_irqs(adapter);
33113302

3312-
clear_bit(__IAVF_IN_CRITICAL_TASK, &adapter->crit_section);
3303+
mutex_unlock(&adapter->crit_lock);
33133304

33143305
/* We explicitly don't free resources here because the hardware is
33153306
* still active and can DMA into memory. Resources are cleared in
@@ -3658,8 +3649,8 @@ static void iavf_init_task(struct work_struct *work)
36583649
init_task.work);
36593650
struct iavf_hw *hw = &adapter->hw;
36603651

3661-
if (iavf_lock_timeout(adapter, __IAVF_IN_CRITICAL_TASK, 5000)) {
3662-
dev_warn(&adapter->pdev->dev, "failed to set __IAVF_IN_CRITICAL_TASK in %s\n", __FUNCTION__);
3652+
if (iavf_lock_timeout(&adapter->crit_lock, 5000)) {
3653+
dev_warn(&adapter->pdev->dev, "failed to acquire crit_lock in %s\n", __FUNCTION__);
36633654
return;
36643655
}
36653656
switch (adapter->state) {
@@ -3694,7 +3685,7 @@ static void iavf_init_task(struct work_struct *work)
36943685
}
36953686
queue_delayed_work(iavf_wq, &adapter->init_task, HZ);
36963687
out:
3697-
clear_bit(__IAVF_IN_CRITICAL_TASK, &adapter->crit_section);
3688+
mutex_unlock(&adapter->crit_lock);
36983689
}
36993690

37003691
/**
@@ -3711,12 +3702,12 @@ static void iavf_shutdown(struct pci_dev *pdev)
37113702
if (netif_running(netdev))
37123703
iavf_close(netdev);
37133704

3714-
if (iavf_lock_timeout(adapter, __IAVF_IN_CRITICAL_TASK, 5000))
3715-
dev_warn(&adapter->pdev->dev, "failed to set __IAVF_IN_CRITICAL_TASK in %s\n", __FUNCTION__);
3705+
if (iavf_lock_timeout(&adapter->crit_lock, 5000))
3706+
dev_warn(&adapter->pdev->dev, "failed to acquire crit_lock in %s\n", __FUNCTION__);
37163707
/* Prevent the watchdog from running. */
37173708
adapter->state = __IAVF_REMOVE;
37183709
adapter->aq_required = 0;
3719-
clear_bit(__IAVF_IN_CRITICAL_TASK, &adapter->crit_section);
3710+
mutex_unlock(&adapter->crit_lock);
37203711

37213712
#ifdef CONFIG_PM
37223713
pci_save_state(pdev);
@@ -3810,6 +3801,9 @@ static int iavf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
38103801
/* set up the locks for the AQ, do this only once in probe
38113802
* and destroy them only once in remove
38123803
*/
3804+
mutex_init(&adapter->crit_lock);
3805+
mutex_init(&adapter->client_lock);
3806+
mutex_init(&adapter->remove_lock);
38133807
mutex_init(&hw->aq.asq_mutex);
38143808
mutex_init(&hw->aq.arq_mutex);
38153809

@@ -3861,8 +3855,7 @@ static int __maybe_unused iavf_suspend(struct device *dev_d)
38613855

38623856
netif_device_detach(netdev);
38633857

3864-
while (test_and_set_bit(__IAVF_IN_CRITICAL_TASK,
3865-
&adapter->crit_section))
3858+
while (!mutex_trylock(&adapter->crit_lock))
38663859
usleep_range(500, 1000);
38673860

38683861
if (netif_running(netdev)) {
@@ -3873,7 +3866,7 @@ static int __maybe_unused iavf_suspend(struct device *dev_d)
38733866
iavf_free_misc_irq(adapter);
38743867
iavf_reset_interrupt_capability(adapter);
38753868

3876-
clear_bit(__IAVF_IN_CRITICAL_TASK, &adapter->crit_section);
3869+
mutex_unlock(&adapter->crit_lock);
38773870

38783871
return 0;
38793872
}
@@ -3935,7 +3928,7 @@ static void iavf_remove(struct pci_dev *pdev)
39353928
struct iavf_hw *hw = &adapter->hw;
39363929
int err;
39373930
/* Indicate we are in remove and not to run reset_task */
3938-
set_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section);
3931+
mutex_lock(&adapter->remove_lock);
39393932
cancel_delayed_work_sync(&adapter->init_task);
39403933
cancel_work_sync(&adapter->reset_task);
39413934
cancel_delayed_work_sync(&adapter->client_task);
@@ -3957,8 +3950,8 @@ static void iavf_remove(struct pci_dev *pdev)
39573950
iavf_request_reset(adapter);
39583951
msleep(50);
39593952
}
3960-
if (iavf_lock_timeout(adapter, __IAVF_IN_CRITICAL_TASK, 5000))
3961-
dev_warn(&adapter->pdev->dev, "failed to set __IAVF_IN_CRITICAL_TASK in %s\n", __FUNCTION__);
3953+
if (iavf_lock_timeout(&adapter->crit_lock, 5000))
3954+
dev_warn(&adapter->pdev->dev, "failed to acquire crit_lock in %s\n", __FUNCTION__);
39623955

39633956
/* Shut down all the garbage mashers on the detention level */
39643957
adapter->state = __IAVF_REMOVE;
@@ -3983,6 +3976,11 @@ static void iavf_remove(struct pci_dev *pdev)
39833976
/* destroy the locks only once, here */
39843977
mutex_destroy(&hw->aq.arq_mutex);
39853978
mutex_destroy(&hw->aq.asq_mutex);
3979+
mutex_destroy(&adapter->client_lock);
3980+
mutex_unlock(&adapter->crit_lock);
3981+
mutex_destroy(&adapter->crit_lock);
3982+
mutex_unlock(&adapter->remove_lock);
3983+
mutex_destroy(&adapter->remove_lock);
39863984

39873985
iounmap(hw->hw_addr);
39883986
pci_release_regions(pdev);

0 commit comments

Comments
 (0)