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@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>

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

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