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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4691e62b-0597-4184-8e85-0e74d8cdab85@intel.com>
Date: Wed, 24 Jul 2024 10:14:19 -0600
From: Ahmed Zaki <ahmed.zaki@...el.com>
To: Simon Horman <horms@...nel.org>
CC: <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>,
	<anthony.l.nguyen@...el.com>, Sridhar Samudrala
	<sridhar.samudrala@...el.com>, Marcin Szycik <marcin.szycik@...ux.intel.com>
Subject: Re: [PATCH iwl-next v3 12/13] iavf: refactor add/del FDIR filters



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@...el.com>
>> Reviewed-by: Marcin Szycik <marcin.szycik@...ux.intel.com>
>> Signed-off-by: Ahmed Zaki <ahmed.zaki@...el.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