[PATCH net 0/3] net: ethtool: timestamping: Fix small issues in the new uAPI

[PATCH net 0/3] net: ethtool: timestamping: Fix small issues in the new uAPI

[Kory Maincent]

Fix the hwtstamp flag parameter type by changing it from u32 to bitset,
ensuring correct representation and usage.
Correct a minor issue with the enumeration size check to improve
validation.

Add myself as the maintainer for socket timestamping to oversee the
changes before Linux is released.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
Kory Maincent (3):
      MAINTAINERS: Add myself as maintainer for socket timestamping and expand file list
      net: ethtool: tsconfig: Fix ts filters and types enums size check
      net: ethtool: tsconfig: Fix netlink type of hwtstamp flags

 Documentation/netlink/specs/ethtool.yaml |  3 ++-
 MAINTAINERS                              |  4 ++++
 include/uapi/linux/ethtool.h             |  2 ++
 net/ethtool/common.c                     |  5 +++++
 net/ethtool/common.h                     |  2 ++
 net/ethtool/strset.c                     |  5 +++++
 net/ethtool/tsconfig.c                   | 37 +++++++++++++++++++++-----------
 7 files changed, 45 insertions(+), 13 deletions(-)
---
base-commit: 05d91cdb1f9108426b14975ef4eeddf15875ca05
change-id: 20250127-fix_tsconfig-6e2a94bc2158

Best regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[PATCH net 1/3] MAINTAINERS: Add myself as maintainer for socket timestamping and expand file list

[Kory Maincent]

Add myself as maintainer for socket timestamping. I have contributed
modifications to the timestamping core to support selection between
multiple PTP instances within a network topology.

Expand the file list to include timestamping ethtool support.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 MAINTAINERS | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 1899ef93e498..1052131a141d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21729,10 +21729,14 @@ F:	sound/soc/uniphier/
 
 SOCKET TIMESTAMPING
 M:	Willem de Bruijn <willemdebruijn.kernel@gmail.com>
+M:	Kory Maincent <kory.maincent@bootlin.com>
 S:	Maintained
 F:	Documentation/networking/timestamping.rst
 F:	include/linux/net_tstamp.h
 F:	include/uapi/linux/net_tstamp.h
+F:	net/core/timestamping.c
+F:	net/ethtool/tsconfig.c
+F:	net/ethtool/tsinfo.c
 F:	tools/testing/selftests/net/so_txtime.c
 
 SOEKRIS NET48XX LED SUPPORT

-- 
2.34.1

Re: [PATCH net 1/3] MAINTAINERS: Add myself as maintainer for socket timestamping and expand file list

[Jakub Kicinski]

On Tue, 28 Jan 2025 16:35:46 +0100 Kory Maincent wrote:
> Add myself as maintainer for socket timestamping. I have contributed
> modifications to the timestamping core to support selection between
> multiple PTP instances within a network topology.
> 
> Expand the file list to include timestamping ethtool support.

Hi Kory, is there more context you could provide for this change?

For core pieces of the stack, with a long history, we tend to
designate as maintainer folks who review the changes, not just
write code. According to our development stats that doesn't
describe you, just yet:

Top reviewer score:

6.12: Negative # 5 ( +6) [ 50] Kory Maincent (Dent Project)
6.13: Negative #11 (***) [ 29] Kory Maincent (Dent Project)

https://lore.kernel.org/20250121200710.19126f7d@kernel.org
https://lore.kernel.org/20241119191608.514ea226@kernel.org
https://lore.kernel.org/20240922190125.24697d06@kernel.org

That said, I do feel like we're lacking maintainers for sections 
of the ethtool code. Maybe we could start with adding and entry 
for you for just:

> +F:	net/ethtool/tsconfig.c
> +F:	net/ethtool/tsinfo.c

Does that sound fair?

Re: [PATCH net 1/3] MAINTAINERS: Add myself as maintainer for socket timestamping and expand file list

[Kory Maincent]

On Wed, 29 Jan 2025 16:39:16 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Tue, 28 Jan 2025 16:35:46 +0100 Kory Maincent wrote:
> > Add myself as maintainer for socket timestamping. I have contributed
> > modifications to the timestamping core to support selection between
> > multiple PTP instances within a network topology.
> > 
> > Expand the file list to include timestamping ethtool support.  
> 
> Hi Kory, is there more context you could provide for this change?
> 
> For core pieces of the stack, with a long history, we tend to
> designate as maintainer folks who review the changes, not just
> write code. According to our development stats that doesn't
> describe you, just yet:
> 
> Top reviewer score:
> 
> 6.12: Negative # 5 ( +6) [ 50] Kory Maincent (Dent Project)
> 6.13: Negative #11 (***) [ 29] Kory Maincent (Dent Project)

Yes indeed, I am not really confident reviewing code part that I don't know
well. My thought that I did not have lots on review in the PTP patch series I
just managed to get merged. So I would try to change that.
Moreover I have changed a bit the management of time stamping configuration so
if there is changes or fixes I could be helpful.
Indeed adding myself as maintainer is a bit overestimated, maybe I could be set
as a reviewer to be in the CC of the patches.

> https://lore.kernel.org/20250121200710.19126f7d@kernel.org
> https://lore.kernel.org/20241119191608.514ea226@kernel.org
> https://lore.kernel.org/20240922190125.24697d06@kernel.org
> 
> That said, I do feel like we're lacking maintainers for sections 
> of the ethtool code. Maybe we could start with adding and entry 
> for you for just:
> 
> > +F:	net/ethtool/tsconfig.c
> > +F:	net/ethtool/tsinfo.c  
> 
> Does that sound fair?

Yes it does. Whether setting me as reviewer for the SOCKET TIMESTAMPING subpart
or adding me as maintainer of these two files it is ok for me. What do you
prefer?

I will try to review more ethtool code now that I began to understand how it
works thanks to my PoE and PTP work. 

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net 1/3] MAINTAINERS: Add myself as maintainer for socket timestamping and expand file list

[Jakub Kicinski]

On Thu, 30 Jan 2025 10:18:11 +0100 Kory Maincent wrote:
> > > +F:	net/ethtool/tsconfig.c
> > > +F:	net/ethtool/tsinfo.c    
> > 
> > Does that sound fair?  
> 
> Yes it does. Whether setting me as reviewer for the SOCKET TIMESTAMPING subpart
> or adding me as maintainer of these two files it is ok for me. What do you
> prefer?

The latter, TBH. I'll send the patch. We recommend setting up lore+lei
to subscribe to threads which touch particular files. It's much more
scalable than adding interested folks to MAINTAINERS. Tho, last time
I looked lei didn't support the weird mdir format used by claws :(

Re: [PATCH net 1/3] MAINTAINERS: Add myself as maintainer for socket timestamping and expand file list

[Kory Maincent]

On Thu, 30 Jan 2025 08:31:04 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Thu, 30 Jan 2025 10:18:11 +0100 Kory Maincent wrote:
>  [...]  
> > > 
> > > Does that sound fair?    
> > 
> > Yes it does. Whether setting me as reviewer for the SOCKET TIMESTAMPING
> > subpart or adding me as maintainer of these two files it is ok for me. What
> > do you prefer?  
> 
> The latter, TBH. I'll send the patch. We recommend setting up lore+lei
> to subscribe to threads which touch particular files. It's much more
> scalable than adding interested folks to MAINTAINERS. Tho, last time
> I looked lei didn't support the weird mdir format used by claws :(

You are right don't bother to add me to MAINTAINERS file, I am indeed not an
experienced reviewer for now.

Oh, didn't know the lei tools. Thanks!

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[PATCH net 2/3] net: ethtool: tsconfig: Fix ts filters and types enums size check

[Kory Maincent]

Fix the size check for the hwtstamp_tx_types and hwtstamp_rx_filters
enumerations. Align this check with the approach used in tsinfo for
consistency and correctness.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
Fixes: 6e9e2eed4f39 ("net: ethtool: Add support for tsconfig command to get/set hwtstamp config")
---
 net/ethtool/tsconfig.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ethtool/tsconfig.c b/net/ethtool/tsconfig.c
index 9188e088fb2f..2dc3a3b76615 100644
--- a/net/ethtool/tsconfig.c
+++ b/net/ethtool/tsconfig.c
@@ -294,8 +294,8 @@ static int ethnl_set_tsconfig(struct ethnl_req_info *req_base,
 	struct nlattr **tb = info->attrs;
 	int ret;
 
-	BUILD_BUG_ON(__HWTSTAMP_TX_CNT >= 32);
-	BUILD_BUG_ON(__HWTSTAMP_FILTER_CNT >= 32);
+	BUILD_BUG_ON(__HWTSTAMP_TX_CNT > 32);
+	BUILD_BUG_ON(__HWTSTAMP_FILTER_CNT > 32);
 
 	if (!netif_device_present(dev))
 		return -ENODEV;

-- 
2.34.1

Re: [PATCH net 2/3] net: ethtool: tsconfig: Fix ts filters and types enums size check

[Jakub Kicinski]

On Tue, 28 Jan 2025 16:35:47 +0100 Kory Maincent wrote:
> Fix the size check for the hwtstamp_tx_types and hwtstamp_rx_filters
> enumerations.

This is just a code cleanup, the constants are way smaller than 32
today. The assert being too restrictive makes no functional difference.

> Align this check with the approach used in tsinfo for
> consistency and correctness.

First-principles based explanation of why 32 is the correct value would
be much better than just alignment. Otherwise reviewer has to figure out
whether we should be changing from >= to > or vice versa...

> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> Fixes: 6e9e2eed4f39 ("net: ethtool: Add support for tsconfig command to get/set hwtstamp config")
-- 
pw-bot: cr

Re: [PATCH net 2/3] net: ethtool: tsconfig: Fix ts filters and types enums size check

[Kory Maincent]

On Wed, 29 Jan 2025 16:45:08 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Tue, 28 Jan 2025 16:35:47 +0100 Kory Maincent wrote:
> > Fix the size check for the hwtstamp_tx_types and hwtstamp_rx_filters
> > enumerations.  
> 
> This is just a code cleanup, the constants are way smaller than 32
> today. The assert being too restrictive makes no functional difference.

That's right, it was mainly for consistency with the other assert. And to avoid
possible future mistake but indeed reaching 32 bit is not expected soon. Should
I remove the patch as it is not a functional issue?

> > Align this check with the approach used in tsinfo for
> > consistency and correctness.  
> 
> First-principles based explanation of why 32 is the correct value would
> be much better than just alignment. Otherwise reviewer has to figure out
> whether we should be changing from >= to > or vice versa...

Ack, I will if I keep the patch.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net 2/3] net: ethtool: tsconfig: Fix ts filters and types enums size check

[Jakub Kicinski]

On Thu, 30 Jan 2025 10:24:51 +0100 Kory Maincent wrote:
> > This is just a code cleanup, the constants are way smaller than 32
> > today. The assert being too restrictive makes no functional difference.  
> 
> That's right, it was mainly for consistency with the other assert. And to avoid
> possible future mistake but indeed reaching 32 bit is not expected soon. Should
> I remove the patch as it is not a functional issue?

Yes, drop it please and repost after net-next re-opens.

[PATCH net 3/3] net: ethtool: tsconfig: Fix netlink type of hwtstamp flags

[Kory Maincent]

Fix the netlink type for hardware timestamp flags, which are represented
as a bitset of flags. Although only one flag is supported currently, the
correct netlink bitset type should be used instead of u32. Address this
by adding a new named string set description for the hwtstamp flag
structure.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
Fixes: 6e9e2eed4f39 ("net: ethtool: Add support for tsconfig command to get/set hwtstamp config")
---
 Documentation/netlink/specs/ethtool.yaml |  3 ++-
 include/uapi/linux/ethtool.h             |  2 ++
 net/ethtool/common.c                     |  5 +++++
 net/ethtool/common.h                     |  2 ++
 net/ethtool/strset.c                     |  5 +++++
 net/ethtool/tsconfig.c                   | 33 ++++++++++++++++++++++----------
 6 files changed, 39 insertions(+), 11 deletions(-)

diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
index 259cb211a338..655d8d10fe24 100644
--- a/Documentation/netlink/specs/ethtool.yaml
+++ b/Documentation/netlink/specs/ethtool.yaml
@@ -1524,7 +1524,8 @@ attribute-sets:
         nested-attributes: bitset
       -
         name: hwtstamp-flags
-        type: u32
+        type: nest
+        nested-attributes: bitset
 
 operations:
   enum-model: directional
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index d1089b88efc7..9b18c4cfe56f 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -682,6 +682,7 @@ enum ethtool_link_ext_substate_module {
  * @ETH_SS_STATS_ETH_CTRL: names of IEEE 802.3 MAC Control statistics
  * @ETH_SS_STATS_RMON: names of RMON statistics
  * @ETH_SS_STATS_PHY: names of PHY(dev) statistics
+ * @ETH_SS_TS_FLAGS: hardware timestamping flags
  *
  * @ETH_SS_COUNT: number of defined string sets
  */
@@ -708,6 +709,7 @@ enum ethtool_stringset {
 	ETH_SS_STATS_ETH_CTRL,
 	ETH_SS_STATS_RMON,
 	ETH_SS_STATS_PHY,
+	ETH_SS_TS_FLAGS,
 
 	/* add new constants above here */
 	ETH_SS_COUNT
diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index 2bd77c94f9f1..d88e9080643b 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -462,6 +462,11 @@ const char ts_rx_filter_names[][ETH_GSTRING_LEN] = {
 };
 static_assert(ARRAY_SIZE(ts_rx_filter_names) == __HWTSTAMP_FILTER_CNT);
 
+const char ts_flags_names[][ETH_GSTRING_LEN] = {
+	[const_ilog2(HWTSTAMP_FLAG_BONDED_PHC_INDEX)] = "bonded-phc-index",
+};
+static_assert(ARRAY_SIZE(ts_flags_names) == __HWTSTAMP_FLAG_CNT);
+
 const char udp_tunnel_type_names[][ETH_GSTRING_LEN] = {
 	[ETHTOOL_UDP_TUNNEL_TYPE_VXLAN]		= "vxlan",
 	[ETHTOOL_UDP_TUNNEL_TYPE_GENEVE]	= "geneve",
diff --git a/net/ethtool/common.h b/net/ethtool/common.h
index 850eadde4bfc..58e9e7db06f9 100644
--- a/net/ethtool/common.h
+++ b/net/ethtool/common.h
@@ -13,6 +13,7 @@
 	ETHTOOL_LINK_MODE_ ## speed ## base ## type ## _ ## duplex ## _BIT
 
 #define __SOF_TIMESTAMPING_CNT (const_ilog2(SOF_TIMESTAMPING_LAST) + 1)
+#define __HWTSTAMP_FLAG_CNT (const_ilog2(HWTSTAMP_FLAG_LAST) + 1)
 
 struct link_mode_info {
 	int				speed;
@@ -38,6 +39,7 @@ extern const char wol_mode_names[][ETH_GSTRING_LEN];
 extern const char sof_timestamping_names[][ETH_GSTRING_LEN];
 extern const char ts_tx_type_names[][ETH_GSTRING_LEN];
 extern const char ts_rx_filter_names[][ETH_GSTRING_LEN];
+extern const char ts_flags_names[][ETH_GSTRING_LEN];
 extern const char udp_tunnel_type_names[][ETH_GSTRING_LEN];
 
 int __ethtool_get_link(struct net_device *dev);
diff --git a/net/ethtool/strset.c b/net/ethtool/strset.c
index 818cf01f0911..6b76c05caba4 100644
--- a/net/ethtool/strset.c
+++ b/net/ethtool/strset.c
@@ -75,6 +75,11 @@ static const struct strset_info info_template[] = {
 		.count		= __HWTSTAMP_FILTER_CNT,
 		.strings	= ts_rx_filter_names,
 	},
+	[ETH_SS_TS_FLAGS] = {
+		.per_dev	= false,
+		.count		= __HWTSTAMP_FLAG_CNT,
+		.strings	= ts_flags_names,
+	},
 	[ETH_SS_UDP_TUNNEL_TYPES] = {
 		.per_dev	= false,
 		.count		= __ETHTOOL_UDP_TUNNEL_TYPE_CNT,
diff --git a/net/ethtool/tsconfig.c b/net/ethtool/tsconfig.c
index 2dc3a3b76615..4e8fd4f5c27b 100644
--- a/net/ethtool/tsconfig.c
+++ b/net/ethtool/tsconfig.c
@@ -54,7 +54,7 @@ static int tsconfig_prepare_data(const struct ethnl_req_info *req_base,
 
 	data->hwtst_config.tx_type = BIT(cfg.tx_type);
 	data->hwtst_config.rx_filter = BIT(cfg.rx_filter);
-	data->hwtst_config.flags = BIT(cfg.flags);
+	data->hwtst_config.flags = cfg.flags;
 
 	data->hwprov_desc.index = -1;
 	hwprov = rtnl_dereference(dev->hwprov);
@@ -91,10 +91,16 @@ static int tsconfig_reply_size(const struct ethnl_req_info *req_base,
 
 	BUILD_BUG_ON(__HWTSTAMP_TX_CNT > 32);
 	BUILD_BUG_ON(__HWTSTAMP_FILTER_CNT > 32);
+	BUILD_BUG_ON(__HWTSTAMP_FLAG_CNT > 32);
 
-	if (data->hwtst_config.flags)
-		/* _TSCONFIG_HWTSTAMP_FLAGS */
-		len += nla_total_size(sizeof(u32));
+	if (data->hwtst_config.flags) {
+		ret = ethnl_bitset32_size(&data->hwtst_config.flags,
+					  NULL, __HWTSTAMP_FLAG_CNT,
+					  ts_flags_names, compact);
+		if (ret < 0)
+			return ret;
+		len += ret;	/* _TSCONFIG_HWTSTAMP_FLAGS */
+	}
 
 	if (data->hwtst_config.tx_type) {
 		ret = ethnl_bitset32_size(&data->hwtst_config.tx_type,
@@ -130,8 +136,10 @@ static int tsconfig_fill_reply(struct sk_buff *skb,
 	int ret;
 
 	if (data->hwtst_config.flags) {
-		ret = nla_put_u32(skb, ETHTOOL_A_TSCONFIG_HWTSTAMP_FLAGS,
-				  data->hwtst_config.flags);
+		ret = ethnl_put_bitset32(skb, ETHTOOL_A_TSCONFIG_HWTSTAMP_FLAGS,
+					 &data->hwtst_config.flags, NULL,
+					 __HWTSTAMP_FLAG_CNT,
+					 ts_flags_names, compact);
 		if (ret < 0)
 			return ret;
 	}
@@ -180,7 +188,7 @@ const struct nla_policy ethnl_tsconfig_set_policy[ETHTOOL_A_TSCONFIG_MAX + 1] =
 	[ETHTOOL_A_TSCONFIG_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy),
 	[ETHTOOL_A_TSCONFIG_HWTSTAMP_PROVIDER] =
 		NLA_POLICY_NESTED(ethnl_ts_hwtst_prov_policy),
-	[ETHTOOL_A_TSCONFIG_HWTSTAMP_FLAGS] = { .type = NLA_U32 },
+	[ETHTOOL_A_TSCONFIG_HWTSTAMP_FLAGS] = { .type = NLA_NESTED },
 	[ETHTOOL_A_TSCONFIG_RX_FILTERS] = { .type = NLA_NESTED },
 	[ETHTOOL_A_TSCONFIG_TX_TYPES] = { .type = NLA_NESTED },
 };
@@ -296,6 +304,7 @@ static int ethnl_set_tsconfig(struct ethnl_req_info *req_base,
 
 	BUILD_BUG_ON(__HWTSTAMP_TX_CNT > 32);
 	BUILD_BUG_ON(__HWTSTAMP_FILTER_CNT > 32);
+	BUILD_BUG_ON(__HWTSTAMP_FLAG_CNT > 32);
 
 	if (!netif_device_present(dev))
 		return -ENODEV;
@@ -377,9 +386,13 @@ static int ethnl_set_tsconfig(struct ethnl_req_info *req_base,
 	}
 
 	if (tb[ETHTOOL_A_TSCONFIG_HWTSTAMP_FLAGS]) {
-		ethnl_update_u32(&hwtst_config.flags,
-				 tb[ETHTOOL_A_TSCONFIG_HWTSTAMP_FLAGS],
-				 &config_mod);
+		ret = ethnl_update_bitset32(&hwtst_config.flags,
+					    __HWTSTAMP_FLAG_CNT,
+					    tb[ETHTOOL_A_TSCONFIG_HWTSTAMP_FLAGS],
+					    ts_flags_names, info->extack,
+					    &config_mod);
+		if (ret < 0)
+			goto err_free_hwprov;
 	}
 
 	ret = net_hwtstamp_validate(&hwtst_config);

-- 
2.34.1

Re: [PATCH net 3/3] net: ethtool: tsconfig: Fix netlink type of hwtstamp flags

[Jakub Kicinski]

On Tue, 28 Jan 2025 16:35:48 +0100 Kory Maincent wrote:
> Fix the netlink type for hardware timestamp flags, which are represented
> as a bitset of flags. Although only one flag is supported currently, the
> correct netlink bitset type should be used instead of u32. Address this
> by adding a new named string set description for the hwtstamp flag
> structure.

Makes sense, please mention explicitly in the commit message that the
code has been introduced in the current release so the uAPI change is
still okay.

In general IMHO YNL makes the bitset functionality less important.
But in this case consistency with other fields seems worth it.
The patch LGTM.

Re: [PATCH net 3/3] net: ethtool: tsconfig: Fix netlink type of hwtstamp flags

[Kory Maincent]

On Wed, 29 Jan 2025 16:49:07 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Tue, 28 Jan 2025 16:35:48 +0100 Kory Maincent wrote:
> > Fix the netlink type for hardware timestamp flags, which are represented
> > as a bitset of flags. Although only one flag is supported currently, the
> > correct netlink bitset type should be used instead of u32. Address this
> > by adding a new named string set description for the hwtstamp flag
> > structure.  
> 
> Makes sense, please mention explicitly in the commit message that the
> code has been introduced in the current release so the uAPI change is
> still okay.

Ack.

> In general IMHO YNL makes the bitset functionality less important.

Do you mean you prefer u32 for bitfield instead of the bitset type? Why?

-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net 3/3] net: ethtool: tsconfig: Fix netlink type of hwtstamp flags

[Jakub Kicinski]

On Thu, 30 Jan 2025 10:27:16 +0100 Kory Maincent wrote:
> > In general IMHO YNL makes the bitset functionality less important.  
> 
> Do you mean you prefer u32 for bitfield instead of the bitset type? Why?

Not in this case, but in general uint can carry up to 64 bits, 
and the names of the fields are in the spec (both Python and 
C code automatically decode them based on the spec). I see less
of a need for the complex "forward compatibility" handling
in the kernel because of that.