Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Forcing `apc->port_is_up = true` on the `mana_attach` error path causes a double-destroy of the hardware vport when the interface is later brought down. - [Medium] Latent bug: Changes to future private flags will be silently ignored if MANA_PRIV_FLAG_USE_FULL_PAGE_RXBUF is not simultaneously toggled. --
commit d7983f6668f209a0b7bfb427c64d5517b326c7b1 Author: Dipayaan Roy <[email protected]> net: mana: force full-page RX buffers via ethtool private flag Adds an ethtool private flag full-page-rx to bypass page_pool fragment allocation and force one RX buffer per page. Includes logic in mana_set_priv_flags to dynamically reconfigure the port on flag change. > diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c > b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c > --- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c > +++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c [ ... ] > +static int mana_set_priv_flags(struct net_device *ndev, u32 priv_flags) > +{ > + struct mana_port_context *apc = netdev_priv(ndev); > + u32 changed = apc->priv_flags ^ priv_flags; > + u32 old_priv_flags = apc->priv_flags; > + bool schedule_port_reset = false; > + int err = 0; > + > + if (!changed) > + return 0; > + > + /* Reject unknown bits */ > + if (priv_flags & ~GENMASK(MANA_PRIV_FLAG_MAX - 1, 0)) > + return -EINVAL; > + > + if (changed & BIT(MANA_PRIV_FLAG_USE_FULL_PAGE_RXBUF)) { > + apc->priv_flags = priv_flags; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: Medium] When additional private flags are added in the future, will changes to those new flags be silently ignored if MANA_PRIV_FLAG_USE_FULL_PAGE_RXBUF isn't also toggled? Since this assignment is nested inside the FULL_PAGE_RXBUF check, changing only a future new flag will bypass this assignment and the function will return success while discarding the new flag state. Should this assignment be moved outside the flag-specific conditional block? > + > + if (!apc->port_is_up) { [ ... ] > + err = mana_attach(ndev); > + if (err) { > + netdev_err(ndev, "mana_attach failed: %d\n", err); > + apc->priv_flags = old_priv_flags; > + > + /* Restore 'port_is_up' so the reset work handler > + * can properly detach/attach. Without this, > + * the handler sees port_is_up=false and skips > + * queue allocation, leaving the port dead. > + */ > + apc->port_is_up = true; ^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] Does forcing port_is_up to true here cause a double-destroy of the hardware vport when the interface is later brought down? If mana_attach fails, it cleans up any partial state by destroying the vport. Leaving the port artificially marked as up means a subsequent interface down event or reset worker will see apc->port_is_up as true and call mana_detach with from_close set to true. This bypasses the netif_device_present check and triggers a second call to mana_destroy_vport, which could drop apc->vport_use_count below zero and send a duplicate deregister message to the firmware for the already-freed hw_vport_handle. > + schedule_port_reset = true; > + } > + } > + > +out: > + mana_pre_dealloc_rxbufs(apc); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
