Re: [PATCH net-next v14 2/2] hinic3: Add ethtool statistic ops part2

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 3 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Of the five new callbacks, only hinic3_get_eth_phy_stats()
  returns early for VFs (`if (HINIC3_IS_VF(nic_dev->hwdev)) return;`).
- [Medium] Several counters are now reported through both the private
  ethtool -S table and the standard RMON uAPI.
- [Medium] Nothing can ever call hinic3_get_pause_stats().
- [Low] The commit message does not describe this patch.

--

hinic3: Add ethtool statistic ops part2

This commit adds five standard ethtool statistics callbacks to the hinic3
driver: get_eth_phy_stats, get_eth_mac_stats, get_eth_ctrl_stats,
get_rmon_stats and get_pause_stats. Each callback reads the physical port
counters from firmware via hinic3_get_phy_port_stats() and copies them
into the standard ethtool structures.

>   Add PF/VF statistics functions in TX and RX processing.

[Severity: Low]
This isn't a bug, but this sentence matches the one in the part1 commit
("hinic3: Add ethtool statistic ops part1"). This patch doesn't touch any
TX or RX processing code. It only adds read-only ethtool callbacks in
hinic3_ethtool.c that use the MAG_CMD_GET_PORT_STAT firmware query.

The message also doesn't say how VFs are handled. Only
hinic3_get_eth_phy_stats() returns early for VFs.

Could the commit message be updated to describe what this patch does?

> diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c b/drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c
> index 3863d26542ad4..f61337d494444 100644
> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c
> @@ -723,6 +723,179 @@ static void hinic3_get_ethtool_stats(struct net_device *netdev,
>  	hinic3_get_drv_queue_stats(netdev, data + i);
>  }
>  
> +static void hinic3_get_eth_phy_stats(struct net_device *netdev,
> +				     struct ethtool_eth_phy_stats *phy_stats)
> +{
> +	struct hinic3_nic_dev *nic_dev = netdev_priv(netdev);
> +	struct mag_cmd_port_stats *ps;
> +	int err;
> +
> +	if (HINIC3_IS_VF(nic_dev->hwdev))
> +		return;

[ ... ]

> +static void hinic3_get_eth_mac_stats(struct net_device *netdev,
> +				     struct ethtool_eth_mac_stats *mac_stats)
> +{
> +	struct hinic3_nic_dev *nic_dev = netdev_priv(netdev);
> +	struct mag_cmd_port_stats *ps;
> +	int err;
> +
> +	ps = kmalloc_obj(*ps);
> +	if (!ps)
> +		return;
> +
> +	err = hinic3_get_phy_port_stats(nic_dev->hwdev, ps);
> +	if (err) {
> +		kfree(ps);
> +		netdev_err(netdev, "Failed to get eth mac stats from fw\n");
> +		return;
> +	}

[Severity: Medium]
Should this callback, and the new ctrl, rmon and pause callbacks, have the
same HINIC3_IS_VF() check as hinic3_get_eth_phy_stats()?

As written, on a VF these call hinic3_get_phy_port_stats(), which sends
MAG_CMD_GET_PORT_STAT for hinic3_physical_port_id(hwdev).

The rest of the driver keeps physical port stats away from VFs. For
example, hinic3_get_ethtool_stats() does:

	if (!HINIC3_IS_VF(nic_dev->hwdev))
		i += hinic3_get_ethtool_port_stats(netdev, data + i);

hinic3_get_hw_stats_strings() and hinic3_get_sset_count() do the same.
hinic3_get_link_settings() also skips the pause query on VFs.

ETHTOOL_MSG_STATS_GET does not require admin permission. Any unprivileged
user can reach these callbacks with something like
"ethtool -S <vf> --groups eth-mac eth-ctrl rmon".

If firmware accepts the query from a VF, the VF sees port-wide MAC, control
and RMON counters. Those include traffic from the PF and every other VF on
the port.

If firmware rejects it, each request makes several blocking mailbox round
trips. Each failure also logs an unratelimited dev_err() in
hinic3_get_phy_port_stats() plus a netdev_err() in the callback. Could
that be used to flood the kernel log?

[ ... ]

> +static void hinic3_get_rmon_stats(struct net_device *netdev,
> +				  struct ethtool_rmon_stats *rmon_stats,
> +				  const struct ethtool_rmon_hist_range **ranges)
> +{

[ ... ]

> +	rmon_stats->undersize_pkts	= ps->mac_rx_undersize_pkt_num;
> +	rmon_stats->oversize_pkts	= ps->mac_rx_oversize_pkt_num;
> +	rmon_stats->fragments		= ps->mac_rx_fragment_pkt_num;
> +	rmon_stats->jabbers		= ps->mac_rx_jabber_pkt_num;
> +
> +	rmon_stats->hist[0]		= ps->mac_rx_64_oct_pkt_num;

[ ... ]

> +	rmon_stats->hist_tx[10]		= ps->mac_tx_9217_12287_oct_pkt_num;

[Severity: Medium]
Are these counters now reported to userspace twice? hinic3_port_stats[],
added by the part1 patch, still exports the same values through
ethtool -S:

	HINIC3_PORT_STAT(mac_tx_64_oct_pkt_num),
	...
	HINIC3_PORT_STAT(mac_tx_9217_12287_oct_pkt_num),
	...
	HINIC3_PORT_STAT(mac_rx_fragment_pkt_num),
	HINIC3_PORT_STAT(mac_rx_undersize_pkt_num),
	...
	HINIC3_PORT_STAT(mac_rx_64_oct_pkt_num),
	...
	HINIC3_PORT_STAT(mac_rx_9217_12287_oct_pkt_num),

The table already omits the counters that the new mac, ctrl, pause and phy
callbacks report (good, multi and broad counts, pause, control, oversize,
jabber, sym_err, fcs_err). The RMON histogram, fragment and undersize
entries are still in it.

Counters that have a standard uAPI should not be duplicated in ethtool -S.
Should these 24 entries be removed from hinic3_port_stats[]?

[ ... ]

> @@ -734,6 +907,11 @@ static const struct ethtool_ops hinic3_ethtool_ops = {
>  	.get_strings                    = hinic3_get_strings,
>  	.get_sset_count                 = hinic3_get_sset_count,
>  	.get_ethtool_stats              = hinic3_get_ethtool_stats,
> +	.get_eth_phy_stats              = hinic3_get_eth_phy_stats,
> +	.get_eth_mac_stats              = hinic3_get_eth_mac_stats,
> +	.get_eth_ctrl_stats             = hinic3_get_eth_ctrl_stats,
> +	.get_rmon_stats                 = hinic3_get_rmon_stats,
> +	.get_pause_stats                = hinic3_get_pause_stats,
>  };

[Severity: Medium]
Can hinic3_get_pause_stats() ever be called? The driver doesn't provide a
.get_pauseparam callback, and pause_prepare_data() in net/ethtool/pause.c
returns before it calls get_pause_stats:

	if (!dev->ethtool_ops->get_pauseparam)
		return -EOPNOTSUPP;
	...
	dev->ethtool_ops->get_pauseparam(dev, &data->pauseparam);
	if (req_base->flags & ETHTOOL_FLAG_STATS &&
	    dev->ethtool_ops->get_pause_stats)
		dev->ethtool_ops->get_pause_stats(dev, &data->pausestat);

Pause stats aren't part of the ETHTOOL_MSG_STATS_GET groups either. So
"ethtool -I -a <dev>" fails with -EOPNOTSUPP, and there is no standard way
to read mac_tx_pause_num or mac_rx_pause_num.

Does this patch also need a .get_pauseparam implementation?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1791858167.git.gongfan1%40huawei.com