[Intel-wired-lan] [PATCH net v2 1/2] ixgbe: do not busy wait in ixgbe_devlink_reload_empr_finish()

[Intel-wired-lan] [PATCH net v2 1/2] 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 spins the CPU for half
a second per iteration and, because the loop bound is 20 iterations, for
up to ten seconds. That keeps a CPU fully occupied while the firmware
performs the EMP reset, and on CONFIG_PREEMPT_NONE it also makes the
loop non-preemptible for that whole window. The loop does not need to be
atomic and holds no spinlock.

Use msleep() instead.

Fixes: c9e563cae19e ("ixgbe: add support for devlink reload")
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
---
v1:https://lore.kernel.org/all/20260914092626.263886-1-xiaolinkui@126.com/

v2:
  - Reworded the commit message: mdelay() does not mask interrupts or
    disable preemption, and the ~10 s window is below the soft lockup /
    RCU stall thresholds.  State the actual rationale instead.
  - Split the macro rename into a separate patch.
  - The mdelay() -> msleep() change itself is unchanged, so the
    Reviewed-by tag is kept.

 drivers/net/ethernet/intel/ixgbe/devlink/devlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c b/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
index cf8908b82f8a..781f13240a0d 100644
--- a/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
+++ b/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
@@ -460,7 +460,7 @@ 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));
 
-- 
2.25.1

[Intel-wired-lan] [PATCH net v2 2/2] ixgbe: rename the EMP reset timeout constant

[Linkui Xiao]

From: Linkui Xiao <xiaolinkui@kylinos.cn>

IXGBE_DEVLINK_RELOAD_TIMEOUT_SEC is misleading: the value counts 0.5 s
poll iterations, not seconds, as the comment right above it already
explains. Rename it to IXGBE_DEVLINK_RELOAD_MAX_ITER, which says what the
value actually is.

Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
---
 drivers/net/ethernet/intel/ixgbe/devlink/devlink.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c b/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
index 781f13240a0d..15b14c5a7c4e 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_MAX_ITER	20
 
 /**
  * ixgbe_devlink_reload_empr_finish - finishes EMP reset
@@ -464,7 +464,7 @@ static int ixgbe_devlink_reload_empr_finish(struct devlink *devlink,
 
 		fwsm = IXGBE_READ_REG(hw, IXGBE_FWSM(hw));
 
-		if (i++ >= IXGBE_DEVLINK_RELOAD_TIMEOUT_SEC)
+		if (i++ >= IXGBE_DEVLINK_RELOAD_MAX_ITER)
 			return -ETIME;
 
 	} while (!(fwsm & IXGBE_FWSM_FW_VAL_BIT));
-- 
2.25.1

Re: [Intel-wired-lan] [PATCH net v2 1/2] ixgbe: do not busy wait in ixgbe_devlink_reload_empr_finish()

[Paul Menzel]

Dear Linkui,


Thank you for your patch.

Am 17.09.26 um 08:54 schrieb 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 spins the CPU for half
> a second per iteration and, because the loop bound is 20 iterations, for
> up to ten seconds. That keeps a CPU fully occupied while the firmware
> performs the EMP reset, and on CONFIG_PREEMPT_NONE it also makes the
> loop non-preemptible for that whole window. The loop does not need to be
> atomic and holds no spinlock.
> 
> Use msleep() instead.

Should you resend, please document the commands how to test this, for 
example, how to trigger .reload.

> Fixes: c9e563cae19e ("ixgbe: add support for devlink reload")
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
> ---
> v1:https://lore.kernel.org/all/20260914092626.263886-1-xiaolinkui@126.com/
> 
> v2:
>    - Reworded the commit message: mdelay() does not mask interrupts or
>      disable preemption, and the ~10 s window is below the soft lockup /
>      RCU stall thresholds.  State the actual rationale instead.
>    - Split the macro rename into a separate patch.
>    - The mdelay() -> msleep() change itself is unchanged, so the
>      Reviewed-by tag is kept.
> 
>   drivers/net/ethernet/intel/ixgbe/devlink/devlink.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c b/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
> index cf8908b82f8a..781f13240a0d 100644
> --- a/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
> +++ b/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
> @@ -460,7 +460,7 @@ 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));


Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul

Re: [Intel-wired-lan] [PATCH net v2 2/2] ixgbe: rename the EMP reset timeout constant

[Paul Menzel]

Dear Linkui,


Thank you for your patch.

Am 17.09.26 um 08:54 schrieb Linkui Xiao:
> From: Linkui Xiao <xiaolinkui@kylinos.cn>
> 
> IXGBE_DEVLINK_RELOAD_TIMEOUT_SEC is misleading: the value counts 0.5 s
> poll iterations, not seconds, as the comment right above it already
> explains. Rename it to IXGBE_DEVLINK_RELOAD_MAX_ITER, which says what the
> value actually is.
> 
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
> ---
>   drivers/net/ethernet/intel/ixgbe/devlink/devlink.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c b/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
> index 781f13240a0d..15b14c5a7c4e 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_MAX_ITER	20
>   
>   /**
>    * ixgbe_devlink_reload_empr_finish - finishes EMP reset
> @@ -464,7 +464,7 @@ static int ixgbe_devlink_reload_empr_finish(struct devlink *devlink,
>   
>   		fwsm = IXGBE_READ_REG(hw, IXGBE_FWSM(hw));
>   
> -		if (i++ >= IXGBE_DEVLINK_RELOAD_TIMEOUT_SEC)
> +		if (i++ >= IXGBE_DEVLINK_RELOAD_MAX_ITER)
>   			return -ETIME;
>   
>   	} while (!(fwsm & IXGBE_FWSM_FW_VAL_BIT));

Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul

RE: [Intel-wired-lan] [PATCH net v2 2/2] ixgbe: rename the EMP reset timeout constant

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Linkui Xiao <xiaolinkui@126.com>
> Sent: Thursday, September 17, 2026 8:55 AM
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel,
> Przemyslaw <przemyslaw.kitszel@intel.com>; andrew+netdev@lunn.ch;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; horms@kernel.org; Jagielski, Jedrzej
> <jedrzej.jagielski@intel.com>; kees@kernel.org; Loktionov, Aleksandr
> <aleksandr.loktionov@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Linkui Xiao <xiaolinkui@kylinos.cn>
> Subject: [Intel-wired-lan] [PATCH net v2 2/2] ixgbe: rename the EMP
> reset timeout constant
> 
> From: Linkui Xiao <xiaolinkui@kylinos.cn>
> 
> IXGBE_DEVLINK_RELOAD_TIMEOUT_SEC is misleading: the value counts 0.5 s
> poll iterations, not seconds, as the comment right above it already
> explains. Rename it to IXGBE_DEVLINK_RELOAD_MAX_ITER, which says what
> the value actually is.
> 
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
> ---
>  drivers/net/ethernet/intel/ixgbe/devlink/devlink.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
> b/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
> index 781f13240a0d..15b14c5a7c4e 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_MAX_ITER	20
> 
>  /**
>   * ixgbe_devlink_reload_empr_finish - finishes EMP reset @@ -464,7
> +464,7 @@ static int ixgbe_devlink_reload_empr_finish(struct devlink
> *devlink,
> 
>  		fwsm = IXGBE_READ_REG(hw, IXGBE_FWSM(hw));
> 
> -		if (i++ >= IXGBE_DEVLINK_RELOAD_TIMEOUT_SEC)
> +		if (i++ >= IXGBE_DEVLINK_RELOAD_MAX_ITER)
>  			return -ETIME;
> 
>  	} while (!(fwsm & IXGBE_FWSM_FW_VAL_BIT));
> --
> 2.25.1

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

Re: [Intel-wired-lan] [PATCH net v2 1/2] ixgbe: do not busy wait in ixgbe_devlink_reload_empr_finish()

[Linkui Xiao]

On 2026/9/17 15:09, Paul Menzel wrote:
> Dear Linkui,
> 
> 
> Thank you for your patch.
> 
> Am 17.09.26 um 08:54 schrieb 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 spins the CPU for half
>> a second per iteration and, because the loop bound is 20 iterations, for
>> up to ten seconds. That keeps a CPU fully occupied while the firmware
>> performs the EMP reset, and on CONFIG_PREEMPT_NONE it also makes the
>> loop non-preemptible for that whole window. The loop does not need to be
>> atomic and holds no spinlock.
>>
>> Use msleep() instead.
> 
> Should you resend, please document the commands how to test this, for 
> example, how to trigger .reload.
Hi Paul,

Thanks for the review and the Reviewed-by.

I don't have an E610 adapter available to actually exercise the devlink
reload path, so I can't provide a tested-by or a verified test log. The
change is a mechanical mdelay() -> msleep() conversion in a sleepable
process-context loop; the rationale stands on its own, but I understand
if you'd prefer to wait for someone with the hardware to run it.

If you'd still like the test commands documented for reference, I can
add them to the changelog, but I want to be clear that I have not run
them myself.

Thanks,
Linkui
> 
>> Fixes: c9e563cae19e ("ixgbe: add support for devlink reload")
>> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>> Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
>> ---
>> v1:https://lore.kernel.org/all/20260914092626.263886-1-xiaolinkui@126.com/
>>
>> v2:
>>    - Reworded the commit message: mdelay() does not mask interrupts or
>>      disable preemption, and the ~10 s window is below the soft lockup /
>>      RCU stall thresholds.  State the actual rationale instead.
>>    - Split the macro rename into a separate patch.
>>    - The mdelay() -> msleep() change itself is unchanged, so the
>>      Reviewed-by tag is kept.
>>
>>   drivers/net/ethernet/intel/ixgbe/devlink/devlink.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c 
>> b/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
>> index cf8908b82f8a..781f13240a0d 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
>> @@ -460,7 +460,7 @@ 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));
> 
> 
> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
> 
> 
> Kind regards,
> 
> Paul