diff options
| author | Przemek Kitszel <przemyslaw.kitszel@intel.com> | 2025-04-04 13:23:21 +0300 |
|---|---|---|
| committer | Tony Nguyen <anthony.l.nguyen@intel.com> | 2025-06-03 19:48:03 +0300 |
| commit | 120f28a6f314fef7f282c99f196923fe44081cad (patch) | |
| tree | d3369de9c5672923bd46ef2dc8cc8b47af0ce174 /drivers/net/ethernet/intel/iavf/iavf_ethtool.c | |
| parent | 05702b5c949bd46243181833d4726f4c5e95f5e3 (diff) | |
| download | linux-120f28a6f314fef7f282c99f196923fe44081cad.tar.xz | |
iavf: get rid of the crit lock
Get rid of the crit lock.
That frees us from the error prone logic of try_locks.
Thanks to netdev_lock() by Jakub it is now easy, and in most cases we were
protected by it already - replace crit lock by netdev lock when it was not
the case.
Lockdep reports that we should cancel the work under crit_lock [splat1],
and that was the scheme we have mostly followed since [1] by Slawomir.
But when that is done we still got into deadlocks [splat2]. So instead
we should look at the bigger problem, namely "weird locking/scheduling"
of the iavf. The first step to fix that is to remove the crit lock.
I will followup with a -next series that simplifies scheduling/tasks.
Cancel the work without netdev lock (weird unlock+lock scheme),
to fix the [splat2] (which would be totally ugly if we would kept
the crit lock).
Extend protected part of iavf_watchdog_task() to include scheduling
more work.
Note that the removed comment in iavf_reset_task() was misplaced,
it belonged to inside of the removed if condition, so it's gone now.
[splat1] - w/o this patch - The deadlock during VF removal:
WARNING: possible circular locking dependency detected
sh/3825 is trying to acquire lock:
((work_completion)(&(&adapter->watchdog_task)->work)){+.+.}-{0:0}, at: start_flush_work+0x1a1/0x470
but task is already holding lock:
(&adapter->crit_lock){+.+.}-{4:4}, at: iavf_remove+0xd1/0x690 [iavf]
which lock already depends on the new lock.
[splat2] - when cancelling work under crit lock, w/o this series,
see [2] for the band aid attempt
WARNING: possible circular locking dependency detected
sh/3550 is trying to acquire lock:
((wq_completion)iavf){+.+.}-{0:0}, at: touch_wq_lockdep_map+0x26/0x90
but task is already holding lock:
(&dev->lock){+.+.}-{4:4}, at: iavf_remove+0xa6/0x6e0 [iavf]
which lock already depends on the new lock.
[1] fc2e6b3b132a ("iavf: Rework mutexes for better synchronisation")
[2] https://github.com/pkitszel/linux/commit/52dddbfc2bb60294083f5711a158a
Fixes: d1639a17319b ("iavf: fix a deadlock caused by rtnl and driver's lock circular dependencies")
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
Diffstat (limited to 'drivers/net/ethernet/intel/iavf/iavf_ethtool.c')
| -rw-r--r-- | drivers/net/ethernet/intel/iavf/iavf_ethtool.c | 23 |
1 files changed, 1 insertions, 22 deletions
diff --git a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c index 03d86fe80ad9..2b2b315205b5 100644 --- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c +++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c @@ -1258,7 +1258,6 @@ static int iavf_add_fdir_ethtool(struct iavf_adapter *adapter, struct ethtool_rx { struct ethtool_rx_flow_spec *fsp = &cmd->fs; struct iavf_fdir_fltr *fltr; - int count = 50; int err; netdev_assert_locked(adapter->netdev); @@ -1281,14 +1280,6 @@ static int iavf_add_fdir_ethtool(struct iavf_adapter *adapter, struct ethtool_rx if (!fltr) return -ENOMEM; - while (!mutex_trylock(&adapter->crit_lock)) { - if (--count == 0) { - kfree(fltr); - return -EINVAL; - } - udelay(1); - } - err = iavf_add_fdir_fltr_info(adapter, fsp, fltr); if (!err) err = iavf_fdir_add_fltr(adapter, fltr); @@ -1296,7 +1287,6 @@ static int iavf_add_fdir_ethtool(struct iavf_adapter *adapter, struct ethtool_rx if (err) kfree(fltr); - mutex_unlock(&adapter->crit_lock); return err; } @@ -1439,9 +1429,9 @@ iavf_set_adv_rss_hash_opt(struct iavf_adapter *adapter, { struct iavf_adv_rss *rss_old, *rss_new; bool rss_new_add = false; - int count = 50, err = 0; bool symm = false; u64 hash_flds; + int err = 0; u32 hdrs; netdev_assert_locked(adapter->netdev); @@ -1469,15 +1459,6 @@ iavf_set_adv_rss_hash_opt(struct iavf_adapter *adapter, return -EINVAL; } - while (!mutex_trylock(&adapter->crit_lock)) { - if (--count == 0) { - kfree(rss_new); - return -EINVAL; - } - - udelay(1); - } - spin_lock_bh(&adapter->adv_rss_lock); rss_old = iavf_find_adv_rss_cfg_by_hdrs(adapter, hdrs); if (rss_old) { @@ -1506,8 +1487,6 @@ iavf_set_adv_rss_hash_opt(struct iavf_adapter *adapter, if (!err) iavf_schedule_aq_request(adapter, IAVF_FLAG_AQ_ADD_ADV_RSS_CFG); - mutex_unlock(&adapter->crit_lock); - if (!rss_new_add) kfree(rss_new); |
