On Wed, Jul 08, 2026 at 04:57:42PM +0100, Simon Horman 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.
> Full review at: https://netdev-ai.bots.linux.dev/sashiko/
> ---
> net: mana: force full-page RX buffers via ethtool private flag
>
> This adds an ethtool private flag "full-page-rx" that forces one RX buffer
> per page to avoid a page_pool fragment path throughput regression on some
> 4K PAGE_SIZE ARM64 platforms. Toggling the flag while the port is up
> triggers a mana_detach() + mana_attach() reconfiguration to apply the new
> buffer layout.
>
> A few questions inline.
>
> > diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c
> > b/drivers/net/ethernet/microsoft/mana/mana_en.c
> > index 7438ea6b3f26..38f50b4eef87 100644
> > --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> > +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
>
> [ ... mana_use_single_rxbuf_per_page() and mana_get_rxbuf_cfg() hunks
> unrelated to the comments below ... ]
>
> > diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> > b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> > index 3531abfdaf43..24f6ce63b031 100644
> > --- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> > +++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
>
> [ ... ]
>
> > @@ -611,6 +630,88 @@ static int mana_get_link_ksettings(struct net_device
> > *ndev,
> > return 0;
> > }
> >
> > +static u32 mana_get_priv_flags(struct net_device *ndev)
> > +{
> > + struct mana_port_context *apc = netdev_priv(ndev);
> > +
> > + return apc->priv_flags;
> > +}
> > +
> > +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;
>
> [Low]
> Is the storage of priv_flags intentionally nested inside the per-flag
> branch? With only one flag today this is fine, but when a second bit is
> added to enum mana_priv_flag_bits, a user toggling only the new bit
> passes the !changed and unknown-bits checks, the outer if is skipped,
> apc->priv_flags is never updated, and mana_set_priv_flags() returns 0
> while mana_get_priv_flags() keeps reporting the old state.
>
> Would it be cleaner to store priv_flags unconditionally after validation
> and use per-flag if (changed & BIT(...)) blocks only for the
> flag-specific reconfiguration?
>
Hi Simon, Paolo,
Agreed on this future concern,
but also we donot plan to have another mana driver private flag.
> > +
> > + if (!apc->port_is_up) {
> > + /* Port is down, flag updated to apply on next up
> > + * so just return.
> > + */
> > + return 0;
> > + }
> > +
> > + /* Pre-allocate buffers to prevent failure in mana_attach
> > + * later
> > + */
> > + err = mana_pre_alloc_rxbufs(apc, ndev->mtu, apc->num_queues);
>
> [Low]
> When the MTU is already jumbo (mtu + MANA_RXBUF_PAD > PAGE_SIZE / 2) or
> XDP is attached, mana_use_single_rxbuf_per_page() already returns true
> regardless of the new priv flag:
>
> if (apc->priv_flags & BIT(MANA_PRIV_FLAG_USE_FULL_PAGE_RXBUF))
> return true;
>
> /* For xdp and jumbo frames make sure only one packet fits per page. */
> if (mtu + MANA_RXBUF_PAD > PAGE_SIZE / 2 || mana_xdp_get(apc))
> return true;
>
> In that case mana_get_rxbuf_cfg() produces identical output whether the
> new flag is on or off, yet toggling the flag here always performs a full
> mana_pre_alloc_rxbufs() + mana_detach() + mana_attach() cycle and flaps
> the link.
>
> Would a pre/post comparison of mana_get_rxbuf_cfg() output be worth
> adding to skip the reconfiguration when nothing actually changes?
Good point will add a pre and post comparison as well to skip the
detach/attach when already in that configuration.
>
> [High]
> mana_set_priv_flags() drives the same detach/attach sequence used by
> mana_set_channels() and mana_per_port_queue_reset_work_handler(), but
> does not take apc->vport_mutex and set apc->channel_changing = true
> across the window.
>
> The struct comment on channel_changing spells out the invariant:
>
> /* Set by mana_set_channels() under vport_mutex to block RDMA
> * from grabbing the vport during the detach/attach window.
> * Checked by mana_cfg_vport() when called from the RDMA path.
> */
> bool channel_changing;
>
> Without it, after mana_detach() uncfg's the vport an RDMA client can
> call mana_cfg_vport() and bump vport_use_count, and then mana_attach()
> will fail with -EBUSY when it re-cfg's the vport.
>
> There is also no equivalent of the '!port_is_up && vport_use_count'
> early -EBUSY check that mana_set_channels() performs, so is the flag
> allowed to be changed silently while an RDMA client is using the vport?
>
Agreed. Will add the same vport_mutex + channel_changing pattern used by
mana_set_channels() and mana_per_port_queue_reset_work_handler() to
block RDMA from grabbing the vport during the detach/attach window.
> > + if (err) {
> > + netdev_err(ndev,
> > + "Insufficient memory for new allocations\n");
> > + apc->priv_flags = old_priv_flags;
> > + return err;
> > + }
> > +
> > + err = mana_detach(ndev, false);
>
> [High]
> mana_detach() begins with ASSERT_RTNL(), and so does mana_attach()
> called below. Is RTNL guaranteed to be held on the SET priv-flags path?
>
> mana registers net_shaper_ops in mana_devops, which makes it an
> ops-locked driver, so netdev_need_ops_lock() is true and the ethtool
> core acquires rtnl only when the driver opts in via
> ETHTOOL_OP_NEEDS_RTNL_* for that specific command.
>
> Looking at mana_ethtool_ops.op_needs_rtnl:
>
> .op_needs_rtnl = ETHTOOL_OP_NEEDS_RTNL_SCHANNELS |
> ETHTOOL_OP_NEEDS_RTNL_SRINGPARAM |
> ETHTOOL_OP_NEEDS_RTNL_GLINK,
>
> ETHTOOL_OP_NEEDS_RTNL_SPFLAGS is not listed, so ASSERT_RTNL() in the
> detach/attach helpers below will WARN, and the code races with
> RTNL-serialized mutators (mana_open, mana_close, mana_change_mtu,
> mana_tx_timeout) that read and write apc->port_is_up, apc->rxqs,
> apc->tx_qp and apc->port_st_save.
>
> Should ETHTOOL_OP_NEEDS_RTNL_SPFLAGS be added to op_needs_rtnl, matching
> what is already done for SET channels?
>
Agreed, Will add ETHTOOL_OP_NEEDS_RTNL_SPFLAGS to .op_needs_rtnl.
> > + if (err) {
> > + netdev_err(ndev, "mana_detach failed: %d\n", err);
> > + apc->priv_flags = old_priv_flags;
> > +
> > + /* Port is in an inconsistent state. Restore
> > + * 'port_is_up' so that queue reset work handler
> > + * can properly detach and re-attach.
> > + */
> > + apc->port_is_up = true;
> > + schedule_port_reset = true;
> > + goto out;
> > + }
> > +
> > + 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;
> > + schedule_port_reset = true;
>
> [High]
> On this branch, mana_detach() has already succeeded and run
> mana_cleanup_port_context(), so apc->rxqs is NULL and queues are torn
> down. Setting apc->port_is_up = true here between now and when
> queue_reset_work runs opens a window where callers gate on port_is_up
> as their "safe to access rxqs/tx_qp" predicate.
>
> For example, mana_get_ethtool_stats() does:
>
> if (!apc->port_is_up)
> return;
> ...
> rxq = apc->rxqs[q];
> ...
>
> A concurrent ethtool -S invocation during that window will pass the
> port_is_up gate and dereference apc->rxqs[q]->stats on a NULL rxqs.
>
> Is the port_is_up restore actually needed for the reset work handler's
> mana_detach() call? On the reset path, mana_detach() takes its early
> return when !netif_device_present(ndev):
>
> if (!from_close && !netif_device_present(ndev))
> return 0;
>
> That early return does not touch apc->port_st_save, so the saved state
> from the earlier successful mana_detach() should already be intact for
> the follow-up mana_attach().
>
> There is also no smp_wmb() paired with this write, unlike the pattern
> used inside mana_detach()/mana_attach() proper.
>
For the mana_attach() failure path: agreed, will remove port_is_up =
true. After a successful mana_detach(ndev, false), netif_device_detach()
has already been called so the reset handler's mana_detach() takes the
early return via !netif_device_present(), preserving port_st_save for
the subsequent mana_attach().
For the mana_detach() failure path: keeping port_is_up = true is
intentional. Here netif_device_detach() was never reached, so the reset
handler's mana_detach() would re-enter fully and overwrite port_st_save
from port_is_up, if left as false, mana_attach() would skip queue
allocation and leave the port dead.
> > + }
> > + }
> > +
> > +out:
> > + mana_pre_dealloc_rxbufs(apc);
> > +
> > + if (schedule_port_reset)
> > + queue_work(apc->ac->per_port_queue_reset_wq,
> > + &apc->queue_reset_work);
> > +
> > + return err;
> > +}
> > +
>
> [ ... remaining hunks unrelated to the comments above ... ]
Thanks for the review comments, I will address them in the next
version.
Regards
Dipayaan Roy
