* Re: [PATCH net] bnge: add missing ndo_set_features
2026-08-24 8:50 [PATCH net] bnge: add missing ndo_set_features Vikas Gupta
@ 2026-08-26 12:30 ` Simon Horman
0 siblings, 0 replies; 2+ messages in thread
From: Simon Horman @ 2026-08-26 12:30 UTC (permalink / raw)
To: vikas.gupta
Cc: Simon Horman, davem, edumazet, kuba, pabeni, andrew+netdev,
netdev, linux-kernel, bhargava.marreddy, rahul-rg.gupta,
vsrama-krishna.nemani, rajashekar.hudumula, dharmender.garg,
ajit.khaparde
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/#/patchset/20260824085028.1174150-1-vikas.gupta@broadcom.com?part=1
---
bnge: add missing ndo_set_features
This adds a bnge_set_features() callback so that toggling LRO/GRO_HW via
ethtool -K actually reprograms the TPA engine. On a running interface it
recomputes bn->priv_flags, then tears down and re-opens the datapath with
bnge_close_core() followed by bnge_set_ring_params() and bnge_open_core().
A few questions about the reconfiguration sequence below.
> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> index a4288f0258f87..0973cc25a3ec4 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> @@ -3267,6 +3267,33 @@ static const struct netdev_stat_ops bnge_stat_ops = {
> .get_base_stats = bnge_get_base_stats,
> };
>
> +static int bnge_set_features(struct net_device *dev, netdev_features_t features)
> +{
> + struct bnge_net *bn = netdev_priv(dev);
> + struct bnge_dev *bd = bn->bd;
> + u32 flags = bn->priv_flags;
> +
> + flags &= ~BNGE_NET_EN_TPA;
> + if (features & NETIF_F_GRO_HW)
> + flags |= BNGE_NET_EN_GRO;
> + else if (features & NETIF_F_LRO)
> + flags |= BNGE_NET_EN_LRO;
> +
> + if (flags == bn->priv_flags)
> + return 0;
> +
> + bn->priv_flags = flags;
^^^^^^^^^^^^^^^^^^^^^
[Severity: Medium]
Is it safe to publish the new TPA mode here, before the datapath has been
quiesced? At this point NAPI is still running; bnge_disable_napi() is only
reached later, inside bnge_close_core().
The RX completion path reads the same word from softirq context with no
lock:
drivers/net/ethernet/broadcom/bnge/bnge_txrx.c:bnge_tpa_end() {
...
#ifdef CONFIG_INET
if (bn->priv_flags & BNGE_NET_EN_GRO)
skb = bnge_gro_skb(bn, tpa_info, tpa_end, tpa_end1, skb);
#endif
...
}
So an aggregation that was started (TPA_START) while the VNIC was still
programmed for the old mode can be completed using the new flag value, and
bnge_gro_func() will then derive skb_set_network_header()/
skb_set_transport_header() offsets from TPA_START metadata that was produced
under the previous configuration.
bnxt_reinit_features() orders this the other way round - it closes first and
publishes afterwards:
drivers/net/ethernet/broadcom/bnxt/bnxt.c:bnxt_reinit_features() {
bnxt_close_nic(bp, irq_re_init, link_re_init);
bp->flags = flags;
if (update_tpa)
bnxt_set_ring_params(bp);
return bnxt_open_nic(bp, irq_re_init, link_re_init);
}
Would moving the bn->priv_flags store below bnge_close_core() be preferable?
> +
> + if (!netif_running(dev)) {
> + bnge_set_ring_params(bd);
> + return 0;
> + }
> +
> + bnge_close_core(bn);
> + bnge_set_ring_params(bd);
> + return bnge_open_core(bn);
^^^^^^^^^^^^^^^^^^^^^^^
[Severity: High]
Can a failing bnge_open_core() here leave the netdev running over a freed
datapath?
Every error exit in bnge_open_core() frees the core and tells firmware the
interface went down:
drivers/net/ethernet/broadcom/bnge/bnge_netdev.c:bnge_open_core() {
...
err_free_irq:
bnge_free_irq(bn);
err_del_napi:
bnge_del_napi(bn);
bnge_free_core(bn);
err_if_change:
bnge_hwrm_if_change(bd, false);
return rc;
}
bnge_free_core() NULLs bn->vnic_info (via bnge_free_vnics()), bn->grp_info,
bn->tx_ring, bn->rx_ring and bn->bnapi.
The core does not close the device when ndo_set_features returns an error:
net/core/dev.c:__netdev_update_features() {
...
if (unlikely(err < 0)) {
netdev_err(dev,
"set_features() failed (%d); wanted %pNF, left %pNF\n",
err, &features, &dev->features);
return -1;
}
...
}
So netif_running() stays true, and the next teardown re-enters
bnge_close_core() on that freed state. bnge_close_core() has no guard, and
bnge_shutdown_nic()->bnge_hwrm_resource_free()->bnge_clear_vnic()->
bnge_hwrm_clear_vnic_filter() starts with:
struct bnge_vnic_info *vnic = &bn->vnic_info[BNGE_VNIC_DEFAULT];
int i;
for (i = 0; i < vnic->uc_filter_count; i++) {
which reads through a NULL bn->vnic_info. bnge_save_ring_stats()
(bn->bnapi[i]), bnge_del_napi() (bn->bnapi[i]) and bnge_free_tx_skbs()
(&bn->tx_ring[i], then txr->tx_buf_ring, unlike
bnge_free_rx_ring_pair_bufs()/bnge_tx_disable() which do test for NULL) are
on the same path.
Two easy second-teardown triggers: ip link set ethX down -> bnge_close(),
which calls bnge_close_core() unconditionally, and simply repeating
ethtool -K, since bnge_set_features() only gates on netif_running().
bnxt_open_nic() forces the device closed in this situation:
drivers/net/ethernet/broadcom/bnxt/bnxt.c:bnxt_open_nic() {
...
if (rc) {
netdev_err(bp->dev, "nic open fail (rc: %x)\n", rc);
netif_close(bp->dev);
}
...
}
Should bnge_set_features() do the same on a failed re-open?
[Severity: Medium]
Related to the above: on that failure bn->priv_flags keeps the new value and
bnge_set_ring_params() has already recomputed rx_agg_ring_size /
rx_buf_size / cp_ring_size for it, while __netdev_update_features() leaves
dev->features at the old value.
If the user then repeats the same ethtool -K command, the recomputed flags
now equal bn->priv_flags, so:
if (flags == bn->priv_flags)
return 0;
returns success without touching hardware, and the core commits
dev->features. Isn't that the same "dev->features no longer reflects the
actual hardware state" mismatch the commit message sets out to fix?
bnxt_set_features() restores the private flag word when the hardware step
fails:
drivers/net/ethernet/broadcom/bnxt/bnxt.c:bnxt_set_features() {
...
rc = bnxt_set_tpa(bp, (flags & BNXT_FLAG_TPA) ? true : false);
if (rc)
bp->flags = old_flags;
...
}
[Severity: Medium]
Does this pairing produce an unbalanced FUNC_DRV_IF_CHANGE handshake?
bnge_open_core() starts with bnge_hwrm_if_change(bd, true), but the matching
DOWN notification lives only in ndo_stop:
drivers/net/ethernet/broadcom/bnge/bnge_netdev.c:bnge_close() {
bnge_close_core(bn);
bnge_hwrm_shutdown_link(bn->bd);
bnge_hwrm_if_change(bn->bd, false);
}
bnge_close_core() does not issue it, so every ethtool -K toggle on a running
interface sends a second UP with no intervening DOWN.
bnge_hwrm_if_change() also discards the response:
if (up)
req->flags = cpu_to_le32(FUNC_DRV_IF_CHANGE_REQ_FLAGS_UP);
return bnge_hwrm_req_send(bd, req);
so any resource-change or hot-FW-reset indication firmware returns on that
extra UP is lost, and bnge_reserve_rings() runs immediately afterwards on
the driver's existing reservation bookkeeping.
bnxt keeps the handshake at the ndo_open/ndo_stop level only -
bnxt_reinit_features() calls bnxt_close_nic()/bnxt_open_nic() and never
bnxt_hwrm_if_change(). Would it make sense to keep bnge_hwrm_if_change()
out of the reinit path here as well?
> +}
> +
> static const struct net_device_ops bnge_netdev_ops = {
> .ndo_open = bnge_open,
> .ndo_stop = bnge_close,
[ ... ]
^ permalink raw reply [flat|nested] 2+ messages in thread