diff mbox series

[net-next,v5,15/16] net ethtool: net: Let the active time stamping layer be selectable

Message ID 20231009155138.86458-16-kory.maincent@bootlin.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: Make timestamping selectable | expand

Checks

Context Check Description
netdev/series_format fail Series longer than 15 patches (and no cover letter)
netdev/tree_selection success Clearly marked for net-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 21 this patch: 21
netdev/cc_maintainers success CCed 10 of 10 maintainers
netdev/build_clang fail Errors and warnings before: 22 this patch: 22
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 23 this patch: 23
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 145 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Kory Maincent Oct. 9, 2023, 3:51 p.m. UTC 
From: Kory Maincent <kory.maincent@bootlin.com>

Now that the current timestamp is saved in a variable lets add the
ETHTOOL_MSG_TS_SET ethtool netlink socket to make it selectable.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---

Changes in v2:
- Move selected_timestamping_layer introduction in this patch.
- Replace strmcmp by sysfs_streq.
- Use the PHY timestamp only if available.

Changes in v3:
- Add a devicetree binding to select the preferred timestamp
- Replace the way to select timestamp through ethtool instead of sysfs
You can test it with the ethtool source on branch feature_ptp of:
https://github.com/kmaincent/ethtool

Changes in v4:
- Change the API to select MAC default time stamping instead of the PHY.
- Add a whitelist to no break the old timestamp PHY default preferences
  for current PHY.
- Replace the ethtool ioctl by netlink.
- Add a netdev notifier to allow network device to create trap on PTP
  packets. Not tested as it need to change the lan966x driver that
  implement packet traps. I will do after the hwtstamp management change
  to NDOs.

Changes in v5:
- Remove the netdev notifier added in v4.
- Extract the default timestamp API change in another patch.
---
 Documentation/networking/ethtool-netlink.rst | 17 +++++
 include/uapi/linux/ethtool_netlink.h         |  1 +
 net/ethtool/netlink.c                        |  8 +++
 net/ethtool/netlink.h                        |  1 +
 net/ethtool/ts.c                             | 66 ++++++++++++++++++++
 5 files changed, 93 insertions(+)

Comments

Florian Fainelli Oct. 9, 2023, 9:28 p.m. UTC | #1 
On 10/9/23 08:51, Köry Maincent wrote:
> From: Kory Maincent <kory.maincent@bootlin.com>
> 
> Now that the current timestamp is saved in a variable lets add the
> ETHTOOL_MSG_TS_SET ethtool netlink socket to make it selectable.
> 
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> ---

[snip]

> +static int ethnl_set_ts_validate(struct ethnl_req_info *req_info,
> +				 struct genl_info *info)
> +{
> +	struct nlattr **tb = info->attrs;
> +	const struct net_device_ops *ops = req_info->dev->netdev_ops;
> +
> +	if (!tb[ETHTOOL_A_TS_LAYER])
> +		return 0;
> +
> +	if (!ops->ndo_hwtstamp_set)
> +		return -EOPNOTSUPP;

I would check for this first, in all likelihood this is what most 
drivers currently do not support, no need to event de-reference the 
array of attributes.

> +
> +	return 1;
> +}
> +
> +static int ethnl_set_ts(struct ethnl_req_info *req_info, struct genl_info *info)
> +{
> +	struct net_device *dev = req_info->dev;
> +	const struct ethtool_ops *ops = dev->ethtool_ops;
> +	struct kernel_hwtstamp_config config = {0};
> +	struct nlattr **tb = info->attrs;
> +	bool mod = false;
> +	u32 ts_layer;
> +	int ret;
> +
> +	ts_layer = dev->ts_layer;
> +	ethnl_update_u32(&ts_layer, tb[ETHTOOL_A_TS_LAYER], &mod);
> +
> +	if (!mod)
> +		return 0;
> +
> +	if (ts_layer & NETDEV_TIMESTAMPING && !ops->get_ts_info) {
> +		NL_SET_ERR_MSG_ATTR(info->extack, tb[ETHTOOL_A_TS_LAYER],
> +				    "this device cannot support timestamping");

Maybe expand the extended ack with "this devices does not support 
MAC-based timestamping"

> +		return -EINVAL;
> +	}
> +
> +	if (ts_layer & PHYLIB_TIMESTAMPING && !phy_has_tsinfo(dev->phydev)) {
> +		NL_SET_ERR_MSG_ATTR(info->extack, tb[ETHTOOL_A_TS_LAYER],
> +				    "this device cannot support timestamping");

Likewise, detail which kind of timestamping is not supported.

> +		return -EINVAL;
> +	}
> +
> +	/* Disable time stamping in the current layer. */
> +	if (netif_device_present(dev) &&
> +	    dev->ts_layer & (PHYLIB_TIMESTAMPING | NETDEV_TIMESTAMPING)) {
> +		ret = dev_set_hwtstamp_phylib(dev, &config, info->extack);

Can we still land in this function even if no changes to the 
timestamping configuration has been made? If so, would suggest first 
getting the current configuration and compare it with the user-supplied 
configuration if there are no changes, return.

> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	dev->ts_layer = ts_layer;
> +
> +	return 1;
> +}
> +
>   const struct ethnl_request_ops ethnl_ts_request_ops = {
>   	.request_cmd		= ETHTOOL_MSG_TS_GET,
>   	.reply_cmd		= ETHTOOL_MSG_TS_GET_REPLY,
> @@ -69,6 +132,9 @@ const struct ethnl_request_ops ethnl_ts_request_ops = {
>   	.prepare_data		= ts_prepare_data,
>   	.reply_size		= ts_reply_size,
>   	.fill_reply		= ts_fill_reply,
> +
> +	.set_validate		= ethnl_set_ts_validate,
> +	.set			= ethnl_set_ts,
>   };
>   
>   /* TS_LIST_GET */
Kory Maincent Oct. 10, 2023, 8:31 a.m. UTC | #2 
On Mon, 9 Oct 2023 14:28:38 -0700
Florian Fainelli <florian.fainelli@broadcom.com> wrote:

> > +static int ethnl_set_ts_validate(struct ethnl_req_info *req_info,
> > +				 struct genl_info *info)
> > +{
> > +	struct nlattr **tb = info->attrs;
> > +	const struct net_device_ops *ops = req_info->dev->netdev_ops;
> > +
> > +	if (!tb[ETHTOOL_A_TS_LAYER])
> > +		return 0;
> > +
> > +	if (!ops->ndo_hwtstamp_set)
> > +		return -EOPNOTSUPP;  
> 
> I would check for this first, in all likelihood this is what most 
> drivers currently do not support, no need to event de-reference the 
> array of attributes.

Indeed seems more logical.

> > +static int ethnl_set_ts(struct ethnl_req_info *req_info, struct genl_info
> > *info) +{
> > +	struct net_device *dev = req_info->dev;
> > +	const struct ethtool_ops *ops = dev->ethtool_ops;
> > +	struct kernel_hwtstamp_config config = {0};
> > +	struct nlattr **tb = info->attrs;
> > +	bool mod = false;
> > +	u32 ts_layer;
> > +	int ret;
> > +
> > +	ts_layer = dev->ts_layer;

> > +
> > +	if (ts_layer & NETDEV_TIMESTAMPING && !ops->get_ts_info) {
> > +		NL_SET_ERR_MSG_ATTR(info->extack, tb[ETHTOOL_A_TS_LAYER],
> > +				    "this device cannot support
> > timestamping");  
> 
> Maybe expand the extended ack with "this devices does not support 
> MAC-based timestamping"

Ok.

> > +	/* Disable time stamping in the current layer. */
> > +	if (netif_device_present(dev) &&
> > +	    dev->ts_layer & (PHYLIB_TIMESTAMPING | NETDEV_TIMESTAMPING)) {
> > +		ret = dev_set_hwtstamp_phylib(dev, &config, info->extack);
> >  
> 
> Can we still land in this function even if no changes to the 
> timestamping configuration has been made? 

We land in this function every time we change the timestamp from a valid
one.  

> If so, would suggest first 
> getting the current configuration and compare it with the user-supplied 
> configuration if there are no changes, return.

It is already done at the beginning of the function:
> > +	ethnl_update_u32(&ts_layer, tb[ETHTOOL_A_TS_LAYER], &mod);
> > +
> > +	if (!mod)
> > +		return 0;
diff mbox series

Patch

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index 963a5aacac8d..eb7f8592986b 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -227,6 +227,7 @@  Userspace to kernel:
   ``ETHTOOL_MSG_MM_SET``                set MAC merge layer parameters
   ``ETHTOOL_MSG_TS_GET``                get current timestamping
   ``ETHTOOL_MSG_TS_LIST_GET``            list available timestampings
+  ``ETHTOOL_MSG_TS_SET``                set current timestamping
   ===================================== =================================
 
 Kernel to userspace:
@@ -2038,6 +2039,21 @@  Kernel response contents:
 
 This command lists all the possible timestamp layer available.
 
+TS_SET
+======
+
+Modify the selected timestamping.
+
+Request contents:
+
+  =======================  ======  ===================
+  ``ETHTOOL_A_TS_HEADER``  nested  reply header
+  ``ETHTOOL_A_TS_LAYER``   u32     timestamping
+  =======================  ======  ===================
+
+This command set the timestamping with one that should be listed by the
+TSLIST_GET command.
+
 Request translation
 ===================
 
@@ -2146,4 +2162,5 @@  are netlink only.
   n/a                                 ``ETHTOOL_MSG_MM_SET``
   n/a                                 ``ETHTOOL_MSG_TSLIST_GET``
   n/a                                 ``ETHTOOL_MSG_TS_GET``
+  n/a                                 ``ETHTOOL_MSG_TS_SET``
   =================================== =====================================
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 62b885d44d06..df6c4fcc62c1 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -59,6 +59,7 @@  enum {
 	ETHTOOL_MSG_MM_SET,
 	ETHTOOL_MSG_TS_GET,
 	ETHTOOL_MSG_TS_LIST_GET,
+	ETHTOOL_MSG_TS_SET,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_USER_CNT,
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 842c9db1531f..8322bf71f80d 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -308,6 +308,7 @@  ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = {
 	[ETHTOOL_MSG_MM_SET]		= &ethnl_mm_request_ops,
 	[ETHTOOL_MSG_TS_GET]		= &ethnl_ts_request_ops,
 	[ETHTOOL_MSG_TS_LIST_GET]	= &ethnl_ts_list_request_ops,
+	[ETHTOOL_MSG_TS_SET]		= &ethnl_ts_request_ops,
 };
 
 static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback *cb)
@@ -1148,6 +1149,13 @@  static const struct genl_ops ethtool_genl_ops[] = {
 		.policy = ethnl_ts_get_policy,
 		.maxattr = ARRAY_SIZE(ethnl_ts_get_policy) - 1,
 	},
+	{
+		.cmd	= ETHTOOL_MSG_TS_SET,
+		.flags	= GENL_UNS_ADMIN_PERM,
+		.doit	= ethnl_default_set_doit,
+		.policy = ethnl_ts_set_policy,
+		.maxattr = ARRAY_SIZE(ethnl_ts_set_policy) - 1,
+	},
 };
 
 static const struct genl_multicast_group ethtool_nl_mcgrps[] = {
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index ea8c312db3af..8fedf234b824 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -444,6 +444,7 @@  extern const struct nla_policy ethnl_plca_get_status_policy[ETHTOOL_A_PLCA_HEADE
 extern const struct nla_policy ethnl_mm_get_policy[ETHTOOL_A_MM_HEADER + 1];
 extern const struct nla_policy ethnl_mm_set_policy[ETHTOOL_A_MM_MAX + 1];
 extern const struct nla_policy ethnl_ts_get_policy[ETHTOOL_A_TS_HEADER + 1];
+extern const struct nla_policy ethnl_ts_set_policy[ETHTOOL_A_TS_MAX + 1];
 
 int ethnl_set_features(struct sk_buff *skb, struct genl_info *info);
 int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info);
diff --git a/net/ethtool/ts.c b/net/ethtool/ts.c
index 11041a16de5b..f77297965e03 100644
--- a/net/ethtool/ts.c
+++ b/net/ethtool/ts.c
@@ -59,6 +59,69 @@  static int ts_fill_reply(struct sk_buff *skb,
 	return nla_put_u32(skb, ETHTOOL_A_TS_LAYER, data->ts_layer);
 }
 
+/* TS_SET */
+const struct nla_policy ethnl_ts_set_policy[] = {
+	[ETHTOOL_A_TS_HEADER]	= NLA_POLICY_NESTED(ethnl_header_policy),
+	[ETHTOOL_A_TS_LAYER]	= NLA_POLICY_RANGE(NLA_U32, 0,
+						   __TIMESTAMPING_COUNT - 1)
+};
+
+static int ethnl_set_ts_validate(struct ethnl_req_info *req_info,
+				 struct genl_info *info)
+{
+	struct nlattr **tb = info->attrs;
+	const struct net_device_ops *ops = req_info->dev->netdev_ops;
+
+	if (!tb[ETHTOOL_A_TS_LAYER])
+		return 0;
+
+	if (!ops->ndo_hwtstamp_set)
+		return -EOPNOTSUPP;
+
+	return 1;
+}
+
+static int ethnl_set_ts(struct ethnl_req_info *req_info, struct genl_info *info)
+{
+	struct net_device *dev = req_info->dev;
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	struct kernel_hwtstamp_config config = {0};
+	struct nlattr **tb = info->attrs;
+	bool mod = false;
+	u32 ts_layer;
+	int ret;
+
+	ts_layer = dev->ts_layer;
+	ethnl_update_u32(&ts_layer, tb[ETHTOOL_A_TS_LAYER], &mod);
+
+	if (!mod)
+		return 0;
+
+	if (ts_layer & NETDEV_TIMESTAMPING && !ops->get_ts_info) {
+		NL_SET_ERR_MSG_ATTR(info->extack, tb[ETHTOOL_A_TS_LAYER],
+				    "this device cannot support timestamping");
+		return -EINVAL;
+	}
+
+	if (ts_layer & PHYLIB_TIMESTAMPING && !phy_has_tsinfo(dev->phydev)) {
+		NL_SET_ERR_MSG_ATTR(info->extack, tb[ETHTOOL_A_TS_LAYER],
+				    "this device cannot support timestamping");
+		return -EINVAL;
+	}
+
+	/* Disable time stamping in the current layer. */
+	if (netif_device_present(dev) &&
+	    dev->ts_layer & (PHYLIB_TIMESTAMPING | NETDEV_TIMESTAMPING)) {
+		ret = dev_set_hwtstamp_phylib(dev, &config, info->extack);
+		if (ret < 0)
+			return ret;
+	}
+
+	dev->ts_layer = ts_layer;
+
+	return 1;
+}
+
 const struct ethnl_request_ops ethnl_ts_request_ops = {
 	.request_cmd		= ETHTOOL_MSG_TS_GET,
 	.reply_cmd		= ETHTOOL_MSG_TS_GET_REPLY,
@@ -69,6 +132,9 @@  const struct ethnl_request_ops ethnl_ts_request_ops = {
 	.prepare_data		= ts_prepare_data,
 	.reply_size		= ts_reply_size,
 	.fill_reply		= ts_fill_reply,
+
+	.set_validate		= ethnl_set_ts_validate,
+	.set			= ethnl_set_ts,
 };
 
 /* TS_LIST_GET */