From: Kory Maincent <[email protected]>
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 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.
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.
Kory Maincent (2):
net: Expose available time stamping layers to user space.
dt-bindings: net: phy: add timestamp preferred choice property
Richard Cochran (3):
net: ethtool: Refactor identical get_ts_info implementations.
net: Let the active time stamping layer be selectable.
net: fix up drivers WRT phy time stamping
.../devicetree/bindings/net/ethernet-phy.yaml | 7 ++
Documentation/networking/ethtool-netlink.rst | 3 +
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 | 34 +++++++++
include/linux/ethtool.h | 8 +++
include/linux/netdevice.h | 6 ++
include/uapi/linux/ethtool.h | 3 +
include/uapi/linux/net_tstamp.h | 6 ++
net/8021q/vlan_dev.c | 15 +---
net/core/dev_ioctl.c | 43 ++++++++++-
net/core/timestamping.c | 6 ++
net/ethtool/common.c | 22 ++++--
net/ethtool/ioctl.c | 71 +++++++++++++++++++
18 files changed, 237 insertions(+), 97 deletions(-)
--
2.25.1
From: Richard Cochran <[email protected]>
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 <[email protected]>
Signed-off-by: Kory Maincent <[email protected]>
---
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
From: Kory Maincent <[email protected]>
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 <[email protected]>
Signed-off-by: Kory Maincent <[email protected]>
---
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
Documentation/networking/ethtool-netlink.rst | 2 +
include/uapi/linux/ethtool.h | 2 +
include/uapi/linux/net_tstamp.h | 6 +++
net/ethtool/ioctl.c | 50 ++++++++++++++++++++
4 files changed, 60 insertions(+)
diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index d578b8bcd8a4..ca8e1182bc8e 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -1787,4 +1787,6 @@ are netlink only.
n/a ``ETHTOOL_MSG_PHC_VCLOCKS_GET``
n/a ``ETHTOOL_MSG_MODULE_GET``
n/a ``ETHTOOL_MSG_MODULE_SET``
+ ``ETHTOOL_LIST_PTP`` n/a
+ ``ETHTOOL_GET_PTP`` n/a
=================================== =====================================
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index dc2aa3d75b39..56cf24388290 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1629,6 +1629,8 @@ enum ethtool_fec_config_bits {
#define ETHTOOL_PHY_STUNABLE 0x0000004f /* Set PHY tunable configuration */
#define ETHTOOL_GFECPARAM 0x00000050 /* Get FEC settings */
#define ETHTOOL_SFECPARAM 0x00000051 /* Set FEC settings */
+#define ETHTOOL_LIST_PTP 0x00000052 /* List PTP providers */
+#define ETHTOOL_GET_PTP 0x00000053 /* Get current PTP provider */
/* compatibility with older code */
#define SPARC_ETH_GSET ETHTOOL_GSET
diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
index 55501e5e7ac8..1ec489e18f97 100644
--- a/include/uapi/linux/net_tstamp.h
+++ b/include/uapi/linux/net_tstamp.h
@@ -13,6 +13,12 @@
#include <linux/types.h>
#include <linux/socket.h> /* for SO_TIMESTAMPING */
+/* Hardware layer of the SO_TIMESTAMPING provider */
+enum timestamping_layer {
+ MAC_TIMESTAMPING = (1<<0),
+ PHY_TIMESTAMPING = (1<<1),
+};
+
/* SO_TIMESTAMPING flags */
enum {
SOF_TIMESTAMPING_TX_HARDWARE = (1<<0),
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 81fe2422fe58..d8a0a5d991e0 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -2319,6 +2319,48 @@ static int ethtool_get_ts_info(struct net_device *dev, void __user *useraddr)
return 0;
}
+static int ethtool_list_ptp(struct net_device *dev, void __user *useraddr)
+{
+ struct ethtool_value edata = {
+ .cmd = ETHTOOL_LIST_PTP,
+ .data = 0,
+ };
+ struct phy_device *phydev = dev->phydev;
+ const struct ethtool_ops *ops = dev->ethtool_ops;
+
+ if (phy_has_tsinfo(phydev))
+ edata.data = PHY_TIMESTAMPING;
+ if (ops->get_ts_info)
+ edata.data |= MAC_TIMESTAMPING;
+
+ if (copy_to_user(useraddr, &edata, sizeof(edata)))
+ return -EFAULT;
+
+ return 0;
+}
+
+static int ethtool_get_ptp(struct net_device *dev, void __user *useraddr)
+{
+ struct ethtool_value edata = {
+ .cmd = ETHTOOL_GET_PTP,
+ .data = 0,
+ };
+ struct phy_device *phydev = dev->phydev;
+ const struct ethtool_ops *ops = dev->ethtool_ops;
+
+ if (phy_has_tsinfo(phydev))
+ edata.data = PHY_TIMESTAMPING;
+ else if (ops->get_ts_info)
+ edata.data = MAC_TIMESTAMPING;
+ else
+ return -EOPNOTSUPP;
+
+ if (copy_to_user(useraddr, &edata, sizeof(edata)))
+ return -EFAULT;
+
+ return 0;
+}
+
int ethtool_get_module_info_call(struct net_device *dev,
struct ethtool_modinfo *modinfo)
{
@@ -2770,6 +2812,8 @@ __dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr,
case ETHTOOL_PHY_GTUNABLE:
case ETHTOOL_GLINKSETTINGS:
case ETHTOOL_GFECPARAM:
+ case ETHTOOL_LIST_PTP:
+ case ETHTOOL_GET_PTP:
break;
default:
if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
@@ -2997,6 +3041,12 @@ __dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr,
case ETHTOOL_SFECPARAM:
rc = ethtool_set_fecparam(dev, useraddr);
break;
+ case ETHTOOL_LIST_PTP:
+ rc = ethtool_list_ptp(dev, useraddr);
+ break;
+ case ETHTOOL_GET_PTP:
+ rc = ethtool_get_ptp(dev, useraddr);
+ break;
default:
rc = -EOPNOTSUPP;
}
--
2.25.1
From: Richard Cochran <[email protected]>
Add the ETHTOOL_SET_PTP ethtool ioctl, 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.
Signed-off-by: Richard Cochran <[email protected]>
Signed-off-by: Kory Maincent <[email protected]>
---
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:
- Added 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
Documentation/networking/ethtool-netlink.rst | 1 +
drivers/net/phy/phy_device.c | 34 ++++++++++++++++
include/linux/netdevice.h | 6 +++
include/uapi/linux/ethtool.h | 1 +
net/core/dev_ioctl.c | 43 ++++++++++++++++++--
net/core/timestamping.c | 6 +++
net/ethtool/common.c | 16 ++++++--
net/ethtool/ioctl.c | 41 ++++++++++++++-----
8 files changed, 131 insertions(+), 17 deletions(-)
diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index ca8e1182bc8e..4a1153dd4859 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -1789,4 +1789,5 @@ are netlink only.
n/a ``ETHTOOL_MSG_MODULE_SET``
``ETHTOOL_LIST_PTP`` n/a
``ETHTOOL_GET_PTP`` n/a
+ ``ETHTOOL_SET_PTP`` n/a
=================================== =====================================
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 8cff61dbc4b5..5e120452a358 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -34,6 +34,9 @@
#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");
@@ -1378,6 +1381,34 @@ int phy_sfp_probe(struct phy_device *phydev,
}
EXPORT_SYMBOL(phy_sfp_probe);
+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;
+
+ if (phy_has_hwtstamp(phydev))
+ ts_layer = PHY_TIMESTAMPING;
+ else if (ops->get_ts_info)
+ ts_layer = MAC_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 = PHY_TIMESTAMPING;
+
+ if (ops->get_ts_info && !strcmp(s, "mac"))
+ ts_layer = 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
@@ -1451,6 +1482,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;
@@ -1762,6 +1795,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..d9a1c12fc43c 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -47,6 +47,7 @@
#include <uapi/linux/netdevice.h>
#include <uapi/linux/if_bonding.h>
#include <uapi/linux/pkt_cls.h>
+#include <uapi/linux/net_tstamp.h>
#include <linux/hashtable.h>
#include <linux/rbtree.h>
#include <net/net_trackers.h>
@@ -1981,6 +1982,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 +2343,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/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 56cf24388290..d3a41b6e9eb0 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1631,6 +1631,7 @@ enum ethtool_fec_config_bits {
#define ETHTOOL_SFECPARAM 0x00000051 /* Set FEC settings */
#define ETHTOOL_LIST_PTP 0x00000052 /* List PTP providers */
#define ETHTOOL_GET_PTP 0x00000053 /* Get current PTP provider */
+#define ETHTOOL_SET_PTP 0x00000054 /* Set PTP provider */
/* compatibility with older code */
#define SPARC_ETH_GSET ETHTOOL_GSET
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index 7674bb9f3076..a75cff331495 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -262,6 +262,42 @@ 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 +433,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 +446,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/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..e55e70bdbb3c 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -548,10 +548,18 @@ 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;
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index d8a0a5d991e0..85a074bef17d 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -2343,17 +2343,8 @@ static int ethtool_get_ptp(struct net_device *dev, void __user *useraddr)
{
struct ethtool_value edata = {
.cmd = ETHTOOL_GET_PTP,
- .data = 0,
+ .data = dev->selected_timestamping_layer,
};
- struct phy_device *phydev = dev->phydev;
- const struct ethtool_ops *ops = dev->ethtool_ops;
-
- if (phy_has_tsinfo(phydev))
- edata.data = PHY_TIMESTAMPING;
- else if (ops->get_ts_info)
- edata.data = MAC_TIMESTAMPING;
- else
- return -EOPNOTSUPP;
if (copy_to_user(useraddr, &edata, sizeof(edata)))
return -EFAULT;
@@ -2361,6 +2352,33 @@ static int ethtool_get_ptp(struct net_device *dev, void __user *useraddr)
return 0;
}
+static int ethtool_set_ptp(struct net_device *dev, void __user *useraddr)
+{
+ struct ethtool_value edata;
+ enum timestamping_layer flavor;
+
+ if (copy_from_user(&edata, useraddr, sizeof(edata)))
+ return -EFAULT;
+
+ flavor = edata.data;
+
+ if (!dev->phydev)
+ return 0;
+
+ if (dev->selected_timestamping_layer != flavor) {
+ 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 0;
+}
+
int ethtool_get_module_info_call(struct net_device *dev,
struct ethtool_modinfo *modinfo)
{
@@ -3047,6 +3065,9 @@ __dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr,
case ETHTOOL_GET_PTP:
rc = ethtool_get_ptp(dev, useraddr);
break;
+ case ETHTOOL_SET_PTP:
+ rc = ethtool_set_ptp(dev, useraddr);
+ break;
default:
rc = -EOPNOTSUPP;
}
--
2.25.1
From: Richard Cochran <[email protected]>
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 <[email protected]>
Signed-off-by: Kory Maincent <[email protected]>
---
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
From: Kory Maincent <[email protected]>
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 <[email protected]>
---
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 ad808e9ce5b9..3ea6d2a59ff7 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
Köry Maincent wrote:
> From: Richard Cochran <[email protected]>
>
> Add the ETHTOOL_SET_PTP ethtool ioctl, 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.
nit: oh -> of
>
> Signed-off-by: Richard Cochran <[email protected]>
> Signed-off-by: Kory Maincent <[email protected]>
> ---
>
> 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:
> - Added 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
>
> Documentation/networking/ethtool-netlink.rst | 1 +
> drivers/net/phy/phy_device.c | 34 ++++++++++++++++
> include/linux/netdevice.h | 6 +++
> include/uapi/linux/ethtool.h | 1 +
> net/core/dev_ioctl.c | 43 ++++++++++++++++++--
> net/core/timestamping.c | 6 +++
> net/ethtool/common.c | 16 ++++++--
> net/ethtool/ioctl.c | 41 ++++++++++++++-----
> 8 files changed, 131 insertions(+), 17 deletions(-)
>
> +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;
> +
> + if (phy_has_hwtstamp(phydev))
> + ts_layer = PHY_TIMESTAMPING;
> + else if (ops->get_ts_info)
> + ts_layer = MAC_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 = PHY_TIMESTAMPING;
> +
> + if (ops->get_ts_info && !strcmp(s, "mac"))
> + ts_layer = MAC_TIMESTAMPING;
> +
> +out:
> + netdev->selected_timestamping_layer = ts_layer;
> +}
> +
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index ba2bd604359d..d9a1c12fc43c 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -47,6 +47,7 @@
> #include <uapi/linux/netdevice.h>
> #include <uapi/linux/if_bonding.h>
> #include <uapi/linux/pkt_cls.h>
> +#include <uapi/linux/net_tstamp.h>
> #include <linux/hashtable.h>
> #include <linux/rbtree.h>
> #include <net/net_trackers.h>
> @@ -1981,6 +1982,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 +2343,8 @@ struct net_device {
> unsigned wol_enabled:1;
> unsigned threaded:1;
>
> + enum timestamping_layer selected_timestamping_layer;
> +
can perhaps be a single bit rather than an enum
> +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);
Please no WARN_ON on error cases that are known to be reachable
and can be handled safely and reported to userspace.
> + }
> + break;
> + }
> +
> + return err;
> +}
> +
> diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> index 64a7e05cf2c2..e55e70bdbb3c 100644
> --- a/net/ethtool/common.c
> +++ b/net/ethtool/common.c
> @@ -548,10 +548,18 @@ 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;
same
> + }
>
> info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |
> SOF_TIMESTAMPING_SOFTWARE;
Hi K?ry,
I love your patch! Perhaps something to improve:
[auto build test WARNING on v6.2]
[cannot apply to robh/for-next horms-ipvs/master net/master net-next/master linus/master v6.3-rc1 next-20230308]
[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/20230308-220453
patch link: https://lore.kernel.org/r/20230308135936.761794-4-kory.maincent%40bootlin.com
patch subject: [PATCH v3 3/5] net: Let the active time stamping layer be selectable.
config: riscv-allmodconfig (https://download.01.org/0day-ci/archive/20230309/[email protected]/config)
compiler: riscv64-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/d81a36f239360e7e3b9ca2633e52b3cb12205590
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/20230308-220453
git checkout d81a36f239360e7e3b9ca2633e52b3cb12205590
# 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=riscv olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/net/phy/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> drivers/net/phy/phy_device.c:1384:6: warning: no previous prototype for 'of_set_timestamp' [-Wmissing-prototypes]
1384 | void of_set_timestamp(struct net_device *netdev, struct phy_device *phydev)
| ^~~~~~~~~~~~~~~~
vim +/of_set_timestamp +1384 drivers/net/phy/phy_device.c
1383
> 1384 void of_set_timestamp(struct net_device *netdev, struct phy_device *phydev)
1385 {
1386 struct device_node *node = phydev->mdio.dev.of_node;
1387 const struct ethtool_ops *ops = netdev->ethtool_ops;
1388 const char *s;
1389 enum timestamping_layer ts_layer = 0;
1390
1391 if (phy_has_hwtstamp(phydev))
1392 ts_layer = PHY_TIMESTAMPING;
1393 else if (ops->get_ts_info)
1394 ts_layer = MAC_TIMESTAMPING;
1395
1396 if (of_property_read_string(node, "preferred-timestamp", &s))
1397 goto out;
1398
1399 if (!s)
1400 goto out;
1401
1402 if (phy_has_hwtstamp(phydev) && !strcmp(s, "phy"))
1403 ts_layer = PHY_TIMESTAMPING;
1404
1405 if (ops->get_ts_info && !strcmp(s, "mac"))
1406 ts_layer = MAC_TIMESTAMPING;
1407
1408 out:
1409 netdev->selected_timestamping_layer = ts_layer;
1410 }
1411
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Hi K?ry,
On Wed, Mar 08, 2023 at 02:59:26PM +0100, K?ry Maincent wrote:
> From: Kory Maincent <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Kory Maincent <[email protected]>
> ---
I'm pretty sure that all new ethtool commands must be implemented
through the genetlink socket interface. The ioctl interface stopped
being extended and is in maintenance mode only.
(trimmed CC list of bouncing email addresses)
On Wed, Mar 08, 2023 at 02:59:27PM +0100, K?ry Maincent wrote:
> +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;
> +
> + if (phy_has_hwtstamp(phydev))
> + ts_layer = PHY_TIMESTAMPING;
> + else if (ops->get_ts_info)
> + ts_layer = MAC_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 = PHY_TIMESTAMPING;
> +
> + if (ops->get_ts_info && !strcmp(s, "mac"))
> + ts_layer = MAC_TIMESTAMPING;
> +
> +out:
> + netdev->selected_timestamping_layer = ts_layer;
> +}
From previous discussions, I believe that a device tree property was
added in order to prevent perceived performance regressions when
timestamping support is added to a PHY driver, correct?
I have a dumb question: if updating the device trees is needed in order
to prevent these behavior changes, then how is the regression problem
addressed for those device trees which don't contain this new property
(all device trees)?
Hi K?ry,
I love your patch! Yet something to improve:
[auto build test ERROR on v6.2]
[cannot apply to robh/for-next horms-ipvs/master net/master net-next/master linus/master v6.3-rc1 next-20230309]
[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/20230308-220453
patch link: https://lore.kernel.org/r/20230308135936.761794-4-kory.maincent%40bootlin.com
patch subject: [PATCH v3 3/5] net: Let the active time stamping layer be selectable.
config: um-i386_defconfig (https://download.01.org/0day-ci/archive/20230309/[email protected]/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/d81a36f239360e7e3b9ca2633e52b3cb12205590
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/20230308-220453
git checkout d81a36f239360e7e3b9ca2633e52b3cb12205590
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=um SUBARCH=i386 olddefconfig
make W=1 O=build_dir ARCH=um SUBARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
/usr/bin/ld: warning: arch/x86/um/checksum_32.o: missing .note.GNU-stack section implies executable stack
/usr/bin/ld: NOTE: This behaviour is deprecated and will be removed in a future version of the linker
/usr/bin/ld: warning: .tmp_vmlinux.kallsyms1 has a LOAD segment with RWX permissions
/usr/bin/ld: net/core/dev_ioctl.o: in function `dev_hwtstamp_ioctl':
net/core/dev_ioctl.c:280: undefined reference to `phy_do_ioctl'
>> /usr/bin/ld: net/core/dev_ioctl.c:290: undefined reference to `phy_mii_ioctl'
collect2: error: ld returned 1 exit status
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Hi K?ry,
I love your patch! Perhaps something to improve:
[auto build test WARNING on v6.2]
[cannot apply to robh/for-next horms-ipvs/master net/master net-next/master linus/master v6.3-rc1 next-20230309]
[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/20230308-220453
patch link: https://lore.kernel.org/r/20230308135936.761794-4-kory.maincent%40bootlin.com
patch subject: [PATCH v3 3/5] net: Let the active time stamping layer be selectable.
config: x86_64-randconfig-a003 (https://download.01.org/0day-ci/archive/20230310/[email protected]/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
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/d81a36f239360e7e3b9ca2633e52b3cb12205590
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/20230308-220453
git checkout d81a36f239360e7e3b9ca2633e52b3cb12205590
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/net/phy/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> drivers/net/phy/phy_device.c:1384:6: warning: no previous prototype for function 'of_set_timestamp' [-Wmissing-prototypes]
void of_set_timestamp(struct net_device *netdev, struct phy_device *phydev)
^
drivers/net/phy/phy_device.c:1384:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
void of_set_timestamp(struct net_device *netdev, struct phy_device *phydev)
^
static
1 warning generated.
vim +/of_set_timestamp +1384 drivers/net/phy/phy_device.c
1383
> 1384 void of_set_timestamp(struct net_device *netdev, struct phy_device *phydev)
1385 {
1386 struct device_node *node = phydev->mdio.dev.of_node;
1387 const struct ethtool_ops *ops = netdev->ethtool_ops;
1388 const char *s;
1389 enum timestamping_layer ts_layer = 0;
1390
1391 if (phy_has_hwtstamp(phydev))
1392 ts_layer = PHY_TIMESTAMPING;
1393 else if (ops->get_ts_info)
1394 ts_layer = MAC_TIMESTAMPING;
1395
1396 if (of_property_read_string(node, "preferred-timestamp", &s))
1397 goto out;
1398
1399 if (!s)
1400 goto out;
1401
1402 if (phy_has_hwtstamp(phydev) && !strcmp(s, "phy"))
1403 ts_layer = PHY_TIMESTAMPING;
1404
1405 if (ops->get_ts_info && !strcmp(s, "mac"))
1406 ts_layer = MAC_TIMESTAMPING;
1407
1408 out:
1409 netdev->selected_timestamping_layer = ts_layer;
1410 }
1411
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Hello Vladimir,
On Thu, 9 Mar 2023 01:03:21 +0200
Vladimir Oltean <[email protected]> wrote:
> (trimmed CC list of bouncing email addresses)
Thanks, I will be more careful on next patch series version.
> From previous discussions, I believe that a device tree property was
> added in order to prevent perceived performance regressions when
> timestamping support is added to a PHY driver, correct?
Yes, i.e. to select the default and better timestamp on a board.
> I have a dumb question: if updating the device trees is needed in order
> to prevent these behavior changes, then how is the regression problem
> addressed for those device trees which don't contain this new property
> (all device trees)?
On that case there is not really solution, but be aware that
CONFIG_PHY_TIMESTAMPING need to be activated to allow timestamping on the PHY.
Currently in mainline only few (3) defconfig have it enabled so it is really
not spread, maybe I could add more documentation to prevent further regression
issue when adding support of timestamp to a PHY driver.
Regards,
Köry
On Fri, Mar 10, 2023 at 11:48:52AM +0100, K?ry Maincent wrote:
> > From previous discussions, I believe that a device tree property was
> > added in order to prevent perceived performance regressions when
> > timestamping support is added to a PHY driver, correct?
>
> Yes, i.e. to select the default and better timestamp on a board.
Is there a way to unambiguously determine the "better" timestamping on a board?
Is it plausible that over time, when PTP timestamping matures and,
for example, MDIO devices get support for PTP_SYS_OFFSET_EXTENDED
(an attempt was here: https://lkml.org/lkml/2019/8/16/638), the
relationship between PTP clock qualities changes, and so does the
preference change?
> > I have a dumb question: if updating the device trees is needed in order
> > to prevent these behavior changes, then how is the regression problem
> > addressed for those device trees which don't contain this new property
> > (all device trees)?
>
> On that case there is not really solution,
If it's not really a solution, then doesn't this fail at its primary
purpose of preventing regressions?
> but be aware that CONFIG_PHY_TIMESTAMPING need to be activated to
> allow timestamping on the PHY. Currently in mainline only few (3)
> defconfig have it enabled so it is really not spread,
Do distribution kernels use the defconfigs from the kernel, or do they
just enable as many options that sound good as possible?
> maybe I could add more documentation to prevent further regression
> issue when adding support of timestamp to a PHY driver.
My opinion is that either the problem was not correctly identified,
or the proposed solution does not address that problem.
What I believe is the problem is that adding support for PHY timestamping
to a PHY driver will cause a behavior change for existing systems which
are deployed with that PHY.
If I had a multi-port NIC where all ports share the same PHC, I would
want to create a boundary clock with it. I can do that just fine when
using MAC timestamping. But assume someone adds support for PHY
timestamping and the kernel switches to using PHY timestamps by default.
Now I need to keep in sync the PHCs of the PHYs, something which was
implicit before (all ports shared the same PHC). I have done nothing
incorrectly, yet my deployment doesn't work anymore. This is just an
example. It doesn't sound like a good idea in general for new features
to cause a behavior change by default.
Having identified that as the problem, I guess the solution should be
to stop doing that (and even though a PHY driver supports timestamping,
keep using the MAC timestamping by default).
There is a slight inconvenience caused by the fact that there are
already PHY drivers using PHY timestamping, and those may have been
introduced into deployments with PHY timestamping. We cannot change the
default behavior for those either. There are 5 such PHY drivers today
(I've grepped for mii_timestamper in drivers/net/phy).
I would suggest that the kernel implements a short whitelist of 5
entries containing PHY driver names, which are compared against
netdev->phydev->drv->name (with the appropriate NULL pointer checks).
Matches will default to PHY timestamping. Otherwise, the new default
will be to keep the behavior as if PHY timestamping doesn't exist
(MAC still provides the timestamps), and the user needs to select the
PHY as the timestamping source explicitly.
Thoughts?
[+ Horatiu]
Am 2023-03-10 12:35, schrieb Vladimir Oltean:
> On Fri, Mar 10, 2023 at 11:48:52AM +0100, Köry Maincent wrote:
>> > From previous discussions, I believe that a device tree property was
>> > added in order to prevent perceived performance regressions when
>> > timestamping support is added to a PHY driver, correct?
>>
>> Yes, i.e. to select the default and better timestamp on a board.
>
> Is there a way to unambiguously determine the "better" timestamping on
> a board?
>
> Is it plausible that over time, when PTP timestamping matures and,
> for example, MDIO devices get support for PTP_SYS_OFFSET_EXTENDED
> (an attempt was here: https://lkml.org/lkml/2019/8/16/638), the
> relationship between PTP clock qualities changes, and so does the
> preference change?
>
>> > I have a dumb question: if updating the device trees is needed in order
>> > to prevent these behavior changes, then how is the regression problem
>> > addressed for those device trees which don't contain this new property
>> > (all device trees)?
>>
>> On that case there is not really solution,
>
> If it's not really a solution, then doesn't this fail at its primary
> purpose of preventing regressions?
>
>> but be aware that CONFIG_PHY_TIMESTAMPING need to be activated to
>> allow timestamping on the PHY. Currently in mainline only few (3)
>> defconfig have it enabled so it is really not spread,
>
> Do distribution kernels use the defconfigs from the kernel, or do they
> just enable as many options that sound good as possible?
>
>> maybe I could add more documentation to prevent further regression
>> issue when adding support of timestamp to a PHY driver.
>
> My opinion is that either the problem was not correctly identified,
> or the proposed solution does not address that problem.
>
> What I believe is the problem is that adding support for PHY
> timestamping
> to a PHY driver will cause a behavior change for existing systems which
> are deployed with that PHY.
>
> If I had a multi-port NIC where all ports share the same PHC, I would
> want to create a boundary clock with it. I can do that just fine when
> using MAC timestamping. But assume someone adds support for PHY
> timestamping and the kernel switches to using PHY timestamps by
> default.
> Now I need to keep in sync the PHCs of the PHYs, something which was
> implicit before (all ports shared the same PHC). I have done nothing
> incorrectly, yet my deployment doesn't work anymore. This is just an
> example. It doesn't sound like a good idea in general for new features
> to cause a behavior change by default.
>
> Having identified that as the problem, I guess the solution should be
> to stop doing that (and even though a PHY driver supports timestamping,
> keep using the MAC timestamping by default).
>
> There is a slight inconvenience caused by the fact that there are
> already PHY drivers using PHY timestamping, and those may have been
> introduced into deployments with PHY timestamping. We cannot change the
> default behavior for those either. There are 5 such PHY drivers today
> (I've grepped for mii_timestamper in drivers/net/phy).
>
> I would suggest that the kernel implements a short whitelist of 5
> entries containing PHY driver names, which are compared against
> netdev->phydev->drv->name (with the appropriate NULL pointer checks).
> Matches will default to PHY timestamping. Otherwise, the new default
> will be to keep the behavior as if PHY timestamping doesn't exist
> (MAC still provides the timestamps), and the user needs to select the
> PHY as the timestamping source explicitly.
>
> Thoughts?
While I agree in principle (I have suggested to make MAC timestamping
the default before), I see a problem with the recent LAN8814 PHY
timestamping support, which will likely be released with 6.3. That
would now switch the timestamping to PHY timestamping for our board
(arch/arm/boot/dts/lan966x-kontron-kswitch-d10-mmt-8g.dts). I could
argue that is a regression for our board iff NETWORK_PHY_TIMESTAMPING
is enabled. Honestly, I don't know how to proceed here and haven't
tried to replicate the regression due to limited time. Assuming,
that I can show it is a regression, what would be the solution then,
reverting the commit? Horatiu, any ideas?
I digress from the original problem a bit. But if there would be such
a whitelist, I'd propose that it won't contain the lan8814 driver.
Other than that, I guess I have to put some time into testing
before it's too late.
-michael
The 03/10/2023 13:15, Michael Walle wrote:
>
> [+ Horatiu]
>
> Am 2023-03-10 12:35, schrieb Vladimir Oltean:
> > On Fri, Mar 10, 2023 at 11:48:52AM +0100, Köry Maincent wrote:
> > > > From previous discussions, I believe that a device tree property was
> > > > added in order to prevent perceived performance regressions when
> > > > timestamping support is added to a PHY driver, correct?
> > >
> > > Yes, i.e. to select the default and better timestamp on a board.
> >
> > Is there a way to unambiguously determine the "better" timestamping on
> > a board?
> >
> > Is it plausible that over time, when PTP timestamping matures and,
> > for example, MDIO devices get support for PTP_SYS_OFFSET_EXTENDED
> > (an attempt was here: https://lkml.org/lkml/2019/8/16/638), the
> > relationship between PTP clock qualities changes, and so does the
> > preference change?
> >
> > > > I have a dumb question: if updating the device trees is needed in order
> > > > to prevent these behavior changes, then how is the regression problem
> > > > addressed for those device trees which don't contain this new property
> > > > (all device trees)?
> > >
> > > On that case there is not really solution,
> >
> > If it's not really a solution, then doesn't this fail at its primary
> > purpose of preventing regressions?
> >
> > > but be aware that CONFIG_PHY_TIMESTAMPING need to be activated to
> > > allow timestamping on the PHY. Currently in mainline only few (3)
> > > defconfig have it enabled so it is really not spread,
> >
> > Do distribution kernels use the defconfigs from the kernel, or do they
> > just enable as many options that sound good as possible?
> >
> > > maybe I could add more documentation to prevent further regression
> > > issue when adding support of timestamp to a PHY driver.
> >
> > My opinion is that either the problem was not correctly identified,
> > or the proposed solution does not address that problem.
> >
> > What I believe is the problem is that adding support for PHY
> > timestamping
> > to a PHY driver will cause a behavior change for existing systems which
> > are deployed with that PHY.
> >
> > If I had a multi-port NIC where all ports share the same PHC, I would
> > want to create a boundary clock with it. I can do that just fine when
> > using MAC timestamping. But assume someone adds support for PHY
> > timestamping and the kernel switches to using PHY timestamps by
> > default.
> > Now I need to keep in sync the PHCs of the PHYs, something which was
> > implicit before (all ports shared the same PHC). I have done nothing
> > incorrectly, yet my deployment doesn't work anymore. This is just an
> > example. It doesn't sound like a good idea in general for new features
> > to cause a behavior change by default.
> >
> > Having identified that as the problem, I guess the solution should be
> > to stop doing that (and even though a PHY driver supports timestamping,
> > keep using the MAC timestamping by default).
> >
> > There is a slight inconvenience caused by the fact that there are
> > already PHY drivers using PHY timestamping, and those may have been
> > introduced into deployments with PHY timestamping. We cannot change the
> > default behavior for those either. There are 5 such PHY drivers today
> > (I've grepped for mii_timestamper in drivers/net/phy).
> >
> > I would suggest that the kernel implements a short whitelist of 5
> > entries containing PHY driver names, which are compared against
> > netdev->phydev->drv->name (with the appropriate NULL pointer checks).
> > Matches will default to PHY timestamping. Otherwise, the new default
> > will be to keep the behavior as if PHY timestamping doesn't exist
> > (MAC still provides the timestamps), and the user needs to select the
> > PHY as the timestamping source explicitly.
> >
> > Thoughts?
>
> While I agree in principle (I have suggested to make MAC timestamping
> the default before), I see a problem with the recent LAN8814 PHY
> timestamping support, which will likely be released with 6.3. That
> would now switch the timestamping to PHY timestamping for our board
> (arch/arm/boot/dts/lan966x-kontron-kswitch-d10-mmt-8g.dts). I could
> argue that is a regression for our board iff NETWORK_PHY_TIMESTAMPING
> is enabled. Honestly, I don't know how to proceed here and haven't
> tried to replicate the regression due to limited time. Assuming,
> that I can show it is a regression, what would be the solution then,
> reverting the commit? Horatiu, any ideas?
I don't think reverting the commit is the best approach. Because this
will block adding any timestamp support to any of the existing PHYs.
Maybe a better solution is to enable or disable NETWORK_PHY_TIMESTAMPING
depending where you want to do the timestamp.
>
> I digress from the original problem a bit. But if there would be such
> a whitelist, I'd propose that it won't contain the lan8814 driver.
I don't have anything against having a whitelist the PHY driver names.
>
> Other than that, I guess I have to put some time into testing
> before it's too late.
I was thinking about another scenario (I am sorry if this was already
discussed).
Currently when setting up to do the timestamp, the MAC will check if the
PHY has timestamping support if that is the case the PHY will do the
timestamping. So in case the switch was supposed to be a TC then we had
to make sure that the HW was setting up some rules not to forward PTP
frames by HW but to copy these frames to CPU.
With this new implementation, this would not be possible anymore as the
MAC will not be notified when doing the timestamping in the PHY.
Does it mean that now the switch should allocate these rules at start
time?
>
> -michael
--
/Horatiu
>> > > > From previous discussions, I believe that a device tree property was
>> > > > added in order to prevent perceived performance regressions when
>> > > > timestamping support is added to a PHY driver, correct?
>> > >
>> > > Yes, i.e. to select the default and better timestamp on a board.
>> >
>> > Is there a way to unambiguously determine the "better" timestamping on
>> > a board?
>> >
>> > Is it plausible that over time, when PTP timestamping matures and,
>> > for example, MDIO devices get support for PTP_SYS_OFFSET_EXTENDED
>> > (an attempt was here: https://lkml.org/lkml/2019/8/16/638), the
>> > relationship between PTP clock qualities changes, and so does the
>> > preference change?
>> >
>> > > > I have a dumb question: if updating the device trees is needed in order
>> > > > to prevent these behavior changes, then how is the regression problem
>> > > > addressed for those device trees which don't contain this new property
>> > > > (all device trees)?
>> > >
>> > > On that case there is not really solution,
>> >
>> > If it's not really a solution, then doesn't this fail at its primary
>> > purpose of preventing regressions?
>> >
>> > > but be aware that CONFIG_PHY_TIMESTAMPING need to be activated to
>> > > allow timestamping on the PHY. Currently in mainline only few (3)
>> > > defconfig have it enabled so it is really not spread,
>> >
>> > Do distribution kernels use the defconfigs from the kernel, or do they
>> > just enable as many options that sound good as possible?
>> >
>> > > maybe I could add more documentation to prevent further regression
>> > > issue when adding support of timestamp to a PHY driver.
>> >
>> > My opinion is that either the problem was not correctly identified,
>> > or the proposed solution does not address that problem.
>> >
>> > What I believe is the problem is that adding support for PHY
>> > timestamping
>> > to a PHY driver will cause a behavior change for existing systems which
>> > are deployed with that PHY.
>> >
>> > If I had a multi-port NIC where all ports share the same PHC, I would
>> > want to create a boundary clock with it. I can do that just fine when
>> > using MAC timestamping. But assume someone adds support for PHY
>> > timestamping and the kernel switches to using PHY timestamps by
>> > default.
>> > Now I need to keep in sync the PHCs of the PHYs, something which was
>> > implicit before (all ports shared the same PHC). I have done nothing
>> > incorrectly, yet my deployment doesn't work anymore. This is just an
>> > example. It doesn't sound like a good idea in general for new features
>> > to cause a behavior change by default.
>> >
>> > Having identified that as the problem, I guess the solution should be
>> > to stop doing that (and even though a PHY driver supports timestamping,
>> > keep using the MAC timestamping by default).
>> >
>> > There is a slight inconvenience caused by the fact that there are
>> > already PHY drivers using PHY timestamping, and those may have been
>> > introduced into deployments with PHY timestamping. We cannot change the
>> > default behavior for those either. There are 5 such PHY drivers today
>> > (I've grepped for mii_timestamper in drivers/net/phy).
>> >
>> > I would suggest that the kernel implements a short whitelist of 5
>> > entries containing PHY driver names, which are compared against
>> > netdev->phydev->drv->name (with the appropriate NULL pointer checks).
>> > Matches will default to PHY timestamping. Otherwise, the new default
>> > will be to keep the behavior as if PHY timestamping doesn't exist
>> > (MAC still provides the timestamps), and the user needs to select the
>> > PHY as the timestamping source explicitly.
>> >
>> > Thoughts?
>>
>> While I agree in principle (I have suggested to make MAC timestamping
>> the default before), I see a problem with the recent LAN8814 PHY
>> timestamping support, which will likely be released with 6.3. That
>> would now switch the timestamping to PHY timestamping for our board
>> (arch/arm/boot/dts/lan966x-kontron-kswitch-d10-mmt-8g.dts). I could
>> argue that is a regression for our board iff NETWORK_PHY_TIMESTAMPING
>> is enabled. Honestly, I don't know how to proceed here and haven't
>> tried to replicate the regression due to limited time. Assuming,
>> that I can show it is a regression, what would be the solution then,
>> reverting the commit? Horatiu, any ideas?
>
> I don't think reverting the commit is the best approach.
I didn't expect any other answer from the author of the patch ;)
> Because this
> will block adding any timestamp support to any of the existing PHYs.
Right, but if I understand it correctly, that is what has happend to
the Marvell PHY PTP support
https://lore.kernel.org/netdev/Y%[email protected]/
(Or it was NAK'ed before it could even get in. Maybe I'm to blame here,
but I have just so much time to follow all the mainline development).
> Maybe a better solution is to enable or disable
> NETWORK_PHY_TIMESTAMPING
> depending where you want to do the timestamp.
No, that is not how this can work. I agree with Vladimir, that in
general, you have no control which kernel options are enabled, see
distros.
>> I digress from the original problem a bit. But if there would be such
>> a whitelist, I'd propose that it won't contain the lan8814 driver.
>
> I don't have anything against having a whitelist the PHY driver names.
Yeah, but my problem right now is, that if this discussion won't find
any good solution, the lan8814 phy timestamping will find it's way
into an official kernel and then it is really hard to undo things.
So, I'd really prefer to *first* have a discussion how to proceed
with the PHY timestamping and then add the lan8814 support, so
existing boards don't show a regressions.
-michael
On Fri, 10 Mar 2023 14:34:07 +0100
Michael Walle <[email protected]> wrote:
> >> > There is a slight inconvenience caused by the fact that there are
> >> > already PHY drivers using PHY timestamping, and those may have been
> >> > introduced into deployments with PHY timestamping. We cannot change the
> >> > default behavior for those either. There are 5 such PHY drivers today
> >> > (I've grepped for mii_timestamper in drivers/net/phy).
> >> >
> >> > I would suggest that the kernel implements a short whitelist of 5
> >> > entries containing PHY driver names, which are compared against
> >> > netdev->phydev->drv->name (with the appropriate NULL pointer checks).
> >> > Matches will default to PHY timestamping. Otherwise, the new default
> >> > will be to keep the behavior as if PHY timestamping doesn't exist
> >> > (MAC still provides the timestamps), and the user needs to select the
> >> > PHY as the timestamping source explicitly.
> >> >
> >> > Thoughts?
> >>
> >> While I agree in principle (I have suggested to make MAC timestamping
> >> the default before), I see a problem with the recent LAN8814 PHY
> >> timestamping support, which will likely be released with 6.3. That
> >> would now switch the timestamping to PHY timestamping for our board
> >> (arch/arm/boot/dts/lan966x-kontron-kswitch-d10-mmt-8g.dts). I could
> >> argue that is a regression for our board iff NETWORK_PHY_TIMESTAMPING
> >> is enabled. Honestly, I don't know how to proceed here and haven't
> >> tried to replicate the regression due to limited time. Assuming,
> >> that I can show it is a regression, what would be the solution then,
> >> reverting the commit? Horatiu, any ideas?
Adding this whitelist will add some PHY driver specific name in the phy API
core.
Will it be accepted? Is it not better to add a "legacy_default_timestamping"
boolean in the phy_device struct and set it for these 5 PHY drivers?
Then move on the default behavior to MAC default timestamping on the otehr
cases.
> >> I digress from the original problem a bit. But if there would be such
> >> a whitelist, I'd propose that it won't contain the lan8814 driver.
> >
> > I don't have anything against having a whitelist the PHY driver names.
>
> Yeah, but my problem right now is, that if this discussion won't find
> any good solution, the lan8814 phy timestamping will find it's way
> into an official kernel and then it is really hard to undo things.
Yes and we need to find a solution as the issue will raise at each new PHY PTP
support.
Köry
On Wed, 08 Mar 2023 10:28:51 -0500
Willem de Bruijn <[email protected]> wrote:
> >
> > + enum timestamping_layer selected_timestamping_layer;
> > +
>
> can perhaps be a single bit rather than an enum
I need at least two bits to be able to list the PTPs available.
Look at the ethtool_list_ptp function of the second patch.
> > + err = -ENODEV;
> > + WARN_ON(1);
>
> Please no WARN_ON on error cases that are known to be reachable
> and can be handled safely and reported to userspace.
Alright, thanks.
Köry
Köry Maincent wrote:
> On Wed, 08 Mar 2023 10:28:51 -0500
> Willem de Bruijn <[email protected]> wrote:
>
> > >
> > > + enum timestamping_layer selected_timestamping_layer;
> > > +
> >
> > can perhaps be a single bit rather than an enum
>
> I need at least two bits to be able to list the PTPs available.
> Look at the ethtool_list_ptp function of the second patch.
In the available bitmap, yes. Since there are only two options,
in the selected case, a single bit would suffice.
On Fri, Mar 10, 2023 at 03:04:36PM +0100, K?ry Maincent wrote:
> Adding this whitelist will add some PHY driver specific name in the phy API
> core.
> Will it be accepted? Is it not better to add a "legacy_default_timestamping"
> boolean in the phy_device struct and set it for these 5 PHY drivers?
> Then move on the default behavior to MAC default timestamping on the otehr
> cases.
This sounds okay to me. @Russell will that work for your board?
Thanks,
Richard
> Adding this whitelist will add some PHY driver specific name in the phy API
> core.
> Will it be accepted? Is it not better to add a "legacy_default_timestamping"
> boolean in the phy_device struct and set it for these 5 PHY drivers?
> Then move on the default behavior to MAC default timestamping on the otehr
> cases.
In the end, it is much the same thing. But if we really want this to
be legacy, not used by new drivers, putting it into the core will make
it harder for new drivers to set this bit and not get noticed during
review.
Andrew
On Fri, Mar 10, 2023 at 09:59:53AM -0500, Willem de Bruijn wrote:
> K?ry Maincent wrote:
> > On Wed, 08 Mar 2023 10:28:51 -0500
> > Willem de Bruijn <[email protected]> wrote:
> >
> > > >
> > > > + enum timestamping_layer selected_timestamping_layer;
> > > > +
> > >
> > > can perhaps be a single bit rather than an enum
> >
> > I need at least two bits to be able to list the PTPs available.
> > Look at the ethtool_list_ptp function of the second patch.
>
> In the available bitmap, yes. Since there are only two options,
> in the selected case, a single bit would suffice.
It was a bit tongue in cheek, but in an earlier thread discussing this
problem, i listed how there could be up to 7 time stampers on the path
from the RJ45 to the network stack.
We got into this problem by assuming there could only ever be one time
stamper. Lets try to avoid potential problems of assuming there can
only every be two time stampers by assuming there can be N stampers.
Andrew
On Fri, Mar 10, 2023 at 02:34:07PM +0100, Michael Walle wrote:
> Yeah, but my problem right now is, that if this discussion won't find
> any good solution, the lan8814 phy timestamping will find it's way
> into an official kernel and then it is really hard to undo things.
>
> So, I'd really prefer to *first* have a discussion how to proceed
> with the PHY timestamping and then add the lan8814 support, so
> existing boards don't show a regressions.
You don't mean LAN8814 but LAN8841, no?
For the former, PTP support was added in commit ece19502834d ("net: phy:
micrel: 1588 support for LAN8814 phy") - first present in v5.18.
For the latter, it was commit cafc3662ee3f ("net: micrel: Add PHC
support for lan8841"), and this one indeed is in the v6.3 release
candidates.
Assuming you can prove a regression, how about adding the PHY driver
whitelist *without* the lan8841 as a patch to net.git? (blaming commit
cafc3662ee3f ("net: micrel: Add PHC support for lan8841")).
Doing this will effectively deactivate lan8841 PHY timestamping without
reverting the code. Then, this PHY timestamping support could be
activated back in net-next, based on some sort of explicit UAPI call.
On Fri, Mar 10, 2023 at 02:15:29PM +0100, Horatiu Vultur wrote:
> I was thinking about another scenario (I am sorry if this was already
> discussed).
> Currently when setting up to do the timestamp, the MAC will check if the
> PHY has timestamping support if that is the case the PHY will do the
> timestamping. So in case the switch was supposed to be a TC then we had
> to make sure that the HW was setting up some rules not to forward PTP
> frames by HW but to copy these frames to CPU.
> With this new implementation, this would not be possible anymore as the
> MAC will not be notified when doing the timestamping in the PHY.
> Does it mean that now the switch should allocate these rules at start
> time?
I would say no (to the allocation of trapping rules at startup time).
It was argued before by people present in this thread that it should be
possible (and default behavior) for switches to forward PTP frames as if
they were PTP-unaware:
https://patchwork.ozlabs.org/project/netdev/patch/[email protected]/
But it raises a really good point about how much care a switch driver
needs to take, such that with PTP timestamping, it must trap but not
timestamp the PTP frames.
There is a huge amount of variability here today.
The ocelot driver would be broken with PHY timestamping, since it would
flood the PTP messages (it installs the traps only if it is responsible
for taking the timestamps too).
The lan966x driver is very fine-tuned to call lan966x_ptp_setup_traps()
regardless of what phy_has_hwtstamp() says.
The sparx5 driver doesn't even seem to install traps at all (unclear if
they are predefined in hardware or not).
I guess that we want something like lan966x to keep working, since it
sounds like it's making the sanest decision about what to do.
But, as you point out, with K?ry's/Richard's proposal, the MAC driver
will be bypassed when the selected timestamping layer is the PHY, and
that's a problem currently.
May I suggest the following? There was another RFC which proposed the
introduction of a netdev notifier when timestamping is turned on/off:
https://lore.kernel.org/netdev/[email protected]/
It didn't go beyond RFC status, because I started doing what Jakub
suggested (converting the raw ioctls handlers to NDOs) but quickly got
absolutely swamped into the whole mess.
If we have a notifier, then we can make switch drivers do things
differently. They can activate timestamping per se in the timestamping
NDO (which is only called when the MAC is the active timestamping layer),
and they can activate PTP traps in the netdev notifier (which is called
any time a timestamping status change takes place - the notifier info
should contain details about which net_device and timestamping layer
this is, for example).
It's just a proposal of how to create an alternative notification path
that doesn't disturb the goals of this patch set.
Am 2023-03-10 17:06, schrieb Vladimir Oltean:
> On Fri, Mar 10, 2023 at 02:34:07PM +0100, Michael Walle wrote:
>> Yeah, but my problem right now is, that if this discussion won't find
>> any good solution, the lan8814 phy timestamping will find it's way
>> into an official kernel and then it is really hard to undo things.
>>
>> So, I'd really prefer to *first* have a discussion how to proceed
>> with the PHY timestamping and then add the lan8814 support, so
>> existing boards don't show a regressions.
>
> You don't mean LAN8814 but LAN8841, no?
Ohh, I'm stupid. No, I mean the LAN8814 (Quad PHY).
> For the former, PTP support was added in commit ece19502834d ("net:
> phy:
> micrel: 1588 support for LAN8814 phy") - first present in v5.18.
Yeah and I remember.. there was some kind of issue with the PHY
latencies. Ok, looks like I'm screwed then. I wonder how Microchip
is doing it, because our board is almost an identical copy of the
reference system.
> For the latter, it was commit cafc3662ee3f ("net: micrel: Add PHC
> support for lan8841"), and this one indeed is in the v6.3 release
> candidates.
>
> Assuming you can prove a regression, how about adding the PHY driver
> whitelist *without* the lan8841 as a patch to net.git? (blaming commit
> cafc3662ee3f ("net: micrel: Add PHC support for lan8841")).
>
> Doing this will effectively deactivate lan8841 PHY timestamping without
> reverting the code. Then, this PHY timestamping support could be
> activated back in net-next, based on some sort of explicit UAPI call.
Sorry for the noise and any inconvenience,
-michael
The 03/10/2023 18:44, Vladimir Oltean wrote:
>
> On Fri, Mar 10, 2023 at 02:15:29PM +0100, Horatiu Vultur wrote:
> > I was thinking about another scenario (I am sorry if this was already
> > discussed).
> > Currently when setting up to do the timestamp, the MAC will check if the
> > PHY has timestamping support if that is the case the PHY will do the
> > timestamping. So in case the switch was supposed to be a TC then we had
> > to make sure that the HW was setting up some rules not to forward PTP
> > frames by HW but to copy these frames to CPU.
> > With this new implementation, this would not be possible anymore as the
> > MAC will not be notified when doing the timestamping in the PHY.
> > Does it mean that now the switch should allocate these rules at start
> > time?
>
> I would say no (to the allocation of trapping rules at startup time).
> It was argued before by people present in this thread that it should be
> possible (and default behavior) for switches to forward PTP frames as if
> they were PTP-unaware:
> https://patchwork.ozlabs.org/project/netdev/patch/[email protected]/
Thanks for the explanation!
>
> But it raises a really good point about how much care a switch driver
> needs to take, such that with PTP timestamping, it must trap but not
> timestamp the PTP frames.
>
> There is a huge amount of variability here today.
>
> The ocelot driver would be broken with PHY timestamping, since it would
> flood the PTP messages (it installs the traps only if it is responsible
> for taking the timestamps too).
>
> The lan966x driver is very fine-tuned to call lan966x_ptp_setup_traps()
> regardless of what phy_has_hwtstamp() says.
>
> The sparx5 driver doesn't even seem to install traps at all (unclear if
> they are predefined in hardware or not).
They are not predefined in HW, I have on my TODO list to add those
traps I just need to get the time to do this.
>
> I guess that we want something like lan966x to keep working, since it
> sounds like it's making the sanest decision about what to do.
>
> But, as you point out, with Köry's/Richard's proposal, the MAC driver
> will be bypassed when the selected timestamping layer is the PHY, and
> that's a problem currently.
>
> May I suggest the following? There was another RFC which proposed the
> introduction of a netdev notifier when timestamping is turned on/off:
> https://lore.kernel.org/netdev/[email protected]/
>
> It didn't go beyond RFC status, because I started doing what Jakub
> suggested (converting the raw ioctls handlers to NDOs) but quickly got
> absolutely swamped into the whole mess.
>
> If we have a notifier, then we can make switch drivers do things
> differently. They can activate timestamping per se in the timestamping
> NDO (which is only called when the MAC is the active timestamping layer),
> and they can activate PTP traps in the netdev notifier (which is called
> any time a timestamping status change takes place - the notifier info
> should contain details about which net_device and timestamping layer
> this is, for example).
>
> It's just a proposal of how to create an alternative notification path
> that doesn't disturb the goals of this patch set.
--
/Horatiu
On Fri, Mar 10, 2023 at 06:44:51PM +0200, Vladimir Oltean wrote:
> On Fri, Mar 10, 2023 at 02:15:29PM +0100, Horatiu Vultur wrote:
> > I was thinking about another scenario (I am sorry if this was already
> > discussed).
> > Currently when setting up to do the timestamp, the MAC will check if the
> > PHY has timestamping support if that is the case the PHY will do the
> > timestamping. So in case the switch was supposed to be a TC then we had
> > to make sure that the HW was setting up some rules not to forward PTP
> > frames by HW but to copy these frames to CPU.
> > With this new implementation, this would not be possible anymore as the
> > MAC will not be notified when doing the timestamping in the PHY.
> > Does it mean that now the switch should allocate these rules at start
> > time?
>
> I would say no (to the allocation of trapping rules at startup time).
> It was argued before by people present in this thread that it should be
> possible (and default behavior) for switches to forward PTP frames as if
> they were PTP-unaware:
> https://patchwork.ozlabs.org/project/netdev/patch/[email protected]/
>
> But it raises a really good point about how much care a switch driver
> needs to take, such that with PTP timestamping, it must trap but not
> timestamp the PTP frames.
>
> There is a huge amount of variability here today.
>
> The ocelot driver would be broken with PHY timestamping, since it would
> flood the PTP messages (it installs the traps only if it is responsible
> for taking the timestamps too).
>
> The lan966x driver is very fine-tuned to call lan966x_ptp_setup_traps()
> regardless of what phy_has_hwtstamp() says.
>
> The sparx5 driver doesn't even seem to install traps at all (unclear if
> they are predefined in hardware or not).
>
> I guess that we want something like lan966x to keep working, since it
> sounds like it's making the sanest decision about what to do.
>
> But, as you point out, with Köry's/Richard's proposal, the MAC driver
> will be bypassed when the selected timestamping layer is the PHY, and
> that's a problem currently.
>
> May I suggest the following? There was another RFC which proposed the
> introduction of a netdev notifier when timestamping is turned on/off:
> https://lore.kernel.org/netdev/[email protected]/
>
> It didn't go beyond RFC status, because I started doing what Jakub
> suggested (converting the raw ioctls handlers to NDOs) but quickly got
> absolutely swamped into the whole mess.
>
> If we have a notifier, then we can make switch drivers do things
> differently. They can activate timestamping per se in the timestamping
> NDO (which is only called when the MAC is the active timestamping layer),
> and they can activate PTP traps in the netdev notifier (which is called
> any time a timestamping status change takes place - the notifier info
> should contain details about which net_device and timestamping layer
> this is, for example).
>
> It's just a proposal of how to create an alternative notification path
> that doesn't disturb the goals of this patch set.
To make things even more complicated - I have a project where the DSA master
should be used for time stamping. Due to board specific limitations, we
are forced to use FEC PHC instead of dsa KSZ switch PHC. So, it is not
a choice between MAC and PHY, it is more the MAC before MAC and PHY.
PTP sync in this case will have more jitter, but it still good enough
for this project.
Currently I use quick hack to do so, but mainlinamble solution working for
most use cases will be nice.
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Mon, 13 Mar 2023 09:40:59 +0100
Oleksij Rempel <[email protected]> wrote:
> > But, as you point out, with Köry's/Richard's proposal, the MAC driver
> > will be bypassed when the selected timestamping layer is the PHY, and
> > that's a problem currently.
> >
> > May I suggest the following? There was another RFC which proposed the
> > introduction of a netdev notifier when timestamping is turned on/off:
> > https://lore.kernel.org/netdev/[email protected]/
> >
> > If we have a notifier, then we can make switch drivers do things
> > differently. They can activate timestamping per se in the timestamping
> > NDO (which is only called when the MAC is the active timestamping layer),
> > and they can activate PTP traps in the netdev notifier (which is called
> > any time a timestamping status change takes place - the notifier info
> > should contain details about which net_device and timestamping layer
> > this is, for example).
> >
> > It's just a proposal of how to create an alternative notification path
> > that doesn't disturb the goals of this patch set.
I will take a look at your patch but indeed adding a timestamp notifier could
be a good idea.
> To make things even more complicated - I have a project where the DSA master
> should be used for time stamping. Due to board specific limitations, we
> are forced to use FEC PHC instead of dsa KSZ switch PHC. So, it is not
> a choice between MAC and PHY, it is more the MAC before MAC and PHY.
> PTP sync in this case will have more jitter, but it still good enough
> for this project.
> Currently I use quick hack to do so, but mainlinamble solution working for
> most use cases will be nice.
In this case it is not a PHY/MAC PTP choice anymore but more a device
PTP choice which bring a lot more of complexity. Or maybe we could simply add a
"switch" timestamp layer later on. As Andrew said we will maybe increase the
number of timestamp layers in the future.
Regards,
Köry
On Mon, 13 Mar 2023 09:40:59 +0100
Oleksij Rempel <[email protected]> wrote:
> > It didn't go beyond RFC status, because I started doing what Jakub
> > suggested (converting the raw ioctls handlers to NDOs) but quickly got
> > absolutely swamped into the whole mess.
Was there any useful work that could be continued on managing timestamp through
NDOs. As it seem we will made some change to the timestamp API, maybe it is a
good time to also take care of this.
Köry
On Thu, Mar 16, 2023 at 04:09:20PM +0100, K?ry Maincent wrote:
> Was there any useful work that could be continued on managing timestamp through
> NDOs. As it seem we will made some change to the timestamp API, maybe it is a
> good time to also take care of this.
Not to my knowledge. Yes, I agree that it would be a good time to add an
NDO for hwtimestamping (while keeping the ioctl fallback), then
transitioning as many devices as we can, and removing the fallback when
the transition is complete.
On Fri, 17 Mar 2023 17:21:50 +0200 Vladimir Oltean wrote:
> On Thu, Mar 16, 2023 at 04:09:20PM +0100, Köry Maincent wrote:
> > Was there any useful work that could be continued on managing timestamp through
> > NDOs. As it seem we will made some change to the timestamp API, maybe it is a
> > good time to also take care of this.
>
> Not to my knowledge. Yes, I agree that it would be a good time to add an
> NDO for hwtimestamping (while keeping the ioctl fallback), then
> transitioning as many devices as we can, and removing the fallback when
> the transition is complete.
I believe Max was looking into it - hi Max! Did you make much
progress? Any code you could share to build on?
Jakub,
I started working on a patch introducing NDO functions for hw
timestamping, but unfortunately put it on hold.
Let me finish it and send it out for review.
On Fri, Mar 17, 2023 at 1:07 PM Jakub Kicinski <[email protected]> wrote:
>
> On Fri, 17 Mar 2023 17:21:50 +0200 Vladimir Oltean wrote:
> > On Thu, Mar 16, 2023 at 04:09:20PM +0100, Köry Maincent wrote:
> > > Was there any useful work that could be continued on managing timestamp through
> > > NDOs. As it seem we will made some change to the timestamp API, maybe it is a
> > > good time to also take care of this.
> >
> > Not to my knowledge. Yes, I agree that it would be a good time to add an
> > NDO for hwtimestamping (while keeping the ioctl fallback), then
> > transitioning as many devices as we can, and removing the fallback when
> > the transition is complete.
>
> I believe Max was looking into it - hi Max! Did you make much
> progress? Any code you could share to build on?
On Fri, Mar 17, 2023 at 05:21:50PM +0200, Vladimir Oltean wrote:
> On Thu, Mar 16, 2023 at 04:09:20PM +0100, K?ry Maincent wrote:
> > Was there any useful work that could be continued on managing timestamp through
> > NDOs. As it seem we will made some change to the timestamp API, maybe it is a
> > good time to also take care of this.
>
> Not to my knowledge. Yes, I agree that it would be a good time to add an
> NDO for hwtimestamping (while keeping the ioctl fallback), then
> transitioning as many devices as we can, and removing the fallback when
> the transition is complete.
Um, user space ABI cannot be removed.
Thanks,
Richard
On Fri, 17 Mar 2023 20:38:49 -0700 Richard Cochran wrote:
> On Fri, Mar 17, 2023 at 05:21:50PM +0200, Vladimir Oltean wrote:
> > On Thu, Mar 16, 2023 at 04:09:20PM +0100, Köry Maincent wrote:
> > > Was there any useful work that could be continued on managing timestamp through
> > > NDOs. As it seem we will made some change to the timestamp API, maybe it is a
> > > good time to also take care of this.
> >
> > Not to my knowledge. Yes, I agree that it would be a good time to add an
> > NDO for hwtimestamping (while keeping the ioctl fallback), then
> > transitioning as many devices as we can, and removing the fallback when
> > the transition is complete.
>
> Um, user space ABI cannot be removed.
NDO meaning a dedicated callback in struct net_device_ops, so at least
for netdevs we can copy the data from user space, validate in the core
and then call the driver with a normal kernel pointer. So just an
internal refactoring, no uAPI changes.
On Fri, Mar 17, 2023 at 09:03:06PM -0700, Jakub Kicinski wrote:
> On Fri, 17 Mar 2023 20:38:49 -0700 Richard Cochran wrote:
> > On Fri, Mar 17, 2023 at 05:21:50PM +0200, Vladimir Oltean wrote:
> > > On Thu, Mar 16, 2023 at 04:09:20PM +0100, K?ry Maincent wrote:
> > > > Was there any useful work that could be continued on managing timestamp through
> > > > NDOs. As it seem we will made some change to the timestamp API, maybe it is a
> > > > good time to also take care of this.
> > >
> > > Not to my knowledge. Yes, I agree that it would be a good time to add an
> > > NDO for hwtimestamping (while keeping the ioctl fallback), then
> > > transitioning as many devices as we can, and removing the fallback when
> > > the transition is complete.
> >
> > Um, user space ABI cannot be removed.
>
> NDO meaning a dedicated callback in struct net_device_ops, so at least
> for netdevs we can copy the data from user space, validate in the core
> and then call the driver with a normal kernel pointer. So just an
> internal refactoring, no uAPI changes.
Yes, I was talking about the current handling via net_device_ops :: ndo_eth_ioctl()
(internal driver-facing kernel API) that should eventually get removed.
The new ndo_hwtstamp_get() and ndo_hwtstamp_set() should also have
slightly different (clearer) semantics IMO, like for example they should
only get called if the selected timestamping layer is the MAC. The MAC
driver would no longer be concerned with marshalling these calls down to
the PHY for PHY timestamping with this new API.
This is also the reason why the conversion can't be realistically done
all at once, because in some cases, as pointed out by Horatiu, simply
marshalling the ndo_eth_ioctl() to phy_mii_ioctl() isn't the only thing
that's necessary - sometimes the MAC driver may need to add filters or
traps for PTP frames itself, even if it doesn't provide the timestamps
per se. That will be solved not via the ndo_hwtstamp_set(), but via a
new (listen-only) NETDEV_HWTSTAMP_SET notifier, where interested drivers
can figure out that timestamping was enabled somewhere along the data
path of their netdev (not necessarily at their MAC layer) and program
those filters or traps accordingly, so that either MAC, or PHY,
timestamping works properly e.g. on a switch.
Also, the ndo_hwtstamp_get() and ndo_hwtstamp_set() API should not need
to explicitly call copy_from_user() and copy_to_user(), those are
especially error-prone w.r.t. their error code - non-zero means "bytes
left to copy IIRC", but -EFAULT should be returned to user space,
instead of blindly propagating what copy_from_user() has returned.
Hello everyone,
I'm sorry to intervene late in this discussion, but since it looks like
the discussion is converging towards the creation of a new API (though
NDOs internally, and through netlink for userspace), I'd like to add a
small other use-case to consider. Of course, this can be addressed
later on.
On Fri, 10 Mar 2023 13:35:33 +0200
Vladimir Oltean <[email protected]> wrote:
> On Fri, Mar 10, 2023 at 11:48:52AM +0100, Köry Maincent wrote:
> > > From previous discussions, I believe that a device tree property
> > > was added in order to prevent perceived performance regressions
> > > when timestamping support is added to a PHY driver, correct?
> >
> > Yes, i.e. to select the default and better timestamp on a board.
>
> Is there a way to unambiguously determine the "better" timestamping
> on a board?
I'd like to point out a series sent a while ago :
https://lore.kernel.org/netdev/[email protected]/
Here, the MAC's timestamping unit can be configured in 2 ways, which
boils down to either more accurate timestamps, or better accuracy in
frequency adjustments for the clock.
The need is to be able to change this mode at runtime, as we can change
the clock source for the timestamping unit to a very precise-one,
therefore using the "accurate timestamps mode" working as a
grand-master, or switching to slave mode, where we would sacrifice a
little bit the timestamping precision to get better frequency
precision.
So, we can consider here that not only the MAC's timestamping unit has
a non-fixed precision, but if we go through the route a a new API,
maybe we can also take this case into account, allowing for a bit of
configuration of the timestamping unit from userspace?
> Is it plausible that over time, when PTP timestamping matures and,
> for example, MDIO devices get support for PTP_SYS_OFFSET_EXTENDED
> (an attempt was here: https://lkml.org/lkml/2019/8/16/638), the
> relationship between PTP clock qualities changes, and so does the
> preference change?
I'm not exactly familiar with PTP_SYS_OFFSET_EXTENDED, but it looks a
bit similar in purpose to the above-mentionned use-case.
Thanks,
Maxime
Hello Max,
On Fri, 17 Mar 2023 13:43:34 -0600
Max Georgiev <[email protected]> wrote:
> Jakub,
>
> I started working on a patch introducing NDO functions for hw
> timestamping, but unfortunately put it on hold.
> Let me finish it and send it out for review.
What is your timeline for it? Do you think of sending it in the followings
weeks, months, years? If you don't have much time ask for help, I am not really
a PTP core expert but I would gladly work with you on this.
Köry
On Thu, 30 Mar 2023 14:38:24 +0200 Köry Maincent wrote:
> > I started working on a patch introducing NDO functions for hw
> > timestamping, but unfortunately put it on hold.
> > Let me finish it and send it out for review.
>
> What is your timeline for it? Do you think of sending it in the followings
> weeks, months, years? If you don't have much time ask for help, I am not really
> a PTP core expert but I would gladly work with you on this.
+1 Max, could you push what you have to GitHub or post as an RFC?
On Thu, Mar 30, 2023 at 10:26 AM Jakub Kicinski <[email protected]> wrote:
>
> On Thu, 30 Mar 2023 14:38:24 +0200 Köry Maincent wrote:
> > > I started working on a patch introducing NDO functions for hw
> > > timestamping, but unfortunately put it on hold.
> > > Let me finish it and send it out for review.
> >
> > What is your timeline for it? Do you think of sending it in the followings
> > weeks, months, years? If you don't have much time ask for help, I am not really
> > a PTP core expert but I would gladly work with you on this.
>
> +1 Max, could you push what you have to GitHub or post as an RFC?
I'm awfully sorry for the delay.
I've sent out what I had as an RFC to netdev list, the subject is
"[PATCH net-next RFC] Add NDOs for hardware timestamp get/set".
I'll continue working on testing the patch. Looking forward to
comments and suggestions.
On Thu, Mar 30, 2023 at 11:05 PM Max Georgiev <[email protected]> wrote:
>
> On Thu, Mar 30, 2023 at 10:26 AM Jakub Kicinski <[email protected]> wrote:
> >
> > On Thu, 30 Mar 2023 14:38:24 +0200 Köry Maincent wrote:
> > > > I started working on a patch introducing NDO functions for hw
> > > > timestamping, but unfortunately put it on hold.
> > > > Let me finish it and send it out for review.
> > >
> > > What is your timeline for it? Do you think of sending it in the followings
> > > weeks, months, years? If you don't have much time ask for help, I am not really
> > > a PTP core expert but I would gladly work with you on this.
> >
> > +1 Max, could you push what you have to GitHub or post as an RFC?
>
> I'm awfully sorry for the delay.
>
> I've sent out what I had as an RFC to netdev list, the subject is
> "[PATCH net-next RFC] Add NDOs for hardware timestamp get/set".
> I'll continue working on testing the patch. Looking forward to
> comments and suggestions.
Here is a link to the RFC patch:
https://lore.kernel.org/netdev/[email protected]/
On Thu, Mar 30, 2023 at 02:38:24PM +0200, K?ry Maincent wrote:
> Hello Max,
>
> On Fri, 17 Mar 2023 13:43:34 -0600
> Max Georgiev <[email protected]> wrote:
>
> > Jakub,
> >
> > I started working on a patch introducing NDO functions for hw
> > timestamping, but unfortunately put it on hold.
> > Let me finish it and send it out for review.
>
> What is your timeline for it? Do you think of sending it in the followings
> weeks, months, years? If you don't have much time ask for help, I am not really
> a PTP core expert but I would gladly work with you on this.
K?ry, I believe you can start looking at that PHY driver whitelist
(for changing the default timestamping layer) in parallel with Maxim's
ndo_hwtstamp_set() effort, since they shouldn't depend on each other?
Hello Maxime,
On Fri, Mar 24, 2023 at 11:25:41AM +0100, Maxime Chevallier wrote:
> I'd like to point out a series sent a while ago :
>
> https://lore.kernel.org/netdev/[email protected]/
>
> Here, the MAC's timestamping unit can be configured in 2 ways, which
> boils down to either more accurate timestamps, or better accuracy in
> frequency adjustments for the clock.
>
> The need is to be able to change this mode at runtime, as we can change
> the clock source for the timestamping unit to a very precise-one,
> therefore using the "accurate timestamps mode" working as a
> grand-master, or switching to slave mode, where we would sacrifice a
> little bit the timestamping precision to get better frequency
> precision.
>
> So, we can consider here that not only the MAC's timestamping unit has
> a non-fixed precision, but if we go through the route a a new API,
> maybe we can also take this case into account, allowing for a bit of
> configuration of the timestamping unit from userspace?
How would you suggest that this API looks like, what would be there to
configure on the timestamping unit? You're not looking for something
specific like "fine vs coarse" to be accepted, I hope?
Perhaps the stmmac is patched to expose 2 PHCs, one for fine mode and
one for coarse mode, and the timestamping PHC selection enables one more
or the other? In other words, if we expressed this stmmac specific thing
in vendor-agnostic terminology that we already understand, would that work?
The ability of a single MAC to register 2 PHCs might be useful for TSN
switches as well. Long story short, sometimes those expose a free
running clock (uncorrectable in offset and frequency), as well as a
correctable one, and they give the option for PTP hardware timestamps to
sample one clock or the other. TSN offloads (tc-taprio, tc-gate etc)
always use the correctable clock, and 802.1AS / gPTP has the option to
use the free running clock. I'm not interested in this personally, but
there were some talks about the value of doing this some time ago, and I
thought it would be worth mentioning it in this context, as something
else that could benefit from a more generic UAPI.
> > Is it plausible that over time, when PTP timestamping matures and,
> > for example, MDIO devices get support for PTP_SYS_OFFSET_EXTENDED
> > (an attempt was here: https://lkml.org/lkml/2019/8/16/638), the
> > relationship between PTP clock qualities changes, and so does the
> > preference change?
>
> I'm not exactly familiar with PTP_SYS_OFFSET_EXTENDED, but it looks a
> bit similar in purpose to the above-mentionned use-case.
Nope, not similar at all.
On Sun, 2 Apr 2023 20:12:49 +0300
Vladimir Oltean <[email protected]> wrote:
> On Thu, Mar 30, 2023 at 02:38:24PM +0200, Köry Maincent wrote:
> > Hello Max,
> >
> > On Fri, 17 Mar 2023 13:43:34 -0600
> > Max Georgiev <[email protected]> wrote:
> >
> > > Jakub,
> > >
> > > I started working on a patch introducing NDO functions for hw
> > > timestamping, but unfortunately put it on hold.
> > > Let me finish it and send it out for review.
> >
> > What is your timeline for it? Do you think of sending it in the followings
> > weeks, months, years? If you don't have much time ask for help, I am not
> > really a PTP core expert but I would gladly work with you on this.
>
> Köry, I believe you can start looking at that PHY driver whitelist
> (for changing the default timestamping layer) in parallel with Maxim's
> ndo_hwtstamp_set() effort, since they shouldn't depend on each other?
Yes, that's true. I will also update the change from ioctl to netlink to handle
the PTP layer selection.
Köry