Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] The patch correctly prevents `mana_detach()` from running on an already-detached device in the reset handler, but identical bugs remain in `mana_change_mtu()`, `mana_set_channels()`, `mana_set_ringparam()`, and `mana_bpf()`. --
commit bac73a4812067e75feeec962b27ab9647e30ed9c Author: Dipayaan Roy <[email protected]> net: mana: Skip redundant detach in queue reset handler if already detached This commit adds a check for netif_device_present() in the queue reset handler. If the device is already detached, it skips the redundant mana_detach() call and proceeds directly to mana_attach(). > diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c > b/drivers/net/ethernet/microsoft/mana/mana_en.c > index 9e7e4bf526bf6..740f3c34db451 100644 > --- a/drivers/net/ethernet/microsoft/mana/mana_en.c > +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c > @@ -316,12 +316,19 @@ static void > mana_per_port_queue_reset_work_handler(struct work_struct *work) [ ... ] > + /* If already detached (indicates detach succeeded but attach failed > + * previously). Now skip mana detach and just retry mana_attach. > + */ > + if (!netif_device_present(ndev)) > + goto attach; > + > err = mana_detach(ndev, false); The fix here correctly prevents mana_detach() from running on an already detached device. However, do identical issues remain in mana_change_mtu(), mana_set_channels(), mana_set_ringparam(), and mana_bpf()? If mana_attach() previously failed (for example, due to out of memory), the port is left with apc->port_is_up = false, but it preserves apc->port_st_save = true, and calls netif_device_detach(ndev). If an admin tries to recover the port by retrying a configuration command like changing the MTU or attaching XDP, these functions appear to unconditionally call mana_detach(ndev, false) without the !netif_device_present(ndev) check added here. Inside mana_detach(), apc->port_st_save = apc->port_is_up executes. Since port_is_up is already false, port_st_save is overwritten to false, permanently erasing the intent that the port should be up. The subsequent call to mana_attach() would then see port_st_save == false, skip mana_alloc_queues(), set port_is_up = false, and successfully call netif_device_attach(ndev). Would this result in the configuration command returning 0 to userspace while the port queues were never allocated, leaving the interface silently broken until manually toggled down and up? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
