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

[netdev-bot+sashiko@kernel.org]

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

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

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] hinic3_get_pause_stats() can never be called.

--

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 and copies them into the standard ethtool
structures.

> diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c b/drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c
> index 64c862052f1e8..2ae4d5afe90df 100644
> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c

[ ... ]

> +static void hinic3_get_pause_stats(struct net_device *netdev,
> +				   struct ethtool_pause_stats *pause_stats)
> +{

[ ... ]

> +	pause_stats->tx_pause_frames = le64_to_cpu(ps->mac_tx_pause_pkts);
> +	pause_stats->rx_pause_frames = le64_to_cpu(ps->mac_rx_pause_pkts);
> +
> +	kfree(ps);
> +}

[ ... ]

> @@ -707,6 +904,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() actually be reached by the ethtool core?

hinic3_ethtool_ops sets .get_pause_stats but has no .get_pauseparam, and
nothing under drivers/net/ethernet/huawei/hinic3/ implements one. The
generic caller of get_pause_stats, pause_prepare_data() in
net/ethtool/pause.c, bails out before it gets to the stats callback:

net/ethtool/pause.c:pause_prepare_data() {
    ...
	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);
    ...
}

The only other user, ethtool_aggregate_pause_stats(), is a helper that
MAC-merge-capable drivers call themselves, and hinic3 doesn't call it.
Pause stats also aren't one of the ETHTOOL_MSG_STATS_GET groups.

So "ethtool -I -a <dev>" would fail with -EOPNOTSUPP. That means
mac_tx_pause_pkts and mac_rx_pause_pkts never reach userspace, even though
the commit message lists get_pause_stats as one of the five added
callbacks.

The driver already has hinic3_get_pause_info() in hinic3_nic_cfg.c, which
hinic3_ethtool.c already calls elsewhere. Could that be used to add a
get_pauseparam implementation? Otherwise, could get_pause_stats wait until
one exists?

>  };
>  
>  void hinic3_set_ethtool_ops(struct net_device *netdev)

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