On 6/5/23 21:17, Maciej Fijalkowski wrote:

On Fri, Jun 02, 2023 at 10:13:01AM -0700, Tony Nguyen wrote:

quoted

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".

quoted

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 <redacted>
Reviewed-by: Michal Swiatkowski <redacted>
Tested-by: Rafal Romanowski <redacted>
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

quoted

  			     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