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 <[email protected]>
Reviewed-by: Dragos Tatulea <[email protected]>
---
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 <[email protected]> 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 <[email protected]>
> Sent: Tuesday, April 2, 2024 10:15 PM
> To: Jakub Kicinski <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; Zaki, Ahmed <[email protected]>; Lobakin, Aleksander
> <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; Keller, Jacob E <[email protected]>;
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; Greenwalt, Paul
> <[email protected]>; Kitszel, Przemyslaw <przemyslaw.kitszel@intelcom>;
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> Drewek, Wojciech <[email protected]>
> 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 <[email protected]> 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" <[email protected]> wrote:
>> On Tue, 02 Apr, 2024 19:18:42 -0700 Jakub Kicinski <[email protected]> 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