| Message ID | 20220614174749.901044-2-maciej.fijalkowski@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | AF_XDP ZC selftests | expand |
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. > + if (if_running) > + ice_open(netdev); > +} > + > /** > * ice_set_features - set the netdev feature flags > * @netdev: ptr to the netdev being adjusted > @@ -5910,6 +5923,7 @@ ice_set_vlan_features(struct net_device *netdev, netdev_features_t features) > static int > ice_set_features(struct net_device *netdev, netdev_features_t features) > { > + netdev_features_t changed = netdev->features ^ features; > struct ice_netdev_priv *np = netdev_priv(netdev); > struct ice_vsi *vsi = np->vsi; > struct ice_pf *pf = vsi->back; > @@ -5917,37 +5931,33 @@ ice_set_features(struct net_device *netdev, netdev_features_t features) > > /* Don't set any netdev advanced features with device in Safe Mode */ > if (ice_is_safe_mode(vsi->back)) { > - dev_err(ice_pf_to_dev(vsi->back), "Device is in Safe Mode - not enabling advanced netdev features\n"); > + dev_err(ice_pf_to_dev(vsi->back), > + "Device is in Safe Mode - not enabling advanced netdev features\n"); This (I mean the whole ice_set_features() cleanup) deserves to be in a separate patch to me. In can be a past of this series for sure. > return ret; > } > > /* Do not change setting during reset */ > if (ice_is_reset_in_progress(pf->state)) { > - dev_err(ice_pf_to_dev(vsi->back), "Device is resetting, changing advanced netdev features temporarily unavailable.\n"); > + dev_err(ice_pf_to_dev(vsi->back), > + "Device is resetting, changing advanced netdev features temporarily unavailable.\n"); > return -EBUSY; > } > > /* Multiple features can be changed in one call so keep features in > * separate if/else statements to guarantee each feature is checked > */ > - if (features & NETIF_F_RXHASH && !(netdev->features & NETIF_F_RXHASH)) > - ice_vsi_manage_rss_lut(vsi, true); > - else if (!(features & NETIF_F_RXHASH) && > - netdev->features & NETIF_F_RXHASH) > - ice_vsi_manage_rss_lut(vsi, false); > + if (changed & NETIF_F_RXHASH) > + ice_vsi_manage_rss_lut(vsi, !!(features & NETIF_F_RXHASH)); > > ret = ice_set_vlan_features(netdev, features); > if (ret) > return ret; > > - if ((features & NETIF_F_NTUPLE) && > - !(netdev->features & NETIF_F_NTUPLE)) { > - ice_vsi_manage_fdir(vsi, true); > - ice_init_arfs(vsi); > - } else if (!(features & NETIF_F_NTUPLE) && > - (netdev->features & NETIF_F_NTUPLE)) { > - ice_vsi_manage_fdir(vsi, false); > - ice_clear_arfs(vsi); > + if (changed & NETIF_F_NTUPLE) { > + bool ena = !!(features & NETIF_F_NTUPLE); > + > + ice_vsi_manage_fdir(vsi, ena); > + ena ? ice_init_arfs(vsi) : ice_clear_arfs(vsi); > } > > /* don't turn off hw_tc_offload when ADQ is already enabled */ > @@ -5956,11 +5966,15 @@ ice_set_features(struct net_device *netdev, netdev_features_t features) > return -EACCES; > } > > - if ((features & NETIF_F_HW_TC) && > - !(netdev->features & NETIF_F_HW_TC)) > - set_bit(ICE_FLAG_CLS_FLOWER, pf->flags); > - else > - clear_bit(ICE_FLAG_CLS_FLOWER, pf->flags); > + if (changed & NETIF_F_HW_TC) { > + bool ena = !!(features & NETIF_F_HW_TC); > + > + ena ? set_bit(ICE_FLAG_CLS_FLOWER, pf->flags) : > + clear_bit(ICE_FLAG_CLS_FLOWER, pf->flags); > + } > + > + if (changed & NETIF_F_LOOPBACK) > + ice_set_loopback(pf, netdev, !!(features & NETIF_F_LOOPBACK)); > > return 0; > } > -- > 2.27.0 The rest is good, I like wiring up standard interfaces with the existing hardware functionality :) Loopback mode is useful for testing stuff. Thanks! Olek
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 > > > + if (if_running) > > + ice_open(netdev); > > +} > > + > > /** > > * ice_set_features - set the netdev feature flags > > * @netdev: ptr to the netdev being adjusted > > @@ -5910,6 +5923,7 @@ ice_set_vlan_features(struct net_device *netdev, netdev_features_t features) > > static int > > ice_set_features(struct net_device *netdev, netdev_features_t features) > > { > > + netdev_features_t changed = netdev->features ^ features; > > struct ice_netdev_priv *np = netdev_priv(netdev); > > struct ice_vsi *vsi = np->vsi; > > struct ice_pf *pf = vsi->back; > > @@ -5917,37 +5931,33 @@ ice_set_features(struct net_device *netdev, netdev_features_t features) > > > > /* Don't set any netdev advanced features with device in Safe Mode */ > > if (ice_is_safe_mode(vsi->back)) { > > - dev_err(ice_pf_to_dev(vsi->back), "Device is in Safe Mode - not enabling advanced netdev features\n"); > > + dev_err(ice_pf_to_dev(vsi->back), > > + "Device is in Safe Mode - not enabling advanced netdev features\n"); > > This (I mean the whole ice_set_features() cleanup) deserves to be in > a separate patch to me. In can be a past of this series for sure. ack, will pull out this to a standalone patch followed by loopback enablement > > > return ret; > > } > > > > /* Do not change setting during reset */ > > if (ice_is_reset_in_progress(pf->state)) { > > - dev_err(ice_pf_to_dev(vsi->back), "Device is resetting, changing advanced netdev features temporarily unavailable.\n"); > > + dev_err(ice_pf_to_dev(vsi->back), > > + "Device is resetting, changing advanced netdev features temporarily unavailable.\n"); > > return -EBUSY; > > } > > > > /* Multiple features can be changed in one call so keep features in > > * separate if/else statements to guarantee each feature is checked > > */ > > - if (features & NETIF_F_RXHASH && !(netdev->features & NETIF_F_RXHASH)) > > - ice_vsi_manage_rss_lut(vsi, true); > > - else if (!(features & NETIF_F_RXHASH) && > > - netdev->features & NETIF_F_RXHASH) > > - ice_vsi_manage_rss_lut(vsi, false); > > + if (changed & NETIF_F_RXHASH) > > + ice_vsi_manage_rss_lut(vsi, !!(features & NETIF_F_RXHASH)); > > > > ret = ice_set_vlan_features(netdev, features); > > if (ret) > > return ret; > > > > - if ((features & NETIF_F_NTUPLE) && > > - !(netdev->features & NETIF_F_NTUPLE)) { > > - ice_vsi_manage_fdir(vsi, true); > > - ice_init_arfs(vsi); > > - } else if (!(features & NETIF_F_NTUPLE) && > > - (netdev->features & NETIF_F_NTUPLE)) { > > - ice_vsi_manage_fdir(vsi, false); > > - ice_clear_arfs(vsi); > > + if (changed & NETIF_F_NTUPLE) { > > + bool ena = !!(features & NETIF_F_NTUPLE); > > + > > + ice_vsi_manage_fdir(vsi, ena); > > + ena ? ice_init_arfs(vsi) : ice_clear_arfs(vsi); > > } > > > > /* don't turn off hw_tc_offload when ADQ is already enabled */ > > @@ -5956,11 +5966,15 @@ ice_set_features(struct net_device *netdev, netdev_features_t features) > > return -EACCES; > > } > > > > - if ((features & NETIF_F_HW_TC) && > > - !(netdev->features & NETIF_F_HW_TC)) > > - set_bit(ICE_FLAG_CLS_FLOWER, pf->flags); > > - else > > - clear_bit(ICE_FLAG_CLS_FLOWER, pf->flags); > > + if (changed & NETIF_F_HW_TC) { > > + bool ena = !!(features & NETIF_F_HW_TC); > > + > > + ena ? set_bit(ICE_FLAG_CLS_FLOWER, pf->flags) : > > + clear_bit(ICE_FLAG_CLS_FLOWER, pf->flags); > > + } > > + > > + if (changed & NETIF_F_LOOPBACK) > > + ice_set_loopback(pf, netdev, !!(features & NETIF_F_LOOPBACK)); > > > > return 0; > > } > > -- > > 2.27.0 > > The rest is good, I like wiring up standard interfaces with the > existing hardware functionality :) Loopback mode is useful for > testing stuff. thanks for review! > > Thanks! > Olek
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); > > > +} > > > + (...)
On Tue, 14 Jun 2022 19:47:40 +0200 Maciej Fijalkowski wrote: > + 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"); > + if (if_running) > + ice_open(netdev); Loopback or not, I don't think we should be accepting the shutdown -> set config -> pray approach in modern drivers. ice_open() seems to be allocating all the Rx memory, and can fail.
On Wed, Jun 15, 2022 at 04:47:40PM -0700, Jakub Kicinski wrote: > On Tue, 14 Jun 2022 19:47:40 +0200 Maciej Fijalkowski wrote: > > + 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"); > > + if (if_running) > > + ice_open(netdev); > > Loopback or not, I don't think we should be accepting the shutdown -> > set config -> pray approach in modern drivers. ice_open() seems to be > allocating all the Rx memory, and can fail. They say that those who sing pray twice, so why don't we sing? :) But seriously, I'll degrade this to ice_down/up and check retvals. I think I just mimicked flow from ice_self_test(), which should be fixed as well... I'll send v4.
On Thu, 16 Jun 2022 18:00:40 +0200 Maciej Fijalkowski wrote: > > Loopback or not, I don't think we should be accepting the shutdown -> > > set config -> pray approach in modern drivers. ice_open() seems to be > > allocating all the Rx memory, and can fail. > > They say that those who sing pray twice, so why don't we sing? :) > > But seriously, I'll degrade this to ice_down/up and check retvals. I think > I just mimicked flow from ice_self_test(), which should be fixed as > well... > > I'll send v4. checking retval is not enough, does ice not have the ability to allocate resources upfront? I think iavf was already restructured to follow the "resilient" paradigm, time for ice to follow suit? This is something DaveM complained about in the first Ethernet driver I upstreamed, which must have been a decade ago by now. It's time we all get on board :)
On Thu, Jun 16, 2022 at 09:47:40AM -0700, Jakub Kicinski wrote: > On Thu, 16 Jun 2022 18:00:40 +0200 Maciej Fijalkowski wrote: > > > Loopback or not, I don't think we should be accepting the shutdown -> > > > set config -> pray approach in modern drivers. ice_open() seems to be > > > allocating all the Rx memory, and can fail. > > > > They say that those who sing pray twice, so why don't we sing? :) > > > > But seriously, I'll degrade this to ice_down/up and check retvals. I think > > I just mimicked flow from ice_self_test(), which should be fixed as > > well... > > > > I'll send v4. > > checking retval is not enough, does ice not have the ability to > allocate resources upfront? I think iavf was already restructured > to follow the "resilient" paradigm, time for ice to follow suit? I'm not aware of such restructure TBH. FWIW ice_down() won't free irqs/rings. I said I'll switch to it plus check its retval whereas I feel like you took it like I would want to keep the ice_stop() and check its retval. > > This is something DaveM complained about in the first Ethernet driver > I upstreamed, which must have been a decade ago by now. It's time we > all get on board :)
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) +{ + 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"); + if (if_running) + ice_open(netdev); +} + /** * ice_set_features - set the netdev feature flags * @netdev: ptr to the netdev being adjusted @@ -5910,6 +5923,7 @@ ice_set_vlan_features(struct net_device *netdev, netdev_features_t features) static int ice_set_features(struct net_device *netdev, netdev_features_t features) { + netdev_features_t changed = netdev->features ^ features; struct ice_netdev_priv *np = netdev_priv(netdev); struct ice_vsi *vsi = np->vsi; struct ice_pf *pf = vsi->back; @@ -5917,37 +5931,33 @@ ice_set_features(struct net_device *netdev, netdev_features_t features) /* Don't set any netdev advanced features with device in Safe Mode */ if (ice_is_safe_mode(vsi->back)) { - dev_err(ice_pf_to_dev(vsi->back), "Device is in Safe Mode - not enabling advanced netdev features\n"); + dev_err(ice_pf_to_dev(vsi->back), + "Device is in Safe Mode - not enabling advanced netdev features\n"); return ret; } /* Do not change setting during reset */ if (ice_is_reset_in_progress(pf->state)) { - dev_err(ice_pf_to_dev(vsi->back), "Device is resetting, changing advanced netdev features temporarily unavailable.\n"); + dev_err(ice_pf_to_dev(vsi->back), + "Device is resetting, changing advanced netdev features temporarily unavailable.\n"); return -EBUSY; } /* Multiple features can be changed in one call so keep features in * separate if/else statements to guarantee each feature is checked */ - if (features & NETIF_F_RXHASH && !(netdev->features & NETIF_F_RXHASH)) - ice_vsi_manage_rss_lut(vsi, true); - else if (!(features & NETIF_F_RXHASH) && - netdev->features & NETIF_F_RXHASH) - ice_vsi_manage_rss_lut(vsi, false); + if (changed & NETIF_F_RXHASH) + ice_vsi_manage_rss_lut(vsi, !!(features & NETIF_F_RXHASH)); ret = ice_set_vlan_features(netdev, features); if (ret) return ret; - if ((features & NETIF_F_NTUPLE) && - !(netdev->features & NETIF_F_NTUPLE)) { - ice_vsi_manage_fdir(vsi, true); - ice_init_arfs(vsi); - } else if (!(features & NETIF_F_NTUPLE) && - (netdev->features & NETIF_F_NTUPLE)) { - ice_vsi_manage_fdir(vsi, false); - ice_clear_arfs(vsi); + if (changed & NETIF_F_NTUPLE) { + bool ena = !!(features & NETIF_F_NTUPLE); + + ice_vsi_manage_fdir(vsi, ena); + ena ? ice_init_arfs(vsi) : ice_clear_arfs(vsi); } /* don't turn off hw_tc_offload when ADQ is already enabled */ @@ -5956,11 +5966,15 @@ ice_set_features(struct net_device *netdev, netdev_features_t features) return -EACCES; } - if ((features & NETIF_F_HW_TC) && - !(netdev->features & NETIF_F_HW_TC)) - set_bit(ICE_FLAG_CLS_FLOWER, pf->flags); - else - clear_bit(ICE_FLAG_CLS_FLOWER, pf->flags); + if (changed & NETIF_F_HW_TC) { + bool ena = !!(features & NETIF_F_HW_TC); + + ena ? set_bit(ICE_FLAG_CLS_FLOWER, pf->flags) : + clear_bit(ICE_FLAG_CLS_FLOWER, pf->flags); + } + + if (changed & NETIF_F_LOOPBACK) + ice_set_loopback(pf, netdev, !!(features & NETIF_F_LOOPBACK)); return 0; }
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(-)