| Message ID | 20220616180609.905015-2-maciej.fijalkowski@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | AF_XDP ZC selftests | expand |
Maciej Fijalkowski wrote: > Instead of rather verbose comparison of current netdev->features bits vs > the incoming ones from user, let us compress them by a helper features > set that will be the result of netdev->features XOR features. This way, > current, extensive branches: > > if (features & NETIF_F_BIT && !(netdev->features & NETIF_F_BIT)) > set_feature(true); > else if (!(features & NETIF_F_BIT) && netdev->features & NETIF_F_BIT) > set_feature(false); > > can become: > > netdev_features_t changed = netdev->features ^ features; > > if (changed & NETIF_F_BIT) > set_feature(!!(features & NETIF_F_BIT)); > > This is nothing new as currently several other drivers use this > approach, which I find much more convenient. Looks good couple nits below. Up to you if you want to follow through on them or not I don't have a strong opinion. For what its worth the other intel drivers also do the 'netdev->features ^ features' assignment. Acked-by: John Fastabend <john.fastabend@gmail.com> > > CC: Alexandr Lobakin <alexandr.lobakin@intel.com> > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> > --- > drivers/net/ethernet/intel/ice/ice_main.c | 40 +++++++++++------------ > 1 file changed, 19 insertions(+), 21 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c > index e1cae253412c..23d1b1fc39fb 100644 > --- a/drivers/net/ethernet/intel/ice/ice_main.c > +++ b/drivers/net/ethernet/intel/ice/ice_main.c > @@ -5910,44 +5910,41 @@ 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; > int ret = 0; > > /* 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"); > + if (ice_is_safe_mode(pf)) { > + dev_err(ice_pf_to_dev(vsi->back), bit of nitpicking but if you use pf in the 'if' above why not use it here as well and save a few keys. Also matches below then. > + "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(pf), > + "Device is resetting, changing advanced netdev features temporarily unavailable.\n"); > return -EBUSY; > } > [...] > @@ -5956,11 +5953,12 @@ 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); > + } Just a note you changed the logic slightly here. Above you always clear the bit. But, it looks like it doesn't matter caveat being I don't know what might happen in hardware. > > return 0; > } > -- > 2.27.0 >
On Fri, Jun 17, 2022 at 06:46:35PM -0700, John Fastabend wrote: > Maciej Fijalkowski wrote: > > Instead of rather verbose comparison of current netdev->features bits vs > > the incoming ones from user, let us compress them by a helper features > > set that will be the result of netdev->features XOR features. This way, > > current, extensive branches: > > > > if (features & NETIF_F_BIT && !(netdev->features & NETIF_F_BIT)) > > set_feature(true); > > else if (!(features & NETIF_F_BIT) && netdev->features & NETIF_F_BIT) > > set_feature(false); > > > > can become: > > > > netdev_features_t changed = netdev->features ^ features; > > > > if (changed & NETIF_F_BIT) > > set_feature(!!(features & NETIF_F_BIT)); > > > > This is nothing new as currently several other drivers use this > > approach, which I find much more convenient. > > Looks good couple nits below. Up to you if you want to follow through > on them or not I don't have a strong opinion. For what its worth the > other intel drivers also do the 'netdev->features ^ features' > assignment. > > Acked-by: John Fastabend <john.fastabend@gmail.com> > > > > > CC: Alexandr Lobakin <alexandr.lobakin@intel.com> > > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> > > --- > > drivers/net/ethernet/intel/ice/ice_main.c | 40 +++++++++++------------ > > 1 file changed, 19 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c > > index e1cae253412c..23d1b1fc39fb 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_main.c > > +++ b/drivers/net/ethernet/intel/ice/ice_main.c > > @@ -5910,44 +5910,41 @@ 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; > > int ret = 0; > > > > /* 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"); > > + if (ice_is_safe_mode(pf)) { > > + dev_err(ice_pf_to_dev(vsi->back), > > bit of nitpicking but if you use pf in the 'if' above why not use it here > as well and save a few keys. Also matches below then. Hey John thanks for all of the input! You're right above, I just forgot to change it to use 'pf' in ice_pf_to_dev(). Will fix. > > > + "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(pf), > > + "Device is resetting, changing advanced netdev features temporarily unavailable.\n"); > > return -EBUSY; > > } > > > > [...] > > > @@ -5956,11 +5953,12 @@ 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); > > + } > > Just a note you changed the logic slightly here. Above you always > clear the bit. But, it looks like it doesn't matter caveat being > I don't know what might happen in hardware. I believe that previous logic was wrong due to the fact that in case NETIF_F_HW_TC was set but in current ice_set_features() was not modified then this setting would be lost. > > > > > return 0; > > } > > -- > > 2.27.0 > > > >
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c index e1cae253412c..23d1b1fc39fb 100644 --- a/drivers/net/ethernet/intel/ice/ice_main.c +++ b/drivers/net/ethernet/intel/ice/ice_main.c @@ -5910,44 +5910,41 @@ 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; int ret = 0; /* 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"); + if (ice_is_safe_mode(pf)) { + 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(pf), + "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 +5953,12 @@ 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); + } return 0; }
Instead of rather verbose comparison of current netdev->features bits vs the incoming ones from user, let us compress them by a helper features set that will be the result of netdev->features XOR features. This way, current, extensive branches: if (features & NETIF_F_BIT && !(netdev->features & NETIF_F_BIT)) set_feature(true); else if (!(features & NETIF_F_BIT) && netdev->features & NETIF_F_BIT) set_feature(false); can become: netdev_features_t changed = netdev->features ^ features; if (changed & NETIF_F_BIT) set_feature(!!(features & NETIF_F_BIT)); This is nothing new as currently several other drivers use this approach, which I find much more convenient. CC: Alexandr Lobakin <alexandr.lobakin@intel.com> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> --- drivers/net/ethernet/intel/ice/ice_main.c | 40 +++++++++++------------ 1 file changed, 19 insertions(+), 21 deletions(-)