Re: [PATCH net-next v7 02/13] net: mana: share the EQ pool across a queue-set swap

Re: [PATCH net-next v7 02/13] net: mana: share the EQ pool across a queue-set swap

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 (1) · Medium: 1 (1) · Low: 2

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] Every Ethernet CQ teardown now adds a blocking
  mana_gd_test_eq() on its parent EQ (mana_deinit_cq()) and an…
- [Low] This feature patch ('share the EQ pool across a queue-set swap')
  bundles two fixes for bugs that can be hit today, with no Fixes: tag,…
- [Low] A comment in struct mana_port_context (include/net/mana/mana.h)
  is out of date.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch's
  new reader protocol does not cover it, and the commit message says it…
- [Medium] This problem wasn't introduced by this patch, but the HWC
  teardown frees CQ callback state before IRQ readers are drained.

--

net: mana: share the EQ pool across a queue-set swap

This commit makes the MANA EQ pool port-owned. The old and new queue sets
in a swap now share EQs, and mana_grow_eqs() grows the pool when needed.

It also moves cq_table publication to release/acquire and adds a per-CQ
EQ flush plus an RCU grace period when a CQ is torn down. EQ
initialization now happens before IRQ registration, and the DMA region is
released when EQ creation fails.

> diff --git a/drivers/infiniband/hw/mana/cq.c b/drivers/infiniband/hw/mana/cq.c
> index d4e5e3f912689..0b6ad2547e0ef 100644
> --- a/drivers/infiniband/hw/mana/cq.c
> +++ b/drivers/infiniband/hw/mana/cq.c
> @@ -155,7 +155,8 @@ int mana_ib_install_cq_cb(struct mana_ib_dev *mdev, struct mana_ib_cq *cq)
>  	gdma_cq->type = GDMA_CQ;
>  	gdma_cq->cq.callback = mana_ib_cq_handler;
>  	gdma_cq->id = cq->queue.id;
> -	gc->cq_table[cq->queue.id] = gdma_cq;
> +	/* Pairs with the acquire load in mana_gd_process_eqe(). */
> +	smp_store_release(&gc->cq_table[cq->queue.id], gdma_cq);
>  	return 0;
>  }

[Severity: High]
This is a pre-existing issue, not one introduced by this patch. This patch
adds a release/acquire and RCU reader protocol for cq_table. Shouldn't the
matching RDMA remover change as well?

mana_ib_remove_cq_cb() still frees the published gdma_queue before it
clears the slot, and it never waits for IRQ readers:

drivers/infiniband/hw/mana/cq.c:mana_ib_remove_cq_cb() {
    ...
	kfree(gc->cq_table[cq->queue.id]);
	gc->cq_table[cq->queue.id] = NULL;
}

mana_ib_destroy_cq() calls it before the hardware CQ is destroyed:

	mana_ib_remove_cq_cb(mdev, cq);
	...
	mana_ib_gd_destroy_cq(mdev, cq);

So the CQ can still raise EQEs at that point. Raw/RSS QPs attach these CQs
to the port's mpc->eqs[] pool, which this patch makes long-lived.

At the same time, mana_gd_process_eqe() runs under rcu_read_lock() in
mana_gd_intr() and does:

	cq = smp_load_acquire(&gc->cq_table[cq_id]);
	if (WARN_ON_ONCE(!cq || cq->type != GDMA_CQ || cq->id != cq_id))
		break;

	if (cq->cq.callback)
		cq->cq.callback(cq->cq.context, cq);

Suppose another CPU has already loaded the pointer when kfree() runs. Could
it read freed memory and call through a function pointer taken from it?

After destroy returns, the IB core also frees the mana_ib_cq that
mana_ib_cq_handler() dereferences.

The other CQ removers now unpublish, call synchronize_rcu(), and then
free, as mana_gd_destroy_cq() does. This is the last plain cq_table store
left, yet the commit message says:

    The HWC and RDMA sites need the matching stores, not changes to
    their teardown or recovery policies.

Does that hold for mana_ib_remove_cq_cb()? Userspace can reach it through
ibv_destroy_cq() while completions are still arriving on the same EQ. The
code still looks unchanged at the end of the series.

> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index ae0ed700b3b93..078b3666947a4 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c

[ ... ]

> @@ -1030,17 +1031,17 @@ static int mana_gd_create_eq(struct gdma_dev *gd,
>  		return -EINVAL;
>  	}
>  
> +	queue->eq.callback = spec->eq.callback;
> +	queue->eq.context = spec->eq.context;
> +	queue->head |= INITIALIZED_OWNER_BIT(log2_num_entries);
> +	queue->eq.log2_throttle_limit = spec->eq.log2_throttle_limit ?: 1;
> +
>  	err = mana_gd_register_irq(queue, spec);
>  	if (err) {
>  		dev_err(dev, "Failed to register irq: %d\n", err);
>  		return err;
>  	}
>  
> -	queue->eq.callback = spec->eq.callback;
> -	queue->eq.context = spec->eq.context;
> -	queue->head |= INITIALIZED_OWNER_BIT(log2_num_entries);
> -	queue->eq.log2_throttle_limit = spec->eq.log2_throttle_limit ?: 1;
> -
>  	if (create_hwq) {
>  		err = mana_gd_create_hw_eq(gc, queue);
>  		if (err)

[ ... ]

> @@ -1273,8 +1273,10 @@ int mana_gd_create_mana_eq(struct gdma_dev *gd,
>  	queue->gdma_dev = gd;
>  
>  	err = mana_gd_create_eq(gd, spec, true, queue);
> -	if (err)
> +	if (err) {
> +		mana_gd_destroy_dma_region(gc, gmi->dma_region_handle);
>  		goto out;
> +	}
>  
>  	*queue_ptr = queue;
>  	return 0;

[Severity: Low]
This isn't a bug, but both of these changes look like fixes for problems
that exist today, separate from the queue-set swap. The commit message
says the reorder ensures "a live shared IRQ cannot observe an
uninitialized EQ". It also says the DMA region release "closes an
existing unwind gap".

Before this patch, mana_gd_create_eq() linked the queue into gic->eq_list
with list_add_rcu() in mana_gd_register_irq(). That happened before
eq.callback, eq.context, the owner bit and log2_throttle_limit were set.
With msi_sharing, mana_gd_intr() could walk the new EQ while its head and
owner phase were still zero.

Also, when mana_gd_create_eq() failed, mana_gd_create_mana_eq() only
called mana_gd_free_memory(). That leaked the DMA region registered by
mana_gd_create_dma_region().

Both paths are reachable without any queue-set swap, through
mana_alloc_queues()->mana_create_eq() and through RDMA's
mana_ib_create_eqs().

Would it make sense to split these into separate patches with Fixes: tags,
so stable can pick them up?

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 3bca4b683134b..aefb8646cd79c 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -702,7 +702,8 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
>  	if (!gc->cq_table)
>  		return -ENOMEM;
>  
> -	gc->cq_table[cq->id] = cq;
> +	/* Pairs with the acquire load in mana_gd_process_eqe(). */
> +	smp_store_release(&gc->cq_table[cq->id], cq);
>  
>  	return 0;
>  }

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch, but the
HWC teardown path doesn't get the new grace period either.
mana_hwc_destroy_channel() does:

	if (gc->max_num_cqs > 0) {
		mana_smc_teardown_hwc(&gc->shm_channel, false);
		gc->max_num_cqs = 0;
	}

Because max_num_cqs is now 0, mana_gd_destroy_cq() returns early on its
id >= gc->max_num_cqs check and skips synchronize_rcu(). Then
mana_hwc_destroy_cq() does:

	kfree(hwc_cq->comp_buf);

	if (hwc_cq->gdma_cq)
		mana_gd_destroy_queue(gc, hwc_cq->gdma_cq);

	if (hwc_cq->gdma_eq)
		mana_gd_destroy_queue(gc, hwc_cq->gdma_eq);

The EQ is deregistered and RCU-synchronized only after comp_buf and the
gdma_cq have already been freed.

An HWC EQ handler could still be running mana_hwc_comp_event() after
mana_smc_teardown_hwc() returns. That can happen with threaded or
preempted IRQ handling, or with a late HWC response during removal,
suspend or reset. Can that handler then touch the freed comp_buf through:

	completions = hwc_cq->comp_buf;

or touch the freed gdma_cq? The commit message deliberately leaves the
HWC teardown policy alone, so this only points out the remaining gap.

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 7586972e78e09..4b2c8039b8fa3 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c

[ ... ]

> @@ -2653,12 +2712,25 @@ static void mana_schedule_napi(void *context, struct gdma_queue *gdma_queue)
>  
>  static void mana_deinit_cq(struct mana_port_context *apc, struct mana_cq *cq)
>  {
> -	struct gdma_dev *gd = apc->ac->gdma_dev;
> +	struct gdma_context *gc = apc->ac->gdma_dev->gdma_context;
> +	struct gdma_queue *gdma_cq = cq->gdma_cq;
> +	struct gdma_queue *eq;
> +	int err;
>  
> -	if (!cq->gdma_cq)
> +	if (!gdma_cq)
>  		return;
>  
> -	mana_gd_destroy_queue(gd->gdma_context, cq->gdma_cq);
> +	eq = gdma_cq->cq.parent;
> +	if (gdma_cq->id < gc->max_num_cqs && eq &&
> +	    eq->id != INVALID_QUEUE_ID) {
> +		/* Flush queued events after WQ teardown, before removing this CQ. */
> +		err = mana_gd_test_eq(gc, eq);
> +		if (err && mana_en_need_log(apc, err))
> +			netdev_err(apc->ndev, "Failed to flush EQ %u for CQ %u: %d\n",
> +				   eq->id, gdma_cq->id, err);
> +	}
> +
> +	mana_gd_destroy_queue(gc, gdma_cq);
>  }

[Severity: Medium]
How much slower does teardown get with this? Every Ethernet CQ now does a
blocking mana_gd_test_eq() on its parent EQ here. It also does an
unconditional synchronize_rcu() in mana_gd_destroy_cq():

	cmpxchg(&gc->cq_table[id], queue, NULL);
	synchronize_rcu();

Both run one CQ at a time from mana_destroy_txq() and mana_destroy_rxq().
They hold RTNL and the netdev instance lock, and they run on close,
detach, reset, create unwind and queue-set free.

mana_gd_test_eq() takes gc->eq_test_event_mutex, sends
GDMA_GENERATE_TEST_EQE over the HWC, and waits up to 30 * HZ. TX CQ i and
RX CQ i share EQ i. After that, mana_destroy_eq()->mana_gd_destroy_eq()
flushes each EQ again and adds another grace period in
mana_gd_deregister_irq().

So teardown seems to go from N test-EQ round trips and N grace periods to
3N of each. synchronize_rcu() now also runs for CQs that were never
published; the old code returned early when the slot was empty.

A failed flush is only logged. Suppose an EQ stops delivering test events
while the HWC still answers. Wouldn't each CQ on that EQ wait the full 30
seconds, and then mana_gd_destroy_eq() wait again?

Would one flush per distinct EQ, plus one grace period after all slots
are cleared, be enough here? This still seems to be the case at the end
of the series.

[ ... ]

> diff --git a/include/net/mana/mana.h b/include/net/mana/mana.h
> index 1d91b62299f56..12a65145cd3fd 100644
> --- a/include/net/mana/mana.h
> +++ b/include/net/mana/mana.h
> @@ -574,7 +574,9 @@ struct mana_port_context {
>  
>  	u8 mac_addr[ETH_ALEN];
>  
> +	/* Port-owned EQ pool: max_queues slots, num_eqs populated. */
>  	struct mana_eq *eqs;
> +	unsigned int num_eqs;
>  	struct dentry *mana_eqs_debugfs;
>  
>  	enum TRI_STATE rss_state;

[Severity: Low]
This isn't a bug, but an older comment further down in struct
mana_port_context still says:

	/* Create num_queues EQs, SQs, SQ-CQs, RQs and RQ-CQs, respectively. */
	unsigned int max_queues;
	unsigned int num_queues;

mana_destroy_eq() now iterates over num_eqs, which can exceed num_queues
after mana_grow_eqs(). Should that comment be updated as well? It is
still there at the end of the series.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1791710735.git.weh%40linux.microsoft.com

[PATCH net-next v7 00/13] net: mana: reconfigure by replacing the queue set

[Wei Hu]

I am carrying Long Li's queue-set reconfiguration series forward for this
revision. Long remains the author of twelve patches, and Dipayaan Roy
remains the author of the detach change. Their authorship and signoffs
are retained; I have added my signoff for this revision.

Replace detach/attach reconfiguration with pre-allocation and queue-set
replacement. Allocation failure leaves the running configuration intact.
Publication failure attempts rollback; a failed rollback closes the port
and holds carrier down until a successful reopen.

EQs and statistics belong to the port. Valid user-configured RSS tables
survive queue rebuilds. Channel-count changes reuse surviving queues:
reductions retire the tail and increases allocate only the added queues.
Ring, MTU, private-flag and XDP layout changes still perform full rebuilds.

This series remains independent of the HWC concurrency/dynamic-depth
series; it is not stacked on its feature commits. Their CQ-publication
hunks overlap, so the later-applied series needs reconciliation preserving
both table-publication and individual-CQ lifetime guarantees.
https://lore.kernel.org/all/cover.1790665894.git.weh@linux.microsoft.com/

Changes since v6:

  - Rebase onto net-next d8674294aefef02266c4d47ad10131f1bffbe534
    after Simon Horman reported that v6 did not apply and CI did not run.
    Preserve upstream's GDMA core/PCI/CDX split, its separate PCI capability
    group and the RX-headroom-aware full-page shortcut.
  - Fix the DMA-region unwind required by repeated EQ-pool growth in
    patch 2. Destroy a registration still owned by software if EQ creation
    fails. A successful hardware creation invalidates that handle, so a
    later EQ-test failure does not destroy transferred ownership twice.
  - Make mana_detach() return void in patch 9 and remove unreachable
    caller error branches. Preserve deallocation logging and ndo_stop's
    required integer return.
  - Keep the thirteen-patch layout, original authors and existing signoffs.

Review notes:

  - The reported /proc/net/dev stats UAF assumes unregister_netdevice()
    defers its grace period. Its NULL-head path synchronously unlists and
    synchronizes network readers, removes sysfs state, then synchronizes
    again before returning, in both the old and new base.
  - The two stats-folding deadlock reports require unsupported 32-bit
    MANA. PCI and CDX both restrict support to 64-bit, where u64_stats_sync
    has no sequence counter or reader retry loop.
  - The xdp_xmit lost-increment race is real but already present on the
    unmodified base. Moving the counter to port-owned storage does not
    introduce it; keep that independent statistics fix separate.

Rebase request:
https://lore.kernel.org/all/20261010162855.GU83879@horms.kernel.org/
EQ unwind / detach reviews:
https://lore.kernel.org/all/sashiko-outbox-166658@kernel.org/
https://lore.kernel.org/all/sashiko-outbox-166664@kernel.org/
v6 review inventory:
https://sashiko.dev/#/patchset/cover.1790795005.git.weh@linux.microsoft.com

Changes since v5 (retained from v6):


  - Rebase the independent queue-set series onto net-next base
    47a1446725732cd3996edf607e8739334bbf4d78.
  - Drain ALL old TX queues after closing the TX/XDP gate and waiting for
    readers, but before publishing pointers, counts or RSS. A shared
    120-second drain deadline rejects a replacement by resuming the old
    configuration without reprogramming RSS or resetting the function.
    Retirement does not use FLR. Ordinary mana_dealloc_queues() remains
    byte-identical to this base: existing reset policy and independent
    failed-FLR hardening are deliberately not changed.
  - Complete the shared EQ/CQ lifetime prerequisite in patch 2. Initialize
    EQ state before IRQ publication. After stopping a retiring CQ's NAPI
    and work and tearing down its WQ object, flush its actual parent EQ
    while the CQ callback is still published. Clear its table entry and
    wait for RCU IRQ readers before freeing the CQ and callback owner.
    Pair IRQ lookups with release publication at all CQ publishers;
    Ethernet publication follows NAPI/DIM initialization. Matching HWC
    and RDMA publication stores do not change their recovery policies.
  - Move port-lifetime statistics preparation ahead of the first live
    swap, now patch 3. Introduce RX retirement, writer handoff and folding
    atomically with the first swap in patch 4, rather than relying on a
    later patch to make intermediate statistics readers safe.
  - Keep publication barriers consistent with open/attach. Latch forced
    carrier shutdown regardless of the previous carrier state and clear
    it only after successful reopen. Retain the rollback reader grace
    period after restoring old pointers, before callers discard scratch
    state.
  - Join TX queue selection to the publication gate in patch 4. Acquire
    port_is_up before count/RSS reads and snapshot real_num_tx_queues.
    Retiring tail RQs can outlive a TX-count reduction; when a recorded
    RX index no longer fits the live TX count, use the current RSS mapping
    instead of returning that stale index.
  - Clear DRV_XOFF on inactive allocated TX queues in patch 4's cold-path
    restart helper, using netif_tx_start_queue() while preserving the
    availability checks and wake behavior for active queues. Publication
    stops all allocated queues, and the netdev watchdog scans allocated
    num_tx_queues, not just real_num_tx_queues. Leaving the inactive tail
    stopped after a channel shrink caused a watchdog timeout and recovery
    on the previous release candidate.
  - Keep old RX statistics retired until rollback steering restoration
    succeeds. Restart DIM only for RXQs actually returning from retirement,
    with NAPI disabled, work cancelled and DIM reinitialized; carried RXQs
    and their counters are not reset.
  - Reserve live MTU/XDP replacement with channel_changing under the
    existing vport_mutex through queue, fail-close and scratch cleanup.
    This excludes concurrent RDMA RAW-QP vport admission without changing
    early validation, no-op or down-port paths.
  - Correct the full-page RX shortcut description to include all existing
    early-return conditions.

Resource and control-path costs:

Full rebuilds temporarily retain both sets of SQ/RQ/CQ objects, RX buffers
and page pools. The port-owned EQ pool is shared, not doubled. Device
resource limits can reject this temporary peak; no fixed hardware queue
threshold is assumed. Allocation failure preserves the live configuration.
The pre-publication TX drain can also reject a replacement without changing
the live pointer/count/RSS configuration.

CQ retirement adds parent-EQ marker operations and RCU reader waits on the
control path; these are not free and no latency improvement is claimed.
The IRQ lookup uses an acquire load, not a new per-packet lock. This series
does not introduce a general recovery policy for failed hardware commands.

Patch layout:
  1:     Queue-set helpers, preserving ordinary deallocation policy.
  2:     Shared EQ pool and EQ/CQ publication/retirement prerequisite.
  3:     Port-lifetime statistics preparation.
  4:     First live swap, nonreset drain, RX handoff and safe TX restart.
  5-8:   Ring, private-flag, MTU and XDP reconfiguration.
  9:     Remove the unreachable detach early return.
  10-11: Release unused EQs and preserve user RSS tables.
  12-13: Reuse queues across channel-count reductions and increases.

Overlap with separately submitted net fixes:

The XDP pre-allocation pointer fix is subsumed by patch 8's queue-set path,
which leaves the live program unchanged during allocation:
https://lore.kernel.org/all/20260904202640.3900685-1-longli@microsoft.com/

The user-RSS-table fix overlaps patch 11. Keep the three-argument
mana_rss_table_keep() and its queue-set callers in the overlapping blocks;
the RTNL RSS requirement is shared with the standalone net submission:
https://lore.kernel.org/all/20260905004401.3937066-1-longli@microsoft.com/

Neither submission is resent as a standalone patch here. These merge
notes do not prescribe changes to unrelated net-tree code.

History:
  v5: https://lore.kernel.org/all/20260909222416.884246-1-longli@microsoft.com/
  v4: https://lore.kernel.org/all/20260908032843.397667-1-longli@microsoft.com/
  v3: https://lore.kernel.org/all/20260901014442.2945689-1-longli@microsoft.com/
  v2 repost:
      https://lore.kernel.org/all/20260813050418.2906468-1-longli@microsoft.com/
  v2 earlier posting:
      https://lore.kernel.org/all/20260811063506.2428213-1-longli@microsoft.com/
  Full-page RX predecessor (v12, not a queue-set v1):
      https://lore.kernel.org/all/20260711041415.3008868-1-dipayanroy@linux.microsoft.com/

The link labelled "v1" in the old queue-set covers actually points to the
four-patch full-page RX v12 predecessor, not an earlier 13-patch queue-set
posting.

Testing:

Source/static qualification completed:
  - Baseline plus all thirteen commits: 28 fresh GCC/Clang W=1 target
    builds and thirteen strict checkpatch/diff checks.
  - Exact-source EQ-unwind regression: both compilers expose four v6
    leaks; nine cases pass at fixed patch 2 and final v7.
  - Full baseline/release kernels build with identical configurations.
    Sparse retains a baseline warning; Smatch retains baseline frontend
    failures/diagnostics and is not counted as a clean pass.

Fresh runtime results:
  - Stable baseline/release TCP and 64-byte UDP comparisons completed,
    forward/reverse with three samples per workload. Supported PD/MR
    and native PD/CQ/MR resource controls passed.
  - VM5 completed 39 named cases across documented exact-release boots
    and three original mixed-profile cycles, not a single-boot matrix.
  - VM6's earlier run completed 34 cases and failed repeated cleanup.
    A later exact-release run passed the four remaining XDP cases and
    all three original mixed-profile cycles. The earlier failure is
    retained, not relabeled as a complete matrix pass.
  - Three PCI remove/rescan cycles per VM passed with concurrent
    /proc/net/dev and four sysfs-stat readers, per-function resource
    identity/count checks and health dwells.
  - Both VMs were restored to their safe kernels and original controls,
    including RSS keys, tables and default/user policy. Owned fixtures
    and helpers were removed and reservations released.
  - All thirteen mails also applied cleanly to net-next d0bd617658dd.

A later unmodified net-next baseline also reproduced the TX-drain failure
class, with a pending send and a matching owned TX_OKAY completion.
DIM causality and a common root cause across runs remain unproven.
That baseline-present issue is recorded for separate investigation, not
silently treated as a passed test or a demonstrated v7 regression.
No speculative driver workaround has been added.

Prior v6 sanitizer/fault and CPU-accounting results are historical,
not fresh v7 qualification. No fresh 32-bit, CDX hardware, full-locking
or general firmware-failure recovery coverage is claimed.

v6:
https://lore.kernel.org/all/cover.1790795005.git.weh@linux.microsoft.com/

Signed-off-by: Wei Hu <weh@microsoft.com>

Dipayaan Roy (1):
  net: mana: do not bail out of mana_detach on dealloc failure

Long Li (12):
  net: mana: add queue-set allocation and teardown helpers
  net: mana: share the EQ pool across a queue-set swap
  net: mana: keep per-queue statistics in the port context
  net: mana: swap queue sets in mana_set_channels
  net: mana: swap queue sets in mana_set_ringparam
  net: mana: swap queue sets in mana_set_priv_flags
  net: mana: swap queue sets in mana_change_mtu
  net: mana: swap queue sets in mana_xdp_set
  net: mana: release EQs left idle by a channel-count reduction
  net: mana: keep a user-configured RSS table across a queue rebuild
  net: mana: keep the surviving queues when the channel count is reduced
  net: mana: keep the existing queues when the channel count is raised

 drivers/infiniband/hw/mana/cq.c               |    3 +-
 .../net/ethernet/microsoft/mana/gdma_main.c   |   26 +-
 .../net/ethernet/microsoft/mana/hw_channel.c  |    3 +-
 .../net/ethernet/microsoft/mana/mana_bpf.c    |  108 +-
 drivers/net/ethernet/microsoft/mana/mana_en.c | 1341 +++++++++++++++--
 .../ethernet/microsoft/mana/mana_ethtool.c    |  313 ++--
 include/net/mana/gdma.h                       |    8 +-
 include/net/mana/mana.h                       |   99 +-
 8 files changed, 1615 insertions(+), 286 deletions(-)


base-commit: d8674294aefef02266c4d47ad10131f1bffbe534
-- 
2.43.0

[PATCH net-next v7 02/13] net: mana: share the EQ pool across a queue-set swap

[Wei Hu]

From: Long Li <longli@microsoft.com>

Make the EQ pool port-owned so overlapping queue sets share EQs instead
of requiring old + new vector allocations. Allocate max_queues slots and
track populated entries with num_eqs.

Grow the pool before creating replacement CQs. Additional EQs survive
allocation failure in this patch and are released at port teardown.

Initialize EQ callbacks, context, owner phase and throttle before IRQ
publication, so a live shared IRQ cannot observe an uninitialized EQ.
Preserve the existing hardware-create and unwind ordering.

A shared EQ also outlives individual CQs. After stopping a CQ's NAPI and
work and tearing down its WQ object, flush its actual parent EQ while the
CQ callback remains published. Remove only that CQ's table entry and wait
for RCU IRQ readers before freeing the CQ and its callback owner.

Pair the IRQ lookup with release publication at all CQ publishers. For
Ethernet, publish only after NAPI and DIM initialization. The HWC and RDMA
sites need the matching stores, not changes to their teardown or recovery
policies.

The EQ markers and reader waits are control-path costs; the IRQ lookup
uses an acquire load without new validity checks. Report failed markers
using the existing error-logging convention. This does not add a reset or
reclamation policy for arbitrary failed hardware commands.

Release a newly registered DMA region if EQ construction fails before
the hardware queue takes ownership. Successful hardware creation
invalidates the software handle, so a later EQ-test failure does not
destroy a transferred region twice. This closes an existing unwind
gap required by repeated EQ-pool growth failures; retain the existing
hardware-command failure policy.

Signed-off-by: Long Li <longli@microsoft.com>
Signed-off-by: Wei Hu <weh@microsoft.com>
---
 drivers/infiniband/hw/mana/cq.c               |   3 +-
 .../net/ethernet/microsoft/mana/gdma_main.c   |  26 ++--
 .../net/ethernet/microsoft/mana/hw_channel.c  |   3 +-
 drivers/net/ethernet/microsoft/mana/mana_en.c | 115 ++++++++++++++----
 include/net/mana/mana.h                       |   6 +-
 5 files changed, 116 insertions(+), 37 deletions(-)

diff --git a/drivers/infiniband/hw/mana/cq.c b/drivers/infiniband/hw/mana/cq.c
index d4e5e3f91268..0b6ad2547e0e 100644
--- a/drivers/infiniband/hw/mana/cq.c
+++ b/drivers/infiniband/hw/mana/cq.c
@@ -155,7 +155,8 @@ int mana_ib_install_cq_cb(struct mana_ib_dev *mdev, struct mana_ib_cq *cq)
 	gdma_cq->type = GDMA_CQ;
 	gdma_cq->cq.callback = mana_ib_cq_handler;
 	gdma_cq->id = cq->queue.id;
-	gc->cq_table[cq->queue.id] = gdma_cq;
+	/* Pairs with the acquire load in mana_gd_process_eqe(). */
+	smp_store_release(&gc->cq_table[cq->queue.id], gdma_cq);
 	return 0;
 }
 
diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
index ae0ed700b3b9..078b3666947a 100644
--- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
+++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
@@ -768,7 +768,8 @@ static void mana_gd_process_eqe(struct gdma_queue *eq)
 		if (WARN_ON_ONCE(cq_id >= gc->max_num_cqs))
 			break;
 
-		cq = gc->cq_table[cq_id];
+		/* Match release publication of the CQ and its callback state. */
+		cq = smp_load_acquire(&gc->cq_table[cq_id]);
 		if (WARN_ON_ONCE(!cq || cq->type != GDMA_CQ || cq->id != cq_id))
 			break;
 
@@ -1030,17 +1031,17 @@ static int mana_gd_create_eq(struct gdma_dev *gd,
 		return -EINVAL;
 	}
 
+	queue->eq.callback = spec->eq.callback;
+	queue->eq.context = spec->eq.context;
+	queue->head |= INITIALIZED_OWNER_BIT(log2_num_entries);
+	queue->eq.log2_throttle_limit = spec->eq.log2_throttle_limit ?: 1;
+
 	err = mana_gd_register_irq(queue, spec);
 	if (err) {
 		dev_err(dev, "Failed to register irq: %d\n", err);
 		return err;
 	}
 
-	queue->eq.callback = spec->eq.callback;
-	queue->eq.context = spec->eq.context;
-	queue->head |= INITIALIZED_OWNER_BIT(log2_num_entries);
-	queue->eq.log2_throttle_limit = spec->eq.log2_throttle_limit ?: 1;
-
 	if (create_hwq) {
 		err = mana_gd_create_hw_eq(gc, queue);
 		if (err)
@@ -1075,13 +1076,12 @@ static void mana_gd_destroy_cq(struct gdma_context *gc,
 {
 	u32 id = queue->id;
 
-	if (id >= gc->max_num_cqs)
-		return;
-
-	if (!gc->cq_table[id])
+	if (id >= gc->max_num_cqs || !gc->cq_table)
 		return;
 
-	gc->cq_table[id] = NULL;
+	/* Leave a reused ID alone, but still drain readers of this CQ. */
+	cmpxchg(&gc->cq_table[id], queue, NULL);
+	synchronize_rcu();
 }
 
 int mana_gd_create_hwc_queue(struct gdma_dev *gd,
@@ -1273,8 +1273,10 @@ int mana_gd_create_mana_eq(struct gdma_dev *gd,
 	queue->gdma_dev = gd;
 
 	err = mana_gd_create_eq(gd, spec, true, queue);
-	if (err)
+	if (err) {
+		mana_gd_destroy_dma_region(gc, gmi->dma_region_handle);
 		goto out;
+	}
 
 	*queue_ptr = queue;
 	return 0;
diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index 3bca4b683134..aefb8646cd79 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -702,7 +702,8 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
 	if (!gc->cq_table)
 		return -ENOMEM;
 
-	gc->cq_table[cq->id] = cq;
+	/* Pairs with the acquire load in mana_gd_process_eqe(). */
+	smp_store_release(&gc->cq_table[cq->id], cq);
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
index 7586972e78e0..4b2c8039b8fa 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_en.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
@@ -1762,7 +1762,7 @@ void mana_destroy_eq(struct mana_port_context *apc)
 	debugfs_remove_recursive(apc->mana_eqs_debugfs);
 	apc->mana_eqs_debugfs = NULL;
 
-	for (i = 0; i < apc->num_queues; i++) {
+	for (i = 0; i < apc->num_eqs; i++) {
 		eq = apc->eqs[i].eq;
 		if (!eq)
 			continue;
@@ -1774,6 +1774,7 @@ void mana_destroy_eq(struct mana_port_context *apc)
 
 	kfree(apc->eqs);
 	apc->eqs = NULL;
+	apc->num_eqs = 0;
 }
 EXPORT_SYMBOL_NS(mana_destroy_eq, "NET_MANA");
 
@@ -1802,9 +1803,11 @@ int mana_create_eq(struct mana_port_context *apc)
 
 	if (WARN_ON(apc->eqs))
 		return -EEXIST;
-	apc->eqs = kzalloc_objs(struct mana_eq, apc->num_queues);
+	/* Keep EQ array addresses stable while CQs reference them. */
+	apc->eqs = kzalloc_objs(struct mana_eq, apc->max_queues);
 	if (!apc->eqs)
 		return -ENOMEM;
+	apc->num_eqs = 0;
 
 	spec.type = GDMA_EQ;
 	spec.monitor_avl_buf = false;
@@ -1834,6 +1837,7 @@ int mana_create_eq(struct mana_port_context *apc)
 		}
 		apc->eqs[i].eq->eq.irq = gic->irq;
 		mana_create_eq_debugfs(apc, i);
+		apc->num_eqs = i + 1;
 	}
 
 	return 0;
@@ -1843,6 +1847,61 @@ int mana_create_eq(struct mana_port_context *apc)
 }
 EXPORT_SYMBOL_NS(mana_create_eq, "NET_MANA");
 
+/* Grow the shared EQ pool without replacing live entries. */
+static int mana_grow_eqs(struct mana_port_context *apc, unsigned int need)
+{
+	struct gdma_dev *gd = apc->ac->gdma_dev;
+	struct gdma_context *gc = gd->gdma_context;
+	struct gdma_queue_spec spec = {};
+	struct gdma_irq_context *gic;
+	unsigned int i;
+	int err;
+	int msi;
+
+	if (WARN_ON(!apc->eqs))
+		return -EINVAL;
+
+	if (need > apc->max_queues)
+		return -EINVAL;
+
+	if (need <= apc->num_eqs)
+		return 0;
+
+	spec.type = GDMA_EQ;
+	spec.monitor_avl_buf = false;
+	spec.queue_size = EQ_SIZE;
+	spec.eq.callback = NULL;
+	spec.eq.context = apc->eqs;
+	spec.eq.log2_throttle_limit = LOG2_EQ_THROTTLE;
+
+	for (i = apc->num_eqs; i < need; i++) {
+		msi = (i + 1) % gc->num_msix_usable;
+
+		gic = mana_gd_get_gic(gc, !gc->msi_sharing, &msi);
+		if (IS_ERR(gic)) {
+			err = PTR_ERR(gic);
+			goto out;
+		}
+		spec.eq.msix_index = msi;
+
+		err = mana_gd_create_mana_eq(gd, &spec, &apc->eqs[i].eq);
+		if (err) {
+			dev_err(gc->dev, "Failed to grow EQ %u : %d\n", i, err);
+			mana_gd_put_gic(gc, !gc->msi_sharing, msi);
+			goto out;
+		}
+		apc->eqs[i].eq->eq.irq = gic->irq;
+		mana_create_eq_debugfs(apc, i);
+		apc->num_eqs = i + 1;
+	}
+
+	return 0;
+out:
+	/* Retain partial growth for reuse; the live set still needs this pool.
+	 */
+	return err;
+}
+
 static int mana_fence_rq(struct mana_port_context *apc, struct mana_rxq *rxq)
 {
 	struct mana_fence_rq_resp resp = {};
@@ -2653,12 +2712,25 @@ static void mana_schedule_napi(void *context, struct gdma_queue *gdma_queue)
 
 static void mana_deinit_cq(struct mana_port_context *apc, struct mana_cq *cq)
 {
-	struct gdma_dev *gd = apc->ac->gdma_dev;
+	struct gdma_context *gc = apc->ac->gdma_dev->gdma_context;
+	struct gdma_queue *gdma_cq = cq->gdma_cq;
+	struct gdma_queue *eq;
+	int err;
 
-	if (!cq->gdma_cq)
+	if (!gdma_cq)
 		return;
 
-	mana_gd_destroy_queue(gd->gdma_context, cq->gdma_cq);
+	eq = gdma_cq->cq.parent;
+	if (gdma_cq->id < gc->max_num_cqs && eq &&
+	    eq->id != INVALID_QUEUE_ID) {
+		/* Flush queued events after WQ teardown, before removing this CQ. */
+		err = mana_gd_test_eq(gc, eq);
+		if (err && mana_en_need_log(apc, err))
+			netdev_err(apc->ndev, "Failed to flush EQ %u for CQ %u: %d\n",
+				   eq->id, gdma_cq->id, err);
+	}
+
+	mana_gd_destroy_queue(gc, gdma_cq);
 }
 
 static void mana_deinit_txq(struct mana_port_context *apc, struct mana_txq *txq)
@@ -2853,8 +2925,6 @@ static int mana_create_txq(struct mana_port_context *apc,
 			goto out;
 		}
 
-		gc->cq_table[cq->gdma_id] = cq->gdma_cq;
-
 		mana_create_txq_debugfs(apc, i);
 
 		set_bit(NAPI_STATE_NO_BUSY_POLL, &cq->napi.state);
@@ -2869,6 +2939,9 @@ static int mana_create_txq(struct mana_port_context *apc,
 		napi_enable_locked(&cq->napi);
 		txq->napi_initialized = true;
 
+		/* Publish the initialized NAPI/DIM state to the EQ handler. */
+		smp_store_release(&gc->cq_table[cq->gdma_id], cq->gdma_cq);
+
 		mana_gd_ring_cq(cq->gdma_cq, SET_ARM_BIT);
 	}
 
@@ -3179,8 +3252,6 @@ static struct mana_rxq *mana_create_rxq(struct mana_port_context *apc,
 		goto out;
 	}
 
-	gc->cq_table[cq->gdma_id] = cq->gdma_cq;
-
 	netif_napi_add_weight_locked(ndev, &cq->napi, mana_poll, 1);
 
 	WARN_ON(xdp_rxq_info_reg(&rxq->xdp_rxq, ndev, rxq_idx,
@@ -3196,6 +3267,9 @@ static struct mana_rxq *mana_create_rxq(struct mana_port_context *apc,
 
 	napi_enable_locked(&cq->napi);
 
+	/* Publish the initialized NAPI/DIM state to the EQ handler. */
+	smp_store_release(&gc->cq_table[cq->gdma_id], cq->gdma_cq);
+
 	mana_gd_ring_cq(cq->gdma_cq, SET_ARM_BIT);
 out:
 	if (!err)
@@ -3799,7 +3873,6 @@ static int mana_dealloc_queues(struct net_device *ndev)
 static void mana_qset_snapshot(const struct mana_port_context *ctx,
 			       struct mana_qset *out)
 {
-	out->eqs		= ctx->eqs;
 	out->tx_qp		= ctx->tx_qp;
 	out->rxqs		= ctx->rxqs;
 	out->indir_table	= ctx->indir_table;
@@ -3816,7 +3889,6 @@ static void mana_qset_snapshot(const struct mana_port_context *ctx,
 static void mana_qset_install(struct mana_port_context *ctx,
 			      const struct mana_qset *qset)
 {
-	ctx->eqs		= qset->eqs;
 	ctx->tx_qp		= qset->tx_qp;
 	ctx->rxqs		= qset->rxqs;
 	ctx->indir_table	= qset->indir_table;
@@ -3829,7 +3901,9 @@ static void mana_qset_install(struct mana_port_context *ctx,
 	ctx->priv_flags		= qset->priv_flags;
 }
 
-/* Copy the vport identity without borrowing the live queues. */
+/* Scratch starts without SQs/RQs and borrows the port's EQ pool. Never call
+ * mana_destroy_eq() on it.
+ */
 struct mana_port_context *mana_qset_scratch_alloc(struct mana_port_context *apc)
 {
 	struct mana_port_context *scratch;
@@ -3840,13 +3914,11 @@ struct mana_port_context *mana_qset_scratch_alloc(struct mana_port_context *apc)
 
 	*scratch = *apc;
 
-	scratch->eqs		= NULL;
 	scratch->tx_qp		= NULL;
 	scratch->rxqs		= NULL;
 	scratch->indir_table	= NULL;
 	scratch->rxobj_table	= NULL;
 	scratch->default_rxobj	= INVALID_MANA_HANDLE;
-	scratch->mana_eqs_debugfs = NULL;
 
 	/* Do not consume the live set's pre-allocated RX buffers. */
 	scratch->rxbufs_pre	= NULL;
@@ -3864,7 +3936,8 @@ void mana_qset_scratch_free(struct mana_port_context *scratch)
 	kvfree(scratch);
 }
 
-int mana_alloc_qset(struct mana_port_context *scratch, unsigned int num_queues,
+int mana_alloc_qset(struct mana_port_context *apc,
+		    struct mana_port_context *scratch, unsigned int num_queues,
 		    unsigned int rx_queue_size, unsigned int tx_queue_size,
 		    u32 priv_flags, struct mana_qset *out)
 {
@@ -3886,13 +3959,16 @@ int mana_alloc_qset(struct mana_port_context *scratch, unsigned int num_queues,
 	if (err)
 		goto cleanup_rxq_array;
 
-	err = mana_create_eq(scratch);
+	err = mana_grow_eqs(apc, num_queues);
 	if (err)
 		goto cleanup_rss;
 
+	scratch->eqs = apc->eqs;
+	scratch->num_eqs = apc->num_eqs;
+
 	err = mana_create_txq(scratch, ndev);
 	if (err)
-		goto cleanup_eq;
+		goto cleanup_rss;
 
 	err = mana_add_rx_queues(scratch, ndev);
 	if (err)
@@ -3906,8 +3982,6 @@ int mana_alloc_qset(struct mana_port_context *scratch, unsigned int num_queues,
 cleanup_rxq:
 	mana_destroy_rxqs(scratch);
 	mana_destroy_txq(scratch);
-cleanup_eq:
-	mana_destroy_eq(scratch);
 cleanup_rss:
 	mana_cleanup_indir_table(scratch);
 cleanup_rxq_array:
@@ -3927,7 +4001,7 @@ void mana_free_qset(struct mana_port_context *scratch, struct mana_qset *qset)
 
 	ASSERT_RTNL();
 
-	if (!qset->rxqs && !qset->tx_qp && !qset->eqs)
+	if (!qset->rxqs && !qset->tx_qp)
 		return;
 
 	if (qset->tx_qp) {
@@ -3961,7 +4035,6 @@ void mana_free_qset(struct mana_port_context *scratch, struct mana_qset *qset)
 	mana_chn_xdp_release(retiring_prog, retiring_queues);
 
 	mana_destroy_txq(scratch);
-	mana_destroy_eq(scratch);
 	mana_cleanup_indir_table(scratch);
 	kfree(scratch->rxqs);
 	scratch->rxqs = NULL;
diff --git a/include/net/mana/mana.h b/include/net/mana/mana.h
index 1d91b62299f5..12a65145cd3f 100644
--- a/include/net/mana/mana.h
+++ b/include/net/mana/mana.h
@@ -574,7 +574,9 @@ struct mana_port_context {
 
 	u8 mac_addr[ETH_ALEN];
 
+	/* Port-owned EQ pool: max_queues slots, num_eqs populated. */
 	struct mana_eq *eqs;
+	unsigned int num_eqs;
 	struct dentry *mana_eqs_debugfs;
 
 	enum TRI_STATE rss_state;
@@ -677,7 +679,6 @@ struct mana_port_context {
 };
 
 struct mana_qset {
-	struct mana_eq		*eqs;
 	struct mana_tx_qp	**tx_qp;
 	struct mana_rxq		**rxqs;
 
@@ -705,7 +706,8 @@ int mana_detach(struct net_device *ndev, bool from_close);
 struct mana_port_context *
 mana_qset_scratch_alloc(struct mana_port_context *apc);
 void mana_qset_scratch_free(struct mana_port_context *scratch);
-int mana_alloc_qset(struct mana_port_context *scratch, unsigned int num_queues,
+int mana_alloc_qset(struct mana_port_context *apc,
+		    struct mana_port_context *scratch, unsigned int num_queues,
 		    unsigned int rx_queue_size, unsigned int tx_queue_size,
 		    u32 priv_flags, struct mana_qset *out);
 void mana_free_qset(struct mana_port_context *scratch, struct mana_qset *qset);
-- 
2.43.0

Re: [PATCH net-next v7 02/13] net: mana: share the EQ pool across a queue-set swap

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mana_ib_remove_cq_cb: Use-After-Free due to missing RCU synchronization
- [High] net: mana: Unbounded RTNL stall during queue teardown if EQ is dead
- [High] mana: NULL pointer dereference in mana_gd_process_eqe()
--

commit 5cede1970aed52e8e7cc0e961f84924a35708274
Author: Long Li <longli@microsoft.com>

net: mana: share the EQ pool across a queue-set swap

This patch makes the EQ pool port-owned so overlapping queue sets share EQs
instead of requiring new allocations. It initializes EQ callbacks and
throttles before IRQ publication, introduces a lockless RCU pattern for
CQ table publication, and modifies CQ teardown to flush events and wait
for RCU readers.

> diff --git a/drivers/infiniband/hw/mana/cq.c b/drivers/infiniband/hw/mana/cq.c
> index d4e5e3f912689..0b6ad2547e0ef 100644
> --- a/drivers/infiniband/hw/mana/cq.c
> +++ b/drivers/infiniband/hw/mana/cq.c
> @@ -155,7 +155,8 @@ int mana_ib_install_cq_cb(struct mana_ib_dev *mdev, struct mana_ib_cq *cq)
>  	gdma_cq->type = GDMA_CQ;
>  	gdma_cq->cq.callback = mana_ib_cq_handler;
>  	gdma_cq->id = cq->queue.id;
> -	gc->cq_table[cq->queue.id] = gdma_cq;
> +	/* Pairs with the acquire load in mana_gd_process_eqe(). */
> +	smp_store_release(&gc->cq_table[cq->queue.id], gdma_cq);
>  	return 0;
>  }

[Severity: High]
This patch converts the IB CQ table publication into a lockless RCU pattern,
but does the teardown path in mana_ib_remove_cq_cb() also need to be updated?

If a hardware EQ completion occurs concurrently while mana_ib_remove_cq_cb()
is tearing down the CQ, it calls kfree(gc->cq_table[cq->queue.id]) directly
without an RCU grace period. The hardirq handler (mana_gd_process_eqe)
reads this via smp_load_acquire() and dereferences the freed pointer,
leading to a use-after-free.

Should mana_ib_remove_cq_cb() use cmpxchg() and synchronize_rcu() for
teardown, similar to what was added for Ethernet CQs in
mana_gd_destroy_cq()?

[ ... ]
> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index ae0ed700b3b93..078b3666947a4 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> @@ -768,7 +768,8 @@ static void mana_gd_process_eqe(struct gdma_queue *eq)
>  		if (WARN_ON_ONCE(cq_id >= gc->max_num_cqs))
>  			break;
>  
> -		cq = gc->cq_table[cq_id];
> +		/* Match release publication of the CQ and its callback state. */
> +		cq = smp_load_acquire(&gc->cq_table[cq_id]);

[Severity: High]
Can gc->cq_table be NULL when this acquire load executes?

During channel initialization, mana_hwc_establish_channel() communicates
with the hardware. The hardware begins executing and sends
GDMA_EQE_HWC_INIT_DATA events to the EQ, including HWC_INIT_DATA_MAX_NUM_CQS,
which populates gc->max_num_cqs.

However, gc->cq_table is not allocated via vcalloc() until after the
hardware initialization completes and mana_hwc_establish_channel() resumes.
If the hardware sends a spurious or unexpected GDMA_EQE_COMPLETION event in
this window, the bounds check passes because gc->max_num_cqs is already set.
The handler then executes smp_load_acquire() before the table is actually
allocated, which would result in a NULL pointer dereference.

[ ... ]
> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 7586972e78e09..4b2c8039b8fa3 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
[ ... ]
> @@ -2653,12 +2712,25 @@ static void mana_schedule_napi(void *context, struct gdma_queue *gdma_queue)
>  
>  static void mana_deinit_cq(struct mana_port_context *apc, struct mana_cq *cq)
>  {
> -	struct gdma_dev *gd = apc->ac->gdma_dev;
> +	struct gdma_context *gc = apc->ac->gdma_dev->gdma_context;
> +	struct gdma_queue *gdma_cq = cq->gdma_cq;
> +	struct gdma_queue *eq;
> +	int err;
>  
> -	if (!cq->gdma_cq)
> +	if (!gdma_cq)
>  		return;
>  
> -	mana_gd_destroy_queue(gd->gdma_context, cq->gdma_cq);
> +	eq = gdma_cq->cq.parent;
> +	if (gdma_cq->id < gc->max_num_cqs && eq &&
> +	    eq->id != INVALID_QUEUE_ID) {
> +		/* Flush queued events after WQ teardown, before removing this CQ. */
> +		err = mana_gd_test_eq(gc, eq);
> +		if (err && mana_en_need_log(apc, err))
> +			netdev_err(apc->ndev, "Failed to flush EQ %u for CQ %u: %d\n",
> +				   eq->id, gdma_cq->id, err);
> +	}
> +
> +	mana_gd_destroy_queue(gc, gdma_cq);
>  }

[Severity: High]
Could this sequential mana_gd_test_eq() call lead to an unbounded RTNL
stall if an EQ stops delivering interrupts?

If an EQ is dead but the hardware channel remains responsive,
mana_gd_test_eq() will wait its full 30-second timeout.

Since mana_deinit_cq() is called sequentially for every single TX and RX
queue during teardown (via mana_destroy_txq() and mana_destroy_rxqs()),
a typical device with 64 queues would experience a 30-second timeout
per queue. This could result in an unrecoverable stall of over an hour
while holding the global RTNL lock, blocking all networking management.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791710735.git.weh@linux.microsoft.com?part=2

Re: [PATCH net-next v7 02/13] net: mana: share the EQ pool across a queue-set swap

[Wei Hu]

Hi,

> Should mana_ib_remove_cq_cb() use cmpxchg() and synchronize_rcu() for
> teardown, similar to what was added for Ethernet CQs in
> mana_gd_destroy_cq()?

I will leave the RDMA teardown path unchanged in v8. The callback-wrapper
lifetime issue is real, but mana_ib_remove_cq_cb() and mana_ib_destroy_cq()
already have this ordering in both unmodified upstream bases used for v6
and v7. The acquire/release publication change does not introduce that
reader/free race or resolve it. An independent RDMA lifetime fix should
cover the hardware CQ and pending events as well as the software readers;
I am not including that separate change in this queue-set revision.

> Can gc->cq_table be NULL when this acquire load executes?

I will also leave this initialization path unchanged in v8. There is a
window after max_num_cqs is populated and before cq_table is allocated.
A spurious or premature completion in that window could hit the missing
table-root check. That window and the unguarded lookup already exist in
both upstream bases, before this series.

For normal HWC startup, WQEs are posted only after channel establishment
returns and the CQ table is installed. I have not identified a normal
completion producer in the window described here. This is a conditional
hardware-protocol hardening issue, not a demonstrated new startup failure
caused by the publication change.

> Could this sequential mana_gd_test_eq() call lead to an unbounded RTNL
> stall if an EQ stops delivering interrupts?

I will address the additional per-CQ timeout amplification in v8. These
extra waits are introduced by the queue-set series and are already in v6.

The correction will stop the retiring WQs, keep their CQ callbacks alive,
and issue all required parent-EQ markers before waiting. Per-EQ
completions will allow one shared 30-second completion budget per RX or TX
cleanup batch. CQ withdrawal and the existing RCU reader wait remain
after the batch, including the partial-allocation cleanup paths.

One detail of the reported bound needs qualification: each TX/RX pair has
its own parent EQ. One stalled parent normally adds two CQ waits, about
60 seconds. The 128 * 30-second case requires all 64 parents to stall
while the hardware channel remains responsive. The cumulative delay is
finite, but it is still a real problem introduced by these extra waits.

The correction bounds the added CQ-marker completion waits, not all RTNL
hold time. Existing per-EQ destruction waits and hardware-channel request
time remain separate. It also preserves the existing best-effort cleanup
policy after a failed marker, rather than claiming new hardware recovery
guarantees.

Thanks,
Wei