| Message ID | 20240530040814.1014446-1-vadfed@meta.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 89e281ebff72e6d37dce2df0e142b2909dafb267 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] ethtool: init tsinfo stats if requested | expand |
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>
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>
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!
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);
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(-)