Skip to content

Commit fdadbf6

Browse files
orospanguy11
orosp
authored and
anguy11
committed
iavf: fix incorrect reset handling in callbacks
Three driver callbacks schedule a reset and wait for its completion: ndo_change_mtu(), ethtool set_ringparam(), and ethtool set_channels(). Waiting for reset in ndo_change_mtu() and set_ringparam() was added by commit c2ed240 ("iavf: Wait for reset in callbacks which trigger it") to fix a race condition where adding an interface to bonding immediately after MTU or ring parameter change failed because the interface was still in __RESETTING state. The same commit also added waiting in iavf_set_priv_flags(), which was later removed by commit 5384467 ("iavf: kill "legacy-rx" for good"). Waiting in set_channels() was introduced earlier by commit 4e5e6b5 ("iavf: Fix return of set the new channel count") to ensure the PF has enough time to complete the VF reset when changing channel count, and to return correct error codes to userspace. Commit ef490bb ("iavf: Add net_shaper_ops support") added net_shaper_ops to iavf, which required reset_task to use _locked NAPI variants (napi_enable_locked, napi_disable_locked) that need the netdev instance lock. Later, commit 7e4d784 ("net: hold netdev instance lock during rtnetlink operations") and commit 2bcf477 ("net: ethtool: try to protect all callback with netdev instance lock") started holding the netdev instance lock during ndo and ethtool callbacks for drivers with net_shaper_ops. Finally, commit 120f28a ("iavf: get rid of the crit lock") replaced the driver's crit_lock with netdev_lock in reset_task, causing incorrect behavior: the callback holds netdev_lock and waits for reset_task, but reset_task needs the same lock: Thread 1 (callback) Thread 2 (reset_task) ------------------- --------------------- netdev_lock() [blocked on workqueue] ndo_change_mtu() or ethtool op iavf_schedule_reset() iavf_wait_for_reset() iavf_reset_task() waiting... netdev_lock() <- blocked This does not strictly deadlock because iavf_wait_for_reset() uses wait_event_interruptible_timeout() with a 5-second timeout. The wait eventually times out, the callback returns an error to userspace, and after the lock is released reset_task completes the reset. This leads to incorrect behavior: userspace sees an error even though the configuration change silently takes effect after the timeout. Fix this by extracting the reset logic from iavf_reset_task() into a new iavf_reset_step() function that expects netdev_lock to be already held. The three callbacks now call iavf_reset_step() directly instead of scheduling the work and waiting, performing the reset synchronously in the caller's context which already holds netdev_lock. This eliminates both the incorrect error reporting and the need for iavf_wait_for_reset(), which is removed along with the now-unused reset_waitqueue. The workqueue-based iavf_reset_task() becomes a thin wrapper that acquires netdev_lock and calls iavf_reset_step(), preserving its use for PF-initiated resets. The callbacks may block for several seconds while iavf_reset_step() polls hardware registers, but this is acceptable since netdev_lock is a per-device mutex and only serializes operations on the same interface. v3: - Remove netif_running() guard from iavf_set_channels(). Unlike set_ringparam where descriptor counts are picked up by iavf_open() directly, num_req_queues is only consumed during iavf_reinit_interrupt_scheme() in the reset path. Skipping the reset on a down device would silently discard the channel count change. - Remove dead reset_waitqueue code (struct field, init, and all wake_up calls) since iavf_wait_for_reset() was the only consumer. Fixes: 120f28a ("iavf: get rid of the crit lock") Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> Signed-off-by: Petr Oros <poros@redhat.com> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> Tested-by: Rafal Romanowski <rafal.romanowski@intel.com> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
1 parent efc54fb commit fdadbf6

4 files changed

Lines changed: 31 additions & 69 deletions

File tree

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -260,7 +260,6 @@ struct iavf_adapter {
260260
struct work_struct adminq_task;
261261
struct work_struct finish_config;
262262
wait_queue_head_t down_waitqueue;
263-
wait_queue_head_t reset_waitqueue;
264263
wait_queue_head_t vc_waitqueue;
265264
struct iavf_q_vector *q_vectors;
266265
struct list_head vlan_filter_list;
@@ -626,5 +625,5 @@ void iavf_add_adv_rss_cfg(struct iavf_adapter *adapter);
626625
void iavf_del_adv_rss_cfg(struct iavf_adapter *adapter);
627626
struct iavf_mac_filter *iavf_add_filter(struct iavf_adapter *adapter,
628627
const u8 *macaddr);
629-
int iavf_wait_for_reset(struct iavf_adapter *adapter);
628+
void iavf_reset_step(struct iavf_adapter *adapter);
630629
#endif /* _IAVF_H_ */

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

Lines changed: 6 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -492,7 +492,6 @@ static int iavf_set_ringparam(struct net_device *netdev,
492492
{
493493
struct iavf_adapter *adapter = netdev_priv(netdev);
494494
u32 new_rx_count, new_tx_count;
495-
int ret = 0;
496495

497496
if ((ring->rx_mini_pending) || (ring->rx_jumbo_pending))
498497
return -EINVAL;
@@ -537,13 +536,11 @@ static int iavf_set_ringparam(struct net_device *netdev,
537536
}
538537

539538
if (netif_running(netdev)) {
540-
iavf_schedule_reset(adapter, IAVF_FLAG_RESET_NEEDED);
541-
ret = iavf_wait_for_reset(adapter);
542-
if (ret)
543-
netdev_warn(netdev, "Changing ring parameters timeout or interrupted waiting for reset");
539+
adapter->flags |= IAVF_FLAG_RESET_NEEDED;
540+
iavf_reset_step(adapter);
544541
}
545542

546-
return ret;
543+
return 0;
547544
}
548545

549546
/**
@@ -1723,7 +1720,6 @@ static int iavf_set_channels(struct net_device *netdev,
17231720
{
17241721
struct iavf_adapter *adapter = netdev_priv(netdev);
17251722
u32 num_req = ch->combined_count;
1726-
int ret = 0;
17271723

17281724
if ((adapter->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_ADQ) &&
17291725
adapter->num_tc) {
@@ -1745,13 +1741,10 @@ static int iavf_set_channels(struct net_device *netdev,
17451741

17461742
adapter->num_req_queues = num_req;
17471743
adapter->flags |= IAVF_FLAG_REINIT_ITR_NEEDED;
1748-
iavf_schedule_reset(adapter, IAVF_FLAG_RESET_NEEDED);
1744+
adapter->flags |= IAVF_FLAG_RESET_NEEDED;
1745+
iavf_reset_step(adapter);
17491746

1750-
ret = iavf_wait_for_reset(adapter);
1751-
if (ret)
1752-
netdev_warn(netdev, "Changing channel count timeout or interrupted waiting for reset");
1753-
1754-
return ret;
1747+
return 0;
17551748
}
17561749

17571750
/**

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

Lines changed: 24 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -185,31 +185,6 @@ static bool iavf_is_reset_in_progress(struct iavf_adapter *adapter)
185185
return false;
186186
}
187187

188-
/**
189-
* iavf_wait_for_reset - Wait for reset to finish.
190-
* @adapter: board private structure
191-
*
192-
* Returns 0 if reset finished successfully, negative on timeout or interrupt.
193-
*/
194-
int iavf_wait_for_reset(struct iavf_adapter *adapter)
195-
{
196-
int ret = wait_event_interruptible_timeout(adapter->reset_waitqueue,
197-
!iavf_is_reset_in_progress(adapter),
198-
msecs_to_jiffies(5000));
199-
200-
/* If ret < 0 then it means wait was interrupted.
201-
* If ret == 0 then it means we got a timeout while waiting
202-
* for reset to finish.
203-
* If ret > 0 it means reset has finished.
204-
*/
205-
if (ret > 0)
206-
return 0;
207-
else if (ret < 0)
208-
return -EINTR;
209-
else
210-
return -EBUSY;
211-
}
212-
213188
/**
214189
* iavf_allocate_dma_mem_d - OS specific memory alloc for shared code
215190
* @hw: pointer to the HW structure
@@ -3113,18 +3088,16 @@ static void iavf_reconfig_qs_bw(struct iavf_adapter *adapter)
31133088
}
31143089

31153090
/**
3116-
* iavf_reset_task - Call-back task to handle hardware reset
3117-
* @work: pointer to work_struct
3091+
* iavf_reset_step - Perform the VF reset sequence
3092+
* @adapter: board private structure
31183093
*
3119-
* During reset we need to shut down and reinitialize the admin queue
3120-
* before we can use it to communicate with the PF again. We also clear
3121-
* and reinit the rings because that context is lost as well.
3122-
**/
3123-
static void iavf_reset_task(struct work_struct *work)
3094+
* Requests a reset from PF, polls for completion, and reconfigures
3095+
* the driver. Caller must hold the netdev instance lock.
3096+
*
3097+
* This can sleep for several seconds while polling HW registers.
3098+
*/
3099+
void iavf_reset_step(struct iavf_adapter *adapter)
31243100
{
3125-
struct iavf_adapter *adapter = container_of(work,
3126-
struct iavf_adapter,
3127-
reset_task);
31283101
struct virtchnl_vf_resource *vfres = adapter->vf_res;
31293102
struct net_device *netdev = adapter->netdev;
31303103
struct iavf_hw *hw = &adapter->hw;
@@ -3135,7 +3108,7 @@ static void iavf_reset_task(struct work_struct *work)
31353108
int i = 0, err;
31363109
bool running;
31373110

3138-
netdev_lock(netdev);
3111+
netdev_assert_locked(netdev);
31393112

31403113
iavf_misc_irq_disable(adapter);
31413114
if (adapter->flags & IAVF_FLAG_RESET_NEEDED) {
@@ -3180,7 +3153,6 @@ static void iavf_reset_task(struct work_struct *work)
31803153
dev_err(&adapter->pdev->dev, "Reset never finished (%x)\n",
31813154
reg_val);
31823155
iavf_disable_vf(adapter);
3183-
netdev_unlock(netdev);
31843156
return; /* Do not attempt to reinit. It's dead, Jim. */
31853157
}
31863158

@@ -3192,7 +3164,6 @@ static void iavf_reset_task(struct work_struct *work)
31923164
iavf_startup(adapter);
31933165
queue_delayed_work(adapter->wq, &adapter->watchdog_task,
31943166
msecs_to_jiffies(30));
3195-
netdev_unlock(netdev);
31963167
return;
31973168
}
31983169

@@ -3335,9 +3306,6 @@ static void iavf_reset_task(struct work_struct *work)
33353306

33363307
adapter->flags &= ~IAVF_FLAG_REINIT_ITR_NEEDED;
33373308

3338-
wake_up(&adapter->reset_waitqueue);
3339-
netdev_unlock(netdev);
3340-
33413309
return;
33423310
reset_err:
33433311
if (running) {
@@ -3346,10 +3314,21 @@ static void iavf_reset_task(struct work_struct *work)
33463314
}
33473315
iavf_disable_vf(adapter);
33483316

3349-
netdev_unlock(netdev);
33503317
dev_err(&adapter->pdev->dev, "failed to allocate resources during reinit\n");
33513318
}
33523319

3320+
static void iavf_reset_task(struct work_struct *work)
3321+
{
3322+
struct iavf_adapter *adapter = container_of(work,
3323+
struct iavf_adapter,
3324+
reset_task);
3325+
struct net_device *netdev = adapter->netdev;
3326+
3327+
netdev_lock(netdev);
3328+
iavf_reset_step(adapter);
3329+
netdev_unlock(netdev);
3330+
}
3331+
33533332
/**
33543333
* iavf_adminq_task - worker thread to clean the admin queue
33553334
* @work: pointer to work_struct containing our data
@@ -4615,22 +4594,17 @@ static int iavf_close(struct net_device *netdev)
46154594
static int iavf_change_mtu(struct net_device *netdev, int new_mtu)
46164595
{
46174596
struct iavf_adapter *adapter = netdev_priv(netdev);
4618-
int ret = 0;
46194597

46204598
netdev_dbg(netdev, "changing MTU from %d to %d\n",
46214599
netdev->mtu, new_mtu);
46224600
WRITE_ONCE(netdev->mtu, new_mtu);
46234601

46244602
if (netif_running(netdev)) {
4625-
iavf_schedule_reset(adapter, IAVF_FLAG_RESET_NEEDED);
4626-
ret = iavf_wait_for_reset(adapter);
4627-
if (ret < 0)
4628-
netdev_warn(netdev, "MTU change interrupted waiting for reset");
4629-
else if (ret)
4630-
netdev_warn(netdev, "MTU change timed out waiting for reset");
4603+
adapter->flags |= IAVF_FLAG_RESET_NEEDED;
4604+
iavf_reset_step(adapter);
46314605
}
46324606

4633-
return ret;
4607+
return 0;
46344608
}
46354609

46364610
/**
@@ -5435,9 +5409,6 @@ static int iavf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
54355409
/* Setup the wait queue for indicating transition to down status */
54365410
init_waitqueue_head(&adapter->down_waitqueue);
54375411

5438-
/* Setup the wait queue for indicating transition to running state */
5439-
init_waitqueue_head(&adapter->reset_waitqueue);
5440-
54415412
/* Setup the wait queue for indicating virtchannel events */
54425413
init_waitqueue_head(&adapter->vc_waitqueue);
54435414

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2736,7 +2736,6 @@ void iavf_virtchnl_completion(struct iavf_adapter *adapter,
27362736
case VIRTCHNL_OP_ENABLE_QUEUES:
27372737
/* enable transmits */
27382738
iavf_irq_enable(adapter, true);
2739-
wake_up(&adapter->reset_waitqueue);
27402739
adapter->flags &= ~IAVF_FLAG_QUEUES_DISABLED;
27412740
break;
27422741
case VIRTCHNL_OP_DISABLE_QUEUES:

0 commit comments

Comments
 (0)