Re: [PATCH net] ethtool: init tsinfo stats if requested

[Rahul Rameshbabu <rrameshbabu@nvidia.com>]

On Wed, 29 May, 2024 21:08:14 -0700 Vadim Fedorenko <vadfed@meta.com> wrote:
> Statistic values should be set to ETHTOOL_STAT_NOT_SET even if the
> device doesn't support statistics. Otherwise zeros will be returned as
> if they are proper values:
>
> host# ethtool -I -T lo
> Time stamping parameters for lo:
> Capabilities:
> 	software-transmit
> 	software-receive
> 	software-system-clock
> PTP Hardware Clock: none
> Hardware Transmit Timestamp Modes: none
> Hardware Receive Filter Modes: none
> Statistics:
>   tx_pkts: 0
>   tx_lost: 0
>   tx_err: 0
>
> Fixes: 0e9c127729be ("ethtool: add interface to read Tx hardware timestamping statistics")
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
> ---
>  net/ethtool/tsinfo.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/ethtool/tsinfo.c b/net/ethtool/tsinfo.c
> index be2755c8d8fd..57d496287e52 100644
> --- a/net/ethtool/tsinfo.c
> +++ b/net/ethtool/tsinfo.c
> @@ -38,11 +38,11 @@ static int tsinfo_prepare_data(const struct ethnl_req_info *req_base,
>  	ret = ethnl_ops_begin(dev);
>  	if (ret < 0)
>  		return ret;
> -	if (req_base->flags & ETHTOOL_FLAG_STATS &&
> -	    dev->ethtool_ops->get_ts_stats) {
> +	if (req_base->flags & ETHTOOL_FLAG_STATS) {
>  		ethtool_stats_init((u64 *)&data->stats,
>  				   sizeof(data->stats) / sizeof(u64));
> -		dev->ethtool_ops->get_ts_stats(dev, &data->stats);
> +		if (dev->ethtool_ops->get_ts_stats)
> +			dev->ethtool_ops->get_ts_stats(dev, &data->stats);
>  	}
>  	ret = __ethtool_get_ts_info(dev, &data->ts_info);
>  	ethnl_ops_complete(dev);

Thanks for this catch! I agree with the change. If stats are requested
and the device does not support the statistics, the initial values need
to be set to illustrate the device does not support advertising
statistics. I think the patch should target "ethtool" instead of "net".
Also added Michal, the ethtool maintainer.

Reviewed-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>