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
