Re: [PATCH net-next v2 3/5] ice: migrate to netdev ops lock

[Simon Horman <horms@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

Queue management ops unconditionally enable netdev locking. The same
lock is taken by default by several NAPI configuration functions, such
as napi_enable() and netif_napi_set_irq().  Request ops locking in
advance and make sure we use the _locked counterparts of those functions
to avoid deadlocks, taking the lock manually where needed (suspend/resume,
queue rebuild and resets).

> diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
> index bd77f1c001ee..2ca3fc6e8abd 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);
>  			else
> -				ice_dis_vsi(vsi, locked);
> +				ice_dis_vsi(vsi);
>  			break;
>  		default:
>  			continue;

[Simon says: this seems to be a false positive and what the AI
 suggests seems to be the exact opposite of what is correct]

The DCB callers all hold the RTNL lock rather than the netdev lock. While
ice_ena_vsi() and ice_dis_vsi() now take the netdev lock internally, their
_locked counterparts (ice_ena_vsi_locked and ice_dis_vsi_locked) would be
more appropriate here since the netdev lock is not held by ice_pf_dcb_cfg()
or ice_dcb_process_lldp_set_mib_change().

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

[ ... ]

> @@ -7263,7 +7271,7 @@ static void ice_napi_disable_all(struct ice_vsi *vsi)
>  		struct ice_q_vector *q_vector = vsi->q_vectors[q_idx];
>
>  		if (q_vector->rx.rx_ring || q_vector->tx.tx_ring)
> -			napi_disable(&q_vector->napi);
> +			napi_disable_locked(&q_vector->napi);
>
>  		cancel_work_sync(&q_vector->tx.dim.work);
>  		cancel_work_sync(&q_vector->rx.dim.work);

[Simon says: this one seems plausible to me. Could you take a look?]

Does this introduce a lockdep warning in the eswitch/switchdev path?

ice_napi_disable_all() is called via ice_down(), and ice_down() can be
reached through the eswitch path without holding the netdev lock:

ice_eswitch_attach()
  -> ice_eswitch_enable_switchdev()
    -> ice_eswitch_setup_env()
      -> ice_down(uplink_vsi)
        -> ice_napi_disable_all()
          -> napi_disable_locked()

At this point napi_disable_locked() will assert that the netdev lock is
held, but the eswitch code path doesn't take it. The uplink_vsi is the
main PF VSI with a valid netdev, so the early return won't trigger.

Should ice_eswitch_setup_env() take the netdev lock before calling
ice_down(), or should ice_down() have both locked and unlocked variants?