Multiple network devices that support hardware timestamping appear to have
common behavior with regards to timestamp handling. Implement common Tx
hardware timestamping statistics in a tx_stats struct_group. Common Rx
hardware timestamping statistics can subsequently be implemented in a
rx_stats struct_group for ethtool_ts_stats.

Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>
---
 Documentation/netlink/specs/ethtool.yaml     | 17 +++++++
 Documentation/networking/ethtool-netlink.rst |  9 ++++
 include/linux/ethtool.h                      | 28 ++++++++++-
 include/uapi/linux/ethtool_netlink.h         | 14 ++++++
 net/ethtool/tsinfo.c                         | 52 +++++++++++++++++++-
 5 files changed, 118 insertions(+), 2 deletions(-)

diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
index 197208f419dc..f5aa1e7f3383 100644
--- a/Documentation/netlink/specs/ethtool.yaml
+++ b/Documentation/netlink/specs/ethtool.yaml
@@ -559,6 +559,18 @@ attribute-sets:
       -
         name: tx-lpi-timer
         type: u32
+  -
+    name: ts-stat
+    attributes:
+      -
+        name: tx-pkts
+        type: uint
+      -
+        name: tx-lost
+        type: uint
+      -
+        name: tx-err
+        type: uint
   -
     name: tsinfo
     attributes:
@@ -581,6 +593,10 @@ attribute-sets:
       -
         name: phc-index
         type: u32
+      -
+        name: stats
+        type: nest
+        nested-attributes: ts-stat
   -
     name: cable-result
     attributes:
@@ -1388,6 +1404,7 @@ operations:
             - tx-types
             - rx-filters
             - phc-index
+            - stats
       dump: *tsinfo-get-op
     -
       name: cable-test-act
diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index d583d9abf2f8..08d330b0f50f 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -1237,12 +1237,21 @@ Kernel response contents:
   ``ETHTOOL_A_TSINFO_TX_TYPES``          bitset  supported Tx types
   ``ETHTOOL_A_TSINFO_RX_FILTERS``        bitset  supported Rx filters
   ``ETHTOOL_A_TSINFO_PHC_INDEX``         u32     PTP hw clock index
+  ``ETHTOOL_A_TSINFO_STATS``             nested  HW timestamping statistics
   =====================================  ======  ==========================
 
 ``ETHTOOL_A_TSINFO_PHC_INDEX`` is absent if there is no associated PHC (there
 is no special value for this case). The bitset attributes are omitted if they
 would be empty (no bit set).
 
+Additional hardware timestamping statistics response contents:
+
+  =====================================  ======  ===================================
+  ``ETHTOOL_A_TS_STAT_TX_PKTS``          u64     Packets with Tx HW timestamps
+  ``ETHTOOL_A_TS_STAT_TX_LOST``          u64     Tx HW timestamp not arrived count
+  ``ETHTOOL_A_TS_STAT_TX_ERR``           u64     HW error request Tx timestamp count
+  =====================================  ======  ===================================
+
 CABLE_TEST
 ==========
 
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 9901e563f706..4b5ca7628700 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -480,6 +480,27 @@ struct ethtool_rmon_stats {
 	);
 };
 
+/**
+ * struct ethtool_ts_stats - HW timestamping statistics
+ * @tx_stats: struct group for TX HW timestamping
+ *	@pkts: Number of packets successfully timestamped by the hardware.
+ *	@lost: Number of hardware timestamping requests where the timestamping
+ *		information from the hardware never arrived for submission with
+ *		the skb.
+ *	@err: Number of arbitrary timestamp generation error events that the
+ *		hardware encountered, exclusive of @lost statistics. Cases such
+ *		as resource exhaustion, unavailability, firmware errors, and
+ *		detected illogical timestamp values not submitted with the skb
+ *		are inclusive to this counter.
+ */
+struct ethtool_ts_stats {
+	struct_group(tx_stats,
+		u64 pkts;
+		u64 lost;
+		u64 err;
+	);
+};
+
 #define ETH_MODULE_EEPROM_PAGE_LEN	128
 #define ETH_MODULE_MAX_I2C_ADDRESS	0x7f
 
@@ -755,7 +776,10 @@ struct ethtool_rxfh_param {
  * @get_ts_info: Get the time stamping and PTP hardware clock capabilities.
  *	It may be called with RCU, or rtnl or reference on the device.
  *	Drivers supporting transmit time stamps in software should set this to
- *	ethtool_op_get_ts_info().
+ *	ethtool_op_get_ts_info(). Drivers must not zero statistics which they
+ *	don't report. The stats	structure is initialized to ETHTOOL_STAT_NOT_SET
+ *	indicating driver does not report statistics.
+ * @get_ts_stats: Query the device hardware timestamping statistics.
  * @get_module_info: Get the size and type of the eeprom contained within
  *	a plug-in module.
  * @get_module_eeprom: Get the eeprom information from the plug-in module
@@ -898,6 +922,8 @@ struct ethtool_ops {
 				 struct ethtool_dump *, void *);
 	int	(*set_dump)(struct net_device *, struct ethtool_dump *);
 	int	(*get_ts_info)(struct net_device *, struct ethtool_ts_info *);
+	void	(*get_ts_stats)(struct net_device *dev,
+				struct ethtool_ts_stats *ts_stats);
 	int     (*get_module_info)(struct net_device *,
 				   struct ethtool_modinfo *);
 	int     (*get_module_eeprom)(struct net_device *,
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 3f89074aa06c..8dfd714c2308 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -478,12 +478,26 @@ enum {
 	ETHTOOL_A_TSINFO_TX_TYPES,			/* bitset */
 	ETHTOOL_A_TSINFO_RX_FILTERS,			/* bitset */
 	ETHTOOL_A_TSINFO_PHC_INDEX,			/* u32 */
+	ETHTOOL_A_TSINFO_STATS,				/* nest - _A_TSINFO_STAT */
 
 	/* add new constants above here */
 	__ETHTOOL_A_TSINFO_CNT,
 	ETHTOOL_A_TSINFO_MAX = (__ETHTOOL_A_TSINFO_CNT - 1)
 };
 
+enum {
+	ETHTOOL_A_TS_STAT_UNSPEC,
+
+	ETHTOOL_A_TS_STAT_TX_PKTS,			/* u64 */
+	ETHTOOL_A_TS_STAT_TX_LOST,			/* u64 */
+	ETHTOOL_A_TS_STAT_TX_ERR,			/* u64 */
+
+	/* add new constants above here */
+	__ETHTOOL_A_TS_STAT_CNT,
+	ETHTOOL_A_TS_STAT_MAX = (__ETHTOOL_A_TS_STAT_CNT - 1)
+
+};
+
 /* PHC VCLOCKS */
 
 enum {
diff --git a/net/ethtool/tsinfo.c b/net/ethtool/tsinfo.c
index 9daed0aab162..be2755c8d8fd 100644
--- a/net/ethtool/tsinfo.c
+++ b/net/ethtool/tsinfo.c
@@ -13,14 +13,18 @@ struct tsinfo_req_info {
 struct tsinfo_reply_data {
 	struct ethnl_reply_data		base;
 	struct ethtool_ts_info		ts_info;
+	struct ethtool_ts_stats		stats;
 };
 
 #define TSINFO_REPDATA(__reply_base) \
 	container_of(__reply_base, struct tsinfo_reply_data, base)
 
+#define ETHTOOL_TS_STAT_CNT \
+	(__ETHTOOL_A_TS_STAT_CNT - (ETHTOOL_A_TS_STAT_UNSPEC + 1))
+
 const struct nla_policy ethnl_tsinfo_get_policy[] = {
 	[ETHTOOL_A_TSINFO_HEADER]		=
-		NLA_POLICY_NESTED(ethnl_header_policy),
+		NLA_POLICY_NESTED(ethnl_header_policy_stats),
 };
 
 static int tsinfo_prepare_data(const struct ethnl_req_info *req_base,
@@ -34,6 +38,12 @@ 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) {
+		ethtool_stats_init((u64 *)&data->stats,
+				   sizeof(data->stats) / sizeof(u64));
+		dev->ethtool_ops->get_ts_stats(dev, &data->stats);
+	}
 	ret = __ethtool_get_ts_info(dev, &data->ts_info);
 	ethnl_ops_complete(dev);
 
@@ -79,10 +89,47 @@ static int tsinfo_reply_size(const struct ethnl_req_info *req_base,
 	}
 	if (ts_info->phc_index >= 0)
 		len += nla_total_size(sizeof(u32));	/* _TSINFO_PHC_INDEX */
+	if (req_base->flags & ETHTOOL_FLAG_STATS)
+		len += nla_total_size(0) + /* _TSINFO_STATS */
+		       nla_total_size_64bit(sizeof(u64)) * ETHTOOL_TS_STAT_CNT;
 
 	return len;
 }
 
+static int tsinfo_put_stat(struct sk_buff *skb, u64 val, u16 attrtype)
+{
+	if (val == ETHTOOL_STAT_NOT_SET)
+		return 0;
+	if (nla_put_uint(skb, attrtype, val))
+		return -EMSGSIZE;
+	return 0;
+}
+
+static int tsinfo_put_stats(struct sk_buff *skb,
+			    const struct ethtool_ts_stats *stats)
+{
+	struct nlattr *nest;
+
+	nest = nla_nest_start(skb, ETHTOOL_A_TSINFO_STATS);
+	if (!nest)
+		return -EMSGSIZE;
+
+	if (tsinfo_put_stat(skb, stats->tx_stats.pkts,
+			    ETHTOOL_A_TS_STAT_TX_PKTS) ||
+	    tsinfo_put_stat(skb, stats->tx_stats.lost,
+			    ETHTOOL_A_TS_STAT_TX_LOST) ||
+	    tsinfo_put_stat(skb, stats->tx_stats.err,
+			    ETHTOOL_A_TS_STAT_TX_ERR))
+		goto err_cancel;
+
+	nla_nest_end(skb, nest);
+	return 0;
+
+err_cancel:
+	nla_nest_cancel(skb, nest);
+	return -EMSGSIZE;
+}
+
 static int tsinfo_fill_reply(struct sk_buff *skb,
 			     const struct ethnl_req_info *req_base,
 			     const struct ethnl_reply_data *reply_base)
@@ -119,6 +166,9 @@ static int tsinfo_fill_reply(struct sk_buff *skb,
 	if (ts_info->phc_index >= 0 &&
 	    nla_put_u32(skb, ETHTOOL_A_TSINFO_PHC_INDEX, ts_info->phc_index))
 		return -EMSGSIZE;
+	if (req_base->flags & ETHTOOL_FLAG_STATS &&
+	    tsinfo_put_stats(skb, &data->stats))
+		return -EMSGSIZE;
 
 	return 0;
 }
-- 
2.42.0

On Tue,  2 Apr 2024 13:52:01 -0700 Rahul Rameshbabu wrote:
> +/**
> + * struct ethtool_ts_stats - HW timestamping statistics
> + * @tx_stats: struct group for TX HW timestamping
> + *	@pkts: Number of packets successfully timestamped by the hardware.
> + *	@lost: Number of hardware timestamping requests where the timestamping
> + *		information from the hardware never arrived for submission with
> + *		the skb.
> + *	@err: Number of arbitrary timestamp generation error events that the
> + *		hardware encountered, exclusive of @lost statistics. Cases such
> + *		as resource exhaustion, unavailability, firmware errors, and
> + *		detected illogical timestamp values not submitted with the skb
> + *		are inclusive to this counter.
> + */
> +struct ethtool_ts_stats {
> +	struct_group(tx_stats,

Doesn't seem like the group should be documented:

include/linux/ethtool.h:503: warning: Excess struct member 'tx_stats' description in 'ethtool_ts_stats'
-- 
pw-bot: cr

On Tue, 02 Apr, 2024 19:18:42 -0700 Jakub Kicinski <kuba@kernel.org> wrote:
> On Tue,  2 Apr 2024 13:52:01 -0700 Rahul Rameshbabu wrote:
>> +/**
>> + * struct ethtool_ts_stats - HW timestamping statistics
>> + * @tx_stats: struct group for TX HW timestamping
>> + *	@pkts: Number of packets successfully timestamped by the hardware.
>> + *	@lost: Number of hardware timestamping requests where the timestamping
>> + *		information from the hardware never arrived for submission with
>> + *		the skb.
>> + *	@err: Number of arbitrary timestamp generation error events that the
>> + *		hardware encountered, exclusive of @lost statistics. Cases such
>> + *		as resource exhaustion, unavailability, firmware errors, and
>> + *		detected illogical timestamp values not submitted with the skb
>> + *		are inclusive to this counter.
>> + */
>> +struct ethtool_ts_stats {
>> +	struct_group(tx_stats,
>
> Doesn't seem like the group should be documented:
>
> include/linux/ethtool.h:503: warning: Excess struct member 'tx_stats' description in 'ethtool_ts_stats'

Was looking into why our internal verification did not catch this. We
run W=1 with clang, but looks like the warning does not get triggered
unless explicitly run with scripts/kernel-doc.

  https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#how-to-format-kernel-doc-comments

I have debugged using strace that the way the kernel doc checking works
when W=1 is set is that the matching source file that is being compiled
is passed to scripts/kernel-doc, so include files are missed from the
doc check. I think this is worth adding to the kernel documentation.

Anyway, I will send out a v2 with this fixed but wait for potentially
more feedback on v1.

--
Thanks,

Rahul Rameshbabu

> -----Original Message-----
> From: Rahul Rameshbabu <rrameshbabu@nvidia.com>
> Sent: Tuesday, April 2, 2024 10:15 PM
> To: Jakub Kicinski <kuba@kernel.org>
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> doc@vger.kernel.org; Zaki, Ahmed <ahmed.zaki@intel.com>; Lobakin, Aleksander
> <aleksander.lobakin@intel.com>; alexandre.torgue@foss.st.com;
> andrew@lunn.ch; cjubran@nvidia.com; corbet@lwn.net; davem@davemloft.net;
> dtatulea@nvidia.com; edumazet@google.com; gal@nvidia.com;
> hkallweit1@gmail.com; Keller, Jacob E <jacob.e.keller@intel.com>;
> jiri@resnulli.us; joabreu@synopsys.com; justinstitt@google.com;
> kory.maincent@bootlin.com; leon@kernel.org; liuhangbin@gmail.com;
> maxime.chevallier@bootlin.com; pabeni@redhat.com; Greenwalt, Paul
> <paul.greenwalt@intel.com>; Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>;
> rdunlap@infradead.org; richardcochran@gmail.com; saeed@kernel.org;
> tariqt@nvidia.com; vadim.fedorenko@linux.dev; vladimir.oltean@nxp.com;
> Drewek, Wojciech <wojciech.drewek@intel.com>
> Subject: Re: [PATCH net-next v1 1/6] ethtool: add interface to read Tx hardware
> timestamping statistics
> 
> On Tue, 02 Apr, 2024 19:18:42 -0700 Jakub Kicinski <kuba@kernel.org> wrote:
> > On Tue,  2 Apr 2024 13:52:01 -0700 Rahul Rameshbabu wrote:
> >> +/**
> >> + * struct ethtool_ts_stats - HW timestamping statistics
> >> + * @tx_stats: struct group for TX HW timestamping
> >> + *	@pkts: Number of packets successfully timestamped by the hardware.
> >> + *	@lost: Number of hardware timestamping requests where the
> timestamping
> >> + *		information from the hardware never arrived for submission with
> >> + *		the skb.
> >> + *	@err: Number of arbitrary timestamp generation error events that the
> >> + *		hardware encountered, exclusive of @lost statistics. Cases such
> >> + *		as resource exhaustion, unavailability, firmware errors, and
> >> + *		detected illogical timestamp values not submitted with the skb
> >> + *		are inclusive to this counter.
> >> + */
> >> +struct ethtool_ts_stats {
> >> +	struct_group(tx_stats,
> >
> > Doesn't seem like the group should be documented:
> >
> > include/linux/ethtool.h:503: warning: Excess struct member 'tx_stats'
> description in 'ethtool_ts_stats'
> 
> Was looking into why our internal verification did not catch this. We
> run W=1 with clang, but looks like the warning does not get triggered
> unless explicitly run with scripts/kernel-doc.
> 
>   https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#how-to-
> format-kernel-doc-comments
> 
> I have debugged using strace that the way the kernel doc checking works
> when W=1 is set is that the matching source file that is being compiled
> is passed to scripts/kernel-doc, so include files are missed from the
> doc check. I think this is worth adding to the kernel documentation.
> 

It would be great if the W=1 setup could figure out the include files and send those to kernel-doc too, but I'm not sure if this is possible and if so how difficult it would be to implement it. A lot of headers produce warnings because a lot fewer people manually run kernel-doc on the entire source.

> Anyway, I will send out a v2 with this fixed but wait for potentially
> more feedback on v1.
> 
> --
> Thanks,
> 
> Rahul Rameshbabu

On Wed, 03 Apr, 2024 18:44:52 +0000 "Keller, Jacob E" <jacob.e.keller@intel.com> wrote:
>> On Tue, 02 Apr, 2024 19:18:42 -0700 Jakub Kicinski <kuba@kernel.org> wrote:
>> > On Tue,  2 Apr 2024 13:52:01 -0700 Rahul Rameshbabu wrote:
>> >> +/**
>> >> + * struct ethtool_ts_stats - HW timestamping statistics
>> >> + * @tx_stats: struct group for TX HW timestamping
>> >> + *	@pkts: Number of packets successfully timestamped by the hardware.
>> >> + *	@lost: Number of hardware timestamping requests where the
>> timestamping
>> >> + *		information from the hardware never arrived for submission with
>> >> + *		the skb.
>> >> + *	@err: Number of arbitrary timestamp generation error events that the
>> >> + *		hardware encountered, exclusive of @lost statistics. Cases such
>> >> + *		as resource exhaustion, unavailability, firmware errors, and
>> >> + *		detected illogical timestamp values not submitted with the skb
>> >> + *		are inclusive to this counter.
>> >> + */
>> >> +struct ethtool_ts_stats {
>> >> +	struct_group(tx_stats,
>> >
>> > Doesn't seem like the group should be documented:
>> >
>> > include/linux/ethtool.h:503: warning: Excess struct member 'tx_stats'
>> description in 'ethtool_ts_stats'
>> 
>> Was looking into why our internal verification did not catch this. We
>> run W=1 with clang, but looks like the warning does not get triggered
>> unless explicitly run with scripts/kernel-doc.
>> 
>>   https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#how-to-
>> format-kernel-doc-comments
>> 
>> I have debugged using strace that the way the kernel doc checking works
>> when W=1 is set is that the matching source file that is being compiled
>> is passed to scripts/kernel-doc, so include files are missed from the
>> doc check. I think this is worth adding to the kernel documentation.
>> 
>
> It would be great if the W=1 setup could figure out the include files and send
> those to kernel-doc too, but I'm not sure if this is possible and if so how
> difficult it would be to implement it. A lot of headers produce warnings because
> a lot fewer people manually run kernel-doc on the entire source.
>

I took a look into this, and the one naive solution I had in mind was a
checkdocs target for the kernel where you use gcc -MM to deduce all the
includes, create a unique list, and then run scripts/kernel-doc against
the list of include files. That said, I do think this is excessive
compared to having a checkpatch logic that runs scripts/kernel-doc on
the parts that change in your commit before and after the patch is
applied.

Kudos to the netdev CI for having this.

  https://github.com/linux-netdev/nipa/blob/main/tests/patch/kdoc/kdoc.sh