[PATCH v2 0/4] 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.

[PATCH v2 0/4] 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.

[Köry Maincent]

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

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 makes the layer selectable at run time.

- Patch 4 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 3 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.

Richard Cochran (4):
  net: ethtool: Refactor identical get_ts_info implementations.
  net: Expose available time stamping layers to user space.
  net: Let the active time stamping layer be selectable.
  net: fix up drivers WRT phy time stamping

 .../ABI/testing/sysfs-class-net-timestamping  |  20 ++++
 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                  |   6 ++
 include/linux/ethtool.h                       |   8 ++
 include/linux/netdevice.h                     |  10 ++
 net/8021q/vlan_dev.c                          |  15 +--
 net/core/dev_ioctl.c                          |  44 +++++++-
 net/core/net-sysfs.c                          | 102 ++++++++++++++++++
 net/core/timestamping.c                       |   6 ++
 net/ethtool/common.c                          |  24 ++++-
 15 files changed, 248 insertions(+), 97 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-net-timestamping

-- 
2.25.1

[PATCH v2 1/4] 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>
---

Notes:
    Changes in V2:
    - Refactor also macvlan driver

 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 fce9301c8ebb..11e025074594 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -5683,9 +5683,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();
@@ -5694,16 +5692,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 b8cc55b2d721..7e923d27196f 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -1059,20 +1059,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 99dc7bfbcd3c..fa20abec4b93 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -834,6 +834,14 @@ ethtool_params_from_link_mode(struct ethtool_link_ksettings *link_ksettings,
  */
 int ethtool_get_phc_vclocks(struct net_device *dev, int **vclock_index);
 
+/**
+ * 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 e1bb41a443c4..3e475feae543 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 566adf85e658..64a7e05cf2c2 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -572,6 +572,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

[PATCH v2 2/4] net: Expose available time stamping layers to user space.

[Köry Maincent]

From: Richard Cochran <richardcochran@gmail.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 sysfs.

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.

 .../ABI/testing/sysfs-class-net-timestamping  | 17 ++++++
 net/core/net-sysfs.c                          | 60 +++++++++++++++++++
 2 files changed, 77 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-net-timestamping

diff --git a/Documentation/ABI/testing/sysfs-class-net-timestamping b/Documentation/ABI/testing/sysfs-class-net-timestamping
new file mode 100644
index 000000000000..529c3a6eb607
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-net-timestamping
@@ -0,0 +1,17 @@
+What:		/sys/class/net/<iface>/available_timestamping_providers
+Date:		January 2022
+Contact:	Richard Cochran <richardcochran@gmail.com>
+Description:
+		Enumerates the available providers for SO_TIMESTAMPING.
+		The possible values are:
+		- "mac"  The MAC provides time stamping.
+		- "phy"  The PHY or MII device provides time stamping.
+
+What:		/sys/class/net/<iface>/current_timestamping_provider
+Date:		January 2022
+Contact:	Richard Cochran <richardcochran@gmail.com>
+Description:
+		Show the current SO_TIMESTAMPING provider.
+		The possible values are:
+		- "mac"  The MAC provides time stamping.
+		- "phy"  The PHY or MII device provides time stamping.
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 8409d41405df..26095634fb31 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -620,6 +620,64 @@ static ssize_t threaded_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(threaded);
 
+static ssize_t available_timestamping_providers_show(struct device *dev,
+						     struct device_attribute *attr,
+						     char *buf)
+{
+	const struct ethtool_ops *ops;
+	struct net_device *netdev;
+	struct phy_device *phydev;
+	int ret = 0;
+
+	netdev = to_net_dev(dev);
+	phydev = netdev->phydev;
+	ops = netdev->ethtool_ops;
+
+	if (!rtnl_trylock())
+		return restart_syscall();
+
+	ret += sprintf(buf, "%s\n", "mac");
+	buf += 4;
+
+	if (phy_has_tsinfo(phydev)) {
+		ret += sprintf(buf, "%s\n", "phy");
+		buf += 4;
+	}
+
+	rtnl_unlock();
+
+	return ret;
+}
+static DEVICE_ATTR_RO(available_timestamping_providers);
+
+static ssize_t current_timestamping_provider_show(struct device *dev,
+						  struct device_attribute *attr,
+						  char *buf)
+{
+	const struct ethtool_ops *ops;
+	struct net_device *netdev;
+	struct phy_device *phydev;
+	int ret;
+
+	netdev = to_net_dev(dev);
+	phydev = netdev->phydev;
+	ops = netdev->ethtool_ops;
+
+	if (!rtnl_trylock())
+		return restart_syscall();
+
+	if (phy_has_tsinfo(phydev)) {
+		ret = sprintf(buf, "%s\n", "phy");
+	} else {
+		ret = sprintf(buf, "%s\n", "mac");
+	}
+
+	rtnl_unlock();
+
+	return ret;
+}
+static DEVICE_ATTR_RO(current_timestamping_provider);
+
 static struct attribute *net_class_attrs[] __ro_after_init = {
 	&dev_attr_netdev_group.attr,
 	&dev_attr_type.attr,
@@ -653,6 +711,8 @@ static struct attribute *net_class_attrs[] __ro_after_init = {
 	&dev_attr_carrier_up_count.attr,
 	&dev_attr_carrier_down_count.attr,
 	&dev_attr_threaded.attr,
+	&dev_attr_available_timestamping_providers.attr,
+	&dev_attr_current_timestamping_provider.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(net_class);
-- 
2.25.1

Re: [PATCH v2 2/4] net: Expose available time stamping layers to user space.

[kernel test robot]

Hi Köry,

I love your patch! Perhaps something to improve:

[auto build test WARNING on v6.2]
[also build test WARNING on next-20230303]
[cannot apply to net/master net-next/master horms-ipvs/master linus/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/K-ry-Maincent/net-ethtool-Refactor-identical-get_ts_info-implementations/20230304-004527
patch link:    https://lore.kernel.org/r/20230303164248.499286-3-kory.maincent%40bootlin.com
patch subject: [PATCH v2 2/4] net: Expose available time stamping layers to user space.
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230304/202303040133.slT4slaW-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/90d54e1c6ed12a0b55c868e7808d93f61dad3534
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review K-ry-Maincent/net-ethtool-Refactor-identical-get_ts_info-implementations/20230304-004527
        git checkout 90d54e1c6ed12a0b55c868e7808d93f61dad3534
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash net/core/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303040133.slT4slaW-lkp@intel.com/

All warnings (new ones prefixed by >>):

   net/core/net-sysfs.c: In function 'available_timestamping_providers_show':
>> net/core/net-sysfs.c:627:35: warning: variable 'ops' set but not used [-Wunused-but-set-variable]
     627 |         const struct ethtool_ops *ops;
         |                                   ^~~
   net/core/net-sysfs.c: In function 'current_timestamping_provider_show':
   net/core/net-sysfs.c:657:35: warning: variable 'ops' set but not used [-Wunused-but-set-variable]
     657 |         const struct ethtool_ops *ops;
         |                                   ^~~


vim +/ops +627 net/core/net-sysfs.c

   622	
   623	static ssize_t available_timestamping_providers_show(struct device *dev,
   624							     struct device_attribute *attr,
   625							     char *buf)
   626	{
 > 627		const struct ethtool_ops *ops;
   628		struct net_device *netdev;
   629		struct phy_device *phydev;
   630		int ret = 0;
   631	
   632		netdev = to_net_dev(dev);
   633		phydev = netdev->phydev;
   634		ops = netdev->ethtool_ops;
   635	
   636		if (!rtnl_trylock())
   637			return restart_syscall();
   638	
   639		ret += sprintf(buf, "%s\n", "mac");
   640		buf += 4;
   641	
   642		if (phy_has_tsinfo(phydev)) {
   643			ret += sprintf(buf, "%s\n", "phy");
   644			buf += 4;
   645		}
   646	
   647		rtnl_unlock();
   648	
   649		return ret;
   650	}
   651	static DEVICE_ATTR_RO(available_timestamping_providers);
   652	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH v2 2/4] net: Expose available time stamping layers to user space.

[Jakub Kicinski]

On Fri,  3 Mar 2023 17:42:39 +0100 Köry Maincent wrote:
> 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 sysfs.

Ethtool, please, no sysfs.

RE: [PATCH v2 2/4] net: Expose available time stamping layers to user space.

[Willem de Bruijn]

Köry Maincent wrote:
> From: Richard Cochran <richardcochran@gmail.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 sysfs.
> 
> 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.
> 
>  .../ABI/testing/sysfs-class-net-timestamping  | 17 ++++++
>  net/core/net-sysfs.c                          | 60 +++++++++++++++++++
>  2 files changed, 77 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-net-timestamping
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-net-timestamping b/Documentation/ABI/testing/sysfs-class-net-timestamping
> new file mode 100644
> index 000000000000..529c3a6eb607
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-net-timestamping
> @@ -0,0 +1,17 @@
> +What:		/sys/class/net/<iface>/available_timestamping_providers
> +Date:		January 2022
> +Contact:	Richard Cochran <richardcochran@gmail.com>
> +Description:
> +		Enumerates the available providers for SO_TIMESTAMPING.
> +		The possible values are:
> +		- "mac"  The MAC provides time stamping.
> +		- "phy"  The PHY or MII device provides time stamping.
> +
> +What:		/sys/class/net/<iface>/current_timestamping_provider
> +Date:		January 2022
> +Contact:	Richard Cochran <richardcochran@gmail.com>
> +Description:
> +		Show the current SO_TIMESTAMPING provider.
> +		The possible values are:
> +		- "mac"  The MAC provides time stamping.
> +		- "phy"  The PHY or MII device provides time stamping.
> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> index 8409d41405df..26095634fb31 100644
> --- a/net/core/net-sysfs.c
> +++ b/net/core/net-sysfs.c
> @@ -620,6 +620,64 @@ static ssize_t threaded_store(struct device *dev,
>  }
>  static DEVICE_ATTR_RW(threaded);
>  
> +static ssize_t available_timestamping_providers_show(struct device *dev,
> +						     struct device_attribute *attr,
> +						     char *buf)
> +{
> +	const struct ethtool_ops *ops;
> +	struct net_device *netdev;
> +	struct phy_device *phydev;
> +	int ret = 0;
> +
> +	netdev = to_net_dev(dev);
> +	phydev = netdev->phydev;
> +	ops = netdev->ethtool_ops;
> +
> +	if (!rtnl_trylock())
> +		return restart_syscall();
> +
> +	ret += sprintf(buf, "%s\n", "mac");
> +	buf += 4;
> +

Should advertising mac be subject to having ops->get_ts_info?

> +	if (phy_has_tsinfo(phydev)) {
> +		ret += sprintf(buf, "%s\n", "phy");
> +		buf += 4;
> +	}
> +
> +	rtnl_unlock();
> +
> +	return ret;
> +}
> +static DEVICE_ATTR_RO(available_timestamping_providers);

[PATCH v2 3/4] net: Let the active time stamping layer be selectable.

[Köry Maincent]

From: Richard Cochran <richardcochran@gmail.com>

Make the sysfs knob writable, and add checks in the ioctl and time
stamping paths to respect the currently selected time stamping layer.

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.

 .../ABI/testing/sysfs-class-net-timestamping  |  5 +-
 drivers/net/phy/phy_device.c                  |  6 +++
 include/linux/netdevice.h                     | 10 ++++
 net/core/dev_ioctl.c                          | 44 ++++++++++++++--
 net/core/net-sysfs.c                          | 50 +++++++++++++++++--
 net/core/timestamping.c                       |  6 +++
 net/ethtool/common.c                          | 18 +++++--
 7 files changed, 127 insertions(+), 12 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-net-timestamping b/Documentation/ABI/testing/sysfs-class-net-timestamping
index 529c3a6eb607..6dfd59740cad 100644
--- a/Documentation/ABI/testing/sysfs-class-net-timestamping
+++ b/Documentation/ABI/testing/sysfs-class-net-timestamping
@@ -11,7 +11,10 @@ What:		/sys/class/net/<iface>/current_timestamping_provider
 Date:		January 2022
 Contact:	Richard Cochran <richardcochran@gmail.com>
 Description:
-		Show the current SO_TIMESTAMPING provider.
+		Shows or sets the current SO_TIMESTAMPING provider.
+		When changing the value, some packets in the kernel
+		networking stack may still be delivered with time
+		stamps from the previous provider.
 		The possible values are:
 		- "mac"  The MAC provides time stamping.
 		- "phy"  The PHY or MII device provides time stamping.
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 8cff61dbc4b5..8dff0c6493b5 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1451,6 +1451,11 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 
 	phydev->phy_link_change = phy_link_change;
 	if (dev) {
+		if (phy_has_hwtstamp(phydev))
+			dev->selected_timestamping_layer = PHY_TIMESTAMPING;
+		else
+			dev->selected_timestamping_layer = MAC_TIMESTAMPING;
+
 		phydev->attached_dev = dev;
 		dev->phydev = phydev;
 
@@ -1762,6 +1767,7 @@ void phy_detach(struct phy_device *phydev)
 
 	phy_suspend(phydev);
 	if (dev) {
+		dev->selected_timestamping_layer = MAC_TIMESTAMPING;
 		phydev->attached_dev->phydev = NULL;
 		phydev->attached_dev = NULL;
 	}
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index ba2bd604359d..d8e9da2526f0 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1742,6 +1742,11 @@ enum netdev_ml_priv_type {
 	ML_PRIV_CAN,
 };
 
+enum timestamping_layer {
+	MAC_TIMESTAMPING,
+	PHY_TIMESTAMPING,
+};
+
 /**
  *	struct net_device - The DEVICE structure.
  *
@@ -1981,6 +1986,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.
@@ -2339,6 +2347,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)
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index 7674bb9f3076..cc7cf2a542fb 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -262,6 +262,43 @@ static int dev_eth_ioctl(struct net_device *dev,
 	return err;
 }
 
+static int dev_hwtstamp_ioctl(struct net_device *dev,
+			      struct ifreq *ifr, unsigned int cmd)
+{
+	const struct net_device_ops *ops = dev->netdev_ops;
+	int err;
+
+	err = dsa_ndo_eth_ioctl(dev, ifr, cmd);
+	if (err == 0 || err != -EOPNOTSUPP)
+		return err;
+
+	if (!netif_device_present(dev))
+		return -ENODEV;
+
+	switch (dev->selected_timestamping_layer) {
+
+	case MAC_TIMESTAMPING:
+		if (ops->ndo_do_ioctl == phy_do_ioctl) {
+			/* Some drivers set .ndo_do_ioctl to phy_do_ioctl. */
+			err = -EOPNOTSUPP;
+		} else {
+			err = ops->ndo_eth_ioctl(dev, ifr, cmd);
+		}
+		break;
+
+	case PHY_TIMESTAMPING:
+		if (phy_has_hwtstamp(dev->phydev)) {
+			err = phy_mii_ioctl(dev->phydev, ifr, cmd);
+		} else {
+			err = -ENODEV;
+			WARN_ON(1);
+		}
+		break;
+	}
+
+	return err;
+}
+
 static int dev_siocbond(struct net_device *dev,
 			struct ifreq *ifr, unsigned int cmd)
 {
@@ -397,6 +434,9 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data,
 			return err;
 		fallthrough;
 
+	case SIOCGHWTSTAMP:
+		return dev_hwtstamp_ioctl(dev, ifr, cmd);
+
 	/*
 	 *	Unknown or private ioctl
 	 */
@@ -407,9 +447,7 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data,
 
 		if (cmd == SIOCGMIIPHY ||
 		    cmd == SIOCGMIIREG ||
-		    cmd == SIOCSMIIREG ||
-		    cmd == SIOCSHWTSTAMP ||
-		    cmd == SIOCGHWTSTAMP) {
+		    cmd == SIOCSMIIREG) {
 			err = dev_eth_ioctl(dev, ifr, cmd);
 		} else if (cmd == SIOCBONDENSLAVE ||
 		    cmd == SIOCBONDRELEASE ||
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 26095634fb31..66079424b100 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -666,17 +666,59 @@ static ssize_t current_timestamping_provider_show(struct device *dev,
 	if (!rtnl_trylock())
 		return restart_syscall();
 
-	if (phy_has_tsinfo(phydev)) {
-		ret = sprintf(buf, "%s\n", "phy");
-	} else {
+	switch (netdev->selected_timestamping_layer) {
+	case MAC_TIMESTAMPING:
 		ret = sprintf(buf, "%s\n", "mac");
+		break;
+	case PHY_TIMESTAMPING:
+		ret = sprintf(buf, "%s\n", "phy");
+		break;
 	}
 
 	rtnl_unlock();
 
 	return ret;
 }
-static DEVICE_ATTR_RO(current_timestamping_provider);
+
+static ssize_t current_timestamping_provider_store(struct device *dev,
+						   struct device_attribute *attr,
+						   const char *buf, size_t len)
+{
+	struct net_device *netdev = to_net_dev(dev);
+	struct net *net = dev_net(netdev);
+	enum timestamping_layer flavor;
+
+	if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
+		return -EPERM;
+
+	if (sysfs_streq(buf, "mac"))
+		flavor = MAC_TIMESTAMPING;
+	else if (sysfs_streq(buf, "phy"))
+		flavor = PHY_TIMESTAMPING;
+	else
+		return -EINVAL;
+
+	if (!rtnl_trylock())
+		return restart_syscall();
+
+	if (!dev_isalive(netdev))
+		goto out;
+
+	if (netdev->selected_timestamping_layer != flavor) {
+		const struct net_device_ops *ops = netdev->netdev_ops;
+		struct ifreq ifr = {0};
+
+		/* Disable time stamping in the current layer. */
+		if (netif_device_present(netdev) && ops->ndo_eth_ioctl)
+			ops->ndo_eth_ioctl(netdev, &ifr, SIOCSHWTSTAMP);
+
+		netdev->selected_timestamping_layer = flavor;
+	}
+out:
+	rtnl_unlock();
+	return len;
+}
+static DEVICE_ATTR_RW(current_timestamping_provider);
 
 static struct attribute *net_class_attrs[] __ro_after_init = {
 	&dev_attr_netdev_group.attr,
diff --git a/net/core/timestamping.c b/net/core/timestamping.c
index 04840697fe79..31c3142787b7 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 != 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 != 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 64a7e05cf2c2..255170c9345a 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -548,10 +548,20 @@ 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 MAC_TIMESTAMPING:
+		if (ops->get_ts_info)
+			return ops->get_ts_info(dev, info);
+		break;
+
+	case PHY_TIMESTAMPING:
+		if (phy_has_tsinfo(phydev)) {
+			return phy_ts_info(phydev, info);
+		}
+		WARN_ON(1);
+		return -ENODEV;
+	}
 
 	info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |
 				SOF_TIMESTAMPING_SOFTWARE;
-- 
2.25.1

Re: [PATCH v2 3/4] net: Let the active time stamping layer be selectable.

[kernel test robot]

Hi Köry,

I love your patch! Perhaps something to improve:

[auto build test WARNING on v6.2]
[also build test WARNING on next-20230303]
[cannot apply to net/master net-next/master horms-ipvs/master linus/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/K-ry-Maincent/net-ethtool-Refactor-identical-get_ts_info-implementations/20230304-004527
patch link:    https://lore.kernel.org/r/20230303164248.499286-4-kory.maincent%40bootlin.com
patch subject: [PATCH v2 3/4] net: Let the active time stamping layer be selectable.
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230304/202303040219.nmNWbGrY-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/00a0656f9b222cfeb7c1253a4a2771b1f63b5c9b
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review K-ry-Maincent/net-ethtool-Refactor-identical-get_ts_info-implementations/20230304-004527
        git checkout 00a0656f9b222cfeb7c1253a4a2771b1f63b5c9b
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash net/core/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303040219.nmNWbGrY-lkp@intel.com/

All warnings (new ones prefixed by >>):

   net/core/net-sysfs.c: In function 'available_timestamping_providers_show':
   net/core/net-sysfs.c:627:35: warning: variable 'ops' set but not used [-Wunused-but-set-variable]
     627 |         const struct ethtool_ops *ops;
         |                                   ^~~
   net/core/net-sysfs.c: In function 'current_timestamping_provider_show':
>> net/core/net-sysfs.c:659:28: warning: variable 'phydev' set but not used [-Wunused-but-set-variable]
     659 |         struct phy_device *phydev;
         |                            ^~~~~~
   net/core/net-sysfs.c:657:35: warning: variable 'ops' set but not used [-Wunused-but-set-variable]
     657 |         const struct ethtool_ops *ops;
         |                                   ^~~


vim +/phydev +659 net/core/net-sysfs.c

90d54e1c6ed12a Richard Cochran 2023-03-03  652  
90d54e1c6ed12a Richard Cochran 2023-03-03  653  static ssize_t current_timestamping_provider_show(struct device *dev,
90d54e1c6ed12a Richard Cochran 2023-03-03  654  						  struct device_attribute *attr,
90d54e1c6ed12a Richard Cochran 2023-03-03  655  						  char *buf)
90d54e1c6ed12a Richard Cochran 2023-03-03  656  {
90d54e1c6ed12a Richard Cochran 2023-03-03  657  	const struct ethtool_ops *ops;
90d54e1c6ed12a Richard Cochran 2023-03-03  658  	struct net_device *netdev;
90d54e1c6ed12a Richard Cochran 2023-03-03 @659  	struct phy_device *phydev;
90d54e1c6ed12a Richard Cochran 2023-03-03  660  	int ret;
90d54e1c6ed12a Richard Cochran 2023-03-03  661  
90d54e1c6ed12a Richard Cochran 2023-03-03  662  	netdev = to_net_dev(dev);
90d54e1c6ed12a Richard Cochran 2023-03-03  663  	phydev = netdev->phydev;
90d54e1c6ed12a Richard Cochran 2023-03-03  664  	ops = netdev->ethtool_ops;
90d54e1c6ed12a Richard Cochran 2023-03-03  665  
90d54e1c6ed12a Richard Cochran 2023-03-03  666  	if (!rtnl_trylock())
90d54e1c6ed12a Richard Cochran 2023-03-03  667  		return restart_syscall();
90d54e1c6ed12a Richard Cochran 2023-03-03  668  
00a0656f9b222c Richard Cochran 2023-03-03  669  	switch (netdev->selected_timestamping_layer) {
00a0656f9b222c Richard Cochran 2023-03-03  670  	case MAC_TIMESTAMPING:
90d54e1c6ed12a Richard Cochran 2023-03-03  671  		ret = sprintf(buf, "%s\n", "mac");
00a0656f9b222c Richard Cochran 2023-03-03  672  		break;
00a0656f9b222c Richard Cochran 2023-03-03  673  	case PHY_TIMESTAMPING:
00a0656f9b222c Richard Cochran 2023-03-03  674  		ret = sprintf(buf, "%s\n", "phy");
00a0656f9b222c Richard Cochran 2023-03-03  675  		break;
90d54e1c6ed12a Richard Cochran 2023-03-03  676  	}
90d54e1c6ed12a Richard Cochran 2023-03-03  677  
90d54e1c6ed12a Richard Cochran 2023-03-03  678  	rtnl_unlock();
90d54e1c6ed12a Richard Cochran 2023-03-03  679  
90d54e1c6ed12a Richard Cochran 2023-03-03  680  	return ret;
90d54e1c6ed12a Richard Cochran 2023-03-03  681  }
00a0656f9b222c Richard Cochran 2023-03-03  682  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

RE: [PATCH v2 3/4] net: Let the active time stamping layer be selectable.

[Willem de Bruijn]

Köry Maincent wrote:
> From: Richard Cochran <richardcochran@gmail.com>
> 
> Make the sysfs knob writable, and add checks in the ioctl and time
> stamping paths to respect the currently selected time stamping layer.
> 
> 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.
> 
>  .../ABI/testing/sysfs-class-net-timestamping  |  5 +-
>  drivers/net/phy/phy_device.c                  |  6 +++
>  include/linux/netdevice.h                     | 10 ++++
>  net/core/dev_ioctl.c                          | 44 ++++++++++++++--
>  net/core/net-sysfs.c                          | 50 +++++++++++++++++--
>  net/core/timestamping.c                       |  6 +++
>  net/ethtool/common.c                          | 18 +++++--
>  7 files changed, 127 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-net-timestamping b/Documentation/ABI/testing/sysfs-class-net-timestamping
> index 529c3a6eb607..6dfd59740cad 100644
> --- a/Documentation/ABI/testing/sysfs-class-net-timestamping
> +++ b/Documentation/ABI/testing/sysfs-class-net-timestamping
> @@ -11,7 +11,10 @@ What:		/sys/class/net/<iface>/current_timestamping_provider
>  Date:		January 2022
>  Contact:	Richard Cochran <richardcochran@gmail.com>
>  Description:
> -		Show the current SO_TIMESTAMPING provider.
> +		Shows or sets the current SO_TIMESTAMPING provider.
> +		When changing the value, some packets in the kernel
> +		networking stack may still be delivered with time
> +		stamps from the previous provider.
>  		The possible values are:
>  		- "mac"  The MAC provides time stamping.
>  		- "phy"  The PHY or MII device provides time stamping.
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 8cff61dbc4b5..8dff0c6493b5 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1451,6 +1451,11 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  
>  	phydev->phy_link_change = phy_link_change;
>  	if (dev) {
> +		if (phy_has_hwtstamp(phydev))
> +			dev->selected_timestamping_layer = PHY_TIMESTAMPING;
> +		else
> +			dev->selected_timestamping_layer = MAC_TIMESTAMPING;
> +
>  		phydev->attached_dev = dev;
>  		dev->phydev = phydev;
>  
> @@ -1762,6 +1767,7 @@ void phy_detach(struct phy_device *phydev)
>  
>  	phy_suspend(phydev);
>  	if (dev) {
> +		dev->selected_timestamping_layer = MAC_TIMESTAMPING;
>  		phydev->attached_dev->phydev = NULL;
>  		phydev->attached_dev = NULL;
>  	}
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index ba2bd604359d..d8e9da2526f0 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1742,6 +1742,11 @@ enum netdev_ml_priv_type {
>  	ML_PRIV_CAN,
>  };
>  
> +enum timestamping_layer {
> +	MAC_TIMESTAMPING,
> +	PHY_TIMESTAMPING,
> +};
> +
>  /**
>   *	struct net_device - The DEVICE structure.
>   *
> @@ -1981,6 +1986,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.
> @@ -2339,6 +2347,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)
> diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
> index 7674bb9f3076..cc7cf2a542fb 100644
> --- a/net/core/dev_ioctl.c
> +++ b/net/core/dev_ioctl.c
> @@ -262,6 +262,43 @@ static int dev_eth_ioctl(struct net_device *dev,
>  	return err;
>  }
>  
> +static int dev_hwtstamp_ioctl(struct net_device *dev,
> +			      struct ifreq *ifr, unsigned int cmd)
> +{
> +	const struct net_device_ops *ops = dev->netdev_ops;
> +	int err;
> +
> +	err = dsa_ndo_eth_ioctl(dev, ifr, cmd);
> +	if (err == 0 || err != -EOPNOTSUPP)
> +		return err;
> +
> +	if (!netif_device_present(dev))
> +		return -ENODEV;
> +
> +	switch (dev->selected_timestamping_layer) {
> +
> +	case MAC_TIMESTAMPING:
> +		if (ops->ndo_do_ioctl == phy_do_ioctl) {
> +			/* Some drivers set .ndo_do_ioctl to phy_do_ioctl. */
> +			err = -EOPNOTSUPP;
> +		} else {
> +			err = ops->ndo_eth_ioctl(dev, ifr, cmd);
> +		}
> +		break;
> +
> +	case PHY_TIMESTAMPING:
> +		if (phy_has_hwtstamp(dev->phydev)) {
> +			err = phy_mii_ioctl(dev->phydev, ifr, cmd);
> +		} else {
> +			err = -ENODEV;
> +			WARN_ON(1);
> +		}
> +		break;
> +	}
> +
> +	return err;
> +}
> +
>  static int dev_siocbond(struct net_device *dev,
>  			struct ifreq *ifr, unsigned int cmd)
>  {
> @@ -397,6 +434,9 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data,
>  			return err;
>  		fallthrough;
>  
> +	case SIOCGHWTSTAMP:
> +		return dev_hwtstamp_ioctl(dev, ifr, cmd);
> +
>  	/*
>  	 *	Unknown or private ioctl
>  	 */
> @@ -407,9 +447,7 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data,
>  
>  		if (cmd == SIOCGMIIPHY ||
>  		    cmd == SIOCGMIIREG ||
> -		    cmd == SIOCSMIIREG ||
> -		    cmd == SIOCSHWTSTAMP ||
> -		    cmd == SIOCGHWTSTAMP) {
> +		    cmd == SIOCSMIIREG) {
>  			err = dev_eth_ioctl(dev, ifr, cmd);
>  		} else if (cmd == SIOCBONDENSLAVE ||
>  		    cmd == SIOCBONDRELEASE ||
> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> index 26095634fb31..66079424b100 100644
> --- a/net/core/net-sysfs.c
> +++ b/net/core/net-sysfs.c
> @@ -666,17 +666,59 @@ static ssize_t current_timestamping_provider_show(struct device *dev,
>  	if (!rtnl_trylock())
>  		return restart_syscall();
>  
> -	if (phy_has_tsinfo(phydev)) {
> -		ret = sprintf(buf, "%s\n", "phy");
> -	} else {
> +	switch (netdev->selected_timestamping_layer) {
> +	case MAC_TIMESTAMPING:
>  		ret = sprintf(buf, "%s\n", "mac");
> +		break;
> +	case PHY_TIMESTAMPING:
> +		ret = sprintf(buf, "%s\n", "phy");
> +		break;
>  	}
>  
>  	rtnl_unlock();
>  
>  	return ret;
>  }
> -static DEVICE_ATTR_RO(current_timestamping_provider);
> +
> +static ssize_t current_timestamping_provider_store(struct device *dev,
> +						   struct device_attribute *attr,
> +						   const char *buf, size_t len)
> +{
> +	struct net_device *netdev = to_net_dev(dev);
> +	struct net *net = dev_net(netdev);
> +	enum timestamping_layer flavor;
> +
> +	if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
> +		return -EPERM;
> +
> +	if (sysfs_streq(buf, "mac"))
> +		flavor = MAC_TIMESTAMPING;
> +	else if (sysfs_streq(buf, "phy"))
> +		flavor = PHY_TIMESTAMPING;
> +	else
> +		return -EINVAL;

Should setting netdev->selected_timestamping_layer be limited to
choices that the device supports?

At a higher level, this series assumes that any timestamp not through
phydev is a MAC timestamp. I don't think that is necessarily true for
all devices. Some may timestamp at the phy, but not expose a phydev.
This is a somewhat pedantic point. I understand that the purpose of
the series is to select from among two sets of APIs.

Re: [PATCH v2 3/4] net: Let the active time stamping layer be selectable.

[Andrew Lunn]

> Should setting netdev->selected_timestamping_layer be limited to
> choices that the device supports?
> 
> At a higher level, this series assumes that any timestamp not through
> phydev is a MAC timestamp. I don't think that is necessarily true for
> all devices. Some may timestamp at the phy, but not expose a phydev.
> This is a somewhat pedantic point. I understand that the purpose of
> the series is to select from among two sets of APIs.

Network drivers tend to fall into one of two classes.

1) Linux drives the whole hardware, MAC, PCS, PHY, SPF cage, LEDs etc.

2) Linux drives just the MAC, and the rest is hidden away by firmware.

For this to work, the MAC API should be sufficient to configure and
get status information for things which are hidden away from Linux.
An example of this is the ethtool .get_link_ksettings, which mostly
deals with PHY settings. Those which have linux controlling the
hardware call phy_ethtool_get_link_ksettings to get phylib to do the
work, where as if the hardware is hidden away, calls into the firmware
are made to implement the API.

When we are talking about time stamping, i assume you are talking
about firmware driver the lower level hardware. I can see two ways
this could go:

1) The MAC driver registers two timestamping devices with the core,
one for the MAC and another for the PHY. In that case, all we need is
the registration API to include some sort of indicator what layer this
time stamper works at. The core can then expose to user space that
there are two, and mux kAPI calls to one or the other.

2) Despite the hardware having two stampers, it only exposes one to
the PTP core. Firmware driven hardware already has intimate knowledge
of the hardware, since it has to have firmware to drive the hardware,
so it should be able to say which is the better stamper. So it just
exposes that one.

So i think the proposed API does work for firmware driven stampers,
but we might need to extend ptp_caps to include a level indication,
MAC, bump in the wire, PHY, etc.

     Andrew

Re: [PATCH v2 3/4] net: Let the active time stamping layer be selectable.

[kernel test robot]

Hi Köry,

I love your patch! Yet something to improve:

[auto build test ERROR on v6.2]
[also build test ERROR on next-20230303]
[cannot apply to net/master net-next/master horms-ipvs/master linus/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/K-ry-Maincent/net-ethtool-Refactor-identical-get_ts_info-implementations/20230304-004527
patch link:    https://lore.kernel.org/r/20230303164248.499286-4-kory.maincent%40bootlin.com
patch subject: [PATCH v2 3/4] net: Let the active time stamping layer be selectable.
config: csky-defconfig (https://download.01.org/0day-ci/archive/20230304/202303041027.GxlLyldN-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/00a0656f9b222cfeb7c1253a4a2771b1f63b5c9b
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review K-ry-Maincent/net-ethtool-Refactor-identical-get_ts_info-implementations/20230304-004527
        git checkout 00a0656f9b222cfeb7c1253a4a2771b1f63b5c9b
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=csky olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=csky SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303041027.GxlLyldN-lkp@intel.com/

All errors (new ones prefixed by >>):

   csky-linux-ld: net/core/dev_ioctl.o: in function `dev_ifsioc':
>> dev_ioctl.c:(.text+0x292): undefined reference to `phy_mii_ioctl'
>> csky-linux-ld: dev_ioctl.c:(.text+0x370): undefined reference to `phy_do_ioctl'
>> csky-linux-ld: dev_ioctl.c:(.text+0x374): undefined reference to `phy_mii_ioctl'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH v2 3/4] net: Let the active time stamping layer be selectable.

[Andrew Lunn]

On Fri, Mar 03, 2023 at 05:42:40PM +0100, Köry Maincent wrote:
> From: Richard Cochran <richardcochran@gmail.com>
> 
> Make the sysfs knob writable, and add checks in the ioctl and time
> stamping paths to respect the currently selected time stamping layer.

Although it probably works, i think the ioctl code is ugly.

I think it would be better to pull the IOCTL code into the PTP object
interface. Add an ioctl member to struct ptp_clock_info. The PTP core
can then directly call into the PTP object.

You now have a rather odd semantic that calling the .ndo_eth_ioctl
means operate on the MAC PTP. If you look at net_device_ops, i don't
think any of the other members have this semantic. They all look at
the netdev as a whole, and ask the netdev to do something, without
caring what level it operates at. So a PTP ioctl should operate on
'the' PTP of the netdev, whichever that might be, MAC or PHY.

Clearly, it is a bigger change, you need to touch every MAC driver
with PTP support, but at the end, you have a cleaner architecture.

     Andrew

Re: [PATCH v2 3/4] net: Let the active time stamping layer be selectable.

[Russell King (Oracle)]

On Sat, Mar 04, 2023 at 04:43:47PM +0100, Andrew Lunn wrote:
> On Fri, Mar 03, 2023 at 05:42:40PM +0100, Köry Maincent wrote:
> > From: Richard Cochran <richardcochran@gmail.com>
> > 
> > Make the sysfs knob writable, and add checks in the ioctl and time
> > stamping paths to respect the currently selected time stamping layer.
> 
> Although it probably works, i think the ioctl code is ugly.
> 
> I think it would be better to pull the IOCTL code into the PTP object
> interface. Add an ioctl member to struct ptp_clock_info. The PTP core
> can then directly call into the PTP object.

Putting it into ptp_clock_info makes little sense to me, because this
is for the PHC, which is exposed via /dev/ptp*, and that's what the
various methods in that structure are for

The timestamping part is via the netdev, which is a separate entity,
and its that entity which is responsible for identifying which PHC it
is connected to (normally by filling in the phc_index field of
ethtool_ts_info.)

Think of is as:

  netdev ---- timestamping ---- PHC

since we can have:

  netdev1 ---- timestamping \
  netdev2 ---- timestamping -*--- PHC
  netdev3 ---- timestamping /

Since the ioctl is to do with requesting what we want the timestamping
layer to be doing with packets, putting it in ptp_clock_info makes
very little sense.

> You now have a rather odd semantic that calling the .ndo_eth_ioctl
> means operate on the MAC PTP. If you look at net_device_ops, i don't
> think any of the other members have this semantic. They all look at
> the netdev as a whole, and ask the netdev to do something, without
> caring what level it operates at. So a PTP ioctl should operate on
> 'the' PTP of the netdev, whichever that might be, MAC or PHY.

Well, what we have today is:

int __ethtool_get_ts_info(struct net_device *dev, struct ethtool_ts_info *info)
{
...
        if (phy_has_tsinfo(phydev))
                return phy_ts_info(phydev, info);
        if (ops->get_ts_info)
                return ops->get_ts_info(dev, info);
}

So, one can argue that we already have this "odd" semantic, in that
calling get_ts_info() means to operate on the MAC PTP implementation.
Making the ioctl also do that merely brings it into line with this
existing code!

If we want in general for the netdev to always be called, then we need
to remove the above, but then we need to go through all the networking
drivers working out which need to provide a get_ts_info() and forward
that to phylib. Maybe that's a good thing in the longer run though?

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

Re: [PATCH v2 3/4] net: Let the active time stamping layer be selectable.

[Andrew Lunn]

> The timestamping part is via the netdev, which is a separate entity,
> and its that entity which is responsible for identifying which PHC it
> is connected to (normally by filling in the phc_index field of
> ethtool_ts_info.)
> 
> Think of is as:
> 
>   netdev ---- timestamping ---- PHC
> 
> since we can have:
> 
>   netdev1 ---- timestamping \
>   netdev2 ---- timestamping -*--- PHC
>   netdev3 ---- timestamping /
> 
> Since the ioctl is to do with requesting what we want the timestamping
> layer to be doing with packets, putting it in ptp_clock_info makes
> very little sense.

So there does not appear to be an object to represent a time stamper?

Should one be added? It looks like it needs two ops hwtstamp_set() and
hwtstamp_get(). It would then be registered with the ptp core. And
then the rest of what i said would apply...

	Andrew

Re: [PATCH v2 3/4] net: Let the active time stamping layer be selectable.

[Jakub Kicinski]

On Sat, 4 Mar 2023 20:46:05 +0100 Andrew Lunn wrote:
> > Since the ioctl is to do with requesting what we want the timestamping
> > layer to be doing with packets, putting it in ptp_clock_info makes
> > very little sense.  
> 
> So there does not appear to be an object to represent a time stamper?
> 
> Should one be added? It looks like it needs two ops hwtstamp_set() and
> hwtstamp_get(). It would then be registered with the ptp core. And
> then the rest of what i said would apply...

IMHO time stamper is very much part of the netdev. I attribute the lack
of clarity palatially to the fact that (for reasons unknown) we still
lug the request as a raw IOCTL/ifreq. Rather than converting it to an
NDO/phydev op in the core.. Also can't think of a reason why modeling
it as a separate object would be useful?

[PATCH v2 4/4] 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.

Signed-off-by: Richard Cochran <richardcochran@gmail.com>
Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 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 f250b0df27fb..b98119551e6a 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -3058,22 +3058,19 @@ static int fec_enet_ioctl(struct net_device *ndev, struct ifreq *rq, int cmd)
 	if (!netif_running(ndev))
 		return -EINVAL;
 
+	switch (cmd) {
+	case SIOCSHWTSTAMP:
+		return fep->bufdesc_ex ? fec_ptp_set(ndev, rq) :
+		-EOPNOTSUPP;
+
+	case SIOCGHWTSTAMP:
+		return fep->bufdesc_ex ? fec_ptp_get(ndev, rq) :
+		-EOPNOTSUPP;
+	}
+
 	if (!phydev)
 		return -ENODEV;
 
-	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);
-		}
-	}
-
 	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 50858cc10fef..8c37db28a93d 100644
--- a/drivers/net/ethernet/mscc/ocelot_net.c
+++ b/drivers/net/ethernet/mscc/ocelot_net.c
@@ -882,18 +882,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 758295c898ac..b15b83bb269a 100644
--- a/drivers/net/ethernet/ti/cpsw_priv.c
+++ b/drivers/net/ethernet/ti/cpsw_priv.c
@@ -714,13 +714,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 751fb0bc65c5..36ce80f8bd6b 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 v2 0/4] 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.

[Köry Maincent]

On Fri,  3 Mar 2023 17:42:37 +0100
Köry Maincent <kory.maincent@bootlin.com> wrote:

Oops, sorry some bug in my cover letter subject.


Regards,
Köry