| Message ID | 20220616180609.905015-3-maciej.fijalkowski@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | AF_XDP ZC selftests | expand |
On Thu, 16 Jun 2022 20:06:01 +0200 Maciej Fijalkowski wrote: > Add support for NETIF_F_LOOPBACK. This feature can be set via: > $ ethtool -K eth0 loopback <on|off> > > Feature can be useful for local data path tests. > > CC: Alexandr Lobakin <alexandr.lobakin@intel.com> > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> Acked-by: Jakub Kicinski <kuba@kernel.org>
Maciej Fijalkowski wrote: > Add support for NETIF_F_LOOPBACK. This feature can be set via: > $ ethtool -K eth0 loopback <on|off> > > Feature can be useful for local data path tests. > > CC: Alexandr Lobakin <alexandr.lobakin@intel.com> > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> > --- Patch looks fine one question about that ice_set_features() function though. Acked-by: John Fastabend <john.fastabend@gmail.com> [...] > +/** > + * ice_set_loopback - turn on/off loopback mode on underlying PF > + * @vsi: ptr to VSI > + * @ena: flag to indicate the on/off setting > + */ > +static int > +ice_set_loopback(struct ice_vsi *vsi, bool ena) > +{ > + bool if_running = netif_running(vsi->netdev); > + int ret; > + > + if (if_running && !test_and_set_bit(ICE_VSI_DOWN, vsi->state)) { > + ret = ice_down(vsi); > + if (ret) { > + netdev_err(vsi->netdev, "Preparing device to toggle loopback failed\n"); > + return ret; > + } > + } > + ret = ice_aq_set_mac_loopback(&vsi->back->hw, ena, NULL); > + if (ret) > + netdev_err(vsi->netdev, "Failed to toggle loopback state\n"); > + if (if_running) > + ret = ice_up(vsi); > + > + return ret; > +} > + > /** > * ice_set_features - set the netdev feature flags > * @netdev: ptr to the netdev being adjusted > @@ -5960,7 +5988,10 @@ ice_set_features(struct net_device *netdev, netdev_features_t features) > clear_bit(ICE_FLAG_CLS_FLOWER, pf->flags); > } > > - return 0; > + if (changed & NETIF_F_LOOPBACK) > + ret = ice_set_loopback(vsi, !!(features & NETIF_F_LOOPBACK)); > + > + return ret; Unrelated to your patch, but because you are messing with 'ret' here a bit, how come you return 0 when ice_is_safe_mode() shouldn't you push that error up so the user who is doing the setting knows it didn't actually work? > } > > /** > -- > 2.27.0 >
On Fri, Jun 17, 2022 at 06:57:41PM -0700, John Fastabend wrote: > Maciej Fijalkowski wrote: > > Add support for NETIF_F_LOOPBACK. This feature can be set via: > > $ ethtool -K eth0 loopback <on|off> > > > > Feature can be useful for local data path tests. > > > > CC: Alexandr Lobakin <alexandr.lobakin@intel.com> > > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> > > --- > > Patch looks fine one question about that ice_set_features() function > though. > > Acked-by: John Fastabend <john.fastabend@gmail.com> > > [...] > > > +/** > > + * ice_set_loopback - turn on/off loopback mode on underlying PF > > + * @vsi: ptr to VSI > > + * @ena: flag to indicate the on/off setting > > + */ > > +static int > > +ice_set_loopback(struct ice_vsi *vsi, bool ena) > > +{ > > + bool if_running = netif_running(vsi->netdev); > > + int ret; > > + > > + if (if_running && !test_and_set_bit(ICE_VSI_DOWN, vsi->state)) { > > + ret = ice_down(vsi); > > + if (ret) { > > + netdev_err(vsi->netdev, "Preparing device to toggle loopback failed\n"); > > + return ret; > > + } > > + } > > + ret = ice_aq_set_mac_loopback(&vsi->back->hw, ena, NULL); > > + if (ret) > > + netdev_err(vsi->netdev, "Failed to toggle loopback state\n"); > > + if (if_running) > > + ret = ice_up(vsi); > > + > > + return ret; > > +} > > + > > /** > > * ice_set_features - set the netdev feature flags > > * @netdev: ptr to the netdev being adjusted > > @@ -5960,7 +5988,10 @@ ice_set_features(struct net_device *netdev, netdev_features_t features) > > clear_bit(ICE_FLAG_CLS_FLOWER, pf->flags); > > } > > > > - return 0; > > + if (changed & NETIF_F_LOOPBACK) > > + ret = ice_set_loopback(vsi, !!(features & NETIF_F_LOOPBACK)); > > + > > + return ret; > > Unrelated to your patch, but because you are messing with 'ret' here a bit, > how come you return 0 when ice_is_safe_mode() shouldn't you push that > error up so the user who is doing the setting knows it didn't actually > work? Safe mode is the first thing checked in ice_set_features() and in case it is set we give the message to user about it and return 0 immediately. So did you miss the immediate exit or are you suggesting that for safe mode we should return some error code, not 0 which is interpreted as 'successful' command execution? > > > } > > > > /** > > -- > > 2.27.0 > > > >
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c index 23d1b1fc39fb..5bdd515142ec 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,33 @@ ice_set_vlan_features(struct net_device *netdev, netdev_features_t features) return 0; } +/** + * ice_set_loopback - turn on/off loopback mode on underlying PF + * @vsi: ptr to VSI + * @ena: flag to indicate the on/off setting + */ +static int +ice_set_loopback(struct ice_vsi *vsi, bool ena) +{ + bool if_running = netif_running(vsi->netdev); + int ret; + + if (if_running && !test_and_set_bit(ICE_VSI_DOWN, vsi->state)) { + ret = ice_down(vsi); + if (ret) { + netdev_err(vsi->netdev, "Preparing device to toggle loopback failed\n"); + return ret; + } + } + ret = ice_aq_set_mac_loopback(&vsi->back->hw, ena, NULL); + if (ret) + netdev_err(vsi->netdev, "Failed to toggle loopback state\n"); + if (if_running) + ret = ice_up(vsi); + + return ret; +} + /** * ice_set_features - set the netdev feature flags * @netdev: ptr to the netdev being adjusted @@ -5960,7 +5988,10 @@ ice_set_features(struct net_device *netdev, netdev_features_t features) clear_bit(ICE_FLAG_CLS_FLOWER, pf->flags); } - return 0; + if (changed & NETIF_F_LOOPBACK) + ret = ice_set_loopback(vsi, !!(features & NETIF_F_LOOPBACK)); + + return ret; } /**
Add support for NETIF_F_LOOPBACK. This feature can be set via: $ ethtool -K eth0 loopback <on|off> Feature can be useful for local data path tests. CC: Alexandr Lobakin <alexandr.lobakin@intel.com> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> --- drivers/net/ethernet/intel/ice/ice_main.c | 33 ++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-)