Re: [PATCH v2 0/5] net/failsafe: fixes for sub-device removal and configuration

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 14 Oct 2026 22:15:59 +0000
Sandeep Penigalapati <sandeep.penigalapati@intel.com> wrote:

> This series collects fixes for the fail-safe PMD found while testing
> live migration of a VF-backed sub-device (iavf) paired with a virtio-net
> standby sub-device.
> 
> - double release of the sub-device port on hot-unplug;
> - NULL queue-release op on sub-devices that do not implement it;
> - MAC address pool size off-by-one and replay of zeroed MAC slots;
> - unsupported link-status control (-ENOTSUP) treated as fatal;
> - sub-device removal not detected without the RMV interrupt (e.g. VFIO
>   device release), now handled through a bus device-event callback.
> 
> v2:
> - patch 2/5: add an explicit ETH(sdev) != NULL check before dereferencing
>   in fs_rx_queue_release()/fs_tx_queue_release().
> - patch 5/5: clarify in a comment that a secondary process detects the
>   removal via the shared sub_device state checked by fs_rx_unsafe()/
>   fs_tx_unsafe() in the burst path.
> 
> 
> Sandeep Penigalapati (5):
>   net/failsafe: fix double release of sub-device port
>   net/failsafe: fix queue release without sub-device op
>   net/failsafe: fix MAC address pool size and replay
>   net/failsafe: ignore unsupported link status update
>   net/failsafe: detect sub-device removal via device event
> 
>  drivers/net/failsafe/failsafe_ether.c   | 40 ++++++++++--
>  drivers/net/failsafe/failsafe_ops.c     | 29 ++++++++-
>  drivers/net/failsafe/failsafe_private.h | 13 ++--
>  drivers/net/failsafe/failsafe_rxtx.c    | 83 +------------------------
>  4 files changed, 67 insertions(+), 98 deletions(-)
> 

More detail semi-automated AI review had some good ideas.

Subject: Re: [PATCH v2 0/5] net/failsafe: hotplug fixes

Series applies to main (eb903f407c) and builds per commit with
-Dwerror=true. Patches 3 and 4 are fine. Comments on 1, 2 and 5.

The most important point is in 5/5: the removal of the safe/fast
burst switching is the real fix for the hot-unplug crash, and it is
mixed with a new feature under the wrong rationale and Fixes tag.


[PATCH v2 1/5] net/failsafe: fix double release of sub-device port

Warning: commit message describes the wrong release path.

For a sub-device in DEV_ACTIVE, the first release is done by
rte_eth_dev_close() (it calls rte_eth_dev_release_port() since
fbd1913561 "ethdev: remove old close behaviour"), not by the bus
remove. The bus remove path only owns release when the sub-device was
DEV_PROBED and never configured. The code change is correct for both.

Also say what actually happens: since 36c46e7381 the first release
sets eth_dev->data = NULL, so the second release dereferences NULL in
eth_dev_fp_ops_reset(rte_eth_fp_ops + eth_dev->data->port_id) and
decrements allocated_ports twice.

Suggested body:

  The sub-device port is already released before fs_dev_remove()
  calls rte_eth_dev_release_port(): by rte_eth_dev_close() for an
  active sub-device, or by the driver remove in rte_dev_remove()
  for a probed one. The second release dereferences the NULL
  eth_dev->data and crashes.

  Remove the redundant call.

With the else branch gone the remaining braces can go too:

	if (ret < 0)
		ERROR("Bus detach failed for sub_device %u",
		      SUB_ID(sdev));


[PATCH v2 2/5] net/failsafe: fix queue release without sub-device op

Warning: wrong Fixes tag. The unconditional SUBOPS() call is in the
original driver; 7483341ae553 only changed the callback signature.

  Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")

Info: the v2 ETH(sdev) != NULL test is dead code. sdev_port_id is
set at probe and only reset to RTE_MAX_ETHPORTS in fs_dev_remove()
on the way to DEV_UNDEFINED, so ETH(sdev) is never NULL for a
DEV_ACTIVE sub-device. The other FOREACH_SUBDEV_STATE(..., DEV_ACTIVE)
loops in this file do not check it either. Drop it.


[PATCH v2 5/5] net/failsafe: detect sub-device removal via device event

Warning: two unrelated changes with different stable status; split.

(a) Dropping failsafe_set_burst_fn() and the _fast variants is the
actual crash fix, and the commit message gets the reason wrong. It is
not "no longer needed because removal is now reported reliably". Since
c87d435a4d79 ("ethdev: copy fast-path API into separate structure"),
rte_eth_dev_start() copies dev->rx_pkt_burst/tx_pkt_burst into
rte_eth_fp_ops after dev_ops->dev_start returns. fs_dev_start() ->
fs_switch_dev() -> failsafe_set_burst_fn(dev, 0) installs the _fast
variants, which then get copied. The later switch to the safe
variants in failsafe_eth_rmv_event_callback() only writes
dev->rx_pkt_burst, which the data path no longer reads. So after any
removal, RMV interrupt or not, the application keeps calling
failsafe_rx_burst_fast(), which skips fs_rx_unsafe() and dereferences
ETH(sdev) == NULL once the sub-device is gone.

This part should be its own patch:

  net/failsafe: fix Rx/Tx burst after sub-device removal

  Since burst functions are copied into rte_eth_fp_ops at start,
  replacing dev->rx_pkt_burst/tx_pkt_burst afterwards has no
  effect. The fast variants installed at start stay in use after
  a sub-device is removed and dereference the removed device.

  Remove the fast variants and always use the checked burst path.

  Fixes: c87d435a4d79 ("ethdev: copy fast-path API into separate structure")
  Cc: stable@dpdk.org

(b) Registering a bus device-event callback for sub-devices without
RMV interrupt is new functionality. It cannot fix a46f8d584eb8; the
rte_dev_event API did not exist when fail-safe was added. No Fixes:,
no Cc: stable.

Info: the device event is raised automatically only for VFIO (via
the req notifier). For UIO sub-devices it comes from the uevent
monitor, which the application must start with
rte_dev_event_monitor_start() and rte_dev_hotplug_handle_enable().
Worth one line in the commit message or in fail_safe.rst.

Info: failsafe_dev_event_callback() can call the RMV callback with 0
as port_id; the argument is __rte_unused and PORT_ID() plus the cast
only adds noise:

	if (type == RTE_DEV_EVENT_REMOVE)
		failsafe_eth_rmv_event_callback(0, RTE_ETH_EVENT_INTR_RMV,
						cb_arg, NULL);