Re: [PATCH net-next v3 01/13] net: mana: add queue-set allocation and teardown helpers

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

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

- [High] mana_drain_txqs() refuses to unmap/free pending TX SKBs when the
  fallback pci_try_reset_function() fails ("leaking pending TX buffers…
- [High] The drain's fallback reset was changed from an unconditional
  pcie_flr() to pci_try_reset_function(), which starts with…
- [Medium] The commit message claims "No functional change otherwise:
  nothing calls the new helpers yet", but the TX-drain rewrite changes…
- [Medium] After a successful function reset invalidates every port's
  queues, the sibling recovery this patch newly relies on can exit…
- [Medium] The drain fallback issues a PCI function-level reset and, on
  success, treats the function as usable: it bumps reset_gen, continues…
- [Low] include/net/mana/mana.h documents mana_context.reset_gen as
  "Bumped on every PCI function reset", and mana_drain_txqs() uses a…
- [Low] The commit message presents "That tears the vport down, so RDMA
  can claim it while released" as applying to all of the named…
- [Low] The live-path change from a bare pcie_flr() to a PCI-core-managed
  function reset is a user-visible correctness fix (the bare FLR wipes…

--

net: mana: add queue-set allocation and teardown helpers

This adds a mana_qset data model plus mana_qset_scratch_alloc(),
mana_alloc_qset() and mana_free_qset() so a queue set can be built and
retired against a scratch port context rather than by tearing the vport
down. It also moves the TX drain out of mana_dealloc_queues() into a new
mana_drain_txqs() and replaces the fallback pcie_flr() with
pci_try_reset_function().

No Link: tags are present in the commit message.

A few questions on the commit message first, then some on the diff.

The ethtool reconfiguration paths (channel count, ring size, private
flags), mana_change_mtu() and mana_xdp_set() rebuild the queues with
mana_detach() then mana_attach(). That tears the vport down, so RDMA can
claim it while released, and a failed mana_attach() leaves the port down
with no way back but manual intervention.

[Severity: Low]
This isn't a bug, but does the RDMA part of that sentence hold for all of
the listed paths?

mana_set_channels() and mana_set_priv_flags() in mana_ethtool.c already
set apc->channel_changing = true across their detach/attach window:

	/* Block RDMA from grabbing the vport during detach/attach */
	apc->channel_changing = true;

and mana_cfg_vport() returns -EBUSY while that flag is set. The RDMA
exposure argument seems to apply to mana_set_ringparam(),
mana_change_mtu() and mana_xdp_set() only, while the failed-attach
argument applies to all of them. Could the wording be narrowed?

The TX drain moves out of mana_dealloc_queues() so the new teardown path
gets it too, and its fallback reset becomes pci_try_reset_function()
rather than an open-coded pcie_flr(), which does not save and restore
config space. Trylock because this runs under RTNL while removal takes
the device lock first.

[Severity: Low]
This part reads as a correctness fix to the pre-existing fallback reset in
mana_dealloc_queues(), but the patch carries no Fixes: tag and declares no
functional change. Would it be better as a separate commit so it can be
considered for stable on its own?

No functional change otherwise: nothing calls the new helpers yet.

[Severity: Medium]
Is the "no functional change otherwise" claim accurate for the live
teardown path (mana_close()/mana_detach()/mana_remove())? Three behaviour
changes seem to ride along:

- On a failed fallback reset, mana_drain_txqs() returns before the
  skb_dequeue() loop, so pending TX SKBs and their DMA mappings are now
  deliberately leaked. The baseline code ran that loop unconditionally
  after the pcie_flr() attempt.

- On a successful reset, mana_dealloc_queues() now walks ac->ports[] and
  queues sib->queue_reset_work for every other port, so unrelated netdevs
  get their queues torn down and rebuilt.

- mana_remove() now clears ac->ports[i], which is only justified by a code
  comment.

Could these be called out in the changelog?

[ ... ]

[Severity: High]
Can this reset ever happen on the remove path? mana_gd_remove() is the PCI
.remove callback, and the driver core invokes it with device_lock held:

mana_gd_remove() -> mana_remove() -> mana_detach() -> mana_dealloc_queues()
  -> mana_drain_txqs()

and pci_try_reset_function() begins with:

	if (!pci_dev_trylock(dev))
		return -EAGAIN;

on a mutex the calling thread already owns, so it looks like it can only
return -EAGAIN there. The old pcie_flr() had no lock dependency and did
stop the device, so on unbind with un-drained TX the device now appears to
never be reset while its queues are still destroyed below. Would
pci_reset_function_locked() (or __pci_reset_function_locked()) be the right
call for a caller that is already under device_lock?

+			if (err) {
+				netdev_err(apc->ndev,
+					   "function reset failed: %d, %d pkts pending in txq %u\n",
+					   err,
+					   atomic_read(&txq->pending_sends),
+					   txq->gdma_txq_id);
+				quiesced = false;
+			} else {
+				/* Every queue on the function is dead now,
+				 * including the ones this loop has not reached
+				 * and those of the other ports.
+				 */
+				WRITE_ONCE(apc->ac->reset_gen,
+					   apc->ac->reset_gen + 1);
+
+				/* Only a reset that actually happened takes the
+				 * other ports down with it; reporting a failed
+				 * one would rebuild them for nothing.
+				 */
+				reset = true;
+			}
+			break;
+		}
+	}
+
+	/* Only a reset that actually happened makes freeing these safe; without
+	 * one the device still owns them. Leak instead, bounded at one SQ ring
+	 * of skbs per queue.
+	 */
+	if (!quiesced) {
+		netdev_err(apc->ndev,
+			   "device not quiesced, leaking pending TX buffers instead of unmapping memory it can still DMA from\n");
+		return reset;
+	}

[Severity: High]
Does this leak-instead-of-unmap decision reach the callers?

The return value is reset, which is false both for a healthy drain and for
a failed reset, so neither mana_dealloc_queues() nor mana_free_qset() can
tell the two apart. Both then run unconditionally:

	mana_fence_rqs(apc);
	mana_destroy_rxqs(apc);
	mana_destroy_txq(apc);
	mana_destroy_eq(apc);

and mana_gd_destroy_queue() in gdma_main.c releases the SQ/CQ/EQ ring
memory regardless of whether the firmware-side destroy succeeded:

	mana_gd_destroy_dma_region(gc, gmi->dma_region_handle);
	mana_gd_free_memory(gmi);
	kfree(queue);

while mana_destroy_rxq() unmaps and frees the RX pages. So the pending SKB
mappings are kept, but the descriptor rings and RX buffers of the same
un-quiesced device go back to the page allocator. Can the device DMA into
that memory after it is reused?

+
+	for (i = 0; i < apc->num_queues; i++) {
+		if (!apc->tx_qp[i])
+			continue;
+
+		txq = &apc->tx_qp[i]->txq;
+		while ((skb = skb_dequeue(&txq->pending_skbs))) {
+			mana_unmap_skb(skb, apc);
+			dev_kfree_skb_any(skb);
+		}
+		atomic_set(&txq->pending_sends, 0);
+	}
+
+	return reset;
+}

[ ... ]

[ ... ]

+		for (i = 0; i < ac->num_ports; i++) {
+			struct mana_port_context *sib;
 
-		for (i = 0; i < apc->num_queues; i++) {
-			txq = &apc->tx_qp[i]->txq;
-			while ((skb = skb_dequeue(&txq->pending_skbs))) {
-				mana_unmap_skb(skb, apc);
-				dev_kfree_skb_any(skb);
-			}
-			atomic_set(&txq->pending_sends, 0);
+			if (!ac->ports[i] || ac->ports[i] == ndev)
+				continue;
+			sib = netdev_priv(ac->ports[i]);
+			netdev_err(ac->ports[i],
+				   "queues reset by a sibling port, scheduling rebuild\n");
+			queue_work(ac->per_port_queue_reset_wq,
+				   &sib->queue_reset_work);
 		}
 	}

[Severity: Medium]
Can this rebuild silently not happen? mana_per_port_queue_reset_work_handler()
gives up before detach/attach when the RX pre-allocation fails:

	err = mana_pre_alloc_rxbufs(apc, ndev->mtu, apc->num_queues);
	if (err) {
		netdev_err(ndev, "Insufficient memory for reset post tx stall detection\n");
		goto clear_flag;
	}

There is no retry and no state change on that branch, so under memory
pressure the sibling netdev stays with port_is_up true and
apc->tx_qp[]/apc->rxqs[] pointing at queues the function reset destroyed,
until someone does a manual ifdown/ifup. Should this branch requeue the
work or mark the port down?

[ ... ]

+void mana_free_qset(struct mana_port_context *scratch, struct mana_qset *qset)
+{

[ ... ]

+	if (mana_drain_txqs(scratch)) {
+		/* The drain had to reset the function to stop the device
+		 * touching those buffers. A function reset takes down every
+		 * port on the adapter, not just this one, so rebuild them all
+		 * - the same recovery mana_tx_timeout() relies on. A port that
+		 * is already down has nothing to rebuild and its handler
+		 * leaves it down.
+		 */

[Severity: Medium]
After the function reset, what re-establishes the GDMA/HWC state that the
scheduled rebuilds need?

The driver registers no pci_error_handlers, so there is no reset_prepare or
reset_done callback, and pci_try_reset_function() only saves and restores
config space. The only path that re-bootstraps the device is:

mana_serv_reset() -> mana_gd_suspend()/mana_gd_resume()
  -> mana_gd_cleanup_device()/mana_gd_setup()

and that is not invoked here. Each queued queue_reset_work then re-enters
mana_attach() and issues HWC requests over a channel the reset destroyed,
while gc->cq_table[], the EQ/MSI-X bookkeeping and the cached vport and
queue handles still describe objects that no longer exist. The mana_ib
auxiliary device on the same function is also not told.

Is the comment's "the same recovery mana_tx_timeout() relies on" accurate?
That path never resets the function, so its HWC is still alive when the
rebuild runs.

+		struct mana_port_context *apc = netdev_priv(scratch->ndev);
+		struct mana_context *ac = apc->ac;
+		struct mana_port_context *sib;
+		unsigned int i;
+
+		netdev_err(scratch->ndev,
+			   "device reset while retiring a queue set, scheduling port reset\n");
+
+		for (i = 0; i < ac->num_ports; i++) {
+			if (!ac->ports[i])
+				continue;
+			sib = netdev_priv(ac->ports[i]);
+			queue_work(ac->per_port_queue_reset_wq,
+				   &sib->queue_reset_work);
+		}
+	}

[ ... ]

[ ... ]

[Severity: Low]
Is "bumped on every PCI function reset" true? The only writer is
mana_drain_txqs(), right after its own successful pci_try_reset_function().

Resets initiated elsewhere leave it unchanged, for example:

  echo 1 > /sys/bus/pci/devices/<bdf>/reset

as well as secondary-bus/slot resets, EEH/AER recovery, and the driver's
own mana_serv_reset() -> mana_gd_suspend()/mana_gd_resume() cycle. There
is no reset_done callback anywhere under drivers/net/ethernet/microsoft/mana/
that would observe them, so mana_drain_txqs() still burns the full 120s
under RTNL and then resets the function again.

The effect is conservative rather than unsafe, so would either adding a
reset_done handler or narrowing the comment to resets issued by
mana_drain_txqs() be preferable?

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