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

[Linkui Xiao <xiaolinkui@126.com>]

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