[PATCH iwl-next v1 0/6] ixgbe: improve FW/SW data synchronization

[PATCH iwl-next v1 0/6] ixgbe: improve FW/SW data synchronization

[Jedrzej Jagielski]

There are some possible discrepancies between data shown by the driver
with the actual NIC/FW state.

First 4 patches address problem with link state correctness. In some
scenarios FW may not be able to notify the driver about link change.
Along with fix, refactor the whole approach to LSE (Link Status Events)
enablement. As there's no any reasonable argument for disabling LSE
during driver life cycle, make them constantly enabled.

5th and 6th patches address problem occurring when FW changes MAC address.
There's no any kind of notify/event accompanying that change. Therefore let the
driver to poll for the new address.

Jedrzej Jagielski (6):
  ixgbe: E610: init Link Status Events mask just once
  ixgbe: E610: prevent from disabling LSE
  ixgbe: E610: do not disable LSE on driver down/remove
  ixgbe: E610: re-enable LSE unconditionally
  ixgbe: E610: add MAC address runtime refresh
  ixgbe: take rtnl lock before ixgbe_reset() is called

 drivers/net/ethernet/intel/ixgbe/ixgbe.h      |   1 -
 drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c |  49 ++++-----
 drivers/net/ethernet/intel/ixgbe/ixgbe_e610.h |   4 +-
 .../net/ethernet/intel/ixgbe/ixgbe_ethtool.c  |   2 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 100 +++++++-----------
 5 files changed, 63 insertions(+), 93 deletions(-)

-- 
2.31.1

[PATCH iwl-next v1 1/6] ixgbe: E610: init Link Status Events mask just once

[Jedrzej Jagielski]

Current approach is that LSE mask is configured each time the LSE feature
itself is toggled. The set of bits corresponding to LSE trigger causes
remain the same for the whole ixgbe lifecycle. There's no support for
enabling or disabling specific bits within LSE mask.

This is redundant; there's no need to configure LSE mask over and over.
The mask persists in hardware and only needs to be set once so should
be separated from the LSE toggling logic.

Do it just once at the init phase.

Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe.h      |  1 -
 drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c | 34 ++++++++++++-------
 drivers/net/ethernet/intel/ixgbe/ixgbe_e610.h |  3 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 28 +++------------
 4 files changed, 27 insertions(+), 39 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index 30f62174acf2..17e0a9824777 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -821,7 +821,6 @@ struct ixgbe_adapter {
 	struct ixgbe_mac_addr *mac_table;
 	u8 tx_hang_count[IXGBE_MAX_TX_QUEUES];
 	struct kobject *info_kobj;
-	u16 lse_mask;
 #ifdef CONFIG_IXGBE_HWMON
 	struct hwmon_buff *ixgbe_hwmon_buff;
 #endif /* CONFIG_IXGBE_HWMON */
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
index 831cfe9a4697..d451da68fd6d 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
@@ -1536,7 +1536,7 @@ int ixgbe_aci_get_link_info(struct ixgbe_hw *hw, bool ena_lse,
  *
  * Return: the exit code of the operation.
  */
-int ixgbe_aci_set_event_mask(struct ixgbe_hw *hw, u8 port_num, u16 mask)
+static int ixgbe_aci_set_event_mask(struct ixgbe_hw *hw, u8 port_num, u16 mask)
 {
 	struct ixgbe_aci_cmd_set_event_mask *cmd;
 	struct libie_aq_desc desc;
@@ -1551,25 +1551,25 @@ int ixgbe_aci_set_event_mask(struct ixgbe_hw *hw, u8 port_num, u16 mask)
 	return ixgbe_aci_send_cmd(hw, &desc, NULL, 0);
 }
 
+static int ixgbe_aci_init_event_mask(struct ixgbe_hw *hw)
+{
+	u16 mask = ~((u16)(IXGBE_ACI_LINK_EVENT_UPDOWN |
+			   IXGBE_ACI_LINK_EVENT_MEDIA_NA |
+			   IXGBE_ACI_LINK_EVENT_MODULE_QUAL_FAIL |
+			   IXGBE_ACI_LINK_EVENT_PHY_FW_LOAD_FAIL));
+
+	return ixgbe_aci_set_event_mask(hw, (u8)hw->bus.func, mask);
+}
+
 /**
  * ixgbe_configure_lse - enable/disable link status events
  * @hw: pointer to the HW struct
  * @activate: true for enable lse, false otherwise
- * @mask: event mask to be set; a set bit means deactivation of the
- * corresponding event
- *
- * Set the event mask and then enable or disable link status events
  *
  * Return: the exit code of the operation.
  */
-int ixgbe_configure_lse(struct ixgbe_hw *hw, bool activate, u16 mask)
+int ixgbe_configure_lse(struct ixgbe_hw *hw, bool activate)
 {
-	int err;
-
-	err = ixgbe_aci_set_event_mask(hw, (u8)hw->bus.func, mask);
-	if (err)
-		return err;
-
 	/* Enabling link status events generation by fw. */
 	return ixgbe_aci_get_link_info(hw, activate, NULL);
 }
@@ -1597,6 +1597,16 @@ static int ixgbe_start_hw_e610(struct ixgbe_hw *hw)
 
 	ixgbe_start_hw_gen2(hw);
 
+	err = ixgbe_aci_init_event_mask(hw);
+	/* In case of error just log it, don't fail the whole init */
+	if (err) {
+		struct ixgbe_adapter *adapter =
+				container_of(hw, struct ixgbe_adapter, hw);
+
+		e_dev_err("Cannot configure Link Status Event mask, err = %d\n",
+			   err);
+	}
+
 	return 0;
 }
 
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.h
index 2cb76a3d30ae..ecda35a77adb 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.h
@@ -34,8 +34,7 @@ int ixgbe_update_link_info(struct ixgbe_hw *hw);
 int ixgbe_get_link_status(struct ixgbe_hw *hw, bool *link_up);
 int ixgbe_aci_get_link_info(struct ixgbe_hw *hw, bool ena_lse,
 			    struct ixgbe_link_status *link);
-int ixgbe_aci_set_event_mask(struct ixgbe_hw *hw, u8 port_num, u16 mask);
-int ixgbe_configure_lse(struct ixgbe_hw *hw, bool activate, u16 mask);
+int ixgbe_configure_lse(struct ixgbe_hw *hw, bool activate);
 int ixgbe_aci_set_port_id_led(struct ixgbe_hw *hw, bool orig_mode);
 enum ixgbe_media_type ixgbe_get_media_type_e610(struct ixgbe_hw *hw);
 int ixgbe_setup_link_e610(struct ixgbe_hw *hw, ixgbe_link_speed speed,
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 7a0783c1abc1..2c8968b9b86c 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -5969,23 +5969,14 @@ static void ixgbe_configure(struct ixgbe_adapter *adapter)
 /**
  * ixgbe_enable_link_status_events - enable link status events
  * @adapter: pointer to the adapter structure
- * @mask: event mask to be set
  *
  * Enables link status events by invoking ixgbe_configure_lse()
  *
  * Return: the exit code of the operation.
  */
-static int ixgbe_enable_link_status_events(struct ixgbe_adapter *adapter,
-					   u16 mask)
+static int ixgbe_enable_link_status_events(struct ixgbe_adapter *adapter)
 {
-	int err;
-
-	err = ixgbe_configure_lse(&adapter->hw, true, mask);
-	if (err)
-		return err;
-
-	adapter->lse_mask = mask;
-	return 0;
+	return ixgbe_configure_lse(&adapter->hw, true);
 }
 
 /**
@@ -5998,14 +5989,7 @@ static int ixgbe_enable_link_status_events(struct ixgbe_adapter *adapter,
  */
 static int ixgbe_disable_link_status_events(struct ixgbe_adapter *adapter)
 {
-	int err;
-
-	err = ixgbe_configure_lse(&adapter->hw, false, adapter->lse_mask);
-	if (err)
-		return err;
-
-	adapter->lse_mask = 0;
-	return 0;
+	return ixgbe_configure_lse(&adapter->hw, false);
 }
 
 /**
@@ -6039,10 +6023,6 @@ static int ixgbe_non_sfp_link_config(struct ixgbe_hw *hw)
 {
 	struct ixgbe_adapter *adapter = container_of(hw, struct ixgbe_adapter,
 						     hw);
-	u16 mask = ~((u16)(IXGBE_ACI_LINK_EVENT_UPDOWN |
-			   IXGBE_ACI_LINK_EVENT_MEDIA_NA |
-			   IXGBE_ACI_LINK_EVENT_MODULE_QUAL_FAIL |
-			   IXGBE_ACI_LINK_EVENT_PHY_FW_LOAD_FAIL));
 	bool autoneg, link_up = false;
 	int ret = -EIO;
 	u32 speed;
@@ -6070,7 +6050,7 @@ static int ixgbe_non_sfp_link_config(struct ixgbe_hw *hw)
 
 	if (hw->mac.ops.setup_link) {
 		if (adapter->hw.mac.type == ixgbe_mac_e610) {
-			ret = ixgbe_enable_link_status_events(adapter, mask);
+			ret = ixgbe_enable_link_status_events(adapter);
 			if (ret)
 				return ret;
 		}
-- 
2.31.1

[PATCH iwl-next v1 2/6] ixgbe: E610: prevent from disabling LSE

[Jedrzej Jagielski]

LSE is not allowed to be switched off while the ixgbe driver is running.
Some of the ixgbe_aci_get_link_info() callers called it with @ena_lse
equal to false what was causing issues.

There was a known issue when FW after being called by Redfish MGMT request
to disable an interface wasn't able to notify the driver about link status
change due to disabled LSE reception on the driver side. As a result driver
exposed incorrect link status.

There's no any requirement forcing to disable LSE either during configuring
FC, either during setting PHY params.

Force calling ixgbe_aci_get_link_info() with @ena_lse == true.

Fixes: 4600cdf9f5ac ("ixgbe: Enable link management in E610 device")
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
index d451da68fd6d..54cc0f116e88 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
@@ -1913,7 +1913,7 @@ void ixgbe_fc_autoneg_e610(struct ixgbe_hw *hw)
 	/* Get current link err.
 	 * Current FC mode will be stored in the hw context.
 	 */
-	err = ixgbe_aci_get_link_info(hw, false, NULL);
+	err = ixgbe_aci_get_link_info(hw, true, NULL);
 	if (err)
 		goto no_autoneg;
 
@@ -2219,7 +2219,7 @@ int ixgbe_setup_phy_link_e610(struct ixgbe_hw *hw)
 	u64 phy_type_low = 0, phy_type_high = 0;
 	int err;
 
-	err = ixgbe_aci_get_link_info(hw, false, NULL);
+	err = ixgbe_aci_get_link_info(hw, true, NULL);
 	if (err)
 		return err;
 
-- 
2.31.1

[PATCH iwl-next v1 3/6] ixgbe: E610: do not disable LSE on driver down/remove

[Jedrzej Jagielski]

For the first case:
LSE especially shall remain enabled when interface is down and link watchdog
cannot track status of the link. Then the only way for link update is by
handling LSE.

For the second case:
There's no value in disabling LSE when the driver's unloading is about to be
terminated.

Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 2c8968b9b86c..f33a534490b5 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -5979,19 +5979,6 @@ static int ixgbe_enable_link_status_events(struct ixgbe_adapter *adapter)
 	return ixgbe_configure_lse(&adapter->hw, true);
 }
 
-/**
- * ixgbe_disable_link_status_events - disable link status events
- * @adapter: pointer to the adapter structure
- *
- * Disables link status events by invoking ixgbe_configure_lse()
- *
- * Return: the exit code of the operation.
- */
-static int ixgbe_disable_link_status_events(struct ixgbe_adapter *adapter)
-{
-	return ixgbe_configure_lse(&adapter->hw, false);
-}
-
 /**
  * ixgbe_sfp_link_config - set up SFP+ link
  * @adapter: pointer to private adapter struct
@@ -6775,8 +6762,6 @@ void ixgbe_down(struct ixgbe_adapter *adapter)
 
 	ixgbe_clean_all_tx_rings(adapter);
 	ixgbe_clean_all_rx_rings(adapter);
-	if (adapter->hw.mac.type == ixgbe_mac_e610)
-		ixgbe_disable_link_status_events(adapter);
 }
 
 /**
@@ -12143,9 +12128,6 @@ static void ixgbe_remove(struct pci_dev *pdev)
 	set_bit(__IXGBE_REMOVING, &adapter->state);
 	cancel_work_sync(&adapter->service_task);
 
-	if (adapter->hw.mac.type == ixgbe_mac_e610)
-		ixgbe_disable_link_status_events(adapter);
-
 	if (adapter->mii_bus)
 		mdiobus_unregister(adapter->mii_bus);
 
-- 
2.31.1

Re: [PATCH iwl-next v1 3/6] ixgbe: E610: do not disable LSE on driver down/remove

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
ixgbe: E610: do not disable LSE on driver down/remove

This patch removes the calls to ixgbe_disable_link_status_events() in
ixgbe_down() and ixgbe_remove() for the E610 MAC. The intent is to keep
Link Status Events (LSE) enabled when the interface is down or unloading.

> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 882021c436d0f..83098bf522e42 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
[ ... ]
> @@ -6733,8 +6720,6 @@ void ixgbe_down(struct ixgbe_adapter *adapter)
>  
>  	ixgbe_clean_all_tx_rings(adapter);
>  	ixgbe_clean_all_rx_rings(adapter);
> -	if (adapter->hw.mac.type == ixgbe_mac_e610)
> -		ixgbe_disable_link_status_events(adapter);

[Severity: High]
Does removing this call allow unhandled asynchronous LSE events to permanently
block the ACI mailbox?

If a link event occurs while the driver is down, the firmware can queue an
event in the ACI queue. Upon the next driver load, before the service task
can clear events, commands like ixgbe_get_flash_data() are called.

In ixgbe_aci_send_cmd_execute(), if the EV bit is asserted, the synchronous
response is overwritten by the unhandled asynchronous event from HIDA_2:

        if ((hicr & IXGBE_PF_HICR_EV) && !(hicr & IXGBE_PF_HICR_C))
                for (i = 0; i < IXGBE_ACI_DESC_SIZE_IN_DWORDS; i++)
                        raw_desc[i] = IXGBE_READ_REG(hw, IXGBE_PF_HIDA_2(i));

        if (desc->opcode != cpu_to_le16(opcode) &&
            opcode != ixgbe_aci_opc_get_fw_event)
                return -EIO;

Could this result in an opcode mismatch and return -EIO for all future
commands, causing device initialization to fail?

>  }
>  
>  /**
[ ... ]
> @@ -12080,9 +12065,6 @@ static void ixgbe_remove(struct pci_dev *pdev)
>  	set_bit(__IXGBE_REMOVING, &adapter->state);
>  	cancel_work_sync(&adapter->service_task);
>  
> -	if (adapter->hw.mac.type == ixgbe_mac_e610)
> -		ixgbe_disable_link_status_events(adapter);
> -

[Severity: High]
Can the same mailbox blockage happen here during driver unload?

If LSE is not disabled on remove, a link state change (such as a cable
unplug) after unload could leave the ACI queue with pending events.

Would this cause the same -EIO failure during the subsequent ixgbe_probe()
when the driver is loaded again?

>  	if (adapter->mii_bus)
>  		mdiobus_unregister(adapter->mii_bus);
>

RE: [PATCH iwl-next v1 3/6] ixgbe: E610: do not disable LSE on driver down/remove

[Jagielski, Jedrzej]

From: Simon Horman <horms@kernel.org> 
Sent: Wednesday, July 8, 2026 7:02 PM

>This is an AI-generated review of your patch. The human sending this
>email has considered the AI review valid, or at least plausible.
>Full review at: https://sashiko.dev
>---

Hi Simon,
yeah i saw most of these on sashiko local run and i believe we should
not be worried about them.
I see that sashiko does not understand some of the FW/NIC <-> driver
communication concepts for this series

>ixgbe: E610: do not disable LSE on driver down/remove
>
>This patch removes the calls to ixgbe_disable_link_status_events() in
>ixgbe_down() and ixgbe_remove() for the E610 MAC. The intent is to keep
>Link Status Events (LSE) enabled when the interface is down or unloading.
>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index 882021c436d0f..83098bf522e42 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>[ ... ]
>> @@ -6733,8 +6720,6 @@ void ixgbe_down(struct ixgbe_adapter *adapter)
>>  
>>  	ixgbe_clean_all_tx_rings(adapter);
>>  	ixgbe_clean_all_rx_rings(adapter);
>> -	if (adapter->hw.mac.type == ixgbe_mac_e610)
>> -		ixgbe_disable_link_status_events(adapter);
>
>[Severity: High]
>Does removing this call allow unhandled asynchronous LSE events to permanently
>block the ACI mailbox?

No, removing this call does the opposite - it let driver to handle events
which without that step would not be received at all.

>
>If a link event occurs while the driver is down, the firmware can queue an
>event in the ACI queue. Upon the next driver load, before the service task
>can clear events, commands like ixgbe_get_flash_data() are called.

That's not true and actually it's opposite to the idea of async events.
Events are not queued when driver is down, that's the period when it
may required to be notified by some sort of event, like eg link state change
so simply its send then

"before the service task can clear events"
they are handled in an async way, not synchronously by servise task

>
>In ixgbe_aci_send_cmd_execute(), if the EV bit is asserted, the synchronous
>response is overwritten by the unhandled asynchronous event from HIDA_2:

It still bases on assumption that for some reason event has not been handled
That's mislesading, handling mechanism is implemented and events are handled
one by one. HIDA_2 corresponds to the cmd which is proceesed in given moment.

>
>        if ((hicr & IXGBE_PF_HICR_EV) && !(hicr & IXGBE_PF_HICR_C))
>                for (i = 0; i < IXGBE_ACI_DESC_SIZE_IN_DWORDS; i++)
>                        raw_desc[i] = IXGBE_READ_REG(hw, IXGBE_PF_HIDA_2(i));
>
>        if (desc->opcode != cpu_to_le16(opcode) &&
>            opcode != ixgbe_aci_opc_get_fw_event)
>                return -EIO;
>
>Could this result in an opcode mismatch and return -EIO for all future
>commands, causing device initialization to fail?

I really do not see such possibility

>
>>  }
>>  
>>  /**
>[ ... ]
>> @@ -12080,9 +12065,6 @@ static void ixgbe_remove(struct pci_dev *pdev)
>>  	set_bit(__IXGBE_REMOVING, &adapter->state);
>>  	cancel_work_sync(&adapter->service_task);
>>  
>> -	if (adapter->hw.mac.type == ixgbe_mac_e610)
>> -		ixgbe_disable_link_status_events(adapter);
>> -
>
>[Severity: High]
>Can the same mailbox blockage happen here during driver unload?
>
>If LSE is not disabled on remove, a link state change (such as a cable
>unplug) after unload could leave the ACI queue with pending events.
>
>Would this cause the same -EIO failure during the subsequent ixgbe_probe()
>when the driver is loaded again?

That's even more odd. Event is not something what is kept to be send
between driver reloads / FW reinit

>
>>  	if (adapter->mii_bus)
>>  		mdiobus_unregister(adapter->mii_bus);
>>

[PATCH iwl-next v1 4/6] ixgbe: E610: re-enable LSE unconditionally

[Jedrzej Jagielski]

ixgbe_aci_get_link_info() currently allows to [en/dis]able LSE by @ena_lse
param. This isn't proper approach as this function objective is completely
different than toggling LSE feature. Creating such parameter was probably
dictated by the fact the flag corresponding for LSE enablement was placed
within the ACI command used by the function.

LSE should not be switched off while the ixgbe driver is running.
Current driver behavior doesn't provide such possibility for the user.

Enable LSE by default whenever 0x0607 ACI command is sent. Such change
corresponds to the fact that FW disables LSE whenever new event is
triggered. ixgbe_aci_get_link_info() is a part of routine handling LSE
events, so it ensures that LSE remain enabled.

Remove wrappers utilizing @ena_lse param to [en/dis]able LSE as they
are no longer valid.

Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c | 25 ++++---------------
 drivers/net/ethernet/intel/ixgbe/ixgbe_e610.h |  3 +--
 .../net/ethernet/intel/ixgbe/ixgbe_ethtool.c  |  2 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 23 +----------------
 4 files changed, 8 insertions(+), 45 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
index 54cc0f116e88..ec7d8073efe6 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
@@ -1391,7 +1391,7 @@ int ixgbe_update_link_info(struct ixgbe_hw *hw)
 
 	li = &hw->link.link_info;
 
-	err = ixgbe_aci_get_link_info(hw, true, NULL);
+	err = ixgbe_aci_get_link_info(hw, NULL);
 	if (err)
 		return err;
 
@@ -1445,7 +1445,6 @@ int ixgbe_get_link_status(struct ixgbe_hw *hw, bool *link_up)
 /**
  * ixgbe_aci_get_link_info - get the link status
  * @hw: pointer to the HW struct
- * @ena_lse: enable/disable LinkStatusEvent reporting
  * @link: pointer to link status structure - optional
  *
  * Get the current Link Status using ACI command (0x607).
@@ -1454,8 +1453,7 @@ int ixgbe_get_link_status(struct ixgbe_hw *hw, bool *link_up)
  *
  * Return: the link status of the adapter.
  */
-int ixgbe_aci_get_link_info(struct ixgbe_hw *hw, bool ena_lse,
-			    struct ixgbe_link_status *link)
+int ixgbe_aci_get_link_info(struct ixgbe_hw *hw, struct ixgbe_link_status *link)
 {
 	struct ixgbe_aci_cmd_get_link_status_data link_data = {};
 	struct ixgbe_aci_cmd_get_link_status *resp;
@@ -1474,7 +1472,7 @@ int ixgbe_aci_get_link_info(struct ixgbe_hw *hw, bool ena_lse,
 	hw_fc_info = &hw->fc;
 
 	ixgbe_fill_dflt_direct_cmd_desc(&desc, ixgbe_aci_opc_get_link_status);
-	cmd_flags = (ena_lse) ? IXGBE_ACI_LSE_ENA : IXGBE_ACI_LSE_DIS;
+	cmd_flags = IXGBE_ACI_LSE_ENA;
 	resp = libie_aq_raw(&desc);
 	resp->cmd_flags = cpu_to_le16(cmd_flags);
 	resp->lport_num = hw->bus.func;
@@ -1561,19 +1559,6 @@ static int ixgbe_aci_init_event_mask(struct ixgbe_hw *hw)
 	return ixgbe_aci_set_event_mask(hw, (u8)hw->bus.func, mask);
 }
 
-/**
- * ixgbe_configure_lse - enable/disable link status events
- * @hw: pointer to the HW struct
- * @activate: true for enable lse, false otherwise
- *
- * Return: the exit code of the operation.
- */
-int ixgbe_configure_lse(struct ixgbe_hw *hw, bool activate)
-{
-	/* Enabling link status events generation by fw. */
-	return ixgbe_aci_get_link_info(hw, activate, NULL);
-}
-
 /**
  * ixgbe_start_hw_e610 - Prepare hardware for Tx/Rx
  * @hw: pointer to hardware structure
@@ -1913,7 +1898,7 @@ void ixgbe_fc_autoneg_e610(struct ixgbe_hw *hw)
 	/* Get current link err.
 	 * Current FC mode will be stored in the hw context.
 	 */
-	err = ixgbe_aci_get_link_info(hw, true, NULL);
+	err = ixgbe_aci_get_link_info(hw, NULL);
 	if (err)
 		goto no_autoneg;
 
@@ -2219,7 +2204,7 @@ int ixgbe_setup_phy_link_e610(struct ixgbe_hw *hw)
 	u64 phy_type_low = 0, phy_type_high = 0;
 	int err;
 
-	err = ixgbe_aci_get_link_info(hw, true, NULL);
+	err = ixgbe_aci_get_link_info(hw, NULL);
 	if (err)
 		return err;
 
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.h
index ecda35a77adb..4fb4ab99d458 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.h
@@ -32,9 +32,8 @@ int ixgbe_aci_set_phy_cfg(struct ixgbe_hw *hw,
 int ixgbe_aci_set_link_restart_an(struct ixgbe_hw *hw, bool ena_link);
 int ixgbe_update_link_info(struct ixgbe_hw *hw);
 int ixgbe_get_link_status(struct ixgbe_hw *hw, bool *link_up);
-int ixgbe_aci_get_link_info(struct ixgbe_hw *hw, bool ena_lse,
+int ixgbe_aci_get_link_info(struct ixgbe_hw *hw,
 			    struct ixgbe_link_status *link);
-int ixgbe_configure_lse(struct ixgbe_hw *hw, bool activate);
 int ixgbe_aci_set_port_id_led(struct ixgbe_hw *hw, bool orig_mode);
 enum ixgbe_media_type ixgbe_get_media_type_e610(struct ixgbe_hw *hw);
 int ixgbe_setup_link_e610(struct ixgbe_hw *hw, ixgbe_link_speed speed,
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
index b8e85bc91a27..5d71c1011cf4 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
@@ -3647,7 +3647,7 @@ static int ixgbe_get_eee_e610(struct net_device *netdev,
 	linkmode_zero(kedata->supported);
 	linkmode_zero(kedata->advertised);
 
-	err = ixgbe_aci_get_link_info(hw, true, &link);
+	err = ixgbe_aci_get_link_info(hw, &link);
 	if (err)
 		return err;
 
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index f33a534490b5..d1cfe913081f 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -5966,19 +5966,6 @@ static void ixgbe_configure(struct ixgbe_adapter *adapter)
 	ixgbe_configure_dfwd(adapter);
 }
 
-/**
- * ixgbe_enable_link_status_events - enable link status events
- * @adapter: pointer to the adapter structure
- *
- * Enables link status events by invoking ixgbe_configure_lse()
- *
- * Return: the exit code of the operation.
- */
-static int ixgbe_enable_link_status_events(struct ixgbe_adapter *adapter)
-{
-	return ixgbe_configure_lse(&adapter->hw, true);
-}
-
 /**
  * ixgbe_sfp_link_config - set up SFP+ link
  * @adapter: pointer to private adapter struct
@@ -6008,8 +5995,6 @@ static void ixgbe_sfp_link_config(struct ixgbe_adapter *adapter)
  **/
 static int ixgbe_non_sfp_link_config(struct ixgbe_hw *hw)
 {
-	struct ixgbe_adapter *adapter = container_of(hw, struct ixgbe_adapter,
-						     hw);
 	bool autoneg, link_up = false;
 	int ret = -EIO;
 	u32 speed;
@@ -6035,14 +6020,8 @@ static int ixgbe_non_sfp_link_config(struct ixgbe_hw *hw)
 	if (ret)
 		return ret;
 
-	if (hw->mac.ops.setup_link) {
-		if (adapter->hw.mac.type == ixgbe_mac_e610) {
-			ret = ixgbe_enable_link_status_events(adapter);
-			if (ret)
-				return ret;
-		}
+	if (hw->mac.ops.setup_link)
 		ret = hw->mac.ops.setup_link(hw, speed, link_up);
-	}
 
 	return ret;
 }
-- 
2.31.1

[PATCH iwl-next v1 5/6] ixgbe: E610: add MAC address runtime refresh

[Jedrzej Jagielski]

Whenever MGMT requests MAC addr change and FW does it, driver does not
get aligned to that change. Current - legacy approach doesn't handle
such scenario.

Poll RAR0 (Receive Address Register) each service task cycle and update
driver and netdev structs once new MAC addr is detected. It may happen
that FW updates MAC address stored in RAR0 during runtime so SW shall fetch
the new address.

Refresh addr also during reset path to ensure the address survives RAR0
clearing during init_hw().

Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 37 +++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index d1cfe913081f..ce2b1e208c0f 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -6499,6 +6499,36 @@ void ixgbe_disable_tx(struct ixgbe_adapter *adapter)
 	}
 }
 
+static void ixgbe_mac_addr_refresh(struct ixgbe_adapter *adapter)
+{
+	struct net_device *netdev = adapter->netdev;
+	struct ixgbe_hw *hw = &adapter->hw;
+	int err;
+
+	if (hw->mac.type != ixgbe_mac_e610)
+		return;
+
+	/* fetch address stored currently in RAR0 in case the addr has been
+	 * altered by FW; if so, use it as the default one
+	 */
+	err = hw->mac.ops.get_mac_addr(hw, hw->mac.addr);
+	if (err) {
+		e_dev_warn("Cannot get MAC address\n");
+		return;
+	}
+
+	if (ether_addr_equal(netdev->dev_addr, hw->mac.addr) ||
+	    !is_valid_ether_addr(hw->mac.addr))
+		return;
+
+	ASSERT_RTNL();
+
+	eth_hw_addr_set(netdev, hw->mac.addr);
+	ether_addr_copy(adapter->mac_table[0].addr, hw->mac.addr);
+
+	call_netdevice_notifiers(NETDEV_CHANGEADDR, netdev);
+}
+
 void ixgbe_reset(struct ixgbe_adapter *adapter)
 {
 	struct ixgbe_hw *hw = &adapter->hw;
@@ -6516,6 +6546,8 @@ void ixgbe_reset(struct ixgbe_adapter *adapter)
 			     IXGBE_FLAG2_SFP_NEEDS_RESET);
 	adapter->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG;
 
+	ixgbe_mac_addr_refresh(adapter);
+
 	err = hw->mac.ops.init_hw(hw);
 	switch (err) {
 	case 0:
@@ -8701,6 +8733,11 @@ static void ixgbe_service_task(struct work_struct *work)
 			ixgbe_handle_fw_event(adapter);
 		ixgbe_check_media_subtask(adapter);
 	}
+
+	rtnl_lock();
+	ixgbe_mac_addr_refresh(adapter);
+	rtnl_unlock();
+
 	ixgbe_reset_subtask(adapter);
 	ixgbe_phy_interrupt_subtask(adapter);
 	ixgbe_sfp_detection_subtask(adapter);
-- 
2.31.1

RE: [Intel-wired-lan] [PATCH iwl-next v1 5/6] ixgbe: E610: add MAC address runtime refresh

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
> Of Jedrzej Jagielski
> Sent: Thursday, July 2, 2026 11:16 AM
> To: intel-wired-lan@lists.osuosl.org
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>;
> netdev@vger.kernel.org; Jagielski, Jedrzej
> <jedrzej.jagielski@intel.com>
> Subject: [Intel-wired-lan] [PATCH iwl-next v1 5/6] ixgbe: E610: add
> MAC address runtime refresh
> 
> Whenever MGMT requests MAC addr change and FW does it, driver does not
> get aligned to that change. Current - legacy approach doesn't handle
> such scenario.
> 
> Poll RAR0 (Receive Address Register) each service task cycle and
> update driver and netdev structs once new MAC addr is detected. It may
> happen that FW updates MAC address stored in RAR0 during runtime so SW
> shall fetch the new address.
> 
> Refresh addr also during reset path to ensure the address survives
> RAR0 clearing during init_hw().
> 
> Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 37
> +++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index d1cfe913081f..ce2b1e208c0f 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -6499,6 +6499,36 @@ void ixgbe_disable_tx(struct ixgbe_adapter
> *adapter)
>  	}
>  }
> 
> +static void ixgbe_mac_addr_refresh(struct ixgbe_adapter *adapter) {
> +	struct net_device *netdev = adapter->netdev;
> +	struct ixgbe_hw *hw = &adapter->hw;
> +	int err;
> +
> +	if (hw->mac.type != ixgbe_mac_e610)
> +		return;
> +
> +	/* fetch address stored currently in RAR0 in case the addr has
> been
> +	 * altered by FW; if so, use it as the default one
> +	 */
> +	err = hw->mac.ops.get_mac_addr(hw, hw->mac.addr);
> +	if (err) {
> +		e_dev_warn("Cannot get MAC address\n");
> +		return;
> +	}
> +
> +	if (ether_addr_equal(netdev->dev_addr, hw->mac.addr) ||
> +	    !is_valid_ether_addr(hw->mac.addr))
> +		return;
> +
> +	ASSERT_RTNL();
> +
> +	eth_hw_addr_set(netdev, hw->mac.addr);
> +	ether_addr_copy(adapter->mac_table[0].addr, hw->mac.addr);
> +
> +	call_netdevice_notifiers(NETDEV_CHANGEADDR, netdev); }
> +
>  void ixgbe_reset(struct ixgbe_adapter *adapter)  {
>  	struct ixgbe_hw *hw = &adapter->hw;
> @@ -6516,6 +6546,8 @@ void ixgbe_reset(struct ixgbe_adapter *adapter)
>  			     IXGBE_FLAG2_SFP_NEEDS_RESET);
>  	adapter->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG;
> 
> +	ixgbe_mac_addr_refresh(adapter);
> +
>  	err = hw->mac.ops.init_hw(hw);
>  	switch (err) {
>  	case 0:
> @@ -8701,6 +8733,11 @@ static void ixgbe_service_task(struct
> work_struct *work)
>  			ixgbe_handle_fw_event(adapter);
>  		ixgbe_check_media_subtask(adapter);
>  	}
> +
> +	rtnl_lock();
> +	ixgbe_mac_addr_refresh(adapter);
> +	rtnl_unlock();
> +
>  	ixgbe_reset_subtask(adapter);
>  	ixgbe_phy_interrupt_subtask(adapter);
>  	ixgbe_sfp_detection_subtask(adapter);
> --
> 2.31.1


Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

Re: [Intel-wired-lan] [PATCH iwl-next v1 5/6] ixgbe: E610: add MAC address runtime refresh

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
ixgbe: E610: add MAC address runtime refresh

This change polls RAR0 each service-task cycle on E610 and updates
netdev->dev_addr, hw->mac.addr and mac_table[0].addr when firmware has
altered the MAC.  A refresh is also invoked from ixgbe_reset() so the
address survives RAR0 clearing during init_hw().

> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index df698ddd08be..eb05c2f982e8 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -6464,6 +6464,36 @@ void ixgbe_disable_tx(struct ixgbe_adapter *adapter)
>  	}
>  }
>
> +static void ixgbe_mac_addr_refresh(struct ixgbe_adapter *adapter)
> +{
> +	struct net_device *netdev = adapter->netdev;
> +	struct ixgbe_hw *hw = &adapter->hw;
> +	int err;
> +
> +	if (hw->mac.type != ixgbe_mac_e610)
> +		return;
> +
> +	/* fetch address stored currently in RAR0 in case the addr has been
> +	 * altered by FW; if so, use it as the default one
> +	 */
> +	err = hw->mac.ops.get_mac_addr(hw, hw->mac.addr);
> +	if (err) {
> +		e_dev_warn("Cannot get MAC address\n");
> +		return;
> +	}

[High]
Can this overwrite hw->mac.addr with transient/garbage RAR0 contents
before validation?  ixgbe_get_mac_addr_generic() unconditionally copies
the raw IXGBE_RAH(0)/IXGBE_RAL(0) values into the buffer passed by the
caller and always returns 0.  Here the caller passes hw->mac.addr
itself as the destination:

    err = hw->mac.ops.get_mac_addr(hw, hw->mac.addr);

so any transient RAR0 value (all zeros while firmware is rewriting the
register, garbage during the PCI slot-reset/resume window, a
multicast/zero pattern in flight) silently clobbers hw->mac.addr.

The is_valid_ether_addr() check below prevents publishing to netdev,
but it does not restore the previous hw->mac.addr.

Consider reading into a local u8 buf[ETH_ALEN], validating that, and
only then copying into hw->mac.addr.

Compounding that, ixgbe_get_mac_addr_generic()'s own comment states "A
reset of the adapter must be performed prior to calling this function
in order for the MAC address to have been loaded from the EEPROM into
RAR0", but in ixgbe_reset() the refresh runs before init_hw().  Can
this ordering yield a stale/invalid RAR0 read on the reset path?

The corruption is then observable: after the refresh returns,
ixgbe_reset() proceeds through init_hw() and ixgbe_mac_set_default_filter()
memcpy()s hw->mac.addr into mac_table[0].addr and programs it via
set_rar() — a corrupted hw->mac.addr can therefore end up written to
hardware.

> +
> +	if (ether_addr_equal(netdev->dev_addr, hw->mac.addr) ||
> +	    !is_valid_ether_addr(hw->mac.addr))
> +		return;
> +
> +	ASSERT_RTNL();
> +
> +	eth_hw_addr_set(netdev, hw->mac.addr);
> +	ether_addr_copy(adapter->mac_table[0].addr, hw->mac.addr);
> +
> +	call_netdevice_notifiers(NETDEV_CHANGEADDR, netdev);
> +}

[Medium]
Does this leave hw->mac.perm_addr, netdev->perm_addr and
netdev->addr_assign_type inconsistent with the newly published
address?  Every other MAC-handling site in ixgbe (probe,
ixgbe_reset_hw_e610, and the equivalent flows on 82598/82599/X540/X550)
writes RAR0 into hw->mac.perm_addr.  After firmware alters the MAC at
runtime, perm_addr will keep reporting the pre-change value, so
consumers such as ethtool -P, bonding and netlink perm-address
attributes will report the stale address while NET_ADDR_PERM is still
advertised for what is now effectively a runtime-assigned MAC.

[Medium]
This unconditionally treats RAR0 (owned by firmware/MGMT) as
authoritative and, on the next service-task tick, silently overwrites
any MAC previously configured via "ip link set dev X address ...":

    eth_hw_addr_set(netdev, hw->mac.addr);
    ether_addr_copy(adapter->mac_table[0].addr, hw->mac.addr);
    call_netdevice_notifiers(NETDEV_CHANGEADDR, netdev);

No log line, no arbitration with an admin-set address, no opt-out.
NETDEV_CHANGEADDR is also emitted from a polling context, which means
userspace (udev, NetworkManager, bridging, bonding) can receive
spontaneous address-change events on E610 with no correlating
administrative action.  Should the commit message describe this uAPI
behavior change ("ip link" no longer produces a persistent MAC on
E610), and should there be some form of arbitration or logging when
firmware overrides an admin-configured address?

> +
>  void ixgbe_reset(struct ixgbe_adapter *adapter)
>  {
>  	struct ixgbe_hw *hw = &adapter->hw;
> @@ -6481,6 +6511,8 @@ void ixgbe_reset(struct ixgbe_adapter *adapter)
>  			     IXGBE_FLAG2_SFP_NEEDS_RESET);
>  	adapter->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG;
>
> +	ixgbe_mac_addr_refresh(adapter);
> +

[High]
Is ixgbe_reset() always called with RTNL held?  ixgbe_mac_addr_refresh()
uses ASSERT_RTNL() and then dispatches eth_hw_addr_set() and
call_netdevice_notifiers(NETDEV_CHANGEADDR, netdev), both of which
require RTNL.

The follow-up commit "ixgbe: take rtnl lock before ixgbe_reset() is
called" only converts the ixgbe_resume() path.  The PCI error-recovery
callback ixgbe_io_slot_reset() still calls ixgbe_reset() without
acquiring rtnl_lock(), and .slot_reset callbacks are not guaranteed to
run under RTNL.

On that path, if RAR0 differs from netdev->dev_addr on an E610,
ASSERT_RTNL() will WARN and the subsequent netdev address publication
and notifier-chain traversal race with concurrent RTNL-protected
users (SIOCSIFHWADDR, notifier chain (un)registration, etc).  Should
ixgbe_io_slot_reset() also take rtnl_lock() around ixgbe_reset(), or
should the refresh be moved to a caller-side location that guarantees
RTNL?

>  	err = hw->mac.ops.init_hw(hw);
>  	switch (err) {
>  	case 0:
> @@ -8651,6 +8683,11 @@ static void ixgbe_service_task(struct work_struct *work)
>  			ixgbe_handle_fw_event(adapter);
>  		ixgbe_check_media_subtask(adapter);
>  	}
> +
> +	rtnl_lock();
> +	ixgbe_mac_addr_refresh(adapter);
> +	rtnl_unlock();
> +

[Medium]
Would it make sense to hoist the E610 gating out of
ixgbe_mac_addr_refresh() and skip the rtnl_lock()/rtnl_unlock()
entirely for non-E610 devices?

The type check lives inside the helper:

    static void ixgbe_mac_addr_refresh(struct ixgbe_adapter *adapter)
    {
        ...
        if (hw->mac.type != ixgbe_mac_e610)
            return;
        ...
    }

so 82598, 82599, X540 and X550 adapters (which comprise most of the
ixgbe device family) acquire the global rtnl_mutex every service-task
cycle only to return immediately.  Something like:

    if (adapter->hw.mac.type == ixgbe_mac_e610) {
        rtnl_lock();
        ixgbe_mac_addr_refresh(adapter);
        rtnl_unlock();
    }

would avoid the rtnl_mutex contention on non-E610 hardware.

>  	ixgbe_reset_subtask(adapter);
>  	ixgbe_phy_interrupt_subtask(adapter);
>  	ixgbe_sfp_detection_subtask(adapter);

RE: [Intel-wired-lan] [PATCH iwl-next v1 5/6] ixgbe: E610: add MAC address runtime refresh

[Jagielski, Jedrzej]

From: Simon Horman <horms@kernel.org> 
Sent: Wednesday, July 8, 2026 7:02 PM

>This is an AI-generated review of your patch. The human sending this
>email has considered the AI review valid, or at least plausible.
>Full review at: https://netdev-ai.bots.linux.dev/sashiko/
>---
>ixgbe: E610: add MAC address runtime refresh
>
>This change polls RAR0 each service-task cycle on E610 and updates
>netdev->dev_addr, hw->mac.addr and mac_table[0].addr when firmware has
>altered the MAC.  A refresh is also invoked from ixgbe_reset() so the
>address survives RAR0 clearing during init_hw().
>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index df698ddd08be..eb05c2f982e8 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -6464,6 +6464,36 @@ void ixgbe_disable_tx(struct ixgbe_adapter *adapter)
>>  	}
>>  }
>>
>> +static void ixgbe_mac_addr_refresh(struct ixgbe_adapter *adapter)
>> +{
>> +	struct net_device *netdev = adapter->netdev;
>> +	struct ixgbe_hw *hw = &adapter->hw;
>> +	int err;
>> +
>> +	if (hw->mac.type != ixgbe_mac_e610)
>> +		return;
>> +
>> +	/* fetch address stored currently in RAR0 in case the addr has been
>> +	 * altered by FW; if so, use it as the default one
>> +	 */
>> +	err = hw->mac.ops.get_mac_addr(hw, hw->mac.addr);
>> +	if (err) {
>> +		e_dev_warn("Cannot get MAC address\n");
>> +		return;
>> +	}
>
>[High]
>Can this overwrite hw->mac.addr with transient/garbage RAR0 contents
>before validation?  ixgbe_get_mac_addr_generic() unconditionally copies
>the raw IXGBE_RAH(0)/IXGBE_RAL(0) values into the buffer passed by the
>caller and always returns 0.  Here the caller passes hw->mac.addr
>itself as the destination:
>
>    err = hw->mac.ops.get_mac_addr(hw, hw->mac.addr);
>
>so any transient RAR0 value (all zeros while firmware is rewriting the
>register, garbage during the PCI slot-reset/resume window, a
>multicast/zero pattern in flight) silently clobbers hw->mac.addr.
>
>The is_valid_ether_addr() check below prevents publishing to netdev,
>but it does not restore the previous hw->mac.addr.

Looking this way mac.addr ca be treated as a sort of transient buf.
It is not propagated further without addr check, what was noticed in that
report.
Then if transient value would be really fetched from the drvice register,
it is instantly overwritten in next service task run with correct addr once
register is written with it

>
>Consider reading into a local u8 buf[ETH_ALEN], validating that, and
>only then copying into hw->mac.addr.
>
>Compounding that, ixgbe_get_mac_addr_generic()'s own comment states "A
>reset of the adapter must be performed prior to calling this function
>in order for the MAC address to have been loaded from the EEPROM into
>RAR0", but in ixgbe_reset() the refresh runs before init_hw().  Can
>this ordering yield a stale/invalid RAR0 read on the reset path?

This comment is not longer valid as previous families did not support
mac addr runtime change from the mgmt side. In such case content of the
register is established.

>
>The corruption is then observable: after the refresh returns,
>ixgbe_reset() proceeds through init_hw() and ixgbe_mac_set_default_filter()
>memcpy()s hw->mac.addr into mac_table[0].addr and programs it via
>set_rar() — a corrupted hw->mac.addr can therefore end up written to
>hardware.

It was already in the HW - that's where the mac addr is taken from.

>
>> +
>> +	if (ether_addr_equal(netdev->dev_addr, hw->mac.addr) ||
>> +	    !is_valid_ether_addr(hw->mac.addr))
>> +		return;
>> +
>> +	ASSERT_RTNL();
>> +
>> +	eth_hw_addr_set(netdev, hw->mac.addr);
>> +	ether_addr_copy(adapter->mac_table[0].addr, hw->mac.addr);
>> +
>> +	call_netdevice_notifiers(NETDEV_CHANGEADDR, netdev);
>> +}
>
>[Medium]
>Does this leave hw->mac.perm_addr, netdev->perm_addr and
>netdev->addr_assign_type inconsistent with the newly published
>address?  Every other MAC-handling site in ixgbe (probe,
>ixgbe_reset_hw_e610, and the equivalent flows on 82598/82599/X540/X550)
>writes RAR0 into hw->mac.perm_addr.  After firmware alters the MAC at
>runtime, perm_addr will keep reporting the pre-change value, so
>consumers such as ethtool -P, bonding and netlink perm-address
>attributes will report the stale address while NET_ADDR_PERM is still
>advertised for what is now effectively a runtime-assigned MAC.

IMHO perm_address should not be changed as the name states; it can be
still restored if needed
Even after mac addr change using some mgmt tool, the fabric mac addr
still stays the same

>
>[Medium]
>This unconditionally treats RAR0 (owned by firmware/MGMT) as
>authoritative and, on the next service-task tick, silently overwrites
>any MAC previously configured via "ip link set dev X address ...":
>
>    eth_hw_addr_set(netdev, hw->mac.addr);
>    ether_addr_copy(adapter->mac_table[0].addr, hw->mac.addr);
>    call_netdevice_notifiers(NETDEV_CHANGEADDR, netdev);
>
>No log line, no arbitration with an admin-set address, no opt-out.
>NETDEV_CHANGEADDR is also emitted from a polling context, which means
>userspace (udev, NetworkManager, bridging, bonding) can receive
>spontaneous address-change events on E610 with no correlating
>administrative action.  Should the commit message describe this uAPI
>behavior change ("ip link" no longer produces a persistent MAC on
>E610), and should there be some form of arbitration or logging when
>firmware overrides an admin-configured address?

mac addr assigned by the mgmt is indeed authoritative
arbitration in that case is not applicable 

>
>> +
>>  void ixgbe_reset(struct ixgbe_adapter *adapter)
>>  {
>>  	struct ixgbe_hw *hw = &adapter->hw;
>> @@ -6481,6 +6511,8 @@ void ixgbe_reset(struct ixgbe_adapter *adapter)
>>  			     IXGBE_FLAG2_SFP_NEEDS_RESET);
>>  	adapter->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG;
>>
>> +	ixgbe_mac_addr_refresh(adapter);
>> +
>
>[High]
>Is ixgbe_reset() always called with RTNL held?  ixgbe_mac_addr_refresh()
>uses ASSERT_RTNL() and then dispatches eth_hw_addr_set() and
>call_netdevice_notifiers(NETDEV_CHANGEADDR, netdev), both of which
>require RTNL.
>
>The follow-up commit "ixgbe: take rtnl lock before ixgbe_reset() is
>called" only converts the ixgbe_resume() path.  The PCI error-recovery
>callback ixgbe_io_slot_reset() still calls ixgbe_reset() without
>acquiring rtnl_lock(), and .slot_reset callbacks are not guaranteed to
>run under RTNL.

i believe ixgbe_resume() path covers all real scenarios
ixgbe_io_slot_reset() path is something purely theoretical imho 

>
>On that path, if RAR0 differs from netdev->dev_addr on an E610,
>ASSERT_RTNL() will WARN and the subsequent netdev address publication
>and notifier-chain traversal race with concurrent RTNL-protected
>users (SIOCSIFHWADDR, notifier chain (un)registration, etc).  Should
>ixgbe_io_slot_reset() also take rtnl_lock() around ixgbe_reset(), or
>should the refresh be moved to a caller-side location that guarantees
>RTNL?

i do not see such necessity; all race potential paths are coverd
despite the one which imho is possible to happen almost only
in theoretical thinking - netdev such be detached in that path so
any potentially racing second side should not be able to operate

>
>>  	err = hw->mac.ops.init_hw(hw);
>>  	switch (err) {
>>  	case 0:
>> @@ -8651,6 +8683,11 @@ static void ixgbe_service_task(struct work_struct *work)
>>  			ixgbe_handle_fw_event(adapter);
>>  		ixgbe_check_media_subtask(adapter);
>>  	}
>> +
>> +	rtnl_lock();
>> +	ixgbe_mac_addr_refresh(adapter);
>> +	rtnl_unlock();
>> +
>
>[Medium]
>Would it make sense to hoist the E610 gating out of
>ixgbe_mac_addr_refresh() and skip the rtnl_lock()/rtnl_unlock()
>entirely for non-E610 devices?
>
>The type check lives inside the helper:
>
>    static void ixgbe_mac_addr_refresh(struct ixgbe_adapter *adapter)
>    {
>        ...
>        if (hw->mac.type != ixgbe_mac_e610)
>            return;
>        ...
>    }
>
>so 82598, 82599, X540 and X550 adapters (which comprise most of the
>ixgbe device family) acquire the global rtnl_mutex every service-task
>cycle only to return immediately.  Something like:
>
>    if (adapter->hw.mac.type == ixgbe_mac_e610) {
>        rtnl_lock();
>        ixgbe_mac_addr_refresh(adapter);
>        rtnl_unlock();
>    }

Cannot introduce rtnl locking into ixgbe_mac_addr_refresh() as this
can be already called within rtnl-locked path

>
>would avoid the rtnl_mutex contention on non-E610 hardware.
>
>>  	ixgbe_reset_subtask(adapter);
>>  	ixgbe_phy_interrupt_subtask(adapter);
>>  	ixgbe_sfp_detection_subtask(adapter);

Re: [Intel-wired-lan] [PATCH iwl-next v1 5/6] ixgbe: E610: add MAC address runtime refresh

[Maciej Fijalkowski]

On Thu, Jul 09, 2026 at 09:37:27AM +0000, Jagielski, Jedrzej wrote:
> From: Simon Horman <horms@kernel.org> 
> Sent: Wednesday, July 8, 2026 7:02 PM
> 
> >This is an AI-generated review of your patch. The human sending this
> >email has considered the AI review valid, or at least plausible.
> >Full review at: https://netdev-ai.bots.linux.dev/sashiko/
> >---
> >ixgbe: E610: add MAC address runtime refresh
> >
> >This change polls RAR0 each service-task cycle on E610 and updates
> >netdev->dev_addr, hw->mac.addr and mac_table[0].addr when firmware has
> >altered the MAC.  A refresh is also invoked from ixgbe_reset() so the
> >address survives RAR0 clearing during init_hw().

[...]

> >[Medium]
> >Would it make sense to hoist the E610 gating out of
> >ixgbe_mac_addr_refresh() and skip the rtnl_lock()/rtnl_unlock()
> >entirely for non-E610 devices?
> >
> >The type check lives inside the helper:
> >
> >    static void ixgbe_mac_addr_refresh(struct ixgbe_adapter *adapter)
> >    {
> >        ...
> >        if (hw->mac.type != ixgbe_mac_e610)
> >            return;
> >        ...
> >    }
> >
> >so 82598, 82599, X540 and X550 adapters (which comprise most of the
> >ixgbe device family) acquire the global rtnl_mutex every service-task
> >cycle only to return immediately.  Something like:
> >
> >    if (adapter->hw.mac.type == ixgbe_mac_e610) {
> >        rtnl_lock();
> >        ixgbe_mac_addr_refresh(adapter);
> >        rtnl_unlock();
> >    }
> 
> Cannot introduce rtnl locking into ixgbe_mac_addr_refresh() as this
> can be already called within rtnl-locked path

i40e does a lot of dancing for cases like this. rtnl lock is handled via
boolean provided as function arg:

static void i40e_rebuild(struct i40e_pf *pf, bool reinit, bool lock_acquired)
...
	/* Release the RTNL lock before we start resetting VFs */
	if (!lock_acquired)
		rtnl_unlock();

...

end_unlock:
	if (!lock_acquired)
		rtnl_unlock();
> 
> >
> >would avoid the rtnl_mutex contention on non-E610 hardware.
> >
> >>  	ixgbe_reset_subtask(adapter);
> >>  	ixgbe_phy_interrupt_subtask(adapter);
> >>  	ixgbe_sfp_detection_subtask(adapter);

RE: [Intel-wired-lan] [PATCH iwl-next v1 5/6] ixgbe: E610: add MAC address runtime refresh

[Jagielski, Jedrzej]

From: Fijalkowski, Maciej <maciej.fijalkowski@intel.com> 
Sent: Friday, July 10, 2026 11:58 AM

>On Thu, Jul 09, 2026 at 09:37:27AM +0000, Jagielski, Jedrzej wrote:
>> From: Simon Horman <horms@kernel.org> 
>> Sent: Wednesday, July 8, 2026 7:02 PM
>> 
>> >This is an AI-generated review of your patch. The human sending this
>> >email has considered the AI review valid, or at least plausible.
>> >Full review at: https://netdev-ai.bots.linux.dev/sashiko/
>> >---
>> >ixgbe: E610: add MAC address runtime refresh
>> >
>> >This change polls RAR0 each service-task cycle on E610 and updates
>> >netdev->dev_addr, hw->mac.addr and mac_table[0].addr when firmware has
>> >altered the MAC.  A refresh is also invoked from ixgbe_reset() so the
>> >address survives RAR0 clearing during init_hw().
>
>[...]
>
>> >[Medium]
>> >Would it make sense to hoist the E610 gating out of
>> >ixgbe_mac_addr_refresh() and skip the rtnl_lock()/rtnl_unlock()
>> >entirely for non-E610 devices?
>> >
>> >The type check lives inside the helper:
>> >
>> >    static void ixgbe_mac_addr_refresh(struct ixgbe_adapter *adapter)
>> >    {
>> >        ...
>> >        if (hw->mac.type != ixgbe_mac_e610)
>> >            return;
>> >        ...
>> >    }
>> >
>> >so 82598, 82599, X540 and X550 adapters (which comprise most of the
>> >ixgbe device family) acquire the global rtnl_mutex every service-task
>> >cycle only to return immediately.  Something like:
>> >
>> >    if (adapter->hw.mac.type == ixgbe_mac_e610) {
>> >        rtnl_lock();
>> >        ixgbe_mac_addr_refresh(adapter);
>> >        rtnl_unlock();
>> >    }
>> 
>> Cannot introduce rtnl locking into ixgbe_mac_addr_refresh() as this
>> can be already called within rtnl-locked path
>
>i40e does a lot of dancing for cases like this. rtnl lock is handled via
>boolean provided as function arg:
>
>static void i40e_rebuild(struct i40e_pf *pf, bool reinit, bool lock_acquired)
>...
>	/* Release the RTNL lock before we start resetting VFs */
>	if (!lock_acquired)
>		rtnl_unlock();
>
>...
>
>end_unlock:
>	if (!lock_acquired)
>		rtnl_unlock();

yeah, not the prettiest solution but, agree, should work :)
will redesign it, thanks!

>> 
>> >
>> >would avoid the rtnl_mutex contention on non-E610 hardware.
>> >
>> >>  	ixgbe_reset_subtask(adapter);
>> >>  	ixgbe_phy_interrupt_subtask(adapter);
>> >>  	ixgbe_sfp_detection_subtask(adapter)

[PATCH iwl-next v1 6/6] ixgbe: take rtnl lock before ixgbe_reset() is called

[Jedrzej Jagielski]

Previous commit introduced ixgbe_mac_addr_refresh which touches netdev
struct by updating mac addr. It should operate after taking rtnl lock.
One of the callers is ixgbe_reset(). Most of scenarios when ixgbe_reset()
is called met taking lock requirement, but there is a ixgbe_resume() path
which calls ixgbe_reset() -> ixgbe_mac_addr_refresh() without taking
the lock. So there is a risk of race.

Move rtnl_lock() before ixgbe_reset() is called.

Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index ce2b1e208c0f..c7261eb0e9b0 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -7574,11 +7574,11 @@ static int ixgbe_resume(struct device *dev_d)
 
 	device_wakeup_disable(dev_d);
 
+	rtnl_lock();
 	ixgbe_reset(adapter);
 
 	IXGBE_WRITE_REG(&adapter->hw, IXGBE_WUS, ~0);
 
-	rtnl_lock();
 	err = ixgbe_init_interrupt_scheme(adapter);
 	if (!err && netif_running(netdev))
 		err = ixgbe_open(netdev);
-- 
2.31.1

RE: [Intel-wired-lan] [PATCH iwl-next v1 6/6] ixgbe: take rtnl lock before ixgbe_reset() is called

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
> Of Jedrzej Jagielski
> Sent: Thursday, July 2, 2026 11:16 AM
> To: intel-wired-lan@lists.osuosl.org
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>;
> netdev@vger.kernel.org; Jagielski, Jedrzej
> <jedrzej.jagielski@intel.com>
> Subject: [Intel-wired-lan] [PATCH iwl-next v1 6/6] ixgbe: take rtnl
> lock before ixgbe_reset() is called
> 
> Previous commit introduced ixgbe_mac_addr_refresh which touches netdev
> struct by updating mac addr. It should operate after taking rtnl lock.
> One of the callers is ixgbe_reset(). Most of scenarios when
> ixgbe_reset() is called met taking lock requirement, but there is a
> ixgbe_resume() path which calls ixgbe_reset() ->
> ixgbe_mac_addr_refresh() without taking the lock. So there is a risk
> of race.
> 
> Move rtnl_lock() before ixgbe_reset() is called.
> 
> Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index ce2b1e208c0f..c7261eb0e9b0 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -7574,11 +7574,11 @@ static int ixgbe_resume(struct device *dev_d)
> 
>  	device_wakeup_disable(dev_d);
> 
> +	rtnl_lock();
>  	ixgbe_reset(adapter);
> 
>  	IXGBE_WRITE_REG(&adapter->hw, IXGBE_WUS, ~0);
> 
> -	rtnl_lock();
>  	err = ixgbe_init_interrupt_scheme(adapter);
>  	if (!err && netif_running(netdev))
>  		err = ixgbe_open(netdev);
> --
> 2.31.1

Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

Re: [PATCH iwl-next v1 6/6] ixgbe: take rtnl lock before ixgbe_reset() is called

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
ixgbe: take rtnl lock before ixgbe_reset() is called

Previous commit introduced ixgbe_mac_addr_refresh which touches netdev
struct by updating mac addr. It should operate after taking rtnl lock.
One of the callers is ixgbe_reset(). Most of scenarios when ixgbe_reset()
is called met taking lock requirement, but there is a ixgbe_resume() path
which calls ixgbe_reset() -> ixgbe_mac_addr_refresh() without taking
the lock. So there is a risk of race.

Move rtnl_lock() before ixgbe_reset() is called.

> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index eb05c2f982e8e..aa5a61f95d883 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -7535,11 +7535,11 @@ static int ixgbe_resume(struct device *dev_d)
>  
>  	device_wakeup_disable(dev_d);
>  
> +	rtnl_lock();
>  	ixgbe_reset(adapter);

[Severity: High]
This patch fixes the missing rtnl_lock() in ixgbe_resume(), but is this fix 
incomplete? 

It looks like there are still other paths that call ixgbe_reset() without 
acquiring rtnl_lock(), which would trigger the same race condition and 
ASSERT_RTNL() warning on the MAC address update. 

For example, the PCI error recovery handler calls it directly:

ixgbe_io_slot_reset() {
	...
	ixgbe_reset(adapter);
	...
}

And the service task can also call it without rtnl_lock():

ixgbe_handle_fw_event() {
	...
	ixgbe_down(adapter); /* calls ixgbe_reset() */
	...
}

[Severity: High]
This isn't a bug introduced by this patch, but does ixgbe_reset() also modify 
the unicast address list without the required lock?

ixgbe_reset() directly calls __dev_uc_unsync(netdev, NULL) which iterates 
over and modifies the dev->uc address list. However, __dev_uc_unsync() is a 
lockless helper that expects the caller to hold netif_addr_lock_bh(netdev). 

Since the callers of ixgbe_reset() only hold rtnl_lock() (or no lock), 
could this cause list corruption or use-after-free if the device resets 
concurrently with unicast address modifications?

[Severity: High]
This is a pre-existing issue, but does ixgbe_reset() use an ad-hoc 
synchronization mechanism instead of standard primitives?

Inside ixgbe_reset():

	/* lock SFP init bit to prevent race conditions with the watchdog */
	while (test_and_set_bit(__IXGBE_IN_SFP_INIT, &adapter->state))
		usleep_range(1000, 2000);

Reimplementing a lock with test_and_set_bit() and usleep_range() lacks 
deadlock detection (lockdep visibility), priority inheritance, and fairness. 

Should this use a proper mutex or spinlock instead of spinning on a bit flag?

>  
>  	IXGBE_WRITE_REG(&adapter->hw, IXGBE_WUS, ~0);
>  
> -	rtnl_lock();
>  	err = ixgbe_init_interrupt_scheme(adapter);

RE: [PATCH iwl-next v1 6/6] ixgbe: take rtnl lock before ixgbe_reset() is called

[Jagielski, Jedrzej]

From: Simon Horman <horms@kernel.org> 
Sent: Wednesday, July 8, 2026 7:03 PM

>This is an AI-generated review of your patch. The human sending this
>email has considered the AI review valid, or at least plausible.
>Full review at: https://sashiko.dev
>---
>ixgbe: take rtnl lock before ixgbe_reset() is called
>
>Previous commit introduced ixgbe_mac_addr_refresh which touches netdev
>struct by updating mac addr. It should operate after taking rtnl lock.
>One of the callers is ixgbe_reset(). Most of scenarios when ixgbe_reset()
>is called met taking lock requirement, but there is a ixgbe_resume() path
>which calls ixgbe_reset() -> ixgbe_mac_addr_refresh() without taking
>the lock. So there is a risk of race.
>
>Move rtnl_lock() before ixgbe_reset() is called.
>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index eb05c2f982e8e..aa5a61f95d883 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -7535,11 +7535,11 @@ static int ixgbe_resume(struct device *dev_d)
>>  
>>  	device_wakeup_disable(dev_d);
>>  
>> +	rtnl_lock();
>>  	ixgbe_reset(adapter);
>
>[Severity: High]
>This patch fixes the missing rtnl_lock() in ixgbe_resume(), but is this fix 
>incomplete? 

That;s the only missing path that really matters

>
>It looks like there are still other paths that call ixgbe_reset() without 
>acquiring rtnl_lock(), which would trigger the same race condition and 
>ASSERT_RTNL() warning on the MAC address update. 
>
>For example, the PCI error recovery handler calls it directly:
>
>ixgbe_io_slot_reset() {
>	...
>	ixgbe_reset(adapter);
>	...
>}
>
>And the service task can also call it without rtnl_lock():
>
>ixgbe_handle_fw_event() {
>	...
>	ixgbe_down(adapter); /* calls ixgbe_reset() */
>	...
>}

With un-attached netdev i believe there in rather no chance to race even
if mac addr refresh would be conducted during pci error recovery

>
>[Severity: High]
>This isn't a bug introduced by this patch, but does ixgbe_reset() also modify 
>the unicast address list without the required lock?
>
>ixgbe_reset() directly calls __dev_uc_unsync(netdev, NULL) which iterates 
>over and modifies the dev->uc address list. However, __dev_uc_unsync() is a 
>lockless helper that expects the caller to hold netif_addr_lock_bh(netdev). 
>
>Since the callers of ixgbe_reset() only hold rtnl_lock() (or no lock), 
>could this cause list corruption or use-after-free if the device resets 
>concurrently with unicast address modifications?
>
>[Severity: High]
>This is a pre-existing issue, but does ixgbe_reset() use an ad-hoc 
>synchronization mechanism instead of standard primitives?
>
>Inside ixgbe_reset():
>
>	/* lock SFP init bit to prevent race conditions with the watchdog */
>	while (test_and_set_bit(__IXGBE_IN_SFP_INIT, &adapter->state))
>		usleep_range(1000, 2000);
>
>Reimplementing a lock with test_and_set_bit() and usleep_range() lacks 
>deadlock detection (lockdep visibility), priority inheritance, and fairness. 
>
>Should this use a proper mutex or spinlock instead of spinning on a bit flag?
>
>>  
>>  	IXGBE_WRITE_REG(&adapter->hw, IXGBE_WUS, ~0);
>>  
>> -	rtnl_lock();
>>  	err = ixgbe_init_interrupt_scheme(adapter);


Both above notes relate to the already existing findings which i believe should
not block this series since it touches different side of the driver. Addresing
these notes would require service task refactor