Re: [PATCH iwl-next v3 12/13] iavf: refactor add/del FDIR filters

[Simon Horman <horms@kernel.org>]

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;
>  }

...