LinuxLists.cc - [PATCH v2 0/4] Up until now, there was no way to let the user select the layer at which time stamping occurs. The stack assumed that PHY time stamping is always preferred, but some MAC/PHY combinations were buggy.

2023-03-03 16:43:02

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

From: Kory Maincent <[email protected]>

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

- Patch 1 refactors get_ts_info copy/paste code.

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

- Patch 3 makes the layer selectable at run time.

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

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

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

.../ABI/testing/sysfs-class-net-timestamping | 20 ++++
drivers/net/bonding/bond_main.c | 14 +--
drivers/net/ethernet/freescale/fec_main.c | 23 ++--
drivers/net/ethernet/mscc/ocelot_net.c | 21 ++--
drivers/net/ethernet/ti/cpsw_priv.c | 12 +--
drivers/net/ethernet/ti/netcp_ethss.c | 26 +----
drivers/net/macvlan.c | 14 +--
drivers/net/phy/phy_device.c | 6 ++
include/linux/ethtool.h | 8 ++
include/linux/netdevice.h | 10 ++
net/8021q/vlan_dev.c | 15 +--
net/core/dev_ioctl.c | 44 +++++++-
net/core/net-sysfs.c | 102 ++++++++++++++++++
net/core/timestamping.c | 6 ++
net/ethtool/common.c | 24 ++++-
15 files changed, 248 insertions(+), 97 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-class-net-timestamping

--
2.25.1

2023-03-03 16:43:14

by Kory Maincent

[permalink] [raw]

Subject: [PATCH v2 1/4] net: ethtool: Refactor identical get_ts_info implementations.

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

2023-03-03 16:43:24

by Kory Maincent

[permalink] [raw]

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

From: Richard Cochran <[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 sysfs.

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

Signed-off-by: Richard Cochran <[email protected]>
Signed-off-by: Kory Maincent <[email protected]>
---

Notes:
Changes in v2:
- Move the introduction of selected_timestamping_layer variable in next
patch.

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

diff --git a/Documentation/ABI/testing/sysfs-class-net-timestamping b/Documentation/ABI/testing/sysfs-class-net-timestamping
new file mode 100644
index 000000000000..529c3a6eb607
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-net-timestamping
@@ -0,0 +1,17 @@
+What: /sys/class/net/<iface>/available_timestamping_providers
+Date: January 2022
+Contact: Richard Cochran <[email protected]>
+Description:
+ Enumerates the available providers for SO_TIMESTAMPING.
+ The possible values are:
+ - "mac" The MAC provides time stamping.
+ - "phy" The PHY or MII device provides time stamping.
+
+What: /sys/class/net/<iface>/current_timestamping_provider
+Date: January 2022
+Contact: Richard Cochran <[email protected]>
+Description:
+ Show the current SO_TIMESTAMPING provider.
+ The possible values are:
+ - "mac" The MAC provides time stamping.
+ - "phy" The PHY or MII device provides time stamping.
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 8409d41405df..26095634fb31 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -620,6 +620,64 @@ static ssize_t threaded_store(struct device *dev,
}
static DEVICE_ATTR_RW(threaded);

+static ssize_t available_timestamping_providers_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ const struct ethtool_ops *ops;
+ struct net_device *netdev;
+ struct phy_device *phydev;
+ int ret = 0;
+
+ netdev = to_net_dev(dev);
+ phydev = netdev->phydev;
+ ops = netdev->ethtool_ops;
+
+ if (!rtnl_trylock())
+ return restart_syscall();
+
+ ret += sprintf(buf, "%s\n", "mac");
+ buf += 4;
+
+ if (phy_has_tsinfo(phydev)) {
+ ret += sprintf(buf, "%s\n", "phy");
+ buf += 4;
+ }
+
+ rtnl_unlock();
+
+ return ret;
+}
+static DEVICE_ATTR_RO(available_timestamping_providers);
+
+static ssize_t current_timestamping_provider_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ const struct ethtool_ops *ops;
+ struct net_device *netdev;
+ struct phy_device *phydev;
+ int ret;
+
+ netdev = to_net_dev(dev);
+ phydev = netdev->phydev;
+ ops = netdev->ethtool_ops;
+
+ if (!rtnl_trylock())
+ return restart_syscall();
+
+ if (phy_has_tsinfo(phydev)) {
+ ret = sprintf(buf, "%s\n", "phy");
+ } else {
+ ret = sprintf(buf, "%s\n", "mac");
+ }
+
+ rtnl_unlock();
+
+ return ret;
+}
+static DEVICE_ATTR_RO(current_timestamping_provider);
+
static struct attribute *net_class_attrs[] __ro_after_init = {
&dev_attr_netdev_group.attr,
&dev_attr_type.attr,
@@ -653,6 +711,8 @@ static struct attribute *net_class_attrs[] __ro_after_init = {
&dev_attr_carrier_up_count.attr,
&dev_attr_carrier_down_count.attr,
&dev_attr_threaded.attr,
+ &dev_attr_available_timestamping_providers.attr,
+ &dev_attr_current_timestamping_provider.attr,
NULL,
};
ATTRIBUTE_GROUPS(net_class);
--
2.25.1

2023-03-03 16:43:37

by Kory Maincent

[permalink] [raw]

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

From: Richard Cochran <[email protected]>

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

Signed-off-by: Richard Cochran <[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.

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

diff --git a/Documentation/ABI/testing/sysfs-class-net-timestamping b/Documentation/ABI/testing/sysfs-class-net-timestamping
index 529c3a6eb607..6dfd59740cad 100644
--- a/Documentation/ABI/testing/sysfs-class-net-timestamping
+++ b/Documentation/ABI/testing/sysfs-class-net-timestamping
@@ -11,7 +11,10 @@ What: /sys/class/net/<iface>/current_timestamping_provider
Date: January 2022
Contact: Richard Cochran <[email protected]>
Description:
- Show the current SO_TIMESTAMPING provider.
+ Shows or sets the current SO_TIMESTAMPING provider.
+ When changing the value, some packets in the kernel
+ networking stack may still be delivered with time
+ stamps from the previous provider.
The possible values are:
- "mac" The MAC provides time stamping.
- "phy" The PHY or MII device provides time stamping.
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 8cff61dbc4b5..8dff0c6493b5 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1451,6 +1451,11 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,

phydev->phy_link_change = phy_link_change;
if (dev) {
+ if (phy_has_hwtstamp(phydev))
+ dev->selected_timestamping_layer = PHY_TIMESTAMPING;
+ else
+ dev->selected_timestamping_layer = MAC_TIMESTAMPING;
+
phydev->attached_dev = dev;
dev->phydev = phydev;

@@ -1762,6 +1767,7 @@ void phy_detach(struct phy_device *phydev)

phy_suspend(phydev);
if (dev) {
+ dev->selected_timestamping_layer = MAC_TIMESTAMPING;
phydev->attached_dev->phydev = NULL;
phydev->attached_dev = NULL;
}
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index ba2bd604359d..d8e9da2526f0 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1742,6 +1742,11 @@ enum netdev_ml_priv_type {
ML_PRIV_CAN,
};

+enum timestamping_layer {
+ MAC_TIMESTAMPING,
+ PHY_TIMESTAMPING,
+};
+
/**
* struct net_device - The DEVICE structure.
*
@@ -1981,6 +1986,9 @@ enum netdev_ml_priv_type {
*
* @threaded: napi threaded mode is enabled
*
+ * @selected_timestamping_layer: Tracks whether the MAC or the PHY
+ * performs packet time stamping.
+ *
* @net_notifier_list: List of per-net netdev notifier block
* that follow this device when it is moved
* to another network namespace.
@@ -2339,6 +2347,8 @@ struct net_device {
unsigned wol_enabled:1;
unsigned threaded:1;

+ enum timestamping_layer selected_timestamping_layer;
+
struct list_head net_notifier_list;

#if IS_ENABLED(CONFIG_MACSEC)
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index 7674bb9f3076..cc7cf2a542fb 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -262,6 +262,43 @@ static int dev_eth_ioctl(struct net_device *dev,
return err;
}

+static int dev_hwtstamp_ioctl(struct net_device *dev,
+ struct ifreq *ifr, unsigned int cmd)
+{
+ const struct net_device_ops *ops = dev->netdev_ops;
+ int err;
+
+ err = dsa_ndo_eth_ioctl(dev, ifr, cmd);
+ if (err == 0 || err != -EOPNOTSUPP)
+ return err;
+
+ if (!netif_device_present(dev))
+ return -ENODEV;
+
+ switch (dev->selected_timestamping_layer) {
+
+ case MAC_TIMESTAMPING:
+ if (ops->ndo_do_ioctl == phy_do_ioctl) {
+ /* Some drivers set .ndo_do_ioctl to phy_do_ioctl. */
+ err = -EOPNOTSUPP;
+ } else {
+ err = ops->ndo_eth_ioctl(dev, ifr, cmd);
+ }
+ break;
+
+ case PHY_TIMESTAMPING:
+ if (phy_has_hwtstamp(dev->phydev)) {
+ err = phy_mii_ioctl(dev->phydev, ifr, cmd);
+ } else {
+ err = -ENODEV;
+ WARN_ON(1);
+ }
+ break;
+ }
+
+ return err;
+}
+
static int dev_siocbond(struct net_device *dev,
struct ifreq *ifr, unsigned int cmd)
{
@@ -397,6 +434,9 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data,
return err;
fallthrough;

+ case SIOCGHWTSTAMP:
+ return dev_hwtstamp_ioctl(dev, ifr, cmd);
+
/*
* Unknown or private ioctl
*/
@@ -407,9 +447,7 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data,

if (cmd == SIOCGMIIPHY ||
cmd == SIOCGMIIREG ||
- cmd == SIOCSMIIREG ||
- cmd == SIOCSHWTSTAMP ||
- cmd == SIOCGHWTSTAMP) {
+ cmd == SIOCSMIIREG) {
err = dev_eth_ioctl(dev, ifr, cmd);
} else if (cmd == SIOCBONDENSLAVE ||
cmd == SIOCBONDRELEASE ||
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 26095634fb31..66079424b100 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -666,17 +666,59 @@ static ssize_t current_timestamping_provider_show(struct device *dev,
if (!rtnl_trylock())
return restart_syscall();

- if (phy_has_tsinfo(phydev)) {
- ret = sprintf(buf, "%s\n", "phy");
- } else {
+ switch (netdev->selected_timestamping_layer) {
+ case MAC_TIMESTAMPING:
ret = sprintf(buf, "%s\n", "mac");
+ break;
+ case PHY_TIMESTAMPING:
+ ret = sprintf(buf, "%s\n", "phy");
+ break;
}

rtnl_unlock();

return ret;
}
-static DEVICE_ATTR_RO(current_timestamping_provider);
+
+static ssize_t current_timestamping_provider_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t len)
+{
+ struct net_device *netdev = to_net_dev(dev);
+ struct net *net = dev_net(netdev);
+ enum timestamping_layer flavor;
+
+ if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
+ return -EPERM;
+
+ if (sysfs_streq(buf, "mac"))
+ flavor = MAC_TIMESTAMPING;
+ else if (sysfs_streq(buf, "phy"))
+ flavor = PHY_TIMESTAMPING;
+ else
+ return -EINVAL;
+
+ if (!rtnl_trylock())
+ return restart_syscall();
+
+ if (!dev_isalive(netdev))
+ goto out;
+
+ if (netdev->selected_timestamping_layer != flavor) {
+ const struct net_device_ops *ops = netdev->netdev_ops;
+ struct ifreq ifr = {0};
+
+ /* Disable time stamping in the current layer. */
+ if (netif_device_present(netdev) && ops->ndo_eth_ioctl)
+ ops->ndo_eth_ioctl(netdev, &ifr, SIOCSHWTSTAMP);
+
+ netdev->selected_timestamping_layer = flavor;
+ }
+out:
+ rtnl_unlock();
+ return len;
+}
+static DEVICE_ATTR_RW(current_timestamping_provider);

static struct attribute *net_class_attrs[] __ro_after_init = {
&dev_attr_netdev_group.attr,
diff --git a/net/core/timestamping.c b/net/core/timestamping.c
index 04840697fe79..31c3142787b7 100644
--- a/net/core/timestamping.c
+++ b/net/core/timestamping.c
@@ -28,6 +28,9 @@ void skb_clone_tx_timestamp(struct sk_buff *skb)
if (!skb->sk)
return;

+ if (skb->dev->selected_timestamping_layer != PHY_TIMESTAMPING)
+ return;
+
type = classify(skb);
if (type == PTP_CLASS_NONE)
return;
@@ -50,6 +53,9 @@ bool skb_defer_rx_timestamp(struct sk_buff *skb)
if (!skb->dev || !skb->dev->phydev || !skb->dev->phydev->mii_ts)
return false;

+ if (skb->dev->selected_timestamping_layer != PHY_TIMESTAMPING)
+ return false;
+
if (skb_headroom(skb) < ETH_HLEN)
return false;

diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index 64a7e05cf2c2..255170c9345a 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -548,10 +548,20 @@ int __ethtool_get_ts_info(struct net_device *dev, struct ethtool_ts_info *info)
memset(info, 0, sizeof(*info));
info->cmd = ETHTOOL_GET_TS_INFO;

- if (phy_has_tsinfo(phydev))
- return phy_ts_info(phydev, info);
- if (ops->get_ts_info)
- return ops->get_ts_info(dev, info);
+ switch (dev->selected_timestamping_layer) {
+
+ case MAC_TIMESTAMPING:
+ if (ops->get_ts_info)
+ return ops->get_ts_info(dev, info);
+ break;
+
+ case PHY_TIMESTAMPING:
+ if (phy_has_tsinfo(phydev)) {
+ return phy_ts_info(phydev, info);
+ }
+ WARN_ON(1);
+ return -ENODEV;
+ }

info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |
SOF_TIMESTAMPING_SOFTWARE;
--
2.25.1

2023-03-03 16:43:54

by Kory Maincent

[permalink] [raw]

Subject: [PATCH v2 4/4] net: fix up drivers WRT phy time stamping

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

2023-03-03 16:46:32

by Kory Maincent

[permalink] [raw]

Subject: Re: [PATCH v2 0/4] Up until now, there was no way to let the user select the layer at which time stamping occurs. The stack assumed that PHY time stamping is always preferred, but some MAC/PHY combinations were buggy.

On Fri, 3 Mar 2023 17:42:37 +0100
Köry Maincent <[email protected]> wrote:

Oops, sorry some bug in my cover letter subject.

Regards,
Köry

2023-03-03 18:13:53

by kernel test robot

[permalink] [raw]

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

2023-03-03 18:55:39

by kernel test robot

[permalink] [raw]

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

Hi K?ry,

I love your patch! Perhaps something to improve:

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

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

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All warnings (new ones prefixed by >>):

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

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

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

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

2023-03-03 23:52:25

by Jakub Kicinski

[permalink] [raw]

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

On Fri, 3 Mar 2023 17:42:39 +0100 Köry Maincent wrote:
> Time stamping on network packets may happen either in the MAC or in
> the PHY, but not both. In preparation for making the choice
> selectable, expose both the current and available layers via sysfs.

Ethtool, please, no sysfs.

2023-03-03 23:57:00

by Willem de Bruijn

[permalink] [raw]

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

Köry Maincent wrote:
> From: Richard Cochran <[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 sysfs.
>
> In accordance with the kernel implementation as it stands, the current
> layer will always read as "phy" when a PHY time stamping device is
> present. Future patches will allow changing the current layer
> administratively.
>
> Signed-off-by: Richard Cochran <[email protected]>
> Signed-off-by: Kory Maincent <[email protected]>
> ---
>
> Notes:
> Changes in v2:
> - Move the introduction of selected_timestamping_layer variable in next
> patch.
>
> .../ABI/testing/sysfs-class-net-timestamping | 17 ++++++
> net/core/net-sysfs.c | 60 +++++++++++++++++++
> 2 files changed, 77 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-class-net-timestamping
>
> diff --git a/Documentation/ABI/testing/sysfs-class-net-timestamping b/Documentation/ABI/testing/sysfs-class-net-timestamping
> new file mode 100644
> index 000000000000..529c3a6eb607
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-net-timestamping
> @@ -0,0 +1,17 @@
> +What: /sys/class/net/<iface>/available_timestamping_providers
> +Date: January 2022
> +Contact: Richard Cochran <[email protected]>
> +Description:
> + Enumerates the available providers for SO_TIMESTAMPING.
> + The possible values are:
> + - "mac" The MAC provides time stamping.
> + - "phy" The PHY or MII device provides time stamping.
> +
> +What: /sys/class/net/<iface>/current_timestamping_provider
> +Date: January 2022
> +Contact: Richard Cochran <[email protected]>
> +Description:
> + Show the current SO_TIMESTAMPING provider.
> + The possible values are:
> + - "mac" The MAC provides time stamping.
> + - "phy" The PHY or MII device provides time stamping.
> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> index 8409d41405df..26095634fb31 100644
> --- a/net/core/net-sysfs.c
> +++ b/net/core/net-sysfs.c
> @@ -620,6 +620,64 @@ static ssize_t threaded_store(struct device *dev,
> }
> static DEVICE_ATTR_RW(threaded);
>
> +static ssize_t available_timestamping_providers_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + const struct ethtool_ops *ops;
> + struct net_device *netdev;
> + struct phy_device *phydev;
> + int ret = 0;
> +
> + netdev = to_net_dev(dev);
> + phydev = netdev->phydev;
> + ops = netdev->ethtool_ops;
> +
> + if (!rtnl_trylock())
> + return restart_syscall();
> +
> + ret += sprintf(buf, "%s\n", "mac");
> + buf += 4;
> +

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

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

2023-03-03 23:59:24

by Willem de Bruijn

[permalink] [raw]

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

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

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

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

2023-03-04 03:07:44

by kernel test robot

[permalink] [raw]

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

Hi K?ry,

I love your patch! Yet something to improve:

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

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

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All errors (new ones prefixed by >>):

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

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

2023-03-04 15:04:48

by Andrew Lunn

[permalink] [raw]

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

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

Network drivers tend to fall into one of two classes.

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

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

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

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

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

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

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

Andrew

2023-03-04 15:43:59

by Andrew Lunn

[permalink] [raw]

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

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

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

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

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

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

Andrew

2023-03-04 16:17:04

by Russell King (Oracle)

[permalink] [raw]

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

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

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

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

Think of is as:

netdev ---- timestamping ---- PHC

since we can have:

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

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

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

Well, what we have today is:

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

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

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

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

2023-03-04 19:46:20

by Andrew Lunn

[permalink] [raw]

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

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

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

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

Andrew

2023-03-06 17:56:59

by Jakub Kicinski

[permalink] [raw]

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

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

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