Re: [PATCH v2 bpf-next 01/10] ice: allow toggling loopback mode via ndo_set_features callback

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Wed, Jun 15, 2022 at 04:01:51PM +0200, Maciej Fijalkowski wrote:
> On Wed, Jun 15, 2022 at 12:38:04PM +0200, Alexander Lobakin wrote:
> > From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > Date: Tue, 14 Jun 2022 19:47:40 +0200
> > 
> > > Add support for NETIF_F_LOOPBACK. Also, simplify checks throughout whole
> > > ice_set_features().
> > > 
> > > CC: Alexandr Lobakin <alexandr.lobakin@intel.com>
> > > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > > ---
> > >  drivers/net/ethernet/intel/ice/ice_main.c | 54 ++++++++++++++---------
> > >  1 file changed, 34 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> > > index e1cae253412c..ab00b0361e87 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice_main.c
> > > +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> > > @@ -3358,6 +3358,7 @@ static void ice_set_netdev_features(struct net_device *netdev)
> > >  	netdev->features |= netdev->hw_features;
> > >  
> > >  	netdev->hw_features |= NETIF_F_HW_TC;
> > > +	netdev->hw_features |= NETIF_F_LOOPBACK;
> > >  
> > >  	/* encap and VLAN devices inherit default, csumo and tso features */
> > >  	netdev->hw_enc_features |= dflt_features | csumo_features |
> > > @@ -5902,6 +5903,18 @@ ice_set_vlan_features(struct net_device *netdev, netdev_features_t features)
> > >  	return 0;
> > >  }
> > >  
> > > +static void ice_set_loopback(struct ice_pf *pf, struct net_device *netdev, bool ena)
> > 
> > I feel like the first argument is redundant in here since we can do
> > 
> > 	const struct ice_netdev_priv *np = netdev_priv(netdev);
> > 	struct ice_pf *pf = np->vsi->back;
> > 
> > But at the same time one function argument can be cheaper than two
> > jumps, so up to you.
> > 
> > > +{
> > > +	bool if_running = netif_running(netdev);
> > > +
> > > +	if (if_running)
> > > +		ice_stop(netdev);
> > > +	if (ice_aq_set_mac_loopback(&pf->hw, ena, NULL))
> > > +		dev_err(ice_pf_to_dev(pf), "Failed to toggle loopback state\n");
> > 
> > netdev_err() instead probably? I guess dev_err() is used for
> > consistency with the rest of ice_set_features(), but I'd recommend
> > using the former anyways.
> 
> So let's use netdev_err and drop the pf from arguments that are passed

brain fart, we need ice_hw ptr for ice_aq_set_mac_loopback(), so i'll pass
this then.

> 
> > 
> > > +	if (if_running)
> > > +		ice_open(netdev);
> > > +}
> > > +

(...)