[PATCH v12 net-next 0/3] Implement PTP support in netdevsim

[PATCH v12 net-next 0/3] Implement PTP support in netdevsim

[Maciek Machnikowski]

This patchset adds support to the PTP HW timestamping emulation in the
netdevsim. It uses existing binding between netdevsim and ptp_mock
driver to generate transmit and receive timestamps.

It also adds the selftest to verify the hw timestamping functionality
running over netdevsim.

v12:
- Unclone the skb before writing hwtstamp to not corrupt the TX errqueue clone
- Clear the TX hwtstamp when peer RX timestamping is off
- Set SKBTX_IN_PROGRESS before skb_tx_timestamp() to avoid SW+HW duplicates
- Use skb_unclone() instead of skb_unclone_full()
- Return -EOPNOTSUPP from nsim_get_ts_config() when there is no PHC
- Use isolated ptp4l UDS paths in the selftest
- Explicitly kill the remote ptp4l leader in the selftest
- Tolerate the deferred pkill finding no ptp4l process in the selftest

v11:
- Avoid aliasing the RX hwtstamp write with AF_XDP completion metadata
  (skb_shared_info's hwtstamps/xsk_meta union) by orphaning the skb
  first, addressing the Critical finding from Simon Horman's review.
  Link: https://lore.kernel.org/netdev/20260819101706.GP265046@horms.kernel.org/
- Only unshare the skb when an RX timestamp is written into it
- Reject non-zero hwtstamp flags and accept the PTP V2 L2 RX filters
- Use READ_ONCE() in ndo_hwtstamp_get and stop overwriting the whole
  config struct
- Skip the selftest if either the local or the remote device lacks
  hardware timestamping support
- Forward declare struct ptp_clock_info in ptp_mock.h
- Fix the ptp_mock commit message wording

v10:
- Drop the local_irq_save()/local_irq_restore() around the PHC reads.

v9:
- Unshare the skb instead of copying it
- Move the PTP timestamping off the xmit fast path
- Sample the TX and RX PHCs with IRQs disabled to keep the reads adjacent
- Validate the hwtstamp config before touching device state
- Report a -1 PHC index from ethtool for ports without a PHC (VFs)
- Clamp the ptp4l poll timeout in the selftest to a non-negative value

v8:
- Fix the lockless tstamp_config access in netdevsim with
  READ_ONCE()/WRITE_ONCE()
- Clarify the TX/RX timestamp handling in netdevsim comments

v7:
- Moved the test script to tools/testing/selftest/drivers/net
- Optimized implementation to avoid potential edge cases reported

v6:
- Enable PTP Mock in the tools/testing/selftests/net/config

v5:
- Rebase

v4:
- Check if Rx timestamps are enabled before generating a timestamp
- Replace bash selftest script with a python one
- Optimized Tx timestamp generation

v3:
- Fixed shellcheck issues in the selftest/net/ptp.sh
- Added selftest/net/ptp.sh to the selftest/net/Makefile
- Modified ptp_mock to use spin_lock_bh

v2:
- Added selftest/net/ptp.sh
- Modified ptp_mock to use spin_lock_bh
- Populate ethtool defaults using ethtool_op_get_ts_info

Maciek Machnikowski (3):
  ptp_mock: Expose ptp_clock_info to external drivers
  netdevsim: Implement basic ptp support
  selftests: drivers/net: Implement ptp4l sync test using netdevsim

 drivers/net/netdevsim/ethtool.c              |  15 +++
 drivers/net/netdevsim/netdev.c               | 124 +++++++++++++++++++
 drivers/net/netdevsim/netdevsim.h            |   1 +
 drivers/ptp/ptp_mock.c                       |  26 ++--
 include/linux/ptp_mock.h                     |   7 ++
 tools/testing/selftests/drivers/net/Makefile |   1 +
 tools/testing/selftests/drivers/net/config   |   1 +
 tools/testing/selftests/drivers/net/ptp.py   | 112 +++++++++++++++++
 8 files changed, 279 insertions(+), 8 deletions(-)
 create mode 100755 tools/testing/selftests/drivers/net/ptp.py

-- 
2.56.0

[PATCH v12 net-next 1/3] ptp_mock: Expose ptp_clock_info to external drivers

[Maciek Machnikowski]

Allow exposing the ptp_clock_info of the ptp_mock to the external drivers.
Use spin_lock_irqsave() in the PHC callbacks to allow gettime to be called
from the netdevsim transmit path.
This is a prerequisite for implementing ptp support on netdevsim.

Co-developed-by: Milena Olech <milena.olech@intel.com>
Signed-off-by: Milena Olech <milena.olech@intel.com>
Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
Reviewed-by: Mina Almasry <almasrymina@google.com>
Signed-off-by: Maciek Machnikowski <maciek@machnikowski.net>
---
 drivers/ptp/ptp_mock.c   | 26 ++++++++++++++++++--------
 include/linux/ptp_mock.h |  7 +++++++
 2 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/drivers/ptp/ptp_mock.c b/drivers/ptp/ptp_mock.c
index 4d66b6147121..7a4e5f3274a6 100644
--- a/drivers/ptp/ptp_mock.c
+++ b/drivers/ptp/ptp_mock.c
@@ -49,15 +49,16 @@ static u64 mock_phc_cc_read(struct cyclecounter *cc)
 static int mock_phc_adjfine(struct ptp_clock_info *info, long scaled_ppm)
 {
 	struct mock_phc *phc = info_to_phc(info);
+	unsigned long flags;
 	s64 adj;
 
 	adj = (s64)scaled_ppm << MOCK_PHC_FADJ_SHIFT;
 	adj = div_s64(adj, MOCK_PHC_FADJ_DENOMINATOR);
 
-	spin_lock(&phc->lock);
+	spin_lock_irqsave(&phc->lock, flags);
 	timecounter_read(&phc->tc);
 	phc->cc.mult = MOCK_PHC_CC_MULT + adj;
-	spin_unlock(&phc->lock);
+	spin_unlock_irqrestore(&phc->lock, flags);
 
 	return 0;
 }
@@ -65,10 +66,11 @@ static int mock_phc_adjfine(struct ptp_clock_info *info, long scaled_ppm)
 static int mock_phc_adjtime(struct ptp_clock_info *info, s64 delta)
 {
 	struct mock_phc *phc = info_to_phc(info);
+	unsigned long flags;
 
-	spin_lock(&phc->lock);
+	spin_lock_irqsave(&phc->lock, flags);
 	timecounter_adjtime(&phc->tc, delta);
-	spin_unlock(&phc->lock);
+	spin_unlock_irqrestore(&phc->lock, flags);
 
 	return 0;
 }
@@ -78,10 +80,11 @@ static int mock_phc_settime64(struct ptp_clock_info *info,
 {
 	struct mock_phc *phc = info_to_phc(info);
 	u64 ns = timespec64_to_ns(ts);
+	unsigned long flags;
 
-	spin_lock(&phc->lock);
+	spin_lock_irqsave(&phc->lock, flags);
 	timecounter_init(&phc->tc, &phc->cc, ns);
-	spin_unlock(&phc->lock);
+	spin_unlock_irqrestore(&phc->lock, flags);
 
 	return 0;
 }
@@ -89,11 +92,12 @@ static int mock_phc_settime64(struct ptp_clock_info *info,
 static int mock_phc_gettime64(struct ptp_clock_info *info, struct timespec64 *ts)
 {
 	struct mock_phc *phc = info_to_phc(info);
+	unsigned long flags;
 	u64 ns;
 
-	spin_lock(&phc->lock);
+	spin_lock_irqsave(&phc->lock, flags);
 	ns = timecounter_read(&phc->tc);
-	spin_unlock(&phc->lock);
+	spin_unlock_irqrestore(&phc->lock, flags);
 
 	*ts = ns_to_timespec64(ns);
 
@@ -171,5 +175,11 @@ void mock_phc_destroy(struct mock_phc *phc)
 }
 EXPORT_SYMBOL_GPL(mock_phc_destroy);
 
+struct ptp_clock_info *mock_phc_get_ptp_info(struct mock_phc *phc)
+{
+	return &phc->info;
+}
+EXPORT_SYMBOL_GPL(mock_phc_get_ptp_info);
+
 MODULE_DESCRIPTION("Mock-up PTP Hardware Clock driver");
 MODULE_LICENSE("GPL");
diff --git a/include/linux/ptp_mock.h b/include/linux/ptp_mock.h
index 72eb401034d9..8f5d248db65c 100644
--- a/include/linux/ptp_mock.h
+++ b/include/linux/ptp_mock.h
@@ -10,12 +10,14 @@
 
 struct device;
 struct mock_phc;
+struct ptp_clock_info;
 
 #if IS_ENABLED(CONFIG_PTP_1588_CLOCK_MOCK)
 
 struct mock_phc *mock_phc_create(struct device *dev);
 void mock_phc_destroy(struct mock_phc *phc);
 int mock_phc_index(struct mock_phc *phc);
+struct ptp_clock_info *mock_phc_get_ptp_info(struct mock_phc *phc);
 
 #else
 
@@ -33,6 +35,11 @@ static inline int mock_phc_index(struct mock_phc *phc)
 	return -1;
 }
 
+static inline struct ptp_clock_info *mock_phc_get_ptp_info(struct mock_phc *phc)
+{
+	return NULL;
+}
+
 #endif
 
 #endif /* _PTP_MOCK_H_ */
-- 
2.56.0

[PATCH v12 net-next 2/3] netdevsim: Implement basic ptp support

[Maciek Machnikowski]

Add support for virtual timestamping inside the netdevsim driver.
The implementation uses two attached ptp_mock clocks, reads the timestamps
of the ones attached either to the netdevsim or its peer and returns
timestamps using standard timestamps APIs.

This implementation enables running ptp4l on netdevsim adapters and
introduces a new ptp selftest.

skb_hwtstamps(skb)->hwtstamp aliases with skb_shinfo(skb)->xsk_meta in
the skb_shared_info union. For an AF_XDP generic-xmit skb forwarded to
a peer with RX timestamping enabled, writing the RX timestamp into
that union turns xsk_meta.tx_timestamp into a PHC-derived value that
the AF_XDP destructor (xsk_destruct_skb()) later dereferences as a
pointer, corrupting memory. Orphan the skb before writing the RX
timestamp so any pending destructor (e.g. the AF_XDP completion) runs
against the original, unmodified union contents first.

Co-developed-by: Milena Olech <milena.olech@intel.com>
Signed-off-by: Milena Olech <milena.olech@intel.com>
Reviewed-by: Mina Almasry <almasrymina@google.com>
Signed-off-by: Maciek Machnikowski <maciek@machnikowski.net>
---
 drivers/net/netdevsim/ethtool.c   |  15 ++++
 drivers/net/netdevsim/netdev.c    | 124 ++++++++++++++++++++++++++++++
 drivers/net/netdevsim/netdevsim.h |   1 +
 3 files changed, 140 insertions(+)

diff --git a/drivers/net/netdevsim/ethtool.c b/drivers/net/netdevsim/ethtool.c
index c3d2bc783d55..3f1f1510b6f8 100644
--- a/drivers/net/netdevsim/ethtool.c
+++ b/drivers/net/netdevsim/ethtool.c
@@ -200,7 +200,22 @@ static int nsim_get_ts_info(struct net_device *dev,
 {
 	struct netdevsim *ns = netdev_priv(dev);
 
+	ethtool_op_get_ts_info(dev, info);
+	if (!ns->phc) {
+		info->phc_index = -1;
+		return 0;
+	}
+
 	info->phc_index = mock_phc_index(ns->phc);
+	if (info->phc_index < 0)
+		return 0;
+
+	info->so_timestamping |= SOF_TIMESTAMPING_TX_HARDWARE |
+				 SOF_TIMESTAMPING_RX_HARDWARE |
+				 SOF_TIMESTAMPING_RAW_HARDWARE;
+
+	info->tx_types = BIT(HWTSTAMP_TX_OFF) | BIT(HWTSTAMP_TX_ON);
+	info->rx_filters = BIT(HWTSTAMP_FILTER_NONE) | BIT(HWTSTAMP_FILTER_ALL);
 
 	return 0;
 }
diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index 0372a9bd63bb..f098520bdb03 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -30,6 +30,8 @@
 #include <net/rtnetlink.h>
 #include <net/udp_tunnel.h>
 #include <net/busy_poll.h>
+#include <linux/ptp_clock_kernel.h>
+#include <linux/timecounter.h>
 
 #include "netdevsim.h"
 
@@ -122,7 +124,11 @@ static int nsim_forward_skb(struct net_device *tx_dev,
 
 static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
+	struct skb_shared_hwtstamps shhwtstamps = {};
+	struct ptp_clock_info *ptp_info_tx = NULL;
+	struct ptp_clock_info *ptp_info_rx = NULL;
 	struct netdevsim *ns = netdev_priv(dev);
+	struct timespec64 tx_ts, rx_ts;
 	struct skb_ext *psp_ext = NULL;
 	struct net_device *peer_dev;
 	unsigned int len = skb->len;
@@ -167,7 +173,53 @@ static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	      cfg->hds_thresh > len)))
 		skb_linearize(skb);
 
+	if (unlikely(READ_ONCE(peer_ns->tstamp_config.rx_filter) !=
+		     HWTSTAMP_FILTER_NONE))
+		ptp_info_rx = mock_phc_get_ptp_info(peer_ns->phc);
+
+	if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
+	    READ_ONCE(ns->tstamp_config.tx_type) == HWTSTAMP_TX_ON) {
+		ptp_info_tx = mock_phc_get_ptp_info(ns->phc);
+		if (ptp_info_tx)
+			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
+	}
+
 	skb_tx_timestamp(skb);
+
+	if (unlikely(ptp_info_tx || ptp_info_rx)) {
+		/* Capture both timestamps closely */
+		if (ptp_info_tx)
+			ptp_info_tx->gettime64(ptp_info_tx, &tx_ts);
+		if (ptp_info_rx)
+			ptp_info_rx->gettime64(ptp_info_rx, &rx_ts);
+
+		/* If TX hardware timestamping is enabled report TX timestamp */
+		if (ptp_info_tx) {
+			shhwtstamps.hwtstamp = timespec64_to_ktime(tx_ts);
+			skb_tstamp_tx(skb, &shhwtstamps);
+		}
+
+		/* Clones (e.g. the TX timestamp errqueue skb) share shinfo */
+		if (unlikely(skb_unclone(skb, GFP_ATOMIC))) {
+			kfree_skb(skb);
+			if (psp_ext)
+				__skb_ext_put(psp_ext);
+			goto out_drop_cnt;
+		}
+
+		if (ptp_info_rx) {
+			/* hwtstamp aliases xsk_meta; orphan first so any
+			 * pending AF_XDP completion isn't corrupted.
+			 */
+			skb_orphan(skb);
+			skb_hwtstamps(skb)->hwtstamp =
+				timespec64_to_ktime(rx_ts);
+		} else {
+			/* Don't leak the TX timestamp as an RX timestamp */
+			skb_hwtstamps(skb)->hwtstamp = 0;
+		}
+	}
+
 	if (unlikely(nsim_forward_skb(dev, peer_dev,
 				      skb, rq, psp_ext) == NET_RX_DROP))
 		goto out_drop_cnt;
@@ -197,6 +249,76 @@ static netdev_tx_t nsim_start_xmit_vf(struct sk_buff *skb,
 	return NETDEV_TX_OK;
 }
 
+static int nsim_set_ts_config(struct net_device *netdev,
+			      struct kernel_hwtstamp_config *config,
+			      struct netlink_ext_ack *extack)
+{
+	struct netdevsim *ns = netdev_priv(netdev);
+	int rx_filter;
+
+	if (!ns->phc)
+		return -EOPNOTSUPP;
+
+	if (config->flags) {
+		NL_SET_ERR_MSG_MOD(extack, "Flags are not supported");
+		return -EINVAL;
+	}
+
+	switch (config->rx_filter) {
+	case HWTSTAMP_FILTER_NONE:
+		rx_filter = HWTSTAMP_FILTER_NONE;
+		break;
+	case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
+	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
+	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
+	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
+	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
+	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
+	case HWTSTAMP_FILTER_NTP_ALL:
+	case HWTSTAMP_FILTER_ALL:
+		rx_filter = HWTSTAMP_FILTER_ALL;
+		break;
+	default:
+		return -ERANGE;
+	}
+
+	switch (config->tx_type) {
+	case HWTSTAMP_TX_OFF:
+		WRITE_ONCE(ns->tstamp_config.tx_type, HWTSTAMP_TX_OFF);
+		break;
+	case HWTSTAMP_TX_ON:
+		WRITE_ONCE(ns->tstamp_config.tx_type, HWTSTAMP_TX_ON);
+		break;
+	default:
+		return -ERANGE;
+	}
+
+	WRITE_ONCE(ns->tstamp_config.rx_filter, rx_filter);
+	config->rx_filter = rx_filter;
+
+	return 0;
+}
+
+static int nsim_get_ts_config(struct net_device *netdev,
+			      struct kernel_hwtstamp_config *config)
+{
+	struct netdevsim *ns = netdev_priv(netdev);
+
+	if (!ns->phc)
+		return -EOPNOTSUPP;
+
+	config->tx_type = READ_ONCE(ns->tstamp_config.tx_type);
+	config->rx_filter = READ_ONCE(ns->tstamp_config.rx_filter);
+
+	return 0;
+}
+
 static int nsim_set_rx_mode(struct net_device *dev,
 			    struct netdev_hw_addr_list *uc,
 			    struct netdev_hw_addr_list *mc)
@@ -668,6 +790,8 @@ static const struct net_device_ops nsim_netdev_ops = {
 	.ndo_vlan_rx_add_vid	= nsim_vlan_rx_add_vid,
 	.ndo_vlan_rx_kill_vid	= nsim_vlan_rx_kill_vid,
 	.net_shaper_ops		= &nsim_shaper_ops,
+	.ndo_hwtstamp_get	= nsim_get_ts_config,
+	.ndo_hwtstamp_set	= nsim_set_ts_config,
 };
 
 static const struct net_device_ops nsim_vf_netdev_ops = {
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index 2aa7ea924396..68a7aff0384a 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -111,6 +111,7 @@ struct netdevsim {
 	struct net_device *netdev;
 	struct nsim_dev *nsim_dev;
 	struct nsim_dev_port *nsim_dev_port;
+	struct kernel_hwtstamp_config tstamp_config;
 	struct mock_phc *phc;
 	struct nsim_rq **rq;
 
-- 
2.56.0

Re: [PATCH v12 net-next 2/3] netdevsim: Implement basic ptp support

[Vadim Fedorenko]

On 08/11/2026 20:15, Maciek Machnikowski wrote:
> Add support for virtual timestamping inside the netdevsim driver.
> The implementation uses two attached ptp_mock clocks, reads the timestamps
> of the ones attached either to the netdevsim or its peer and returns
> timestamps using standard timestamps APIs.
> 
> This implementation enables running ptp4l on netdevsim adapters and
> introduces a new ptp selftest.
> 
> skb_hwtstamps(skb)->hwtstamp aliases with skb_shinfo(skb)->xsk_meta in
> the skb_shared_info union. For an AF_XDP generic-xmit skb forwarded to
> a peer with RX timestamping enabled, writing the RX timestamp into
> that union turns xsk_meta.tx_timestamp into a PHC-derived value that
> the AF_XDP destructor (xsk_destruct_skb()) later dereferences as a
> pointer, corrupting memory. Orphan the skb before writing the RX
> timestamp so any pending destructor (e.g. the AF_XDP completion) runs
> against the original, unmodified union contents first.
> 
> Co-developed-by: Milena Olech <milena.olech@intel.com>
> Signed-off-by: Milena Olech <milena.olech@intel.com>
> Reviewed-by: Mina Almasry <almasrymina@google.com>
> Signed-off-by: Maciek Machnikowski <maciek@machnikowski.net>

Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

Re: [PATCH v12 net-next 2/3] netdevsim: Implement basic ptp support

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] nsim_start_xmit() (drivers/net/netdevsim/netdev.c) only
  overwrites or clears skb_hwtstamps(skb)->hwtstamp inside `if…

--

netdevsim: Implement basic ptp support

This adds virtual hardware timestamping to netdevsim. Each netdevsim
uses an attached ptp_mock clock. TX timestamps are reported through
skb_tstamp_tx(), and RX timestamps are written into the forwarded
skb's shinfo hwtstamps. The patch also adds ndo_hwtstamp_get/set and
updates ethtool ts_info.

> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
> index 0372a9bd63bbb..f098520bdb034 100644
> --- a/drivers/net/netdevsim/netdev.c
> +++ b/drivers/net/netdevsim/netdev.c

[ ... ]

> @@ -167,7 +173,53 @@ static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  	      cfg->hds_thresh > len)))
>  		skb_linearize(skb);
>  
> +	if (unlikely(READ_ONCE(peer_ns->tstamp_config.rx_filter) !=
> +		     HWTSTAMP_FILTER_NONE))
> +		ptp_info_rx = mock_phc_get_ptp_info(peer_ns->phc);
> +
> +	if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
> +	    READ_ONCE(ns->tstamp_config.tx_type) == HWTSTAMP_TX_ON) {
> +		ptp_info_tx = mock_phc_get_ptp_info(ns->phc);
> +		if (ptp_info_tx)
> +			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> +	}
> +
>  	skb_tx_timestamp(skb);
> +
> +	if (unlikely(ptp_info_tx || ptp_info_rx)) {
> +		/* Capture both timestamps closely */
> +		if (ptp_info_tx)
> +			ptp_info_tx->gettime64(ptp_info_tx, &tx_ts);
> +		if (ptp_info_rx)
> +			ptp_info_rx->gettime64(ptp_info_rx, &rx_ts);
> +
> +		/* If TX hardware timestamping is enabled report TX timestamp */
> +		if (ptp_info_tx) {
> +			shhwtstamps.hwtstamp = timespec64_to_ktime(tx_ts);
> +			skb_tstamp_tx(skb, &shhwtstamps);
> +		}
> +
> +		/* Clones (e.g. the TX timestamp errqueue skb) share shinfo */
> +		if (unlikely(skb_unclone(skb, GFP_ATOMIC))) {
> +			kfree_skb(skb);
> +			if (psp_ext)
> +				__skb_ext_put(psp_ext);
> +			goto out_drop_cnt;
> +		}
> +
> +		if (ptp_info_rx) {
> +			/* hwtstamp aliases xsk_meta; orphan first so any
> +			 * pending AF_XDP completion isn't corrupted.
> +			 */
> +			skb_orphan(skb);
> +			skb_hwtstamps(skb)->hwtstamp =
> +				timespec64_to_ktime(rx_ts);
> +		} else {
> +			/* Don't leak the TX timestamp as an RX timestamp */
> +			skb_hwtstamps(skb)->hwtstamp = 0;
> +		}
> +	}

[Severity: Medium]
Can the TX timestamp still leak out as an RX timestamp through the
retransmit queue skb?

In nsim_start_xmit(), skb_tstamp_tx() runs before skb_unclone(). If the
socket does not use SOF_TIMESTAMPING_OPT_TSONLY, __skb_tstamp_tx() clones
the skb and writes the hardware timestamp into the clone:

net/core/skbuff.c:__skb_tstamp_tx() {
    ...
	skb = skb_clone(orig_skb, GFP_ATOMIC);
    ...
	if (hwtstamps)
		*skb_hwtstamps(skb) = *hwtstamps;
    ...
}

That clone shares skb_shared_info with the skb that was passed to the
driver. For TCP, the driver's skb is itself a clone of the retransmit
queue skb. If it is still cloned at this point, tx_ts ends up in the rtx
queue skb's shinfo.

skb_unclone() and the hwtstamp = 0 branch only clean the private copy
that gets forwarded.

By default, the earlier skb_linearize() goes through pskb_expand_head()
and gives the skb a private shinfo. That call is skipped when the peer
has tcp-data-split enabled and hds_thresh <= len. The default hds_thresh
is 0. The path is also open when skb_linearize() fails, because its
return value is ignored.

A later retransmit can then skip the TX branch in one of two ways:

  - tcp_fragment() copies the stale hwtstamps into the head via
    skb_unclone_keeptruesize(), while tcp_fragment_tstamp() moves
    SKBTX_HW_TSTAMP to the tail fragment.
  - tx_type has been switched to HWTSTAMP_TX_OFF in the meantime.

If the peer also has rx_filter == HWTSTAMP_FILTER_NONE, this check is
false:

	if (unlikely(ptp_info_tx || ptp_info_rx)) {

So nothing touches hwtstamp before
nsim_forward_skb()->__dev_forward_skb()->skb_scrub_packet(), and none of
those clear the shinfo hwtstamps.

Would the peer's TCP socket then report the sender's old TX PHC time as
a hardware RX timestamp, even though RX timestamping is disabled on that
device?

A related problem existed before this patch and was not introduced by
it: an AF_XDP generic-xmit skb's xsk_meta.tx_timestamp pointer lives in
the same union. It is forwarded to the peer unchanged when neither
branch runs.

> +
>  	if (unlikely(nsim_forward_skb(dev, peer_dev,
>  				      skb, rq, psp_ext) == NET_RX_DROP))
>  		goto out_drop_cnt;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261108201503.85000-1-maciek%40machnikowski.net

Re: [PATCH v12 net-next 2/3] netdevsim: Implement basic ptp support

[Maciek Machnikowski]

On 09/11/2026 21:28, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] nsim_start_xmit() (drivers/net/netdevsim/netdev.c) only
>   overwrites or clears skb_hwtstamps(skb)->hwtstamp inside `if…A non-issue. We are not working with a real HW, so we have to cut some
corners to make it work. The most probable leaks are fixed, those are
hypothetical.

Thanks,
Maciek

Re: [PATCH v12 net-next 2/3] netdevsim: Implement basic ptp support

[Jakub Kicinski]

On Tue, 10 Nov 2026 21:46:55 +0100 Maciek Machnikowski wrote:
> > If you believe none of the issues are legitimate, please reply to each
> > issue of Medium severity or higher and explain why the AI is wrong.
> > If any of them are legitimate and you plan to respin, please let patchwork
> > know by including "pw-bot: cr" as a separate line at the end of your reply
> > (one such reply per series is enough).
> > 
> > - [Medium] nsim_start_xmit() (drivers/net/netdevsim/netdev.c) only
> >   overwrites or clears skb_hwtstamps(skb)->hwtstamp inside `if…A non-issue. We are not working with a real HW, so we have to cut some  
> corners to make it work. The most probable leaks are fixed, those are
> hypothetical.

It's about as hypothetical as testing this in SW is useful.
Please drop the netdevsim patches and just add the selftest under
drivers/net/hw

Re: [PATCH v12 net-next 2/3] netdevsim: Implement basic ptp support

[Jakub Kicinski]

On Sun,  8 Nov 2026 21:15:02 +0100 Maciek Machnikowski wrote:
> +	if (config->flags) {
> +		NL_SET_ERR_MSG_MOD(extack, "Flags are not supported");
> +		return -EINVAL;
> +	}

Why?

[PATCH v12 net-next 3/3] selftests: drivers/net: Implement ptp4l sync test using netdevsim

[Maciek Machnikowski]

Add PTP synchronization test using ptp4l and netdevsim.
The test uses the NetDrvEpEnv to link a local netdevsim
device to a remote endpoint, runs ptp4l as leader and follower
on the two ends, and waits for the follower to report the
synchronized state (s2).

Reviewed-by: Mina Almasry <almasrymina@google.com>
Signed-off-by: Maciek Machnikowski <maciek@machnikowski.net>
---
 tools/testing/selftests/drivers/net/Makefile |   1 +
 tools/testing/selftests/drivers/net/config   |   1 +
 tools/testing/selftests/drivers/net/ptp.py   | 112 +++++++++++++++++++
 3 files changed, 114 insertions(+)
 create mode 100755 tools/testing/selftests/drivers/net/ptp.py

diff --git a/tools/testing/selftests/drivers/net/Makefile b/tools/testing/selftests/drivers/net/Makefile
index 79c6f998e704..ed41a69ef6ad 100644
--- a/tools/testing/selftests/drivers/net/Makefile
+++ b/tools/testing/selftests/drivers/net/Makefile
@@ -23,6 +23,7 @@ TEST_PROGS := \
 	ping.py \
 	pppoe_gro.py \
 	psp.py \
+	ptp.py \
 	queues.py \
 	ring_reconfig.py \
 	rss_key.py \
diff --git a/tools/testing/selftests/drivers/net/config b/tools/testing/selftests/drivers/net/config
index 4838adf27fa1..30e69446d41d 100644
--- a/tools/testing/selftests/drivers/net/config
+++ b/tools/testing/selftests/drivers/net/config
@@ -21,6 +21,7 @@ CONFIG_NET_SCH_INGRESS=y
 CONFIG_NET_SCH_PRIO=m
 CONFIG_PPP=y
 CONFIG_PPPOE=y
+CONFIG_PTP_1588_CLOCK_MOCK=y
 CONFIG_TLS=y
 CONFIG_VLAN_8021Q=m
 CONFIG_XDP_SOCKETS=y
diff --git a/tools/testing/selftests/drivers/net/ptp.py b/tools/testing/selftests/drivers/net/ptp.py
new file mode 100755
index 000000000000..1065bd42ba53
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/ptp.py
@@ -0,0 +1,112 @@
+#!/usr/bin/env python3
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# By Maciek Machnikowski <maciek@machnikowski.net> (c) 2026
+
+"""
+Test suite for PTP sync using ptp4l.
+
+Start a ptp4l leader and follower and check that the follower locks onto the
+leader (state s2)
+"""
+
+import time
+
+from lib.py import (
+    KsftSkipEx,
+    NetDrvEpEnv,
+    bkg,
+    cmd,
+    defer,
+    fd_read_timeout,
+    ksft_exit,
+    ksft_pr,
+    ksft_run,
+    ksft_true,
+)
+
+PTP4L_SYNC_TIMEOUT = 40
+
+
+def _require_hw_timestamping(ifname, host=None):
+    """Skip the test unless ifname supports HW TX and RX timestamping."""
+    end = "Remote" if host else "Local"
+    ts_info = cmd(f"ethtool -T {ifname}", host=host).stdout
+    if "hardware-transmit" not in ts_info or "hardware-receive" not in ts_info:
+        raise KsftSkipEx(
+            f"{end} device does not support hardware timestamping")
+
+
+def _poll_follower_sync(follower, timeout):
+    """Read the follower stdout pipe until ptp4l reports sync state s2.
+
+    Returns a tuple (synced, output) where output is the text read so far.
+    The synchronized ptp4l log line looks like this:
+    ptp4l[18374.558]: master offset        844 s2 freq    +822 path delay       521
+    """
+    fd_file = follower.proc.stdout
+    fd = fd_file.fileno()
+    buf = b""
+    deadline = time.monotonic() + timeout
+    while time.monotonic() < deadline:
+        if b" s2 " in buf:
+            break
+        if follower.proc.poll() is not None:
+            chunk = fd_file.read()
+            if chunk:
+                buf += chunk
+            break
+        try:
+            remaining = deadline - time.monotonic()
+            buf += fd_read_timeout(fd, max(0, min(1, remaining)))
+        except TimeoutError:
+            continue
+    return b" s2 " in buf, buf.decode("utf-8", "replace")
+
+
+def ptp_sync_test(cfg):
+    """Verify ptp4l leader/follower synchronization reaches state s2."""
+    cfg.require_cmd("ptp4l", remote=True)
+    cfg.require_cmd("ethtool", remote=True)
+
+    _require_hw_timestamping(cfg.ifname)
+    _require_hw_timestamping(cfg.remote_ifname, host=cfg.remote)
+
+    local_dir = cmd("mktemp -d").stdout.strip()
+    remote_dir = cmd("mktemp -d", host=cfg.remote).stdout.strip()
+    defer(cmd, f"rm -rf {local_dir}")
+    defer(cmd, f"rm -rf {remote_dir}", host=cfg.remote)
+
+    leader_cmd = (f"ptp4l -i {cfg.remote_ifname} -m -2 "
+                  f"--uds_address {remote_dir}/ptp4l "
+                  f"--uds_ro_address {remote_dir}/ptp4lro")
+    follower_cmd = (f"ptp4l -i {cfg.ifname} -m -s -2 "
+                    f"--uds_address {local_dir}/ptp4l "
+                    f"--uds_ro_address {local_dir}/ptp4lro")
+
+    # bkg() can't reliably signal the remote ptp4l over SSH (no pty), so
+    # kill it by pattern explicitly in addition to the normal teardown.
+    # pkill exits 1 when ptp4l already terminated, so don't treat it as fatal.
+    defer(cmd, f'pkill -INT -f "{leader_cmd}"', host=cfg.remote, fail=False)
+
+    with bkg(leader_cmd, host=cfg.remote), \
+         bkg(follower_cmd) as follower:
+        synced, output = _poll_follower_sync(follower, PTP4L_SYNC_TIMEOUT)
+
+    if not synced:
+        ksft_pr(f"ptp4l follower did not reach locked state (s2) within "
+                f"{PTP4L_SYNC_TIMEOUT}s")
+        tail = output.strip().split("\n")[-10:]
+        ksft_pr("Follower log (last 10 lines): " + " | ".join(tail))
+    ksft_true(synced, "PTP sync timeout")
+
+
+def main():
+    """Run ksft tests."""
+    with NetDrvEpEnv(__file__) as cfg:
+        ksft_run([ptp_sync_test], args=(cfg, ))
+    ksft_exit()
+
+
+if __name__ == "__main__":
+    main()
-- 
2.56.0

Re: [PATCH v12 net-next 3/3] selftests: drivers/net: Implement ptp4l sync test using netdevsim

[Hangbin Liu]

On Sun, Nov 08, 2026 at 09:15:03PM +0100, Maciek Machnikowski wrote:
> Add PTP synchronization test using ptp4l and netdevsim.
> The test uses the NetDrvEpEnv to link a local netdevsim
> device to a remote endpoint, runs ptp4l as leader and follower
> on the two ends, and waits for the follower to report the
> synchronized state (s2).
> 
> Reviewed-by: Mina Almasry <almasrymina@google.com>
> Signed-off-by: Maciek Machnikowski <maciek@machnikowski.net>
> ---
>  tools/testing/selftests/drivers/net/Makefile |   1 +
>  tools/testing/selftests/drivers/net/config   |   1 +
>  tools/testing/selftests/drivers/net/ptp.py   | 112 +++++++++++++++++++
>  3 files changed, 114 insertions(+)
>  create mode 100755 tools/testing/selftests/drivers/net/ptp.py
> 
> diff --git a/tools/testing/selftests/drivers/net/Makefile b/tools/testing/selftests/drivers/net/Makefile
> index 79c6f998e704..ed41a69ef6ad 100644
> --- a/tools/testing/selftests/drivers/net/Makefile
> +++ b/tools/testing/selftests/drivers/net/Makefile
> @@ -23,6 +23,7 @@ TEST_PROGS := \
>  	ping.py \
>  	pppoe_gro.py \
>  	psp.py \
> +	ptp.py \
>  	queues.py \
>  	ring_reconfig.py \
>  	rss_key.py \
> diff --git a/tools/testing/selftests/drivers/net/config b/tools/testing/selftests/drivers/net/config
> index 4838adf27fa1..30e69446d41d 100644
> --- a/tools/testing/selftests/drivers/net/config
> +++ b/tools/testing/selftests/drivers/net/config
> @@ -21,6 +21,7 @@ CONFIG_NET_SCH_INGRESS=y
>  CONFIG_NET_SCH_PRIO=m
>  CONFIG_PPP=y
>  CONFIG_PPPOE=y
> +CONFIG_PTP_1588_CLOCK_MOCK=y
>  CONFIG_TLS=y
>  CONFIG_VLAN_8021Q=m
>  CONFIG_XDP_SOCKETS=y
> diff --git a/tools/testing/selftests/drivers/net/ptp.py b/tools/testing/selftests/drivers/net/ptp.py
> new file mode 100755
> index 000000000000..1065bd42ba53
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/ptp.py
> @@ -0,0 +1,112 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# By Maciek Machnikowski <maciek@machnikowski.net> (c) 2026
> +
> +"""
> +Test suite for PTP sync using ptp4l.
> +
> +Start a ptp4l leader and follower and check that the follower locks onto the
> +leader (state s2)
> +"""
> +
> +import time
> +
> +from lib.py import (
> +    KsftSkipEx,
> +    NetDrvEpEnv,
> +    bkg,
> +    cmd,
> +    defer,
> +    fd_read_timeout,
> +    ksft_exit,
> +    ksft_pr,
> +    ksft_run,
> +    ksft_true,
> +)
> +
> +PTP4L_SYNC_TIMEOUT = 40
> +
> +
> +def _require_hw_timestamping(ifname, host=None):
> +    """Skip the test unless ifname supports HW TX and RX timestamping."""
> +    end = "Remote" if host else "Local"
> +    ts_info = cmd(f"ethtool -T {ifname}", host=host).stdout
> +    if "hardware-transmit" not in ts_info or "hardware-receive" not in ts_info:
> +        raise KsftSkipEx(
> +            f"{end} device does not support hardware timestamping")
> +
> +
> +def _poll_follower_sync(follower, timeout):
> +    """Read the follower stdout pipe until ptp4l reports sync state s2.
> +
> +    Returns a tuple (synced, output) where output is the text read so far.
> +    The synchronized ptp4l log line looks like this:
> +    ptp4l[18374.558]: master offset        844 s2 freq    +822 path delay       521
> +    """
> +    fd_file = follower.proc.stdout
> +    fd = fd_file.fileno()
> +    buf = b""
> +    deadline = time.monotonic() + timeout
> +    while time.monotonic() < deadline:
> +        if b" s2 " in buf:
> +            break
> +        if follower.proc.poll() is not None:
> +            chunk = fd_file.read()
> +            if chunk:
> +                buf += chunk
> +            break
> +        try:
> +            remaining = deadline - time.monotonic()
> +            buf += fd_read_timeout(fd, max(0, min(1, remaining)))
> +        except TimeoutError:
> +            continue
> +    return b" s2 " in buf, buf.decode("utf-8", "replace")
> +
> +
> +def ptp_sync_test(cfg):
> +    """Verify ptp4l leader/follower synchronization reaches state s2."""
> +    cfg.require_cmd("ptp4l", remote=True)
> +    cfg.require_cmd("ethtool", remote=True)
> +
> +    _require_hw_timestamping(cfg.ifname)
> +    _require_hw_timestamping(cfg.remote_ifname, host=cfg.remote)
> +
> +    local_dir = cmd("mktemp -d").stdout.strip()
> +    remote_dir = cmd("mktemp -d", host=cfg.remote).stdout.strip()
> +    defer(cmd, f"rm -rf {local_dir}")
> +    defer(cmd, f"rm -rf {remote_dir}", host=cfg.remote)
> +
> +    leader_cmd = (f"ptp4l -i {cfg.remote_ifname} -m -2 "
> +                  f"--uds_address {remote_dir}/ptp4l "
> +                  f"--uds_ro_address {remote_dir}/ptp4lro")
> +    follower_cmd = (f"ptp4l -i {cfg.ifname} -m -s -2 "
> +                    f"--uds_address {local_dir}/ptp4l "
> +                    f"--uds_ro_address {local_dir}/ptp4lro")
> +
> +    # bkg() can't reliably signal the remote ptp4l over SSH (no pty), so
> +    # kill it by pattern explicitly in addition to the normal teardown.
> +    # pkill exits 1 when ptp4l already terminated, so don't treat it as fatal.
> +    defer(cmd, f'pkill -INT -f "{leader_cmd}"', host=cfg.remote, fail=False)
> +
> +    with bkg(leader_cmd, host=cfg.remote), \
> +         bkg(follower_cmd) as follower:
> +        synced, output = _poll_follower_sync(follower, PTP4L_SYNC_TIMEOUT)
> +
> +    if not synced:
> +        ksft_pr(f"ptp4l follower did not reach locked state (s2) within "
> +                f"{PTP4L_SYNC_TIMEOUT}s")
> +        tail = output.strip().split("\n")[-10:]
> +        ksft_pr("Follower log (last 10 lines): " + " | ".join(tail))
> +    ksft_true(synced, "PTP sync timeout")
> +
> +
> +def main():
> +    """Run ksft tests."""
> +    with NetDrvEpEnv(__file__) as cfg:
> +        ksft_run([ptp_sync_test], args=(cfg, ))
> +    ksft_exit()
> +
> +
> +if __name__ == "__main__":
> +    main()
> -- 
> 2.56.0
> 

Tested-by: Hangbin Liu <liuhangbin@kylinos.cn>
Reviewed-by: Hangbin Liu <liuhangbin@kylinos.cn>

Re: [PATCH v12 net-next 3/3] selftests: drivers/net: Implement ptp4l sync test using netdevsim

[Vadim Fedorenko]

On 08/11/2026 20:15, Maciek Machnikowski wrote:
> Add PTP synchronization test using ptp4l and netdevsim.
> The test uses the NetDrvEpEnv to link a local netdevsim
> device to a remote endpoint, runs ptp4l as leader and follower
> on the two ends, and waits for the follower to report the
> synchronized state (s2).
> 
> Reviewed-by: Mina Almasry <almasrymina@google.com>
> Signed-off-by: Maciek Machnikowski <maciek@machnikowski.net>

Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

Re: [PATCH v12 net-next 3/3] selftests: drivers/net: Implement ptp4l sync test using netdevsim

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 5

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] When ptp4l fails to start, ptp_sync_test() in
  tools/testing/selftests/drivers/net/ptp.py reports it as a sync…
- [Low] The test says it checks that 'the follower locks onto the
  leader'.
- [Low] In ptp_sync_test(), the local temp directory is created at line
  75 (`cmd("mktemp -d")`).
- [Low] Both ptp4l instances turn on hardware timestamping on their
  interfaces (tx_type=HWTSTAMP_TX_ON, a PTP rx_filter; netdevsim…
- [Low] _require_hw_timestamping() only checks `ethtool -T` for the
  'hardware-transmit' and 'hardware-receive' strings.
- [Low] Both ptp4l command lines in ptp_sync_test() use the default
  tx_timestamp_timeout (no --tx_timestamp_timeout and no config file).

--

selftests: drivers/net: Implement ptp4l sync test using netdevsim

This commit adds a ptp.py selftest. It uses NetDrvEpEnv to link a local
netdevsim device to a remote endpoint and runs ptp4l as leader and
follower on the two ends. The test passes once the follower reports
the synchronized state (s2).

> diff --git a/tools/testing/selftests/drivers/net/ptp.py b/tools/testing/selftests/drivers/net/ptp.py
> new file mode 100755
> index 0000000000000..1065bd42ba538
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/ptp.py

[ ... ]

> +def _require_hw_timestamping(ifname, host=None):
> +    """Skip the test unless ifname supports HW TX and RX timestamping."""
> +    end = "Remote" if host else "Local"
> +    ts_info = cmd(f"ethtool -T {ifname}", host=host).stdout
> +    if "hardware-transmit" not in ts_info or "hardware-receive" not in ts_info:

[Severity: Low]
This check only looks for the hardware-transmit and hardware-receive
strings. Should it also compare the PTP Hardware Clock index of the local
and remote interfaces?

With NetDrvEpEnv the remote can be a netns that holds a sibling port of
the same NIC. Some multi-port NICs share one PHC across all ports.

In that setup the follower ptp4l would discipline the same clock that the
leader uses as its time source. The offset would be close to zero by
construction, so the test could reach s2 without measuring sync between
two independent clocks.

netdevsim is not affected, because each port gets its own mock PHC.

> +        raise KsftSkipEx(
> +            f"{end} device does not support hardware timestamping")
> +
> +
> +def _poll_follower_sync(follower, timeout):

[ ... ]

> +        try:
> +            remaining = deadline - time.monotonic()
> +            buf += fd_read_timeout(fd, max(0, min(1, remaining)))
> +        except TimeoutError:
> +            continue
> +    return b" s2 " in buf, buf.decode("utf-8", "replace")

[Severity: Low]
Is finding the " s2 " substring enough to show that "the follower locks
onto the leader", as the module docstring says? The master offset printed
on the same log line is never checked.

With the default config (step_threshold=0), the PI servo moves from s0 to
s1 to s2 within a few Sync intervals. It then stays in s2 whatever the
offset is.

Both netdevsim mock PHCs also start at 0 on CLOCK_MONOTONIC_RAW:

drivers/ptp/ptp_mock.c:mock_phc_create() {
    ...
	timecounter_init(&phc->tc, &phc->cc, 0);
    ...
}

So the two clocks already agree to within milliseconds before the servo
does anything.

Would the test still pass if adjfine/adjtime/settime64 on the follower
PHC had no effect? One option is to require the reported offset to stay
below a threshold for a few samples.

> +
> +
> +def ptp_sync_test(cfg):
> +    """Verify ptp4l leader/follower synchronization reaches state s2."""
> +    cfg.require_cmd("ptp4l", remote=True)
> +    cfg.require_cmd("ethtool", remote=True)
> +
> +    _require_hw_timestamping(cfg.ifname)
> +    _require_hw_timestamping(cfg.remote_ifname, host=cfg.remote)
> +
> +    local_dir = cmd("mktemp -d").stdout.strip()
> +    remote_dir = cmd("mktemp -d", host=cfg.remote).stdout.strip()
> +    defer(cmd, f"rm -rf {local_dir}")
> +    defer(cmd, f"rm -rf {remote_dir}", host=cfg.remote)

[Severity: Low]
What happens to local_dir if the remote mktemp fails? cmd() defaults to
fail=True, so it raises CmdExitFailure before the local rm -rf is
deferred, and the local temp directory is left behind.

Registering each defer() right after its mktemp would avoid this.

> +
> +    leader_cmd = (f"ptp4l -i {cfg.remote_ifname} -m -2 "
> +                  f"--uds_address {remote_dir}/ptp4l "
> +                  f"--uds_ro_address {remote_dir}/ptp4lro")
> +    follower_cmd = (f"ptp4l -i {cfg.ifname} -m -s -2 "
> +                    f"--uds_address {local_dir}/ptp4l "
> +                    f"--uds_ro_address {local_dir}/ptp4lro")

[Severity: Low]
Both command lines use ptp4l's default tx_timestamp_timeout. netdevsim
delivers TX timestamps synchronously from nsim_start_xmit() via
skb_tstamp_tx(), so the default is enough there.

Some real NICs deliver TX timestamps later, from a PTP aux worker or a
FIFO polled on interrupt. Could that delay exceed the default, so that
ptp4l faults the port and never reaches s2?

The test supports real devices through NetDrvEpEnv, so this would show up
as a plain "PTP sync timeout" FAIL with no hint about the timestamp
timeout. One could argue such a timeout is a valid failure signal, but
the output would not point to it.

> +
> +    # bkg() can't reliably signal the remote ptp4l over SSH (no pty), so
> +    # kill it by pattern explicitly in addition to the normal teardown.
> +    # pkill exits 1 when ptp4l already terminated, so don't treat it as fatal.
> +    defer(cmd, f'pkill -INT -f "{leader_cmd}"', host=cfg.remote, fail=False)

[Severity: Low]
Both ptp4l instances turn on hardware timestamping on their interfaces.
netdevsim also upgrades the rx filter to HWTSTAMP_FILTER_ALL in
nsim_set_ts_config(). The follower additionally steps the local PHC and
changes its frequency.

The only cleanup registered here is the pkill and the temp dir removal.
Nothing saves or restores the original timestamping config.

With netdevsim the devices are torn down when NetDrvEpEnv exits, so
nothing persists. When the test runs on real NICs (NETIF/REMOTE_*), would
both interfaces be left with HW timestamping enabled, and the local PHC
left with the servo's last step and frequency?

hw/nic_timestamp.py saves orig_tscfg and restores it with
__set_hwtimestamp_config(cfg, orig_tscfg). Would something similar make
sense here?

> +
> +    with bkg(leader_cmd, host=cfg.remote), \
> +         bkg(follower_cmd) as follower:
> +        synced, output = _poll_follower_sync(follower, PTP4L_SYNC_TIMEOUT)

[Severity: Medium]
What happens here if either ptp4l fails to start?

If the follower exits early, _poll_follower_sync() sees that
follower.proc.poll() is not None. It reads what is left on stdout, breaks
out of the loop and returns synced=False. That is the same result as a
real 40s sync timeout.

The leader is never polled. A leader that dies at startup costs the full
PTP4L_SYNC_TIMEOUT and is also reported as a sync timeout.

Neither bkg() call passes fail=, so on exit cmd.process() is called with
terminate=True and fail=None:

tools/testing/selftests/net/lib/py/utils.py:cmd.process() {
    ...
        if fail is None:
            fail = not terminate
    ...
}

As a result, a non-zero exit status and the stderr of either ptp4l are
silently dropped. ptp4l writes option errors and its usage text to
stderr, so the follower log tail printed below would be empty or
unhelpful.

One concrete trigger is --uds_ro_address, which is passed to both
instances. If I recall correctly, this option first appeared in linuxptp
4.0, while cfg.require_cmd("ptp4l") only checks that the binary exists.

On a host with an older linuxptp (some distros still ship 3.1.x), would
both instances exit at once, and the test FAIL with no visible cause
instead of skipping?

Could the test detect an early exit of either process and print its
stderr, or skip when ptp4l rejects the options?

> +
> +    if not synced:
> +        ksft_pr(f"ptp4l follower did not reach locked state (s2) within "
> +                f"{PTP4L_SYNC_TIMEOUT}s")
> +        tail = output.strip().split("\n")[-10:]
> +        ksft_pr("Follower log (last 10 lines): " + " | ".join(tail))
> +    ksft_true(synced, "PTP sync timeout")

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261108201503.85000-1-maciek%40machnikowski.net

Re: [PATCH v12 net-next 3/3] selftests: drivers/net: Implement ptp4l sync test using netdevsim

[Maciek Machnikowski]

On 09/11/2026 21:28, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 6 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 5
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] When ptp4l fails to start, ptp_sync_test() in
>   tools/testing/selftests/drivers/net/ptp.py reports it as a sync…
We may improve that once we implement more advanced tests. For this
version I consider it a non-issue. If the ptp4l fails, the test fails too.

> - [Low] The test says it checks that 'the follower locks onto the
>   leader'.
This is as designed, s2 means they are relatively close which is enough
to prove the mechanism works.

> - [Low] In ptp_sync_test(), the local temp directory is created at line
>   75 (`cmd("mktemp -d")`).
Nit. May be improved later, but not a dealbreaker - worst that could
happen is we'll have a temp dir after running the test

> - [Low] Both ptp4l instances turn on hardware timestamping on their
>   interfaces (tx_type=HWTSTAMP_TX_ON, a PTP rx_filter; netdevsim…
That's how it always work. Next ptp4l instance will clean it anyway

> - [Low] _require_hw_timestamping() only checks `ethtool -T` for the
>   'hardware-transmit' and 'hardware-receive' strings.
This is only a safe check. Irrelevant

> - [Low] Both ptp4l command lines in ptp_sync_test() use the default
>   tx_timestamp_timeout (no --tx_timestamp_timeout and no config file).
That may be relevant if we ever plan using some non-compatible NICs in
the HW setup, but we won't enable all ptp4l arguments.