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

[Pin-yen Lin]

On Tue, Oct 27, 2026 at 9:42 AM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 1 · Low: 0
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [Medium] Synchronous link-status update in gve_queues_start() races
>   with gve_service_task(); the carrier can be left in the wrong state.
>
> --
>
> gve: Fix link flap after ethtool ops
>
> gve_queues_start() now calls gve_turnup_and_check_status() directly
> instead of queueing the service task, so the carrier state is updated
> before the call returns to userspace. This avoids the transient carrier
> drop after ethtool reconfiguration that was breaking the
> drivers/net/ping.py selftest.
>
> > diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> > index 52c152a986eb7..e2a016f4430a8 100644
> > --- a/drivers/net/ethernet/google/gve/gve_main.c
> > +++ b/drivers/net/ethernet/google/gve/gve_main.c
>
> [ ... ]
>
> > @@ -1486,8 +1515,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);
>
> [Severity: Medium]
> Can this race with gve_service_task() and leave the carrier in the wrong
> state?
>
> Before this change, gve_queues_start() sent its link update through
> service_task. A work item never runs concurrently with itself, so these
> updates were serialized.
>
> gve_queues_start() is reached from ndo_open, from gve_adjust_config(),
> and from gve_reset()->gve_recover()->gve_open(). It holds rtnl and the
> netdev instance lock, and it now runs the same sequence as
> gve_service_task().
>
> gve_service_task() is queued from gve_mgmnt_intr() and holds neither
> lock:
>
> gve_service_task() {
>     ...
>         gve_handle_reset(priv);
>         ops->report_link_status(priv);
>         gve_handle_link_status(priv);
> }
>
> gve_adminq_report_link_status() reads the register and writes the
> cached value in two separate steps:
>
>         status = ioread32be(&priv->reg_bar0->device_status);
>         WRITE_ONCE(priv->link_up,
>                    !!(GVE_DEVICE_STATUS_LINK_STATUS_MASK & status));
>
> Here is one possible interleaving:
>
> gve_queues_start()                  gve_service_task()
>   ioread32be() -> link down
>                                     (device link up, mgmt irq)
>                                     ioread32be() -> link up
>                                     WRITE_ONCE(link_up, true)
>                                     netif_carrier_on()
>   WRITE_ONCE(link_up, false)
>   netif_carrier_off()
>
> At the end the carrier is off while the device link is up. The service
> task run triggered by the interrupt has already finished, so nothing
> checks the link again until the device raises another status interrupt.
>
> gve_handle_link_status() has a similar window, because its check and
> its action are not done under a lock:
>
>         link_up = READ_ONCE(priv->link_up);
>         if (link_up == netif_carrier_ok(priv->dev))
>                 return;
>
>         if (link_up) {
>                 netdev_info(priv->dev, "Device link is up.\n");
>                 netif_carrier_on(priv->dev);
>
> Suppose gve_queues_start() reads link_up == true with the carrier off,
> and is then delayed in netdev_info(). In that time gve_service_task()
> can write link_up = false, see that it matches netif_carrier_ok(), and
> return. gve_queues_start() then calls netif_carrier_on() while the link
> is down.
>
> gve_set_xdp() and the per-queue start path already use the same
> pattern without a lock. This patch also brings it to the common
> gve_queues_start() path, which used to be serialized through the
> service task.
>
> Should gve_turnup_and_check_status() and gve_service_task() be
> serialized against each other here?

Yes, this needs to be serialized. All other callers of
gve_handle_link_status() already run under the netdev instance lock,
so in v4 we will take netdev_lock() around the link status update in
gve_service_task() as well.
>
> >       priv->interface_up_cnt++;
> >       return 0;
>
> [ ... ]
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023204706.3112215-1-hramamurthy%40google.com

Regards,
Pin-yen