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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <877cfbrjo5.fsf@nvidia.com>
Date: Wed, 29 May 2024 21:42:05 -0700
From: Rahul Rameshbabu <rrameshbabu@...dia.com>
To: Vadim Fedorenko <vadfed@...a.com>
Cc: Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Jiri
  Pirko <jiri@...nulli.us>, Vadim  Fedorenko <vadim.fedorenko@...ux.dev>,
 netdev@...r.kernel.org, Michal Kubecek <mkubecek@...e.cz>
Subject: Re: [PATCH net] ethtool: init tsinfo stats if requested

On Wed, 29 May, 2024 21:08:14 -0700 Vadim Fedorenko <vadfed@...a.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@...nel.org>
> Signed-off-by: Vadim Fedorenko <vadfed@...a.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@...dia.com>