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@xxxxxxxxx>
> > 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@xxxxxxxxx>
> > > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@xxxxxxxxx>
> > > ---
> > > 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);
> > > +}
> > > +
(...)
