Re: [Intel-wired-lan] [PATCH iwl-next] ixgbe: remove ixgbe_ping_all_vfs() from watchdog link-up handler

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
ixgbe: remove ixgbe_ping_all_vfs() from watchdog link-up handler

This patch removes the ixgbe_ping_all_vfs() call from
ixgbe_watchdog_link_is_up() to fix a race condition where VFs receive
CTS (Clear To Send) mailbox messages instead of expected RESET|ACK
responses during initialization.

> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 0bc806aaed90..6996547a8084 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -8151,9 +8151,6 @@ static void ixgbe_watchdog_link_is_up(struct ixgbe_adapter *adapter)
>
>  	/* update the default user priority for VFs */
>  	ixgbe_update_default_up(adapter);
> -
> -	/* ping all the active vfs to let them know link has changed */
> -	ixgbe_ping_all_vfs(adapter);
>  }

The patch removes ixgbe_ping_all_vfs() from ixgbe_watchdog_link_is_up()
to prevent the race condition during VF initialization. However, the
identical call remains in ixgbe_watchdog_link_is_down().

Doesn't the same race condition apply to link-down events?

When a VF initiates reset and the physical link state changes to down,
the watchdog would trigger:

VF side:
- Sends IXGBE_VF_RESET message
- Waits for RESET|ACK response

PF side:
- ixgbe_vf_reset_msg() processes reset, sets clear_to_send=true
- Link goes down, triggers watchdog
- ixgbe_watchdog_link_is_down() calls ixgbe_ping_all_vfs()
- Sends gratuitous ping with CTS bit

VF receives CTS message instead of expected RESET response, causing the
same initialization failure described in the commit message.

Looking at the unchanged code in ixgbe_watchdog_link_is_down():

	e_info(drv, "NIC Link is Down\n");
	netif_carrier_off(netdev);

	/* ping all the active vfs to let them know link has changed */
	ixgbe_ping_all_vfs(adapter);

The commit message states "Link-state changes are already communicated
to VFs through the normal mailbox protocol; the extra ping here is
redundant and races with VF initialization."

Does this reasoning apply equally to the link-down case?