[PATCH net] ethtool: init tsinfo stats if requested

[PATCH net] ethtool: init tsinfo stats if requested

[Vadim Fedorenko]

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);
-- 
2.43.0

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

[Rahul Rameshbabu]

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>

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

[Rahul Rameshbabu]

On Wed, 29 May, 2024 21:42:05 -0700 Rahul Rameshbabu <rrameshbabu@nvidia.com> wrote:
> 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);

<snip>

>I think the patch should target "ethtool" instead of "net".
> Also added Michal, the ethtool maintainer.

Ignore. Was thinking for some reason this would end up in the ethtool
tree. The patch is correct in targeting net.

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


-- 
Thanks,

Rahul Rameshbabu

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 29 May 2024 21:08:14 -0700 you 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
> 
> [...]

Here is the summary with links:
  - [net] ethtool: init tsinfo stats if requested
    https://git.kernel.org/netdev/net/c/89e281ebff72

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html