RE: [PATCH net v4 4/4] ice: skip unnecessary VF reset when setting trust

Loktionov, Aleksandr
2026-04-24T16:05:23Z

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
> Of Loktionov, Aleksandr
> Sent: Thursday, April 23, 2026 3:17 PM
> To: Jose Ignacio Tornos Martinez <jtornosm@redhat.com>;
> netdev@vger.kernel.org
> Cc: intel-wired-lan@lists.osuosl.org; Kitszel, Przemyslaw
> <przemyslaw.kitszel@intel.com>; Keller, Jacob E
> <jacob.e.keller@intel.com>; horms@kernel.org;
> jesse.brandeburg@intel.com; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com
> Subject: Re: [Intel-wired-lan] [PATCH net v4 4/4] ice: skip
> unnecessary VF reset when setting trust
>
>
>
> > -----Original Message-----
> > From: Jose Ignacio Tornos Martinez <jtornosm@redhat.com>
> > Sent: Thursday, April 23, 2026 3:04 PM
> > To: netdev@vger.kernel.org
> > Cc: intel-wired-lan@lists.osuosl.org; Kitszel, Przemyslaw
> > <przemyslaw.kitszel@intel.com>; Loktionov, Aleksandr
> > <aleksandr.loktionov@intel.com>; Keller, Jacob E
> > <jacob.e.keller@intel.com>; horms@kernel.org;
> > jesse.brandeburg@intel.com; Nguyen, Anthony L
> > <anthony.l.nguyen@intel.com>; davem@davemloft.net;
> > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; Jose
> Ignacio
> > Tornos Martinez <jtornosm@redhat.com>
> > Subject: [PATCH net v4 4/4] ice: skip unnecessary VF reset when
> > setting trust
> >
> > Similar to the i40e fix, ice_set_vf_trust() unconditionally calls
> > ice_reset_vf() when the trust setting changes. While the delay is
> > smaller than i40e this reset is still unnecessary in most cases.
> >
> > Additionally, the original code has a race condition: it deletes MAC
> > LLDP filters BEFORE resetting the VF. During this deletion, the VF
> is
> > still ACTIVE and can add new MAC LLDP filters concurrently,
> > potentially corrupting the filter list.
> >
> > When granting trust, no reset is needed - we can just set the
> > capability flag to allow privileged operations.
> >
> > When revoking trust, we need to:
> > 1. Clear the capability flag to block privileged operations 2.
> Disable
> > promiscuous mode if it was enabled (trusted VFs can enable it) 3.
> Only
> > reset if MAC LLDP filters exist (to clean them up)
> >
> > When we do reset (MAC LLDP case), we fix the race condition by
> > resetting first to clear VF state (which blocks new MAC LLDP filter
> > additions), then delete existing filters safely. During cleanup, vf-
> > >trusted remains true so
> > ice_vf_is_lldp_ena() works properly. Only after cleanup do we set
> > vf->trusted = false.
> >
> > When we don't reset, we manually handle capability flag and
> > promiscuous mode via helper function.
> >
> > The ice driver already has logic to clean up MAC LLDP filters when
> > removing trust. After this cleanup, the VF reset is only necessary
> if
> > there were actually filters to remove (num_mac_lldp was non-zero).
> >
> > This saves time and eliminates unnecessary service disruption when
> > changing VF trust settings in most cases, while properly handling
> > filter cleanup.
> >
> > Fixes: 2296345416b0 ("ice: receive LLDP on trusted VFs")
> For me it looks like cc: stable@vger.kernel.org must be added
>
> > Signed-off-by: Jose Ignacio Tornos Martinez <jtornosm@redhat.com>
> > ---
> > v4:
> > - Address AI review (sashiko.dev) from Simon Horman:
> > vf->trusted ordering bug
> > - Fix upstream race condition when comparing with i40e code
> > - Apply capability flag and promiscuous mode fixes from i40e AI
> > review
> > - Add helper function ice_setup_vf_trust() for non-reset path
> > - Export ice_vf_clear_all_promisc_modes() for code reuse
> > v3: https://lore.kernel.org/all/20260414110006.124286-5-
> > jtornosm@redhat.com/
> >
> > drivers/net/ethernet/intel/ice/ice_sriov.c | 41
> +++++++++++++++++++-
> > - drivers/net/ethernet/intel/ice/ice_vf_lib.c | 2 +-
> > drivers/net/ethernet/intel/ice/ice_vf_lib.h | 1 +
> > 3 files changed, 39 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c
> > b/drivers/net/ethernet/intel/ice/ice_sriov.c
> > index 7e00e091756d..d0da7f6adc23 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_sriov.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
> > @@ -1364,6 +1364,34 @@ int ice_set_vf_mac(struct net_device *netdev,
> > int vf_id, u8 *mac)
> > return __ice_set_vf_mac(ice_netdev_to_pf(netdev), vf_id, mac);
> }
> >
> > +/**
> > + * ice_setup_vf_trust - Enable/disable VF trust mode without reset
> > + * @vf: VF to configure
> > + * @setting: trust setting
> > + *
> > + * Manually handle capability flag and promiscuous mode when
> changing
> > +trust
> > + * without performing a VF reset.
> > + * When reset is performed, this is not necessary as the reset
> > +procedure
> > + * already handles this.
> > + **/
One more nit, kdoc should end with '*/' not '**/'

With the best regards
Alex

> > +static void ice_setup_vf_trust(struct ice_vf *vf, bool setting) {
> > + struct ice_vsi *vsi;
> > +
> > + if (setting) {
> > + set_bit(ICE_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps);
> > + } else {
> > + clear_bit(ICE_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps);
> > +
> > + if (test_bit(ICE_VF_STATE_UC_PROMISC, vf->vf_states) ||
> > + test_bit(ICE_VF_STATE_MC_PROMISC, vf->vf_states)) {
> > + vsi = ice_get_vf_vsi(vf);
> > + if (vsi)
> > + ice_vf_clear_all_promisc_modes(vf, vsi);
> You declare ice_vf_clear_all_promisc_modes() returning int, but ignore
> the return value.
> Looks suspicious isn't it?
>
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
>
> > + }
> > + }
> > +}
> > +
> > /**
> > * ice_set_vf_trust
> > * @netdev: network interface device structure @@ -1399,11 +1427,16
> > @@ int ice_set_vf_trust(struct net_device *netdev, int vf_id, bool
> > trusted)
> >
> > mutex_lock(&vf->cfg_lock);
> >
> > - while (!trusted && vf->num_mac_lldp)
> > - ice_vf_update_mac_lldp_num(vf, ice_get_vf_vsi(vf),
> > false);
> > -
> > + /* Reset only if revoking trust with MAC LLDP filters */
> > + if (!trusted && vf->num_mac_lldp) {
> > + ice_reset_vf(vf, ICE_VF_RESET_NOTIFY);
> > + while (vf->num_mac_lldp)
> > + ice_vf_update_mac_lldp_num(vf,
> > ice_get_vf_vsi(vf), false);
> > + } else {
> > + ice_setup_vf_trust(vf, trusted);
> > + }
> > vf->trusted = trusted;
> > - ice_reset_vf(vf, ICE_VF_RESET_NOTIFY);
> > +
> > dev_info(ice_pf_to_dev(pf), "VF %u is now %strusted\n",
> > vf_id, trusted ? "" : "un");
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c
> > b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
> > index c8bc952f05cd..81bbf30e5c29 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
> > @@ -623,7 +623,7 @@ ice_vf_get_promisc_masks(struct ice_vf *vf,
> struct
> > ice_vsi *vsi,
> > *
> > * Clear all promiscuous/allmulticast filters for a VF
> > */
> > -static int
> > +int
> > ice_vf_clear_all_promisc_modes(struct ice_vf *vf, struct ice_vsi
> > *vsi) {
> > struct ice_pf *pf = vf->pf;
> > diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.h
> > b/drivers/net/ethernet/intel/ice/ice_vf_lib.h
> > index 7a9c75d1d07c..a3501bd92311 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.h
> > +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.h
> > @@ -310,6 +310,7 @@ bool ice_is_any_vf_in_unicast_promisc(struct
> > ice_pf *pf); void ice_vf_get_promisc_masks(struct ice_vf *vf,
> struct
> > ice_vsi *vsi,
> > u8 *ucast_m, u8 *mcast_m);
> > +int ice_vf_clear_all_promisc_modes(struct ice_vf *vf, struct
> ice_vsi
> > +*vsi);
> > int
> > ice_vf_set_vsi_promisc(struct ice_vf *vf, struct ice_vsi *vsi, u8
> > promisc_m); int
> > --
> > 2.53.0