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

[Jakub Kicinski <kuba@kernel.org>]

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.
-- 
pw-bot: cr