Re: [PATCH net-next v3 10/13] net: mana: release EQs left idle by a channel-count reduction

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

Critical: 0 · High: 0 · Medium: 0 · Low: 3

- [Low] Three comments (and the commit message) state a shrink
  precondition that this patch itself violates, and one pre-existing…
- [Low] mana_free_qset() gains a leading `struct mana_port_context *apc`
  parameter (exported prototype in include/net/mana/mana.h plus eight…
- [Low] Concern (assessed as a false positive): the commit lacks a Fixes:
  trailer for the commit that made the grow-only EQ pool reachable.

--

net: mana: release EQs left idle by a channel-count reduction

The shared EQ pool previously only grew, so a port kept the high-water mark
of every channel count it had ever used. This adds mana_shrink_eqs() to
release the EQs above the live queue count once a retiring set is gone, and
fixes mana_create_eq_debugfs() to record the dentry in apc->eqs[i] so a
single EQ's debugfs directory can be removed.

[ ... ]

[Severity: Low]

This comment says a shrink is only safe after mana_free_qset(), and the
commit message says the same:

  "Release the EQs above the live queue count once a retiring set has been
  torn down. That is the only safe point"

The same patch also calls mana_shrink_eqs() from mana_alloc_qset()'s
out_err path, which is not after mana_free_qset().  Can the two be
reconciled?

The rule that appears to hold at both sites is "no CQ of any set
references eqs[keep..num_eqs-1]": mana_alloc_qset()'s out_err is reached
either before any queue was built, or after mana_destroy_rxqs(scratch) and
mana_destroy_txq(scratch) destroyed the CQs that attempt created.  Would
stating that condition instead be more accurate, here and in
mana_free_qset()'s "Safe here and only here" comment below?

Separately, the error exit of mana_grow_eqs() just above this new helper
still reads:

	out:
		/* Keep whatever was created: the running queue set still needs its
		 * own EQs, and the extras are reused by the next attempt.
		 */
		return err;

Since its only caller, mana_alloc_qset(), now runs
mana_shrink_eqs(apc, apc->num_queues) on that same error return, is
anything still kept for the next attempt?

+static void mana_shrink_eqs(struct mana_port_context *apc, unsigned int keep)
+{
+	struct gdma_context *gc = apc->ac->gdma_dev->gdma_context;
+	struct gdma_queue *eq;
+	unsigned int msi;
+	unsigned int i;
+
+	if (!apc->eqs || keep >= apc->num_eqs)
+		return;
+
+	for (i = keep; i < apc->num_eqs; i++) {
+		eq = apc->eqs[i].eq;
+		if (!eq)
+			continue;
+
+		debugfs_remove_recursive(apc->eqs[i].mana_eq_debugfs);
+		apc->eqs[i].mana_eq_debugfs = NULL;
+
+		msi = eq->eq.msix_index;
+		mana_gd_destroy_queue(gc, eq);
+		mana_gd_put_gic(gc, !gc->msi_sharing, msi);
+		apc->eqs[i].eq = NULL;
+	}
+
+	apc->num_eqs = keep;
+}
+
 static int mana_fence_rq(struct mana_port_context *apc, struct mana_rxq *rxq)
 {
 	struct mana_fence_rq_resp resp = {};

[Severity: Low]

Does the new apc parameter carry anything the function cannot already
derive?  mana_qset_scratch_alloc() does *scratch = *apc, so
netdev_priv(scratch->ndev) == apc for every one of the eight call sites,
which is exactly what this patch relies on when it replaces
mana_qset_debugfs_publish(netdev_priv(scratch->ndev)) with
mana_qset_debugfs_publish(apc).

The TX-drain-failure block inside the same function still has:

	if (mana_drain_txqs(scratch)) {
		...
		struct mana_port_context *apc = netdev_priv(scratch->ndev);

so that local now shadows the new parameter, which -Wshadow / W=2 builds
will flag.

Nothing verifies that a caller passes a matching (apc, scratch) pair, and a
mismatched pair would let the mana_shrink_eqs() call below destroy another
port's EQs.  Would dropping either the parameter or the inner declaration
be preferable?

The exported prototype in include/net/mana/mana.h changes with it - is that
signature change worth a line in the commit message?

Both observations above are documentation and readability points; I did not
find a path where either shrink call site touches an EQ that a live CQ
still references.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260901014442.2945689-1-longli%40microsoft.com