Re: [PATCH net v4 4/4] gve: Fix link flap after ethtool ops

[Pin-yen Lin]

On Mon, Nov 2, 2026 at 1:18 PM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
>
> Low priority / nit picks for your consideration:
>
> - [Low] The patch removes `queue_work(priv->gve_wq, &priv->service_task)`
>   from the end of gve_queues_start()…
>
> --
>
> gve: Fix link flap after ethtool ops
>
> This patch changes gve_queues_start() to update the carrier state
> synchronously through gve_turnup_and_check_status(), instead of queueing
> the service task to do it. It also takes netdev_lock() around the link
> status update in gve_service_task(), so that update is serialized with
> the synchronous path.
>
> > diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> > index b5e2f39a903c5..bc7d7c41948c3 100644
> > --- a/drivers/net/ethernet/google/gve/gve_main.c
> > +++ b/drivers/net/ethernet/google/gve/gve_main.c
>
> [ ... ]
>
> > @@ -1494,8 +1526,7 @@ static int gve_queues_start(struct gve_priv *priv,
> >                         round_jiffies(jiffies +
> >                               msecs_to_jiffies(priv->stats_report_timer_period)));
> >
> > -     gve_turnup(priv);
> > -     queue_work(priv->gve_wq, &priv->service_task);
> > +     gve_turnup_and_check_status(priv);
> >       priv->interface_up_cnt++;
> >       return 0;
>
> [Severity: Low]
> Commit 3b7cc73628ff added this queue_work() for the link status update,
> and the new synchronous check handles that. Does removing it also drop
> the service task run that used to follow every reset recovery?
>
> gve_reset() clears the reset flag first and then disables the service
> task:
>
> gve_reset() {
>     ...
>         gve_clear_do_reset(priv);
>         gve_set_reset_in_progress(priv);
>
>         if (was_up) {
>                 gve_turndown(priv);
>                 ...
>         }
>
>         disable_work(&priv->service_task);
>         ...
>         enable_work(&priv->service_task);
>         err = gve_recover(priv, was_up && !skip_queue_setup);
>     ...
> }
>
> gve_schedule_reset() could run after gve_clear_do_reset() but before
> disable_work(). One way is gve_tx_timeout() from dev_watchdog racing
> with gve_turndown(). Another is an RX NAPI error path just before
> napi_disable:
>
> gve_schedule_reset() {
>         gve_set_do_reset(priv);
>         queue_work(priv->gve_wq, &priv->service_task);
> }
>
> In that case, disable_work() cancels the queued work, or queue_work()
> is rejected while the work is disabled. enable_work() does not queue it
> again.
>
> Before this patch, gve_recover()->gve_open()->gve_queues_start() ended
> with queue_work(), so a leftover GVE_PRIV_FLAGS_DO_RESET was handled
> right after recovery.
>
> After this patch, the service task is not queued again until some
> unrelated event, such as a later gve_mgmnt_intr(). When it does run,
> gve_service_task()->gve_handle_reset() would perform a full gve_reset()
> because of the stale flag. That reset could happen long after, and
> unrelated to, the event that set the flag.
>
> gve_suspend() and gve_resume() seem to have the same gap. They call
> disable_work_sync() in gve_suspend() and enable_work() before
> gve_recover() in gve_resume().
>
> The other parts of this look fine. A missed device REPORT_STATS request
> only skips a stats_report_trigger_cnt increment, because
> gve_stats_report_schedule() sets do_report_stats on every timer period.
> A device RESET request raised inside the disable window is covered by
> the reset that is already running.
>
> Could gve_reset() and gve_resume() queue the service task again after
> recovery, or re-check gve_get_do_reset() once the work is re-enabled?

Yes, this is indeed a regression introduced by this patch. When the
reset/suspend races with the Rx NAPI polling error path, the reset
will be delayed until the next service task is queued.

We will add an additional queue_work() after the
gve_turnup_and_check_status() in gve_queues_start().
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029211634.2232784-1-hramamurthy%40google.com

Regards,
Pin-yen