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

[netdev-bot+sashiko@kernel.org]

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?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029211634.2232784-1-hramamurthy%40google.com