Re: [PATCH net-next v3 08/13] net: mana: do not bail out of mana_detach on dealloc failure

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

Critical: 0 · High: 0 · Medium: 0 · Low: 1

- [Low] Commit-message framing concern: the reviewed patch removes the
  `return err` after `mana_dealloc_queues()` in `mana_detach()`…

--

net: mana: do not bail out of mana_detach on dealloc failure

This removes the early return that mana_detach() took when
mana_dealloc_queues() returned an error, so that netif_device_detach()
and mana_cleanup_port_context() always run. The error is now only
logged.

mana_detach() sets port_is_up = false before calling
mana_dealloc_queues(). If that call were to fail and return early,
netif_device_detach() and mana_cleanup_port_context() are skipped,
leaving the port in an inconsistent state where port_is_up is false but
netif_device_present() still returns true.

With the ethtool, MTU and XDP paths converted to pre-allocate and swap,
the callers that reach this code with from_close == false are the TX
timeout reset handler and the suspend path. For the reset handler the
inconsistent state is fatal: its mana_detach() would overwrite
port_st_save with false, so the following mana_attach() skips queue
allocation and leaves the port permanently dead. For suspend it leaves
the port context allocated across the transition.

[Severity: Low]
This isn't a bug in the code, but could the description of the reset
handler case be toned down, since the "permanently dead" port does not
look reachable?

mana_dealloc_queues() has exactly one non-zero return:

drivers/net/ethernet/microsoft/mana/mana_en.c:mana_dealloc_queues() {
	if (apc->port_is_up)
		return -EINVAL;
	...
}

and mana_detach() clears that flag two statements before the call:

drivers/net/ethernet/microsoft/mana/mana_en.c:mana_detach() {
	apc->port_st_save = apc->port_is_up;
	apc->port_is_up = false;
	...
	if (apc->port_st_save) {
		err = mana_dealloc_queues(ndev);
	...
}

Every other error inside mana_dealloc_queues(), such as the
mana_disable_vport_rx() failure, is only logged and the function ends with
an unconditional return 0.

The last paragraph does say "in practice cannot fail here since port_is_up
is already false", and the text uses "were to fail" / "would overwrite",
so the conditional framing is already there. Would it read better to state
up front that this is a robustness cleanup for an unreachable branch,
rather than describing a fatal reset-handler outcome first?

The code change itself looks fine; with port_is_up already false the
deleted return was dead code, so no live caller changes behaviour here.
Consistent with that, there is no Fixes: tag on the patch.

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