> -----Original Message-----
> From: Nguyen, Anthony L <[email protected]>
> Sent: Thursday, October 8, 2026 7:35 PM
> To: Loktionov, Aleksandr <[email protected]>; intel-wired-
> [email protected]
> Cc: Simon Horman <[email protected]>
> Subject: Re: [PATCH iwl-next v2] ixgbe: remove ixgbe_ping_all_vfs()
> from link state change handlers
>
>
>
> On 9/18/2026 6:33 AM, Aleksandr Loktionov wrote:
> > When multiple VFs are brought up simultaneously, a VF can receive a
> > CTS (Clear To Send) mailbox message instead of the expected RESET |
> > (N)ACK response, because the watchdog fires a gratuitous ping right
> as
> > the VF mailbox transaction is in progress.
> >
> > Remove the ixgbe_ping_all_vfs() call from both
> > ixgbe_watchdog_link_is_up() and ixgbe_watchdog_link_is_down(). Both
> > paths contain an identical call with the same race window during VF
> > initialization: link-state changes (up or down) can arrive while VFs
> > are mid-initialization, and the gratuitous ping from either
> direction
> > collides with the VF mailbox state machine for the same reason.
> >
> > Link-state changes are already communicated to VFs through the
> normal
> > mailbox protocol; these extra pings are redundant on both paths.
> >
> > This removes the last two callers of ixgbe_ping_all_vfs(), so remove
> > the now-unused function and its declaration as well.
> >
> > Suggested-by: Sebastian Basierski <[email protected]>
> > Signed-off-by: Aleksandr Loktionov <[email protected]>
> > Reviewed-by: Simon Horman <[email protected]>
> > ---
> > v1 -> v2: no code changes, resending to pick up Reviewed-by
> > ---
> > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 ------
> > drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 14 --------------
> > drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h | 1 -
> > 3 files changed, 21 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > index f918564..365acf8 100644
> > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > @@ -8181,9 +8181,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);
>
> From Sashiko:
>
> Does removing this PF-to-VF mailbox write prevent the PF from sending
> the CTS message required for the VF to transition its internal link
> state from down to up?
>
> The ixgbevf driver appears to rely on this CTS message to correctly
> clear mac->get_link_status and transition to up:
>
> drivers/net/ethernet/intel/ixgbevf/vf.c:ixgbevf_check_mac_link_vf() {
> ...
> if (!(in_msg & IXGBE_VT_MSGTYPE_CTS)) {
> /* msg is not CTS and is NACK we must have lost CTS status */
> if (in_msg & IXGBE_VT_MSGTYPE_FAILURE)
> ret_val = -1;
> goto out;
> }
> ...
> mac->get_link_status = false;
> out:
> *link_up = !mac->get_link_status;
> }
>
> Without this ping, will the VF persistently fail the mailbox read and
> force the link state down?
>
> > }
> >
> > /**
> > @@ -8214,9 +8211,6 @@ static void ixgbe_watchdog_link_is_down(struct
> > ixgbe_adapter *adapter)
> >
> > 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);
>
> Does removing this ping sever the mechanism that triggers the VF's
> mailbox interrupt on physical link drops?
>
> It looks like the ixgbevf driver uses mac->get_link_status to
> determine if it should poll the physical link. Once up, it sets this
> to false and relies on a mailbox interrupt (ixgbevf_msix_other()) to
> set it back to 1 on state changes.
>
> Will SR-IOV VFs remain stuck in the up state and permanently report
> incorrect link states if they are not notified of link drops via this
> mailbox write?
>
> > }
> >
> > static bool ixgbe_ring_tx_pending(struct ixgbe_adapter *adapter)
> > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
> > b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
> > index 431d77d..da641e9 100644
> > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
> > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
> > @@ -1515,20 +1515,6 @@ static inline void ixgbe_ping_vf(struct
> ixgbe_adapter *adapter, int vf)
> > ixgbe_write_mbx(hw, &ping, 1, vf);
> > }
> >
> > -void ixgbe_ping_all_vfs(struct ixgbe_adapter *adapter) -{
> > - struct ixgbe_hw *hw = &adapter->hw;
> > - u32 ping;
> > - int i;
> > -
> > - for (i = 0 ; i < adapter->num_vfs; i++) {
> > - ping = IXGBE_PF_CONTROL_MSG;
> > - if (adapter->vfinfo[i].clear_to_send)
> > - ping |= IXGBE_VT_MSGTYPE_CTS;
> > - ixgbe_write_mbx(hw, &ping, 1, i);
> > - }
> > -}
>
> Is this function actually essential for notifying VFs of asynchronous
> link state changes?
>
> Since VFs do not have access to physical PHY interrupts, it seems they
> might depend on this PF-initiated ping to learn about state
> transitions.
>
> > /**
> > * ixgbe_set_all_vfs - update vfs queues
> > * @adapter: Pointer to adapter struct diff --git
> > a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h
> > b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h
> > index bc4cab9..f755bfd 100644
> > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h
> > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h
> > @@ -18,7 +18,6 @@ void ixgbe_restore_vf_multicasts(struct
> ixgbe_adapter *adapter);
> > bool ixgbe_check_mdd_event(struct ixgbe_adapter *adapter);
> > void ixgbe_msg_task(struct ixgbe_adapter *adapter);
> > int ixgbe_vf_configuration(struct pci_dev *pdev, unsigned int
> > event_mask); -void ixgbe_ping_all_vfs(struct ixgbe_adapter
> *adapter);
> > void ixgbe_set_all_vfs(struct ixgbe_adapter *adapter);
> > int ixgbe_ndo_set_vf_mac(struct net_device *netdev, int queue, u8
> *mac);
> > int ixgbe_ndo_set_vf_vlan(struct net_device *netdev, int queue,
> u16
> > vlan,
Good day, Tony
Good catch! I dug through ixgbevf's link-check path and the concern holds up.
ixgbevf_check_mac_link_vf() only re-reads the VFLINKS register or looks
for a new CTS message while mac->get_link_status is true. Once a VF
latches get_link_status = false ("link is up, no need to recheck"), the
only things that flip it back to true are a mailbox reset, a mailbox
timeout, or the "other" MSI-X IRQ - which fires only when the PF writes
something new to that VF's mailbox. Once a VF finishes its API/queue
negotiation and goes quiet, nothing else writes to its mailbox, so
there's no delivery path left for either direction once both pings
are gone:
- watchdog_link_is_up(): without the ping's CTS, a VF still waiting on
get_link_status will keep seeing an up VFLINKS register but never
receive the CTS it needs to actually clear the flag.
- watchdog_link_is_down(): without the ping, a VF that already latched
get_link_status = false has no trigger to ever re-read the register,
so it can get stuck reporting link up after the physical link drops.
The CTS-collision race during simultaneous VF bring-up that motivated
this patch is real, but removing the ping outright trades it for a
worse, silent one.
Dropping this patch rather than carry the
Reviewed-by forward onto a diff with this gap. If I come back to the
underlying race, it'll need to narrow the window instead (e.g. only
suppress the ping while a VF reset is actually in flight), not remove
notification entirely.
Thanks,
Alex
