Re: [net-next,3/5] 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 netdev ops locking. Queue
management ops unconditionally enable netdev locking, so the driver now
uses _locked counterparts of NAPI configuration functions to avoid
deadlocks.

> diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
> index bd77f1c001ee..08157dc04ed6 100644
> --- a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
> @@ -273,14 +273,13 @@ void ice_vsi_cfg_dcb_rings(struct ice_vsi *vsi)
>  /**
>   * ice_dcb_ena_dis_vsi - disable certain VSIs for DCB config/reconfig
>   * @pf: pointer to the PF instance
>   * @ena: true to enable VSIs, false to disable
> - * @locked: true if caller holds RTNL lock, false otherwise
>   *
>   * Before a new DCB configuration can be applied, VSIs of type PF, SWITCHDEV
>   * and CHNL need to be brought down. Following completion of DCB configuration
>   * the VSIs that were downed need to be brought up again. This helper function
>   * does both.
>   */
> -static void ice_dcb_ena_dis_vsi(struct ice_pf *pf, bool ena, bool locked)
> +static void ice_dcb_ena_dis_vsi(struct ice_pf *pf, bool ena)
>  {
>  	int i;
>
> @@ -294,9 +293,9 @@ static void ice_dcb_ena_dis_vsi(struct ice_pf *pf, bool ena, bool locked)
>  		case ICE_VSI_CHNL:
>  		case ICE_VSI_PF:
>  			if (ena)
> -				ice_ena_vsi(vsi, locked);
> +				ice_ena_vsi(vsi, true);
>  			else
> -				ice_dis_vsi(vsi, locked);
> +				ice_dis_vsi_locked(vsi);

Does ice_dcb_ena_dis_vsi() need to acquire the netdev lock before calling
ice_dis_vsi_locked() and ice_ena_vsi()?

Looking at the disable path, ice_dis_vsi_locked() calls ice_vsi_close()
which goes through ice_down() -> ice_napi_disable_all() ->
napi_disable_locked(), and that function has netdev_assert_locked(n->dev) in
net/core/dev.c. Similarly, ice_vsi_close() calls
ice_vsi_clear_napi_queues_locked() -> netif_napi_set_irq_locked() which
checks netdev_assert_locked_or_invisible().

The enable path calls ice_ena_vsi() -> ice_open_internal() ->
ice_vsi_open(), and that now calls ice_vsi_set_napi_queues_locked() ->
netif_napi_set_irq_locked() (requires netdev lock) and ice_up_complete() ->
ice_napi_enable_all() -> napi_enable_locked().

The old code called ice_dis_vsi(vsi, locked) and ice_ena_vsi(vsi, locked)
where the old ice_dis_vsi() only dealt with rtnl_lock. The new code switches
to _locked variants that require the netdev lock but does not acquire it.

Callers ice_pf_dcb_cfg() and ice_dcb_process_lldp_set_mib_change() hold
rtnl_lock but not the netdev lock.

[ ... ]

> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index e7308e381e2f..a8e2db6dc05a 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c

[ ... ]

> @@ -7563,7 +7570,7 @@ int ice_vsi_open(struct ice_vsi *vsi)
>  		if (err)
>  			goto err_set_qs;
>
> -		ice_vsi_set_napi_queues(vsi);
> +		ice_vsi_set_napi_queues_locked(vsi);
>  	}
>
>  	err = ice_up_complete(vsi);

Does ice_ena_vsi() need to take the netdev lock before calling
ice_open_internal()?

ice_vsi_open() now calls ice_vsi_set_napi_queues_locked() which requires the
netdev lock via netif_napi_set_irq_locked() -> netdev_assert_locked_or_
invisible(). It also calls ice_up_complete() -> ice_napi_enable_all() ->
napi_enable_locked().

However, ice_ena_vsi() in ice_lib.c calls ice_open_internal() ->
ice_vsi_open() while holding only rtnl_lock, not the netdev lock. This path
is reached during PF reset rebuild:

ice_rebuild()
    ice_vsi_rebuild_by_type()
        ice_ena_vsi(vsi, false)
            ice_open_internal()
                ice_vsi_open()

The netdev lock is NOT held at this point. ice_vsi_rebuild() took and
released it earlier in ice_vsi_rebuild_by_type().

Additionally, if ice_up_complete() fails, the error path goes through
ice_down() -> ice_napi_disable_all() -> napi_disable_locked(), which has an
explicit netdev_assert_locked(n->dev) check.
-- 
pw-bot: cr