[PATCH net-next RFC v4 0/5] net: Make MAC/PHY time stamping selectable

[PATCH net-next RFC v4 0/5] net: Make MAC/PHY time stamping selectable

[Köry Maincent]

From: Kory Maincent <kory.maincent@bootlin.com>

Up until now, there was no way to let the user select the layer at
which time stamping occurs.  The stack assumed that PHY time stamping
is always preferred, but some MAC/PHY combinations were buggy.

This series aims to allow the user to select the desired layer
administratively.

- Patch 1 refactors get_ts_info copy/paste code.

- Patch 2 introduces sysfs files that reflect the current, static
  preference of PHY over MAC.

- Patch 3 add devicetree binding to select the default time stamping.

- Patch 4 makes the layer selectable at run time.

- Patch 5 fixes up MAC drivers that attempt to defer to the PHY layer.
  This patch is broken out for review, but it will eventually be
  squashed into Patch 4 after comments come in.

Changes in v2:
- Move selected_timestamping_layer variable of the concerned patch.
- Use sysfs_streq instead of strmcmp.
- Use the PHY timestamp only if available.

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

Changes in v4:
- Move on to ethtool netlink instead of ioctl.
- Add a netdev notifier to allow packet trapping by the MAC in case of PHY
  time stamping.
- Add a PHY whitelist to not break the old PHY default time-stamping
  preference API.

Kory Maincent (3):
  net: Expose available time stamping layers to user space.
  dt-bindings: net: phy: add timestamp preferred choice property
  net: Let the active time stamping layer be selectable.

Richard Cochran (2):
  net: ethtool: Refactor identical get_ts_info implementations.
  net: fix up drivers WRT phy time stamping

 .../devicetree/bindings/net/ethernet-phy.yaml |   7 +
 Documentation/networking/ethtool-netlink.rst  |  53 ++++++
 drivers/net/bonding/bond_main.c               |  14 +-
 drivers/net/ethernet/freescale/fec_main.c     |  23 ++-
 drivers/net/ethernet/mscc/ocelot_net.c        |  21 +--
 drivers/net/ethernet/ti/cpsw_priv.c           |  12 +-
 drivers/net/ethernet/ti/netcp_ethss.c         |  26 +--
 drivers/net/macvlan.c                         |  14 +-
 drivers/net/phy/phy_device.c                  |  85 +++++++++
 include/linux/ethtool.h                       |   8 +
 include/linux/netdevice.h                     |  12 ++
 include/uapi/linux/ethtool_netlink.h          |  17 ++
 include/uapi/linux/net_tstamp.h               |   8 +
 net/8021q/vlan_dev.c                          |  15 +-
 net/core/dev.c                                |   2 +-
 net/core/dev_ioctl.c                          |  56 +++++-
 net/core/timestamping.c                       |   6 +
 net/ethtool/Makefile                          |   2 +-
 net/ethtool/common.c                          |  21 ++-
 net/ethtool/netlink.c                         |  30 ++++
 net/ethtool/netlink.h                         |   4 +
 net/ethtool/ts.c                              | 168 ++++++++++++++++++
 22 files changed, 506 insertions(+), 98 deletions(-)
 create mode 100644 net/ethtool/ts.c

-- 
2.25.1

[PATCH net-next RFC v4 1/5] net: ethtool: Refactor identical get_ts_info implementations.

[Köry Maincent]

From: Richard Cochran <richardcochran@gmail.com>

The vlan, macvlan and the bonding drivers call their "real" device driver
in order to report the time stamping capabilities.  Provide a core
ethtool helper function to avoid copy/paste in the stack.

Signed-off-by: Richard Cochran <richardcochran@gmail.com>
Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 drivers/net/bonding/bond_main.c | 14 ++------------
 drivers/net/macvlan.c           | 14 +-------------
 include/linux/ethtool.h         |  8 ++++++++
 net/8021q/vlan_dev.c            | 15 +--------------
 net/ethtool/common.c            |  6 ++++++
 5 files changed, 18 insertions(+), 39 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 236e5219c811..322fef637059 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -5695,9 +5695,7 @@ static int bond_ethtool_get_ts_info(struct net_device *bond_dev,
 				    struct ethtool_ts_info *info)
 {
 	struct bonding *bond = netdev_priv(bond_dev);
-	const struct ethtool_ops *ops;
 	struct net_device *real_dev;
-	struct phy_device *phydev;
 	int ret = 0;
 
 	rcu_read_lock();
@@ -5706,16 +5704,8 @@ static int bond_ethtool_get_ts_info(struct net_device *bond_dev,
 	rcu_read_unlock();
 
 	if (real_dev) {
-		ops = real_dev->ethtool_ops;
-		phydev = real_dev->phydev;
-
-		if (phy_has_tsinfo(phydev)) {
-			ret = phy_ts_info(phydev, info);
-			goto out;
-		} else if (ops->get_ts_info) {
-			ret = ops->get_ts_info(real_dev, info);
-			goto out;
-		}
+		ret = ethtool_get_ts_info_by_layer(real_dev, info);
+		goto out;
 	}
 
 	info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |
diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 4a53debf9d7c..a396a35fec2e 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -1093,20 +1093,8 @@ static int macvlan_ethtool_get_ts_info(struct net_device *dev,
 				       struct ethtool_ts_info *info)
 {
 	struct net_device *real_dev = macvlan_dev_real_dev(dev);
-	const struct ethtool_ops *ops = real_dev->ethtool_ops;
-	struct phy_device *phydev = real_dev->phydev;
 
-	if (phy_has_tsinfo(phydev)) {
-		return phy_ts_info(phydev, info);
-	} else if (ops->get_ts_info) {
-		return ops->get_ts_info(real_dev, info);
-	} else {
-		info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |
-			SOF_TIMESTAMPING_SOFTWARE;
-		info->phc_index = -1;
-	}
-
-	return 0;
+	return ethtool_get_ts_info_by_layer(real_dev, info);
 }
 
 static netdev_features_t macvlan_fix_features(struct net_device *dev,
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 798d35890118..a21302032dfa 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -1042,6 +1042,14 @@ static inline int ethtool_mm_frag_size_min_to_add(u32 val_min, u32 *val_add,
 	return -EINVAL;
 }
 
+/**
+ * ethtool_get_ts_info_by_layer - Obtains time stamping capabilities from the MAC or PHY layer.
+ * @dev: pointer to net_device structure
+ * @info: buffer to hold the result
+ * Returns zero on sauces, non-zero otherwise.
+ */
+int ethtool_get_ts_info_by_layer(struct net_device *dev, struct ethtool_ts_info *info);
+
 /**
  * ethtool_sprintf - Write formatted string to ethtool string data
  * @data: Pointer to start of string to update
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 5920544e93e8..7c33c7216f35 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -683,20 +683,7 @@ static int vlan_ethtool_get_ts_info(struct net_device *dev,
 				    struct ethtool_ts_info *info)
 {
 	const struct vlan_dev_priv *vlan = vlan_dev_priv(dev);
-	const struct ethtool_ops *ops = vlan->real_dev->ethtool_ops;
-	struct phy_device *phydev = vlan->real_dev->phydev;
-
-	if (phy_has_tsinfo(phydev)) {
-		return phy_ts_info(phydev, info);
-	} else if (ops->get_ts_info) {
-		return ops->get_ts_info(vlan->real_dev, info);
-	} else {
-		info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |
-			SOF_TIMESTAMPING_SOFTWARE;
-		info->phc_index = -1;
-	}
-
-	return 0;
+	return ethtool_get_ts_info_by_layer(vlan->real_dev, info);
 }
 
 static void vlan_dev_get_stats64(struct net_device *dev,
diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index 5fb19050991e..695c7c4a816b 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -661,6 +661,12 @@ int ethtool_get_phc_vclocks(struct net_device *dev, int **vclock_index)
 }
 EXPORT_SYMBOL(ethtool_get_phc_vclocks);
 
+int ethtool_get_ts_info_by_layer(struct net_device *dev, struct ethtool_ts_info *info)
+{
+	return __ethtool_get_ts_info(dev, info);
+}
+EXPORT_SYMBOL(ethtool_get_ts_info_by_layer);
+
 const struct ethtool_phy_ops *ethtool_phy_ops;
 
 void ethtool_set_ethtool_phy_ops(const struct ethtool_phy_ops *ops)
-- 
2.25.1

Re: [PATCH net-next RFC v4 1/5] net: ethtool: Refactor identical get_ts_info implementations.

[Vladimir Oltean]

On Thu, Apr 06, 2023 at 07:33:04PM +0200, Köry Maincent wrote:
> From: Richard Cochran <richardcochran@gmail.com>
> 
> The vlan, macvlan and the bonding drivers call their "real" device driver
> in order to report the time stamping capabilities.  Provide a core
> ethtool helper function to avoid copy/paste in the stack.
> 
> Signed-off-by: Richard Cochran <richardcochran@gmail.com>
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> ---
>  static netdev_features_t macvlan_fix_features(struct net_device *dev,
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 798d35890118..a21302032dfa 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -1042,6 +1042,14 @@ static inline int ethtool_mm_frag_size_min_to_add(u32 val_min, u32 *val_add,
>  	return -EINVAL;
>  }
>  
> +/**
> + * ethtool_get_ts_info_by_layer - Obtains time stamping capabilities from the MAC or PHY layer.
> + * @dev: pointer to net_device structure
> + * @info: buffer to hold the result
> + * Returns zero on sauces, non-zero otherwise.

Not sure if I'm missing some joke with the sauces here.

> + */
> +int ethtool_get_ts_info_by_layer(struct net_device *dev, struct ethtool_ts_info *info);
> +
>  /**
>   * ethtool_sprintf - Write formatted string to ethtool string data
>   * @data: Pointer to start of string to update

Re: [PATCH net-next RFC v4 1/5] net: ethtool: Refactor identical get_ts_info implementations.

[Köry Maincent]

On Wed, 12 Apr 2023 16:16:36 +0300
Vladimir Oltean <vladimir.oltean@nxp.com> wrote:

> On Thu, Apr 06, 2023 at 07:33:04PM +0200, Köry Maincent wrote:
> > From: Richard Cochran <richardcochran@gmail.com>
> > 
> > The vlan, macvlan and the bonding drivers call their "real" device driver
> > in order to report the time stamping capabilities.  Provide a core
> > ethtool helper function to avoid copy/paste in the stack.
> > 
> > Signed-off-by: Richard Cochran <richardcochran@gmail.com>
> > Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> > ---
> >  static netdev_features_t macvlan_fix_features(struct net_device *dev,
> > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> > index 798d35890118..a21302032dfa 100644
> > --- a/include/linux/ethtool.h
> > +++ b/include/linux/ethtool.h
> > @@ -1042,6 +1042,14 @@ static inline int
> > ethtool_mm_frag_size_min_to_add(u32 val_min, u32 *val_add, return -EINVAL;
> >  }
> >  
> > +/**
> > + * ethtool_get_ts_info_by_layer - Obtains time stamping capabilities from
> > the MAC or PHY layer.
> > + * @dev: pointer to net_device structure
> > + * @info: buffer to hold the result
> > + * Returns zero on sauces, non-zero otherwise.  
> 
> Not sure if I'm missing some joke with the sauces here.

mmh don't know. Richards it comes from your code, any joke here?
I think it is merely a brain fart. I will fix it.

Re: [PATCH net-next RFC v4 1/5] net: ethtool: Refactor identical get_ts_info implementations.

[Richard Cochran]

On Wed, Apr 12, 2023 at 03:49:58PM +0200, Köry Maincent wrote:
> mmh don't know. Richards it comes from your code, any joke here?

No, just a typo.

Thanks,
Richard

[PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Köry Maincent]

From: Kory Maincent <kory.maincent@bootlin.com>

Time stamping on network packets may happen either in the MAC or in
the PHY, but not both.  In preparation for making the choice
selectable, expose both the current and available layers via ethtool.

In accordance with the kernel implementation as it stands, the current
layer will always read as "phy" when a PHY time stamping device is
present.  Future patches will allow changing the current layer
administratively.

Signed-off-by: Richard Cochran <richardcochran@gmail.com>
Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---

Notes:
    Changes in v2:
    - Move the introduction of selected_timestamping_layer variable in next
      patch.
    
    Changes in v3:
    - Move on to ethtool instead of syfs
    
    Changes in v4:
    - Move on to netlink ethtool instead of ioctl. I am not familiar with
      netlink so there might be some code that does not follow the good code
      practice.

 Documentation/networking/ethtool-netlink.rst |  40 +++++++
 include/uapi/linux/ethtool_netlink.h         |  15 +++
 include/uapi/linux/net_tstamp.h              |   8 ++
 net/ethtool/Makefile                         |   2 +-
 net/ethtool/netlink.c                        |  20 ++++
 net/ethtool/netlink.h                        |   3 +
 net/ethtool/ts.c                             | 114 +++++++++++++++++++
 7 files changed, 201 insertions(+), 1 deletion(-)
 create mode 100644 net/ethtool/ts.c

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index cd0973d4ba01..539425fdaf7c 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -210,6 +210,8 @@ Userspace to kernel:
   ``ETHTOOL_MSG_EEE_GET``               get EEE settings
   ``ETHTOOL_MSG_EEE_SET``               set EEE settings
   ``ETHTOOL_MSG_TSINFO_GET``		get timestamping info
+  ``ETHTOOL_MSG_TS_GET``                get current hardware timestamping
+  ``ETHTOOL_MSG_TSLIST_GET``            list available hardware timestamping
   ``ETHTOOL_MSG_CABLE_TEST_ACT``        action start cable test
   ``ETHTOOL_MSG_CABLE_TEST_TDR_ACT``    action start raw TDR cable test
   ``ETHTOOL_MSG_TUNNEL_INFO_GET``       get tunnel offload info
@@ -1990,6 +1992,42 @@ The attributes are propagated to the driver through the following structure:
 .. kernel-doc:: include/linux/ethtool.h
     :identifiers: ethtool_mm_cfg
 
+TS_GET
+======
+
+Gets transceiver module parameters.
+
+Request contents:
+
+  =================================  ======  ==========================
+  ``ETHTOOL_A_TS_HEADER``            nested  request header
+  =================================  ======  ==========================
+
+Kernel response contents:
+
+  =======================  ======  ====================================
+  ``ETHTOOL_A_TS_HEADER``  nested  reply header
+  ``ETHTOOL_A_TS_LAYER``   u32     current hardware timestamping
+  =======================  ======  ====================================
+
+TSLIST_GET
+==========
+
+Gets transceiver module parameters.
+
+Request contents:
+
+  =================================  ======  ==========================
+  ``ETHTOOL_A_TS_HEADER``            nested  request header
+  =================================  ======  ==========================
+
+Kernel response contents:
+
+  =======================  ======  ===================================
+  ``ETHTOOL_A_TS_HEADER``  nested  reply header
+  ``ETHTOOL_A_TS_LAYER``   u32     available hardware timestamping
+  =======================  ======  ===================================
+
 Request translation
 ===================
 
@@ -2096,4 +2134,6 @@ are netlink only.
   n/a                                 ``ETHTOOL_MSG_PLCA_GET_STATUS``
   n/a                                 ``ETHTOOL_MSG_MM_GET``
   n/a                                 ``ETHTOOL_MSG_MM_SET``
+  n/a                                 ``ETHTOOL_MSG_TS_GET``
+  n/a                                 ``ETHTOOL_MSG_TSLIST_GET``
   =================================== =====================================
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 1ebf8d455f07..447908393b91 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -39,6 +39,8 @@ enum {
 	ETHTOOL_MSG_EEE_GET,
 	ETHTOOL_MSG_EEE_SET,
 	ETHTOOL_MSG_TSINFO_GET,
+	ETHTOOL_MSG_TSLIST_GET,
+	ETHTOOL_MSG_TS_GET,
 	ETHTOOL_MSG_CABLE_TEST_ACT,
 	ETHTOOL_MSG_CABLE_TEST_TDR_ACT,
 	ETHTOOL_MSG_TUNNEL_INFO_GET,
@@ -92,6 +94,8 @@ enum {
 	ETHTOOL_MSG_EEE_GET_REPLY,
 	ETHTOOL_MSG_EEE_NTF,
 	ETHTOOL_MSG_TSINFO_GET_REPLY,
+	ETHTOOL_MSG_TSLIST_GET_REPLY,
+	ETHTOOL_MSG_TS_GET_REPLY,
 	ETHTOOL_MSG_CABLE_TEST_NTF,
 	ETHTOOL_MSG_CABLE_TEST_TDR_NTF,
 	ETHTOOL_MSG_TUNNEL_INFO_GET_REPLY,
@@ -484,6 +488,17 @@ enum {
 	ETHTOOL_A_TSINFO_MAX = (__ETHTOOL_A_TSINFO_CNT - 1)
 };
 
+/* TS LAYER */
+
+enum {
+	ETHTOOL_A_TS_UNSPEC,
+	ETHTOOL_A_TS_HEADER,			/* nest - _A_HEADER_* */
+	ETHTOOL_A_TS_LAYER,			/* u32 */
+
+	/* add new constants above here */
+	__ETHTOOL_A_TS_CNT,
+	ETHTOOL_A_TS_MAX = (__ETHTOOL_A_TS_CNT - 1)
+};
 /* PHC VCLOCKS */
 
 enum {
diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
index a2c66b3d7f0f..d7c1798d45fe 100644
--- a/include/uapi/linux/net_tstamp.h
+++ b/include/uapi/linux/net_tstamp.h
@@ -13,6 +13,14 @@
 #include <linux/types.h>
 #include <linux/socket.h>   /* for SO_TIMESTAMPING */
 
+/* Hardware layer of the SO_TIMESTAMPING provider */
+enum timestamping_layer {
+	SOF_MAC_TIMESTAMPING = (1<<0),
+	SOF_PHY_TIMESTAMPING = (1<<1),
+
+	SOF_LAYER_TIMESTAMPING_LAST = SOF_PHY_TIMESTAMPING,
+};
+
 /* SO_TIMESTAMPING flags */
 enum {
 	SOF_TIMESTAMPING_TX_HARDWARE = (1<<0),
diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile
index 504f954a1b28..4ea64c080639 100644
--- a/net/ethtool/Makefile
+++ b/net/ethtool/Makefile
@@ -8,4 +8,4 @@ ethtool_nl-y	:= netlink.o bitset.o strset.o linkinfo.o linkmodes.o rss.o \
 		   linkstate.o debug.o wol.o features.o privflags.o rings.o \
 		   channels.o coalesce.o pause.o eee.o tsinfo.o cabletest.o \
 		   tunnels.o fec.o eeprom.o stats.o phc_vclocks.o mm.o \
-		   module.o pse-pd.o plca.o mm.o
+		   module.o pse-pd.o plca.o mm.o ts.o
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 08120095cc68..8d9e27b13e28 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -293,6 +293,8 @@ ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = {
 	[ETHTOOL_MSG_FEC_GET]		= &ethnl_fec_request_ops,
 	[ETHTOOL_MSG_FEC_SET]		= &ethnl_fec_request_ops,
 	[ETHTOOL_MSG_TSINFO_GET]	= &ethnl_tsinfo_request_ops,
+	[ETHTOOL_MSG_TS_GET]		= &ethnl_ts_request_ops,
+	[ETHTOOL_MSG_TSLIST_GET]	= &ethnl_tslist_request_ops,
 	[ETHTOOL_MSG_MODULE_EEPROM_GET]	= &ethnl_module_eeprom_request_ops,
 	[ETHTOOL_MSG_STATS_GET]		= &ethnl_stats_request_ops,
 	[ETHTOOL_MSG_PHC_VCLOCKS_GET]	= &ethnl_phc_vclocks_request_ops,
@@ -1011,6 +1013,24 @@ static const struct genl_ops ethtool_genl_ops[] = {
 		.policy = ethnl_tsinfo_get_policy,
 		.maxattr = ARRAY_SIZE(ethnl_tsinfo_get_policy) - 1,
 	},
+	{
+		.cmd	= ETHTOOL_MSG_TSLIST_GET,
+		.doit	= ethnl_default_doit,
+		.start	= ethnl_default_start,
+		.dumpit	= ethnl_default_dumpit,
+		.done	= ethnl_default_done,
+		.policy = ethnl_ts_get_policy,
+		.maxattr = ARRAY_SIZE(ethnl_ts_get_policy) - 1,
+	},
+	{
+		.cmd	= ETHTOOL_MSG_TS_GET,
+		.doit	= ethnl_default_doit,
+		.start	= ethnl_default_start,
+		.dumpit	= ethnl_default_dumpit,
+		.done	= ethnl_default_done,
+		.policy = ethnl_ts_get_policy,
+		.maxattr = ARRAY_SIZE(ethnl_ts_get_policy) - 1,
+	},
 	{
 		.cmd	= ETHTOOL_MSG_CABLE_TEST_ACT,
 		.flags	= GENL_UNS_ADMIN_PERM,
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 79424b34b553..49c700777a32 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -385,6 +385,8 @@ extern const struct ethnl_request_ops ethnl_coalesce_request_ops;
 extern const struct ethnl_request_ops ethnl_pause_request_ops;
 extern const struct ethnl_request_ops ethnl_eee_request_ops;
 extern const struct ethnl_request_ops ethnl_tsinfo_request_ops;
+extern const struct ethnl_request_ops ethnl_ts_request_ops;
+extern const struct ethnl_request_ops ethnl_tslist_request_ops;
 extern const struct ethnl_request_ops ethnl_fec_request_ops;
 extern const struct ethnl_request_ops ethnl_module_eeprom_request_ops;
 extern const struct ethnl_request_ops ethnl_stats_request_ops;
@@ -423,6 +425,7 @@ extern const struct nla_policy ethnl_pause_set_policy[ETHTOOL_A_PAUSE_TX + 1];
 extern const struct nla_policy ethnl_eee_get_policy[ETHTOOL_A_EEE_HEADER + 1];
 extern const struct nla_policy ethnl_eee_set_policy[ETHTOOL_A_EEE_TX_LPI_TIMER + 1];
 extern const struct nla_policy ethnl_tsinfo_get_policy[ETHTOOL_A_TSINFO_HEADER + 1];
+extern const struct nla_policy ethnl_ts_get_policy[ETHTOOL_A_TS_HEADER + 1];
 extern const struct nla_policy ethnl_cable_test_act_policy[ETHTOOL_A_CABLE_TEST_HEADER + 1];
 extern const struct nla_policy ethnl_cable_test_tdr_act_policy[ETHTOOL_A_CABLE_TEST_TDR_CFG + 1];
 extern const struct nla_policy ethnl_tunnel_info_get_policy[ETHTOOL_A_TUNNEL_INFO_HEADER + 1];
diff --git a/net/ethtool/ts.c b/net/ethtool/ts.c
new file mode 100644
index 000000000000..a71c47ff0c6b
--- /dev/null
+++ b/net/ethtool/ts.c
@@ -0,0 +1,114 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/net_tstamp.h>
+#include <linux/phy.h>
+
+#include "netlink.h"
+#include "common.h"
+#include "bitset.h"
+
+struct ts_req_info {
+	struct ethnl_req_info		base;
+};
+
+struct ts_reply_data {
+	struct ethnl_reply_data		base;
+	u32				ts;
+};
+
+#define TS_REPDATA(__reply_base) \
+	container_of(__reply_base, struct ts_reply_data, base)
+
+/* TS_GET */
+const struct nla_policy ethnl_ts_get_policy[] = {
+	[ETHTOOL_A_TS_HEADER]		=
+		NLA_POLICY_NESTED(ethnl_header_policy),
+};
+
+static int ts_prepare_data(const struct ethnl_req_info *req_base,
+			       struct ethnl_reply_data *reply_base,
+			       struct genl_info *info)
+{
+	struct ts_reply_data *data = TS_REPDATA(reply_base);
+	struct net_device *dev = reply_base->dev;
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	int ret;
+
+	ret = ethnl_ops_begin(dev);
+	if (ret < 0)
+		return ret;
+
+	if (phy_has_tsinfo(dev->phydev))
+		data->ts = SOF_PHY_TIMESTAMPING;
+	else if (ops->get_ts_info)
+		data->ts = SOF_MAC_TIMESTAMPING;
+	else
+		return -EOPNOTSUPP;
+
+	ethnl_ops_complete(dev);
+
+	return ret;
+}
+
+static int ts_reply_size(const struct ethnl_req_info *req_base,
+			     const struct ethnl_reply_data *reply_base)
+{
+	return nla_total_size(sizeof(u32));
+}
+
+static int ts_fill_reply(struct sk_buff *skb,
+			     const struct ethnl_req_info *req_base,
+			     const struct ethnl_reply_data *reply_base)
+{
+	struct ts_reply_data *data = TS_REPDATA(reply_base);
+	return nla_put_u32(skb, ETHTOOL_A_TS_LAYER, data->ts);
+}
+
+const struct ethnl_request_ops ethnl_ts_request_ops = {
+	.request_cmd		= ETHTOOL_MSG_TS_GET,
+	.reply_cmd		= ETHTOOL_MSG_TS_GET_REPLY,
+	.hdr_attr		= ETHTOOL_A_TS_HEADER,
+	.req_info_size		= sizeof(struct ts_req_info),
+	.reply_data_size	= sizeof(struct ts_reply_data),
+
+	.prepare_data		= ts_prepare_data,
+	.reply_size		= ts_reply_size,
+	.fill_reply		= ts_fill_reply,
+};
+
+/* TSLIST_GET */
+static int tslist_prepare_data(const struct ethnl_req_info *req_base,
+			       struct ethnl_reply_data *reply_base,
+			       struct genl_info *info)
+{
+	struct ts_reply_data *data = TS_REPDATA(reply_base);
+	struct net_device *dev = reply_base->dev;
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	int ret;
+
+	ret = ethnl_ops_begin(dev);
+	if (ret < 0)
+		return ret;
+
+	data->ts = 0;
+	if (phy_has_tsinfo(dev->phydev))
+		data->ts = SOF_PHY_TIMESTAMPING;
+	if (ops->get_ts_info)
+		data->ts |= SOF_MAC_TIMESTAMPING;
+
+	ethnl_ops_complete(dev);
+
+	return ret;
+}
+
+const struct ethnl_request_ops ethnl_tslist_request_ops = {
+	.request_cmd		= ETHTOOL_MSG_TSLIST_GET,
+	.reply_cmd		= ETHTOOL_MSG_TSLIST_GET_REPLY,
+	.hdr_attr		= ETHTOOL_A_TS_HEADER,
+	.req_info_size		= sizeof(struct ts_req_info),
+	.reply_data_size	= sizeof(struct ts_reply_data),
+
+	.prepare_data		= tslist_prepare_data,
+	.reply_size		= ts_reply_size,
+	.fill_reply		= ts_fill_reply,
+};
-- 
2.25.1

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Jakub Kicinski]

On Thu,  6 Apr 2023 19:33:05 +0200 Köry Maincent wrote:
> +Gets transceiver module parameters.
> +
> +Request contents:
> +
> +  =================================  ======  ==========================
> +  ``ETHTOOL_A_TS_HEADER``            nested  request header
> +  =================================  ======  ==========================
> +
> +Kernel response contents:
> +
> +  =======================  ======  ====================================
> +  ``ETHTOOL_A_TS_HEADER``  nested  reply header
> +  ``ETHTOOL_A_TS_LAYER``   u32     current hardware timestamping
> +  =======================  ======  ====================================
> +
> +TSLIST_GET
> +==========
> +
> +Gets transceiver module parameters.
> +
> +Request contents:
> +
> +  =================================  ======  ==========================
> +  ``ETHTOOL_A_TS_HEADER``            nested  request header
> +  =================================  ======  ==========================
> +
> +Kernel response contents:
> +
> +  =======================  ======  ===================================
> +  ``ETHTOOL_A_TS_HEADER``  nested  reply header
> +  ``ETHTOOL_A_TS_LAYER``   u32     available hardware timestamping
> +  =======================  ======  ===================================

Please put some words in here :) Documentation matters to the users.

Also let me advertise tools/net/ynl/cli.py to you:
https://docs.kernel.org/next/userspace-api/netlink/intro-specs.html

Please fill in the new commands in the ethtool spec. Feel free to ask
if you have any questions, it's still a bit of a rough.. clay?

> +/* Hardware layer of the SO_TIMESTAMPING provider */
> +enum timestamping_layer {
> +	SOF_MAC_TIMESTAMPING = (1<<0),
> +	SOF_PHY_TIMESTAMPING = (1<<1),

What does SOF_ stand for? 🤔️

We need a value for DMA timestamps here.

> +/* TSLIST_GET */
> +static int tslist_prepare_data(const struct ethnl_req_info *req_base,
> +			       struct ethnl_reply_data *reply_base,
> +			       struct genl_info *info)
> +{
> +	struct ts_reply_data *data = TS_REPDATA(reply_base);
> +	struct net_device *dev = reply_base->dev;
> +	const struct ethtool_ops *ops = dev->ethtool_ops;
> +	int ret;
> +
> +	ret = ethnl_ops_begin(dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	data->ts = 0;
> +	if (phy_has_tsinfo(dev->phydev))
> +		data->ts = SOF_PHY_TIMESTAMPING;
> +	if (ops->get_ts_info)
> +		data->ts |= SOF_MAC_TIMESTAMPING;

We can't make that assumption, that info must come from the driver.

Also don't we need some way to identify the device / phc from which 
the timestamp at the given layer will come?

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Köry Maincent]

Thanks for the review.

On Thu, 6 Apr 2023 18:46:46 -0700
Jakub Kicinski <kuba@kernel.org> wrote:
 
> Please put some words in here :) Documentation matters to the users.

Ok.

> 
> Also let me advertise tools/net/ynl/cli.py to you:
> https://docs.kernel.org/next/userspace-api/netlink/intro-specs.html

Ok I will take look.
Seems broken on net-next:
./tools/net/ynl/cli.py --spec Documentation/netlink/specs/ethtool.yaml --do rings-get --json '{"header":{"dev-index": 18}}'
Traceback (most recent call last):
  File "./tools/net/ynl/cli.py", line 52, in <module>
    main()
  File "./tools/net/ynl/cli.py", line 31, in main
    ynl = YnlFamily(args.spec, args.schema)
  File "/home/kmaincent/Documents/linux/tools/net/ynl/lib/ynl.py", line 361, in __init__
    self.family = GenlFamily(self.yaml['name'])
  File "/home/kmaincent/Documents/linux/tools/net/ynl/lib/ynl.py", line 331, in __init__
    self.genl_family = genl_family_name_to_id[family_name]
KeyError: 'ethtool'


> Please fill in the new commands in the ethtool spec. Feel free to ask
> if you have any questions, it's still a bit of a rough.. clay?
> 
> > +/* Hardware layer of the SO_TIMESTAMPING provider */
> > +enum timestamping_layer {
> > +	SOF_MAC_TIMESTAMPING = (1<<0),
> > +	SOF_PHY_TIMESTAMPING = (1<<1),  
> 
> What does SOF_ stand for? 🤔️

It was to follow the naming reference in
"Documentation/networking/timestamping.rst" like SOF_TIMESTAMPING_RX_HARDWARE
or SOF_TIMESTAMPING_RX_SOFTAWRE but indeed I do not really understand the SOF
prefix. SocketF?

> 
> We need a value for DMA timestamps here.

Alright,

> 
> > +/* TSLIST_GET */
> > +static int tslist_prepare_data(const struct ethnl_req_info *req_base,
> > +			       struct ethnl_reply_data *reply_base,
> > +			       struct genl_info *info)
> > +{
> > +	struct ts_reply_data *data = TS_REPDATA(reply_base);
> > +	struct net_device *dev = reply_base->dev;
> > +	const struct ethtool_ops *ops = dev->ethtool_ops;
> > +	int ret;
> > +
> > +	ret = ethnl_ops_begin(dev);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	data->ts = 0;
> > +	if (phy_has_tsinfo(dev->phydev))
> > +		data->ts = SOF_PHY_TIMESTAMPING;
> > +	if (ops->get_ts_info)
> > +		data->ts |= SOF_MAC_TIMESTAMPING;  
> 
> We can't make that assumption, that info must come from the driver.

Why can't we list the available time stamp like that, can't we just test if they
have time stamp support. 

> Also don't we need some way to identify the device / phc from which 
> the timestamp at the given layer will come?

I don't think so, the PHC will be described by the get_ts_info from each
ethernet driver. Do I miss something?

Köry

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Jakub Kicinski]

On Fri, 7 Apr 2023 10:58:57 +0200 Köry Maincent wrote:
> > Also let me advertise tools/net/ynl/cli.py to you:
> > https://docs.kernel.org/next/userspace-api/netlink/intro-specs.html  
> 
> Ok I will take look.
> Seems broken on net-next:
> ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/ethtool.yaml --do rings-get --json '{"header":{"dev-index": 18}}'
> Traceback (most recent call last):
>   File "./tools/net/ynl/cli.py", line 52, in <module>
>     main()
>   File "./tools/net/ynl/cli.py", line 31, in main
>     ynl = YnlFamily(args.spec, args.schema)
>   File "/home/kmaincent/Documents/linux/tools/net/ynl/lib/ynl.py", line 361, in __init__
>     self.family = GenlFamily(self.yaml['name'])
>   File "/home/kmaincent/Documents/linux/tools/net/ynl/lib/ynl.py", line 331, in __init__
>     self.genl_family = genl_family_name_to_id[family_name]
> KeyError: 'ethtool'

IIRC this usually means ethtool netlink is not selected by you Kconfig.
I should add a clearer error for that I guess.
Booting net-next now, I'll get back to you with a confirmation.

> > Please fill in the new commands in the ethtool spec. Feel free to ask
> > if you have any questions, it's still a bit of a rough.. clay?
> >   
> > > +/* Hardware layer of the SO_TIMESTAMPING provider */
> > > +enum timestamping_layer {
> > > +	SOF_MAC_TIMESTAMPING = (1<<0),
> > > +	SOF_PHY_TIMESTAMPING = (1<<1),    
> > 
> > What does SOF_ stand for? 🤔️  
> 
> It was to follow the naming reference in
> "Documentation/networking/timestamping.rst" like SOF_TIMESTAMPING_RX_HARDWARE
> or SOF_TIMESTAMPING_RX_SOFTAWRE but indeed I do not really understand the SOF
> prefix. 

My default would be to prefix it with ethtool, but who knows maybe one
day it will be used in socket (actually it may get used on the
timestamps themselves?) so it's a judgment call. 

> SocketF?

:D  Yeah, some socketFff feature? flag? 🤷️

> > We need a value for DMA timestamps here.  
> 
> Alright,
> 
> > > +/* TSLIST_GET */
> > > +static int tslist_prepare_data(const struct ethnl_req_info *req_base,
> > > +			       struct ethnl_reply_data *reply_base,
> > > +			       struct genl_info *info)
> > > +{
> > > +	struct ts_reply_data *data = TS_REPDATA(reply_base);
> > > +	struct net_device *dev = reply_base->dev;
> > > +	const struct ethtool_ops *ops = dev->ethtool_ops;
> > > +	int ret;
> > > +
> > > +	ret = ethnl_ops_begin(dev);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	data->ts = 0;
> > > +	if (phy_has_tsinfo(dev->phydev))
> > > +		data->ts = SOF_PHY_TIMESTAMPING;
> > > +	if (ops->get_ts_info)
> > > +		data->ts |= SOF_MAC_TIMESTAMPING;    
> > 
> > We can't make that assumption, that info must come from the driver.  
> 
> Why can't we list the available time stamp like that, can't we just test if they
> have time stamp support. 

On embedded devices this will work, but for integrated NICs there's no
standalone PHY driver. The one vendor driver takes care of all.

Maybe we can do something like:

if (dev->phydev)
	/* your code, it should work for phydev users */
else
	/* vendor driver, we need to call the driver */

Until vendor drivers fill in their implementations we may need to
report some form of unknown source. So we'll either have to add
"unknown" to the source enum or return -EOPNOTSUPP.

> > Also don't we need some way to identify the device / phc from which 
> > the timestamp at the given layer will come?  
> 
> I don't think so, the PHC will be described by the get_ts_info from each
> ethernet driver. Do I miss something?

Right, but then to discover all PHCs the user space would have to
switch the source, read the index, switch the source, read the index,
no? I may just be confused..

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Jakub Kicinski]

On Fri, 7 Apr 2023 07:26:15 -0700 Jakub Kicinski wrote:
> > Ok I will take look.
> > Seems broken on net-next:
> > ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/ethtool.yaml --do rings-get --json '{"header":{"dev-index": 18}}'
> > Traceback (most recent call last):
> >   File "./tools/net/ynl/cli.py", line 52, in <module>
> >     main()
> >   File "./tools/net/ynl/cli.py", line 31, in main
> >     ynl = YnlFamily(args.spec, args.schema)
> >   File "/home/kmaincent/Documents/linux/tools/net/ynl/lib/ynl.py", line 361, in __init__
> >     self.family = GenlFamily(self.yaml['name'])
> >   File "/home/kmaincent/Documents/linux/tools/net/ynl/lib/ynl.py", line 331, in __init__
> >     self.genl_family = genl_family_name_to_id[family_name]
> > KeyError: 'ethtool'  
> 
> IIRC this usually means ethtool netlink is not selected by you Kconfig.
> I should add a clearer error for that I guess.
> Booting net-next now, I'll get back to you with a confirmation.

Yeah, works here. FWIW if you want to use it on the VM / remote host
you just need to copy over a couple of dirs:

$ scp -r tools/net/ynl/ $bla:~/
$ scp -r Documentation/netlink/ $bla:~/
$ ssh $bla
bla$ dnf install python-yaml
bla$ ./ynl/cli.py \
	--no-schema \
	--spec netlink/specs/ethtool.yaml \
	--do rings-get \
	--json '{"header":{"dev-index": 2}}'

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Jacob Keller]

On 4/7/2023 1:58 AM, Köry Maincent wrote:
> On Thu, 6 Apr 2023 18:46:46 -0700
>> What does SOF_ stand for? 🤔️
> 
> It was to follow the naming reference in
> "Documentation/networking/timestamping.rst" like SOF_TIMESTAMPING_RX_HARDWARE
> or SOF_TIMESTAMPING_RX_SOFTAWRE but indeed I do not really understand the SOF
> prefix. SocketF?
> 

I believe its something like "Socket Option Flag" or "Socket Option
Feature", but it is pretty old, so I don't really remember full context.

Thanks,
Jake

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Michael Walle]

>> +/* Hardware layer of the SO_TIMESTAMPING provider */
>> +enum timestamping_layer {
>> +	SOF_MAC_TIMESTAMPING =3D (1<<0),
>> +	SOF_PHY_TIMESTAMPING =3D (1<<1),
>
> What does SOF_ stand for?

I'd guess start of frame. The timestamp will be taken at the
beginning of the frame.

-michael

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Vladimir Oltean]

On Wed, Apr 12, 2023 at 12:50:34PM +0200, Michael Walle wrote:
> >> +/* Hardware layer of the SO_TIMESTAMPING provider */
> >> +enum timestamping_layer {
> >> +	SOF_MAC_TIMESTAMPING =3D (1<<0),
> >> +	SOF_PHY_TIMESTAMPING =3D (1<<1),
> >
> > What does SOF_ stand for?
> 
> I'd guess start of frame. The timestamp will be taken at the
> beginning of the frame.

I would suggest (with all due respect) that it was an inapt adaptation
of the Socket Option Flags that can be seen in
Documentation/networking/timestamping.rst.

These are not socket option flags (because these settings are not per
socket), so the namespace/prefix is not really correctly used here.

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Michael Walle]

Am 2023-04-12 13:08, schrieb Vladimir Oltean:
> On Wed, Apr 12, 2023 at 12:50:34PM +0200, Michael Walle wrote:
>> >> +/* Hardware layer of the SO_TIMESTAMPING provider */
>> >> +enum timestamping_layer {
>> >> +	SOF_MAC_TIMESTAMPING =3D (1<<0),
>> >> +	SOF_PHY_TIMESTAMPING =3D (1<<1),
>> >
>> > What does SOF_ stand for?
>> 
>> I'd guess start of frame. The timestamp will be taken at the
>> beginning of the frame.
> 
> I would suggest (with all due respect) that it was an inapt adaptation
> of the Socket Option Flags that can be seen in
> Documentation/networking/timestamping.rst.

Agreed. Noticing the other SOF_TIMESTAMPING_* names, your
suggestion makes way more sense. Sorry for the noise.

-michael

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Köry Maincent]

On Wed, 12 Apr 2023 14:08:40 +0300
Vladimir Oltean <vladimir.oltean@nxp.com> wrote:

> On Wed, Apr 12, 2023 at 12:50:34PM +0200, Michael Walle wrote:
> > >> +/* Hardware layer of the SO_TIMESTAMPING provider */
> > >> +enum timestamping_layer {
> > >> +	SOF_MAC_TIMESTAMPING =3D (1<<0),
> > >> +	SOF_PHY_TIMESTAMPING =3D (1<<1),  
> > >
> > > What does SOF_ stand for?  
> > 
> > I'd guess start of frame. The timestamp will be taken at the
> > beginning of the frame.  
> 
> I would suggest (with all due respect) that it was an inapt adaptation
> of the Socket Option Flags that can be seen in
> Documentation/networking/timestamping.rst.
> 
> These are not socket option flags (because these settings are not per
> socket), so the namespace/prefix is not really correctly used here.

As Jakub said, who knows maybe one day it will be per socket information,
but indeed for now it is not the case.
I will stick to whatever name the community prefer.

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Vladimir Oltean]

On Thu, Apr 06, 2023 at 06:46:46PM -0700, Jakub Kicinski wrote:
> > +/* Hardware layer of the SO_TIMESTAMPING provider */
> > +enum timestamping_layer {
> > +	SOF_MAC_TIMESTAMPING = (1<<0),
> > +	SOF_PHY_TIMESTAMPING = (1<<1),
> 
> We need a value for DMA timestamps here.

What's a DMA timestamp, technically? Is it a snapshot of the PHC's time
domain, just not at the MII pins?

Which drivers provide DMA timestamps, and how do they currently signal
that they do this? Do they do this for all packets that get timestamped,
or for "some"?

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Andrew Lunn]

On Thu, May 11, 2023 at 11:36:46PM +0300, Vladimir Oltean wrote:
> On Thu, Apr 06, 2023 at 06:46:46PM -0700, Jakub Kicinski wrote:
> > > +/* Hardware layer of the SO_TIMESTAMPING provider */
> > > +enum timestamping_layer {
> > > +	SOF_MAC_TIMESTAMPING = (1<<0),
> > > +	SOF_PHY_TIMESTAMPING = (1<<1),
> > 
> > We need a value for DMA timestamps here.
> 
> What's a DMA timestamp, technically? Is it a snapshot of the PHC's time
> domain, just not at the MII pins?

I also wounder if there is one definition of DMA timestampting, or
multiple. It could simply be that the time stamp is in the DMA
descriptor, but the sample of the PHC was taken as the SOF was
received. It could be the sample of the PHC at the point the DMA
descriptor was handed back to the host. It could be the PHC was
sampled when the host reads the descriptor, etc.

I also wounder if there is sufficient documentation to be able to tell
them apart for devices which support it. So maybe it does not even
matter what the exact definition is?

	Andrew

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Russell King (Oracle)]

On Thu, May 11, 2023 at 10:50:30PM +0200, Andrew Lunn wrote:
> On Thu, May 11, 2023 at 11:36:46PM +0300, Vladimir Oltean wrote:
> > On Thu, Apr 06, 2023 at 06:46:46PM -0700, Jakub Kicinski wrote:
> > > > +/* Hardware layer of the SO_TIMESTAMPING provider */
> > > > +enum timestamping_layer {
> > > > +	SOF_MAC_TIMESTAMPING = (1<<0),
> > > > +	SOF_PHY_TIMESTAMPING = (1<<1),
> > > 
> > > We need a value for DMA timestamps here.
> > 
> > What's a DMA timestamp, technically? Is it a snapshot of the PHC's time
> > domain, just not at the MII pins?
> 
> I also wounder if there is one definition of DMA timestampting, or
> multiple. It could simply be that the time stamp is in the DMA
> descriptor,

Surely that is equivalent to MAC timestamping? Whether the MAC
places it in a DMA descriptor, or whether it places it in some
auxiliary information along with the packet is surely irrelevant,
because the MAC has to have the timestamp available to it in some
manner. Where it ends up is just a function of implementation surely?

I'm just wondering what this would mean for mvpp2, where the
timestamps are in the descriptors. If we have a "DMA timestamp"
is that a "DMA timestamp" or a "MAC timestamp"? The timestamp comes
from the MAC in this case.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Vladimir Oltean]

On Thu, May 11, 2023 at 09:55:39PM +0100, Russell King (Oracle) wrote:
> On Thu, May 11, 2023 at 10:50:30PM +0200, Andrew Lunn wrote:
> > On Thu, May 11, 2023 at 11:36:46PM +0300, Vladimir Oltean wrote:
> > > On Thu, Apr 06, 2023 at 06:46:46PM -0700, Jakub Kicinski wrote:
> > > > > +/* Hardware layer of the SO_TIMESTAMPING provider */
> > > > > +enum timestamping_layer {
> > > > > +	SOF_MAC_TIMESTAMPING = (1<<0),
> > > > > +	SOF_PHY_TIMESTAMPING = (1<<1),
> > > > 
> > > > We need a value for DMA timestamps here.
> > > 
> > > What's a DMA timestamp, technically? Is it a snapshot of the PHC's time
> > > domain, just not at the MII pins?
> > 
> > I also wounder if there is one definition of DMA timestampting, or
> > multiple. It could simply be that the time stamp is in the DMA
> > descriptor,
> 
> Surely that is equivalent to MAC timestamping? Whether the MAC
> places it in a DMA descriptor, or whether it places it in some
> auxiliary information along with the packet is surely irrelevant,
> because the MAC has to have the timestamp available to it in some
> manner. Where it ends up is just a function of implementation surely?
> 
> I'm just wondering what this would mean for mvpp2, where the
> timestamps are in the descriptors. If we have a "DMA timestamp"
> is that a "DMA timestamp" or a "MAC timestamp"? The timestamp comes
> from the MAC in this case.

No, a MAC timestamp carried through a DMA descriptor is still a MAC
timestamp (better said: timestamp taken at the MAC).

DMA timestamps probably have to do with this igc patch set, which I
admit to not having had the patience to follow along all the way and
understand what is its status and if it was ever accepted in that form,
or a different form, or if Vinicius' work for multiple in-flight TX
timestamps is an alternate solution to the same problem as DMA
timestamps, or whatever:
https://lore.kernel.org/netdev/20221018010733.4765-1-muhammad.husaini.zulkifli@intel.com/

So I just asked.

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Jakub Kicinski]

On Fri, 12 May 2023 00:02:37 +0300 Vladimir Oltean wrote:
> > Surely that is equivalent to MAC timestamping? Whether the MAC
> > places it in a DMA descriptor, or whether it places it in some
> > auxiliary information along with the packet is surely irrelevant,
> > because the MAC has to have the timestamp available to it in some
> > manner. Where it ends up is just a function of implementation surely?
> > 
> > I'm just wondering what this would mean for mvpp2, where the
> > timestamps are in the descriptors. If we have a "DMA timestamp"
> > is that a "DMA timestamp" or a "MAC timestamp"? The timestamp comes
> > from the MAC in this case.  
> 
> No, a MAC timestamp carried through a DMA descriptor is still a MAC
> timestamp (better said: timestamp taken at the MAC).

Right. The method of communicating the TS and where TS is taken 
are theoretically unrelated (in practice DMA timestamp not in
a descriptor is likely pointless).

> DMA timestamps probably have to do with this igc patch set, which I
> admit to not having had the patience to follow along all the way and
> understand what is its status and if it was ever accepted in that form,
> or a different form, or if Vinicius' work for multiple in-flight TX

Exactly, think Tx. For Rx hauling the TS in metadata from PHY/MAC to
descriptor is easy. For Tx device will buffer the packet so the DMA
completion happens before the packet actually left onto the wire.

Which is not to say that some devices may not generate the Rx timestamp
when the packet is DMA'ed out of laziness, too.

> timestamps is an alternate solution to the same problem as DMA
> timestamps, or whatever:
> https://lore.kernel.org/netdev/20221018010733.4765-1-muhammad.husaini.zulkifli@intel.com/

What I was thinking was:

 - PHY - per spec, at the RS layer
 - MAC - "close to the wire" in the MAC, specifically the pipeline
   delay (PHY stamp vs MAC stamp) should be constant for all packets;
   there must be no variable-time buffering and (for Tx) the time
   stamping must be past the stage of the pipeline affected by pause
   frames
 - DMA - worst quality, variable delay timestamp, usually taken when
   packets DMA descriptors (Rx or completion) are being written

With these definitions MAC and PHY timestamps are pretty similar
from the perspective of accuracy.

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Vladimir Oltean]

On Thu, May 11, 2023 at 03:09:02PM -0700, Jakub Kicinski wrote:
> Exactly, think Tx. For Rx hauling the TS in metadata from PHY/MAC to
> descriptor is easy. For Tx device will buffer the packet so the DMA
> completion happens before the packet actually left onto the wire.
> 
> Which is not to say that some devices may not generate the Rx timestamp
> when the packet is DMA'ed out of laziness, too.
> 
> > timestamps is an alternate solution to the same problem as DMA
> > timestamps, or whatever:
> > https://lore.kernel.org/netdev/20221018010733.4765-1-muhammad.husaini.zulkifli@intel.com/
> 
> What I was thinking was:
> 
>  - PHY - per spec, at the RS layer
>  - MAC - "close to the wire" in the MAC, specifically the pipeline
>    delay (PHY stamp vs MAC stamp) should be constant for all packets;
>    there must be no variable-time buffering and (for Tx) the time
>    stamping must be past the stage of the pipeline affected by pause
>    frames
>  - DMA - worst quality, variable delay timestamp, usually taken when
>    packets DMA descriptors (Rx or completion) are being written
> 
> With these definitions MAC and PHY timestamps are pretty similar
> from the perspective of accuracy.

So if I add a call to ptp_clock_info :: gettimex64() where the
skb_tx_timestamp() call is located in a driver, could that pass as
a DMA timestamp?

The question is how much do we want to encourage these DMA timestamps:
enough to make them a first-class citizen in the UAPI? Are users even
happy with their existence?

I mean, I can't ignore the fact that there are NICs that can provide
2-step TX timestamps at line rate for all packets (not just PTP) for
line rates exceeding 10G, in band with the descriptor in its TX
completion queue. I don't want to give any names, just to point out
that there isn't any inherent limitation in the technology. AFAIU from
igc_ptp_tx_hwtstamp(), it's just that the igc DMA controller did not
bother to transport the timestamps from the MAC back into the
descriptor, leaving it up to software to do it out of band, which of
course may cause correlation bugs and limits throughput. Surely they
can do better.

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Jakub Kicinski]

On Fri, 12 May 2023 02:07:17 +0300 Vladimir Oltean wrote:
> On Thu, May 11, 2023 at 03:09:02PM -0700, Jakub Kicinski wrote:
> > Exactly, think Tx. For Rx hauling the TS in metadata from PHY/MAC to
> > descriptor is easy. For Tx device will buffer the packet so the DMA
> > completion happens before the packet actually left onto the wire.
> > 
> > Which is not to say that some devices may not generate the Rx timestamp
> > when the packet is DMA'ed out of laziness, too.
> >   
> > > timestamps is an alternate solution to the same problem as DMA
> > > timestamps, or whatever:
> > > https://lore.kernel.org/netdev/20221018010733.4765-1-muhammad.husaini.zulkifli@intel.com/  
> > 
> > What I was thinking was:
> > 
> >  - PHY - per spec, at the RS layer
> >  - MAC - "close to the wire" in the MAC, specifically the pipeline
> >    delay (PHY stamp vs MAC stamp) should be constant for all packets;
> >    there must be no variable-time buffering and (for Tx) the time
> >    stamping must be past the stage of the pipeline affected by pause
> >    frames
> >  - DMA - worst quality, variable delay timestamp, usually taken when
> >    packets DMA descriptors (Rx or completion) are being written
> > 
> > With these definitions MAC and PHY timestamps are pretty similar
> > from the perspective of accuracy.  
> 
> So if I add a call to ptp_clock_info :: gettimex64() where the
> skb_tx_timestamp() call is located in a driver, could that pass as
> a DMA timestamp?

Yes.

> The question is how much do we want to encourage these DMA timestamps:
> enough to make them a first-class citizen in the UAPI? Are users even
> happy with their existence?

I don't want to call anyone out, but multiple high speed devices
with the current, in tree drivers report DMA timestamps when you 
ask for HW timestamps. DMA stamps are still 2 orders of magnitude
better than SW stamping.

> I mean, I can't ignore the fact that there are NICs that can provide
> 2-step TX timestamps at line rate for all packets (not just PTP) for
> line rates exceeding 10G, in band with the descriptor in its TX
> completion queue. I don't want to give any names, just to point out
> that there isn't any inherent limitation in the technology.

Oh, you should tell me, maybe off-list then. 'Cause I don't know any.

> AFAIU from igc_ptp_tx_hwtstamp(), it's just that the igc DMA
> controller did not bother to transport the timestamps from the MAC
> back into the descriptor, leaving it up to software to do it out of
> band, which of course may cause correlation bugs and limits
> throughput. Surely they can do better.

It's not that simple. Any packet loss or diversion and you may end up
missing a completion.

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Vladimir Oltean]

On Thu, May 11, 2023 at 04:16:25PM -0700, Jakub Kicinski wrote:
> > I mean, I can't ignore the fact that there are NICs that can provide
> > 2-step TX timestamps at line rate for all packets (not just PTP) for
> > line rates exceeding 10G, in band with the descriptor in its TX
> > completion queue. I don't want to give any names, just to point out
> > that there isn't any inherent limitation in the technology.
> 
> Oh, you should tell me, maybe off-list then. 'Cause I don't know any.

I hope the examples given in private will make you reconsider the
validity of my argument about DMA timestamps.

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Jakub Kicinski]

On Fri, 12 May 2023 13:29:11 +0300 Vladimir Oltean wrote:
> On Thu, May 11, 2023 at 04:16:25PM -0700, Jakub Kicinski wrote:
> > > I mean, I can't ignore the fact that there are NICs that can provide
> > > 2-step TX timestamps at line rate for all packets (not just PTP) for
> > > line rates exceeding 10G, in band with the descriptor in its TX
> > > completion queue. I don't want to give any names, just to point out
> > > that there isn't any inherent limitation in the technology.  
> > 
> > Oh, you should tell me, maybe off-list then. 'Cause I don't know any.  
> 
> I hope the examples given in private will make you reconsider the
> validity of my argument about DMA timestamps.

I may have lost track of what the argument is. There are devices
which will provide a DMA stamp for Tx and Rx. We need an API that'll
inform the user about it. 

To be clear I'm talking about drivers which are already in the tree,
not opening the door for some shoddy new HW in.

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Jakub Kicinski]

On Fri, 12 May 2023 10:38:52 -0700 Jakub Kicinski wrote:
> On Fri, 12 May 2023 13:29:11 +0300 Vladimir Oltean wrote:
> > On Thu, May 11, 2023 at 04:16:25PM -0700, Jakub Kicinski wrote:  
> > > Oh, you should tell me, maybe off-list then. 'Cause I don't know any.    
> > 
> > I hope the examples given in private will make you reconsider the
> > validity of my argument about DMA timestamps.  
> 
> I may have lost track of what the argument is. There are devices
> which will provide a DMA stamp for Tx and Rx. We need an API that'll
> inform the user about it. 
> 
> To be clear I'm talking about drivers which are already in the tree,
> not opening the door for some shoddy new HW in.

It dawned on me while reading a phylink discussion that I may have
misunderstood the meaning of the MAC vs PHY time stamp sources.
By the standard - stamping happens under the MAC, so MAC is 
the "right" place to stamp, not the PHY. And there can be multiple 
PHYs technically? Are we just using the MAC vs PHY thing as an
implementation aid, to know which driver to send the request to?

Shouldn't we use the clock ID instead?

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Andrew Lunn]

On Wed, May 17, 2023 at 12:19:25PM -0700, Jakub Kicinski wrote:
> On Fri, 12 May 2023 10:38:52 -0700 Jakub Kicinski wrote:
> > On Fri, 12 May 2023 13:29:11 +0300 Vladimir Oltean wrote:
> > > On Thu, May 11, 2023 at 04:16:25PM -0700, Jakub Kicinski wrote:  
> > > > Oh, you should tell me, maybe off-list then. 'Cause I don't know any.    
> > > 
> > > I hope the examples given in private will make you reconsider the
> > > validity of my argument about DMA timestamps.  
> > 
> > I may have lost track of what the argument is. There are devices
> > which will provide a DMA stamp for Tx and Rx. We need an API that'll
> > inform the user about it. 
> > 
> > To be clear I'm talking about drivers which are already in the tree,
> > not opening the door for some shoddy new HW in.
> 
> It dawned on me while reading a phylink discussion that I may have
> misunderstood the meaning of the MAC vs PHY time stamp sources.
> By the standard - stamping happens under the MAC, so MAC is 
> the "right" place to stamp, not the PHY. And there can be multiple 
> PHYs technically? Are we just using the MAC vs PHY thing as an
> implementation aid, to know which driver to send the request to?
> 
> Shouldn't we use the clock ID instead?

As i said in an earlier thread, with a bit of a stretch, there could
be 7 places to take time stamps in the system. We need some sort of
identifier to indicate which of these stampers to use.

Is clock ID unique? In a switch, i think there could be multiple
stampers, one per MAC port, sharing one clock? So you actually need
more than a clock ID.

Also, 'By the standard - stamping happens under the MAC'. Which MAC?
There can be multple MAC's in the pipeline. MACSEC and rate adaptation
in the PHY are often implemented by the PHY having a MAC
reconstituting the frame from the bitstream and putting it into a
queue. Rate adaptation can then be performed by the PHY by sending
pause frames to the 'primary' MAC to slow it down. MACSEC in the PHY
takes frames in the queues and if they match a filter they get
encrypted. The PHY then takes the frame out of the queue and passes
them to a second MAC in the PHY which creates a bitstream and then to
a 'PHY' to generate signals for the line.

In this sort of setup, you obviously don't want the 'primary' MAC
doing the stamping. You want the MAC nearest to the line, or better
still the 'PHY' within the PHY just before the line.

      Andrew

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Jakub Kicinski]

On Wed, 17 May 2023 21:46:43 +0200 Andrew Lunn wrote:
> As i said in an earlier thread, with a bit of a stretch, there could
> be 7 places to take time stamps in the system. We need some sort of
> identifier to indicate which of these stampers to use.
> 
> Is clock ID unique? In a switch, i think there could be multiple
> stampers, one per MAC port, sharing one clock? So you actually need
> more than a clock ID.

Clock ID is a bit vague too, granted, but in practice clock ID should
correspond to the driver fairly well? My thinking was - use clock ID
to select the (silicon) device, use a different attribute to select 
the stamping point.

IOW try to use the existing attribute before inventing a new one.

> Also, 'By the standard - stamping happens under the MAC'. Which MAC?
> There can be multple MAC's in the pipeline. MACSEC and rate adaptation
> in the PHY are often implemented by the PHY having a MAC
> reconstituting the frame from the bitstream and putting it into a
> queue. Rate adaptation can then be performed by the PHY by sending
> pause frames to the 'primary' MAC to slow it down. MACSEC in the PHY
> takes frames in the queues and if they match a filter they get
> encrypted. The PHY then takes the frame out of the queue and passes
> them to a second MAC in the PHY which creates a bitstream and then to
> a 'PHY' to generate signals for the line.
> 
> In this sort of setup, you obviously don't want the 'primary' MAC
> doing the stamping. You want the MAC nearest to the line, or better
> still the 'PHY' within the PHY just before the line.

For a PHY "always use the point closest to the wire" makes sense.

For DMA we'd have a different set of priorities - precision vs
volume vs 1-step / 2-step; but all from the same clock. I think 
that we may want to defer figuring out selection within a single
clock for now, to make some progress.

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Köry Maincent]

Hello I am resuming my work on selectable timestamping layers.

On Wed, 17 May 2023 13:07:06 -0700
Jakub Kicinski <kuba@kernel.org> wrote:

> On Wed, 17 May 2023 21:46:43 +0200 Andrew Lunn wrote:
> > As i said in an earlier thread, with a bit of a stretch, there could
> > be 7 places to take time stamps in the system. We need some sort of
> > identifier to indicate which of these stampers to use.
> > 
> > Is clock ID unique? In a switch, i think there could be multiple
> > stampers, one per MAC port, sharing one clock? So you actually need
> > more than a clock ID.  
> 
> Clock ID is a bit vague too, granted, but in practice clock ID should
> correspond to the driver fairly well? My thinking was - use clock ID
> to select the (silicon) device, use a different attribute to select 
> the stamping point.
> 
> IOW try to use the existing attribute before inventing a new one.

What do you think of using the clock ID to select the timestamp layer, and add
a ts_layer field in ts_info that will describe the timestamp layer. This allow
to have more information than the vague clock ID. We set it in the driver.
With it, we could easily add new layers different than simple mac and phy.
I am currently working on this implementation.

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Richard Cochran]

On Mon, Sep 04, 2023 at 05:22:45PM +0200, Köry Maincent wrote:
> Hello I am resuming my work on selectable timestamping layers.
> 
> On Wed, 17 May 2023 13:07:06 -0700
> Jakub Kicinski <kuba@kernel.org> wrote:
> 
> > On Wed, 17 May 2023 21:46:43 +0200 Andrew Lunn wrote:
> > > As i said in an earlier thread, with a bit of a stretch, there could
> > > be 7 places to take time stamps in the system. We need some sort of
> > > identifier to indicate which of these stampers to use.
> > > 
> > > Is clock ID unique? In a switch, i think there could be multiple
> > > stampers, one per MAC port, sharing one clock? So you actually need
> > > more than a clock ID.  

Let's not mix things up.  A clock ID identifies a dynamic posix clock,
just like the name suggests.  A clock is a device that supports
operations like gettime and settime.

A given clock might or might not generate time stamps.

The time stamp API is completely separate.

> > Clock ID is a bit vague too, granted, but in practice clock ID should
> > correspond to the driver fairly well? My thinking was - use clock ID
> > to select the (silicon) device, use a different attribute to select 
> > the stamping point.

I've never heard of a device that has different time stamping points.
If one ever appeared, then it ought to present two interfaces.

> > IOW try to use the existing attribute before inventing a new one.
> 
> What do you think of using the clock ID to select the timestamp layer, and add
> a ts_layer field in ts_info that will describe the timestamp layer. This allow
> to have more information than the vague clock ID. We set it in the driver.
> With it, we could easily add new layers different than simple mac and phy.
> I am currently working on this implementation.

I think you should model the layers explicitly.

Thanks,
Richard

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Jakub Kicinski]

On Mon, 4 Sep 2023 10:47:00 -0700 Richard Cochran wrote:
> > > Clock ID is a bit vague too, granted, but in practice clock ID should
> > > correspond to the driver fairly well? My thinking was - use clock ID
> > > to select the (silicon) device, use a different attribute to select 
> > > the stamping point.  
> 
> I've never heard of a device that has different time stamping points.
> If one ever appeared, then it ought to present two interfaces.
> 
> > > IOW try to use the existing attribute before inventing a new one.  
> > 
> > What do you think of using the clock ID to select the timestamp layer, and add
> > a ts_layer field in ts_info that will describe the timestamp layer. This allow
> > to have more information than the vague clock ID. We set it in the driver.
> > With it, we could easily add new layers different than simple mac and phy.
> > I am currently working on this implementation.  
> 
> I think you should model the layers explicitly.

Maybe we should try to enumerate the use cases, I don't remember now
but I think the concern was that there may be multiple PHYs?

(Using [] to denote a single device)

#0    integrated NIC: [DMA MAC PHY]
#1       "Linux" NIC: [DMA MAC][PHY]
#2   DSA switch trap: [DMA MAC][MAC PHY]
#3 DSA switch switch: [PHY MAC  MAC PHY]
#4   DSA distributed: [PHY MAC][MAC PHY]

All these should be fine with netdev + layer, IIUC.

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Andrew Lunn]

> Maybe we should try to enumerate the use cases, I don't remember now
> but I think the concern was that there may be multiple PHYs?

You often see a Marvell 10G PHY between a MAC and an SFP cage. You can
then get a copper SFP module which has a PHY in it.

So:

"Linux" NIC: [DMA MAC][PHY][PHY] 

And just to make it more interesting, you sometimes see:

[MAC] - MII MUX -+---[PHY][PHY]
                 |
                 +---[PHY]

This is currently not supported, but there is work in progress to
address this, by giving each PHY and ID, and extending the netlink
ethtool so you can enumerate PHYs and individually configure them.

And i pointed out maybe the worst case scenario:

[MAC][PHY][PHY][MAC]switch core[MAC][PHY][PHY]

	Andrew

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Köry Maincent]

On Tue, 5 Sep 2023 22:29:51 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > Maybe we should try to enumerate the use cases, I don't remember now
> > but I think the concern was that there may be multiple PHYs?  
> 
> You often see a Marvell 10G PHY between a MAC and an SFP cage. You can
> then get a copper SFP module which has a PHY in it.
> 
> So:
> 
> "Linux" NIC: [DMA MAC][PHY][PHY] 
> 
> And just to make it more interesting, you sometimes see:
> 
> [MAC] - MII MUX -+---[PHY][PHY]
>                  |
>                  +---[PHY]
> 
> This is currently not supported, but there is work in progress to
> address this, by giving each PHY and ID, and extending the netlink
> ethtool so you can enumerate PHYs and individually configure them.

Yes, I have talked to maxime about his PHY ID support patch series.

> And i pointed out maybe the worst case scenario:
> 
> [MAC][PHY][PHY][MAC]switch core[MAC][PHY][PHY]

always more complex! :)

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Russell King (Oracle)]

On Tue, Sep 05, 2023 at 10:29:51PM +0200, Andrew Lunn wrote:
> > Maybe we should try to enumerate the use cases, I don't remember now
> > but I think the concern was that there may be multiple PHYs?
> 
> You often see a Marvell 10G PHY between a MAC and an SFP cage. You can
> then get a copper SFP module which has a PHY in it.
> 
> So:
> 
> "Linux" NIC: [DMA MAC][PHY][PHY] 

Let's be clear - one of the reasons that this whole topic was triggered
was because of mvpp2 plus Marvell 1G PHYs. mvpp2 has a good PTP
implementation, where as Marvell 1G PHYs are not quite as good. With
the code as it stood, merely adding PTP support to Marvell PHYs would
result in setups that use a Marvell 1G PHY with mvpp2 to break - some
PTP API calls would end up going to one PTP implementation while other
PTP API calls end up going to the other.

Once this gets solved properly, then I can think about sending the
patches that add support for PTP in Marvell 1G PHYs, and then we will
have the situation where we have a MAC and a PHY on the *same* network
interface that are PTP capable.

People have been asking me about the Marvell PHY PTP patches - and I
have had to tell them that they can't be merged because of the PTP
API crazyness.

So... it would be entirely possible, if PTP were to be implemented for
the Marvell 10G PHYs, that there would be _three_ points with a SFP
module to do PTP, although it probably does not make much sense to
attempt to do so on the SFP PHY. In any case, before we get to that
point, we first need to work out how to support multiple ethernet PHYs
on one MAC.

Even with that solved, the situation you describe above is likely to be
problematical. PHYs that connect to SFPs generally only support fibre
interface modes and not SGMII on that port which limits the usefulness
of copper SFPs - they won't be able to do 10M / 100M unless they're one
of those PHYs that does rate adaption which seem fairly rare at the
moment.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Vladimir Oltean]

On Fri, May 12, 2023 at 10:38:52AM -0700, Jakub Kicinski wrote:
> On Fri, 12 May 2023 13:29:11 +0300 Vladimir Oltean wrote:
> > On Thu, May 11, 2023 at 04:16:25PM -0700, Jakub Kicinski wrote:
> > > > I mean, I can't ignore the fact that there are NICs that can provide
> > > > 2-step TX timestamps at line rate for all packets (not just PTP) for
> > > > line rates exceeding 10G, in band with the descriptor in its TX
> > > > completion queue. I don't want to give any names, just to point out
> > > > that there isn't any inherent limitation in the technology.  
> > > 
> > > Oh, you should tell me, maybe off-list then. 'Cause I don't know any.  
> > 
> > I hope the examples given in private will make you reconsider the
> > validity of my argument about DMA timestamps.
> 
> I may have lost track of what the argument is. There are devices
> which will provide a DMA stamp for Tx and Rx. We need an API that'll
> inform the user about it. 
> 
> To be clear I'm talking about drivers which are already in the tree,
> not opening the door for some shoddy new HW in.

So this is circling back to my original question (with emphasis on the
last part):

| Which drivers provide DMA timestamps, and how do they currently signal
| that they do this? Do they do this for all packets that get timestamped,
| or for "some"?

https://lore.kernel.org/netdev/20230511203646.ihljeknxni77uu5j@skbuf/

If only "some" packets (unpredictable which) get DMA timestamps when
a MAC timestamp isn't available, what UAPI can satisfactorily describe
that? And who would want that?

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Jakub Kicinski]

On Fri, 19 May 2023 16:28:02 +0300 Vladimir Oltean wrote:
> > I may have lost track of what the argument is. There are devices
> > which will provide a DMA stamp for Tx and Rx. We need an API that'll
> > inform the user about it. 
> > 
> > To be clear I'm talking about drivers which are already in the tree,
> > not opening the door for some shoddy new HW in.  
> 
> So this is circling back to my original question (with emphasis on the
> last part):
> 
> | Which drivers provide DMA timestamps, and how do they currently signal
> | that they do this? Do they do this for all packets that get timestamped,
> | or for "some"?
> 
> https://lore.kernel.org/netdev/20230511203646.ihljeknxni77uu5j@skbuf/
> 
> If only "some" packets (unpredictable which) get DMA timestamps when
> a MAC timestamp isn't available, what UAPI can satisfactorily describe
> that? And who would want that?

The short answer is "I don't know". There's been a lot of talk about
time stamps due to Swift/BBRv2, but I don't have first hand experience
with it. That'd require time stamping "all" packets but the precision 
is really only required to be in usec.

Maybe Muhammad has a clearer use case in mind.

RE: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Zulkifli, Muhammad Husaini]

Hi All,

> On Fri, 19 May 2023 16:28:02 +0300 Vladimir Oltean wrote:
> > > I may have lost track of what the argument is. There are devices
> > > which will provide a DMA stamp for Tx and Rx. We need an API that'll
> > > inform the user about it.
> > >
> > > To be clear I'm talking about drivers which are already in the tree,
> > > not opening the door for some shoddy new HW in.
> >
> > So this is circling back to my original question (with emphasis on the
> > last part):
> >
> > | Which drivers provide DMA timestamps, and how do they currently
> > | signal that they do this? Do they do this for all packets that get
> > | timestamped, or for "some"?
> >
> > https://lore.kernel.org/netdev/20230511203646.ihljeknxni77uu5j@skbuf/
> >
> > If only "some" packets (unpredictable which) get DMA timestamps when a
> > MAC timestamp isn't available, what UAPI can satisfactorily describe
> > that? And who would want that?
> 
> The short answer is "I don't know". There's been a lot of talk about time
> stamps due to Swift/BBRv2, but I don't have first hand experience with it.
> That'd require time stamping "all" packets but the precision is really only
> required to be in usec.
> 
> Maybe Muhammad has a clearer use case in mind.

A controller may only support one HW Timestamp (PHY/Port) and one MAC Timestamp 
(DMA Timestamp) for packet timestamp activity. If a PTP packet has used the HW Timestamp (PHY/Port), 
the MAC timestamp can be used for another packet timestamp activity (ex. AF_XDP/AF_PACKET).
If all packets require and use the same HW Timestamp (PHY/Port timestamp) and we send a huge 
amount of traffic for AF_XDP/AF_PACKET, we may discover that some packets are missing the 
timestamp since every packet is attempting to read the same HW Port/PHY Timestamp register. 
You may see the tx_timestamp_timeout from ptp4l also in this scenario. 
Giving the user the choice of selecting MAC or PHY timestamp through the socket options can help 
resolve the above issue if they enable per-packet MAC(DMA) timestamping.

Thanks, 
Husaini

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Richard Cochran]

On Mon, May 22, 2023 at 03:56:36AM +0000, Zulkifli, Muhammad Husaini wrote:

> A controller may only support one HW Timestamp (PHY/Port) and one MAC Timestamp 
> (DMA Timestamp) for packet timestamp activity. If a PTP packet has used the HW Timestamp (PHY/Port), 

This is wrong.

The time stamping setting is global, at the device level, not at the
socket.  And that is not going to change.  This series is about
selecting between MAC/PHY time stamping globally, at the device level.

Thanks,
Richard

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Jakub Kicinski]

On Mon, 22 May 2023 13:04:33 -0700 Richard Cochran wrote:
> On Mon, May 22, 2023 at 03:56:36AM +0000, Zulkifli, Muhammad Husaini wrote:
> 
> > A controller may only support one HW Timestamp (PHY/Port) and one MAC Timestamp 
> > (DMA Timestamp) for packet timestamp activity. If a PTP packet has used the HW Timestamp (PHY/Port),   
> 
> This is wrong.
>
> The time stamping setting is global, at the device level, not at the
> socket.  And that is not going to change.  This series is about
> selecting between MAC/PHY time stamping globally, at the device level.

What constitutes a device?

I'd present the facts differently. This series selects which _device_
(MAC or PHY) is responsible for delivering timestamps for a given
netdev.

HW which supports different timestamping points with different
capabilities is commonplace, so an API in this vicinity should be
extended to support the configuration. Today it's configured via device
private flags, or some out-of-tree tooling, which helps nobody :|

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Richard Cochran]

On Mon, May 22, 2023 at 01:21:44PM -0700, Jakub Kicinski wrote:
> On Mon, 22 May 2023 13:04:33 -0700 Richard Cochran wrote:
> > On Mon, May 22, 2023 at 03:56:36AM +0000, Zulkifli, Muhammad Husaini wrote:
> > 
> > > A controller may only support one HW Timestamp (PHY/Port) and one MAC Timestamp 
> > > (DMA Timestamp) for packet timestamp activity. If a PTP packet has used the HW Timestamp (PHY/Port),   
> > 
> > This is wrong.
> >
> > The time stamping setting is global, at the device level, not at the
> > socket.  And that is not going to change.  This series is about
> > selecting between MAC/PHY time stamping globally, at the device level.
> 
> What constitutes a device?

Sorry, maybe I should have said "interface".

You cannot have one socket with MAC and another with DMA time stamps,
as the above post implies or wishes for.

Thanks,
Richard

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Jacob Keller]

On 5/11/2023 4:16 PM, Jakub Kicinski wrote:
> On Fri, 12 May 2023 02:07:17 +0300 Vladimir Oltean wrote:
>> AFAIU from igc_ptp_tx_hwtstamp(), it's just that the igc DMA
>> controller did not bother to transport the timestamps from the MAC
>> back into the descriptor, leaving it up to software to do it out of
>> band, which of course may cause correlation bugs and limits
>> throughput. Surely they can do better.
> 

In myy understanding for for MAC timestamping, the igc hardware stores
the timestamp in a register and assumes you only have one outstanding
timestamp request at a time.

The MAC timestamp is captured well after the Tx descriptor is written
back as complete. There's no completion queue on this device to send
such a message out-of-band of the Tx writeback. Delaying the writeback
until after the Tx timestamp is captured would have its own challenges.

Obviously it *can* be done better than this, but thats not how the igc
hardware was designed.

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Jacob Keller]

On 4/6/2023 6:46 PM, Jakub Kicinski wrote:
> On Thu,  6 Apr 2023 19:33:05 +0200 Köry Maincent wrote:
>> +/* TSLIST_GET */
>> +static int tslist_prepare_data(const struct ethnl_req_info *req_base,
>> +			       struct ethnl_reply_data *reply_base,
>> +			       struct genl_info *info)
>> +{
>> +	struct ts_reply_data *data = TS_REPDATA(reply_base);
>> +	struct net_device *dev = reply_base->dev;
>> +	const struct ethtool_ops *ops = dev->ethtool_ops;
>> +	int ret;
>> +
>> +	ret = ethnl_ops_begin(dev);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	data->ts = 0;
>> +	if (phy_has_tsinfo(dev->phydev))
>> +		data->ts = SOF_PHY_TIMESTAMPING;
>> +	if (ops->get_ts_info)
>> +		data->ts |= SOF_MAC_TIMESTAMPING;
> 
> We can't make that assumption, that info must come from the driver.
> 
> Also don't we need some way to identify the device / phc from which 
> the timestamp at the given layer will come?

For example, ice hardware captures timestamp data in its internal PHY
well after the MAC layer finishes, but it doesn't expose the PHY to the
host at all..

From a timing perspective it matters that its PHY, but from an
implementation perspective there's not much difference since we don't
support MAC timestamping at all (and the PHY isn't accessible through
phylink...)

Perhaps that doesn't fit the use cace here though where the issue is
with supporting both MAC and PHY but no way to configure that

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Richard Cochran]

On Wed, May 17, 2023 at 03:13:06PM -0700, Jacob Keller wrote:

> For example, ice hardware captures timestamp data in its internal PHY
> well after the MAC layer finishes, but it doesn't expose the PHY to the
> host at all..
> 
> From a timing perspective it matters that its PHY, but from an
> implementation perspective there's not much difference since we don't
> support MAC timestamping at all (and the PHY isn't accessible through
> phylink...)

Here is a crazy idea:  Wouldn't it be nice to have all PHYs represented
in the kernel driver world, even those PHYs that are built in?

I've long thought that having NETWORK_PHY_TIMESTAMPING limited to
PHYLIB (and in practice device tree) systems is unfortunate.

Thanks,
Richard

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Jacob Keller]

On 5/17/2023 3:46 PM, Richard Cochran wrote:
> On Wed, May 17, 2023 at 03:13:06PM -0700, Jacob Keller wrote:
> 
>> For example, ice hardware captures timestamp data in its internal PHY
>> well after the MAC layer finishes, but it doesn't expose the PHY to the
>> host at all..
>>
>> From a timing perspective it matters that its PHY, but from an
>> implementation perspective there's not much difference since we don't
>> support MAC timestamping at all (and the PHY isn't accessible through
>> phylink...)
> 
> Here is a crazy idea:  Wouldn't it be nice to have all PHYs represented
> in the kernel driver world, even those PHYs that are built in?
> 

I agree. I've wanted us to enable phylib/phylink/something for our
internal PHYs, but never got traction here to actually make it happen.
There was a push a few years ago for using it in igb, but ultimately
couldn't get enough support to make the development happen :( Similar
with using the i2c interfaces... These days, so much of the control
happens only in firmware that it has its own challenges.

> I've long thought that having NETWORK_PHY_TIMESTAMPING limited to
> PHYLIB (and in practice device tree) systems is unfortunate.
> 

It is a bit unfortunate. In the ice driver case, we just report the
timestamps in the usual way for a network driver, which is ok enough
since no other timestamps exist for us.

> Thanks,
> Richard

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Andrew Lunn]

On Thu, May 18, 2023 at 04:23:10PM -0700, Jacob Keller wrote:
> 
> 
> On 5/17/2023 3:46 PM, Richard Cochran wrote:
> > On Wed, May 17, 2023 at 03:13:06PM -0700, Jacob Keller wrote:
> > 
> >> For example, ice hardware captures timestamp data in its internal PHY
> >> well after the MAC layer finishes, but it doesn't expose the PHY to the
> >> host at all..
> >>
> >> From a timing perspective it matters that its PHY, but from an
> >> implementation perspective there's not much difference since we don't
> >> support MAC timestamping at all (and the PHY isn't accessible through
> >> phylink...)
> > 
> > Here is a crazy idea:  Wouldn't it be nice to have all PHYs represented
> > in the kernel driver world, even those PHYs that are built in?
> > 
> 
> I agree. I've wanted us to enable phylib/phylink/something for our
> internal PHYs, but never got traction here to actually make it happen.
> There was a push a few years ago for using it in igb, but ultimately
> couldn't get enough support to make the development happen :( Similar
> with using the i2c interfaces... These days, so much of the control
> happens only in firmware that it has its own challenges.

I know there is some cleanup going on reducing replicated code in
Intel Ethernet drivers. I was wondering if that would extend to
PHYs. But as you say, recent Intel hardware have firmware controlling
the PHYs, not Linux. So such cleanups would be limited to older
controllers which i guess Intel probably no longer cares about.

> > I've long thought that having NETWORK_PHY_TIMESTAMPING limited to
> > PHYLIB (and in practice device tree) systems is unfortunate.
> > 
> 
> It is a bit unfortunate. In the ice driver case, we just report the
> timestamps in the usual way for a network driver, which is ok enough
> since no other timestamps exist for us.

I would actually say there is nothing fundamentally blocking using
NETWORK_PHY_TIMESTAMPING with something other than DT. It just needs
somebody to lead the way.

For ACPI in the scope of networking, everybody just seems to accept DT
won, and stuffs DT properties into ACPI tables. For PCI devices, there
has been some good work being done by Trustnetic using software nodes,
for gluing together GPIO controllers, I2C controller, SFP and
PHYLINK. It should be possible to do the same with PHY timestampers.

	 Andrew

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Richard Cochran]

On Fri, May 19, 2023 at 02:50:42PM +0200, Andrew Lunn wrote:

> I would actually say there is nothing fundamentally blocking using
> NETWORK_PHY_TIMESTAMPING with something other than DT. It just needs
> somebody to lead the way.

+1
 
> For ACPI in the scope of networking, everybody just seems to accept DT
> won, and stuffs DT properties into ACPI tables.

Is that stuff mainline?

> For PCI devices, there
> has been some good work being done by Trustnetic using software nodes,
> for gluing together GPIO controllers, I2C controller, SFP and
> PHYLINK.

mainline also?

> It should be possible to do the same with PHY timestampers.

Sounds promising...

Thanks,
Richard

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Andrew Lunn]

On Fri, May 19, 2023 at 06:50:19AM -0700, Richard Cochran wrote:
> On Fri, May 19, 2023 at 02:50:42PM +0200, Andrew Lunn wrote:
> 
> > I would actually say there is nothing fundamentally blocking using
> > NETWORK_PHY_TIMESTAMPING with something other than DT. It just needs
> > somebody to lead the way.
> 
> +1
>  
> > For ACPI in the scope of networking, everybody just seems to accept DT
> > won, and stuffs DT properties into ACPI tables.
> 
> Is that stuff mainline?

There is some support. But it is somewhat limited.

ACPI and networking is an odd area. ACPI has historically been
x86. And few x86 SoCs have integrated networking. Those that do seem
to me to be PCI devices internally glued onto the PCIe bus, and
firmware driving the hardware, not Linux.

Integrated networking is much more popular for other architectures
SoCs, ARM, MIPS, PowerPc. These are all DT. And in general, Linux is
controlling the hardware, which is why we have good standardised DT
bindings for MDIO busses, PHYs, SFPs, etc.

Then ARM pushed ACPI for server class ARM systems. Now server class
systems generally don't have integrated Ethernet. They have lots of
PCIe lanes, and it seems normal to put one or more NICs on PCIe. That
also gives the flexibility you can get a high performance DPU from a
network specialist, or just a plain boring 10G PCIe device. As a
result, ACPI not saying anything about networking is not really an
issue for server class machines.

The little interest i've seen for ACPI networking has come from
'hobbist' trying to use ACPI on ARM systems which are not intended to
be servers. Generally, there is a fully working DT description of the
hardware, and Linux is happy to control the hardware using that DT
description. Getting ACPI to work is mostly straight forward, due to
most building blocks being standard. xhci for USB, ata for block
devices, etc.  But they then run into the complete lack of
standardisation for networking, and nothing at all about networking in
the ACPI standard. And these people tend not to be ACPI gurus who
could extend ACPI to cover the complexity of networking hardware. So
they just stuff the existing DT properties into ACPI tables and call
it done. And i have to push back on this, because they try to stuff
everything in, including properties we have deprecated because DT has
a long history and we got things wrong along the way.
 
> > For PCI devices, there
> > has been some good work being done by Trustnetic using software nodes,
> > for gluing together GPIO controllers, I2C controller, SFP and
> > PHYLINK.
> 
> mainline also?

On the way. Trustnetic got thrown in the deep end. They are new to
mainline. They brought a typical "vendor crap driver" and tried to get
it mainlined. It reinvented everything rather then reuse what already
exists in Linux. So they are effectively writing a new driver under
our guidance. It is an unusual device, because it is a PCIe device,
but without firmware. Linux controls everything. So they have the
double trouble of being mainline newbies, plus having to do things
with mainline that nobody else has done before in order to support
their hardware. So it is moving slow. But they are sticking at it, so
i think in the end they will get it working, and it could become a
reference for others to follow.

      Andrew

Re: [PATCH net-next RFC v4 2/5] net: Expose available time stamping layers to user space.

[Richard Cochran]

On Thu, Apr 06, 2023 at 07:33:05PM +0200, Köry Maincent wrote:
> Signed-off-by: Richard Cochran <richardcochran@gmail.com>
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>

Since this patch is now completely re-written, you can drop my Sob and
authorship, keeping all the credit for yourself!

Thanks,
Richard

[PATCH net-next RFC v4 3/5] dt-bindings: net: phy: add timestamp preferred choice property

[Köry Maincent]

From: Kory Maincent <kory.maincent@bootlin.com>

Add property to select the preferred hardware timestamp layer.
The choice of using devicetree binding has been made as the PTP precision
and quality depends of external things, like adjustable clock, or the lack
of a temperature compensated crystal or specific features. Even if the
preferred timestamp is a configuration it is hardly related to the design
of the board.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 Documentation/devicetree/bindings/net/ethernet-phy.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
index ac04f8efa35c..32d7ef7520e6 100644
--- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
@@ -144,6 +144,13 @@ properties:
       Mark the corresponding energy efficient ethernet mode as
       broken and request the ethernet to stop advertising it.
 
+  preferred-timestamp:
+    enum:
+      - phy
+      - mac
+    description:
+      Specifies the preferred hardware timestamp layer.
+
   pses:
     $ref: /schemas/types.yaml#/definitions/phandle-array
     maxItems: 1
-- 
2.25.1

Re: [PATCH net-next RFC v4 3/5] dt-bindings: net: phy: add timestamp preferred choice property

[Vladimir Oltean]

On Thu, Apr 06, 2023 at 07:33:06PM +0200, Köry Maincent wrote:
> From: Kory Maincent <kory.maincent@bootlin.com>
> 
> Add property to select the preferred hardware timestamp layer.
> The choice of using devicetree binding has been made as the PTP precision
> and quality depends of external things, like adjustable clock, or the lack
> of a temperature compensated crystal or specific features. Even if the
> preferred timestamp is a configuration it is hardly related to the design
> of the board.
> 
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> ---
>  Documentation/devicetree/bindings/net/ethernet-phy.yaml | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> index ac04f8efa35c..32d7ef7520e6 100644
> --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> @@ -144,6 +144,13 @@ properties:
>        Mark the corresponding energy efficient ethernet mode as
>        broken and request the ethernet to stop advertising it.
>  
> +  preferred-timestamp:
> +    enum:
> +      - phy
> +      - mac
> +    description:
> +      Specifies the preferred hardware timestamp layer.
> +
>    pses:
>      $ref: /schemas/types.yaml#/definitions/phandle-array
>      maxItems: 1
> -- 
> 2.25.1
>

Do we need this device tree functionality?

Re: [PATCH net-next RFC v4 3/5] dt-bindings: net: phy: add timestamp preferred choice property

[Köry Maincent]

On Wed, 12 Apr 2023 16:14:21 +0300
Vladimir Oltean <vladimir.oltean@nxp.com> wrote:

> > +  preferred-timestamp:
> > +    enum:
> > +      - phy
> > +      - mac
> > +    description:
> > +      Specifies the preferred hardware timestamp layer.
> > +
> >    pses:
> >      $ref: /schemas/types.yaml#/definitions/phandle-array
> >      maxItems: 1
> > -- 
> > 2.25.1
> >  
> 
> Do we need this device tree functionality?

I would say so. Expected as I wrote the patch. ;)

My point was that the new behavior to MAC as default timestamping does not fit
all the case, especially when a board is designed with PHY like the TI PHYTER
which is a far better timestamping choice (according to Richard). The user
doesn't need to know this, he wants to have the better time stamp selected by
default without any investigation. That's why having devicetree property for
that could be useful. 

Re: [PATCH net-next RFC v4 3/5] dt-bindings: net: phy: add timestamp preferred choice property

[Vladimir Oltean]

On Wed, Apr 12, 2023 at 03:44:46PM +0200, Köry Maincent wrote:
> On Wed, 12 Apr 2023 16:14:21 +0300 Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> > Do we need this device tree functionality?
> 
> I would say so. Expected as I wrote the patch. ;)
> 
> My point was that the new behavior to MAC as default timestamping does not fit
> all the case, especially when a board is designed with PHY like the TI PHYTER
> which is a far better timestamping choice (according to Richard). The user
> doesn't need to know this, he wants to have the better time stamp selected by
> default without any investigation. That's why having devicetree property for
> that could be useful.

The TI PHYTER is the "NatSemi DP83640" entry in the whitelist for PHYs
that still use their timestamping by default. Can you please come up
with an example which is actually useful?

Re: [PATCH net-next RFC v4 3/5] dt-bindings: net: phy: add timestamp preferred choice property

[Köry Maincent]

On Sat, 29 Apr 2023 20:42:17 +0300
Vladimir Oltean <vladimir.oltean@nxp.com> wrote:

> On Wed, Apr 12, 2023 at 03:44:46PM +0200, Köry Maincent wrote:
> > On Wed, 12 Apr 2023 16:14:21 +0300 Vladimir Oltean
> > <vladimir.oltean@nxp.com> wrote:  
> > > Do we need this device tree functionality?  
> > 
> > I would say so. Expected as I wrote the patch. ;)
> > 
> > My point was that the new behavior to MAC as default timestamping does not
> > fit all the case, especially when a board is designed with PHY like the TI
> > PHYTER which is a far better timestamping choice (according to Richard).
> > The user doesn't need to know this, he wants to have the better time stamp
> > selected by default without any investigation. That's why having devicetree
> > property for that could be useful.  
> 
> The TI PHYTER is the "NatSemi DP83640" entry in the whitelist for PHYs
> that still use their timestamping by default. Can you please come up
> with an example which is actually useful?

If a future PHY, featured like this TI PHYTER, is supported in the future the
default timestamp will be the MAC and we won't be able to select the PHY by
default.
Another example is my case with the 88E151x PHY, on the Russell side with the
Macchiatobin board, the MAC is more precise, and in my side with a custom board
with macb MAC, the PHY is more precise. Be able to select the prefer one from
devicetree is convenient.

Re: [PATCH net-next RFC v4 3/5] dt-bindings: net: phy: add timestamp preferred choice property

[Vladimir Oltean]

On Tue, May 02, 2023 at 11:10:43AM +0200, Köry Maincent wrote:
> If a future PHY, featured like this TI PHYTER, is supported in the future the
> default timestamp will be the MAC and we won't be able to select the PHY by
> default.
> Another example is my case with the 88E151x PHY, on the Russell side with the
> Macchiatobin board, the MAC is more precise, and in my side with a custom board
> with macb MAC, the PHY is more precise. Be able to select the prefer one from
> devicetree is convenient.

If convenience is all that there is, I guess that's not a very strong
argument for putting something in the device tree which couldn't have
been handled by user space through an init script, and nothing would
have been broken as a result of that.

Re: [PATCH net-next RFC v4 3/5] dt-bindings: net: phy: add timestamp preferred choice property

[Köry Maincent]

On Thu, 11 May 2023 16:10:08 +0300
Vladimir Oltean <vladimir.oltean@nxp.com> wrote:

> On Tue, May 02, 2023 at 11:10:43AM +0200, Köry Maincent wrote:
> > If a future PHY, featured like this TI PHYTER, is supported in the future
> > the default timestamp will be the MAC and we won't be able to select the
> > PHY by default.
> > Another example is my case with the 88E151x PHY, on the Russell side with
> > the Macchiatobin board, the MAC is more precise, and in my side with a
> > custom board with macb MAC, the PHY is more precise. Be able to select the
> > prefer one from devicetree is convenient.  
> 
> If convenience is all that there is, I guess that's not a very strong
> argument for putting something in the device tree which couldn't have
> been handled by user space through an init script, and nothing would
> have been broken as a result of that.

The user may not and don't need to know which hardware timestamping is better.
He just want to use the best one by default without investigation and
benchmarking.
It is more related to the hardware design of the board which should be
described in the devicetree, don't you think? Of course it should not break
anything and if it does, well then let the user select it in userspace.
But if you really think my point is irrelevant then I will drop this feature.

Re: [PATCH net-next RFC v4 3/5] dt-bindings: net: phy: add timestamp preferred choice property

[Vladimir Oltean]

On Thu, May 11, 2023 at 03:25:50PM +0200, Köry Maincent wrote:
> The user may not and don't need to know which hardware timestamping is better.
> He just want to use the best one by default without investigation and
> benchmarking.
> It is more related to the hardware design of the board which should be
> described in the devicetree, don't you think? Of course it should not break
> anything and if it does, well then let the user select it in userspace.
> But if you really think my point is irrelevant then I will drop this feature.

You are putting an equality sign between user space and the end-user of
a system. A user space distribution has a lot of configuration files
where the end user isn't expected to know how to configure them all, and
that's not an argument for putting them in the kernel/device tree, is it?

I can relate to 2 examples which are closer to what I know slightly
better (Documentation/devicetree/bindings/net/dsa/dsa-port.yaml).

One is "label" (the netdev name of a switch port). It has been argued
that we should deprecate this, because udev permits selecting specific
netdev named based on hardware properties already.

I agree with this, and this is why on NXP LS1028A, we don't use "label"
in the device tree, but advise people to ship this in the rootfs:

cat /etc/udev/rules.d/10-network.rules
# ENETC rules
ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:00.0", DRIVERS=="fsl_enetc", NAME:="eno0"
ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:00.1", DRIVERS=="fsl_enetc", NAME:="eno1"
ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:00.2", DRIVERS=="fsl_enetc", NAME:="eno2"
ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:00.6", DRIVERS=="fsl_enetc", NAME:="eno3"
ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:01.0", DRIVERS=="fsl_enetc_vf", NAME:="eno0vf0"
ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:01.1", DRIVERS=="fsl_enetc_vf", NAME:="eno0vf1"
ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:01.2", DRIVERS=="fsl_enetc_vf", NAME:="eno1vf0"
ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:01.3", DRIVERS=="fsl_enetc_vf", NAME:="eno1vf1"
# LS1028 switch rules
ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:00.5", DRIVERS=="mscc_felix", ATTR{phys_port_name}=="p0", NAME="swp0"
ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:00.5", DRIVERS=="mscc_felix", ATTR{phys_port_name}=="p1", NAME="swp1"
ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:00.5", DRIVERS=="mscc_felix", ATTR{phys_port_name}=="p2", NAME="swp2"
ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:00.5", DRIVERS=="mscc_felix", ATTR{phys_port_name}=="p3", NAME="swp3"
ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:00.5", DRIVERS=="mscc_felix", ATTR{phys_port_name}=="p4", NAME="swp4"
ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:00.5", DRIVERS=="mscc_felix", ATTR{phys_port_name}=="p5", NAME="swp5"

The other example is "dsa-tag-protocol", which you'd normally expect
that user space would select through /sys/class/net/ethN/dsa/tagging and
that would be the end of the story. I was only convinced to let it live
in the device tree because a tagging protocol change might be necessary
for traffic on the port to work at all, and if traffic doesn't work,
then the kernel can't load userspace through e.g. NFS, and thus, user
space can't change this setting. In your case, I don't think this
argument applies.

I guess the general rule of thumb is - if a functionality can live outside
the kernel or of the device tree, it's probably better that it does.

Re: [PATCH net-next RFC v4 3/5] dt-bindings: net: phy: add timestamp preferred choice property

[Köry Maincent]

On Thu, 11 May 2023 16:56:31 +0300
Vladimir Oltean <vladimir.oltean@nxp.com> wrote:

> On Thu, May 11, 2023 at 03:25:50PM +0200, Köry Maincent wrote:

> 
> I guess the general rule of thumb is - if a functionality can live outside
> the kernel or of the device tree, it's probably better that it does.

Ok it is clearer to me. Thanks for your explanation! Then I will drop the
device tree parameter.

Re: [PATCH net-next RFC v4 3/5] dt-bindings: net: phy: add timestamp preferred choice property

[Krzysztof Kozlowski]

On 06/04/2023 19:33, Köry Maincent wrote:
> From: Kory Maincent <kory.maincent@bootlin.com>
> 
> Add property to select the preferred hardware timestamp layer.
> The choice of using devicetree binding has been made as the PTP precision
> and quality depends of external things, like adjustable clock, or the lack
> of a temperature compensated crystal or specific features. Even if the
> preferred timestamp is a configuration it is hardly related to the design
> of the board.

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC.  It might happen, that command when run on an older
kernel, gives you outdated entries.  Therefore please be sure you base
your patches on recent Linux kernel.

Since you skipped some entries, no tests will be executed here. Resend
following Linux kernel process.


Best regards,
Krzysztof

[PATCH net-next RFC v4 4/5] net: Let the active time stamping layer be selectable.

[Köry Maincent]

From: Kory Maincent <kory.maincent@bootlin.com>

Add the ETHTOOL_MSG_TS_SET ethtool netlink socket, and add checks in the
ioctl and time stamping paths to respect the currently selected time
stamping layer.

Add a preferred-timestamp devicetree binding to select the preferred
hardware timestamp layer between PHY and MAC. The choice of using
devicetree binding has been made as the PTP precision and quality depends
of external things, like adjustable clock, or the lack of a temperature
compensated crystal or specific features. Even if the preferred timestamp
is a configuration it is hardly related to the design oh the board.

Introduce a NETDEV_CHANGE_HWTSTAMP netdev notifier to let MAC or DSA know
when a hwtstamp change happen. This is useful to manage packet trap in that
case like done by the lan966x driver.

Change the API to select MAC default time stamping instead of the PHY.
Indeed the PHY is closer to the wire therefore theoretically it have less
delay than the MAC timestamping but the reality is different. Due to lower
time stamping clock frequency, latency in the MDIO bus and no PHC hardware
synchronization between different PHY, the PHY PTP is often less precise
than the MAC. The exception is for PHY designed specially for PTP case but
these board are not very widespread. For not breaking the compatibility I
introduce a whitelist to reference all current PHY that support
time stamping and let them keep the old API behavior.

Signed-off-by: Richard Cochran <richardcochran@gmail.com>
Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---

Notes:
    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.

 Documentation/networking/ethtool-netlink.rst | 13 +++
 drivers/net/phy/phy_device.c                 | 85 ++++++++++++++++++++
 include/linux/netdevice.h                    | 12 +++
 include/uapi/linux/ethtool_netlink.h         |  2 +
 net/core/dev.c                               |  2 +-
 net/core/dev_ioctl.c                         | 56 ++++++++++++-
 net/core/timestamping.c                      |  6 ++
 net/ethtool/common.c                         | 15 +++-
 net/ethtool/netlink.c                        | 10 +++
 net/ethtool/netlink.h                        |  1 +
 net/ethtool/ts.c                             | 68 ++++++++++++++--
 11 files changed, 256 insertions(+), 14 deletions(-)

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index 539425fdaf7c..12996f38b956 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -2028,6 +2028,18 @@ Kernel response contents:
   ``ETHTOOL_A_TS_LAYER``   u32     available hardware timestamping
   =======================  ======  ===================================
 
+TS_SET
+======
+
+Gets transceiver module parameters.
+
+Request contents:
+
+  =============================  ======  ==============================
+  ``ETHTOOL_A_TS_HEADER``        nested  request header
+  ``ETHTOOL_A_TS_LAYER``         u32     set hardware timestamping
+  =============================  ======  ==============================
+
 Request translation
 ===================
 
@@ -2136,4 +2148,5 @@ are netlink only.
   n/a                                 ``ETHTOOL_MSG_MM_SET``
   n/a                                 ``ETHTOOL_MSG_TS_GET``
   n/a                                 ``ETHTOOL_MSG_TSLIST_GET``
+  n/a                                 ``ETHTOOL_MSG_TS_SET``
   =================================== =====================================
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 917ba84105fc..d415c62938cd 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -34,6 +34,8 @@
 #include <linux/string.h>
 #include <linux/uaccess.h>
 #include <linux/unistd.h>
+#include <linux/of.h>
+#include <uapi/linux/net_tstamp.h>
 
 MODULE_DESCRIPTION("PHY library");
 MODULE_AUTHOR("Andy Fleming");
@@ -1408,6 +1410,83 @@ int phy_sfp_probe(struct phy_device *phydev,
 }
 EXPORT_SYMBOL(phy_sfp_probe);
 
+/**
+ * A whitelist for PHYs selected as default timesetamping.
+ * Its use is to keep compatibility with old PTP API which is selecting
+ * these PHYs as default timestamping.
+ * The new API is selecting the MAC as default timestamping.
+ */
+const char * const phy_timestamping_whitelist[] = {
+	"Broadcom BCM5411",
+	"Broadcom BCM5421",
+	"Broadcom BCM54210E",
+	"Broadcom BCM5461",
+	"Broadcom BCM54612E",
+	"Broadcom BCM5464",
+	"Broadcom BCM5481",
+	"Broadcom BCM54810",
+	"Broadcom BCM54811",
+	"Broadcom BCM5482",
+	"Broadcom BCM50610",
+	"Broadcom BCM50610M",
+	"Broadcom BCM57780",
+	"Broadcom BCM5395",
+	"Broadcom BCM53125",
+	"Broadcom BCM53128",
+	"Broadcom BCM89610",
+	"NatSemi DP83640",
+	"Microchip LAN8841 Gigabit PHY"
+	"Microchip INDY Gigabit Quad PHY",
+	"Microsemi GE VSC856X SyncE",
+	"Microsemi GE VSC8575 SyncE",
+	"Microsemi GE VSC8582 SyncE",
+	"Microsemi GE VSC8584 SyncE",
+	"NXP C45 TJA1103",
+	NULL,
+};
+
+static void of_set_timestamp(struct net_device *netdev, struct phy_device *phydev)
+{
+	struct device_node *node = phydev->mdio.dev.of_node;
+	const struct ethtool_ops *ops = netdev->ethtool_ops;
+	const char *s;
+	enum timestamping_layer ts_layer = 0;
+	int i;
+
+	/* Backward compatibility to old API */
+	for (i = 0; phy_timestamping_whitelist[i] != NULL; i++)
+	{
+		if (!strcmp(phy_timestamping_whitelist[i],
+			    phydev->drv->name)) {
+			if (phy_has_hwtstamp(phydev))
+				ts_layer = SOF_PHY_TIMESTAMPING;
+			else if (ops->get_ts_info)
+				ts_layer = SOF_MAC_TIMESTAMPING;
+			goto out;
+		}
+	}
+
+	if (ops->get_ts_info)
+		ts_layer = SOF_MAC_TIMESTAMPING;
+	else if (phy_has_hwtstamp(phydev))
+		ts_layer = SOF_PHY_TIMESTAMPING;
+
+	if (of_property_read_string(node, "preferred-timestamp", &s))
+		goto out;
+
+	if (!s)
+		goto out;
+
+	if (phy_has_hwtstamp(phydev) && !strcmp(s, "phy"))
+		ts_layer = SOF_PHY_TIMESTAMPING;
+
+	if (ops->get_ts_info && !strcmp(s, "mac"))
+		ts_layer = SOF_MAC_TIMESTAMPING;
+
+out:
+	netdev->selected_timestamping_layer = ts_layer;
+}
+
 /**
  * phy_attach_direct - attach a network device to a given PHY device pointer
  * @dev: network device to attach
@@ -1481,6 +1560,8 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 
 	phydev->phy_link_change = phy_link_change;
 	if (dev) {
+		of_set_timestamp(dev, phydev);
+
 		phydev->attached_dev = dev;
 		dev->phydev = phydev;
 
@@ -1811,6 +1892,10 @@ void phy_detach(struct phy_device *phydev)
 
 	phy_suspend(phydev);
 	if (dev) {
+		if (dev->ethtool_ops->get_ts_info)
+			dev->selected_timestamping_layer = SOF_MAC_TIMESTAMPING;
+		else
+			dev->selected_timestamping_layer = 0;
 		phydev->attached_dev->phydev = NULL;
 		phydev->attached_dev = NULL;
 	}
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index a740be3bb911..3dd6be012daf 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -48,6 +48,7 @@
 #include <uapi/linux/if_bonding.h>
 #include <uapi/linux/pkt_cls.h>
 #include <uapi/linux/netdev.h>
+#include <uapi/linux/net_tstamp.h>
 #include <linux/hashtable.h>
 #include <linux/rbtree.h>
 #include <net/net_trackers.h>
@@ -2019,6 +2020,9 @@ enum netdev_ml_priv_type {
  *
  *	@threaded:	napi threaded mode is enabled
  *
+ *	@selected_timestamping_layer:	Tracks whether the MAC or the PHY
+ *					performs packet time stamping.
+ *
  *	@net_notifier_list:	List of per-net netdev notifier block
  *				that follow this device when it is moved
  *				to another network namespace.
@@ -2388,6 +2392,8 @@ struct net_device {
 	unsigned		wol_enabled:1;
 	unsigned		threaded:1;
 
+	enum timestamping_layer selected_timestamping_layer;
+
 	struct list_head	net_notifier_list;
 
 #if IS_ENABLED(CONFIG_MACSEC)
@@ -2879,6 +2885,7 @@ enum netdev_cmd {
 	NETDEV_OFFLOAD_XSTATS_REPORT_DELTA,
 	NETDEV_XDP_FEAT_CHANGE,
 	NETDEV_PRE_CHANGE_HWTSTAMP,
+	NETDEV_CHANGE_HWTSTAMP,
 };
 const char *netdev_cmd_to_name(enum netdev_cmd cmd);
 
@@ -2934,6 +2941,11 @@ struct netdev_notifier_hwtstamp_info {
 	struct kernel_hwtstamp_config *config;
 };
 
+struct netdev_notifier_hwtstamp_layer {
+	struct netdev_notifier_info info; /* must be first */
+	enum timestamping_layer ts_layer;
+};
+
 enum netdev_offload_xstats_type {
 	NETDEV_OFFLOAD_XSTATS_TYPE_L3 = 1,
 };
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 447908393b91..4f03e7cde271 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -41,6 +41,7 @@ enum {
 	ETHTOOL_MSG_TSINFO_GET,
 	ETHTOOL_MSG_TSLIST_GET,
 	ETHTOOL_MSG_TS_GET,
+	ETHTOOL_MSG_TS_SET,
 	ETHTOOL_MSG_CABLE_TEST_ACT,
 	ETHTOOL_MSG_CABLE_TEST_TDR_ACT,
 	ETHTOOL_MSG_TUNNEL_INFO_GET,
@@ -96,6 +97,7 @@ enum {
 	ETHTOOL_MSG_TSINFO_GET_REPLY,
 	ETHTOOL_MSG_TSLIST_GET_REPLY,
 	ETHTOOL_MSG_TS_GET_REPLY,
+	ETHTOOL_MSG_TS_NTF,
 	ETHTOOL_MSG_CABLE_TEST_NTF,
 	ETHTOOL_MSG_CABLE_TEST_TDR_NTF,
 	ETHTOOL_MSG_TUNNEL_INFO_GET_REPLY,
diff --git a/net/core/dev.c b/net/core/dev.c
index 7ce5985be84b..481f03ef2283 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1612,7 +1612,7 @@ const char *netdev_cmd_to_name(enum netdev_cmd cmd)
 	N(SVLAN_FILTER_PUSH_INFO) N(SVLAN_FILTER_DROP_INFO)
 	N(PRE_CHANGEADDR) N(OFFLOAD_XSTATS_ENABLE) N(OFFLOAD_XSTATS_DISABLE)
 	N(OFFLOAD_XSTATS_REPORT_USED) N(OFFLOAD_XSTATS_REPORT_DELTA)
-	N(XDP_FEAT_CHANGE) N(PRE_CHANGE_HWTSTAMP)
+	N(XDP_FEAT_CHANGE) N(PRE_CHANGE_HWTSTAMP) N(CHANGE_HWTSTAMP)
 	}
 #undef N
 	return "UNKNOWN_NETDEV_EVENT";
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index 6d772837eb3f..210ff1fd0574 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -7,6 +7,7 @@
 #include <linux/net_tstamp.h>
 #include <linux/wireless.h>
 #include <linux/if_bridge.h>
+#include <linux/phy.h>
 #include <net/dsa.h>
 #include <net/wext.h>
 
@@ -252,9 +253,60 @@ static int dev_eth_ioctl(struct net_device *dev,
 	return ops->ndo_eth_ioctl(dev, ifr, cmd);
 }
 
+static int __dev_eth_ioctl(struct net_device *dev,
+			   struct ifreq *ifr, unsigned int cmd)
+{
+	struct netdev_notifier_hwtstamp_layer hwtstamp_layer = {
+		.info.dev = dev,
+		.ts_layer = dev->selected_timestamping_layer,
+	};
+	const struct net_device_ops *ops = dev->netdev_ops;
+	struct netlink_ext_ack extack = {};
+	int err;
+
+	if (!netif_device_present(dev))
+		return -ENODEV;
+
+	switch (dev->selected_timestamping_layer) {
+	case SOF_MAC_TIMESTAMPING:
+		if (ops->ndo_do_ioctl == phy_do_ioctl) {
+			/* Some drivers set .ndo_do_ioctl to phy_do_ioctl. */
+			return -EOPNOTSUPP;
+		} else {
+			err = dev_eth_ioctl(dev, ifr, cmd);
+			if (err)
+				goto out;
+		}
+		break;
+
+	case SOF_PHY_TIMESTAMPING:
+		if (phy_has_hwtstamp(dev->phydev)) {
+			err = phy_mii_ioctl(dev->phydev, ifr, cmd);
+			if (err)
+				goto out;
+		} else {
+			return -ENODEV;
+		}
+		break;
+	}
+
+	hwtstamp_layer.info.extack = &extack;
+
+	err = call_netdevice_notifiers_info(NETDEV_CHANGE_HWTSTAMP,
+					    &hwtstamp_layer.info);
+	err = notifier_to_errno(err);
+	if (err) {
+		if (extack._msg)
+			netdev_err(dev, "%s\n", extack._msg);
+	}
+
+out:
+	return err;
+}
+
 static int dev_get_hwtstamp(struct net_device *dev, struct ifreq *ifr)
 {
-	return dev_eth_ioctl(dev, ifr, SIOCGHWTSTAMP);
+	return __dev_eth_ioctl(dev, ifr, SIOCGHWTSTAMP);
 }
 
 static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr)
@@ -288,7 +340,7 @@ static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr)
 		return err;
 	}
 
-	return dev_eth_ioctl(dev, ifr, SIOCSHWTSTAMP);
+	return __dev_eth_ioctl(dev, ifr, SIOCSHWTSTAMP);
 }
 
 static int dev_siocbond(struct net_device *dev,
diff --git a/net/core/timestamping.c b/net/core/timestamping.c
index 04840697fe79..8bd7b2c21ab6 100644
--- a/net/core/timestamping.c
+++ b/net/core/timestamping.c
@@ -28,6 +28,9 @@ void skb_clone_tx_timestamp(struct sk_buff *skb)
 	if (!skb->sk)
 		return;
 
+	if (skb->dev->selected_timestamping_layer != SOF_PHY_TIMESTAMPING)
+		return;
+
 	type = classify(skb);
 	if (type == PTP_CLASS_NONE)
 		return;
@@ -50,6 +53,9 @@ bool skb_defer_rx_timestamp(struct sk_buff *skb)
 	if (!skb->dev || !skb->dev->phydev || !skb->dev->phydev->mii_ts)
 		return false;
 
+	if (skb->dev->selected_timestamping_layer != SOF_PHY_TIMESTAMPING)
+		return false;
+
 	if (skb_headroom(skb) < ETH_HLEN)
 		return false;
 
diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index 695c7c4a816b..daea7221dd25 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -637,10 +637,17 @@ int __ethtool_get_ts_info(struct net_device *dev, struct ethtool_ts_info *info)
 	memset(info, 0, sizeof(*info));
 	info->cmd = ETHTOOL_GET_TS_INFO;
 
-	if (phy_has_tsinfo(phydev))
-		return phy_ts_info(phydev, info);
-	if (ops->get_ts_info)
-		return ops->get_ts_info(dev, info);
+	switch (dev->selected_timestamping_layer) {
+	case SOF_MAC_TIMESTAMPING:
+		if (ops->get_ts_info)
+			return ops->get_ts_info(dev, info);
+		break;
+
+	case SOF_PHY_TIMESTAMPING:
+		if (phy_has_tsinfo(phydev))
+			return phy_ts_info(phydev, info);
+		return -ENODEV;
+	}
 
 	info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |
 				SOF_TIMESTAMPING_SOFTWARE;
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 8d9e27b13e28..b753b7da51f1 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -294,6 +294,7 @@ ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = {
 	[ETHTOOL_MSG_FEC_SET]		= &ethnl_fec_request_ops,
 	[ETHTOOL_MSG_TSINFO_GET]	= &ethnl_tsinfo_request_ops,
 	[ETHTOOL_MSG_TS_GET]		= &ethnl_ts_request_ops,
+	[ETHTOOL_MSG_TS_SET]		= &ethnl_ts_request_ops,
 	[ETHTOOL_MSG_TSLIST_GET]	= &ethnl_tslist_request_ops,
 	[ETHTOOL_MSG_MODULE_EEPROM_GET]	= &ethnl_module_eeprom_request_ops,
 	[ETHTOOL_MSG_STATS_GET]		= &ethnl_stats_request_ops,
@@ -671,6 +672,7 @@ ethnl_default_notify_ops[ETHTOOL_MSG_KERNEL_MAX + 1] = {
 	[ETHTOOL_MSG_MODULE_NTF]	= &ethnl_module_request_ops,
 	[ETHTOOL_MSG_PLCA_NTF]		= &ethnl_plca_cfg_request_ops,
 	[ETHTOOL_MSG_MM_NTF]		= &ethnl_mm_request_ops,
+	[ETHTOOL_MSG_TS_NTF]		= &ethnl_ts_request_ops,
 };
 
 /* default notification handler */
@@ -766,6 +768,7 @@ static const ethnl_notify_handler_t ethnl_notify_handlers[] = {
 	[ETHTOOL_MSG_MODULE_NTF]	= ethnl_default_notify,
 	[ETHTOOL_MSG_PLCA_NTF]		= ethnl_default_notify,
 	[ETHTOOL_MSG_MM_NTF]		= ethnl_default_notify,
+	[ETHTOOL_MSG_TS_NTF]		= ethnl_default_notify,
 };
 
 void ethtool_notify(struct net_device *dev, unsigned int cmd, const void *data)
@@ -1031,6 +1034,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,
+	},
 	{
 		.cmd	= ETHTOOL_MSG_CABLE_TEST_ACT,
 		.flags	= GENL_UNS_ADMIN_PERM,
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 49c700777a32..ba38aa0b5506 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -426,6 +426,7 @@ extern const struct nla_policy ethnl_eee_get_policy[ETHTOOL_A_EEE_HEADER + 1];
 extern const struct nla_policy ethnl_eee_set_policy[ETHTOOL_A_EEE_TX_LPI_TIMER + 1];
 extern const struct nla_policy ethnl_tsinfo_get_policy[ETHTOOL_A_TSINFO_HEADER + 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_LAYER + 1];
 extern const struct nla_policy ethnl_cable_test_act_policy[ETHTOOL_A_CABLE_TEST_HEADER + 1];
 extern const struct nla_policy ethnl_cable_test_tdr_act_policy[ETHTOOL_A_CABLE_TEST_TDR_CFG + 1];
 extern const struct nla_policy ethnl_tunnel_info_get_policy[ETHTOOL_A_TUNNEL_INFO_HEADER + 1];
diff --git a/net/ethtool/ts.c b/net/ethtool/ts.c
index a71c47ff0c6b..7a1e27add492 100644
--- a/net/ethtool/ts.c
+++ b/net/ethtool/ts.c
@@ -31,19 +31,13 @@ static int ts_prepare_data(const struct ethnl_req_info *req_base,
 {
 	struct ts_reply_data *data = TS_REPDATA(reply_base);
 	struct net_device *dev = reply_base->dev;
-	const struct ethtool_ops *ops = dev->ethtool_ops;
 	int ret;
 
 	ret = ethnl_ops_begin(dev);
 	if (ret < 0)
 		return ret;
 
-	if (phy_has_tsinfo(dev->phydev))
-		data->ts = SOF_PHY_TIMESTAMPING;
-	else if (ops->get_ts_info)
-		data->ts = SOF_MAC_TIMESTAMPING;
-	else
-		return -EOPNOTSUPP;
+	data->ts = dev->selected_timestamping_layer;
 
 	ethnl_ops_complete(dev);
 
@@ -61,9 +55,65 @@ static int ts_fill_reply(struct sk_buff *skb,
 			     const struct ethnl_reply_data *reply_base)
 {
 	struct ts_reply_data *data = TS_REPDATA(reply_base);
+
 	return nla_put_u32(skb, ETHTOOL_A_TS_LAYER, data->ts);
 }
 
+/* TS_SET */
+const struct nla_policy ethnl_ts_set_policy[] = {
+	[ETHTOOL_A_TS_HEADER]	= NLA_POLICY_NESTED(ethnl_header_policy),
+	[ETHTOOL_A_TS_LAYER]	= { .type = NLA_U32 },
+};
+
+static int ethnl_set_ts_validate(struct ethnl_req_info *req_info,
+				 struct genl_info *info)
+{
+	const struct ethtool_ops *ops = req_info->dev->ethtool_ops;
+	struct net_device *dev = req_info->dev;
+	struct nlattr **tb = info->attrs;
+
+	if (!tb[ETHTOOL_A_TS_LAYER])
+		return 0;
+
+	if (!phy_has_tsinfo(dev->phydev) && !ops->get_ts_info) {
+		NL_SET_ERR_MSG_ATTR(info->extack,
+				    tb[ETHTOOL_A_TS_LAYER],
+				    "Hardware time stamping is not supported by this device");
+		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;
+	struct nlattr **tb = info->attrs;
+	enum timestamping_layer flavor;
+	bool mod = false;
+
+	if (!dev->phydev) {
+		return -EOPNOTSUPP;
+	}
+
+	flavor = dev->selected_timestamping_layer;
+	ethnl_update_u32(&flavor, tb[ETHTOOL_A_TS_LAYER], &mod);
+
+	if (mod) {
+		const struct net_device_ops *ops = dev->netdev_ops;
+		struct ifreq ifr = {0};
+
+		/* Disable time stamping in the current layer. */
+		if (netif_device_present(dev) && ops->ndo_eth_ioctl)
+			ops->ndo_eth_ioctl(dev, &ifr, SIOCSHWTSTAMP);
+
+		dev->selected_timestamping_layer = flavor;
+		return 1;
+	};
+
+	return 0;
+}
+
 const struct ethnl_request_ops ethnl_ts_request_ops = {
 	.request_cmd		= ETHTOOL_MSG_TS_GET,
 	.reply_cmd		= ETHTOOL_MSG_TS_GET_REPLY,
@@ -74,6 +124,10 @@ 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,
+	.set_ntf_cmd		= ETHTOOL_MSG_TS_NTF,
 };
 
 /* TSLIST_GET */
-- 
2.25.1

Re: [PATCH net-next RFC v4 4/5] net: Let the active time stamping layer be selectable.

[Jakub Kicinski]

On Thu,  6 Apr 2023 19:33:07 +0200 Köry Maincent wrote:
> +/**
> + * A whitelist for PHYs selected as default timesetamping.
> + * Its use is to keep compatibility with old PTP API which is selecting
> + * these PHYs as default timestamping.
> + * The new API is selecting the MAC as default timestamping.
> + */
> +const char * const phy_timestamping_whitelist[] = {

whitelist -> allowlist or some such, inclusive naming

Re: [PATCH net-next RFC v4 4/5] net: Let the active time stamping layer be selectable.

[Vladimir Oltean]

On Thu, Apr 06, 2023 at 07:33:07PM +0200, Köry Maincent wrote:
> +static void of_set_timestamp(struct net_device *netdev, struct phy_device *phydev)
> +{
> +	struct device_node *node = phydev->mdio.dev.of_node;
> +	const struct ethtool_ops *ops = netdev->ethtool_ops;
> +	const char *s;
> +	enum timestamping_layer ts_layer = 0;

netdev likes variable declaration lines sorted longest to shortest

> +	int i;
> +
> +	/* Backward compatibility to old API */
> +	for (i = 0; phy_timestamping_whitelist[i] != NULL; i++)
> +	{

The kernel coding style likes the opening { on the same line as the for

> +		if (!strcmp(phy_timestamping_whitelist[i],
> +			    phydev->drv->name)) {
> +			if (phy_has_hwtstamp(phydev))
> +				ts_layer = SOF_PHY_TIMESTAMPING;
> +			else if (ops->get_ts_info)
> +				ts_layer = SOF_MAC_TIMESTAMPING;
> +			goto out;
> +		}
> +	}
> +
> +	if (ops->get_ts_info)
> +		ts_layer = SOF_MAC_TIMESTAMPING;
> +	else if (phy_has_hwtstamp(phydev))
> +		ts_layer = SOF_PHY_TIMESTAMPING;
> +
> +	if (of_property_read_string(node, "preferred-timestamp", &s))
> +		goto out;
> +
> +	if (!s)
> +		goto out;
> +
> +	if (phy_has_hwtstamp(phydev) && !strcmp(s, "phy"))
> +		ts_layer = SOF_PHY_TIMESTAMPING;
> +
> +	if (ops->get_ts_info && !strcmp(s, "mac"))
> +		ts_layer = SOF_MAC_TIMESTAMPING;
> +
> +out:
> +	netdev->selected_timestamping_layer = ts_layer;
> +}
> +
>  /**
>   * phy_attach_direct - attach a network device to a given PHY device pointer
>   * @dev: network device to attach
> @@ -1481,6 +1560,8 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  
>  	phydev->phy_link_change = phy_link_change;
>  	if (dev) {
> +		of_set_timestamp(dev, phydev);
> +
>  		phydev->attached_dev = dev;
>  		dev->phydev = phydev;
>  
> @@ -1811,6 +1892,10 @@ void phy_detach(struct phy_device *phydev)
>  
>  	phy_suspend(phydev);
>  	if (dev) {
> +		if (dev->ethtool_ops->get_ts_info)
> +			dev->selected_timestamping_layer = SOF_MAC_TIMESTAMPING;
> +		else
> +			dev->selected_timestamping_layer = 0;
>  		phydev->attached_dev->phydev = NULL;
>  		phydev->attached_dev = NULL;
>  	}
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index a740be3bb911..3dd6be012daf 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -48,6 +48,7 @@
>  #include <uapi/linux/if_bonding.h>
>  #include <uapi/linux/pkt_cls.h>
>  #include <uapi/linux/netdev.h>
> +#include <uapi/linux/net_tstamp.h>
>  #include <linux/hashtable.h>
>  #include <linux/rbtree.h>
>  #include <net/net_trackers.h>
> @@ -2019,6 +2020,9 @@ enum netdev_ml_priv_type {
>   *
>   *	@threaded:	napi threaded mode is enabled
>   *
> + *	@selected_timestamping_layer:	Tracks whether the MAC or the PHY
> + *					performs packet time stamping.
> + *
>   *	@net_notifier_list:	List of per-net netdev notifier block
>   *				that follow this device when it is moved
>   *				to another network namespace.
> @@ -2388,6 +2392,8 @@ struct net_device {
>  	unsigned		wol_enabled:1;
>  	unsigned		threaded:1;
>  
> +	enum timestamping_layer selected_timestamping_layer;
> +
>  	struct list_head	net_notifier_list;
>  
>  #if IS_ENABLED(CONFIG_MACSEC)
> @@ -2879,6 +2885,7 @@ enum netdev_cmd {
>  	NETDEV_OFFLOAD_XSTATS_REPORT_DELTA,
>  	NETDEV_XDP_FEAT_CHANGE,
>  	NETDEV_PRE_CHANGE_HWTSTAMP,
> +	NETDEV_CHANGE_HWTSTAMP,

Don't create new netdev notifiers with no users. Also, NETDEV_PRE_CHANGE_HWTSTAMP
has disappered.

>  };
>  const char *netdev_cmd_to_name(enum netdev_cmd cmd);
>  
> @@ -2934,6 +2941,11 @@ struct netdev_notifier_hwtstamp_info {
>  	struct kernel_hwtstamp_config *config;
>  };
>  
> +struct netdev_notifier_hwtstamp_layer {
> +	struct netdev_notifier_info info; /* must be first */
> +	enum timestamping_layer ts_layer;
> +};
> +
>  enum netdev_offload_xstats_type {
>  	NETDEV_OFFLOAD_XSTATS_TYPE_L3 = 1,
>  };
> diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
> index 447908393b91..4f03e7cde271 100644
> --- a/include/uapi/linux/ethtool_netlink.h
> +++ b/include/uapi/linux/ethtool_netlink.h
> @@ -41,6 +41,7 @@ enum {
>  	ETHTOOL_MSG_TSINFO_GET,
>  	ETHTOOL_MSG_TSLIST_GET,
>  	ETHTOOL_MSG_TS_GET,
> +	ETHTOOL_MSG_TS_SET,

The way in which the Linux kernel ensures a stable API towards user
space is that programs written against kernel headers from 5 years ago
still work with kernels from today. In your case, you would be breaking
that, because before this patch, ETHTOOL_MSG_CABLE_TEST_ACT was 26, and
after your patch it is 27. So old applications emitting a cable test
netlink message would be interpreted by new kernels as emitting a "set
timestamping layer" netlink message. Of course not only the cable test
is affected, every other netlink message until the end is now shifted by
1. This is why we put new enum values only at the very end, where it
actually says they should go:

	/* add new constants above here */

>  	ETHTOOL_MSG_CABLE_TEST_ACT,
>  	ETHTOOL_MSG_CABLE_TEST_TDR_ACT,
>  	ETHTOOL_MSG_TUNNEL_INFO_GET,
> @@ -96,6 +97,7 @@ enum {
>  	ETHTOOL_MSG_TSINFO_GET_REPLY,
>  	ETHTOOL_MSG_TSLIST_GET_REPLY,
>  	ETHTOOL_MSG_TS_GET_REPLY,
> +	ETHTOOL_MSG_TS_NTF,

Similar issue.

>  	ETHTOOL_MSG_CABLE_TEST_NTF,
>  	ETHTOOL_MSG_CABLE_TEST_TDR_NTF,
>  	ETHTOOL_MSG_TUNNEL_INFO_GET_REPLY,
> diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> index 695c7c4a816b..daea7221dd25 100644
> --- a/net/ethtool/common.c
> +++ b/net/ethtool/common.c
> @@ -61,9 +55,65 @@ static int ts_fill_reply(struct sk_buff *skb,
>  			     const struct ethnl_reply_data *reply_base)
>  {
>  	struct ts_reply_data *data = TS_REPDATA(reply_base);
> +

gratuitous change

>  	return nla_put_u32(skb, ETHTOOL_A_TS_LAYER, data->ts);
>  }
>  
> +/* TS_SET */
> +const struct nla_policy ethnl_ts_set_policy[] = {
> +	[ETHTOOL_A_TS_HEADER]	= NLA_POLICY_NESTED(ethnl_header_policy),
> +	[ETHTOOL_A_TS_LAYER]	= { .type = NLA_U32 },
> +};

I wanted to explore this topic myself, but I can't seem to find the
time, so here's something a bit hand-wavey.

We should make a distinction between what the kernel exposes as UAPI
(our future selves need to work with that in a backwards-compatible way)
and the internal, unstable kernel implementation.

It would be good if, instead of the ETHTOOL_A_TS_LAYER netlink
attribute, the kernel could expose a more generic ETHTOOL_A_TS_PHC, from
which the ethtool core could deduce if the PHC number belongs to the MAC
or to the PHY. We could still keep something like netdev->selected_timestamping_layer
in code private to the kernel, but from the UAPI perspective, I agree
with Andrew that we should design something that is cleanly extensible
to N PHCs, not just to a vague "layer".

Re: [PATCH net-next RFC v4 4/5] net: Let the active time stamping layer be selectable.

[Köry Maincent]

On Sat, 29 Apr 2023 20:58:07 +0300
Vladimir Oltean <vladimir.oltean@nxp.com> wrote:

Thanks for the review and sorry for the coding style issues, I forgot to run the
checkpatch script.

> >  
> >  #if IS_ENABLED(CONFIG_MACSEC)
> > @@ -2879,6 +2885,7 @@ enum netdev_cmd {
> >  	NETDEV_OFFLOAD_XSTATS_REPORT_DELTA,
> >  	NETDEV_XDP_FEAT_CHANGE,
> >  	NETDEV_PRE_CHANGE_HWTSTAMP,
> > +	NETDEV_CHANGE_HWTSTAMP,  
> 
> Don't create new netdev notifiers with no users. Also,
> NETDEV_PRE_CHANGE_HWTSTAMP has disappered.

Ok, right you move it on to dsa stub. What do you think of our case, should we
continue with netdev notifier? 

> > diff --git a/include/uapi/linux/ethtool_netlink.h
> > b/include/uapi/linux/ethtool_netlink.h index 447908393b91..4f03e7cde271
> > 100644 --- a/include/uapi/linux/ethtool_netlink.h
> > +++ b/include/uapi/linux/ethtool_netlink.h
> > @@ -41,6 +41,7 @@ enum {
> >  	ETHTOOL_MSG_TSINFO_GET,
> >  	ETHTOOL_MSG_TSLIST_GET,
> >  	ETHTOOL_MSG_TS_GET,
> > +	ETHTOOL_MSG_TS_SET,  
> 
> The way in which the Linux kernel ensures a stable API towards user
> space is that programs written against kernel headers from 5 years ago
> still work with kernels from today. In your case, you would be breaking
> that, because before this patch, ETHTOOL_MSG_CABLE_TEST_ACT was 26, and
> after your patch it is 27. So old applications emitting a cable test
> netlink message would be interpreted by new kernels as emitting a "set
> timestamping layer" netlink message. Of course not only the cable test
> is affected, every other netlink message until the end is now shifted by
> 1. This is why we put new enum values only at the very end, where it
> actually says they should go:

Sorry for that, this indeed didn't cross my head. I will fix it.

> >  
> > +/* TS_SET */
> > +const struct nla_policy ethnl_ts_set_policy[] = {
> > +	[ETHTOOL_A_TS_HEADER]	=
> > NLA_POLICY_NESTED(ethnl_header_policy),
> > +	[ETHTOOL_A_TS_LAYER]	= { .type = NLA_U32 },
> > +};  
> 
> I wanted to explore this topic myself, but I can't seem to find the
> time, so here's something a bit hand-wavey.
> 
> We should make a distinction between what the kernel exposes as UAPI
> (our future selves need to work with that in a backwards-compatible way)
> and the internal, unstable kernel implementation.
> 
> It would be good if, instead of the ETHTOOL_A_TS_LAYER netlink
> attribute, the kernel could expose a more generic ETHTOOL_A_TS_PHC, from
> which the ethtool core could deduce if the PHC number belongs to the MAC
> or to the PHY. We could still keep something like
> netdev->selected_timestamping_layer in code private to the kernel, but from
> the UAPI perspective, I agree with Andrew that we should design something
> that is cleanly extensible to N PHCs, not just to a vague "layer".

Just some thought:
Maybe we could create a PHC_ID 0xXY where X is the layer and Y the PHCs number
in this layer, but what will append if there is two MACs consecutively?
Also in case of several MACs or PHYs in serial
netdev->selected_timestamping_layer is inappropriate. 

Maybe using it like 0xABC where A is the MAC number, B the PHY number and C
the PHC number in the device.
For example if we select the second MAC and its third PHC, PHC_ID will be
0x203. Or if we select the second PHC of the PHY it will be 0x012.

Re: [PATCH net-next RFC v4 4/5] net: Let the active time stamping layer be selectable.

[Vladimir Oltean]

On Tue, May 02, 2023 at 01:05:25PM +0200, Köry Maincent wrote:
> On Sat, 29 Apr 2023 20:58:07 +0300
> Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> 
> Thanks for the review and sorry for the coding style issues, I forgot to run the
> checkpatch script.
> 
> > >  
> > >  #if IS_ENABLED(CONFIG_MACSEC)
> > > @@ -2879,6 +2885,7 @@ enum netdev_cmd {
> > >  	NETDEV_OFFLOAD_XSTATS_REPORT_DELTA,
> > >  	NETDEV_XDP_FEAT_CHANGE,
> > >  	NETDEV_PRE_CHANGE_HWTSTAMP,
> > > +	NETDEV_CHANGE_HWTSTAMP,  
> > 
> > Don't create new netdev notifiers with no users. Also,
> > NETDEV_PRE_CHANGE_HWTSTAMP has disappered.
> 
> Ok, right you move it on to dsa stub. What do you think of our case, should we
> continue with netdev notifier? 

I don't know.

AFAIU, the plan forward with this patch set is that, if the active
timestamping layer is the PHY, phy_mii_ioctl() gets called and the MAC
driver does not get notified in any way of that. That is an issue
because if it's a switch, it will want to trap PTP even if it doesn't
timestamp it, and with this proposal it doesn't get a chance to do that.

What is your need for this? Do you have this scenario? If not, just drop
this part from the patch.

Jakub, you said "nope" to netdev notifiers, what would you suggest here
instead? ndo_change_ptp_traps()?

> Just some thought:
> Maybe we could create a PHC_ID 0xXY where X is the layer and Y the PHCs number
> in this layer, but what will append if there is two MACs consecutively?
> Also in case of several MACs or PHYs in serial
> netdev->selected_timestamping_layer is inappropriate. 
> 
> Maybe using it like 0xABC where A is the MAC number, B the PHY number and C
> the PHC number in the device.
> For example if we select the second MAC and its third PHC, PHC_ID will be
> 0x203. Or if we select the second PHC of the PHY it will be 0x012.

I think the kernel has (or can have) enough information to deduce the
timestamping layer from just the PHC number, everything else is redundant
information (also, what's "MAC number"? what is "PHY number"? how can
user space correlate them to anything?).

If you want to design an UAPI where individual PHCs present in the
timestamping list of a netdev can individually have timestamping turned
on or off (for a future where a socket can communicate hardware
timestamps from multiple PHCs to user space in an identifiable way),
then that is fine by me and maybe even a good idea. Just be aware that
the current kernel will have to deny more than a single active
timestamping PHC for now, because of the lack of hwtstamp identification
in the SO_TIMESTAMPING API.

Re: [PATCH net-next RFC v4 4/5] net: Let the active time stamping layer be selectable.

[Jakub Kicinski]

On Thu, 11 May 2023 16:48:07 +0300 Vladimir Oltean wrote:
> > Ok, right you move it on to dsa stub. What do you think of our case, should we
> > continue with netdev notifier?   
> 
> I don't know.
> 
> AFAIU, the plan forward with this patch set is that, if the active
> timestamping layer is the PHY, phy_mii_ioctl() gets called and the MAC
> driver does not get notified in any way of that. That is an issue
> because if it's a switch, it will want to trap PTP even if it doesn't
> timestamp it, and with this proposal it doesn't get a chance to do that.
> 
> What is your need for this? Do you have this scenario? If not, just drop
> this part from the patch.
> 
> Jakub, you said "nope" to netdev notifiers, what would you suggest here
> instead? ndo_change_ptp_traps()?

More importantly "monolithic" drivers have DMA/MAC/PHY all under 
the NDO so assuming that SOF_PHY_TIMESTAMPING implies a phylib PHY
is not going to work.

We need a more complex calling convention for the NDO.

Re: [PATCH net-next RFC v4 4/5] net: Let the active time stamping layer be selectable.

[Vladimir Oltean]

On Thu, May 11, 2023 at 08:36:20AM -0700, Jakub Kicinski wrote:
> On Thu, 11 May 2023 16:48:07 +0300 Vladimir Oltean wrote:
> > > Ok, right you move it on to dsa stub. What do you think of our case, should we
> > > continue with netdev notifier?   
> > 
> > I don't know.
> > 
> > AFAIU, the plan forward with this patch set is that, if the active
> > timestamping layer is the PHY, phy_mii_ioctl() gets called and the MAC
> > driver does not get notified in any way of that. That is an issue
> > because if it's a switch, it will want to trap PTP even if it doesn't
> > timestamp it, and with this proposal it doesn't get a chance to do that.
> > 
> > What is your need for this? Do you have this scenario? If not, just drop
> > this part from the patch.
> > 
> > Jakub, you said "nope" to netdev notifiers, what would you suggest here
> > instead? ndo_change_ptp_traps()?
> 
> More importantly "monolithic" drivers have DMA/MAC/PHY all under 
> the NDO so assuming that SOF_PHY_TIMESTAMPING implies a phylib PHY
> is not going to work.
> 
> We need a more complex calling convention for the NDO.

It's the first time I become aware of the issue of PHY timestamping in
monolithic drivers that don't use phylib, and it's actually a very good
point. I guess that input gives a clearer set of constraints for Köry to
design an API where the selected timestamping layer is maybe passed to
ndo_hwtstamp_set() and MAC drivers are obliged to look at it.

OTOH, a complaint about the current ndo_eth_ioctl() -> phy_mii_ioctl()
code path was that phylib PHY drivers need to have the explicit blessing
from the MAC driver in order to enable timestamping. This patch set
attempts to circumvent that, and you're basically saying that it shouldn't.

Re: [PATCH net-next RFC v4 4/5] net: Let the active time stamping layer be selectable.

[Jakub Kicinski]

On Thu, 11 May 2023 18:56:40 +0300 Vladimir Oltean wrote:
> > More importantly "monolithic" drivers have DMA/MAC/PHY all under 
> > the NDO so assuming that SOF_PHY_TIMESTAMPING implies a phylib PHY
> > is not going to work.
> > 
> > We need a more complex calling convention for the NDO.  
> 
> It's the first time I become aware of the issue of PHY timestamping in
> monolithic drivers that don't use phylib, and it's actually a very good
> point. I guess that input gives a clearer set of constraints for Köry to
> design an API where the selected timestamping layer is maybe passed to
> ndo_hwtstamp_set() and MAC drivers are obliged to look at it.
> 
> OTOH, a complaint about the current ndo_eth_ioctl() -> phy_mii_ioctl()
> code path was that phylib PHY drivers need to have the explicit blessing
> from the MAC driver in order to enable timestamping. This patch set
> attempts to circumvent that, and you're basically saying that it shouldn't.

Yes, we don't want to lose the simplification benefit for the common
cases. I think we should make the "please call me for PHY requests"
an opt in.

Annoyingly the "please call me for PHY/all requests" needs to be
separate from "this MAC driver supports PHY timestamps". Because in
your example the switch driver may not in fact implement PHY stamping,
it just wants to know about the configuration.

So we need a bit somewhere (in ops? in some other struct? in output 
of get_ts?) to let the driver declare that it wants to see all TS
requests. (I've been using bits in ops, IDK if people find that
repulsive or neat :))

Then if bit is not set or NDO returns -EOPNOTSUPP for PHY requests we
still try to call the PHY in the core?

Separately the MAC driver needs to be able to report what stamping 
it supports (DMA, MAC, PHY, as mentioned in reply to patch 2).

Re: [PATCH net-next RFC v4 4/5] net: Let the active time stamping layer be selectable.

[Vladimir Oltean]

On Thu, May 11, 2023 at 09:25:39AM -0700, Jakub Kicinski wrote:
> On Thu, 11 May 2023 18:56:40 +0300 Vladimir Oltean wrote:
> > > More importantly "monolithic" drivers have DMA/MAC/PHY all under 
> > > the NDO so assuming that SOF_PHY_TIMESTAMPING implies a phylib PHY
> > > is not going to work.
> > > 
> > > We need a more complex calling convention for the NDO.  
> > 
> > It's the first time I become aware of the issue of PHY timestamping in
> > monolithic drivers that don't use phylib, and it's actually a very good
> > point. I guess that input gives a clearer set of constraints for Köry to
> > design an API where the selected timestamping layer is maybe passed to
> > ndo_hwtstamp_set() and MAC drivers are obliged to look at it.
> > 
> > OTOH, a complaint about the current ndo_eth_ioctl() -> phy_mii_ioctl()
> > code path was that phylib PHY drivers need to have the explicit blessing
> > from the MAC driver in order to enable timestamping. This patch set
> > attempts to circumvent that, and you're basically saying that it shouldn't.
> 
> Yes, we don't want to lose the simplification benefit for the common
> cases.

While I'm all for simplification in general, let's not take that for
granted and see what it implies, first.

If the new default behavior for the common case is going to be to bypass
the MAC's ndo_hwtstamp_set(), then MAC drivers which didn't explicitly
allow phylib PHY timestamping will now do.

Let's group them into:

(A) MAC drivers where that is perfectly fine

(B) MAC drivers with forwarding offload, which would forward/flood PTP
    frames carrying PHY timestamps

(C) "solipsistic" MAC drivers which assume that any skb with
    SKBTX_HW_TSTAMP is a skb for *me* to timestamp

Going for the simplification would mean making sure that cases (B)
and (C) are well handled, and have a reasonable chance of not getting
reintroduced in the future.

For case (B) it would mean patching all existing switch drivers which
don't allow PHY timestamping to still not allow PHY timestamping, and
fixing those switch drivers which allow PHY timestamping but don't set
up traps (probably those weren't tested in a bridging configuration).

For case (C) it would mean scanning all MAC drivers for bugs akin to the
one fixed in commit c26a2c2ddc01 ("gianfar: Fix TX timestamping with a
stacked DSA driver"). I did that once, but it was years ago and I can't
guarantee what is the current state or that I didn't miss anything.
For example, I missed the minor fact that igc would count skbs
timestamped by a non-igc entity in its TX path as 'tx_hwtstamp_skipped'
packets, visible in ethtool -S:
https://lore.kernel.org/intel-wired-lan/20230504235233.1850428-2-vinicius.gomes@intel.com/

It has to be said that nowadays, Documentation/networking/timestamping.rst
does warn about stacked PHCs, and those who read will find it. Also,
ptp4l nowadays warns if there are multiple TX timestamps received for
the same skb, and that's a major user of this functionality. So I don't
mean to point this case out as a form of discouragement, but it is going
to be a bit of a roll of dice.


The alternative (ditching the simplification) is that someone still
has to code up the glue logic from ndo_hwtstamp_set() -> phy_mii_ioctl(),
and that presumes some minimal level of testing, which we are now
"simplifying" away.

The counter-argument against ditching the simplification is that DSA
is also vulnerable to the bugs from case (C), but as opposed to PHY
timestamping where currently MACs need to voluntarily pass the
phy_mii_ioctl() call themselves, MACs don't get to choose if they want
to act as DSA masters or not. That gives some more confidence that bugs
in this area might not be so common, and leaves just (B) a concern.

To analyze how common is the common case is a simple matter of counting
how many drivers among those with SIOCSHWTSTAMP implementations also
have some form of forwarding offload, OR, as you point out, how many
don't use phylib.

> I think we should make the "please call me for PHY requests" an opt in.
> 
> Annoyingly the "please call me for PHY/all requests" needs to be
> separate from "this MAC driver supports PHY timestamps". Because in
> your example the switch driver may not in fact implement PHY stamping,
> it just wants to know about the configuration.
> 
> So we need a bit somewhere (in ops? in some other struct? in output 
> of get_ts?) to let the driver declare that it wants to see all TS
> requests. (I've been using bits in ops, IDK if people find that
> repulsive or neat :))

It's either repulsive or neat, depending on the context.

Last time you suggested something like this in an ops structure was IIRC
something like whether "MAC Merge is supported". My objection was that
DSA has a common set of ops structures (dsa_slave_ethtool_ops,
dsa_slave_netdev_ops) behind which lie different switch families from
at least 13 vendors. A shared const ops structure is not an appropriate
means to communicate whether 13 switch vendors support a TSN MAC Merge
layer or not.

With declaring interest in all hardware timestamping requests in the
data path of a MAC, be they MAC-level requests or otherwise, it's a bit
different, because all DSA switches have one thing in common, which is
that they're switches, and that is relevant here. So I'm not opposed to
setting a bit in the ethtool ops structure, at least for DSA that could
work just fine.

> Then if bit is not set or NDO returns -EOPNOTSUPP for PHY requests we
> still try to call the PHY in the core?

Well, if there is no interest for special behavior from the MAC driver,
then I guess the memo is "yolo"...

But OTOH, if a macro-driver like DSA declares its interest in receiving
all timestamping requests, but then that particular DSA switch returns
-EOPNOTSUPP in the ndo_hwtstamp_set() handler, it would be silly for the
core to still "force the entry" and call phy_mii_ioctl() anyway - because
we know that's going to be broken.

So with "NDO returns -EOPNOTSUPP", I hope you don't mean *that* NDO
(ndo_hwtstamp_set()) but a previously unnamed one that serves the same
purpose as the capability bit - ndo_hey_are_you_interested_in_this_hwtstamp_request().
In that case, yes - with -EOPNOTSUPP we're back to "yolo".

> Separately the MAC driver needs to be able to report what stamping 
> it supports (DMA, MAC, PHY, as mentioned in reply to patch 2).

I'm a bit unclear on that - responded there.

Re: [PATCH net-next RFC v4 4/5] net: Let the active time stamping layer be selectable.

[Jakub Kicinski]

On Thu, 11 May 2023 23:54:35 +0300 Vladimir Oltean wrote:
> On Thu, May 11, 2023 at 09:25:39AM -0700, Jakub Kicinski wrote:
> > > It's the first time I become aware of the issue of PHY timestamping in
> > > monolithic drivers that don't use phylib, and it's actually a very good
> > > point. I guess that input gives a clearer set of constraints for Köry to
> > > design an API where the selected timestamping layer is maybe passed to
> > > ndo_hwtstamp_set() and MAC drivers are obliged to look at it.
> > > 
> > > OTOH, a complaint about the current ndo_eth_ioctl() -> phy_mii_ioctl()
> > > code path was that phylib PHY drivers need to have the explicit blessing
> > > from the MAC driver in order to enable timestamping. This patch set
> > > attempts to circumvent that, and you're basically saying that it shouldn't.  
> > 
> > Yes, we don't want to lose the simplification benefit for the common
> > cases.  
> 
> While I'm all for simplification in general, let's not take that for
> granted and see what it implies, first.
> 
> If the new default behavior for the common case is going to be to bypass
> the MAC's ndo_hwtstamp_set(), then MAC drivers which didn't explicitly
> allow phylib PHY timestamping will now do.
> 
> Let's group them into:
> 
> (A) MAC drivers where that is perfectly fine
> 
> (B) MAC drivers with forwarding offload, which would forward/flood PTP
>     frames carrying PHY timestamps
> 
> (C) "solipsistic" MAC drivers which assume that any skb with
>     SKBTX_HW_TSTAMP is a skb for *me* to timestamp
> 
> Going for the simplification would mean making sure that cases (B)
> and (C) are well handled, and have a reasonable chance of not getting
> reintroduced in the future.
> 
> For case (B) it would mean patching all existing switch drivers which
> don't allow PHY timestamping to still not allow PHY timestamping, and
> fixing those switch drivers which allow PHY timestamping but don't set
> up traps (probably those weren't tested in a bridging configuration).
> 
> For case (C) it would mean scanning all MAC drivers for bugs akin to the
> one fixed in commit c26a2c2ddc01 ("gianfar: Fix TX timestamping with a
> stacked DSA driver"). I did that once, but it was years ago and I can't
> guarantee what is the current state or that I didn't miss anything.
> For example, I missed the minor fact that igc would count skbs
> timestamped by a non-igc entity in its TX path as 'tx_hwtstamp_skipped'
> packets, visible in ethtool -S:
> https://lore.kernel.org/intel-wired-lan/20230504235233.1850428-2-vinicius.gomes@intel.com/
> 
> It has to be said that nowadays, Documentation/networking/timestamping.rst
> does warn about stacked PHCs, and those who read will find it. Also,
> ptp4l nowadays warns if there are multiple TX timestamps received for
> the same skb, and that's a major user of this functionality. So I don't
> mean to point this case out as a form of discouragement, but it is going
> to be a bit of a roll of dice.

I think it's worth calling out that we may be touching most / all
drivers anyway to transition them from the IOCTL NDO to a normal
timestamp NDO.

> The alternative (ditching the simplification) is that someone still
> has to code up the glue logic from ndo_hwtstamp_set() -> phy_mii_ioctl(),
> and that presumes some minimal level of testing, which we are now
> "simplifying" away.
> 
> The counter-argument against ditching the simplification is that DSA
> is also vulnerable to the bugs from case (C), but as opposed to PHY
> timestamping where currently MACs need to voluntarily pass the
> phy_mii_ioctl() call themselves, MACs don't get to choose if they want
> to act as DSA masters or not. That gives some more confidence that bugs
> in this area might not be so common, and leaves just (B) a concern.
> 
> To analyze how common is the common case is a simple matter of counting
> how many drivers among those with SIOCSHWTSTAMP implementations also
> have some form of forwarding offload, OR, as you point out, how many
> don't use phylib.

"Common" is one way of looking at it, another is trying to get the
default ("I didn't know I have to set extra flags") to do the right
thing or fail.

> > I think we should make the "please call me for PHY requests" an opt in.
> > 
> > Annoyingly the "please call me for PHY/all requests" needs to be
> > separate from "this MAC driver supports PHY timestamps". Because in
> > your example the switch driver may not in fact implement PHY stamping,
> > it just wants to know about the configuration.
> > 
> > So we need a bit somewhere (in ops? in some other struct? in output 
> > of get_ts?) to let the driver declare that it wants to see all TS
> > requests. (I've been using bits in ops, IDK if people find that
> > repulsive or neat :))  
> 
> It's either repulsive or neat, depending on the context.
> 
> Last time you suggested something like this in an ops structure was IIRC
> something like whether "MAC Merge is supported". My objection was that
> DSA has a common set of ops structures (dsa_slave_ethtool_ops,
> dsa_slave_netdev_ops) behind which lie different switch families from
> at least 13 vendors. A shared const ops structure is not an appropriate
> means to communicate whether 13 switch vendors support a TSN MAC Merge
> layer or not.
> 
> With declaring interest in all hardware timestamping requests in the
> data path of a MAC, be they MAC-level requests or otherwise, it's a bit
> different, because all DSA switches have one thing in common, which is
> that they're switches, and that is relevant here. So I'm not opposed to
> setting a bit in the ethtool ops structure, at least for DSA that could
> work just fine.
> 
> > Then if bit is not set or NDO returns -EOPNOTSUPP for PHY requests we
> > still try to call the PHY in the core?  
> 
> Well, if there is no interest for special behavior from the MAC driver,
> then I guess the memo is "yolo"...
> 
> But OTOH, if a macro-driver like DSA declares its interest in receiving
> all timestamping requests, but then that particular DSA switch returns
> -EOPNOTSUPP in the ndo_hwtstamp_set() handler, it would be silly for the
> core to still "force the entry" and call phy_mii_ioctl() anyway - because
> we know that's going to be broken.
> 
> So with "NDO returns -EOPNOTSUPP", I hope you don't mean *that* NDO
> (ndo_hwtstamp_set()) but a previously unnamed one that serves the same
> purpose as the capability bit - ndo_hey_are_you_interested_in_this_hwtstamp_request().
> In that case, yes - with -EOPNOTSUPP we're back to "yolo".

Why can't we treat ndo_hwtstamp_set() == -EOPNOTSUPP as a signal 
to call the PHY? ndo_hwtstamp_set() does not exist, we can give
it whatever semantics we want.

> > Separately the MAC driver needs to be able to report what stamping 
> > it supports (DMA, MAC, PHY, as mentioned in reply to patch 2).  
> 
> I'm a bit unclear on that - responded there.

Re: [PATCH net-next RFC v4 4/5] net: Let the active time stamping layer be selectable.

[Vladimir Oltean]

On Thu, May 11, 2023 at 04:08:45PM -0700, Jakub Kicinski wrote:
> I think it's worth calling out that we may be touching most / all
> drivers anyway to transition them from the IOCTL NDO to a normal
> timestamp NDO.

Right, and figuring out how to deal with PHY timestamping in the new API
is "step 1.5" between creating that new API (Maxim's patch set) and
making the timestamping layer selectable (Köry's patch set).

> > But OTOH, if a macro-driver like DSA declares its interest in receiving
> > all timestamping requests, but then that particular DSA switch returns
> > -EOPNOTSUPP in the ndo_hwtstamp_set() handler, it would be silly for the
> > core to still "force the entry" and call phy_mii_ioctl() anyway - because
> > we know that's going to be broken.
> > 
> > So with "NDO returns -EOPNOTSUPP", I hope you don't mean *that* NDO
> > (ndo_hwtstamp_set()) but a previously unnamed one that serves the same
> > purpose as the capability bit - ndo_hey_are_you_interested_in_this_hwtstamp_request().
> > In that case, yes - with -EOPNOTSUPP we're back to "yolo".
> 
> Why can't we treat ndo_hwtstamp_set() == -EOPNOTSUPP as a signal 
> to call the PHY? ndo_hwtstamp_set() does not exist, we can give
> it whatever semantics we want.

Hmm, because if we do that, bridged DSA switch ports without hardware
timestamping support and without logic to trap PTP to the CPU will just
spew those PTP frames with PHY hardware timestamps everywhere, instead
of just telling the user hey, the configuration isn't supported?

Re: [PATCH net-next RFC v4 4/5] net: Let the active time stamping layer be selectable.

[Jakub Kicinski]

On Fri, 12 May 2023 02:18:03 +0300 Vladimir Oltean wrote:
> > Why can't we treat ndo_hwtstamp_set() == -EOPNOTSUPP as a signal 
> > to call the PHY? ndo_hwtstamp_set() does not exist, we can give
> > it whatever semantics we want.  
> 
> Hmm, because if we do that, bridged DSA switch ports without hardware
> timestamping support and without logic to trap PTP to the CPU will just
> spew those PTP frames with PHY hardware timestamps everywhere, instead
> of just telling the user hey, the configuration isn't supported?

I see, so there is a legit reason to abort. 

We could use one of the high error codes, then, to signal 
the "I didn't care, please carry on to the PHY" condition?
-ENOTSUPP?

I guess we can add a separate "please configure traps for PTP/NTP" 
NDO, if you prefer. Mostly an implementation detail.

Re: [PATCH net-next RFC v4 4/5] net: Let the active time stamping layer be selectable.

[Köry Maincent]

On Thu, 11 May 2023 16:35:47 -0700
Jakub Kicinski <kuba@kernel.org> wrote:

> On Fri, 12 May 2023 02:18:03 +0300 Vladimir Oltean wrote:
> > > Why can't we treat ndo_hwtstamp_set() == -EOPNOTSUPP as a signal 
> > > to call the PHY? ndo_hwtstamp_set() does not exist, we can give
> > > it whatever semantics we want.    
> > 
> > Hmm, because if we do that, bridged DSA switch ports without hardware
> > timestamping support and without logic to trap PTP to the CPU will just
> > spew those PTP frames with PHY hardware timestamps everywhere, instead
> > of just telling the user hey, the configuration isn't supported?  
> 
> I see, so there is a legit reason to abort. 
> 
> We could use one of the high error codes, then, to signal 
> the "I didn't care, please carry on to the PHY" condition?
> -ENOTSUPP?
> 
> I guess we can add a separate "please configure traps for PTP/NTP" 
> NDO, if you prefer. Mostly an implementation detail.

I am not as expert as you on the network stack therefore I am trying to follow
and understand all the remarks. Please correct me if I say something wrong. It
is interesting to understand all the complications that these changes bring.

To summary, what do you think is preferable for this patch series?
- New ops for TS as suggested by Russell.

- Continue on this implementation and check that Vladimir A,B and C cases are
  handled. Which imply, if I understand well, find a good way to deal with PTP
  change trap (bit or new ndo ops), convert most drivers from IOCTL to NDO
  beforehand. 

- Add MAC-DMA TS? It think it is needed as MAC-DMA TS seems already used and
  different from simple MAC TS in term of quality, as described by Jakub.

Re: [PATCH net-next RFC v4 4/5] net: Let the active time stamping layer be selectable.

[Jakub Kicinski]

On Mon, 15 May 2023 11:04:32 +0200 Köry Maincent wrote:
> > I see, so there is a legit reason to abort. 
> > 
> > We could use one of the high error codes, then, to signal 
> > the "I didn't care, please carry on to the PHY" condition?
> > -ENOTSUPP?
> > 
> > I guess we can add a separate "please configure traps for PTP/NTP" 
> > NDO, if you prefer. Mostly an implementation detail.  
> 
> I am not as expert as you on the network stack therefore I am trying to follow
> and understand all the remarks. Please correct me if I say something wrong. It
> is interesting to understand all the complications that these changes bring.
> 
> To summary, what do you think is preferable for this patch series?
> - New ops for TS as suggested by Russell.
> 
> - Continue on this implementation and check that Vladimir A,B and C cases are
>   handled. Which imply, if I understand well, find a good way to deal with PTP
>   change trap (bit or new ndo ops), convert most drivers from IOCTL to NDO
>   beforehand. 
> 
> - Add MAC-DMA TS? It think it is needed as MAC-DMA TS seems already used and
>   different from simple MAC TS in term of quality, as described by Jakub.

These aren't really disjoint alternatives, we need MAC-DMA TS and we
need a way to direct all TS requests to the MAC driver. Whether we do
it via an NDO, flags or new ops is kind of up to you. Question of
aesthetics.

Perhaps to move forward it'd be good to rev the patches, and address
the feedback which is clear?

Re: [PATCH net-next RFC v4 4/5] net: Let the active time stamping layer be selectable.

[Russell King (Oracle)]

On Thu, May 11, 2023 at 09:25:39AM -0700, Jakub Kicinski wrote:
> Yes, we don't want to lose the simplification benefit for the common
> cases. I think we should make the "please call me for PHY requests"
> an opt in.
> 
> Annoyingly the "please call me for PHY/all requests" needs to be
> separate from "this MAC driver supports PHY timestamps". Because in
> your example the switch driver may not in fact implement PHY stamping,
> it just wants to know about the configuration.
> 
> So we need a bit somewhere (in ops? in some other struct? in output 
> of get_ts?) to let the driver declare that it wants to see all TS
> requests. (I've been using bits in ops, IDK if people find that
> repulsive or neat :))

I haven't thought this through fully, but just putting this out there
as a potential suggestion...

Would it help at all if we distilled the entire timestamping interface
into a separate set of ops which are registered independently from the
NDO, and NDO has a call to get the ops for the layer being configured?

That would allow a netdev driver to return the ops appropriate for the
MAC layer or its own PHY layer, or maybe phylib.

In the case of phylib, as there is a raft of drivers that only bind to
their phylib PHY in the NDO open method, we'd need to figure out how
to get the ops for the current mode at that time.

There's probably lots I've missed though...

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next RFC v4 4/5] net: Let the active time stamping layer be selectable.

[Jakub Kicinski]

On Thu, 11 May 2023 22:06:36 +0100 Russell King (Oracle) wrote:
> I haven't thought this through fully, but just putting this out there
> as a potential suggestion...
> 
> Would it help at all if we distilled the entire timestamping interface
> into a separate set of ops which are registered independently from the
> NDO, and NDO has a call to get the ops for the layer being configured?
> 
> That would allow a netdev driver to return the ops appropriate for the
> MAC layer or its own PHY layer, or maybe phylib.
> 
> In the case of phylib, as there is a raft of drivers that only bind to
> their phylib PHY in the NDO open method, we'd need to figure out how
> to get the ops for the current mode at that time.
> 
> There's probably lots I've missed though...

Sounds reasonable, only question is what to pass as the object (first
argument) of these ops. Some new struct?

Re: [PATCH net-next RFC v4 4/5] net: Let the active time stamping layer be selectable.

[Jacob Keller]

On 5/11/2023 9:25 AM, Jakub Kicinski wrote:
> So we need a bit somewhere (in ops? in some other struct? in output 
> of get_ts?) to let the driver declare that it wants to see all TS
> requests. (I've been using bits in ops, IDK if people find that
> repulsive or neat :))

+1 to using bits in ops for opt-in functionality. I like it.

[PATCH net-next RFC v4 5/5] net: fix up drivers WRT phy time stamping

[Köry Maincent]

From: Richard Cochran <richardcochran@gmail.com>

For "git bisect" correctness, this patch should be squashed in to the
previous one, but it is broken out here for the purpose of review.

I will also add the fix up of lan966x driver to validate the netdev
 notifier when the hwtstamp action will be converted to NDO.

Signed-off-by: Richard Cochran <richardcochran@gmail.com>
Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---

Notes:
    I will add the lan966x driver update to verify the netdev notifer once
    the replacement to NDO by Max will be done. Also I don't have this hardware
    to test it, it will be useful if someone will be willing to test for me.

 drivers/net/ethernet/freescale/fec_main.c | 23 +++++++++-----------
 drivers/net/ethernet/mscc/ocelot_net.c    | 21 +++++++++---------
 drivers/net/ethernet/ti/cpsw_priv.c       | 12 +++++------
 drivers/net/ethernet/ti/netcp_ethss.c     | 26 +++++------------------
 4 files changed, 31 insertions(+), 51 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index f3b16a6673e2..bc4bfdad83ca 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -3213,22 +3213,19 @@ static int fec_enet_ioctl(struct net_device *ndev, struct ifreq *rq, int cmd)
 	if (!netif_running(ndev))
 		return -EINVAL;
 
-	if (!phydev)
-		return -ENODEV;
+	switch (cmd) {
+	case SIOCSHWTSTAMP:
+		return fep->bufdesc_ex ? fec_ptp_set(ndev, rq) :
+		-EOPNOTSUPP;
 
-	if (fep->bufdesc_ex) {
-		bool use_fec_hwts = !phy_has_hwtstamp(phydev);
-
-		if (cmd == SIOCSHWTSTAMP) {
-			if (use_fec_hwts)
-				return fec_ptp_set(ndev, rq);
-			fec_ptp_disable_hwts(ndev);
-		} else if (cmd == SIOCGHWTSTAMP) {
-			if (use_fec_hwts)
-				return fec_ptp_get(ndev, rq);
-		}
+	case SIOCGHWTSTAMP:
+		return fep->bufdesc_ex ? fec_ptp_get(ndev, rq) :
+		-EOPNOTSUPP;
 	}
 
+	if (!phydev)
+		return -ENODEV;
+
 	return phy_mii_ioctl(phydev, rq, cmd);
 }
 
diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
index 21a87a3fc556..29f7a8a3a4d9 100644
--- a/drivers/net/ethernet/mscc/ocelot_net.c
+++ b/drivers/net/ethernet/mscc/ocelot_net.c
@@ -873,18 +873,19 @@ static int ocelot_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	struct ocelot *ocelot = priv->port.ocelot;
 	int port = priv->port.index;
 
-	/* If the attached PHY device isn't capable of timestamping operations,
-	 * use our own (when possible).
-	 */
-	if (!phy_has_hwtstamp(dev->phydev) && ocelot->ptp) {
-		switch (cmd) {
-		case SIOCSHWTSTAMP:
-			return ocelot_hwstamp_set(ocelot, port, ifr);
-		case SIOCGHWTSTAMP:
-			return ocelot_hwstamp_get(ocelot, port, ifr);
-		}
+	switch (cmd) {
+	case SIOCSHWTSTAMP:
+		return ocelot->ptp ? ocelot_hwstamp_set(ocelot, port, ifr) :
+		-EOPNOTSUPP;
+
+	case SIOCGHWTSTAMP:
+		return ocelot->ptp ? ocelot_hwstamp_get(ocelot, port, ifr) :
+		-EOPNOTSUPP;
 	}
 
+	if (!dev->phydev)
+		return -ENODEV;
+
 	return phy_mii_ioctl(dev->phydev, ifr, cmd);
 }
 
diff --git a/drivers/net/ethernet/ti/cpsw_priv.c b/drivers/net/ethernet/ti/cpsw_priv.c
index e966dd47e2db..cec4c65532c4 100644
--- a/drivers/net/ethernet/ti/cpsw_priv.c
+++ b/drivers/net/ethernet/ti/cpsw_priv.c
@@ -715,13 +715,11 @@ int cpsw_ndo_ioctl(struct net_device *dev, struct ifreq *req, int cmd)
 
 	phy = cpsw->slaves[slave_no].phy;
 
-	if (!phy_has_hwtstamp(phy)) {
-		switch (cmd) {
-		case SIOCSHWTSTAMP:
-			return cpsw_hwtstamp_set(dev, req);
-		case SIOCGHWTSTAMP:
-			return cpsw_hwtstamp_get(dev, req);
-		}
+	switch (cmd) {
+	case SIOCSHWTSTAMP:
+		return cpsw_hwtstamp_set(dev, req);
+	case SIOCGHWTSTAMP:
+		return cpsw_hwtstamp_get(dev, req);
 	}
 
 	if (phy)
diff --git a/drivers/net/ethernet/ti/netcp_ethss.c b/drivers/net/ethernet/ti/netcp_ethss.c
index 2adf82a32bf6..6074ce13e130 100644
--- a/drivers/net/ethernet/ti/netcp_ethss.c
+++ b/drivers/net/ethernet/ti/netcp_ethss.c
@@ -2557,15 +2557,6 @@ static int gbe_txtstamp_mark_pkt(struct gbe_intf *gbe_intf,
 	    !gbe_dev->tx_ts_enabled)
 		return 0;
 
-	/* If phy has the txtstamp api, assume it will do it.
-	 * We mark it here because skb_tx_timestamp() is called
-	 * after all the txhooks are called.
-	 */
-	if (phy_has_txtstamp(phydev)) {
-		skb_shinfo(p_info->skb)->tx_flags |= SKBTX_IN_PROGRESS;
-		return 0;
-	}
-
 	if (gbe_need_txtstamp(gbe_intf, p_info)) {
 		p_info->txtstamp = gbe_txtstamp;
 		p_info->ts_context = (void *)gbe_intf;
@@ -2583,11 +2574,6 @@ static int gbe_rxtstamp(struct gbe_intf *gbe_intf, struct netcp_packet *p_info)
 	if (p_info->rxtstamp_complete)
 		return 0;
 
-	if (phy_has_rxtstamp(phydev)) {
-		p_info->rxtstamp_complete = true;
-		return 0;
-	}
-
 	if (gbe_dev->rx_ts_enabled)
 		cpts_rx_timestamp(gbe_dev->cpts, p_info->skb);
 
@@ -2821,13 +2807,11 @@ static int gbe_ioctl(void *intf_priv, struct ifreq *req, int cmd)
 	struct gbe_intf *gbe_intf = intf_priv;
 	struct phy_device *phy = gbe_intf->slave->phy;
 
-	if (!phy_has_hwtstamp(phy)) {
-		switch (cmd) {
-		case SIOCGHWTSTAMP:
-			return gbe_hwtstamp_get(gbe_intf, req);
-		case SIOCSHWTSTAMP:
-			return gbe_hwtstamp_set(gbe_intf, req);
-		}
+	switch (cmd) {
+	case SIOCGHWTSTAMP:
+		return gbe_hwtstamp_get(gbe_intf, req);
+	case SIOCSHWTSTAMP:
+		return gbe_hwtstamp_set(gbe_intf, req);
 	}
 
 	if (phy)
-- 
2.25.1

Re: [PATCH net-next RFC v4 0/5] net: Make MAC/PHY time stamping selectable

[Köry Maincent]

On Thu,  6 Apr 2023 19:33:03 +0200
Köry Maincent <kory.maincent@bootlin.com> wrote:

> From: Kory Maincent <kory.maincent@bootlin.com>
> 
> Up until now, there was no way to let the user select the layer at
> which time stamping occurs.  The stack assumed that PHY time stamping
> is always preferred, but some MAC/PHY combinations were buggy.
> 
> This series aims to allow the user to select the desired layer
> administratively.

Forgot to run the checkpatch script, sorry for that. There is few coding
style issues.

Köry