Received-SPF: pass (google.com: domain of linux-kernel+bounces-103603-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
References: <20240223192658.45893-1-rrameshbabu@nvidia.com>
 <20240309084440.299358-1-rrameshbabu@nvidia.com>
 <20240309084440.299358-2-rrameshbabu@nvidia.com>
 <20240312165346.14ec1941@kernel.org>
User-agent: mu4e 1.10.8; emacs 28.2
From: Rahul Rameshbabu <rrameshbabu@nvidia.com>
To: Jakub Kicinski <kuba@kernel.org>
Cc: ahmed.zaki@intel.com, aleksander.lobakin@intel.com,
 alexandre.torgue@foss.st.com, andrew@lunn.ch, corbet@lwn.net,
 davem@davemloft.net, dtatulea@nvidia.com, edumazet@google.com,
 gal@nvidia.com, hkallweit1@gmail.com, jacob.e.keller@intel.com,
 jiri@resnulli.us, joabreu@synopsys.com, justinstitt@google.com,
 kory.maincent@bootlin.com, leon@kernel.org, linux-doc@vger.kernel.org,
 linux-kernel@vger.kernel.org, liuhangbin@gmail.com,
 maxime.chevallier@bootlin.com, netdev@vger.kernel.org, pabeni@redhat.com,
 paul.greenwalt@intel.com, przemyslaw.kitszel@intel.com,
 rdunlap@infradead.org, richardcochran@gmail.com, saeed@kernel.org,
 tariqt@nvidia.com, vadim.fedorenko@linux.dev, vladimir.oltean@nxp.com,
 wojciech.drewek@intel.com
Subject: Re: [PATCH RFC v2 1/6] ethtool: add interface to read Tx hardware
 timestamping statistics
Date: Thu, 14 Mar 2024 10:01:49 -0700
In-reply-to: <20240312165346.14ec1941@kernel.org>
Message-ID: <87v85ovj4d.fsf@nvidia.com>
Content-Type: text/plain
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?VN2BFuPYaPmzoW5msA5UyyR+mH8awecrEc4jnI/mCQ1aycj/MJNGC3VqO7eO?=
 =?us-ascii?Q?o5SmJXjcQiPOyE/Q/827lTvwse/JMlYKAlvDAgogzQhXhKctyPNU8yLuqIG1?=
 =?us-ascii?Q?3vfHAYIY05MoW+n1FUfLEn19JVyzQkEU6Gg08+F/ccAMkLfx5+oV5GCm/CoS?=
 =?us-ascii?Q?hZPjsnqE3hZZDyD1b/Zz0zNZXKwP0e9jVZf4QcTLhzoN6yWis4AFLKgoEoqN?=
 =?us-ascii?Q?NAsLvjQ9Vpwe7gS8CMgOqK0GxT/ZGhwWJvVxPWGakA+bczoE4Yjg+jAmUYRy?=
 =?us-ascii?Q?E5wRk9ssPJ/uLAR6g0OfG4KXzIvO+R+/Z/Z49/ymBDxWwoglIJjD5TIM3aTd?=
 =?us-ascii?Q?fdH/iVoqnWCfxnhltNNRMBj91vW6nvE7/vK6ZgKakKpiYClJ3VhaEHKrCQiv?=
 =?us-ascii?Q?LwNAlp/3GtsD8dmLnzqB3i5a8HZft/OkmCjdUBzTv74BvZTTTNX5W5M9WkGR?=
 =?us-ascii?Q?7UU5OM2xy1ccqRs0HPSdEgGmv9hG95wkiPxJLMXlhLSyGr1wYgTVOWIWhAG6?=
 =?us-ascii?Q?8Zb0CMdjQ+SPUuzEnB98oDIDhp0m6b1QPLwrDklXH3fjawIA3XNruyPug7b7?=
 =?us-ascii?Q?355CcezobwXsjTv6aPmOsPP0HNB69KCyT27CNhoCluxBT+cPNa7N5imryKAd?=
 =?us-ascii?Q?hjRlSsJ0pxerGmfSfZ5epfpI5aaB8d9ENkeRARWQDZMqsbGttVfqdgKB/bJ/?=
 =?us-ascii?Q?NGKcP0kt7ALX1BjBC053RcNSdn9gOXky6EiWWtB3ymHpTEnMrJmOM3HT7FtS?=
 =?us-ascii?Q?rh9QfayxfZGxMPkLKR2mrbSr7uqEOaVWW8UHs64ltKxqrSUq3A/XvKKHXxBu?=
 =?us-ascii?Q?0N3DgGHbNA97FEpbNQfjZcHAiBCX+EktA0SueW1ffL9tuc9qQgNoffzpIl5Z?=
 =?us-ascii?Q?m7dv3PxfA8DQu/jpPQRyfT56k4f7dbMbdYMswHcQn6xAu1DNyzRVV1e4t7Ss?=
 =?us-ascii?Q?KBZdmyBQvvdffWgqx6RoXCowT+6e3fTh/1lXbUtPQe84Ic+PEl/wqR7gGDIp?=
 =?us-ascii?Q?uREb+Vi25uV1AUgCDG8uppFZlYun7v/lFu96ubFkpacEvD2oE+Bp0dkNraeE?=
 =?us-ascii?Q?rYbfzB1glBifNZqCSnNUyUX+8rwmGsGSOqndnxKj8AYa9e8kk4RELNLB6qvm?=
 =?us-ascii?Q?Zr4ZFqeC++iBjycrlSskUsX6/ZwXEWryMEJ22EyH0b8tGefFfSi18kNcER1q?=
 =?us-ascii?Q?+drp11f5s+08Qs2brRuqYbhU6TuUvJirREG8oB+eRopqGMs7yO1Onxws5m7/?=
 =?us-ascii?Q?9GR4+J/ni6LU/9H00zXrxi4JsBpWlQllgceAFkMn3AYOo4qQqugxHnmj6Tqs?=
 =?us-ascii?Q?vNPdIiHN2d0ptlmtHF7bmDcWgZ+eSUt82tf6no+IJO0w2aWAfhszSLvUvQTK?=
 =?us-ascii?Q?yl+jkEwesmKmvwdvZ/WPe2hegnArnjKNWZNmEwSbopXgGdAS3fhoKgA15wkO?=
 =?us-ascii?Q?Xrur/7E6mcx03xq8nfSAx/klBBbHtsejHvbj9ngErTfdBJ9zza4mwH4tqe8a?=
 =?us-ascii?Q?14PEPCdaeu5ZdIqkm7MxddqLtEKFi4qTQOWRUlcbc594G898+K+pjrx7JgDa?=
 =?us-ascii?Q?5CLEmXvl8pdnjdpV1zVOmz5q+mRDUvu402/ToULeGkcy3B227bcZvCh/FPq9?=
 =?us-ascii?Q?Bw=3D=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a898300a-3c5a-4b34-4f77-08dc4449036b
X-MS-Exchange-CrossTenant-AuthSource: BYAPR12MB2743.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Mar 2024 17:05:56.2661
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Ipe8ROyK0zwhB4i4BZDoqkZcOvQhGXsx3exShxCaTgrfc0oTLBwBz/q5I+XAEPq4IqjZUNRHnecjmu22AC4fjg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR12MB7573


On Tue, 12 Mar, 2024 16:53:46 -0700 Jakub Kicinski <kuba@kernel.org> wrote:
> On Sat,  9 Mar 2024 00:44:35 -0800 Rahul Rameshbabu wrote:
>> 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.
>
>>  Documentation/netlink/specs/ethtool.yaml | 20 +++++++++
>>  include/linux/ethtool.h                  | 21 ++++++++++
>>  include/uapi/linux/ethtool_netlink.h     | 15 +++++++
>>  net/ethtool/tsinfo.c                     | 52 +++++++++++++++++++++++-
>>  4 files changed, 107 insertions(+), 1 deletion(-)
>
> Feels like we should mention the new stats somehow in 
> Documentation/networking/ethtool-netlink.rst
>
>> diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
>> index 197208f419dc..f99b003c78c0 100644
>> --- a/Documentation/netlink/specs/ethtool.yaml
>> +++ b/Documentation/netlink/specs/ethtool.yaml
>> @@ -559,6 +559,21 @@ attribute-sets:
>>        -
>>          name: tx-lpi-timer
>>          type: u32
>> +  -
>> +    name: ts-stat
>> +    attributes:
>> +      -
>> +        name: pad
>> +        type: pad
>
> You can remove the pad entry, and...
>

You need the pad to match with ETHTOOL_A_TS_STAT_PAD (which similar to
other ethtool stats currently defined). Otherwise, you run into the
following.... mm-stat and fec-stat are good examples.

  [root@binary-eater-vm-01 linux-ethtool-ts]# ./tools/net/ynl/ethtool.py --show-time-stamping mlx5_1
  Traceback (most recent call last):
    File "/root/linux-ethtool-ts/tools/net/ynl/lib/ynl.py", line 598, in _decode
      attr_spec = attr_space.attrs_by_val[attr.type]
                  ~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^
  KeyError: 4

  During handling of the above exception, another exception occurred:

  Traceback (most recent call last):
    File "/root/linux-ethtool-ts/./tools/net/ynl/ethtool.py", line 437, in <module>
      main()
    File "/root/linux-ethtool-ts/./tools/net/ynl/ethtool.py", line 333, in main
      tsinfo = dumpit(ynl, args, 'tsinfo-get', req)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    File "/root/linux-ethtool-ts/./tools/net/ynl/ethtool.py", line 91, in dumpit
      reply = ynl.dump(op_name, { 'header': {} } | extra)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    File "/root/linux-ethtool-ts/tools/net/ynl/lib/ynl.py", line 873, in dump
      return self._op(method, vals, [], dump=True)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    File "/root/linux-ethtool-ts/tools/net/ynl/lib/ynl.py", line 858, in _op
      rsp_msg = self._decode(decoded.raw_attrs, op.attr_set.name)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    File "/root/linux-ethtool-ts/tools/net/ynl/lib/ynl.py", line 607, in _decode
      subdict = self._decode(NlAttrs(attr.raw), attr_spec['nested-attributes'], search_attrs)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    File "/root/linux-ethtool-ts/tools/net/ynl/lib/ynl.py", line 601, in _decode
      raise Exception(f"Space '{space}' has no attribute with value '{attr.type}'")
  Exception: Space 'ts-stat' has no attribute with value '4'

>> +enum {
>> +	ETHTOOL_A_TS_STAT_UNSPEC,
>> +	ETHTOOL_A_TS_STAT_PAD,
>> +
>> +	ETHTOOL_A_TS_STAT_TX_PKT,			/* array, u64 */
>> +	ETHTOOL_A_TS_STAT_TX_LOST,			/* array, u64 */
>> +	ETHTOOL_A_TS_STAT_TX_ERR,			/* array, u64 */
>
> I don't think these are arrays.
>
>> +
>> +	/* add new constants above here */
>> +	__ETHTOOL_A_TS_STAT_CNT,
>> +	ETHTOOL_A_TS_STAT_MAX = (__ETHTOOL_A_TS_STAT_CNT - 1)
>> +
>> +};

--
Thanks,

Rahul Rameshbabu