Re: [net-next,3/9] ice: migrate to netdev ops lock

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Jakub Kicinski <kuba@kernel.org>
Date: Tue, 10 Feb 2026 20:24:58 -0800

Hi,

> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> 
> For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
> ---
> ice: migrate to netdev ops lock
> 
> This commit migrates the ice driver to use the netdev ops lock by
> requesting ops locking and using _locked counterparts of NAPI
> configuration functions to avoid deadlocks during suspend/resume, queue
> rebuild and resets.
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
>> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> 
> [ ... ]
> 
>> @@ -2308,10 +2308,14 @@ static int ice_vsi_cfg_tc_lan(struct ice_pf *pf, struct ice_vsi *vsi)
>> }
>>
>> /**
>> - * ice_vsi_cfg_def - configure default VSI based on the type
>> + * ice_vsi_cfg_def_locked - configure default VSI based on the type
>>  * @vsi: pointer to VSI
>> + *
>> + * Should be called only with the netdev lock taken.
>> + *
>> + * Return: 0 on success, -errno on failure.
>>  */
>> -static int ice_vsi_cfg_def(struct ice_vsi *vsi)
>> +static int ice_vsi_cfg_def_locked(struct ice_vsi *vsi)
> 
> [ ... ]
> 
>> +unroll_vector_base:
>> +	/* reclaim SW interrupts back to the common pool */
>> +unroll_alloc_q_vector:
>> +	ice_vsi_free_q_vectors(vsi);
> 
> Should this be ice_vsi_free_q_vectors_locked(vsi)?
> 
> ice_vsi_cfg_def_locked() is called with the netdev lock held (either by
> ice_vsi_cfg_def() wrapper or directly from callers like
> ice_vsi_rebuild_locked()). The error path here calls ice_vsi_free_q_vectors()
> which tries to acquire the netdev lock:
> 
> ice_vsi_cfg_def_locked() [netdev lock held]
>   -> ice_vsi_free_q_vectors()
>      -> netdev_lock(dev)  /* deadlock - already held */
> 
> This would deadlock when an error triggers unroll_vector_base or
> unroll_alloc_q_vector paths (e.g., memory allocation failure in
> ice_vsi_alloc_rings() or ice_vsi_alloc_ring_stats()).
> 
> [ ... ]
> 
>> @@ -2703,7 +2748,7 @@ void ice_vsi_close(struct ice_vsi *vsi)
>> {
>> 	if (!test_and_set_bit(ICE_VSI_DOWN, vsi->state))
>> 		ice_down(vsi);
>>
>> -	ice_vsi_clear_napi_queues(vsi);
>> +	ice_vsi_clear_napi_queues_locked(vsi);
> 
> ice_vsi_clear_napi_queues_locked() expects the netdev lock to be held.
> Are all callers of ice_vsi_close() holding the netdev lock?
> 
> Looking at ice_sf_dev_remove():
> 
> ice_sf_dev_remove()
>   -> ice_vsi_close(vsi)  /* no netdev_lock held */
>   -> ice_sf_decfg_netdev()  /* unregisters netdev later */
> 
> And ice_dis_vsi() when !netif_running(vsi->netdev):
> 
> ice_dis_vsi(vsi, false)
>   -> ice_vsi_close(vsi)  /* rtnl_lock only taken when netif_running */
> 
> Both paths call ice_vsi_close() without holding the netdev lock while
> the netdev is still registered, which could trigger
> netdev_assert_locked_or_invisible warnings.
> 
> [ ... ]
> 
>> @@ -3069,16 +3152,17 @@
>> }
>>
>> /**
>> - * ice_vsi_rebuild - Rebuild VSI after reset
>> + * ice_vsi_rebuild_locked - Rebuild VSI after reset
>>  * @vsi: VSI to be rebuild
>>  * @vsi_flags: flags used for VSI rebuild flow
>>  *
>>  * Set vsi_flags to ICE_VSI_FLAG_INIT to initialize a new VSI, or
>>  * ICE_VSI_FLAG_NO_INIT to rebuild an existing VSI in hardware.
>> + * Should be called only under the netdev lock.
>>  *
>>  * Returns 0 on success and negative value on failure
>>  */
>> -int ice_vsi_rebuild(struct ice_vsi *vsi, u32 vsi_flags)
>> +int ice_vsi_rebuild_locked(struct ice_vsi *vsi, u32 vsi_flags)
> 
> [ ... ]
> 
>> +free_coalesce:
>> +	kfree(coalesce);
>> +decfg:
>> +	if (ret)
>> +		ice_vsi_decfg(vsi);
> 
> Should this be ice_vsi_decfg_locked(vsi)?
> 
> ice_vsi_rebuild_locked() is called with the netdev lock already held
> (either by the ice_vsi_rebuild() wrapper or by callers like
> ice_vsi_recfg_qs()). The error path at label 'decfg:' calls ice_vsi_decfg()
> which tries to acquire the lock again:
> 
> ice_vsi_rebuild_locked() [netdev lock held]
>   -> ice_vsi_decfg()
>      -> netdev_lock(dev)  /* deadlock - already held */
> 
> This would deadlock when an error occurs after ice_vsi_cfg_def_locked()
> succeeds but a later operation fails.

Tony also fed the series to AI, two times, and each time he got a
different answer.
The series was on iwl-next for 1.5 months and only one bug was reported,
which I fixed immediately.

I can take a look into this, but wouldn't be better if we take the
series now and then have 2 months to fix bugs if any appears?

Thanks,
Olek