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

[netdev-bot+sashiko@kernel.org]

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?

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index af2fdd5e876eb..5c5d1edd20f84 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> @@ -4594,10 +4594,8 @@ int mana_detach(struct net_device *ndev, bool from_close)
>  
>  	if (apc->port_st_save) {
>  		err = mana_dealloc_queues(ndev);
> -		if (err) {
> +		if (err)
>  			netdev_err(ndev, "%s failed to deallocate queues: %d\n", __func__, err);
> -			return err;
> -		}
>  	}
>  
>  	if (!from_close) {

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