Re: [PATCH net] bnge: add missing ndo_set_features

[Simon Horman <horms@kernel.org>]

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,

[ ... ]