Message ID | 20240710204015.124233-13-ahmed.zaki@intel.com |
---|---|
State | Changes Requested |
Headers | show |
Series | ice: iavf: add support for TC U32 filters on VFs | expand |
On Wed, Jul 10, 2024 at 02:40:14PM -0600, Ahmed Zaki wrote: > In preparation for a second type of FDIR filters that can be added by > tc-u32, move the add/del of the FDIR logic to be entirely contained in > iavf_fdir.c. > > The iavf_find_fdir_fltr_by_loc() is renamed to iavf_find_fdir_fltr() > to be more agnostic to the filter ID parameter (for now @loc, which is > relevant only to current FDIR filters added via ethtool). > > Reviewed-by: Sridhar Samudrala <sridhar.samudrala@intel.com> > Reviewed-by: Marcin Szycik <marcin.szycik@linux.intel.com> > Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com> > --- > drivers/net/ethernet/intel/iavf/iavf.h | 5 ++ > .../net/ethernet/intel/iavf/iavf_ethtool.c | 56 ++------------- > drivers/net/ethernet/intel/iavf/iavf_fdir.c | 72 +++++++++++++++++-- > drivers/net/ethernet/intel/iavf/iavf_fdir.h | 7 +- > 4 files changed, 83 insertions(+), 57 deletions(-) > > diff --git a/drivers/net/ethernet/intel/iavf/iavf.h b/drivers/net/ethernet/intel/iavf/iavf.h > index 23a6557fc3db..85bd6a85cf2d 100644 > --- a/drivers/net/ethernet/intel/iavf/iavf.h > +++ b/drivers/net/ethernet/intel/iavf/iavf.h > @@ -444,6 +444,11 @@ struct iavf_adapter { > spinlock_t adv_rss_lock; /* protect the RSS management list */ > }; > > +/* Must be called with fdir_fltr_lock lock held */ > +static inline bool iavf_fdir_max_reached(struct iavf_adapter *adapter) > +{ > + return (adapter->fdir_active_fltr >= IAVF_MAX_FDIR_FILTERS); nit: these parentheses seem unnecessary. > +} > > /* Ethtool Private Flags */ > ... > diff --git a/drivers/net/ethernet/intel/iavf/iavf_fdir.c b/drivers/net/ethernet/intel/iavf/iavf_fdir.c ... > +/** > + * iavf_fdir_del_fltr - delete a flow director filter from the list > + * @adapter: pointer to the VF adapter structure > + * @loc: location to delete. > + * > + * Return: 0 on success or negative errno on failure. > + */ > +int iavf_fdir_del_fltr(struct iavf_adapter *adapter, u32 loc) > +{ > + struct iavf_fdir_fltr *fltr = NULL; > + int err = 0; > + > + spin_lock_bh(&adapter->fdir_fltr_lock); > + fltr = iavf_find_fdir_fltr(adapter, loc); > + > + if (fltr) { > + if (fltr->state == IAVF_FDIR_FLTR_ACTIVE) { > + fltr->state = IAVF_FDIR_FLTR_DEL_REQUEST; > + } else if (fltr->state == IAVF_FDIR_FLTR_INACTIVE) { > + list_del(&fltr->list); > + kfree(fltr); > + adapter->fdir_active_fltr--; > + fltr = NULL; > + } else { > + err = -EBUSY; > + } > + } else if (adapter->fdir_active_fltr) { > + err = -EINVAL; > + } > + > + if (fltr && fltr->state == IAVF_FDIR_FLTR_DEL_REQUEST) > + iavf_schedule_aq_request(adapter, IAVF_FLAG_AQ_DEL_FDIR_FILTER); It seems that prior to this change the condition and call to iavf_schedule_aq_request were not protected by fdir_fltr_lock, and now they are. If so, is this change intentional. > + > + spin_unlock_bh(&adapter->fdir_fltr_lock); > + return err; > } ...
On 2024-07-22 9:04 a.m., Simon Horman wrote: > On Wed, Jul 10, 2024 at 02:40:14PM -0600, Ahmed Zaki wrote: >> In preparation for a second type of FDIR filters that can be added by >> tc-u32, move the add/del of the FDIR logic to be entirely contained in >> iavf_fdir.c. >> >> The iavf_find_fdir_fltr_by_loc() is renamed to iavf_find_fdir_fltr() >> to be more agnostic to the filter ID parameter (for now @loc, which is >> relevant only to current FDIR filters added via ethtool). >> >> Reviewed-by: Sridhar Samudrala <sridhar.samudrala@intel.com> >> Reviewed-by: Marcin Szycik <marcin.szycik@linux.intel.com> >> Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com> >> --- >> drivers/net/ethernet/intel/iavf/iavf.h | 5 ++ >> .../net/ethernet/intel/iavf/iavf_ethtool.c | 56 ++------------- >> drivers/net/ethernet/intel/iavf/iavf_fdir.c | 72 +++++++++++++++++-- >> drivers/net/ethernet/intel/iavf/iavf_fdir.h | 7 +- >> 4 files changed, 83 insertions(+), 57 deletions(-) >> >> diff --git a/drivers/net/ethernet/intel/iavf/iavf.h b/drivers/net/ethernet/intel/iavf/iavf.h >> index 23a6557fc3db..85bd6a85cf2d 100644 >> --- a/drivers/net/ethernet/intel/iavf/iavf.h >> +++ b/drivers/net/ethernet/intel/iavf/iavf.h >> @@ -444,6 +444,11 @@ struct iavf_adapter { >> spinlock_t adv_rss_lock; /* protect the RSS management list */ >> }; >> >> +/* Must be called with fdir_fltr_lock lock held */ >> +static inline bool iavf_fdir_max_reached(struct iavf_adapter *adapter) >> +{ >> + return (adapter->fdir_active_fltr >= IAVF_MAX_FDIR_FILTERS); > > nit: these parentheses seem unnecessary. > >> +} >> >> /* Ethtool Private Flags */ >> > > ... > >> diff --git a/drivers/net/ethernet/intel/iavf/iavf_fdir.c b/drivers/net/ethernet/intel/iavf/iavf_fdir.c > > ... > >> +/** >> + * iavf_fdir_del_fltr - delete a flow director filter from the list >> + * @adapter: pointer to the VF adapter structure >> + * @loc: location to delete. >> + * >> + * Return: 0 on success or negative errno on failure. >> + */ >> +int iavf_fdir_del_fltr(struct iavf_adapter *adapter, u32 loc) >> +{ >> + struct iavf_fdir_fltr *fltr = NULL; >> + int err = 0; >> + >> + spin_lock_bh(&adapter->fdir_fltr_lock); >> + fltr = iavf_find_fdir_fltr(adapter, loc); >> + >> + if (fltr) { >> + if (fltr->state == IAVF_FDIR_FLTR_ACTIVE) { >> + fltr->state = IAVF_FDIR_FLTR_DEL_REQUEST; >> + } else if (fltr->state == IAVF_FDIR_FLTR_INACTIVE) { >> + list_del(&fltr->list); >> + kfree(fltr); >> + adapter->fdir_active_fltr--; >> + fltr = NULL; >> + } else { >> + err = -EBUSY; >> + } >> + } else if (adapter->fdir_active_fltr) { >> + err = -EINVAL; >> + } >> + >> + if (fltr && fltr->state == IAVF_FDIR_FLTR_DEL_REQUEST) >> + iavf_schedule_aq_request(adapter, IAVF_FLAG_AQ_DEL_FDIR_FILTER); > > It seems that prior to this change the condition and call to > iavf_schedule_aq_request were not protected by fdir_fltr_lock, and now they > are. If so, is this change intentional. > yes it is, fltr is member of the list that should be protected by the spinlock. All other nits and changes will be part of v4. Thanks for the review. Ahmed
On Wed, Jul 24, 2024 at 10:14:19AM -0600, Ahmed Zaki wrote: ... > > > +/** > > > + * iavf_fdir_del_fltr - delete a flow director filter from the list > > > + * @adapter: pointer to the VF adapter structure > > > + * @loc: location to delete. > > > + * > > > + * Return: 0 on success or negative errno on failure. > > > + */ > > > +int iavf_fdir_del_fltr(struct iavf_adapter *adapter, u32 loc) > > > +{ > > > + struct iavf_fdir_fltr *fltr = NULL; > > > + int err = 0; > > > + > > > + spin_lock_bh(&adapter->fdir_fltr_lock); > > > + fltr = iavf_find_fdir_fltr(adapter, loc); > > > + > > > + if (fltr) { > > > + if (fltr->state == IAVF_FDIR_FLTR_ACTIVE) { > > > + fltr->state = IAVF_FDIR_FLTR_DEL_REQUEST; > > > + } else if (fltr->state == IAVF_FDIR_FLTR_INACTIVE) { > > > + list_del(&fltr->list); > > > + kfree(fltr); > > > + adapter->fdir_active_fltr--; > > > + fltr = NULL; > > > + } else { > > > + err = -EBUSY; > > > + } > > > + } else if (adapter->fdir_active_fltr) { > > > + err = -EINVAL; > > > + } > > > + > > > + if (fltr && fltr->state == IAVF_FDIR_FLTR_DEL_REQUEST) > > > + iavf_schedule_aq_request(adapter, IAVF_FLAG_AQ_DEL_FDIR_FILTER); > > > > It seems that prior to this change the condition and call to > > iavf_schedule_aq_request were not protected by fdir_fltr_lock, and now they > > are. If so, is this change intentional. > > > > yes it is, fltr is member of the list that should be protected by the > spinlock. Thanks, I would suggest moving this into a separate patch: changing locking is a bit different to refactoring. Or, if not, at least mentioning it in the patch description.
On 2024-07-24 10:30 a.m., Simon Horman wrote: > On Wed, Jul 24, 2024 at 10:14:19AM -0600, Ahmed Zaki wrote: > > ... > >>>> +/** >>>> + * iavf_fdir_del_fltr - delete a flow director filter from the list >>>> + * @adapter: pointer to the VF adapter structure >>>> + * @loc: location to delete. >>>> + * >>>> + * Return: 0 on success or negative errno on failure. >>>> + */ >>>> +int iavf_fdir_del_fltr(struct iavf_adapter *adapter, u32 loc) >>>> +{ >>>> + struct iavf_fdir_fltr *fltr = NULL; >>>> + int err = 0; >>>> + >>>> + spin_lock_bh(&adapter->fdir_fltr_lock); >>>> + fltr = iavf_find_fdir_fltr(adapter, loc); >>>> + >>>> + if (fltr) { >>>> + if (fltr->state == IAVF_FDIR_FLTR_ACTIVE) { >>>> + fltr->state = IAVF_FDIR_FLTR_DEL_REQUEST; >>>> + } else if (fltr->state == IAVF_FDIR_FLTR_INACTIVE) { >>>> + list_del(&fltr->list); >>>> + kfree(fltr); >>>> + adapter->fdir_active_fltr--; >>>> + fltr = NULL; >>>> + } else { >>>> + err = -EBUSY; >>>> + } >>>> + } else if (adapter->fdir_active_fltr) { >>>> + err = -EINVAL; >>>> + } >>>> + >>>> + if (fltr && fltr->state == IAVF_FDIR_FLTR_DEL_REQUEST) >>>> + iavf_schedule_aq_request(adapter, IAVF_FLAG_AQ_DEL_FDIR_FILTER); >>> >>> It seems that prior to this change the condition and call to >>> iavf_schedule_aq_request were not protected by fdir_fltr_lock, and now they >>> are. If so, is this change intentional. >>> >> >> yes it is, fltr is member of the list that should be protected by the >> spinlock. > > Thanks, > > I would suggest moving this into a separate patch: changing locking is a > bit different to refactoring. > > Or, if not, at least mentioning it in the patch description. I will mention it in the commit message. A separate patch is an overkill IMHO since the function location has changed and the patch would not be applied to previous versions. Thanks, Ahmed
diff --git a/drivers/net/ethernet/intel/iavf/iavf.h b/drivers/net/ethernet/intel/iavf/iavf.h index 23a6557fc3db..85bd6a85cf2d 100644 --- a/drivers/net/ethernet/intel/iavf/iavf.h +++ b/drivers/net/ethernet/intel/iavf/iavf.h @@ -444,6 +444,11 @@ struct iavf_adapter { spinlock_t adv_rss_lock; /* protect the RSS management list */ }; +/* Must be called with fdir_fltr_lock lock held */ +static inline bool iavf_fdir_max_reached(struct iavf_adapter *adapter) +{ + return (adapter->fdir_active_fltr >= IAVF_MAX_FDIR_FILTERS); +} /* Ethtool Private Flags */ diff --git a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c index 52273f7eab2c..7ab445eeee18 100644 --- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c +++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c @@ -927,7 +927,7 @@ iavf_get_ethtool_fdir_entry(struct iavf_adapter *adapter, spin_lock_bh(&adapter->fdir_fltr_lock); - rule = iavf_find_fdir_fltr_by_loc(adapter, fsp->location); + rule = iavf_find_fdir_fltr(adapter, fsp->location); if (!rule) { ret = -EINVAL; goto release_lock; @@ -1263,15 +1263,7 @@ static int iavf_add_fdir_ethtool(struct iavf_adapter *adapter, struct ethtool_rx return -EINVAL; spin_lock_bh(&adapter->fdir_fltr_lock); - if (adapter->fdir_active_fltr >= IAVF_MAX_FDIR_FILTERS) { - spin_unlock_bh(&adapter->fdir_fltr_lock); - dev_err(&adapter->pdev->dev, - "Unable to add Flow Director filter because VF reached the limit of max allowed filters (%u)\n", - IAVF_MAX_FDIR_FILTERS); - return -ENOSPC; - } - - if (iavf_find_fdir_fltr_by_loc(adapter, fsp->location)) { + if (iavf_find_fdir_fltr(adapter, fsp->location)) { dev_err(&adapter->pdev->dev, "Failed to add Flow Director filter, it already exists\n"); spin_unlock_bh(&adapter->fdir_fltr_lock); return -EEXIST; @@ -1291,23 +1283,10 @@ static int iavf_add_fdir_ethtool(struct iavf_adapter *adapter, struct ethtool_rx } err = iavf_add_fdir_fltr_info(adapter, fsp, fltr); - if (err) - goto ret; - - spin_lock_bh(&adapter->fdir_fltr_lock); - iavf_fdir_list_add_fltr(adapter, fltr); - adapter->fdir_active_fltr++; - - if (adapter->link_up) - fltr->state = IAVF_FDIR_FLTR_ADD_REQUEST; - else - fltr->state = IAVF_FDIR_FLTR_INACTIVE; - spin_unlock_bh(&adapter->fdir_fltr_lock); + if (!err) + err = iavf_fdir_add_fltr(adapter, fltr); - if (adapter->link_up) - iavf_schedule_aq_request(adapter, IAVF_FLAG_AQ_ADD_FDIR_FILTER); -ret: - if (err && fltr) + if (err) kfree(fltr); mutex_unlock(&adapter->crit_lock); @@ -1324,34 +1303,11 @@ static int iavf_add_fdir_ethtool(struct iavf_adapter *adapter, struct ethtool_rx static int iavf_del_fdir_ethtool(struct iavf_adapter *adapter, struct ethtool_rxnfc *cmd) { struct ethtool_rx_flow_spec *fsp = (struct ethtool_rx_flow_spec *)&cmd->fs; - struct iavf_fdir_fltr *fltr = NULL; - int err = 0; if (!(adapter->flags & IAVF_FLAG_FDIR_ENABLED)) return -EOPNOTSUPP; - spin_lock_bh(&adapter->fdir_fltr_lock); - fltr = iavf_find_fdir_fltr_by_loc(adapter, fsp->location); - if (fltr) { - if (fltr->state == IAVF_FDIR_FLTR_ACTIVE) { - fltr->state = IAVF_FDIR_FLTR_DEL_REQUEST; - } else if (fltr->state == IAVF_FDIR_FLTR_INACTIVE) { - list_del(&fltr->list); - kfree(fltr); - adapter->fdir_active_fltr--; - fltr = NULL; - } else { - err = -EBUSY; - } - } else if (adapter->fdir_active_fltr) { - err = -EINVAL; - } - spin_unlock_bh(&adapter->fdir_fltr_lock); - - if (fltr && fltr->state == IAVF_FDIR_FLTR_DEL_REQUEST) - iavf_schedule_aq_request(adapter, IAVF_FLAG_AQ_DEL_FDIR_FILTER); - - return err; + return iavf_fdir_del_fltr(adapter, fsp->location); } /** diff --git a/drivers/net/ethernet/intel/iavf/iavf_fdir.c b/drivers/net/ethernet/intel/iavf/iavf_fdir.c index 2d47b0b4640e..1e1daf71dfa0 100644 --- a/drivers/net/ethernet/intel/iavf/iavf_fdir.c +++ b/drivers/net/ethernet/intel/iavf/iavf_fdir.c @@ -815,13 +815,14 @@ bool iavf_fdir_is_dup_fltr(struct iavf_adapter *adapter, struct iavf_fdir_fltr * } /** - * iavf_find_fdir_fltr_by_loc - find filter with location + * iavf_find_fdir_fltr - find FDIR filter * @adapter: pointer to the VF adapter structure * @loc: location to find. * - * Returns pointer to Flow Director filter if found or null + * Returns: pointer to Flow Director filter if found or NULL. Lock must be held. */ -struct iavf_fdir_fltr *iavf_find_fdir_fltr_by_loc(struct iavf_adapter *adapter, u32 loc) +struct iavf_fdir_fltr *iavf_find_fdir_fltr(struct iavf_adapter *adapter, + u32 loc) { struct iavf_fdir_fltr *rule; @@ -833,14 +834,26 @@ struct iavf_fdir_fltr *iavf_find_fdir_fltr_by_loc(struct iavf_adapter *adapter, } /** - * iavf_fdir_list_add_fltr - add a new node to the flow director filter list + * iavf_fdir_add_fltr - add a new node to the flow director filter list * @adapter: pointer to the VF adapter structure * @fltr: filter node to add to structure + * + * Return: 0 on success or negative errno on failure. */ -void iavf_fdir_list_add_fltr(struct iavf_adapter *adapter, struct iavf_fdir_fltr *fltr) +int iavf_fdir_add_fltr(struct iavf_adapter *adapter, + struct iavf_fdir_fltr *fltr) { struct iavf_fdir_fltr *rule, *parent = NULL; + spin_lock_bh(&adapter->fdir_fltr_lock); + if (iavf_fdir_max_reached(adapter)) { + spin_unlock_bh(&adapter->fdir_fltr_lock); + dev_err(&adapter->pdev->dev, + "Unable to add Flow Director filter (limit (%u) reached)\n", + IAVF_MAX_FDIR_FILTERS); + return -ENOSPC; + } + list_for_each_entry(rule, &adapter->fdir_list_head, list) { if (rule->loc >= fltr->loc) break; @@ -851,4 +864,53 @@ void iavf_fdir_list_add_fltr(struct iavf_adapter *adapter, struct iavf_fdir_fltr list_add(&fltr->list, &parent->list); else list_add(&fltr->list, &adapter->fdir_list_head); + adapter->fdir_active_fltr++; + + if (adapter->link_up) + fltr->state = IAVF_FDIR_FLTR_ADD_REQUEST; + else + fltr->state = IAVF_FDIR_FLTR_INACTIVE; + spin_unlock_bh(&adapter->fdir_fltr_lock); + + if (adapter->link_up) + iavf_schedule_aq_request(adapter, IAVF_FLAG_AQ_ADD_FDIR_FILTER); + + return 0; +} + +/** + * iavf_fdir_del_fltr - delete a flow director filter from the list + * @adapter: pointer to the VF adapter structure + * @loc: location to delete. + * + * Return: 0 on success or negative errno on failure. + */ +int iavf_fdir_del_fltr(struct iavf_adapter *adapter, u32 loc) +{ + struct iavf_fdir_fltr *fltr = NULL; + int err = 0; + + spin_lock_bh(&adapter->fdir_fltr_lock); + fltr = iavf_find_fdir_fltr(adapter, loc); + + if (fltr) { + if (fltr->state == IAVF_FDIR_FLTR_ACTIVE) { + fltr->state = IAVF_FDIR_FLTR_DEL_REQUEST; + } else if (fltr->state == IAVF_FDIR_FLTR_INACTIVE) { + list_del(&fltr->list); + kfree(fltr); + adapter->fdir_active_fltr--; + fltr = NULL; + } else { + err = -EBUSY; + } + } else if (adapter->fdir_active_fltr) { + err = -EINVAL; + } + + if (fltr && fltr->state == IAVF_FDIR_FLTR_DEL_REQUEST) + iavf_schedule_aq_request(adapter, IAVF_FLAG_AQ_DEL_FDIR_FILTER); + + spin_unlock_bh(&adapter->fdir_fltr_lock); + return err; } diff --git a/drivers/net/ethernet/intel/iavf/iavf_fdir.h b/drivers/net/ethernet/intel/iavf/iavf_fdir.h index d31bd923ba8c..5c85eb25fa2a 100644 --- a/drivers/net/ethernet/intel/iavf/iavf_fdir.h +++ b/drivers/net/ethernet/intel/iavf/iavf_fdir.h @@ -128,6 +128,9 @@ int iavf_validate_fdir_fltr_masks(struct iavf_adapter *adapter, int iavf_fill_fdir_add_msg(struct iavf_adapter *adapter, struct iavf_fdir_fltr *fltr); void iavf_print_fdir_fltr(struct iavf_adapter *adapter, struct iavf_fdir_fltr *fltr); bool iavf_fdir_is_dup_fltr(struct iavf_adapter *adapter, struct iavf_fdir_fltr *fltr); -void iavf_fdir_list_add_fltr(struct iavf_adapter *adapter, struct iavf_fdir_fltr *fltr); -struct iavf_fdir_fltr *iavf_find_fdir_fltr_by_loc(struct iavf_adapter *adapter, u32 loc); +int iavf_fdir_add_fltr(struct iavf_adapter *adapter, + struct iavf_fdir_fltr *fltr); +int iavf_fdir_del_fltr(struct iavf_adapter *adapter, u32 loc); +struct iavf_fdir_fltr *iavf_find_fdir_fltr(struct iavf_adapter *adapter, + u32 loc); #endif /* _IAVF_FDIR_H_ */