[PATCH] ixgbe: do not busy wait in ixgbe_devlink_reload_empr_finish()

[PATCH] ixgbe: do not busy wait in ixgbe_devlink_reload_empr_finish()

[Linkui Xiao]

From: Linkui Xiao <xiaolinkui@kylinos.cn>

ixgbe_devlink_reload_empr_finish() is the .reload_up devlink operation,
so it always runs in process context with the devlink instance lock held.
Its polling loop delays with mdelay(500), i.e. it busy waits for half a
second per iteration and, because the loop bound is 20 iterations, for up
to ten seconds with preemption and interrupts to the timer subsystem
effectively blocked on that CPU. That is long enough to trip the soft
lockup detector and to stall RCU grace periods, and it keeps a CPU fully
occupied while the firmware performs the EMP reset.

Use msleep() instead, the loop does not need to be atomic and nothing in
it holds a spinlock.

While at it, rename IXGBE_DEVLINK_RELOAD_TIMEOUT_SEC to
IXGBE_DEVLINK_RELOAD_TIMEOUT_TICS: the value counts 0.5 s tics, as the
comment right above it already explains, so the _SEC suffix is
misleading.

Fixes: c9e563cae19e ("ixgbe: add support for devlink reload")
Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
---
 drivers/net/ethernet/intel/ixgbe/devlink/devlink.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c b/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
index cf8908b82f8a..7ce0a0cbf9d2 100644
--- a/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
+++ b/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
@@ -430,7 +430,7 @@ static int ixgbe_devlink_reload_empr_start(struct devlink *devlink,
 }
 
 /*Wait for 10 sec with 0.5 sec tic. EMPR takes no less than half of a sec */
-#define IXGBE_DEVLINK_RELOAD_TIMEOUT_SEC	20
+#define IXGBE_DEVLINK_RELOAD_TIMEOUT_TICS	20
 
 /**
  * ixgbe_devlink_reload_empr_finish - finishes EMP reset
@@ -460,11 +460,11 @@ static int ixgbe_devlink_reload_empr_finish(struct devlink *devlink,
 		 * may be not cleared yet, so begin the loop with the delay
 		 * in order to not check the not updated register.
 		 */
-		mdelay(500);
+		msleep(500);
 
 		fwsm = IXGBE_READ_REG(hw, IXGBE_FWSM(hw));
 
-		if (i++ >= IXGBE_DEVLINK_RELOAD_TIMEOUT_SEC)
+		if (i++ >= IXGBE_DEVLINK_RELOAD_TIMEOUT_TICS)
 			return -ETIME;
 
 	} while (!(fwsm & IXGBE_FWSM_FW_VAL_BIT));
-- 
2.25.1

Re: [PATCH] ixgbe: do not busy wait in ixgbe_devlink_reload_empr_finish()

[Przemek Kitszel]

On 9/14/26 11:26, Linkui Xiao wrote:
> From: Linkui Xiao <xiaolinkui@kylinos.cn>
> 
> ixgbe_devlink_reload_empr_finish() is the .reload_up devlink operation,
> so it always runs in process context with the devlink instance lock held.
> Its polling loop delays with mdelay(500), i.e. it busy waits for half a
> second per iteration and, because the loop bound is 20 iterations, for up
> to ten seconds with preemption and interrupts to the timer subsystem
> effectively blocked on that CPU. That is long enough to trip the soft
> lockup detector and to stall RCU grace periods, and it keeps a CPU fully
> occupied while the firmware performs the EMP reset.
> 
> Use msleep() instead, the loop does not need to be atomic and nothing in
> it holds a spinlock.
> 
> While at it, rename IXGBE_DEVLINK_RELOAD_TIMEOUT_SEC to
> IXGBE_DEVLINK_RELOAD_TIMEOUT_TICS: the value counts 0.5 s tics, as the
> comment right above it already explains, so the _SEC suffix is
> misleading.
> 
> Fixes: c9e563cae19e ("ixgbe: add support for devlink reload")
> Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>

thank you,
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>

> ---
>   drivers/net/ethernet/intel/ixgbe/devlink/devlink.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c b/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
> index cf8908b82f8a..7ce0a0cbf9d2 100644
> --- a/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
> +++ b/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
> @@ -430,7 +430,7 @@ static int ixgbe_devlink_reload_empr_start(struct devlink *devlink,
>   }
>   
>   /*Wait for 10 sec with 0.5 sec tic. EMPR takes no less than half of a sec */
> -#define IXGBE_DEVLINK_RELOAD_TIMEOUT_SEC	20
> +#define IXGBE_DEVLINK_RELOAD_TIMEOUT_TICS	20
>   
>   /**
>    * ixgbe_devlink_reload_empr_finish - finishes EMP reset
> @@ -460,11 +460,11 @@ static int ixgbe_devlink_reload_empr_finish(struct devlink *devlink,
>   		 * may be not cleared yet, so begin the loop with the delay
>   		 * in order to not check the not updated register.
>   		 */
> -		mdelay(500);
> +		msleep(500);
>   
>   		fwsm = IXGBE_READ_REG(hw, IXGBE_FWSM(hw));
>   
> -		if (i++ >= IXGBE_DEVLINK_RELOAD_TIMEOUT_SEC)
> +		if (i++ >= IXGBE_DEVLINK_RELOAD_TIMEOUT_TICS)
>   			return -ETIME;
>   
>   	} while (!(fwsm & IXGBE_FWSM_FW_VAL_BIT));

Re: [PATCH] ixgbe: do not busy wait in ixgbe_devlink_reload_empr_finish()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 (2) · Low: 1

- [Medium] The commit message justifies the mdelay()->msleep() conversion
  with technically false claims about mdelay().
- [Low] The patch bundles a purely cosmetic macro rename
  (IXGBE_DEVLINK_RELOAD_TIMEOUT_SEC ->…

Pre-existing issues:
- [Medium] In ixgbe_devlink_reload_empr_finish()
  (drivers/net/ethernet/intel/ixgbe/devlink/devlink.c:455-470) the…
- [Medium] The EMPR completion poll in ixgbe_devlink_reload_empr_finish()
  treats an all-ones FWSM read as 'firmware valid'.

--

ixgbe: do not busy wait in ixgbe_devlink_reload_empr_finish()

This converts the EMP reset completion poll in
ixgbe_devlink_reload_empr_finish() from mdelay(500) to msleep(500), and
renames IXGBE_DEVLINK_RELOAD_TIMEOUT_SEC to
IXGBE_DEVLINK_RELOAD_TIMEOUT_TICS.  The commit carries a Fixes: tag
pointing at c9e563cae19e ("ixgbe: add support for devlink reload").

A few questions on the changelog and on the polling loop below.

> ixgbe_devlink_reload_empr_finish() is the .reload_up devlink operation,
> so it always runs in process context with the devlink instance lock held.
> Its polling loop delays with mdelay(500), i.e. it busy waits for half a
> second per iteration and, because the loop bound is 20 iterations, for up
> to ten seconds with preemption and interrupts to the timer subsystem
> effectively blocked on that CPU. That is long enough to trip the soft
> lockup detector and to stall RCU grace periods, and it keeps a CPU fully
> occupied while the firmware performs the EMP reset.

[Severity: Medium]
Is this description of mdelay() accurate?  mdelay() is a plain udelay()
loop:

include/linux/delay.h:
    #define mdelay(n) (\
    	(__builtin_constant_p(n) && (n)<=MAX_UDELAY_MS) ? udelay((n)*1000) : \
    	({unsigned long __ms=(n); while (__ms--) udelay(1000);}))

There is no local_irq_disable() or preempt_disable() anywhere in that
path, so timer interrupts (including the watchdog hrtimer) keep firing
and the loop stays preemptible on CONFIG_PREEMPT kernels.  In what sense
are "preemption and interrupts to the timer subsystem effectively
blocked on that CPU"?

Are the two detectors named here actually reachable at the stated bound?
get_softlockup_thresh() in kernel/watchdog.c returns watchdog_thresh * 2,
which is 20 s by default, and RCU_CPU_STALL_TIMEOUT defaults to 21 s, both
above the ~10 s worst case described.

Since the changelog is paired with a Fixes: tag, could it be reworded to
state the rationale that does hold, namely that spinning a CPU for ~10 s
in sleepable process context is wasteful and needlessly non-preemptible
on CONFIG_PREEMPT_NONE, rather than citing soft lockup splats and RCU
stalls?

> diff --git a/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c b/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
> index cf8908b82f8a3..7ce0a0cbf9d2a 100644
> --- a/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
> +++ b/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
> @@ -430,7 +430,7 @@ static int ixgbe_devlink_reload_empr_start(struct devlink *devlink,
>  }
>  
>  /*Wait for 10 sec with 0.5 sec tic. EMPR takes no less than half of a sec */
> -#define IXGBE_DEVLINK_RELOAD_TIMEOUT_SEC	20
> +#define IXGBE_DEVLINK_RELOAD_TIMEOUT_TICS	20

[Severity: Low]
This isn't a bug, but would it be better to keep this rename out of a
Fixes-tagged commit that also carries the behavioural mdelay() to
msleep() change?

Also, "TICS" reads as a misspelling of "TICKS", and "tick" in kernel
parlance means a jiffy/timer tick rather than an arbitrary 500 ms poll
iteration.  What the constant expresses is a maximum iteration count, so
something like IXGBE_DEVLINK_RELOAD_MAX_ITER, or a bound stated in ms
next to the 500 ms step, would say what the value is.

>  
>  /**
>   * ixgbe_devlink_reload_empr_finish - finishes EMP reset
> @@ -460,11 +460,11 @@ static int ixgbe_devlink_reload_empr_finish(struct devlink *devlink,
>  		 * may be not cleared yet, so begin the loop with the delay
>  		 * in order to not check the not updated register.
>  		 */
> -		mdelay(500);
> +		msleep(500);
>  
>  		fwsm = IXGBE_READ_REG(hw, IXGBE_FWSM(hw));
>  
> -		if (i++ >= IXGBE_DEVLINK_RELOAD_TIMEOUT_SEC)
> +		if (i++ >= IXGBE_DEVLINK_RELOAD_TIMEOUT_TICS)
>  			return -ETIME;

[Severity: Medium]
This isn't a bug introduced by this patch, but since the patch renames the
macro to declare its unit as tics and touches the delay right above it:
does this loop really perform the 20 tics that the macro name and the
comment advertise?

With i starting at 0, the test is false for the pre-increment values 0
through 19, so the check first fires on the 21st pass:

    do {
    	msleep(500);

    	fwsm = IXGBE_READ_REG(hw, IXGBE_FWSM(hw));

    	if (i++ >= IXGBE_DEVLINK_RELOAD_TIMEOUT_TICS)
    		return -ETIME;

    } while (!(fwsm & IXGBE_FWSM_FW_VAL_BIT));

That is 21 sleeps, i.e. at least 10.5 s, and more once msleep() rounds up
to whole jiffies, against the "/*Wait for 10 sec with 0.5 sec tic." above.

Separately, is the fwsm value read on that last pass being thrown away?
The return -ETIME happens before the while() condition is evaluated, so if
firmware sets IXGBE_FWSM_FW_VAL_BIT exactly in the final 500 ms window,
devlink reload reports -ETIME even though the read already showed the bit
set.  In that case *actions_performed is left unset, the
IXGBE_FLAG2_API_MISMATCH and IXGBE_FLAG2_FW_ROLLBACK bits stay stale and
ixgbe_refresh_fw_version() is skipped.  Would moving the bound check
before the sleep, or testing fwsm before returning -ETIME, be preferable?

>  
>  	} while (!(fwsm & IXGBE_FWSM_FW_VAL_BIT));

[Severity: Medium]
This is also a pre-existing issue rather than something this patch
introduces, but can a failed register read end this wait early and be
reported as a successful firmware activation?

IXGBE_READ_REG() goes to ixgbe_read_reg() in ixgbe_main.c, which returns
IXGBE_FAILED_READ_REG (0xFFFFFFFF) when the adapter is marked removed, and
otherwise funnels an all-ones readl() through ixgbe_check_remove():

drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:ixgbe_read_reg() {
	if (ixgbe_removed(reg_addr))
		return IXGBE_FAILED_READ_REG;
	...
	value = readl(reg_addr + reg);
	if (unlikely(value == IXGBE_FAILED_READ_REG))
		value = ixgbe_check_remove(hw, reg);
}

ixgbe_check_remove() only tolerates about 15 ms of unresponsiveness before
declaring the adapter gone:

drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:ixgbe_check_remove() {
	for (i = 0; i < IXGBE_FAILED_READ_RETRIES; i++) {
		value = readl(reg_addr + IXGBE_STATUS);
		if (value != IXGBE_FAILED_READ_REG)
			break;
		mdelay(3);
	}

	if (value == IXGBE_FAILED_READ_REG)
		ixgbe_remove_adapter(hw);
}

0xFFFFFFFF has IXGBE_FWSM_FW_VAL_BIT (BIT(15)) set, so the while()
condition above is satisfied by a failed read.  The function then sets
*actions_performed to BIT(DEVLINK_RELOAD_ACTION_FW_ACTIVATE), clears the
API_MISMATCH and FW_ROLLBACK flags and calls ixgbe_refresh_fw_version(),
which issues ACI commands to a device the driver has just flagged as
surprise-removed.

Should the poll reject fwsm == IXGBE_FAILED_READ_REG, or check
ixgbe_removed(hw->hw_addr), and keep polling until the timeout instead?
This depends on whether the E610 memory BAR actually returns all-ones
during the EMP reset window, which I could not confirm from the tree.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914092626.263886-1-xiaolinkui%40126.com