RE: [Intel-wired-lan] [PATCH net v3] iavf: fix incorrect reset handling in callbacks

[Romanowski, Rafal]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@xxxxxxxxxx> On Behalf Of Petr
> Oros
> Sent: Wednesday, February 11, 2026 20:19
> To: netdev@xxxxxxxxxxxxxxx
> Cc: Vecera, Ivan <ivecera@xxxxxxxxxx>; shaojijie@xxxxxxxxxx; Kitszel,
> Przemyslaw <przemyslaw.kitszel@xxxxxxxxx>; Eric Dumazet
> <edumazet@xxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; Loktionov, Aleksandr
> <aleksandr.loktionov@xxxxxxxxx>; Andrew Lunn <andrew+netdev@xxxxxxx>;
> Stanislav Fomichev <sdf@xxxxxxxxxxx>; Nguyen, Anthony L
> <anthony.l.nguyen@xxxxxxxxx>; intel-wired-lan@xxxxxxxxxxxxxxxx; Keller, Jacob E
> <jacob.e.keller@xxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>; Paolo Abeni
> <pabeni@xxxxxxxxxx>; David S. Miller <davem@xxxxxxxxxxxxx>
> Subject: [Intel-wired-lan] [PATCH net v3] 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
> c2ed2403f12c ("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
> 53844673d555 ("iavf: kill "legacy-rx" for good").
> 
> Waiting in set_channels() was introduced earlier by commit 4e5e6b5d9d13
> ("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 ef490bbb2267 ("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 7e4d784f5810 ("net: hold netdev instance lock during rtnetlink
> operations") and commit 2bcf4772e45a ("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 120f28a6f314 ("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: 120f28a6f314 ("iavf: get rid of the crit lock")
> Reviewed-by: Jacob Keller <jacob.e.keller@xxxxxxxxx>
> Signed-off-by: Petr Oros <poros@xxxxxxxxxx>
> ---
>  drivers/net/ethernet/intel/iavf/iavf.h        |  3 +-
>  .../net/ethernet/intel/iavf/iavf_ethtool.c    | 19 ++---
>  drivers/net/ethernet/intel/iavf/iavf_main.c   | 77 ++++++-------------
>  .../net/ethernet/intel/iavf/iavf_virtchnl.c   |  1 -
>  4 files changed, 31 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/iavf/iavf.h
> b/drivers/net/ethernet/intel/iavf/iavf.h
> index a87e0c6d4017ad..e9fb0a0919e376 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf.h
> +++ b/drivers/net/ethernet/intel/iavf/iavf.h
> @@ -260,7 +260,6 @@ struct iavf_adapter {


Tested-by: Rafal Romanowski <rafal.romanowski@xxxxxxxxx>