Re: [PATCH net-next 2/3] iavf: fix err handling for MAC replace
From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Date: 2023-06-05 19:18:43
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?
quoted hunk ↗ jump to hunk
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.
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