On 6/6/23 12:23, Maciej Fijalkowski wrote:

On Tue, Jun 06, 2023 at 12:14:49PM +0200, Przemek Kitszel wrote:

quoted

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

quoted

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

Thanks,
Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

quoted

quoted

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

are you going to followup on this? probably there are some more low
hanging fruits out in iavf such as this one.

Sure, after some digging it looks like static-whats-possible warrants 
another patch ;) (will send soon as separate series)

quoted

quoted

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