Re: [PATCH v3 0/2] net/mana: add device reset support

[Stephen Hemminger <stephen@networkplumber.org>]

On Thu, 21 May 2026 23:59:41 -0700
Wei Hu <weh@linux.microsoft.com> wrote:

> From: Wei Hu <weh@microsoft.com>
> 
> Add support for handling hardware service reset events in the
> MANA driver. When the MANA kernel driver receives a hardware
> service event, it initiates a device reset and notifies userspace
> via IBV_EVENT_DEVICE_FATAL. The MANA PMD handles this by
> performing an automatic teardown and recovery sequence.
> 
> The driver uses ethdev recovery events (ERR_RECOVERING,
> RECOVERY_SUCCESS, RECOVERY_FAILED) to notify upper layers of
> the reset lifecycle, and a PCI device removal event callback
> to distinguish hot-remove from service reset.
> 
> Changes since v2:
> - Fixed dev_state_qsv memory leak on device removal
> - Fixed reset thread TCB/stack leak: reset_thread_active is now
>   only cleared by the joiner, not the thread itself
> - Fixed second reset crash: removed reset thread join logic from
>   mana_dev_close (inner function) to avoid corrupting dev_state
>   when called from mana_reset_enter
> - Made reset_thread_active RTE_ATOMIC(bool) with explicit ordering
> - Added retry loop for rte_dev_event_callback_unregister on -EAGAIN
> - Initialized condvar/mutex with PTHREAD_PROCESS_SHARED since priv
>   is in hugepage shared memory
> - Added re-check of dev_state after lock acquisition in
>   mana_intr_handler to prevent racing with pci_remove_event_cb
> - Replaced (void *)0 with NULL in mp.c
> - Added lock ownership comment block at mana_reset_enter
> - Documented rte_dev_event_monitor_start() requirement
> - Added mana.rst documentation and release note (patch 2/2)
> 
> Changes since v1:
> - Removed net/netvsc patch from this series
> - Simplified reset exit: mana_reset_exit calls
>   mana_reset_exit_delay directly instead of spawning a thread
> - Added __rte_no_thread_safety_analysis annotations for clang
> - Switched to rte_thread_create_internal_control
> - Fixed declaration-after-statement style issues
> - Removed unnecessary blank lines and stale comments
> 
> Wei Hu (2):
>   net/mana: add device reset support
>   net/mana: add documentation for device reset support
> 
>  doc/guides/nics/mana.rst               |  33 +
>  doc/guides/rel_notes/release_26_07.rst |   8 +
>  drivers/net/mana/mana.c                | 995 ++++++++++++++++++++++---
>  drivers/net/mana/mana.h                |  33 +-
>  drivers/net/mana/meson.build           |   2 +-
>  drivers/net/mana/mp.c                  |  89 ++-
>  drivers/net/mana/mr.c                  |   6 +-
>  drivers/net/mana/rx.c                  |  24 +-
>  drivers/net/mana/tx.c                  |  40 +-
>  9 files changed, 1123 insertions(+), 107 deletions(-)
> 

AI review found a few things...


Thanks for the v3.  Most of the v2 items are addressed: dev_state_qsv
is freed via mana_dev_free_resources, reset_thread_active is now
RTE_ATOMIC(bool), the joiner owns the flag, pthread mutex/cond use
PROCESS_SHARED attrs, intr_handler re-checks state under the lock,
the lock hand-off has a clear comment, and a doc + release note
patch is included.

A few items remain or were introduced by the v2->v3 changes.

Errors

1. Deadlock in dev_close_lock + EAGAIN loop on PCI-remove callback.

   The v3 fix for the discarded unregister return uses a busy-wait:

       if (dev->device) {
               do {
                       ret = rte_dev_event_callback_unregister(...);
               } while (ret == -EAGAIN);
       }

   mana_intr_uninstall runs from mana_dev_close, which runs from
   mana_dev_close_lock with reset_ops_lock held.  EAL returns
   -EAGAIN while cb_lst->active is 1 (callback dispatching).  The
   callback in question is mana_pci_remove_event_cb, which itself
   blocks on rte_spinlock_lock(&priv->reset_ops_lock).  Result:
   dev_close holds the spinlock, the callback waits for the
   spinlock, the loop waits for the callback to finish -- three-way
   deadlock if a hot-remove event arrives during close.

   Either drop the lock before unregistering and re-take it after,
   or (better) use a sleeping mutex for reset_ops_lock so the
   callback can complete while close is suspended.

2. reset_ops_lock is still rte_spinlock_t held across blocking work.

   Unchanged from v2: the spinlock is held through

     - mana_reset_enter:      rte_rcu_qsbr_check spin,
                              mana_dev_stop,
                              mana_mp_req_on_rxtx (5s IPC timeout),
                              mana_dev_close
     - mana_reset_exit_delay: ibv_close_device, mana_pci_probe,
                              mana_mp_req_on_rxtx, mana_dev_start

   Any other ethdev op that hits dev_ops while reset is in flight
   spins for tens of seconds.  Blocking calls under a spinlock are
   an Error; use a properly-initialised pthread_mutex_t (you
   already have PROCESS_SHARED attrs handy).

Warnings

3. Secondary process: qsbr does not actually quiesce secondary lcores.

   rte_rcu_qsbr_thread_register is only called from
   mana_dev_configure, which the secondary never runs.  The
   secondary's rx_burst/tx_burst still call thread_online/offline
   against the shared qsv, but the secondary tids are unregistered,
   so they are invisible to rte_rcu_qsbr_check in the primary, and
   the secondary MP handler (mana_mp_reset_enter) does not call
   qsbr_check at all -- it just sets db_page = NULL and munmaps.

   The dev_state check at the top of secondary tx_burst is racy:
   the page can be munmapped after the in-loop read of db_page but
   before the doorbell write at the bottom.  The "All secondary
   threads are quiescent" log line in mana_mp_reset_enter is not
   true.

   The secondary needs a real reader-side primitive -- its own
   qsbr with secondary lcore registration, or an rwlock the MP
   handler takes before munmap.

4. Double-join race between dev_close_lock and mana_reset_enter.

   dev_close_lock signals + joins the reset thread *before* taking
   reset_ops_lock.  intr_handler may fire in that window:

     1. dev_close_lock: pthread_cond_signal, rte_thread_join
     2. intr_handler:   take lock, see ACTIVE, call reset_enter
     3. reset_enter:    reset_thread_active still true (step 1
                        hasn't cleared it yet) -> rte_thread_join
                        on a thread that step 1 just joined -> UB

   The pre-lock signal/join was added to avoid deadlock with the
   reset thread holding the lock, which is fine in itself, but the
   reset_thread_active update needs to be inside that critical
   section, or reset_enter has to recheck under the lock.

5. mana_dev_close (non-_lock) does not join the reset thread.

   mana_dev_uninit -> mana_dev_close path doesn't go through
   dev_close_lock, so if pci_remove arrives while the reset thread
   is alive, mana_dev_free_resources can destroy reset_cond_mutex /
   reset_cond before the thread has exited.  In practice the
   pci_remove callback signals RESET_FAILED and the reset thread
   should exit quickly, but the ordering isn't enforced.  Either
   join in mana_dev_uninit, or document the assumption.

Info

6. mana_reset_exit_delay sets priv->ib_ctx = NULL even when
   ibv_close_device fails:

       ret = ibv_close_device(priv->ib_ctx);
       priv->ib_ctx = NULL;
       if (ret) { ... goto out; }

   The handle is leaked if close fails.  Either keep the old
   pointer on failure or accept that close-failure is unrecoverable
   and free it anyway with a comment.

Patch 2/2

7. The events list is a term/description pattern and should be a
   definition list per DPDK RST style:

       ``RTE_ETH_EVENT_ERR_RECOVERING``
          Reset has started.

       ``RTE_ETH_EVENT_RECOVERY_SUCCESS``
          Device has recovered successfully.

       ``RTE_ETH_EVENT_RECOVERY_FAILED``
          Recovery failed.

8. Convention is to fold doc + release note updates into the same
   commit as the feature.  Patches 1/2 and 2/2 can be squashed.