Re: [net-next,v3,6/8] bng_en: add initial support for ethtool stats display

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
bng_en: add initial support for ethtool stats display

This commit implements ethtool -S callbacks for per-ring RX/TX, TPA,
and port-level MAC/extended statistics, including string names, counts,
and DMA-backed memory allocation for port stats via HWRM wrappers.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_ethtool.c b/drivers/net/ethernet/broadcom/bnge/bnge_ethtool.c
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_ethtool.c
> @@ -31,6 +31,331 @@ static int bnge_nway_reset(struct net_device *dev)
>  	return rc;
>  }
>
> +static const char * const bnge_ring_rx_stats_str[] = {
> +	"rx_ucast_packets",
> +	"rx_mcast_packets",
> +	"rx_bcast_packets",
> +	"rx_discards",
> +	"rx_errors",
> +	"rx_ucast_bytes",
> +	"rx_mcast_bytes",
> +	"rx_bcast_bytes",
> +};
> +
> +static const char * const bnge_ring_tx_stats_str[] = {
> +	"tx_ucast_packets",
> +	"tx_mcast_packets",
> +	"tx_bcast_packets",
> +	"tx_errors",
> +	"tx_discards",

Jakub Kicinski raised this concern during v3 review: standard stats like
rx_errors and tx_errors should not be duplicated in ethtool -S output.
While this patch adds structured callbacks (.get_eth_mac_stats,
.get_rmon_stats, etc.) for standard stats, rx_errors and tx_errors still
appear in the bnge_ring_rx_stats_str[] and bnge_ring_tx_stats_str[]
arrays above.

These arrays are used by bnge_get_strings() which feeds ethtool -S,
meaning the standard stats will still appear in ethtool -S output.

Should rx_errors and tx_errors be removed from these arrays to comply
with the kernel policy against duplicating standard stats?

[ ... ]

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_hwrm_lib.c b/drivers/net/ethernet/broadcom/bnge/bnge_hwrm_lib.c
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_hwrm_lib.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_hwrm_lib.c

[ ... ]

> +int bnge_hwrm_port_qstats_ext(struct bnge_dev *bd, u8 flags)
> +{
> +	struct hwrm_queue_pri2cos_qcfg_output *resp_qc;
> +	struct bnge_net *bn = netdev_priv(bd->netdev);
> +	struct hwrm_queue_pri2cos_qcfg_input *req_qc;
> +	struct hwrm_port_qstats_ext_output *resp_qs;
> +	struct hwrm_port_qstats_ext_input *req_qs;
> +	struct bnge_pf_info *pf = &bd->pf;
> +	u32 tx_stat_size;
> +	int rc;
> +
> +	if (!(bn->flags & BNGE_FLAG_PORT_STATS_EXT))
> +		return 0;
> +
> +	if (flags && !(bd->fw_cap & BNGE_FW_CAP_EXT_HW_STATS_SUPPORTED))
> +		return -EOPNOTSUPP;
> +
> +	rc = bnge_hwrm_req_init(bd, req_qs, HWRM_PORT_QSTATS_EXT);
> +	if (rc)
> +		return rc;
> +
> +	req_qs->flags = flags;
> +	req_qs->port_id = cpu_to_le16(pf->port_id);
> +	req_qs->rx_stat_size = cpu_to_le16(sizeof(struct rx_port_stats_ext));
> +	req_qs->rx_stat_host_addr =
> +		cpu_to_le64(bn->rx_port_stats_ext.hw_stats_map);
> +	tx_stat_size = bn->tx_port_stats_ext.hw_stats ?
> +		       sizeof(struct tx_port_stats_ext) : 0;
> +	req_qs->tx_stat_size = cpu_to_le16(tx_stat_size);
> +	req_qs->tx_stat_host_addr =
> +		cpu_to_le64(bn->tx_port_stats_ext.hw_stats_map);
> +	resp_qs = bnge_hwrm_req_hold(bd, req_qs);
> +	rc = bnge_hwrm_req_send(bd, req_qs);
> +	if (!rc) {
> +		bn->fw_rx_stats_ext_size =
> +			le16_to_cpu(resp_qs->rx_stat_size) / 8;
> +		bn->fw_tx_stats_ext_size = tx_stat_size ?
> +			le16_to_cpu(resp_qs->tx_stat_size) / 8 : 0;
> +	} else {
> +		bn->fw_rx_stats_ext_size = 0;
> +		bn->fw_tx_stats_ext_size = 0;
> +	}
> +	bnge_hwrm_req_drop(bd, req_qs);
> +
> +	if (flags)
> +		return rc;
> +
> +	if (bn->fw_tx_stats_ext_size <=
> +	    offsetof(struct tx_port_stats_ext, pfc_pri0_tx_duration_us) / 8) {
> +		bn->pri2cos_valid = false;
> +		return rc;
> +	}
> +
> +	rc = bnge_hwrm_req_init(bd, req_qc, HWRM_QUEUE_PRI2COS_QCFG);
> +	if (rc)
> +		return rc;
> +
> +	req_qc->flags = cpu_to_le32(QUEUE_PRI2COS_QCFG_REQ_FLAGS_IVLAN);
> +
> +	resp_qc = bnge_hwrm_req_hold(bd, req_qc);
> +	rc = bnge_hwrm_req_send(bd, req_qc);
> +	if (!rc) {
> +		u8 *pri2cos;
> +		int i, j;
> +
> +		pri2cos = &resp_qc->pri0_cos_queue_id;
> +		for (i = 0; i < 8; i++) {
> +			u8 queue_id = pri2cos[i];
> +			u8 queue_idx;
> +
> +			/* Per port queue IDs start from 0, 10, 20, etc */
> +			queue_idx = queue_id % 10;
> +			if (queue_idx > BNGE_MAX_QUEUE) {
> +				bn->pri2cos_valid = false;
> +				bnge_hwrm_req_drop(bd, req_qc);
> +				return rc;
> +			}
>                         ^^^^

Is this bounds check off by one? BNGE_MAX_QUEUE is 8, so queue_idx of
exactly 8 passes this check and gets stored in bn->pri2cos_idx[i].

Looking at the usage in bnge_get_ethtool_stats()
(bnge_ethtool.c:482-504), the code computes base_off + bn->pri2cos_idx[i]
to index into the CoS stat arrays:

	for (i = 0; i < 8; i++, j++) {
		long n = bnge_rx_bytes_pri_arr[i].base_off +
			 bn->pri2cos_idx[i];
		buf[j] = *(rx_port_stats_ext + n);
	}

The CoS arrays in rx_port_stats_ext and tx_port_stats_ext have entries
cos0 through cos7, which are 8 entries total with valid indices 0-7.
When pri2cos_idx[i] equals 8, the computed offset reads one entry past
cos7, returning data from the wrong stats counter.

This can happen when firmware reports a queue_id whose modulo 10 equals 8
(e.g., queue_id 8, 18, 28, etc.).

Should the check be queue_idx >= BNGE_MAX_QUEUE instead?

[ ... ]