On Tue, May 19, 2026 at 03:55:59PM -0700, Jakub Kicinski wrote: > This is an AI-generated review of your patch. The human sending this > email has considered the AI review valid, or at least plausible. > --- > 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?
Hi Jakub, I agree that mana_change_mtu() and mana_xdp_set() would also need the same netif_device_present() guard (or alternatively, an early return when the port is already detached). I'll investigate and address those in a follow-up patch to keep this fix minimal, as this patch specifically fixes the reset handler that runs without user intervention, unlike the ohers cases mentioned above. Regards Dipayaan Roy
