Re: [PATCH net-next 2/3] iavf: fix err handling for MAC replace

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 6/5/23 21:17, Maciej Fijalkowski wrote:
> On Fri, Jun 02, 2023 at 10:13:01AM -0700, Tony Nguyen wrote:
>> From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>>
>> Defer removal of current primary MAC until a replacement is successfully added.
>> Previous implementation would left filter list with no primary MAC.
> 
> and this opens up for what kind of issues? do you mean that
> iavf_add_filter() could break and existing primary filter has been marked
> for removal?

Yes, prior to the patch the flow was:
1. mark all MACs non-primary;
2. mark current HW MAC for removal;
3. try to add new MAC, say it fails, so that's an end with -ENOMEM;
4. ::is_primary and ::remove fields for the ::mac_filter_list, alongside 
with ::aq_required are left modified, to be finalized next time 
user/watchdog processes that.

For me it was enough to treat it as a bug, and for sure a "bad smell".


> 
>> This was found while reading the code.
>>
>> The patch takes advantage of the fact that there can only be a single primary
>> MAC filter at any time.
>>
>> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>> Signed-off-by: Piotr Gardocki <piotrx.gardocki@intel.com>
>> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
>> Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
>> ---
>>   drivers/net/ethernet/intel/iavf/iavf_main.c | 42 ++++++++++-----------
>>   1 file changed, 19 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
>> index 420aaca548a0..3a78f86ba4f9 100644
>> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
>> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
>> @@ -1010,40 +1010,36 @@ int iavf_replace_primary_mac(struct iavf_adapter *adapter,
> 
> from what i'm looking at, iavf_replace_primary_mac() could be scoped only
> to iavf_main.c and become static func.
> 

makes sense, thanks

>>   			     const u8 *new_mac)
>>   {
>>   	struct iavf_hw *hw = &adapter->hw;
>> -	struct iavf_mac_filter *f;
>> +	struct iavf_mac_filter *new_f;
>> +	struct iavf_mac_filter *old_f;
>>   
>>   	spin_lock_bh(&adapter->mac_vlan_list_lock);
>>   
>> -	list_for_each_entry(f, &adapter->mac_filter_list, list) {
>> -		f->is_primary = false;
>> +	new_f = iavf_add_filter(adapter, new_mac);
>> +	if (!new_f) {
>> +		spin_unlock_bh(&adapter->mac_vlan_list_lock);
>> +		return -ENOMEM;
>>   	}
>>   
>> -	f = iavf_find_filter(adapter, hw->mac.addr);
>> -	if (f) {
>> -		f->remove = true;
>> +	old_f = iavf_find_filter(adapter, hw->mac.addr);
>> +	if (old_f) {
>> +		old_f->is_primary = false;
>> +		old_f->remove = true;
>>   		adapter->aq_required |= IAVF_FLAG_AQ_DEL_MAC_FILTER;
>>   	}
>> -
>> -	f = iavf_add_filter(adapter, new_mac);
>> -
>> -	if (f) {
>> -		/* Always send the request to add if changing primary MAC
>> -		 * even if filter is already present on the list
>> -		 */
>> -		f->is_primary = true;
>> -		f->add = true;
>> -		adapter->aq_required |= IAVF_FLAG_AQ_ADD_MAC_FILTER;
>> -		ether_addr_copy(hw->mac.addr, new_mac);
>> -	}
>> +	/* Always send the request to add if changing primary MAC,
>> +	 * even if filter is already present on the list
>> +	 */
>> +	new_f->is_primary = true;
>> +	new_f->add = true;
>> +	adapter->aq_required |= IAVF_FLAG_AQ_ADD_MAC_FILTER;
>> +	ether_addr_copy(hw->mac.addr, new_mac);
>>   
>>   	spin_unlock_bh(&adapter->mac_vlan_list_lock);
>>   
>>   	/* schedule the watchdog task to immediately process the request */
>> -	if (f) {
>> -		mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0);
>> -		return 0;
>> -	}
>> -	return -ENOMEM;
>> +	mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0);
>> +	return 0;
>>   }
>>   
>>   /**
>> -- 
>> 2.38.1
>>
>>