[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));