[PATCH iwl-next] ixgbe: increase SWFW semaphore timeout for X550 FW updates

[PATCH iwl-next] ixgbe: increase SWFW semaphore timeout for X550 FW updates

[Aleksandr Loktionov]

From: Soumen Karmakar <soumen.karmakar@intel.com>

According to FW documentation, the most time-consuming part of continuous
FW activity is Shadow RAM (SR) dump which takes up to 3.2 seconds.  For
X550 devices, the module-update FW command can take over 4.5 s.  Increase
the max Software/Firmware (SW/FW) semaphore wait time from the default
200 ms to 5 s for X550 to avoid spurious semaphore timeout failures
during FW update operations.

Signed-off-by: Soumen Karmakar <soumen.karmakar@intel.com>
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c
index e67e2fe..85047ef 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c
@@ -577,6 +577,9 @@ int ixgbe_acquire_swfw_sync_X540(struct ixgbe_hw *hw, u32 mask)
 
 	swmask |= swi2c_mask;
 	fwmask |= swi2c_mask << 2;
+	if (hw->mac.type == ixgbe_mac_X550)
+		timeout = 1000;
+
 	for (i = 0; i < timeout; i++) {
 		/* SW NVM semaphore bit is used for access to all
 		 * SW_FW_SYNC bits (not just NVM)
-- 
2.52.0

[PATCH iwl-next] ixgbe: add bounds check for debugfs register access

[Aleksandr Loktionov]

From: Paul Greenwalt <paul.greenwalt@intel.com>

Prevent out-of-bounds MMIO accesses triggered through user-controlled
register offsets.  IXGBE_HFDR (0x15FE8) is the highest valid MMIO
register in the ixgbe register map; any offset beyond it would address
unmapped memory.

Add a defense-in-depth check at two levels:

1. ixgbe_read_reg() -- the noinline register read accessor.  A
   WARN_ON_ONCE() guard here catches any future code path (including
   ioctl extensions) that might inadvertently pass an out-of-range
   offset without relying on higher layers to catch it first.
   ixgbe_write_reg() is a static inline called from the TX/RX hot path;
   adding WARN_ON_ONCE there would inline the check at every call site,
   so only the read path gets this guard.

2. ixgbe_dbg_reg_ops_write() -- the debugfs 'reg_ops' interface is the
   only current path where a raw, user-supplied offset enters the driver.
   Gating it before invoking the register accessors provides a clean,
   user-visible failure (silent ignore with no kernel splat) for
   deliberately malformed debugfs writes.

Add a reg <= IXGBE_HFDR guard to both the read and write paths in
ixgbe_dbg_reg_ops_write(), and a WARN_ON_ONCE + early-return guard to
ixgbe_read_reg().

Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com>
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_debugfs.c | 6 ++++--
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c    | 2 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_debugfs.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_debugfs.c
index 5b1cf49d..a6a19c0 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_debugfs.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_debugfs.c
@@ -86,7 +86,8 @@ static ssize_t ixgbe_dbg_reg_ops_write(struct file *filp,
 		u32 reg, value;
 		int cnt;
 		cnt = sscanf(&ixgbe_dbg_reg_ops_buf[5], "%x %x", &reg, &value);
-		if (cnt == 2) {
+		/* check format and bounds check register access */
+		if (cnt == 2 && reg <= IXGBE_HFDR) {
 			IXGBE_WRITE_REG(&adapter->hw, reg, value);
 			value = IXGBE_READ_REG(&adapter->hw, reg);
 			e_dev_info("write: 0x%08x = 0x%08x\n", reg, value);
@@ -97,7 +98,8 @@ static ssize_t ixgbe_dbg_reg_ops_write(struct file *filp,
 		u32 reg, value;
 		int cnt;
 		cnt = sscanf(&ixgbe_dbg_reg_ops_buf[4], "%x", &reg);
-		if (cnt == 1) {
+		/* check format and bounds check register access */
+		if (cnt == 1 && reg <= IXGBE_HFDR) {
 			value = IXGBE_READ_REG(&adapter->hw, reg);
 			e_dev_info("read 0x%08x = 0x%08x\n", reg, value);
 		} else {

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 210c7b9..4a1f3c2 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -354,4 +354,6 @@ u32 ixgbe_read_reg(struct ixgbe_hw *hw, u32 reg)
 	if (ixgbe_removed(reg_addr))
 		return IXGBE_FAILED_READ_REG;
+	if (WARN_ON_ONCE(reg > IXGBE_HFDR))
+		return IXGBE_FAILED_READ_REG;
 	if (unlikely(hw->phy.nw_mng_if_sel &
 		     IXGBE_NW_MNG_IF_SEL_SGMII_ENABLE)) {
-- 
2.52.0

Re: [PATCH iwl-next] ixgbe: add bounds check for debugfs register access

[Simon Horman]

On Fri, Mar 27, 2026 at 08:30:36AM +0100, Aleksandr Loktionov wrote:
> From: Paul Greenwalt <paul.greenwalt@intel.com>
> 
> Prevent out-of-bounds MMIO accesses triggered through user-controlled
> register offsets.  IXGBE_HFDR (0x15FE8) is the highest valid MMIO
> register in the ixgbe register map; any offset beyond it would address
> unmapped memory.
> 
> Add a defense-in-depth check at two levels:
> 
> 1. ixgbe_read_reg() -- the noinline register read accessor.  A
>    WARN_ON_ONCE() guard here catches any future code path (including
>    ioctl extensions) that might inadvertently pass an out-of-range
>    offset without relying on higher layers to catch it first.
>    ixgbe_write_reg() is a static inline called from the TX/RX hot path;
>    adding WARN_ON_ONCE there would inline the check at every call site,
>    so only the read path gets this guard.
> 
> 2. ixgbe_dbg_reg_ops_write() -- the debugfs 'reg_ops' interface is the
>    only current path where a raw, user-supplied offset enters the driver.
>    Gating it before invoking the register accessors provides a clean,
>    user-visible failure (silent ignore with no kernel splat) for
>    deliberately malformed debugfs writes.
> 
> Add a reg <= IXGBE_HFDR guard to both the read and write paths in
> ixgbe_dbg_reg_ops_write(), and a WARN_ON_ONCE + early-return guard to
> ixgbe_read_reg().
> 
> Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com>
> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

This feels like a bug fix to me, assuming users can
cause out of range access using the debugfs 'reg_ops' interface,

If so I think it should have a Fixes tag and go via iwl-net.

...

[PATCH iwl-next] ixgbe: clean up adaptive interrupt moderation algorithm

[Aleksandr Loktionov]

From: Alexander Duyck <alexander.h.duyck@intel.com>

Improve the adaptive interrupt throttle (ITR) algorithm in several ways:

 - Lower IXGBE_ITR_ADAPTIVE_MAX_USECS from 126 to 84 us (12K interrupts/s
   minimum in bulk mode) to prevent RX starvation in full-blown bulk
   scenarios.

 - Add ixgbe_container_is_rx() helper to split the Rx vs Tx logic in
   ixgbe_update_itr(); Rx uses a latency-favouring path for small bursts
   (< 24 packets and < 12112 bytes), targeting 8x throughput growth per
   step.

 - Limit the ITR decrease in latency mode to at most 2 us per update so
   ACK workloads do not overdrive the moderation and starve TCP senders.

 - Add IXGBE_ITR_ADAPTIVE_MASK_USECS (= IXGBE_ITR_ADAPTIVE_LATENCY - 1
   = 0x7F) to mask out the mode flag bit 7 in ixgbe_set_itr(), replacing
   the open-coded ~IXGBE_ITR_ADAPTIVE_LATENCY.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe.h      |  4 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 78 +++++++++++--------
 2 files changed, 48 insertions(+), 34 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index 59a1cee4..c704cc6 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -475,9 +475,10 @@ static inline unsigned int ixgbe_rx_pg_order(struct ixgbe_ring *ring)
 
 #define IXGBE_ITR_ADAPTIVE_MIN_INC	2
 #define IXGBE_ITR_ADAPTIVE_MIN_USECS	10
-#define IXGBE_ITR_ADAPTIVE_MAX_USECS	126
+#define IXGBE_ITR_ADAPTIVE_MAX_USECS	84
 #define IXGBE_ITR_ADAPTIVE_LATENCY	0x80
 #define IXGBE_ITR_ADAPTIVE_BULK		0x00
+#define IXGBE_ITR_ADAPTIVE_MASK_USECS	(IXGBE_ITR_ADAPTIVE_LATENCY - 1)
 
 struct ixgbe_ring_container {
 	struct ixgbe_ring *ring;	/* pointer to linked list of rings */
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 0bc806a..1885fe8 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -2711,6 +2711,12 @@ static void ixgbe_configure_msix(struct ixgbe_adapter *adapter)
 	IXGBE_WRITE_REG(&adapter->hw, IXGBE_EIAC, mask);
 }
 
+static inline bool ixgbe_container_is_rx(struct ixgbe_q_vector *q_vector,
+					 struct ixgbe_ring_container *rc)
+{
+	return &q_vector->rx == rc;
+}
+
 /**
  * ixgbe_update_itr - update the dynamic ITR value based on statistics
  * @q_vector: structure containing interrupt and ring information
@@ -2747,35 +2753,24 @@ static void ixgbe_update_itr(struct ixgbe_q_vector *q_vector,
 		goto clear_counts;
 
 	packets = ring_container->total_packets;
-
-	/* We have no packets to actually measure against. This means
-	 * either one of the other queues on this vector is active or
-	 * we are a Tx queue doing TSO with too high of an interrupt rate.
-	 *
-	 * When this occurs just tick up our delay by the minimum value
-	 * and hope that this extra delay will prevent us from being called
-	 * without any work on our queue.
-	 */
-	if (!packets) {
-		itr = (q_vector->itr >> 2) + IXGBE_ITR_ADAPTIVE_MIN_INC;
-		if (itr > IXGBE_ITR_ADAPTIVE_MAX_USECS)
-			itr = IXGBE_ITR_ADAPTIVE_MAX_USECS;
-		itr += ring_container->itr & IXGBE_ITR_ADAPTIVE_LATENCY;
-		goto clear_counts;
-	}
-
 	bytes = ring_container->total_bytes;
 
-	/* If packets are less than 4 or bytes are less than 9000 assume
-	 * insufficient data to use bulk rate limiting approach. We are
-	 * likely latency driven.
-	 */
-	if (packets < 4 && bytes < 9000) {
-		itr = IXGBE_ITR_ADAPTIVE_LATENCY;
-		goto adjust_by_size;
+	if (ixgbe_container_is_rx(q_vector, ring_container)) {
+		/* If Rx and there are 1 to 23 packets and bytes are less than
+		 * 12112 assume insufficient data to use bulk rate limiting
+		 * approach. Instead we will focus on simply trying to target
+		 * receiving 8 times as much data in the next interrupt.
+		 */
+		if (packets && packets < 24 && bytes < 12112) {
+			itr = IXGBE_ITR_ADAPTIVE_LATENCY;
+			avg_wire_size = (bytes + packets * 24) * 2;
+			avg_wire_size = clamp_t(unsigned int,
+						avg_wire_size, 2560, 12800);
+			goto adjust_for_speed;
+		}
 	}
 
-	/* Between 4 and 48 we can assume that our current interrupt delay
+	/* Less than 48 packets we can assume that our current interrupt delay
 	 * is only slightly too low. As such we should increase it by a small
 	 * fixed amount.
 	 */
@@ -2783,6 +2778,20 @@ static void ixgbe_update_itr(struct ixgbe_q_vector *q_vector,
 		itr = (q_vector->itr >> 2) + IXGBE_ITR_ADAPTIVE_MIN_INC;
 		if (itr > IXGBE_ITR_ADAPTIVE_MAX_USECS)
 			itr = IXGBE_ITR_ADAPTIVE_MAX_USECS;
+
+		/* If sample size is 0 - 7 we should probably switch
+		 * to latency mode instead of trying to control
+		 * things as though we are in bulk.
+		 *
+		 * Otherwise if the number of packets is less than 48
+		 * we should maintain whatever mode we are currently
+		 * in. The range between 8 and 48 is the cross-over
+		 * point between latency and bulk traffic.
+		 */
+		if (packets < 8)
+			itr += IXGBE_ITR_ADAPTIVE_LATENCY;
+		else
+			itr += ring_container->itr & IXGBE_ITR_ADAPTIVE_LATENCY;
 		goto clear_counts;
 	}
 
@@ -2813,7 +2822,6 @@ static void ixgbe_update_itr(struct ixgbe_q_vector *q_vector,
 	 */
 	itr = IXGBE_ITR_ADAPTIVE_BULK;
 
-adjust_by_size:
 	/* If packet counts are 256 or greater we can assume we have a gross
 	 * overestimation of what the rate should be. Instead of trying to fine
 	 * tune it just use the formula below to try and dial in an exact value
@@ -2856,12 +2864,7 @@ static void ixgbe_update_itr(struct ixgbe_q_vector *q_vector,
 		avg_wire_size = 32256;
 	}
 
-	/* If we are in low latency mode half our delay which doubles the rate
-	 * to somewhere between 100K to 16K ints/sec
-	 */
-	if (itr & IXGBE_ITR_ADAPTIVE_LATENCY)
-		avg_wire_size >>= 1;
-
+adjust_for_speed:
 	/* Resultant value is 256 times larger than it needs to be. This
 	 * gives us room to adjust the value as needed to either increase
 	 * or decrease the value based on link speeds of 10G, 2.5G, 1G, etc.
@@ -2888,6 +2891,15 @@ static void ixgbe_update_itr(struct ixgbe_q_vector *q_vector,
 		break;
 	}
 
+	/* In the case of a latency specific workload only allow us to
+	 * reduce the ITR by at most 2us. By doing this we should dial
+	 * in so that our number of interrupts is no more than 2x the number
+	 * of packets for the least busy workload. So for example in the case
+	 * of a TCP workload the ACK packets being received would set the
+	 * interrupt rate as they are a latency specific workload.
+	 */
+	if ((itr & IXGBE_ITR_ADAPTIVE_LATENCY) && itr < ring_container->itr)
+		itr = ring_container->itr - IXGBE_ITR_ADAPTIVE_MIN_INC;
 clear_counts:
 	/* write back value */
 	ring_container->itr = itr;
@@ -2948,7 +2960,7 @@ static void ixgbe_set_itr(struct ixgbe_q_vector *q_vector)
 	new_itr = min(q_vector->rx.itr, q_vector->tx.itr);
 
 	/* Clear latency flag if set, shift into correct position */
-	new_itr &= ~IXGBE_ITR_ADAPTIVE_LATENCY;
+	new_itr &= IXGBE_ITR_ADAPTIVE_MASK_USECS;
 	new_itr <<= 2;
 
 	if (new_itr != q_vector->itr) {
-- 
2.52.0

Re: [PATCH iwl-next] ixgbe: clean up adaptive interrupt moderation algorithm

[Simon Horman]

On Fri, Mar 27, 2026 at 08:30:37AM +0100, Aleksandr Loktionov wrote:
> From: Alexander Duyck <alexander.h.duyck@intel.com>
> 
> Improve the adaptive interrupt throttle (ITR) algorithm in several ways:
> 
>  - Lower IXGBE_ITR_ADAPTIVE_MAX_USECS from 126 to 84 us (12K interrupts/s
>    minimum in bulk mode) to prevent RX starvation in full-blown bulk
>    scenarios.
> 
>  - Add ixgbe_container_is_rx() helper to split the Rx vs Tx logic in
>    ixgbe_update_itr(); Rx uses a latency-favouring path for small bursts
>    (< 24 packets and < 12112 bytes), targeting 8x throughput growth per
>    step.
> 
>  - Limit the ITR decrease in latency mode to at most 2 us per update so
>    ACK workloads do not overdrive the moderation and starve TCP senders.
> 
>  - Add IXGBE_ITR_ADAPTIVE_MASK_USECS (= IXGBE_ITR_ADAPTIVE_LATENCY - 1
>    = 0x7F) to mask out the mode flag bit 7 in ixgbe_set_itr(), replacing
>    the open-coded ~IXGBE_ITR_ADAPTIVE_LATENCY.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

This patch is doing 4 things.
Please split it up.

...

[PATCH iwl-next] ixgbe: remove ixgbe_ping_all_vfs() from watchdog link-up handler

[Aleksandr Loktionov]

From: Sebastian Basierski <sebastianx.basierski@intel.com>

When multiple VFs are brought up simultaneously, a VF can receive a
CTS (Clear To Send) mailbox message instead of the expected RESET |
(N)ACK response, because the watchdog is sending a gratuitous ping
right as the VF mailbox transaction is in progress.

Remove the ixgbe_ping_all_vfs() call from ixgbe_watchdog_link_is_up().
Link-state changes are already communicated to VFs through the normal
mailbox protocol; the extra ping here is redundant and races with VF
initialization.

Signed-off-by: Sebastian Basierski <sebastianx.basierski@intel.com>
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 1885fe8..52f0cdc 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -8163,9 +8163,6 @@ static void ixgbe_watchdog_link_is_up(struct ixgbe_adapter *adapter)
 
 	/* update the default user priority for VFs */
 	ixgbe_update_default_up(adapter);
-
-	/* ping all the active vfs to let them know link has changed */
-	ixgbe_ping_all_vfs(adapter);
 }
 
 /**
-- 
2.52.0

Re: [Intel-wired-lan] [PATCH iwl-next] ixgbe: remove ixgbe_ping_all_vfs() from watchdog link-up handler

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

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
ixgbe: remove ixgbe_ping_all_vfs() from watchdog link-up handler

This patch removes the ixgbe_ping_all_vfs() call from
ixgbe_watchdog_link_is_up() to fix a race condition where VFs receive
CTS (Clear To Send) mailbox messages instead of expected RESET|ACK
responses during initialization.

> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 0bc806aaed90..6996547a8084 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -8151,9 +8151,6 @@ static void ixgbe_watchdog_link_is_up(struct ixgbe_adapter *adapter)
>
>  	/* update the default user priority for VFs */
>  	ixgbe_update_default_up(adapter);
> -
> -	/* ping all the active vfs to let them know link has changed */
> -	ixgbe_ping_all_vfs(adapter);
>  }

The patch removes ixgbe_ping_all_vfs() from ixgbe_watchdog_link_is_up()
to prevent the race condition during VF initialization. However, the
identical call remains in ixgbe_watchdog_link_is_down().

Doesn't the same race condition apply to link-down events?

When a VF initiates reset and the physical link state changes to down,
the watchdog would trigger:

VF side:
- Sends IXGBE_VF_RESET message
- Waits for RESET|ACK response

PF side:
- ixgbe_vf_reset_msg() processes reset, sets clear_to_send=true
- Link goes down, triggers watchdog
- ixgbe_watchdog_link_is_down() calls ixgbe_ping_all_vfs()
- Sends gratuitous ping with CTS bit

VF receives CTS message instead of expected RESET response, causing the
same initialization failure described in the commit message.

Looking at the unchanged code in ixgbe_watchdog_link_is_down():

	e_info(drv, "NIC Link is Down\n");
	netif_carrier_off(netdev);

	/* ping all the active vfs to let them know link has changed */
	ixgbe_ping_all_vfs(adapter);

The commit message states "Link-state changes are already communicated
to VFs through the normal mailbox protocol; the extra ping here is
redundant and races with VF initialization."

Does this reasoning apply equally to the link-down case?

[PATCH iwl-next] ixgbe: use ktime_get_real_ns() in ixgbe_ptp_reset()

[Aleksandr Loktionov]

From: Jacob Keller <jacob.e.keller@intel.com>

Replace ktime_to_ns(ktime_get_real()) with the direct equivalent
ktime_get_real_ns() in ixgbe_ptp_reset().  Using the combined helper
avoids the unnecessary intermediate ktime_t variable and makes the
intent clearer.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Reviewed-by: Marcin Szycik <marcin.szycik@linux.intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
index 6885d23..a7d1635 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
@@ -1347,7 +1347,7 @@ void ixgbe_ptp_reset(struct ixgbe_adapter *adapter)
 
 	spin_lock_irqsave(&adapter->tmreg_lock, flags);
 	timecounter_init(&adapter->hw_tc, &adapter->hw_cc,
-			 ktime_to_ns(ktime_get_real()));
+			 ktime_get_real_ns());
 	spin_unlock_irqrestore(&adapter->tmreg_lock, flags);
 
 	adapter->last_overflow_check = jiffies;
-- 
2.52.0

Re: [PATCH iwl-next] ixgbe: use ktime_get_real_ns() in ixgbe_ptp_reset()

[Simon Horman]

On Fri, Mar 27, 2026 at 08:30:39AM +0100, Aleksandr Loktionov wrote:
> From: Jacob Keller <jacob.e.keller@intel.com>
> 
> Replace ktime_to_ns(ktime_get_real()) with the direct equivalent
> ktime_get_real_ns() in ixgbe_ptp_reset().  Using the combined helper
> avoids the unnecessary intermediate ktime_t variable and makes the
> intent clearer.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> Reviewed-by: Marcin Szycik <marcin.szycik@linux.intel.com>

Reviewed-by: Simon Horman <horms@kernel.org>

FWIIW, this pattern also seems to exist in e1000e, ixgbe and igb (twice).

Re: [PATCH iwl-next] ixgbe: use ktime_get_real_ns() in ixgbe_ptp_reset()

[Simon Horman]

On Fri, Apr 03, 2026 at 02:10:38PM +0100, Simon Horman wrote:
> On Fri, Mar 27, 2026 at 08:30:39AM +0100, Aleksandr Loktionov wrote:
> > From: Jacob Keller <jacob.e.keller@intel.com>
> > 
> > Replace ktime_to_ns(ktime_get_real()) with the direct equivalent
> > ktime_get_real_ns() in ixgbe_ptp_reset().  Using the combined helper
> > avoids the unnecessary intermediate ktime_t variable and makes the
> > intent clearer.
> > 
> > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> > Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> > Reviewed-by: Marcin Szycik <marcin.szycik@linux.intel.com>
> 
> Reviewed-by: Simon Horman <horms@kernel.org>
> 
> FWIIW, this pattern also seems to exist in e1000e, ixgbe and igb (twice).

Of course, with this patch applied it's not present in ixgbe anymore :^)

RE: [PATCH iwl-next] ixgbe: use ktime_get_real_ns() in ixgbe_ptp_reset()

[Keller, Jacob E]

> -----Original Message-----
> From: Simon Horman <horms@kernel.org>
> Sent: Friday, April 3, 2026 6:12 AM
> To: Loktionov, Aleksandr <aleksandr.loktionov@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; netdev@vger.kernel.org; Keller, Jacob E
> <jacob.e.keller@intel.com>; Marcin Szycik <marcin.szycik@linux.intel.com>
> Subject: Re: [PATCH iwl-next] ixgbe: use ktime_get_real_ns() in
> ixgbe_ptp_reset()
> 
> On Fri, Apr 03, 2026 at 02:10:38PM +0100, Simon Horman wrote:
> > On Fri, Mar 27, 2026 at 08:30:39AM +0100, Aleksandr Loktionov wrote:
> > > From: Jacob Keller <jacob.e.keller@intel.com>
> > >
> > > Replace ktime_to_ns(ktime_get_real()) with the direct equivalent
> > > ktime_get_real_ns() in ixgbe_ptp_reset().  Using the combined helper
> > > avoids the unnecessary intermediate ktime_t variable and makes the
> > > intent clearer.
> > >
> > > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> > > Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> > > Reviewed-by: Marcin Szycik <marcin.szycik@linux.intel.com>
> >
> > Reviewed-by: Simon Horman <horms@kernel.org>
> >
> > FWIIW, this pattern also seems to exist in e1000e, ixgbe and igb (twice).
> 
> Of course, with this patch applied it's not present in ixgbe anymore :^)

Right. Given that e1000e and igb are basically on life support only, I would limit touching them unless we have relevant work to do in that area in the future.

Thanks,
Jake
 

Re: [PATCH iwl-next] ixgbe: use ktime_get_real_ns() in ixgbe_ptp_reset()

[Simon Horman]

On Fri, Apr 03, 2026 at 08:26:20PM +0000, Keller, Jacob E wrote:
> 
> 
> > -----Original Message-----
> > From: Simon Horman <horms@kernel.org>
> > Sent: Friday, April 3, 2026 6:12 AM
> > To: Loktionov, Aleksandr <aleksandr.loktionov@intel.com>
> > Cc: intel-wired-lan@lists.osuosl.org; Nguyen, Anthony L
> > <anthony.l.nguyen@intel.com>; netdev@vger.kernel.org; Keller, Jacob E
> > <jacob.e.keller@intel.com>; Marcin Szycik <marcin.szycik@linux.intel.com>
> > Subject: Re: [PATCH iwl-next] ixgbe: use ktime_get_real_ns() in
> > ixgbe_ptp_reset()
> > 
> > On Fri, Apr 03, 2026 at 02:10:38PM +0100, Simon Horman wrote:
> > > On Fri, Mar 27, 2026 at 08:30:39AM +0100, Aleksandr Loktionov wrote:
> > > > From: Jacob Keller <jacob.e.keller@intel.com>
> > > >
> > > > Replace ktime_to_ns(ktime_get_real()) with the direct equivalent
> > > > ktime_get_real_ns() in ixgbe_ptp_reset().  Using the combined helper
> > > > avoids the unnecessary intermediate ktime_t variable and makes the
> > > > intent clearer.
> > > >
> > > > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> > > > Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> > > > Reviewed-by: Marcin Szycik <marcin.szycik@linux.intel.com>
> > >
> > > Reviewed-by: Simon Horman <horms@kernel.org>
> > >
> > > FWIIW, this pattern also seems to exist in e1000e, ixgbe and igb (twice).
> > 
> > Of course, with this patch applied it's not present in ixgbe anymore :^)
> 
> Right. Given that e1000e and igb are basically on life support only, I
> would limit touching them unless we have relevant work to do in that area
> in the future.

Sure, that is fine by me.

[PATCH iwl-next] ixgbe: call ixgbe_setup_fc() before fc_enable() after NVM update

[Aleksandr Loktionov]

From: Radoslaw Tyl <radoslawx.tyl@intel.com>

During an NVM update the PHY reset clears the Technology Ability Field
(IEEE 802.3 clause 37 register 7.10) back to hardware defaults.  When
the driver subsequently calls only hw->mac.ops.fc_enable() the SRRCTL
register is recalculated from the stale autonegotiated capability bits,
which the MDD (Malicious Driver Detect) logic treats as an invalid
change and halts traffic on the PF.

Fix by calling ixgbe_setup_fc() immediately before fc_enable() in
ixgbe_watchdog_update_link() so that flow-control autoneg and the
PHY registers are re-programmed in the correct order after any reset.

Signed-off-by: Radoslaw Tyl <radoslawx.tyl@intel.com>
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 52f0cdc..db954e9 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -8041,6 +8041,12 @@ static void ixgbe_watchdog_update_link(struct ixgbe_adapter *adapter)
 		pfc_en |= !!(adapter->ixgbe_ieee_pfc->pfc_en);
 
 	if (link_up && !((adapter->flags & IXGBE_FLAG_DCB_ENABLED) && pfc_en)) {
+		if (hw->mac.ops.setup_fc) {
+			int err = hw->mac.ops.setup_fc(hw);
+
+			if (err)
+				e_warn(drv, "setup_fc failed: %d\n", err);
+		}
 		hw->mac.ops.fc_enable(hw);
 		ixgbe_set_rx_drop_en(adapter);
 	}
-- 
2.52.0

Re: [PATCH iwl-next] ixgbe: call ixgbe_setup_fc() before fc_enable() after NVM update

[Simon Horman]

On Fri, Mar 27, 2026 at 08:30:40AM +0100, Aleksandr Loktionov wrote:
> From: Radoslaw Tyl <radoslawx.tyl@intel.com>
> 
> During an NVM update the PHY reset clears the Technology Ability Field
> (IEEE 802.3 clause 37 register 7.10) back to hardware defaults.  When
> the driver subsequently calls only hw->mac.ops.fc_enable() the SRRCTL
> register is recalculated from the stale autonegotiated capability bits,
> which the MDD (Malicious Driver Detect) logic treats as an invalid
> change and halts traffic on the PF.
> 
> Fix by calling ixgbe_setup_fc() immediately before fc_enable() in
> ixgbe_watchdog_update_link() so that flow-control autoneg and the
> PHY registers are re-programmed in the correct order after any reset.
> 
> Signed-off-by: Radoslaw Tyl <radoslawx.tyl@intel.com>
> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

This feels like a fix that should have a Fixes tag and be routed
via iwl-net. And some explanation of what effect this has on the user.

Otherwise, please reword the commit message to make it clear this
is not a bug fix.

...

Re: [Intel-wired-lan] [PATCH iwl-next] ixgbe: call ixgbe_setup_fc() before fc_enable() after NVM update

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

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
ixgbe: call ixgbe_setup_fc() before fc_enable() after NVM update

During an NVM update the PHY reset clears the Technology Ability Field
(IEEE 802.3 clause 37 register 7.10) back to hardware defaults. When
the driver subsequently calls only hw->mac.ops.fc_enable() the SRRCTL
register is recalculated from the stale autonegotiated capability bits,
which the MDD (Malicious Driver Detect) logic treats as an invalid
change and halts traffic on the PF.

Fix by calling ixgbe_setup_fc() immediately before fc_enable() in
ixgbe_watchdog_update_link() so that flow-control autoneg and the
PHY registers are re-programmed in the correct order after any reset.

> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 0bc806aaed90..96e912371d9a 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -8029,6 +8029,12 @@ static void ixgbe_watchdog_update_link(struct ixgbe_adapter *adapter)
>  	pfc_en |= !!(adapter->ixgbe_ieee_pfc->pfc_en);
>
>  	if (link_up && !((adapter->flags & IXGBE_FLAG_DCB_ENABLED) && pfc_en)) {
> +		if (hw->mac.ops.setup_fc) {
> +			int err = hw->mac.ops.setup_fc(hw);
> +
> +			if (err)
> +				e_warn(drv, "setup_fc failed: %d\n", err);
> +		}

What happens if setup_fc() fails here? The patch adds this call to fix
MDD issues caused by stale PHY autoneg state after NVM updates.

However, if setup_fc() returns an error, the code only logs a warning
and continues:

	if (hw->mac.ops.setup_fc) {
		int err = hw->mac.ops.setup_fc(hw);

		if (err)
			e_warn(drv, "setup_fc failed: %d\n", err);
	}
	hw->mac.ops.fc_enable(hw);

Since fc_enable() relies on hw->fc.current_mode being properly set by
the autoneg process initiated by setup_fc(), calling it after a
setup_fc failure means it operates on the same stale state the patch
was meant to fix.

Could this leave a residual bug where MDD can still trigger and halt
traffic if setup_fc fails?

>  		hw->mac.ops.fc_enable(hw);
>  		ixgbe_set_rx_drop_en(adapter);
>  	}

[PATCH iwl-next] ixgbe: replace GFP_ATOMIC with GFP_KERNEL in ixgbe_fcoe_ddp_setup()

[Aleksandr Loktionov]

From: Sebastian Basierski <sebastianx.basierski@intel.com>

ixgbe_fcoe_ddp_setup() is always called from process context (FCoE
offload setup paths) and never from an atomic context.  Using
GFP_ATOMIC is therefore unnecessarily restrictive and wastes memory
allocator headroom that is reserved for genuine atomic callers.
Replace the dma_pool_alloc() flag with GFP_KERNEL.

Signed-off-by: Sebastian Basierski <sebastianx.basierski@intel.com>
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c
index 011fda9..7fa0971 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c
@@ -193,7 +193,7 @@ static int ixgbe_fcoe_ddp_setup(struct net_device *netdev, u16 xid,
 	}
 
 	/* alloc the udl from per cpu ddp pool */
-	ddp->udl = dma_pool_alloc(ddp_pool->pool, GFP_ATOMIC, &ddp->udp);
+	ddp->udl = dma_pool_alloc(ddp_pool->pool, GFP_KERNEL, &ddp->udp);
 	if (!ddp->udl) {
 		e_err(drv, "failed allocated ddp context\n");
 		goto out_noddp_unmap;
-- 
2.52.0

Re: [Intel-wired-lan] [PATCH iwl-next] ixgbe: replace GFP_ATOMIC with GFP_KERNEL in ixgbe_fcoe_ddp_setup()

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

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
ixgbe: replace GFP_ATOMIC with GFP_KERNEL in ixgbe_fcoe_ddp_setup()

This commit changes the memory allocation flag in ixgbe_fcoe_ddp_setup()
from GFP_ATOMIC to GFP_KERNEL based on the claim that the function is
never called from atomic context.

> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c
> index 011fda9c6193..7fa0971073e5 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c
> @@ -193,7 +193,7 @@ static int ixgbe_fcoe_ddp_setup(struct net_device *netdev, u16 xid,
>  	}
>
>  	/* alloc the udl from per cpu ddp pool */
> -	ddp->udl = dma_pool_alloc(ddp_pool->pool, GFP_ATOMIC, &ddp->udp);
> +	ddp->udl = dma_pool_alloc(ddp_pool->pool, GFP_KERNEL, &ddp->udp);
                                                    ^^^^^^^^^^
Doesn't this allocation happen in atomic context? Looking at the function,
get_cpu() is called earlier:

ixgbe_fcoe_ddp_setup() {
    ...
    ddp_pool = per_cpu_ptr(fcoe->ddp_pool, get_cpu());
    ...
    ddp->udl = dma_pool_alloc(ddp_pool->pool, GFP_KERNEL, &ddp->udp);
    ...
    put_cpu();
}

get_cpu() disables preemption, which means GFP_KERNEL allocations (which
can sleep during direct reclaim) would trigger a BUG() when attempted with
preemption disabled.

The commit message claims the function is "never from an atomic context",
but while the callers may be in process context, the function itself creates
atomic context via get_cpu().

>  	if (!ddp->udl) {
>  		e_err(drv, "failed allocated ddp context\n");
>  		goto out_noddp_unmap;

Re: [Intel-wired-lan] [PATCH iwl-next] ixgbe: replace GFP_ATOMIC with GFP_KERNEL in ixgbe_fcoe_ddp_setup()

[Kohei Enju]

On 03/27 08:30, Aleksandr Loktionov wrote:
> From: Sebastian Basierski <sebastianx.basierski@intel.com>
> 
> ixgbe_fcoe_ddp_setup() is always called from process context (FCoE
> offload setup paths) and never from an atomic context.  Using
> GFP_ATOMIC is therefore unnecessarily restrictive and wastes memory
> allocator headroom that is reserved for genuine atomic callers.
> Replace the dma_pool_alloc() flag with GFP_KERNEL.
> 
> Signed-off-by: Sebastian Basierski <sebastianx.basierski@intel.com>
> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c
> index 011fda9..7fa0971 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c
> @@ -193,7 +193,7 @@ static int ixgbe_fcoe_ddp_setup(struct net_device *netdev, u16 xid,
>  	}
>  
>  	/* alloc the udl from per cpu ddp pool */
> -	ddp->udl = dma_pool_alloc(ddp_pool->pool, GFP_ATOMIC, &ddp->udp);
> +	ddp->udl = dma_pool_alloc(ddp_pool->pool, GFP_KERNEL, &ddp->udp);

Commit fa39ab5184d6 ("scsi: fcoe: Fix I/O path allocation") had introduced
this GFP_ATOMIC flag, since some IO paths called this function with a
spin_lock held. Now, is it safe to roll back this change?

>  	if (!ddp->udl) {
>  		e_err(drv, "failed allocated ddp context\n");
>  		goto out_noddp_unmap;
> -- 
> 2.52.0
> 

[PATCH iwl-next] ixgbe: fix cls_u32 nexthdr path returning success when no entry installed

[Aleksandr Loktionov]

ixgbe_configure_clsu32() returns 0 (success) after the nexthdr loop
even when ixgbe_clsu32_build_input() fails for every candidate entry
and no jump-table slot is actually programmed.  Callers that test the
return value would then falsely believe the filter was installed.

The variable 'err' already tracks the last ixgbe_clsu32_build_input()
return value; if the loop completes with a successful break, err is 0.
If all attempts failed, err holds the last failure code.  Change the
unconditional 'return 0' to 'return err' so errors are propagated
correctly.

Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Reviewed-by: Marcin Szycik <marcin.szycik@linux.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 db954e9..8ac80ff 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -10311,7 +10311,7 @@ static int ixgbe_configure_clsu32(struct ixgbe_adapter *adapter,
 				kfree(jump);
 			}
 		}
-		return 0;
+		return err;
 	}
 
 	input = kzalloc_obj(*input);
-- 
2.52.0

Re: [PATCH iwl-next] ixgbe: fix cls_u32 nexthdr path returning success when no entry installed

[Simon Horman]

On Fri, Mar 27, 2026 at 08:30:42AM +0100, Aleksandr Loktionov wrote:
> ixgbe_configure_clsu32() returns 0 (success) after the nexthdr loop
> even when ixgbe_clsu32_build_input() fails for every candidate entry
> and no jump-table slot is actually programmed.  Callers that test the
> return value would then falsely believe the filter was installed.
> 
> The variable 'err' already tracks the last ixgbe_clsu32_build_input()
> return value; if the loop completes with a successful break, err is 0.
> If all attempts failed, err holds the last failure code.  Change the
> unconditional 'return 0' to 'return err' so errors are propagated
> correctly.
> 
> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> Reviewed-by: Marcin Szycik <marcin.szycik@linux.intel.com>

This feels like a bug fix that should be routed via iwl-net with
a Fixes tag and some description of the user-visible effect
of this bug.

Else, please clearly state in the commit message that this is not a bug.

...

[PATCH iwl-next] ixgbe: use int instead of u32 for error code variables

[Aleksandr Loktionov]

The variables used to store return values of kernel and driver functions
throughout the ixgbe driver are declared as u32 in several places.  Such
functions return negative errno values on error (e.g. -EIO, -EFAULT),
which are sign-extended negative integers.  Storing them in an unsigned
u32 silently wraps the value: -EIO (0xFFFFFFF7) stored in u32 becomes a
large positive number, so any "if (status)" truthiness check still works
by accident, but comparisons against specific negative error codes or
propagation up the call stack would produce wrong results.

In the Linux kernel, u32 is reserved for fixed-width quantities used in
hardware interfaces or protocol structures.  Using it for generic error
codes misleads reviewers into thinking the value is hardware-constrained.

Change all such local variables from u32 to int driver-wide: one in
ixgbe_main.c (ixgbe_resume), three in ixgbe_phy.c
(ixgbe_identify_phy_generic, ixgbe_tn_check_overtemp,
ixgbe_set_copper_phy_power), and six in ixgbe_x550.c
(ixgbe_check_link_t_X550em, ixgbe_get_lasi_ext_t_x550em,
ixgbe_enable_lasi_ext_t_x550em, ixgbe_handle_lasi_ext_t_x550em,
ixgbe_ext_phy_t_x550em_get_link, ixgbe_setup_internal_phy_t_x550em).

No functional change.

Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |  2 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c  |  6 +++---
 drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c | 12 ++++++------
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 210c7b9..703451d 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -7518,6 +7518,6 @@ static int ixgbe_resume(struct device *dev_d)
 	struct pci_dev *pdev = to_pci_dev(dev_d);
 	struct ixgbe_adapter *adapter = pci_get_drvdata(pdev);
 	struct net_device *netdev = adapter->netdev;
-	u32 err;
+	int err;
 
 	adapter->hw.hw_addr = adapter->io_addr;
 
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c
index a1b2c3d..b4c5d6e 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c
@@ -264,5 +264,5 @@ int ixgbe_identify_phy_generic(struct ixgbe_hw *hw)
 {
-	u32 status = -EFAULT;
+	int status = -EFAULT;
 	u32 phy_addr;
 
 	if (!hw->phy.phy_semaphore_mask) {
@@ -2812,6 +2812,6 @@ bool ixgbe_tn_check_overtemp(struct ixgbe_hw *hw)
 {
 	u16 phy_data = 0;
-	u32 status;
+	int status;
 
 	if (hw->device_id != IXGBE_DEV_ID_82599_T3_LOM)
 		return false;
@@ -2833,5 +2833,5 @@ int ixgbe_set_copper_phy_power(struct ixgbe_hw *hw, bool on)
 {
-	u32 status;
+	int status;
 	u16 reg;
 
 	/* Bail if we don't have copper phy */
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c
index c1d2e3f..d4e5f6a 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c
@@ -1912,6 +1912,6 @@ static int ixgbe_check_link_t_X550em(struct ixgbe_hw *hw,
 				     bool link_up_wait_to_complete)
 {
-	u32 status;
+	int status;
 	u16 i, autoneg_status;
 
 	if (hw->mac.ops.get_media_type(hw) != ixgbe_media_type_copper)
@@ -2331,6 +2331,6 @@ static int ixgbe_get_lasi_ext_t_x550em(struct ixgbe_hw *hw, bool *lsc,
 				       bool *is_overtemp)
 {
-	u32 status;
+	int status;
 	u16 reg;
 
 	*is_overtemp = false;
@@ -2419,6 +2419,6 @@ static int ixgbe_enable_lasi_ext_t_x550em(struct ixgbe_hw *hw)
 {
 	bool lsc, overtemp;
-	u32 status;
+	int status;
 	u16 reg;
 
 	/* Clear interrupt flags */
@@ -2512,7 +2512,7 @@ static int ixgbe_handle_lasi_ext_t_x550em(struct ixgbe_hw *hw,
 {
 	struct ixgbe_phy_info *phy = &hw->phy;
 	bool lsc;
-	u32 status;
+	int status;
 
 	status = ixgbe_get_lasi_ext_t_x550em(hw, &lsc, is_overtemp);
 	if (status)
@@ -2608,5 +2608,5 @@ static int ixgbe_ext_phy_t_x550em_get_link(struct ixgbe_hw *hw, bool *link_up)
 {
-	u32 ret;
+	int ret;
 	u16 autoneg_status;
 
 	*link_up = false;
@@ -2642,7 +2642,7 @@ static int ixgbe_setup_internal_phy_t_x550em(struct ixgbe_hw *hw)
 {
 	ixgbe_link_speed force_speed;
 	bool link_up;
-	u32 status;
+	int status;
 	u16 speed;
 
 	if (hw->mac.ops.get_media_type(hw) != ixgbe_media_type_copper)
-- 
2.52.0

Re: [PATCH iwl-next] ixgbe: use int instead of u32 for error code variables

[Simon Horman]

On Fri, Mar 27, 2026 at 08:30:43AM +0100, Aleksandr Loktionov wrote:
> The variables used to store return values of kernel and driver functions
> throughout the ixgbe driver are declared as u32 in several places.  Such
> functions return negative errno values on error (e.g. -EIO, -EFAULT),
> which are sign-extended negative integers.  Storing them in an unsigned
> u32 silently wraps the value: -EIO (0xFFFFFFF7) stored in u32 becomes a
> large positive number, so any "if (status)" truthiness check still works
> by accident, but comparisons against specific negative error codes or
> propagation up the call stack would produce wrong results.
> 
> In the Linux kernel, u32 is reserved for fixed-width quantities used in
> hardware interfaces or protocol structures.  Using it for generic error
> codes misleads reviewers into thinking the value is hardware-constrained.
> 
> Change all such local variables from u32 to int driver-wide: one in
> ixgbe_main.c (ixgbe_resume), three in ixgbe_phy.c
> (ixgbe_identify_phy_generic, ixgbe_tn_check_overtemp,
> ixgbe_set_copper_phy_power), and six in ixgbe_x550.c
> (ixgbe_check_link_t_X550em, ixgbe_get_lasi_ext_t_x550em,
> ixgbe_enable_lasi_ext_t_x550em, ixgbe_handle_lasi_ext_t_x550em,
> ixgbe_ext_phy_t_x550em_get_link, ixgbe_setup_internal_phy_t_x550em).
> 
> No functional change.
> 
> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

Nice cleanup, thanks.

Reviewed-by: Simon Horman <horms@kernel.org>

[PATCH iwl-next] ixgbe: fix integer overflow and wrong bit position in ixgbe_validate_rtr()

[Aleksandr Loktionov]

Two bugs in the same loop in ixgbe_validate_rtr():

1. When extracting 3-bit traffic class values from the IXGBE_RTRUP2TC
   register the shifted value was assigned directly to a u8, silently
   truncating any bits above bit 7.  Mask with IXGBE_RTRUP2TC_UP_MASK
   before the assignment so only the intended 3 bits are kept.

2. When clearing an out-of-bounds entry the mask was always shifted by
   the fixed constant IXGBE_RTRUP2TC_UP_SHIFT (== 3), regardless of
   which loop iteration was being processed.  This means only the entry
   at bit position 3 was ever cleared; entries at bit positions 0, 6, 9,
   ..., 21 were left unreset.  Use i * IXGBE_RTRUP2TC_UP_SHIFT to target
   the correct field for each iteration.

Also replace the hardcoded 0x7 literal with the IXGBE_RTRUP2TC_UP_MASK
constant for consistency with other parts of the driver.

Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 9aec66c..53b82a5 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -9798,11 +9798,12 @@ static void ixgbe_validate_rtr(struct ixgbe_adapter *adapter, u8 tc)
 	rsave = reg;
 
 	for (i = 0; i < MAX_TRAFFIC_CLASS; i++) {
-		u8 up2tc = reg >> (i * IXGBE_RTRUP2TC_UP_SHIFT);
+		u8 up2tc = IXGBE_RTRUP2TC_UP_MASK &
+			   (reg >> (i * IXGBE_RTRUP2TC_UP_SHIFT));
 
 		/* If up2tc is out of bounds default to zero */
 		if (up2tc > tc)
-			reg &= ~(0x7 << IXGBE_RTRUP2TC_UP_SHIFT);
+			reg &= ~(IXGBE_RTRUP2TC_UP_MASK << (i * IXGBE_RTRUP2TC_UP_SHIFT));
 	}
 
 	if (reg != rsave)
-- 
2.52.0

Re: [PATCH iwl-next] ixgbe: fix integer overflow and wrong bit position in ixgbe_validate_rtr()

[Simon Horman]

On Fri, Mar 27, 2026 at 08:30:44AM +0100, Aleksandr Loktionov wrote:
> Two bugs in the same loop in ixgbe_validate_rtr():

If these are bugs then they should have a Fixes tag.
If there is more than one bug, possibly split into two patches.
And if these are present in net, then probably they are iwl-net material.

OTOH, if they are not bugs, then please don't describe them as such.

> 
> 1. When extracting 3-bit traffic class values from the IXGBE_RTRUP2TC
>    register the shifted value was assigned directly to a u8, silently
>    truncating any bits above bit 7.  Mask with IXGBE_RTRUP2TC_UP_MASK
>    before the assignment so only the intended 3 bits are kept.
> 
> 2. When clearing an out-of-bounds entry the mask was always shifted by
>    the fixed constant IXGBE_RTRUP2TC_UP_SHIFT (== 3), regardless of
>    which loop iteration was being processed.  This means only the entry
>    at bit position 3 was ever cleared; entries at bit positions 0, 6, 9,
>    ..., 21 were left unreset.  Use i * IXGBE_RTRUP2TC_UP_SHIFT to target
>    the correct field for each iteration.

This describes the effect at the register level.
But I think it would be useful to describe the effect that
users will experience.

> 
> Also replace the hardcoded 0x7 literal with the IXGBE_RTRUP2TC_UP_MASK
> constant for consistency with other parts of the driver.
> 
> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

This does seem ripe for use of FIELD_GET and FIELD_PREP.

But if these are bug fixes, then this minimalist approach looks good to me.


> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 9aec66c..53b82a5 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -9798,11 +9798,12 @@ static void ixgbe_validate_rtr(struct ixgbe_adapter *adapter, u8 tc)
>  	rsave = reg;
>  
>  	for (i = 0; i < MAX_TRAFFIC_CLASS; i++) {
> -		u8 up2tc = reg >> (i * IXGBE_RTRUP2TC_UP_SHIFT);
> +		u8 up2tc = IXGBE_RTRUP2TC_UP_MASK &
> +			   (reg >> (i * IXGBE_RTRUP2TC_UP_SHIFT));
>  
>  		/* If up2tc is out of bounds default to zero */
>  		if (up2tc > tc)
> -			reg &= ~(0x7 << IXGBE_RTRUP2TC_UP_SHIFT);
> +			reg &= ~(IXGBE_RTRUP2TC_UP_MASK << (i * IXGBE_RTRUP2TC_UP_SHIFT));
>  	}
>  
>  	if (reg != rsave)
> -- 
> 2.52.0
> 

Re: [PATCH iwl-next] ixgbe: fix integer overflow and wrong bit position in ixgbe_validate_rtr()

[David Laight]

On Fri, 27 Mar 2026 08:30:44 +0100
Aleksandr Loktionov <aleksandr.loktionov@intel.com> wrote:

> Two bugs in the same loop in ixgbe_validate_rtr():
> 
> 1. When extracting 3-bit traffic class values from the IXGBE_RTRUP2TC
>    register the shifted value was assigned directly to a u8, silently
>    truncating any bits above bit 7.  Mask with IXGBE_RTRUP2TC_UP_MASK
>    before the assignment so only the intended 3 bits are kept.
> 
> 2. When clearing an out-of-bounds entry the mask was always shifted by
>    the fixed constant IXGBE_RTRUP2TC_UP_SHIFT (== 3), regardless of
>    which loop iteration was being processed.  This means only the entry
>    at bit position 3 was ever cleared; entries at bit positions 0, 6, 9,
>    ..., 21 were left unreset.  Use i * IXGBE_RTRUP2TC_UP_SHIFT to target
>    the correct field for each iteration.
> 
> Also replace the hardcoded 0x7 literal with the IXGBE_RTRUP2TC_UP_MASK
> constant for consistency with other parts of the driver.
> 
> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 9aec66c..53b82a5 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -9798,11 +9798,12 @@ static void ixgbe_validate_rtr(struct ixgbe_adapter *adapter, u8 tc)
>  	rsave = reg;
>  
>  	for (i = 0; i < MAX_TRAFFIC_CLASS; i++) {
> -		u8 up2tc = reg >> (i * IXGBE_RTRUP2TC_UP_SHIFT);
> +		u8 up2tc = IXGBE_RTRUP2TC_UP_MASK &
> +			   (reg >> (i * IXGBE_RTRUP2TC_UP_SHIFT));

That really ought to be the opposite way round: expr & constant.
Or add a second line to mask the value.

	David

>  
>  		/* If up2tc is out of bounds default to zero */
>  		if (up2tc > tc)
> -			reg &= ~(0x7 << IXGBE_RTRUP2TC_UP_SHIFT);
> +			reg &= ~(IXGBE_RTRUP2TC_UP_MASK << (i * IXGBE_RTRUP2TC_UP_SHIFT));
>  	}
>  
>  	if (reg != rsave)

[PATCH iwl-next] ixgbe: fix ITR value overflow in adaptive interrupt throttling

[Aleksandr Loktionov]

ixgbe_update_itr() packs a mode flag (IXGBE_ITR_ADAPTIVE_LATENCY, bit 7)
and a usecs delay (bits [6:0]) into an unsigned int, then stores it in
ring_container->itr which is u8.  Values above 0xFF wrap, corrupting both
the delay and the mode-flag on the next readback.

Separate the mode bits from the usecs sub-field; clamp only the latter to
[0, IXGBE_ITR_ADAPTIVE_LATENCY - 1] via min_t(unsigned int, ...) so
overflow cannot bleed into bit 7.  Add a WARN_ONCE() when the raw usecs
value exceeds U8_MAX so out-of-range ITR computations are visible in
dmesg during development and testing.

Fixes: b4ded8327fea ("ixgbe: Update adaptive ITR algorithm")
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 1885fe8..4d53bd63 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -2901,8 +2901,12 @@ static void ixgbe_update_itr(struct ixgbe_q_vector *q_vector,
 	if ((itr & IXGBE_ITR_ADAPTIVE_LATENCY) && itr < ring_container->itr)
 		itr = ring_container->itr - IXGBE_ITR_ADAPTIVE_MIN_INC;
 clear_counts:
-	/* write back value */
-	ring_container->itr = itr;
+	WARN_ONCE((itr & ~IXGBE_ITR_ADAPTIVE_LATENCY) > U8_MAX,
+		  "ITR value %u exceeds U8_MAX, clamping\n", itr);
+
+	ring_container->itr = (itr & IXGBE_ITR_ADAPTIVE_LATENCY) |
+		min_t(unsigned int, itr & ~IXGBE_ITR_ADAPTIVE_LATENCY,
+		      IXGBE_ITR_ADAPTIVE_LATENCY - 1);
 
 	/* next update should occur within next jiffy */
 	ring_container->next_update = next_update + 1;
-- 
2.52.0

Re: [PATCH iwl-next] ixgbe: fix ITR value overflow in adaptive interrupt throttling

[Simon Horman]

On Fri, Mar 27, 2026 at 08:30:45AM +0100, Aleksandr Loktionov wrote:
> ixgbe_update_itr() packs a mode flag (IXGBE_ITR_ADAPTIVE_LATENCY, bit 7)
> and a usecs delay (bits [6:0]) into an unsigned int, then stores it in
> ring_container->itr which is u8.  Values above 0xFF wrap, corrupting both
> the delay and the mode-flag on the next readback.
> 
> Separate the mode bits from the usecs sub-field; clamp only the latter to
> [0, IXGBE_ITR_ADAPTIVE_LATENCY - 1] via min_t(unsigned int, ...) so
> overflow cannot bleed into bit 7.  Add a WARN_ONCE() when the raw usecs
> value exceeds U8_MAX so out-of-range ITR computations are visible in
> dmesg during development and testing.
> 
> Fixes: b4ded8327fea ("ixgbe: Update adaptive ITR algorithm")
> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

Somehow - perhaps because there is no m/n notation present in the [] in the
subject - Patchwork has treated each patch in this series as it's own
patch-set.

And in the case of this patch, it didn't apply - I assume
because in order to do so an earlier patch in the series
needs to be present.

So CI didn't run on this patch :(

RE: [PATCH iwl-next] ixgbe: fix ITR value overflow in adaptive interrupt throttling

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Simon Horman <horms@kernel.org>
> Sent: Friday, April 3, 2026 3:19 PM
> To: Loktionov, Aleksandr <aleksandr.loktionov@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; netdev@vger.kernel.org
> Subject: Re: [PATCH iwl-next] ixgbe: fix ITR value overflow in
> adaptive interrupt throttling
> 
> On Fri, Mar 27, 2026 at 08:30:45AM +0100, Aleksandr Loktionov wrote:
> > ixgbe_update_itr() packs a mode flag (IXGBE_ITR_ADAPTIVE_LATENCY,
> bit
> > 7) and a usecs delay (bits [6:0]) into an unsigned int, then stores
> it
> > in ring_container->itr which is u8.  Values above 0xFF wrap,
> > corrupting both the delay and the mode-flag on the next readback.
> >
> > Separate the mode bits from the usecs sub-field; clamp only the
> latter
> > to [0, IXGBE_ITR_ADAPTIVE_LATENCY - 1] via min_t(unsigned int, ...)
> so
> > overflow cannot bleed into bit 7.  Add a WARN_ONCE() when the raw
> > usecs value exceeds U8_MAX so out-of-range ITR computations are
> > visible in dmesg during development and testing.
> >
> > Fixes: b4ded8327fea ("ixgbe: Update adaptive ITR algorithm")
> > Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> 
> Somehow - perhaps because there is no m/n notation present in the []
> in the subject - Patchwork has treated each patch in this series as
> it's own patch-set.
> 
> And in the case of this patch, it didn't apply - I assume because in
> order to do so an earlier patch in the series needs to be present.
> 
> So CI didn't run on this patch :(

Good day, Simon

I have a bunch of simple /* independent */ patches for submission every week.
Whis way you'd recommend to send them to ease /* and accelerate review */ ?

1. submit one by one
2. submit as a batch by one git command (with the same internal main ID tag internally)
3. imagine a cover latter and submit as a patch-set 

Thank you
Alex

Re: [PATCH iwl-next] ixgbe: fix ITR value overflow in adaptive interrupt throttling

[Simon Horman]

On Fri, Apr 03, 2026 at 04:12:02PM +0000, Loktionov, Aleksandr wrote:
> 
> 
> > -----Original Message-----
> > From: Simon Horman <horms@kernel.org>
> > Sent: Friday, April 3, 2026 3:19 PM
> > To: Loktionov, Aleksandr <aleksandr.loktionov@intel.com>
> > Cc: intel-wired-lan@lists.osuosl.org; Nguyen, Anthony L
> > <anthony.l.nguyen@intel.com>; netdev@vger.kernel.org
> > Subject: Re: [PATCH iwl-next] ixgbe: fix ITR value overflow in
> > adaptive interrupt throttling
> > 
> > On Fri, Mar 27, 2026 at 08:30:45AM +0100, Aleksandr Loktionov wrote:
> > > ixgbe_update_itr() packs a mode flag (IXGBE_ITR_ADAPTIVE_LATENCY,
> > bit
> > > 7) and a usecs delay (bits [6:0]) into an unsigned int, then stores
> > it
> > > in ring_container->itr which is u8.  Values above 0xFF wrap,
> > > corrupting both the delay and the mode-flag on the next readback.
> > >
> > > Separate the mode bits from the usecs sub-field; clamp only the
> > latter
> > > to [0, IXGBE_ITR_ADAPTIVE_LATENCY - 1] via min_t(unsigned int, ...)
> > so
> > > overflow cannot bleed into bit 7.  Add a WARN_ONCE() when the raw
> > > usecs value exceeds U8_MAX so out-of-range ITR computations are
> > > visible in dmesg during development and testing.
> > >
> > > Fixes: b4ded8327fea ("ixgbe: Update adaptive ITR algorithm")
> > > Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> > 
> > Somehow - perhaps because there is no m/n notation present in the []
> > in the subject - Patchwork has treated each patch in this series as
> > it's own patch-set.
> > 
> > And in the case of this patch, it didn't apply - I assume because in
> > order to do so an earlier patch in the series needs to be present.
> > 
> > So CI didn't run on this patch :(
> 
> Good day, Simon
> 
> I have a bunch of simple /* independent */ patches for submission every week.
> Whis way you'd recommend to send them to ease /* and accelerate review */ ?
> 
> 1. submit one by one
> 2. submit as a batch by one git command (with the same internal main ID tag internally)
> 3. imagine a cover latter and submit as a patch-set 

Hi Aleksandr,

I would suggest 3 and lean towards small batches.

[PATCH iwl-next] ixgbe: extend 5 s SWFW semaphore timeout to all X550EM variants

[Aleksandr Loktionov]

From: Marta Plantykow <marta.a.plantykow@intel.com>

The 5-second SWFW semaphore timeout added for X550 (ixgbe_mac_X550)
also applies to X550EM devices (e.g. X550EM_a, X550EM_x) since they
share the same FW and the same SR-dump-driven worst-case latency of
~3.2 s / module-update latency of ~4.5 s.  Change the mac-type
comparison from '== ixgbe_mac_X550' to a range check that covers
all three X550-family enum values (ixgbe_mac_X550, ixgbe_mac_X550EM_x,
ixgbe_mac_x550em_a) without inadvertently capturing later entries such
as ixgbe_mac_e610 which share the same swfw-sync function but have
not been validated against this exact timing requirement.

Signed-off-by: Marta Plantykow <marta.a.plantykow@intel.com>
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c
index 85047ef..298958d 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c
@@ -577,7 +577,8 @@ int ixgbe_acquire_swfw_sync_X540(struct ixgbe_hw *hw, u32 mask)
 
 	swmask |= swi2c_mask;
 	fwmask |= swi2c_mask << 2;
-	if (hw->mac.type == ixgbe_mac_X550)
+	if (hw->mac.type >= ixgbe_mac_X550 &&
+	    hw->mac.type <= ixgbe_mac_x550em_a)
 		timeout = 1000;
 
 	for (i = 0; i < timeout; i++) {
-- 
2.52.0

Re: [PATCH iwl-next] ixgbe: extend 5 s SWFW semaphore timeout to all X550EM variants

[Tony Nguyen]

On 3/27/2026 12:30 AM, Aleksandr Loktionov wrote:
> From: Marta Plantykow <marta.a.plantykow@intel.com>
> 
> The 5-second SWFW semaphore timeout added for X550 (ixgbe_mac_X550)
> also applies to X550EM devices (e.g. X550EM_a, X550EM_x) since they
> share the same FW and the same SR-dump-driven worst-case latency of
> ~3.2 s / module-update latency of ~4.5 s.  Change the mac-type
> comparison from '== ixgbe_mac_X550' to a range check that covers
> all three X550-family enum values (ixgbe_mac_X550, ixgbe_mac_X550EM_x,
> ixgbe_mac_x550em_a) without inadvertently capturing later entries such
> as ixgbe_mac_e610 which share the same swfw-sync function but have
> not been validated against this exact timing requirement.
> 
> Signed-off-by: Marta Plantykow <marta.a.plantykow@intel.com>
> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> ---
>   drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c
> index 85047ef..298958d 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c
> @@ -577,7 +577,8 @@ int ixgbe_acquire_swfw_sync_X540(struct ixgbe_hw *hw, u32 mask)
>   
>   	swmask |= swi2c_mask;
>   	fwmask |= swi2c_mask << 2;
> -	if (hw->mac.type == ixgbe_mac_X550)
> +	if (hw->mac.type >= ixgbe_mac_X550 &&
> +	    hw->mac.type <= ixgbe_mac_x550em_a)

Seems like this can be squashed with this patch?
https://lore.kernel.org/intel-wired-lan/20260327073046.134085-1-aleksandr.loktionov@intel.com/

Also, since the X550 range is 3 MAC types, I think it'd be clearer to 
use each MAC type rather than the range. It's only 1 more line and much 
more explicit on what qualifies.

Thanks,
Tony

>   		timeout = 1000;
>   
>   	for (i = 0; i < timeout; i++) {

Re: [PATCH iwl-next] ixgbe: increase SWFW semaphore timeout for X550 FW updates

[Simon Horman]

On Fri, Mar 27, 2026 at 08:30:35AM +0100, Aleksandr Loktionov wrote:
> From: Soumen Karmakar <soumen.karmakar@intel.com>
> 
> According to FW documentation, the most time-consuming part of continuous
> FW activity is Shadow RAM (SR) dump which takes up to 3.2 seconds.  For
> X550 devices, the module-update FW command can take over 4.5 s.  Increase
> the max Software/Firmware (SW/FW) semaphore wait time from the default
> 200 ms to 5 s for X550 to avoid spurious semaphore timeout failures

Should 200ms be 1s (200 x 5us) ?

> during FW update operations.
> 
> Signed-off-by: Soumen Karmakar <soumen.karmakar@intel.com>
> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c
> index e67e2fe..85047ef 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c
> @@ -577,6 +577,9 @@ int ixgbe_acquire_swfw_sync_X540(struct ixgbe_hw *hw, u32 mask)
>  
>  	swmask |= swi2c_mask;
>  	fwmask |= swi2c_mask << 2;
> +	if (hw->mac.type == ixgbe_mac_X550)
> +		timeout = 1000;

I think it would be nice to centralise the initialisation of timeout,
to either it's default or device-specific value, here.

And to provide a comment indicating what the values mean in terms
of maximum delay in s or ms.

> +
>  	for (i = 0; i < timeout; i++) {
>  		/* SW NVM semaphore bit is used for access to all
>  		 * SW_FW_SYNC bits (not just NVM)
> -- 
> 2.52.0
>