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@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 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@xxxxxxxxx>
> > Signed-off-by: Linkui Xiao <xiaolinkui@xxxxxxxxxx>
> > ---
> > v1:https://lore.kernel.org/all/20260914092626.263886-1-xiaolinkui@xxxxxxx/
> >
> > 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@xxxxxxxxxxxxx>
>
>
> Kind regards,
>
> Paul