[PATCH v4 net-next 0/8] net: dsa: netc: add PTP support for NETC switch

[PATCH v4 net-next 0/8] net: dsa: netc: add PTP support for NETC switch

[wei.fang]

From: Wei Fang <wei.fang@nxp.com>

This patch set adds PTP timestamping support to the NETC switch, so that
the switch ports can be used for IEEE 1588 PTP synchronization.

The NETC switch has no time registers of its own and instead shares the
PTP time of the NETC Timer, which is a separate PCIe function driven by
its own driver. The first 3 patches prepare the Timer driver for this
role: tidy up its 64-bit register access, drop an ineffective reset, and
export netc_timer_get_current_time() so the switch driver can read the
current PTP time across the driver boundary in a race-free way.

The switch redirects PTP frames to the CPU through the ingress port
filter table (IPFT). A few patches harden and simplify the existing IPFT
and host flood handling first, then enable the ingress port filtering
lookup by default so PTP filter rules take effect without extra software
tracking.

On top of that groundwork, the switch driver gains RX timestamping and
both flavours of TX timestamping. Timestamps are carried between the
hardware and the driver through dedicated switch tags: the ingress and
transmit timestamps ride in To_Host tags, and per-frame requests ride in
To_Port tags. Two-step TX matches a queued skb clone against the
timestamp response echoed back by the hardware. One-step TX defers the
Sync frame to a per-port work, where the driver reads a fresh PTP time
and writes it into the Sync frame's timestamp field, then programs the
single-step register and transmits the frame so the hardware can add the
residence time to the correction field on the wire.

Together these changes let standard user-space PTP stacks drive hardware
timestamping on the NETC switch ports.

---
v4 changes:
1. Patch 3: read the PTP time under a module-private timer list guarded
   by a spinlock instead of the per-device lock, so the getter is
   softirq-safe and races against Timer unbind are avoided.
2. Patch 4: initialize ipft_hf_eid in the common per-port init path and
   shorten the related comment.
3. Patch 5: drops the rollback. netc_port_set_host_flood() returns void,
   so the upper layer can't act on failure anyway; just deletes the old
   entry first, then adds the new one, logging an error if it fails.
   Commit message expanded to explain why the leak was harmless today
   but matters for future IPFT users (PTP trap, flow policing).
4. Patch 7: rename the tstamp handler/queue/helpers consistently, use
   64-bit jiffies for the timeout, track RX timestamp validity with an
   explicit flag, reject PTP v1 on TX, and check the tag type before
   dispatching the two-step response.
5. Patch 8: rework one-step Sync. Replace the reference-counted onestep
   context and its skb-destructor scheme with simple per-port state, and
   drive frame completion from the hardware response frame (To_Port
   subtype 3) that echoes the request ID, reusing the two-step ID
   allocator and timeout work.
6. Improve some commit messages.
v3 link: https://lore.kernel.org/imx/20260831082335.1184605-1-wei.fang@oss.nxp.com/
v2 link: https://lore.kernel.org/imx/20260808032146.2335723-1-wei.fang@oss.nxp.com/
v1 link: https://lore.kernel.org/imx/20260728104548.3301214-1-wei.fang@oss.nxp.com/
---

Wei Fang (8):
  ptp: netc: use ioread64_lo_hi/iowrite64_lo_hi for 64-bit register
    access
  ptp: netc: remove unnecessary pcie_flr() call in probe
  ptp: netc: export netc_timer_get_current_time() for cross-driver use
  net: dsa: netc: use entry ID instead of pointer to track host flood
    rule
  net: dsa: netc: check return value of ntmp_ipft_delete_entry()
  net: dsa: netc: enable ingress port filtering lookup by default
  net: dsa: netc: add PTP two-step timestamping support
  net: dsa: netc: add PTP one-step timestamping support

 drivers/net/dsa/netc/Kconfig          |   2 +
 drivers/net/dsa/netc/Makefile         |   3 +-
 drivers/net/dsa/netc/netc_main.c      | 187 ++++--
 drivers/net/dsa/netc/netc_platform.c  |   1 +
 drivers/net/dsa/netc/netc_ptp.c       | 849 ++++++++++++++++++++++++++
 drivers/net/dsa/netc/netc_switch.h    |  67 +-
 drivers/net/dsa/netc/netc_switch_hw.h |   5 +
 drivers/ptp/ptp_netc.c                | 136 +++--
 include/linux/dsa/tag_netc.h          |  38 ++
 include/linux/fsl/netc_global.h       |  10 +
 net/dsa/tag_netc.c                    | 228 ++++++-
 11 files changed, 1402 insertions(+), 124 deletions(-)
 create mode 100644 drivers/net/dsa/netc/netc_ptp.c

-- 
2.34.1

[PATCH v4 net-next 1/8] ptp: netc: use ioread64_lo_hi/iowrite64_lo_hi for 64-bit register access

[wei.fang]

From: Wei Fang <wei.fang@nxp.com>

Replace the open-coded 64-bit register read/write sequences with
ioread64_lo_hi() and iowrite64_lo_hi() helpers. Introduce two new macros
netc_timer_rd64() and netc_timer_wr64() that wrap these helpers and use
them throughout the driver. This reduces boilerplate and makes the intent
of each operation clearer.

The high-half register defines (NETC_TMR_*_H) are kept to document the
register map; they are not used directly since netc_timer_rd/wr64()
address the 64-bit register pair via the low-half offset, relying on the
hardware layout where H is always at L + 4.

Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 drivers/ptp/ptp_netc.c | 72 ++++++++++++------------------------------
 1 file changed, 21 insertions(+), 51 deletions(-)

diff --git a/drivers/ptp/ptp_netc.c b/drivers/ptp/ptp_netc.c
index 19ca99d80e95..376b3b0d1a38 100644
--- a/drivers/ptp/ptp_netc.c
+++ b/drivers/ptp/ptp_netc.c
@@ -127,6 +127,17 @@ struct netc_timer {
 
 #define netc_timer_rd(p, o)		netc_read((p)->base + (o))
 #define netc_timer_wr(p, o, v)		netc_write((p)->base + (o), v)
+
+/* The 64-bit timer registers consist of a low (L) and high (H) register pair.
+ * Hardware requires a strict access order: for writes, TMR_xxx_L must be
+ * written first, which latches the value into a shadow register; the write
+ * to TMR_xxx_H then atomically transfers both shadow registers into the live
+ * counter. For reads, TMR_xxx_L must be read first to capture a coherent
+ * snapshot. iowrite64_lo_hi() and ioread64_lo_hi() enforce this L-before-H
+ * ordering.
+ */
+#define netc_timer_rd64(p, o)		ioread64_lo_hi((p)->base + (o))
+#define netc_timer_wr64(p, o, v)	iowrite64_lo_hi(v, (p)->base + (o))
 #define ptp_to_netc_timer(ptp)		container_of((ptp), struct netc_timer, caps)
 
 static const char *const timer_clk_src[] = {
@@ -136,66 +147,28 @@ static const char *const timer_clk_src[] = {
 
 static void netc_timer_cnt_write(struct netc_timer *priv, u64 ns)
 {
-	u32 tmr_cnt_h = upper_32_bits(ns);
-	u32 tmr_cnt_l = lower_32_bits(ns);
-
-	/* Writes to the TMR_CNT_L register copies the written value
-	 * into the shadow TMR_CNT_L register. Writes to the TMR_CNT_H
-	 * register copies the values written into the shadow TMR_CNT_H
-	 * register. Contents of the shadow registers are copied into
-	 * the TMR_CNT_L and TMR_CNT_H registers following a write into
-	 * the TMR_CNT_H register. So the user must writes to TMR_CNT_L
-	 * register first. Other H/L registers should have the same
-	 * behavior.
-	 */
-	netc_timer_wr(priv, NETC_TMR_CNT_L, tmr_cnt_l);
-	netc_timer_wr(priv, NETC_TMR_CNT_H, tmr_cnt_h);
+	netc_timer_wr64(priv, NETC_TMR_CNT_L, ns);
 }
 
 static u64 netc_timer_offset_read(struct netc_timer *priv)
 {
-	u32 tmr_off_l, tmr_off_h;
-	u64 offset;
-
-	tmr_off_l = netc_timer_rd(priv, NETC_TMR_OFF_L);
-	tmr_off_h = netc_timer_rd(priv, NETC_TMR_OFF_H);
-	offset = (((u64)tmr_off_h) << 32) | tmr_off_l;
-
-	return offset;
+	return netc_timer_rd64(priv, NETC_TMR_OFF_L);
 }
 
 static void netc_timer_offset_write(struct netc_timer *priv, u64 offset)
 {
-	u32 tmr_off_h = upper_32_bits(offset);
-	u32 tmr_off_l = lower_32_bits(offset);
-
-	netc_timer_wr(priv, NETC_TMR_OFF_L, tmr_off_l);
-	netc_timer_wr(priv, NETC_TMR_OFF_H, tmr_off_h);
+	netc_timer_wr64(priv, NETC_TMR_OFF_L, offset);
 }
 
 static u64 netc_timer_cur_time_read(struct netc_timer *priv)
 {
-	u32 time_h, time_l;
-	u64 ns;
-
-	/* The user should read NETC_TMR_CUR_TIME_L first to
-	 * get correct current time.
-	 */
-	time_l = netc_timer_rd(priv, NETC_TMR_CUR_TIME_L);
-	time_h = netc_timer_rd(priv, NETC_TMR_CUR_TIME_H);
-	ns = (u64)time_h << 32 | time_l;
-
-	return ns;
+	return netc_timer_rd64(priv, NETC_TMR_CUR_TIME_L);
 }
 
 static void netc_timer_alarm_write(struct netc_timer *priv,
 				   u64 alarm, int index)
 {
-	u32 alarm_h = upper_32_bits(alarm);
-	u32 alarm_l = lower_32_bits(alarm);
-
-	netc_timer_wr(priv, NETC_TMR_ALARM_L(index), alarm_l);
-	netc_timer_wr(priv, NETC_TMR_ALARM_H(index), alarm_h);
+	netc_timer_wr64(priv, NETC_TMR_ALARM_L(index), alarm);
 }
 
 static u32 netc_timer_get_integral_period(struct netc_timer *priv)
@@ -500,22 +473,19 @@ static void netc_timer_handle_etts_event(struct netc_timer *priv, int index,
 					 bool update_event)
 {
 	struct ptp_clock_event event;
-	u32 etts_l = 0, etts_h = 0;
+	u64 etts = 0;
 
-	while (netc_timer_rd(priv, NETC_TMR_STAT) & TMR_STAT_ETS_VLD(index)) {
-		etts_l = netc_timer_rd(priv, NETC_TMR_ETTS_L(index));
-		etts_h = netc_timer_rd(priv, NETC_TMR_ETTS_H(index));
-	}
+	while (netc_timer_rd(priv, NETC_TMR_STAT) & TMR_STAT_ETS_VLD(index))
+		etts = netc_timer_rd64(priv, NETC_TMR_ETTS_L(index));
 
 	/* Invalid time stamp */
-	if (!etts_l && !etts_h)
+	if (!etts)
 		return;
 
 	if (update_event) {
 		event.type = PTP_CLOCK_EXTTS;
 		event.index = index;
-		event.timestamp = (u64)etts_h << 32;
-		event.timestamp |= etts_l;
+		event.timestamp = etts;
 		ptp_clock_event(priv->clock, &event);
 	}
 }
-- 
2.34.1

[PATCH v4 net-next 2/8] ptp: netc: remove unnecessary pcie_flr() call in probe

[wei.fang]

From: Wei Fang <wei.fang@nxp.com>

According to the NETC reference manual, function level reset is not
applicable to the timer as a supporting function. Remove the pcie_flr()
call from netc_timer_pci_probe() as it has no effect.

Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 drivers/ptp/ptp_netc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/ptp/ptp_netc.c b/drivers/ptp/ptp_netc.c
index 376b3b0d1a38..aa9be8e2a630 100644
--- a/drivers/ptp/ptp_netc.c
+++ b/drivers/ptp/ptp_netc.c
@@ -777,7 +777,6 @@ static int netc_timer_pci_probe(struct pci_dev *pdev)
 	if (!priv)
 		return -ENOMEM;
 
-	pcie_flr(pdev);
 	err = pci_enable_device_mem(pdev);
 	if (err)
 		return dev_err_probe(dev, err, "Failed to enable device\n");
-- 
2.34.1

[PATCH v4 net-next 3/8] ptp: netc: export netc_timer_get_current_time() for cross-driver use

[wei.fang]

From: Wei Fang <wei.fang@nxp.com>

The NETC Switch does not have its own time registers and must obtain
the current PTP time from the NETC Timer bound to it. As they are
separate PCIe functions with independent drivers, add
netc_timer_get_current_time() to the Timer driver and export it so the
Switch driver can call it.

The Timer may be unbound while the Switch is calling this function,
which would free priv and unmap the MMIO region under an in-flight
read. To keep priv valid for the whole read, track each probed Timer
in a module-private list keyed by its pci_dev, and protect the lookup
and the register read with a global spinlock. A list hit proves the
pci_dev is bound to this driver, so priv is valid. remove() deletes
the entry under the same lock before tearing the device down, so no
read can observe a stale priv.

Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 drivers/ptp/ptp_netc.c          | 63 ++++++++++++++++++++++++++++++++-
 include/linux/fsl/netc_global.h | 10 ++++++
 2 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/drivers/ptp/ptp_netc.c b/drivers/ptp/ptp_netc.c
index aa9be8e2a630..1211aa9a37d3 100644
--- a/drivers/ptp/ptp_netc.c
+++ b/drivers/ptp/ptp_netc.c
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
 /*
  * NXP NETC V4 Timer driver
- * Copyright 2025 NXP
+ * Copyright 2025-2026 NXP
  */
 
 #include <linux/bitfield.h>
@@ -123,8 +123,12 @@ struct netc_timer {
 	u8 fs_alarm_num;
 	u8 fs_alarm_bitmap;
 	struct netc_pp pp[NETC_TMR_FIPER_NUM]; /* periodic pulse */
+	struct list_head node;
 };
 
+static LIST_HEAD(netc_timer_list);
+static DEFINE_SPINLOCK(netc_timer_list_lock);
+
 #define netc_timer_rd(p, o)		netc_read((p)->base + (o))
 #define netc_timer_wr(p, o, v)		netc_write((p)->base + (o), v)
 
@@ -985,6 +989,10 @@ static int netc_timer_probe(struct pci_dev *pdev,
 
 	enable_irq(priv->irq);
 
+	spin_lock_bh(&netc_timer_list_lock);
+	list_add(&priv->node, &netc_timer_list);
+	spin_unlock_bh(&netc_timer_list_lock);
+
 	return 0;
 
 free_msix_irq:
@@ -999,6 +1007,10 @@ static void netc_timer_remove(struct pci_dev *pdev)
 {
 	struct netc_timer *priv = pci_get_drvdata(pdev);
 
+	spin_lock_bh(&netc_timer_list_lock);
+	list_del(&priv->node);
+	spin_unlock_bh(&netc_timer_list_lock);
+
 	disable_irq(priv->irq);
 	ptp_clock_unregister(priv->clock);
 	netc_timer_wr(priv, NETC_TMR_TEMASK, 0);
@@ -1021,5 +1033,54 @@ static struct pci_driver netc_timer_driver = {
 };
 module_pci_driver(netc_timer_driver);
 
+/**
+ * netc_timer_get_current_time - read the current PTP time from the NETC Timer
+ * @pdev: PCI device of the NETC Timer
+ * @ns: Output, the current PTP clock time in nanoseconds
+ *
+ * Read TMR_CUR_TIME from the NETC Timer bound to @pdev. The lookup and read
+ * run under netc_timer_list_lock, so the Timer cannot be unbound and its priv
+ * freed during the read.
+ *
+ * Context: Process or softirq context. Must not be called from hardirq.
+ *
+ * Return: 0 on success, -ENODEV if the Timer is not present (not yet probed
+ *         or already removed).
+ */
+int netc_timer_get_current_time(struct pci_dev *pdev, u64 *ns)
+{
+	struct netc_timer *priv = NULL;
+	struct netc_timer *tmp;
+	unsigned long flags;
+	int err = 0;
+
+	/* Serialize against driver unbind, so holding it here ensures that
+	 * priv remains valid for the entire duration of the register read.
+	 */
+	spin_lock_bh(&netc_timer_list_lock);
+
+	list_for_each_entry(tmp, &netc_timer_list, node) {
+		if (tmp->pdev == pdev) {
+			priv = tmp;
+			break;
+		}
+	}
+
+	if (!priv) {
+		err = -ENODEV;
+		goto netc_timer_list_unlock;
+	}
+
+	spin_lock_irqsave(&priv->lock, flags);
+	*ns = netc_timer_cur_time_read(priv);
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+netc_timer_list_unlock:
+	spin_unlock_bh(&netc_timer_list_lock);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(netc_timer_get_current_time);
+
 MODULE_DESCRIPTION("NXP NETC Timer PTP Driver");
 MODULE_LICENSE("Dual BSD/GPL");
diff --git a/include/linux/fsl/netc_global.h b/include/linux/fsl/netc_global.h
index 5b8ff528d369..d4a26c17f99a 100644
--- a/include/linux/fsl/netc_global.h
+++ b/include/linux/fsl/netc_global.h
@@ -6,6 +6,7 @@
 
 #include <linux/io.h>
 #include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/pci.h>
 
 static inline u32 netc_read(void __iomem *reg)
 {
@@ -22,4 +23,13 @@ static inline u64 netc_read64(void __iomem *reg)
 	return ioread64(reg);
 }
 
+#if IS_REACHABLE(CONFIG_PTP_NETC_V4_TIMER)
+int netc_timer_get_current_time(struct pci_dev *pdev, u64 *ns);
+#else
+static inline int netc_timer_get_current_time(struct pci_dev *pdev, u64 *ns)
+{
+	return -ENODEV;
+}
+#endif
+
 #endif
-- 
2.34.1

[PATCH v4 net-next 4/8] net: dsa: netc: use entry ID instead of pointer to track host flood rule

[wei.fang]

From: Wei Fang <wei.fang@nxp.com>

Replace the struct ipft_entry_data pointer in struct netc_port with a
plain u32 entry ID (ipft_hf_eid), using NTMP_NULL_ENTRY_ID as the
sentinel value. The ipft_entry_data allocation is now freed immediately
inside netc_port_add_host_flood_rule() after the hardware entry is
committed, so no heap memory survives beyond that function. As a result,
netc_free_host_flood_rules() is no longer needed and can be removed.

Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 drivers/net/dsa/netc/netc_main.c   | 62 +++++++++++-------------------
 drivers/net/dsa/netc/netc_switch.h |  2 +-
 2 files changed, 24 insertions(+), 40 deletions(-)

diff --git a/drivers/net/dsa/netc/netc_main.c b/drivers/net/dsa/netc/netc_main.c
index 77077352c1a5..c590931a4e1b 100644
--- a/drivers/net/dsa/netc/netc_main.c
+++ b/drivers/net/dsa/netc/netc_main.c
@@ -266,6 +266,11 @@ static int netc_init_all_ports(struct netc_switch *priv)
 
 		np->switch_priv = priv;
 		np->iobase = priv->regs.port + PORT_IOBASE(i);
+		/* The ipft_hf_eid is initialized to an invalid entry
+		 * ID because the host flood rule (IPFT entry) has not
+		 * been created.
+		 */
+		np->ipft_hf_eid = NTMP_NULL_ENTRY_ID;
 		netc_port_get_capability(np);
 		priv->ports[i] = np;
 	}
@@ -938,30 +943,12 @@ static void netc_destroy_all_lists(struct netc_switch *priv)
 	mutex_destroy(&priv->vft_lock);
 }
 
-static void netc_free_host_flood_rules(struct netc_switch *priv)
-{
-	struct dsa_port *dp;
-
-	dsa_switch_for_each_user_port(dp, priv->ds) {
-		struct netc_port *np = priv->ports[dp->index];
-
-		/* No need to clear the hardware IPFT entry. Because PCIe
-		 * FLR will be performed when the switch is re-registered,
-		 * it will reset hardware state. So only need to free the
-		 * memory to avoid memory leak.
-		 */
-		kfree(np->host_flood);
-		np->host_flood = NULL;
-	}
-}
-
 static void netc_teardown(struct dsa_switch *ds)
 {
 	struct netc_switch *priv = ds->priv;
 
 	disable_delayed_work_sync(&priv->fdbt_ageing_work);
 	netc_destroy_all_lists(priv);
-	netc_free_host_flood_rules(priv);
 	netc_free_ntmp_user(priv);
 }
 
@@ -1759,37 +1746,36 @@ static int netc_port_add_host_flood_rule(struct netc_port *np,
 	cfge->cfg = cpu_to_le32(cfg);
 
 	err = ntmp_ipft_add_entry(&priv->ntmp, host_flood);
-	if (err) {
-		kfree(host_flood);
-		return err;
-	}
+	if (err)
+		goto free_host_flood;
 
 	np->uc = uc;
 	np->mc = mc;
-	np->host_flood = host_flood;
+	np->ipft_hf_eid = host_flood->entry_id;
 	/* Enable ingress port filter table lookup */
 	netc_port_wr(np, NETC_PIPFCR, PIPFCR_EN);
 
-	return 0;
+free_host_flood:
+	kfree(host_flood);
+
+	return err;
 }
 
-static void netc_port_remove_host_flood(struct netc_port *np,
-					struct ipft_entry_data *host_flood)
+static void netc_port_remove_host_flood(struct netc_port *np, u32 entry_id)
 {
 	struct netc_switch *priv = np->switch_priv;
 	bool disable_host_flood = false;
 
-	if (!host_flood)
+	if (entry_id == NTMP_NULL_ENTRY_ID)
 		return;
 
-	if (np->host_flood == host_flood)
+	if (np->ipft_hf_eid == entry_id)
 		disable_host_flood = true;
 
-	ntmp_ipft_delete_entry(&priv->ntmp, host_flood->entry_id);
-	kfree(host_flood);
+	ntmp_ipft_delete_entry(&priv->ntmp, entry_id);
 
 	if (disable_host_flood) {
-		np->host_flood = NULL;
+		np->ipft_hf_eid = NTMP_NULL_ENTRY_ID;
 		np->uc = false;
 		np->mc = false;
 		netc_port_wr(np, NETC_PIPFCR, 0);
@@ -1800,7 +1786,7 @@ static void netc_port_set_host_flood(struct dsa_switch *ds, int port,
 				     bool uc, bool mc)
 {
 	struct netc_port *np = NETC_PORT(ds, port);
-	struct ipft_entry_data *old_host_flood;
+	u32 old_entry_id;
 
 	/* Do not add host flood rule to ingress port filter table when
 	 * the port has joined a bridge. Otherwise, the ingress frames
@@ -1808,7 +1794,7 @@ static void netc_port_set_host_flood(struct dsa_switch *ds, int port,
 	 * will be redirected directly to the CPU port.
 	 */
 	if (dsa_port_bridge_dev_get(np->dp)) {
-		netc_port_remove_host_flood(np, np->host_flood);
+		netc_port_remove_host_flood(np, np->ipft_hf_eid);
 
 		return;
 	}
@@ -1818,20 +1804,18 @@ static void netc_port_set_host_flood(struct dsa_switch *ds, int port,
 
 	/* IPFT does not support in-place updates to the KEYE element,
 	 * we need to add a new entry and then delete the old one. So
-	 * save the old entry first.
+	 * save the old entry ID first.
 	 */
-	old_host_flood = np->host_flood;
-	np->host_flood = NULL;
+	old_entry_id = np->ipft_hf_eid;
 
 	if (netc_port_add_host_flood_rule(np, uc, mc)) {
-		np->host_flood = old_host_flood;
 		dev_err(ds->dev, "Failed to add host flood rule on port %d\n",
 			port);
 		return;
 	}
 
 	/* Remove the old host flood entry */
-	netc_port_remove_host_flood(np, old_host_flood);
+	netc_port_remove_host_flood(np, old_entry_id);
 }
 
 static int netc_single_vlan_aware_bridge(struct dsa_switch *ds,
@@ -2020,7 +2004,7 @@ static int netc_port_bridge_join(struct dsa_switch *ds, int port,
 	netc_port_set_pvid(np, vlan_unaware_pvid);
 
 out:
-	netc_port_remove_host_flood(np, np->host_flood);
+	netc_port_remove_host_flood(np, np->ipft_hf_eid);
 
 	if (atomic_inc_return(&priv->br_cnt) == 1)
 		schedule_delayed_work(&priv->fdbt_ageing_work,
diff --git a/drivers/net/dsa/netc/netc_switch.h b/drivers/net/dsa/netc/netc_switch.h
index 305f2a92e2f9..aeb08e6eda09 100644
--- a/drivers/net/dsa/netc/netc_switch.h
+++ b/drivers/net/dsa/netc/netc_switch.h
@@ -84,7 +84,7 @@ struct netc_port {
 	u16 uc:1;
 	u16 mc:1;
 	u16 pvid;
-	struct ipft_entry_data *host_flood;
+	u32 ipft_hf_eid; /* Must be initialized to NTMP_NULL_ENTRY_ID */
 };
 
 struct netc_switch_regs {
-- 
2.34.1

[PATCH v4 net-next 5/8] net: dsa: netc: check return value of ntmp_ipft_delete_entry()

[wei.fang]

From: Wei Fang <wei.fang@nxp.com>

ntmp_ipft_delete_entry() may fail, but the driver ignored its return
value, so a failed deletion left the IPFT entry untracked and
impossible to clean up later. This is harmless today because the port
also disables its ingress port filter table lookup, so the leftover
entry is never matched. However, IPFT is going to serve other features,
such as trapping PTP packets and flow policing, for which disabling the
lookup of the whole port is no longer an option. Note that the operation
to disable IPFT lookup in netc_port_remove_host_flood() will be removed
in a subsequent patch.

Propagate the error to the callers and only update the driver state
once the entry has really been deleted. Since IPFT does not support
in-place updates to the KEYE element, netc_port_set_host_flood() now
deletes the old entry before adding the new one, instead of adding the
new entry first and deleting the old one afterwards. Otherwise, if the
deletion of the old entry failed, each port would have to track two or
more host flood rules, which complicates the logic for little benefit.

Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 drivers/net/dsa/netc/netc_main.c | 76 +++++++++++++++++++-------------
 1 file changed, 45 insertions(+), 31 deletions(-)

diff --git a/drivers/net/dsa/netc/netc_main.c b/drivers/net/dsa/netc/netc_main.c
index c590931a4e1b..082ed9405868 100644
--- a/drivers/net/dsa/netc/netc_main.c
+++ b/drivers/net/dsa/netc/netc_main.c
@@ -1706,14 +1706,8 @@ static int netc_port_add_host_flood_rule(struct netc_port *np,
 	u32 cfg;
 	int err;
 
-	if (!uc && !mc) {
-		/* Disable ingress port filter table lookup */
-		netc_port_wr(np, NETC_PIPFCR, 0);
-		np->uc = false;
-		np->mc = false;
-
+	if (!uc && !mc)
 		return 0;
-	}
 
 	host_flood = kzalloc_obj(*host_flood);
 	if (!host_flood)
@@ -1761,32 +1755,31 @@ static int netc_port_add_host_flood_rule(struct netc_port *np,
 	return err;
 }
 
-static void netc_port_remove_host_flood(struct netc_port *np, u32 entry_id)
+static int netc_port_remove_host_flood(struct netc_port *np)
 {
 	struct netc_switch *priv = np->switch_priv;
-	bool disable_host_flood = false;
+	u32 entry_id = np->ipft_hf_eid;
+	int err;
 
 	if (entry_id == NTMP_NULL_ENTRY_ID)
-		return;
+		return 0;
 
-	if (np->ipft_hf_eid == entry_id)
-		disable_host_flood = true;
+	err = ntmp_ipft_delete_entry(&priv->ntmp, entry_id);
+	if (err)
+		return err;
 
-	ntmp_ipft_delete_entry(&priv->ntmp, entry_id);
+	np->ipft_hf_eid = NTMP_NULL_ENTRY_ID;
+	np->uc = false;
+	np->mc = false;
+	netc_port_wr(np, NETC_PIPFCR, 0);
 
-	if (disable_host_flood) {
-		np->ipft_hf_eid = NTMP_NULL_ENTRY_ID;
-		np->uc = false;
-		np->mc = false;
-		netc_port_wr(np, NETC_PIPFCR, 0);
-	}
+	return 0;
 }
 
 static void netc_port_set_host_flood(struct dsa_switch *ds, int port,
 				     bool uc, bool mc)
 {
 	struct netc_port *np = NETC_PORT(ds, port);
-	u32 old_entry_id;
 
 	/* Do not add host flood rule to ingress port filter table when
 	 * the port has joined a bridge. Otherwise, the ingress frames
@@ -1794,7 +1787,12 @@ static void netc_port_set_host_flood(struct dsa_switch *ds, int port,
 	 * will be redirected directly to the CPU port.
 	 */
 	if (dsa_port_bridge_dev_get(np->dp)) {
-		netc_port_remove_host_flood(np, np->ipft_hf_eid);
+		if (!netc_port_remove_host_flood(np))
+			return;
+
+		dev_err(ds->dev,
+			"Failed to delete host flood rule on bridge port %d\n",
+			port);
 
 		return;
 	}
@@ -1803,19 +1801,24 @@ static void netc_port_set_host_flood(struct dsa_switch *ds, int port,
 		return;
 
 	/* IPFT does not support in-place updates to the KEYE element,
-	 * we need to add a new entry and then delete the old one. So
-	 * save the old entry ID first.
+	 * we need to delete the old one and then add the new rule. If
+	 * the deletion fails, return immediately.
 	 */
-	old_entry_id = np->ipft_hf_eid;
-
-	if (netc_port_add_host_flood_rule(np, uc, mc)) {
-		dev_err(ds->dev, "Failed to add host flood rule on port %d\n",
+	if (netc_port_remove_host_flood(np)) {
+		dev_err(ds->dev,
+			"Failed to delete old host flood rule on port %d\n",
 			port);
+
 		return;
 	}
 
-	/* Remove the old host flood entry */
-	netc_port_remove_host_flood(np, old_entry_id);
+	/* Restoring the previous configuration is pointless because
+	 * .port_set_host_flood() returns void, so the upper layer cannot
+	 * detect the error and the RX flags have changed.
+	 */
+	if (netc_port_add_host_flood_rule(np, uc, mc))
+		dev_err(ds->dev,
+			"Failed to add host flood rule on port %d\n", port);
 }
 
 static int netc_single_vlan_aware_bridge(struct dsa_switch *ds,
@@ -1980,6 +1983,8 @@ static int netc_port_bridge_join(struct dsa_switch *ds, int port,
 	struct netc_port *np = NETC_PORT(ds, port);
 	struct netc_switch *priv = ds->priv;
 	u16 vlan_unaware_pvid;
+	bool uc = np->uc;
+	bool mc = np->mc;
 	int err;
 
 	if (!bridge.num) {
@@ -1991,6 +1996,12 @@ static int netc_port_bridge_join(struct dsa_switch *ds, int port,
 	if (err)
 		return err;
 
+	err = netc_port_remove_host_flood(np);
+	if (err) {
+		NL_SET_ERR_MSG_MOD(extack, "Failed to delete host flood rule");
+		return err;
+	}
+
 	netc_port_set_mlo(np, MLO_NOT_OVERRIDE);
 
 	if (br_vlan_enabled(bridge.dev))
@@ -2004,8 +2015,6 @@ static int netc_port_bridge_join(struct dsa_switch *ds, int port,
 	netc_port_set_pvid(np, vlan_unaware_pvid);
 
 out:
-	netc_port_remove_host_flood(np, np->ipft_hf_eid);
-
 	if (atomic_inc_return(&priv->br_cnt) == 1)
 		schedule_delayed_work(&priv->fdbt_ageing_work,
 				      READ_ONCE(priv->fdbt_ageing_delay));
@@ -2015,6 +2024,11 @@ static int netc_port_bridge_join(struct dsa_switch *ds, int port,
 disable_mlo:
 	netc_port_set_mlo(np, MLO_DISABLE);
 
+	if (netc_port_add_host_flood_rule(np, uc, mc))
+		dev_err(ds->dev,
+			"Failed to restore host flood rule on port %u\n",
+			port);
+
 	return err;
 }
 
-- 
2.34.1

[PATCH v4 net-next 6/8] net: dsa: netc: enable ingress port filtering lookup by default

[wei.fang]

From: Wei Fang <wei.fang@nxp.com>

The ingress port filtering lookup function involves performing a lookup
against the ingress port filter table (IPFT). If the frame matches an
entry, subsequent frame processing functions will perform corresponding
operations based on the parameters specified in that entry. If no entry
matches the frame, the frame is allowed in, and passed to the next frame
processing function.

In the future, the IPFT will be used to filter PTP frames and support tc
flower. Tracking in software whether any IPFT entry exists for a port in
order to decide whether to enable the ingress port filtering lookup would
increase code complexity unnecessarily. Since enabling the lookup when
the IPFT is empty has no effect on RX frames, enable the ingress port
filtering lookup unconditionally during driver initialization to simplify
code logic.

The only concern is that when deleting an IPFT entry fails, the hardware
will continue filtering RX frames based on that entry. However, we have
no choice. We cannot disable ingress port filter lookup because this
would deactivate other IPFT entries, which is obviously not what we want.
Moreover, deletion failures are almost impossible under normal
circumstances, and we don't need to be bothered by this minor issue.

Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 drivers/net/dsa/netc/netc_main.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/netc/netc_main.c b/drivers/net/dsa/netc/netc_main.c
index 082ed9405868..336f5a4633e9 100644
--- a/drivers/net/dsa/netc/netc_main.c
+++ b/drivers/net/dsa/netc/netc_main.c
@@ -554,6 +554,12 @@ static void netc_port_fixed_config(struct netc_port *np)
 	netc_port_rmw(np, NETC_PCR, PCR_L2DOSE | PCR_L3DOSE,
 		      PCR_L2DOSE | PCR_L3DOSE);
 
+	/* Enable ingress port filter table lookup, if no match is found,
+	 * the frame is allowed and passed to the next frame processing
+	 * function.
+	 */
+	netc_port_wr(np, NETC_PIPFCR, PIPFCR_EN);
+
 	/* Set the quanta value of TX PAUSE frame */
 	netc_mac_port_wr(np, NETC_PM_PAUSE_QUANTA(0), NETC_PAUSE_QUANTA);
 
@@ -1746,8 +1752,6 @@ static int netc_port_add_host_flood_rule(struct netc_port *np,
 	np->uc = uc;
 	np->mc = mc;
 	np->ipft_hf_eid = host_flood->entry_id;
-	/* Enable ingress port filter table lookup */
-	netc_port_wr(np, NETC_PIPFCR, PIPFCR_EN);
 
 free_host_flood:
 	kfree(host_flood);
@@ -1771,7 +1775,6 @@ static int netc_port_remove_host_flood(struct netc_port *np)
 	np->ipft_hf_eid = NTMP_NULL_ENTRY_ID;
 	np->uc = false;
 	np->mc = false;
-	netc_port_wr(np, NETC_PIPFCR, 0);
 
 	return 0;
 }
-- 
2.34.1

[PATCH v4 net-next 7/8] net: dsa: netc: add PTP two-step timestamping support

[wei.fang]

From: Wei Fang <wei.fang@nxp.com>

Add two-step TX timestamping and RX timestamping for the NETC switch.

For RX, install ingress port filter table (IPFT) rules that redirect
PTP frames to the CPU port, covering L2, L4 over IPv4 and L4 over IPv6
according to the hwtstamp rx_filter. The hardware inserts a To_Host
subtype 1 tag carrying the ingress timestamp, which the tagger extracts
into the skb control buffer for netc_port_rxtstamp() to report.

For two-step TX, clone the skb, assign a per-port timestamp request ID,
and queue the clone. netc_xmit() carries the same ID in the To_Port
subtype 2 tag of the transmitted frame. The hardware then returns a
To_Host subtype 2 response frame echoing that ID and the transmit
timestamp. The tagger dispatches both to the driver through the
twostep_tstamp_handler callback in netc_tagger_data, which matches the
ID to the queued clone and completes it via skb_complete_tx_timestamp().
Non-PTP frames keep using the To_Port subtype 0 tag on the fast path.

Manage request IDs per port: each transmit takes the first ID not in
flight, and an ID is freed once its clone leaves the queue. A per-port
delayed work drops any clone left unmatched for 5 seconds, far beyond
the hardware's normal response latency.

The response frame carries no payload and is only 26 bytes long. As
eth_type_trans() advances skb->data past the DMAC, SMAC and TPID, the
tag starts at (skb->data - 2), so the pskb_may_pull() check must use
(NETC_TAG_MAX_LEN - 2) to avoid dropping the response frame.

Add the To_Port subtype 2 and To_Host subtype 1/2 tag structures, select
the TX tag from ptp_flag in netc_xmit(), and add netc_connect()/
netc_disconnect() to manage the per-switch netc_tagger_data. Grab the
PTP timer's pci_dev in netc_setup() so get_ts_info() can report its PHC
index, and release it on teardown and error paths.

Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 drivers/net/dsa/netc/Kconfig         |   1 +
 drivers/net/dsa/netc/Makefile        |   3 +-
 drivers/net/dsa/netc/netc_main.c     |  71 +++-
 drivers/net/dsa/netc/netc_platform.c |   1 +
 drivers/net/dsa/netc/netc_ptp.c      | 492 +++++++++++++++++++++++++++
 drivers/net/dsa/netc/netc_switch.h   |  42 +++
 include/linux/dsa/tag_netc.h         |  21 ++
 net/dsa/tag_netc.c                   | 165 ++++++++-
 8 files changed, 781 insertions(+), 15 deletions(-)
 create mode 100644 drivers/net/dsa/netc/netc_ptp.c

diff --git a/drivers/net/dsa/netc/Kconfig b/drivers/net/dsa/netc/Kconfig
index 793f7691a24f..8770b65d0f62 100644
--- a/drivers/net/dsa/netc/Kconfig
+++ b/drivers/net/dsa/netc/Kconfig
@@ -4,6 +4,7 @@ config NET_DSA_NETC_SWITCH
 	depends on ARM64 || COMPILE_TEST
 	depends on NET_DSA && PCI
 	depends on NET_VENDOR_FREESCALE
+	depends on PTP_1588_CLOCK_OPTIONAL
 	select NET_DSA_TAG_NETC
 	select FSL_ENETC_MDIO
 	select NXP_NTMP
diff --git a/drivers/net/dsa/netc/Makefile b/drivers/net/dsa/netc/Makefile
index f40b13c702e0..572b833ad80f 100644
--- a/drivers/net/dsa/netc/Makefile
+++ b/drivers/net/dsa/netc/Makefile
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_NET_DSA_NETC_SWITCH) += nxp-netc-switch.o
-nxp-netc-switch-objs := netc_main.o netc_platform.o netc_ethtool.o
+nxp-netc-switch-objs := netc_main.o netc_platform.o netc_ethtool.o \
+			netc_ptp.o
diff --git a/drivers/net/dsa/netc/netc_main.c b/drivers/net/dsa/netc/netc_main.c
index 336f5a4633e9..03200fc63a09 100644
--- a/drivers/net/dsa/netc/netc_main.c
+++ b/drivers/net/dsa/netc/netc_main.c
@@ -65,6 +65,20 @@ netc_get_tag_protocol(struct dsa_switch *ds, int port,
 	return DSA_TAG_PROTO_NETC;
 }
 
+static int netc_connect_tag_protocol(struct dsa_switch *ds,
+				     enum dsa_tag_protocol proto)
+{
+	struct netc_tagger_data *tagger_data;
+
+	if (proto != DSA_TAG_PROTO_NETC)
+		return -EPROTONOSUPPORT;
+
+	tagger_data = ds->tagger_data;
+	tagger_data->txtstamp_handler = netc_port_txtstamp_handler;
+
+	return 0;
+}
+
 static void netc_port_rmw(struct netc_port *np, u32 reg,
 			  u32 mask, u32 val)
 {
@@ -291,6 +305,15 @@ static int netc_init_all_ports(struct netc_switch *priv)
 				dev_err(dev, "Failed to create MDIO bus\n");
 				return err;
 			}
+
+			/* Only the user port needs to support PTP feature, so
+			 * PTP-related resources, such as tstamp_queue,
+			 * tstamp_lock, etc., are initialized only for the user
+			 * port.
+			 */
+			err = netc_port_ptp_init(np);
+			if (err)
+				return err;
 		}
 	}
 
@@ -880,6 +903,15 @@ static int netc_switch_bpt_default_config(struct netc_switch *priv)
 	return 0;
 }
 
+static struct pci_dev *netc_get_ptp_timer(struct netc_switch *priv)
+{
+	struct pci_bus *bus = priv->pdev->bus;
+	u32 devfn = priv->info->tmr_devfn;
+
+	return pci_get_domain_bus_and_slot(pci_domain_nr(bus),
+					   bus->number, devfn);
+}
+
 static int netc_setup(struct dsa_switch *ds)
 {
 	struct netc_switch *priv = ds->priv;
@@ -892,13 +924,23 @@ static int netc_setup(struct dsa_switch *ds)
 
 	netc_get_switch_capabilities(priv);
 
+	/* The PTP timer sits on the same PCI bus as the switch. PCI creates
+	 * every function's pci_dev during bus enumeration, before any driver
+	 * probes, so we can grab the timer's pci_dev here even if the timer
+	 * driver has not probed yet.
+	 */
+	priv->tmr_dev = netc_get_ptp_timer(priv);
+	if (!priv->tmr_dev)
+		dev_info(priv->dev,
+			 "PTP timer PCI device not found\n");
+
 	err = netc_init_all_ports(priv);
 	if (err)
-		return err;
+		goto put_ptp_timer;
 
 	err = netc_init_ntmp_user(priv);
 	if (err)
-		return err;
+		goto put_ptp_timer;
 
 	INIT_HLIST_HEAD(&priv->fdb_list);
 	mutex_init(&priv->fdbt_lock);
@@ -937,6 +979,8 @@ static int netc_setup(struct dsa_switch *ds)
 	mutex_destroy(&priv->fdbt_lock);
 	mutex_destroy(&priv->vft_lock);
 	netc_free_ntmp_user(priv);
+put_ptp_timer:
+	pci_dev_put(priv->tmr_dev);
 
 	return err;
 }
@@ -949,6 +993,21 @@ static void netc_destroy_all_lists(struct netc_switch *priv)
 	mutex_destroy(&priv->vft_lock);
 }
 
+static void netc_free_ports_resources(struct netc_switch *priv)
+{
+	struct dsa_port *dp;
+
+	dsa_switch_for_each_available_port(dp, priv->ds) {
+		struct netc_port *np = priv->ports[dp->index];
+
+		if (!dsa_port_is_user(dp))
+			continue;
+
+		disable_delayed_work_sync(&np->tstamp_timeout_work);
+		netc_port_purge_tstamp_queue(np);
+	}
+}
+
 static void netc_teardown(struct dsa_switch *ds)
 {
 	struct netc_switch *priv = ds->priv;
@@ -956,6 +1015,8 @@ static void netc_teardown(struct dsa_switch *ds)
 	disable_delayed_work_sync(&priv->fdbt_ageing_work);
 	netc_destroy_all_lists(priv);
 	netc_free_ntmp_user(priv);
+	netc_free_ports_resources(priv);
+	pci_dev_put(priv->tmr_dev);
 }
 
 static bool netc_port_is_emdio_consumer(struct device_node *node)
@@ -2403,6 +2464,7 @@ static const struct phylink_mac_ops netc_phylink_mac_ops = {
 
 static const struct dsa_switch_ops netc_switch_ops = {
 	.get_tag_protocol		= netc_get_tag_protocol,
+	.connect_tag_protocol		= netc_connect_tag_protocol,
 	.setup				= netc_setup,
 	.teardown			= netc_teardown,
 	.phylink_get_caps		= netc_phylink_get_caps,
@@ -2431,6 +2493,11 @@ static const struct dsa_switch_ops netc_switch_ops = {
 	.get_sset_count			= netc_port_get_sset_count,
 	.get_strings			= netc_port_get_strings,
 	.get_ethtool_stats		= netc_port_get_ethtool_stats,
+	.get_ts_info			= netc_get_ts_info,
+	.port_hwtstamp_set		= netc_port_hwtstamp_set,
+	.port_hwtstamp_get		= netc_port_hwtstamp_get,
+	.port_rxtstamp			= netc_port_rxtstamp,
+	.port_txtstamp			= netc_port_txtstamp,
 };
 
 static int netc_switch_probe(struct pci_dev *pdev,
diff --git a/drivers/net/dsa/netc/netc_platform.c b/drivers/net/dsa/netc/netc_platform.c
index 34aeb6fceb3c..4fd0ce6770c3 100644
--- a/drivers/net/dsa/netc/netc_platform.c
+++ b/drivers/net/dsa/netc/netc_platform.c
@@ -50,6 +50,7 @@ static void imx94_switch_phylink_get_caps(int port,
 
 static const struct netc_switch_info imx94_info = {
 	.num_ports = 4,
+	.tmr_devfn = PCI_DEVFN(0, 1),
 	.phylink_get_caps = imx94_switch_phylink_get_caps,
 };
 
diff --git a/drivers/net/dsa/netc/netc_ptp.c b/drivers/net/dsa/netc/netc_ptp.c
new file mode 100644
index 000000000000..2e743443ca03
--- /dev/null
+++ b/drivers/net/dsa/netc/netc_ptp.c
@@ -0,0 +1,492 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
+/*
+ * NXP NETC switch driver
+ * Copyright 2025-2026 NXP
+ */
+
+#include <linux/ptp_classify.h>
+#include <linux/ptp_clock_kernel.h>
+
+#include "netc_switch.h"
+
+#define NETC_NUM_TS_REQ_ID		16
+#define NETC_TSTAMP_TIMEOUT		(5 * HZ)
+
+static void netc_port_tstamp_timeout_work(struct work_struct *work)
+{
+	struct netc_port *np = container_of(work, struct netc_port,
+					    tstamp_timeout_work.work);
+	struct sk_buff_head free_list;
+	struct sk_buff *skb, *skb_tmp;
+
+	__skb_queue_head_init(&free_list);
+
+	spin_lock_bh(&np->tstamp_lock);
+	skb_queue_walk_safe(&np->tstamp_queue, skb, skb_tmp) {
+		if (time_before64(jiffies_64, NETC_SKB_CB(skb)->ptp_tx_time +
+				  NETC_TSTAMP_TIMEOUT))
+			continue;
+
+		dev_dbg_ratelimited(np->switch_priv->dev,
+				    "Port %d ts_req_id %u which seems lost\n",
+				    np->dp->index, NETC_SKB_CB(skb)->ts_req_id);
+
+		__skb_unlink(skb, &np->tstamp_queue);
+		__skb_queue_tail(&free_list, skb);
+	}
+
+	/* Reschedule if there are still pending clones that have not
+	 * timed out yet.
+	 */
+	if (!skb_queue_empty(&np->tstamp_queue))
+		schedule_delayed_work(&np->tstamp_timeout_work,
+				      NETC_TSTAMP_TIMEOUT);
+
+	spin_unlock_bh(&np->tstamp_lock);
+	__skb_queue_purge(&free_list);
+}
+
+static int netc_get_ts_req_id(struct netc_port *np)
+{
+	DECLARE_BITMAP(ts_req_id_bitmap, NETC_NUM_TS_REQ_ID);
+	struct sk_buff *skb, *skb_tmp;
+	unsigned long ts_req_id;
+
+	bitmap_zero(ts_req_id_bitmap, NETC_NUM_TS_REQ_ID);
+
+	skb_queue_walk_safe(&np->tstamp_queue, skb, skb_tmp)
+		__set_bit(NETC_SKB_CB(skb)->ts_req_id, ts_req_id_bitmap);
+
+	ts_req_id = find_first_zero_bit(ts_req_id_bitmap, NETC_NUM_TS_REQ_ID);
+	if (ts_req_id == NETC_NUM_TS_REQ_ID) {
+		dev_dbg_ratelimited(np->switch_priv->dev,
+				    "Port %d has no available ts_req_id\n",
+				    np->dp->index);
+		return -ENOSPC;
+	}
+
+	return ts_req_id;
+}
+
+int netc_port_ptp_init(struct netc_port *np)
+{
+	/* Initialize to invalid entry IDs */
+	for (int i = 0; i < NETC_PTP_MAX; i++)
+		np->ptp_ipft_eid[i] = NTMP_NULL_ENTRY_ID;
+
+	spin_lock_init(&np->tstamp_lock);
+	__skb_queue_head_init(&np->tstamp_queue);
+	INIT_DELAYED_WORK(&np->tstamp_timeout_work,
+			  netc_port_tstamp_timeout_work);
+
+	return 0;
+}
+
+void netc_port_purge_tstamp_queue(struct netc_port *np)
+{
+	struct sk_buff_head free_list;
+
+	__skb_queue_head_init(&free_list);
+
+	spin_lock_bh(&np->tstamp_lock);
+	skb_queue_splice_init(&np->tstamp_queue, &free_list);
+	spin_unlock_bh(&np->tstamp_lock);
+
+	__skb_queue_purge(&free_list);
+}
+
+static int netc_get_phc_index(struct netc_switch *priv)
+{
+	if (!priv->tmr_dev)
+		return -1;
+
+	return ptp_clock_index_by_dev(&priv->tmr_dev->dev);
+}
+
+int netc_get_ts_info(struct dsa_switch *ds, int port,
+		     struct kernel_ethtool_ts_info *info)
+{
+	struct netc_switch *priv = ds->priv;
+
+	info->phc_index = netc_get_phc_index(priv);
+	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_PTP_V2_EVENT) |
+			   BIT(HWTSTAMP_FILTER_PTP_V2_L2_EVENT) |
+			   BIT(HWTSTAMP_FILTER_PTP_V2_L4_EVENT);
+
+	return 0;
+}
+
+static int netc_port_del_ptp_filter(struct netc_port *np)
+{
+	struct netc_switch *priv = np->switch_priv;
+	int ret = 0;
+	int err;
+
+	for (int i = 0; i < NETC_PTP_MAX; i++) {
+		if (np->ptp_ipft_eid[i] == NTMP_NULL_ENTRY_ID)
+			continue;
+
+		/* No -ETIMEDOUT here: with the command BD ring enabled, the
+		 * hardware never times out on a command. Any remaining error
+		 * means the entry is still present in the table.
+		 */
+		err = ntmp_ipft_delete_entry(&priv->ntmp,
+					     np->ptp_ipft_eid[i]);
+		if (likely(!err)) {
+			np->ptp_ipft_eid[i] = NTMP_NULL_ENTRY_ID;
+			continue;
+		}
+
+		ret = err;
+		dev_err(priv->dev,
+			"Delete PTP entry 0x%x (type %d) on port %d failed\n",
+			np->ptp_ipft_eid[i], i, np->dp->index);
+	}
+
+	return ret;
+}
+
+static int netc_build_ptp_ipft_keye(struct ipft_keye_data *keye, int port,
+				    enum netc_ptp_type type)
+{
+	u16 src_port, frm_attr_flags;
+
+	keye->precedence = cpu_to_le16(NETC_IPFT_PTP_PRECEDENCE);
+	src_port = FIELD_PREP(IPFT_SRC_PORT, port);
+	src_port |= IPFT_SRC_PORT_MASK;
+	keye->src_port = cpu_to_le16(src_port);
+
+	switch (type) {
+	case NETC_PTP_L2:
+		keye->ethertype = htons(ETH_P_1588);
+		keye->ethertype_mask = htons(0xffff);
+		break;
+	case NETC_PTP_L4_IPV4_EVENT:
+	case NETC_PTP_L4_IPV4_GENERAL:
+	case NETC_PTP_L4_IPV6_EVENT:
+	case NETC_PTP_L4_IPV6_GENERAL:
+		frm_attr_flags = IPFT_FAF_IP_HDR | FIELD_PREP(IPFT_FAF_L4_CODE,
+				 IPFT_FAF_UDP_HDR);
+		if (type == NETC_PTP_L4_IPV6_EVENT ||
+		    type == NETC_PTP_L4_IPV6_GENERAL)
+			frm_attr_flags |= IPFT_FAF_IP_VER6;
+
+		keye->frm_attr_flags = cpu_to_le16(frm_attr_flags);
+
+		/* Set IP version bit in flags_mask to match IPv4 or IPv6
+		 * packets
+		 */
+		frm_attr_flags |= IPFT_FAF_IP_VER6;
+		keye->frm_attr_flags_mask = cpu_to_le16(frm_attr_flags);
+		keye->ip_protocol = IPPROTO_UDP;
+		keye->ip_protocol_mask = 0xff;
+
+		if (type == NETC_PTP_L4_IPV4_EVENT ||
+		    type == NETC_PTP_L4_IPV6_EVENT)
+			keye->l4_dst_port = htons(PTP_EV_PORT);
+		else
+			keye->l4_dst_port = htons(PTP_GEN_PORT);
+
+		keye->l4_dst_port_mask = htons(0xffff);
+		break;
+	default:
+		return -ERANGE;
+	}
+
+	return 0;
+}
+
+static int netc_port_add_ipft_ptp_entry(struct netc_port *np,
+					enum netc_ptp_type type)
+{
+	struct netc_switch *priv = np->switch_priv;
+	struct ipft_entry_data *entry;
+	struct ipft_keye_data *keye;
+	u32 cfg;
+	int err;
+
+	entry = kzalloc_obj(*entry);
+	if (!entry)
+		return -ENOMEM;
+
+	keye = &entry->keye;
+	err = netc_build_ptp_ipft_keye(keye, np->dp->index, type);
+	if (err)
+		goto free_entry;
+
+	cfg = FIELD_PREP(IPFT_FLTFA, IPFT_FLTFA_REDIRECT);
+	cfg |= FIELD_PREP(IPFT_HR, NETC_HR_PTP_TRAP);
+	cfg |= IPFT_TIMECAPE | IPFT_RRT;
+	entry->cfge.cfg = cpu_to_le32(cfg);
+
+	err = ntmp_ipft_add_entry(&priv->ntmp, entry);
+	if (err)
+		goto free_entry;
+
+	np->ptp_ipft_eid[type] = entry->entry_id;
+
+free_entry:
+	kfree(entry);
+
+	return err;
+}
+
+static int netc_port_add_l2_ptp_filter(struct netc_port *np)
+{
+	return netc_port_add_ipft_ptp_entry(np, NETC_PTP_L2);
+}
+
+static int netc_port_add_l4_ptp_filter(struct netc_port *np)
+{
+	int err;
+
+	err = netc_port_add_ipft_ptp_entry(np, NETC_PTP_L4_IPV4_EVENT);
+	if (err)
+		return err;
+
+	err = netc_port_add_ipft_ptp_entry(np, NETC_PTP_L4_IPV4_GENERAL);
+	if (err)
+		goto del_ptp_filter;
+
+	err = netc_port_add_ipft_ptp_entry(np, NETC_PTP_L4_IPV6_EVENT);
+	if (err)
+		goto del_ptp_filter;
+
+	err = netc_port_add_ipft_ptp_entry(np, NETC_PTP_L4_IPV6_GENERAL);
+	if (err)
+		goto del_ptp_filter;
+
+	return 0;
+
+del_ptp_filter:
+	netc_port_del_ptp_filter(np);
+
+	return err;
+}
+
+static int netc_port_add_l2_l4_ptp_filter(struct netc_port *np)
+{
+	int err;
+
+	err = netc_port_add_l2_ptp_filter(np);
+	if (err)
+		return err;
+
+	err = netc_port_add_l4_ptp_filter(np);
+	if (err)
+		goto del_ptp_filter;
+
+	return 0;
+
+del_ptp_filter:
+	netc_port_del_ptp_filter(np);
+
+	return err;
+}
+
+static int netc_port_set_ptp_filter(struct netc_port *np, int rx_filter)
+{
+	int err;
+
+	err = netc_port_del_ptp_filter(np);
+	if (err)
+		return err;
+
+	np->ptp_rx_filter = HWTSTAMP_FILTER_NONE;
+
+	switch (rx_filter) {
+	case HWTSTAMP_FILTER_NONE:
+		break;
+	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
+		err = netc_port_add_l2_ptp_filter(np);
+		break;
+	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
+		err = netc_port_add_l4_ptp_filter(np);
+		break;
+	case HWTSTAMP_FILTER_PTP_V2_EVENT:
+		err = netc_port_add_l2_l4_ptp_filter(np);
+		break;
+	default:
+		err = -ERANGE;
+	}
+
+	if (err)
+		return err;
+
+	np->ptp_rx_filter = rx_filter;
+
+	return 0;
+}
+
+int netc_port_hwtstamp_set(struct dsa_switch *ds, int port,
+			   struct kernel_hwtstamp_config *config,
+			   struct netlink_ext_ack *extack)
+{
+	struct netc_port *np = NETC_PORT(ds, port);
+	struct netc_switch *priv = ds->priv;
+	int rx_filter, err;
+
+	if ((config->tx_type != HWTSTAMP_TX_OFF ||
+	     config->rx_filter != HWTSTAMP_FILTER_NONE) &&
+	    netc_get_phc_index(priv) < 0)
+		return -EOPNOTSUPP;
+
+	switch (config->tx_type) {
+	case HWTSTAMP_TX_ON:
+	case HWTSTAMP_TX_OFF:
+		break;
+	default:
+		return -ERANGE;
+	}
+
+	switch (config->rx_filter) {
+	case HWTSTAMP_FILTER_NONE:
+		rx_filter = HWTSTAMP_FILTER_NONE;
+		break;
+	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
+		rx_filter = HWTSTAMP_FILTER_PTP_V2_L4_EVENT;
+		break;
+	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
+		rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
+		break;
+	case HWTSTAMP_FILTER_PTP_V2_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
+		rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
+		break;
+	default:
+		return -ERANGE;
+	}
+
+	err = netc_port_set_ptp_filter(np, rx_filter);
+	if (err) {
+		NL_SET_ERR_MSG_MOD(extack, "Failed to set PTP filter");
+		return err;
+	}
+
+	WRITE_ONCE(np->ptp_tx_type, config->tx_type);
+	config->rx_filter = rx_filter;
+
+	return 0;
+}
+
+int netc_port_hwtstamp_get(struct dsa_switch *ds, int port,
+			   struct kernel_hwtstamp_config *config)
+{
+	struct netc_port *np = NETC_PORT(ds, port);
+
+	config->tx_type = READ_ONCE(np->ptp_tx_type);
+	config->rx_filter = np->ptp_rx_filter;
+
+	return 0;
+}
+
+static void netc_port_prepare_twostep(struct netc_port *np,
+				      struct sk_buff *nskb)
+{
+	struct sk_buff *clone = skb_clone_sk(nskb);
+	int ts_req_id;
+
+	if (unlikely(!clone))
+		return;
+
+	spin_lock_bh(&np->tstamp_lock);
+
+	ts_req_id = netc_get_ts_req_id(np);
+	if (ts_req_id < 0) {
+		spin_unlock_bh(&np->tstamp_lock);
+		kfree_skb(clone);
+		return;
+	}
+
+	NETC_SKB_CB(nskb)->ptp_flag = NETC_PTP_FLAG_TWOSTEP;
+	NETC_SKB_CB(nskb)->ts_req_id = ts_req_id;
+	NETC_SKB_CB(clone)->ts_req_id = ts_req_id;
+	NETC_SKB_CB(clone)->ptp_tx_time = jiffies_64;
+	NETC_SKB_CB(clone)->ptp_flag = NETC_PTP_FLAG_TWOSTEP;
+	skb_shinfo(clone)->tx_flags |= SKBTX_IN_PROGRESS;
+	__skb_queue_tail(&np->tstamp_queue, clone);
+	if (!delayed_work_pending(&np->tstamp_timeout_work))
+		schedule_delayed_work(&np->tstamp_timeout_work,
+				      NETC_TSTAMP_TIMEOUT);
+
+	spin_unlock_bh(&np->tstamp_lock);
+}
+
+void netc_port_txtstamp_handler(struct dsa_switch *ds, int port,
+				u8 ts_req_id, u64 ts)
+{
+	struct sk_buff *skb, *skb_tmp, *skb_match = NULL;
+	struct netc_port *np = NETC_PORT(ds, port);
+	struct skb_shared_hwtstamps hwtstamps;
+
+	spin_lock_bh(&np->tstamp_lock);
+	skb_queue_walk_safe(&np->tstamp_queue, skb, skb_tmp) {
+		if (NETC_SKB_CB(skb)->ts_req_id != ts_req_id)
+			continue;
+
+		__skb_unlink(skb, &np->tstamp_queue);
+		skb_match = skb;
+		break;
+	}
+	spin_unlock_bh(&np->tstamp_lock);
+
+	if (!skb_match) {
+		dev_dbg_ratelimited(np->switch_priv->dev,
+				    "Port %d ts_req_id %u which seems lost\n",
+				    port, ts_req_id);
+		return;
+	}
+
+	hwtstamps.hwtstamp = ns_to_ktime(ts);
+	skb_complete_tx_timestamp(skb_match, &hwtstamps);
+}
+
+bool netc_port_rxtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb,
+			unsigned int type)
+{
+	struct skb_shared_hwtstamps *hwtstamps = skb_hwtstamps(skb);
+
+	if (!NETC_SKB_CB(skb)->rx_tstamp_valid)
+		return false;
+
+	hwtstamps->hwtstamp = ns_to_ktime(NETC_SKB_CB(skb)->tstamp);
+
+	return false;
+}
+
+void netc_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
+{
+	struct netc_port *np = NETC_PORT(ds, port);
+	u32 ptp_class;
+	int tx_type;
+
+	NETC_SKB_CB(skb)->ptp_flag = 0;
+	ptp_class = ptp_classify_raw(skb);
+	if (ptp_class == PTP_CLASS_NONE)
+		return;
+
+	/* The rx_filters in netc_get_ts_info() has already declared that
+	 * it only supports PTP v2, so TX only supports v2 as well.
+	 */
+	if (unlikely(ptp_class & PTP_CLASS_V1))
+		return;
+
+	tx_type = READ_ONCE(np->ptp_tx_type);
+	if (tx_type == HWTSTAMP_TX_ON)
+		netc_port_prepare_twostep(np, skb);
+}
diff --git a/drivers/net/dsa/netc/netc_switch.h b/drivers/net/dsa/netc/netc_switch.h
index aeb08e6eda09..a1f4b1bc04cb 100644
--- a/drivers/net/dsa/netc/netc_switch.h
+++ b/drivers/net/dsa/netc/netc_switch.h
@@ -53,10 +53,16 @@
 #define NETC_FDBT_AGEING_DELAY		(3 * HZ)
 #define NETC_FDBT_AGEING_THRESH		100
 
+/* PTP frames have a higher priority, so a higher priority is defined to
+ * prioritize matching (The higher the value, the higher the priority).
+ */
+#define NETC_IPFT_PTP_PRECEDENCE	0xf000
+
 struct netc_switch;
 
 struct netc_switch_info {
 	u32 num_ports;
+	u32 tmr_devfn;
 	void (*phylink_get_caps)(int port, struct phylink_config *config);
 };
 
@@ -66,9 +72,19 @@ struct netc_port_caps {
 	u32 pseudo_link:1;
 };
 
+enum netc_ptp_type {
+	NETC_PTP_L2,
+	NETC_PTP_L4_IPV4_EVENT,
+	NETC_PTP_L4_IPV4_GENERAL,
+	NETC_PTP_L4_IPV6_EVENT,
+	NETC_PTP_L4_IPV6_GENERAL,
+	NETC_PTP_MAX,
+};
+
 enum netc_host_reason {
 	/* Software defined host reasons */
 	NETC_HR_HOST_FLOOD = 8,
+	NETC_HR_PTP_TRAP   = 9,
 };
 
 struct netc_port {
@@ -85,6 +101,15 @@ struct netc_port {
 	u16 mc:1;
 	u16 pvid;
 	u32 ipft_hf_eid; /* Must be initialized to NTMP_NULL_ENTRY_ID */
+
+	/* Serialize access to tstamp_queue */
+	spinlock_t tstamp_lock;
+	/* skb queue for TX timestamp frames */
+	struct sk_buff_head tstamp_queue;
+	struct delayed_work tstamp_timeout_work;
+	int ptp_tx_type;
+	int ptp_rx_filter;
+	u32 ptp_ipft_eid[NETC_PTP_MAX];
 };
 
 struct netc_switch_regs {
@@ -139,6 +164,7 @@ struct netc_switch {
 	u32 num_bp;
 
 	struct bpt_cfge_data *bpt_list;
+	struct pci_dev *tmr_dev; /* The PTP Timer PCI device */
 };
 
 #define NETC_PRIV(ds)			((struct netc_switch *)((ds)->priv))
@@ -203,4 +229,20 @@ void netc_port_get_strings(struct dsa_switch *ds, int port,
 			   u32 sset, u8 *data);
 void netc_port_get_ethtool_stats(struct dsa_switch *ds, int port, u64 *data);
 
+/* PTP APIs */
+int netc_port_ptp_init(struct netc_port *np);
+int netc_get_ts_info(struct dsa_switch *ds, int port,
+		     struct kernel_ethtool_ts_info *info);
+void netc_port_purge_tstamp_queue(struct netc_port *np);
+int netc_port_hwtstamp_set(struct dsa_switch *ds, int port,
+			   struct kernel_hwtstamp_config *config,
+			   struct netlink_ext_ack *extack);
+int netc_port_hwtstamp_get(struct dsa_switch *ds, int port,
+			   struct kernel_hwtstamp_config *config);
+void netc_port_txtstamp_handler(struct dsa_switch *ds, int port,
+				u8 ts_req_id, u64 ts);
+bool netc_port_rxtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb,
+			unsigned int type);
+void netc_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb);
+
 #endif
diff --git a/include/linux/dsa/tag_netc.h b/include/linux/dsa/tag_netc.h
index fe964722e5b0..5a567af20094 100644
--- a/include/linux/dsa/tag_netc.h
+++ b/include/linux/dsa/tag_netc.h
@@ -10,5 +10,26 @@
 #include <net/dsa.h>
 
 #define NETC_TAG_MAX_LEN			14
+#define NETC_PTP_FLAG_TWOSTEP			BIT(1)
+
+struct netc_skb_cb {
+	u64 ptp_tx_time;
+	u64 tstamp;
+	bool rx_tstamp_valid;
+	u8 ptp_flag;
+	u8 ts_req_id;
+};
+
+#define NETC_SKB_CB(skb)	((struct netc_skb_cb *)((skb)->cb))
+
+/**
+ * struct netc_tagger_data - NETC tagger/switch-driver shared operations
+ * @txtstamp_handler: Called by the tagger when a two-step transmit timestamp
+ *	response is received, to deliver the timestamp to the switch driver.
+ */
+struct netc_tagger_data {
+	void (*txtstamp_handler)(struct dsa_switch *ds, int port,
+				 u8 ts_req_id, u64 ts);
+};
 
 #endif
diff --git a/net/dsa/tag_netc.c b/net/dsa/tag_netc.c
index df72a61796ad..6b492451d092 100644
--- a/net/dsa/tag_netc.c
+++ b/net/dsa/tag_netc.c
@@ -16,6 +16,8 @@
 #define NETC_TAG_TO_PORT		1
 /* SubType0: No request to perform timestamping */
 #define NETC_TAG_TP_SUBTYPE0		0
+/* SubType2: Request to perform two-step timestamping */
+#define NETC_TAG_TP_SUBTYPE2		2
 
 /* To_Host NXP switch tag */
 #define NETC_TAG_TO_HOST		2
@@ -29,6 +31,7 @@
 /* NETC switch tag lengths */
 #define NETC_TAG_FORWARD_LEN		6
 #define NETC_TAG_TP_SUBTYPE0_LEN	6
+#define NETC_TAG_TP_SUBTYPE2_LEN	6
 #define NETC_TAG_TH_SUBTYPE0_LEN	6
 #define NETC_TAG_TH_SUBTYPE1_LEN	14
 #define NETC_TAG_TH_SUBTYPE2_LEN	14
@@ -40,6 +43,7 @@
 #define NETC_TAG_IPV			GENMASK(4, 2)
 #define NETC_TAG_SWITCH			GENMASK(2, 0)
 #define NETC_TAG_PORT			GENMASK(7, 3)
+#define NETC_TAG_TS_REQ_ID		GENMASK(3, 0)
 
 struct netc_tag_cmn {
 	__be16 tpid;
@@ -48,6 +52,23 @@ struct netc_tag_cmn {
 	u8 switch_port;
 } __packed;
 
+struct netc_tag_tp_subtype2 {
+	struct netc_tag_cmn cmn;
+	u8 ts_req_id;
+} __packed;
+
+struct netc_tag_th_subtype1 {
+	struct netc_tag_cmn cmn;
+	u8 host_reason;
+	__be64 timestamp;
+} __packed;
+
+struct netc_tag_th_subtype2 {
+	struct netc_tag_cmn cmn;
+	u8 hr_tsreq_id;
+	__be64 timestamp;
+} __packed;
+
 static void netc_fill_common_tag(struct netc_tag_cmn *tag, u8 type,
 				 u8 subtype, u8 sw_id, u8 port, u8 ipv)
 {
@@ -97,15 +118,57 @@ static void netc_fill_tp_tag_subtype0(struct sk_buff *skb,
 				NETC_TAG_TP_SUBTYPE0_LEN);
 }
 
-/* Currently only support To_Port tag, subtype 0 */
+static void netc_fill_tp_tag_subtype2(struct sk_buff *skb,
+				      struct net_device *ndev)
+{
+	u8 ts_req_id = NETC_SKB_CB(skb)->ts_req_id;
+	struct netc_tag_tp_subtype2 *tag;
+
+	tag = netc_fill_common_tp_tag(skb, ndev, NETC_TAG_TP_SUBTYPE2,
+				      NETC_TAG_TP_SUBTYPE2_LEN);
+	tag->ts_req_id = FIELD_PREP(NETC_TAG_TS_REQ_ID, ts_req_id);
+}
+
 static struct sk_buff *netc_xmit(struct sk_buff *skb,
 				 struct net_device *ndev)
 {
-	netc_fill_tp_tag_subtype0(skb, ndev);
+	u8 ptp_flag = NETC_SKB_CB(skb)->ptp_flag;
+
+	/* Fast path: the overwhelming majority of frames are not PTP frames */
+	if (likely(!ptp_flag))
+		netc_fill_tp_tag_subtype0(skb, ndev);
+	else
+		/* ptp_flag == NETC_PTP_FLAG_TWOSTEP */
+		netc_fill_tp_tag_subtype2(skb, ndev);
 
 	return skb;
 }
 
+static void netc_rx_tstamp_process(struct netc_tag_th_subtype1 *tag,
+				   struct sk_buff *skb)
+{
+	u64 ts = get_unaligned_be64(&tag->timestamp);
+
+	NETC_SKB_CB(skb)->rx_tstamp_valid = true;
+	NETC_SKB_CB(skb)->tstamp = ts;
+}
+
+static void netc_twostep_tstamp_process(struct netc_tag_th_subtype2 *tag,
+					struct sk_buff *skb)
+{
+	u8 ts_req_id = FIELD_GET(NETC_TAG_TS_REQ_ID, tag->hr_tsreq_id);
+	struct dsa_port *dp = dsa_user_to_port(skb->dev);
+	u64 ts = get_unaligned_be64(&tag->timestamp);
+	struct netc_tagger_data *tagger_data;
+	struct dsa_switch *ds = dp->ds;
+
+	tagger_data = ds->tagger_data;
+	if (unlikely(!tagger_data->txtstamp_handler))
+		return;
+
+	tagger_data->txtstamp_handler(ds, dp->index, ts_req_id, ts);
+}
+
 static int netc_get_rx_tag_len(int type, int subtype)
 {
 	/* Only NETC_TAG_TO_HOST and NETC_TAG_FORWARD are expected in RX,
@@ -129,11 +192,22 @@ static struct sk_buff *netc_rcv(struct sk_buff *skb,
 	struct netc_tag_cmn *tag_cmn;
 	int tag_len, sw_id, port;
 	int type, subtype;
+	void *tag;
 
-	if (unlikely(!pskb_may_pull(skb, NETC_TAG_MAX_LEN)))
+	/* eth_type_trans() pulled ETH_HLEN bytes, so skb->data sits 2 bytes
+	 * past the start of the switch tag (past the TPID) and skb->len is
+	 * ETH_HLEN bytes shorter than the original frame length. The longest
+	 * switch tag is NETC_TAG_MAX_LEN (14) bytes, but since 2 of those
+	 * bytes are already behind skb->data, only NETC_TAG_MAX_LEN - 2 bytes
+	 * need to be in the linear buffer. For the To_Host subtype 2 response
+	 * frame, whose total length is only 26 bytes with no payload after the
+	 * tag, this check is the only guard against a too-short frame.
+	 */
+	if (unlikely(!pskb_may_pull(skb, NETC_TAG_MAX_LEN - 2)))
 		goto err_free_skb;
 
-	tag_cmn = dsa_etype_header_pos_rx(skb);
+	tag = dsa_etype_header_pos_rx(skb);
+	tag_cmn = tag;
 	if (ntohs(tag_cmn->tpid) != ETH_P_NXP_NETC) {
 		dev_warn_ratelimited(&ndev->dev, "Unknown TPID 0x%04x\n",
 				     ntohs(tag_cmn->tpid));
@@ -156,17 +230,50 @@ static struct sk_buff *netc_rcv(struct sk_buff *skb,
 	if (!skb->dev)
 		goto err_free_skb;
 
+	/* skb->cb may be used to store hardware RX timestamp, so clear
+	 * rx_tstamp_valid before processing to avoid data pollution from
+	 * the previous layer.
+	 */
+	NETC_SKB_CB(skb)->rx_tstamp_valid = false;
+
 	type = FIELD_GET(NETC_TAG_TYPE, tag_cmn->type);
 	subtype = FIELD_GET(NETC_TAG_SUBTYPE, tag_cmn->type);
 	if (type == NETC_TAG_FORWARD) {
 		dsa_default_offload_fwd_mark(skb);
 	} else if (type == NETC_TAG_TO_HOST) {
-		/* Currently only subtype0 supported */
-		if (subtype != NETC_TAG_TH_SUBTYPE0)
+		switch (subtype) {
+		case NETC_TAG_TH_SUBTYPE0:
+			break;
+		case NETC_TAG_TH_SUBTYPE1:
+			/* To_Host Subtype 1 tag is 14 bytes, ensure it and the
+			 * EtherType behind it are fully present in the linear
+			 * area before netc_rcv() calls dsa_strip_etype_header()
+			 * to strip the tag.
+			 */
+			if (unlikely(!pskb_may_pull(skb,
+						    NETC_TAG_TH_SUBTYPE1_LEN)))
+				goto err_free_skb;
+
+			tag = dsa_etype_header_pos_rx(skb);
+			netc_rx_tstamp_process(tag, skb);
+			break;
+		case NETC_TAG_TH_SUBTYPE2:
+			/* This skb is a hardware-generated response to a
+			 * two-step transmit timestamp request. The tag
+			 * driver must free the skb after processing.
+			 */
+			netc_twostep_tstamp_process(tag, skb);
+			consume_skb(skb);
+			return NULL;
+		default:
+			dev_warn_ratelimited(&ndev->dev,
+					     "Unsupported To_Host subtype: %d\n",
+					     subtype);
 			goto err_free_skb;
+		}
 	} else {
 		dev_warn_ratelimited(&ndev->dev,
-				     "Unexpected  tag type %d\n", type);
+				     "Unexpected tag type %d\n", type);
 		goto err_free_skb;
 	}
 
@@ -190,16 +297,48 @@ static void netc_flow_dissect(const struct sk_buff *skb, __be16 *proto,
 	int type = FIELD_GET(NETC_TAG_TYPE, tag_cmn->type);
 	int tag_len = netc_get_rx_tag_len(type, subtype);
 
-	/* The RX minimum frame length of the NETC switch port is 64 bytes,
-	 * and the frame is received by the ENETC driver. From the hardware
-	 * perspective, the receive buffer of RX BD is at least 128 bytes,
-	 * so the switch tag header is guaranteed to be in the linear region
-	 * of the skb.
+	/* The CPU port of the switch is connected to the ENETC, so the frame
+	 * is received by the ENETC driver. From the hardware perspective, the
+	 * receive buffer of RX BD is at least 128 bytes, so the switch tag
+	 * header is guaranteed to be in the linear region of the skb.
+	 *
+	 * When the subtype of the frame is NETC_TAG_TH_SUBTYPE2, it indicates
+	 * the frame is a hardware generated timestamp response, which is only
+	 * 26 bytes (DMAC + SMAC + tag), so the frame has no payload after the
+	 * tag. Therefore, there is no need to parse the protocol and offset.
+	 * For other types of the frames, they are all received from the switch
+	 * ports, and the RX minimum frame length of the port is 64 bytes,
+	 * frames shorter than 64 bytes will be discarded by the hardware and
+	 * will not be received by the software.
 	 */
+	if (type == NETC_TAG_TO_HOST && subtype == NETC_TAG_TH_SUBTYPE2)
+		return;
+
 	*offset = tag_len;
 	*proto = ((__be16 *)skb->data)[(tag_len / 2) - 1];
 }
 
+static int netc_connect(struct dsa_switch *ds)
+{
+	struct netc_tagger_data *tagger_data;
+
+	tagger_data = kzalloc_obj(*tagger_data);
+	if (!tagger_data)
+		return -ENOMEM;
+
+	ds->tagger_data = tagger_data;
+
+	return 0;
+}
+
+static void netc_disconnect(struct dsa_switch *ds)
+{
+	struct netc_tagger_data *tagger_data = ds->tagger_data;
+
+	kfree(tagger_data);
+	ds->tagger_data = NULL;
+}
+
 static const struct dsa_device_ops netc_netdev_ops = {
 	.name			= NETC_NAME,
 	.proto			= DSA_TAG_PROTO_NETC,
@@ -207,6 +346,8 @@ static const struct dsa_device_ops netc_netdev_ops = {
 	.rcv			= netc_rcv,
 	.needed_headroom	= NETC_TAG_MAX_LEN,
 	.flow_dissect		= netc_flow_dissect,
+	.connect		= netc_connect,
+	.disconnect		= netc_disconnect,
 };
 
 MODULE_DESCRIPTION("DSA tag driver for NXP NETC switch family");
-- 
2.34.1

[PATCH v4 net-next 8/8] net: dsa: netc: add PTP one-step timestamping support

[wei.fang]

From: Wei Fang <wei.fang@nxp.com>

Add one-step Sync offload for NETC switch ports and advertise
HWTSTAMP_TX_ONESTEP_SYNC. Frames that are not one-step Sync still fall
back to two-step timestamping.

Each port has a single PM_SINGLE_STEP register, which holds the offset
of the correction field in the Sync frame and tells the hardware whether
the UDP checksum needs to be updated. As the register is shared by all
frames of the port, one-step Sync frames have to be sent one at a time.
Therefore, let the tagger hand each one-step Sync over to the switch
driver, which queues it on a per-port queue and drains the queue from a
work.

For every frame, the work reads the current PTP time, programs
PM_SINGLE_STEP, writes the time into the originTimestamp field and lets
the tagger insert a To_Port subtype 3 tag carrying the lower 30 bits of
that time together with a timestamp request ID. The hardware captures
the SFD transmit time, adds the difference between it and the software
timestamp to the correction field, and sends a response frame echoing
the request ID once the Sync frame has left the port. Receiving the
response schedules the work for the next frame.

A dropped frame produces no response, so reuse tstamp_timeout_work to
reclaim its request ID after 5 seconds, which is far longer than the
expected transmit time. Since the tag timestamp is only 30 bits wide
and the hardware accounts for a single wrap at most, also schedule the
work when the PTP time has advanced beyond NETC_ONESTEP_VALID_WINDOW
since the in-flight frame was queued, as its correction field can no
longer be computed correctly.

Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 drivers/net/dsa/netc/Kconfig          |   1 +
 drivers/net/dsa/netc/netc_main.c      |   7 +-
 drivers/net/dsa/netc/netc_ptp.c       | 361 +++++++++++++++++++++++++-
 drivers/net/dsa/netc/netc_switch.h    |  23 ++
 drivers/net/dsa/netc/netc_switch_hw.h |   5 +
 include/linux/dsa/tag_netc.h          |  17 ++
 net/dsa/tag_netc.c                    |  69 ++++-
 7 files changed, 477 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/netc/Kconfig b/drivers/net/dsa/netc/Kconfig
index 8770b65d0f62..7c930579f1c9 100644
--- a/drivers/net/dsa/netc/Kconfig
+++ b/drivers/net/dsa/netc/Kconfig
@@ -5,6 +5,7 @@ config NET_DSA_NETC_SWITCH
 	depends on NET_DSA && PCI
 	depends on NET_VENDOR_FREESCALE
 	depends on PTP_1588_CLOCK_OPTIONAL
+	depends on PTP_NETC_V4_TIMER || PTP_NETC_V4_TIMER=n
 	select NET_DSA_TAG_NETC
 	select FSL_ENETC_MDIO
 	select NXP_NTMP
diff --git a/drivers/net/dsa/netc/netc_main.c b/drivers/net/dsa/netc/netc_main.c
index 03200fc63a09..af39a71aece0 100644
--- a/drivers/net/dsa/netc/netc_main.c
+++ b/drivers/net/dsa/netc/netc_main.c
@@ -75,6 +75,7 @@ static int netc_connect_tag_protocol(struct dsa_switch *ds,
 
 	tagger_data = ds->tagger_data;
 	tagger_data->txtstamp_handler = netc_port_txtstamp_handler;
+	tagger_data->onestep_sync_enqueue = netc_port_onestep_sync_enqueue;
 
 	return 0;
 }
@@ -94,7 +95,7 @@ static void netc_port_rmw(struct netc_port *np, u32 reg,
 	netc_port_wr(np, reg, new);
 }
 
-static void netc_mac_port_wr(struct netc_port *np, u32 reg, u32 val)
+void netc_mac_port_wr(struct netc_port *np, u32 reg, u32 val)
 {
 	if (is_netc_pseudo_port(np))
 		return;
@@ -1571,6 +1572,7 @@ static int netc_port_enable(struct dsa_switch *ds, int port,
 		return err;
 	}
 
+	netc_port_enable_onestep(np);
 	np->enable = true;
 
 	return 0;
@@ -1588,6 +1590,7 @@ static void netc_port_disable(struct dsa_switch *ds, int port)
 	if (!np->enable)
 		return;
 
+	netc_port_disable_onestep(np);
 	clk_disable_unprepare(np->ref_clk);
 	np->enable = false;
 }
@@ -2441,6 +2444,7 @@ static void netc_mac_link_up(struct phylink_config *config,
 	netc_port_set_rx_pause(np, rx_pause);
 	netc_port_mac_tx_enable(np);
 	netc_port_mac_rx_enable(np);
+	netc_port_enable_onestep(np);
 }
 
 static void netc_mac_link_down(struct phylink_config *config,
@@ -2451,6 +2455,7 @@ static void netc_mac_link_down(struct phylink_config *config,
 	struct netc_port *np;
 
 	np = NETC_PORT(dp->ds, dp->index);
+	netc_port_disable_onestep(np);
 	netc_port_mac_rx_graceful_stop(np);
 	netc_port_mac_tx_graceful_stop(np);
 	netc_port_remove_dynamic_entries(np);
diff --git a/drivers/net/dsa/netc/netc_ptp.c b/drivers/net/dsa/netc/netc_ptp.c
index 2e743443ca03..1bc0f0956206 100644
--- a/drivers/net/dsa/netc/netc_ptp.c
+++ b/drivers/net/dsa/netc/netc_ptp.c
@@ -11,6 +11,15 @@
 
 #define NETC_NUM_TS_REQ_ID		16
 #define NETC_TSTAMP_TIMEOUT		(5 * HZ)
+#define NETC_MAX_STEP_OFFSET		0x1ff
+#define NETC_ONESTEP_QTH		512
+/* The 30-bit timestamp of the To_Port subtype 3 tag lets the hardware
+ * account for a single wrap, so the correction field of a one-step Sync
+ * frame is only correct if it is sent out within 2^30 ns after the
+ * software timestamp is read. Past this window the frame is beyond
+ * repair, and PM_SINGLE_STEP becomes safe to reprogram.
+ */
+#define NETC_ONESTEP_VALID_WINDOW	0x40000000 /* ns */
 
 static void netc_port_tstamp_timeout_work(struct work_struct *work)
 {
@@ -68,12 +77,276 @@ static int netc_get_ts_req_id(struct netc_port *np)
 	return ts_req_id;
 }
 
+static int netc_get_phc_time(struct netc_switch *priv, u64 *ns)
+{
+	if (unlikely(!priv->tmr_dev))
+		return -ENODEV;
+
+	return netc_timer_get_current_time(priv->tmr_dev, ns);
+}
+
+static void netc_port_set_onestep_control(struct netc_port *np,
+					  bool csum_update, int offset)
+{
+	u32 val;
+
+	val = PM_SINGLE_STEP_EN | FIELD_PREP(PM_SINGLE_STEP_OFFSET, offset);
+	if (csum_update)
+		val |= PM_SINGLE_STEP_CH;
+	netc_mac_port_wr(np, NETC_PM_SINGLE_STEP(0), val);
+}
+
+static void netc_port_program_onestep(struct netc_port *np,
+				      struct sk_buff *skb)
+{
+	u16 correction_offset = NETC_SKB_CB(skb)->correction_offset;
+	u16 tstamp_offset = NETC_SKB_CB(skb)->timestamp_offset;
+	u64 tstamp = NETC_SKB_CB(skb)->tstamp;
+	u8 *hdr = skb_mac_header(skb);
+	bool csum_update = false;
+	__be32 new_sec_l, new_ns;
+	__be16 new_sec_h;
+	u64 sec;
+	u32 ns;
+
+	/* Update originTimestamp field of Sync packet
+	 * - 48 bits seconds field
+	 * - 32 bits nanoseconds field
+	 */
+	sec = div_u64_rem(tstamp, NSEC_PER_SEC, &ns);
+	new_sec_h = htons((sec >> 32) & 0xffff);
+	new_sec_l = htonl(sec & 0xffffffff);
+	new_ns = htonl(ns);
+
+	if (NETC_SKB_CB(skb)->is_udp) {
+		__be32 old_sec_l, old_ns;
+		struct udphdr *uh;
+		__be16 old_sec_h;
+
+		if (skb->ip_summed == CHECKSUM_PARTIAL) {
+			csum_update = true;
+			goto update_timestamp;
+		}
+
+		if (unlikely(!skb_transport_header_was_set(skb)))
+			uh = (struct udphdr *)(hdr + tstamp_offset -
+					       sizeof(struct ptp_header) -
+					       sizeof(struct udphdr));
+		else
+			uh = udp_hdr(skb);
+
+		/* For IPv4, a UDP checksum of zero on the wire means "no
+		 * checksum". For IPv6, its UDP checksum is mandatory and
+		 * never zero.
+		 */
+		if (!uh->check)
+			goto update_timestamp;
+
+		old_sec_h = __get_unaligned_t(__be16, hdr + tstamp_offset);
+		old_sec_l = __get_unaligned_t(__be32, hdr + tstamp_offset + 2);
+		old_ns = __get_unaligned_t(__be32, hdr + tstamp_offset + 6);
+		inet_proto_csum_replace2(&uh->check, skb, old_sec_h,
+					 new_sec_h, false);
+		inet_proto_csum_replace4(&uh->check, skb, old_sec_l,
+					 new_sec_l, false);
+		inet_proto_csum_replace4(&uh->check, skb, old_ns,
+					 new_ns, false);
+		csum_update = true;
+	}
+
+update_timestamp:
+	__put_unaligned_t(__be16, new_sec_h, hdr + tstamp_offset);
+	__put_unaligned_t(__be32, new_sec_l, hdr + tstamp_offset + 2);
+	__put_unaligned_t(__be32, new_ns, hdr + tstamp_offset + 6);
+
+	netc_port_set_onestep_control(np, csum_update, correction_offset);
+}
+
+void netc_port_disable_onestep(struct netc_port *np)
+{
+	struct sk_buff_head free_list;
+
+	if (!dsa_port_is_user(np->dp))
+		return;
+
+	__skb_queue_head_init(&free_list);
+
+	spin_lock_bh(&np->onestep_lock);
+	skb_queue_splice_init(&np->onestep_queue, &free_list);
+	np->onestep_state = NETC_ONESTEP_PORT_INACTIVE;
+	spin_unlock_bh(&np->onestep_lock);
+
+	cancel_work_sync(&np->onestep_work);
+	__skb_queue_purge(&free_list);
+}
+
+void netc_port_enable_onestep(struct netc_port *np)
+{
+	if (!dsa_port_is_user(np->dp))
+		return;
+
+	spin_lock_bh(&np->onestep_lock);
+	np->onestep_state = NETC_ONESTEP_IDLE;
+	spin_unlock_bh(&np->onestep_lock);
+}
+
+static void netc_port_purge_onestep_queue(struct netc_port *np)
+{
+	struct sk_buff_head free_list;
+
+	__skb_queue_head_init(&free_list);
+
+	spin_lock_bh(&np->onestep_lock);
+	skb_queue_splice_init(&np->onestep_queue, &free_list);
+	spin_unlock_bh(&np->onestep_lock);
+
+	__skb_queue_purge(&free_list);
+}
+
+static void netc_port_onestep_work(struct work_struct *work)
+{
+	struct netc_port *np = container_of(work, struct netc_port,
+					    onestep_work);
+	struct netc_switch *priv = np->switch_priv;
+	struct netc_tagger_data *tagger_data;
+	struct sk_buff *clone = NULL;
+	struct sk_buff *skb = NULL;
+	int ts_req_id;
+	u64 tstamp;
+
+	spin_lock_bh(&np->onestep_lock);
+
+	if (unlikely(np->onestep_state == NETC_ONESTEP_PORT_INACTIVE))
+		goto purge_onestep_queue;
+
+skb_dequeue:
+	skb = __skb_dequeue(&np->onestep_queue);
+	if (!skb)
+		goto set_onestep_state_idle;
+
+	/* Clone is a ts_req_id token only; its payload is never read, so
+	 * sharing the buffer with the mutated original is fine.
+	 */
+	clone = skb_clone(skb, GFP_ATOMIC);
+	if (unlikely(!clone)) {
+		kfree_skb(skb);
+		goto skb_dequeue;
+	}
+
+	spin_lock_bh(&np->tstamp_lock);
+	ts_req_id = netc_get_ts_req_id(np);
+	if (unlikely(ts_req_id < 0)) {
+		spin_unlock_bh(&np->tstamp_lock);
+
+		/* Re-queuing the frame and immediately rescheduling the work
+		 * would busy-loop on system_percpu_wq and burn CPU until an
+		 * ID is freed, so drop this frame and move on to the next one
+		 * in the queue instead.
+		 */
+		np->onestep_state = NETC_ONESTEP_SCHEDULED;
+		schedule_work(&np->onestep_work);
+
+		goto onestep_unlock;
+	}
+
+	/* PHC is unavailable, drop the whole queue */
+	if (unlikely(netc_get_phc_time(priv, &tstamp))) {
+		spin_unlock_bh(&np->tstamp_lock);
+		goto set_onestep_state_idle;
+	}
+
+	NETC_SKB_CB(skb)->tstamp = tstamp;
+	NETC_SKB_CB(skb)->ts_req_id = ts_req_id;
+	NETC_SKB_CB(skb)->ptp_flag = NETC_PTP_FLAG_ONESTEP;
+	NETC_SKB_CB(clone)->ts_req_id = ts_req_id;
+	NETC_SKB_CB(clone)->ptp_tx_time = jiffies_64;
+	NETC_SKB_CB(clone)->ptp_flag = NETC_PTP_FLAG_ONESTEP;
+	np->onestep_tx_time = NETC_SKB_CB(clone)->ptp_tx_time;
+	np->onestep_ts_req_id = ts_req_id;
+
+	__skb_queue_tail(&np->tstamp_queue, clone);
+	if (!delayed_work_pending(&np->tstamp_timeout_work))
+		schedule_delayed_work(&np->tstamp_timeout_work,
+				      NETC_TSTAMP_TIMEOUT);
+
+	spin_unlock_bh(&np->tstamp_lock);
+
+	np->onestep_state = NETC_ONESTEP_IN_FLIGHT;
+	spin_unlock_bh(&np->onestep_lock);
+
+	netc_port_program_onestep(np, skb);
+	tagger_data = priv->ds->tagger_data;
+	tagger_data->onestep_sync_xmit(skb, np->dp->user);
+
+	return;
+
+set_onestep_state_idle:
+	np->onestep_state = NETC_ONESTEP_IDLE;
+purge_onestep_queue:
+	__skb_queue_purge(&np->onestep_queue);
+onestep_unlock:
+	spin_unlock_bh(&np->onestep_lock);
+	kfree_skb(skb);
+	kfree_skb(clone);
+}
+
+static bool netc_onestep_timeout(struct netc_port *np)
+{
+	u64 expire_time;
+
+	/* Use monotonic jiffies_64, as the PHC may be stepped backwards.
+	 * Add one tick since the ns-to-jiffies conversion rounds down, so
+	 * the software window is never shorter than the hardware window.
+	 */
+	expire_time = np->onestep_tx_time + 1 +
+		      nsecs_to_jiffies64(NETC_ONESTEP_VALID_WINDOW);
+	if (np->onestep_state == NETC_ONESTEP_IN_FLIGHT &&
+	    time_after64(jiffies_64, expire_time))
+		return true;
+
+	return false;
+}
+
+void netc_port_onestep_sync_enqueue(struct dsa_switch *ds, int port,
+				    struct sk_buff *skb)
+{
+	struct netc_port *np = NETC_PORT(ds, port);
+
+	spin_lock_bh(&np->onestep_lock);
+	if (unlikely(np->onestep_state == NETC_ONESTEP_PORT_INACTIVE)) {
+		kfree_skb(skb);
+		goto onestep_unlock;
+	}
+
+	if (unlikely(skb_queue_len(&np->onestep_queue) >= NETC_ONESTEP_QTH)) {
+		dev_dbg_ratelimited(np->switch_priv->dev,
+				    "The onestep_queue of port %d is full\n",
+				    port);
+		kfree_skb(skb);
+		goto onestep_unlock;
+	}
+
+	__skb_queue_tail(&np->onestep_queue, skb);
+	if (likely(np->onestep_state == NETC_ONESTEP_IDLE) ||
+	    netc_onestep_timeout(np)) {
+		np->onestep_state = NETC_ONESTEP_SCHEDULED;
+		schedule_work(&np->onestep_work);
+	}
+
+onestep_unlock:
+	spin_unlock_bh(&np->onestep_lock);
+}
+
 int netc_port_ptp_init(struct netc_port *np)
 {
 	/* Initialize to invalid entry IDs */
 	for (int i = 0; i < NETC_PTP_MAX; i++)
 		np->ptp_ipft_eid[i] = NTMP_NULL_ENTRY_ID;
 
+	spin_lock_init(&np->onestep_lock);
+	__skb_queue_head_init(&np->onestep_queue);
+	INIT_WORK(&np->onestep_work, netc_port_onestep_work);
+
 	spin_lock_init(&np->tstamp_lock);
 	__skb_queue_head_init(&np->tstamp_queue);
 	INIT_DELAYED_WORK(&np->tstamp_timeout_work,
@@ -116,7 +389,8 @@ int netc_get_ts_info(struct dsa_switch *ds, int port,
 				 SOF_TIMESTAMPING_RX_HARDWARE |
 				 SOF_TIMESTAMPING_RAW_HARDWARE;
 
-	info->tx_types = BIT(HWTSTAMP_TX_OFF) | BIT(HWTSTAMP_TX_ON);
+	info->tx_types = BIT(HWTSTAMP_TX_OFF) | BIT(HWTSTAMP_TX_ON) |
+			 BIT(HWTSTAMP_TX_ONESTEP_SYNC);
 
 	info->rx_filters = BIT(HWTSTAMP_FILTER_NONE) |
 			   BIT(HWTSTAMP_FILTER_PTP_V2_EVENT) |
@@ -342,6 +616,7 @@ int netc_port_hwtstamp_set(struct dsa_switch *ds, int port,
 		return -EOPNOTSUPP;
 
 	switch (config->tx_type) {
+	case HWTSTAMP_TX_ONESTEP_SYNC:
 	case HWTSTAMP_TX_ON:
 	case HWTSTAMP_TX_OFF:
 		break;
@@ -379,6 +654,9 @@ int netc_port_hwtstamp_set(struct dsa_switch *ds, int port,
 	}
 
 	WRITE_ONCE(np->ptp_tx_type, config->tx_type);
+	if (config->tx_type != HWTSTAMP_TX_ONESTEP_SYNC)
+		netc_port_purge_onestep_queue(np);
+
 	config->rx_filter = rx_filter;
 
 	return 0;
@@ -395,6 +673,66 @@ int netc_port_hwtstamp_get(struct dsa_switch *ds, int port,
 	return 0;
 }
 
+static void netc_port_prepare_onestep_sync(struct netc_port *np,
+					   struct sk_buff *skb,
+					   u32 ptp_class, bool *twostep)
+{
+	struct netc_switch *priv = np->switch_priv;
+	u16 correction_offset, tstamp_offset;
+	struct ptp_header *ptp_hdr;
+	u8 msg_type, twostep_flag;
+	bool is_udp = false;
+	u32 pkt_type;
+	u8 *pkt_hdr;
+
+	if (unlikely(skb_linearize_cow(skb)))
+		goto set_ptp_flag_drop;
+
+	ptp_hdr = ptp_parse_header(skb, ptp_class);
+	if (unlikely(!ptp_hdr))
+		goto set_ptp_flag_drop;
+
+	msg_type = ptp_get_msgtype(ptp_hdr, ptp_class);
+	twostep_flag = ptp_hdr->flag_field[0] & 0x2;
+	if (msg_type != PTP_MSGTYPE_SYNC || twostep_flag != 0) {
+		*twostep = true;
+		return;
+	}
+
+	pkt_hdr = skb_mac_header(skb);
+	correction_offset = (u8 *)&ptp_hdr->correction - pkt_hdr;
+	tstamp_offset = (u8 *)ptp_hdr + sizeof(*ptp_hdr) - pkt_hdr;
+
+	/* Ensure that the entire originTimestamp field is present in the
+	 * linear buffer of the skb and the correction_offset must be within
+	 * the hardware capability.
+	 */
+	if (unlikely(tstamp_offset + 10 > skb_headlen(skb) ||
+		     correction_offset > NETC_MAX_STEP_OFFSET))
+		goto set_ptp_flag_drop;
+
+	pkt_type = ptp_class & PTP_CLASS_PMASK;
+	if (pkt_type == PTP_CLASS_IPV4 || pkt_type == PTP_CLASS_IPV6)
+		is_udp = true;
+
+	NETC_SKB_CB(skb)->correction_offset = correction_offset;
+	NETC_SKB_CB(skb)->timestamp_offset = tstamp_offset;
+	NETC_SKB_CB(skb)->is_udp = is_udp;
+	NETC_SKB_CB(skb)->ptp_flag = NETC_PTP_FLAG_ONESTEP;
+
+	return;
+
+set_ptp_flag_drop:
+	/* Drop instead of falling back to two-step: if it is a Sync,
+	 * one-step offload will not be executed, the timestamp in the
+	 * frame is inaccurate, which may affect PTP synchronization.
+	 */
+	NETC_SKB_CB(skb)->ptp_flag = NETC_PTP_FLAG_DROP;
+	dev_dbg_ratelimited(priv->dev,
+			    "Port %d: PTP frame dropped in error\n",
+			    np->dp->index);
+}
+
 static void netc_port_prepare_twostep(struct netc_port *np,
 				      struct sk_buff *nskb)
 {
@@ -449,6 +787,21 @@ void netc_port_txtstamp_handler(struct dsa_switch *ds, int port,
 		dev_dbg_ratelimited(np->switch_priv->dev,
 				    "Port %d ts_req_id %u which seems lost\n",
 				    port, ts_req_id);
+
+		return;
+	}
+
+	if (NETC_SKB_CB(skb_match)->ptp_flag == NETC_PTP_FLAG_ONESTEP) {
+		spin_lock_bh(&np->onestep_lock);
+		if (likely(np->onestep_state == NETC_ONESTEP_IN_FLIGHT &&
+			   np->onestep_ts_req_id == ts_req_id) ||
+		    np->onestep_state == NETC_ONESTEP_IDLE) {
+			np->onestep_state = NETC_ONESTEP_SCHEDULED;
+			schedule_work(&np->onestep_work);
+		}
+		spin_unlock_bh(&np->onestep_lock);
+		consume_skb(skb_match);
+
 		return;
 	}
 
@@ -472,6 +825,7 @@ bool netc_port_rxtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb,
 void netc_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
 {
 	struct netc_port *np = NETC_PORT(ds, port);
+	bool twostep = false;
 	u32 ptp_class;
 	int tx_type;
 
@@ -487,6 +841,9 @@ void netc_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
 		return;
 
 	tx_type = READ_ONCE(np->ptp_tx_type);
-	if (tx_type == HWTSTAMP_TX_ON)
+	if (tx_type == HWTSTAMP_TX_ONESTEP_SYNC)
+		netc_port_prepare_onestep_sync(np, skb, ptp_class, &twostep);
+
+	if (tx_type == HWTSTAMP_TX_ON || twostep)
 		netc_port_prepare_twostep(np, skb);
 }
diff --git a/drivers/net/dsa/netc/netc_switch.h b/drivers/net/dsa/netc/netc_switch.h
index a1f4b1bc04cb..624cf323c9a6 100644
--- a/drivers/net/dsa/netc/netc_switch.h
+++ b/drivers/net/dsa/netc/netc_switch.h
@@ -87,6 +87,13 @@ enum netc_host_reason {
 	NETC_HR_PTP_TRAP   = 9,
 };
 
+enum netc_onestep_state {
+	NETC_ONESTEP_IDLE	= 0,
+	NETC_ONESTEP_SCHEDULED,
+	NETC_ONESTEP_IN_FLIGHT,
+	NETC_ONESTEP_PORT_INACTIVE,
+};
+
 struct netc_port {
 	void __iomem *iobase;
 	struct netc_switch *switch_priv;
@@ -102,6 +109,17 @@ struct netc_port {
 	u16 pvid;
 	u32 ipft_hf_eid; /* Must be initialized to NTMP_NULL_ENTRY_ID */
 
+	/* Serialize access to onestep_queue, onestep_state and
+	 * onestep_tx_time
+	 */
+	spinlock_t onestep_lock;
+	u8 onestep_state;
+	u8 onestep_ts_req_id;
+	u64 onestep_tx_time;
+	/* skb queue for one-step Sync frames */
+	struct sk_buff_head onestep_queue;
+	struct work_struct onestep_work;
+
 	/* Serialize access to tstamp_queue */
 	spinlock_t tstamp_lock;
 	/* skb queue for TX timestamp frames */
@@ -213,6 +231,7 @@ static inline void netc_del_vlan_entry(struct netc_vlan_entry *entry)
 }
 
 int netc_switch_platform_probe(struct netc_switch *priv);
+void netc_mac_port_wr(struct netc_port *np, u32 reg, u32 val);
 
 /* ethtool APIs */
 void netc_port_get_pause_stats(struct dsa_switch *ds, int port,
@@ -244,5 +263,9 @@ void netc_port_txtstamp_handler(struct dsa_switch *ds, int port,
 bool netc_port_rxtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb,
 			unsigned int type);
 void netc_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb);
+void netc_port_disable_onestep(struct netc_port *np);
+void netc_port_enable_onestep(struct netc_port *np);
+void netc_port_onestep_sync_enqueue(struct dsa_switch *ds, int port,
+				    struct sk_buff *skb);
 
 #endif
diff --git a/drivers/net/dsa/netc/netc_switch_hw.h b/drivers/net/dsa/netc/netc_switch_hw.h
index 1404ae41c7bc..37d1dd7ec2c7 100644
--- a/drivers/net/dsa/netc/netc_switch_hw.h
+++ b/drivers/net/dsa/netc/netc_switch_hw.h
@@ -203,6 +203,11 @@ enum netc_stg_stage {
 #define   SSP_10M			1
 #define   SSP_1G			2
 
+#define NETC_PM_SINGLE_STEP(a)		(0x10c0 + (a) * 0x400)
+#define  PM_SINGLE_STEP_CH		BIT(6)
+#define  PM_SINGLE_STEP_OFFSET		GENMASK(15, 7)
+#define  PM_SINGLE_STEP_EN		BIT(31)
+
 /* Port MAC 0/1 Receive Ethernet Octets Counter */
 #define NETC_PM_REOCT(a)		(0x1100 + (a) * 0x400)
 
diff --git a/include/linux/dsa/tag_netc.h b/include/linux/dsa/tag_netc.h
index 5a567af20094..baf0f93bd994 100644
--- a/include/linux/dsa/tag_netc.h
+++ b/include/linux/dsa/tag_netc.h
@@ -10,7 +10,9 @@
 #include <net/dsa.h>
 
 #define NETC_TAG_MAX_LEN			14
+#define NETC_PTP_FLAG_ONESTEP			BIT(0)
 #define NETC_PTP_FLAG_TWOSTEP			BIT(1)
+#define NETC_PTP_FLAG_DROP			BIT(2)
 
 struct netc_skb_cb {
 	u64 ptp_tx_time;
@@ -18,6 +20,9 @@ struct netc_skb_cb {
 	bool rx_tstamp_valid;
 	u8 ptp_flag;
 	u8 ts_req_id;
+	bool is_udp;
+	u16 correction_offset;
+	u16 timestamp_offset;
 };
 
 #define NETC_SKB_CB(skb)	((struct netc_skb_cb *)((skb)->cb))
@@ -26,10 +31,22 @@ struct netc_skb_cb {
  * struct netc_tagger_data - NETC tagger/switch-driver shared operations
  * @txtstamp_handler: Called by the tagger when a two-step transmit timestamp
  *	response is received, to deliver the timestamp to the switch driver.
+ * @onestep_sync_enqueue: Called from the tagger xmit path for a one-step Sync
+ *	frame. The switch driver takes ownership of the skb and queues it for
+ *	deferred transmission from process context, where the shared
+ *	PM_SINGLE_STEP register can be programmed and the PTP timer read. The
+ *	tagger must not touch the skb after this call and returns NULL to
+ *	dsa_user_xmit().
+ * @onestep_sync_xmit: Called by the switch driver to transmit a deferred
+ *	one-step Sync frame directly to the conduit, bypassing dsa_user_xmit().
  */
 struct netc_tagger_data {
 	void (*txtstamp_handler)(struct dsa_switch *ds, int port,
 				 u8 ts_req_id, u64 ts);
+	void (*onestep_sync_enqueue)(struct dsa_switch *ds, int port,
+				     struct sk_buff *skb);
+	netdev_tx_t (*onestep_sync_xmit)(struct sk_buff *skb,
+					 struct net_device *ndev);
 };
 
 #endif
diff --git a/net/dsa/tag_netc.c b/net/dsa/tag_netc.c
index 6b492451d092..c1c0ffee75c1 100644
--- a/net/dsa/tag_netc.c
+++ b/net/dsa/tag_netc.c
@@ -18,6 +18,8 @@
 #define NETC_TAG_TP_SUBTYPE0		0
 /* SubType2: Request to perform two-step timestamping */
 #define NETC_TAG_TP_SUBTYPE2		2
+/* SubType3: Request to perform both one-step and two-step timestamping */
+#define NETC_TAG_TP_SUBTYPE3		3
 
 /* To_Host NXP switch tag */
 #define NETC_TAG_TO_HOST		2
@@ -32,6 +34,7 @@
 #define NETC_TAG_FORWARD_LEN		6
 #define NETC_TAG_TP_SUBTYPE0_LEN	6
 #define NETC_TAG_TP_SUBTYPE2_LEN	6
+#define NETC_TAG_TP_SUBTYPE3_LEN	10
 #define NETC_TAG_TH_SUBTYPE0_LEN	6
 #define NETC_TAG_TH_SUBTYPE1_LEN	14
 #define NETC_TAG_TH_SUBTYPE2_LEN	14
@@ -44,6 +47,7 @@
 #define NETC_TAG_SWITCH			GENMASK(2, 0)
 #define NETC_TAG_PORT			GENMASK(7, 3)
 #define NETC_TAG_TS_REQ_ID		GENMASK(3, 0)
+#define NETC_TAG_TIMESTAMP		GENMASK(29, 0)
 
 struct netc_tag_cmn {
 	__be16 tpid;
@@ -57,6 +61,12 @@ struct netc_tag_tp_subtype2 {
 	u8 ts_req_id;
 } __packed;
 
+struct netc_tag_tp_subtype3 {
+	struct netc_tag_cmn cmn;
+	u8 ts_req_id;
+	__be32 timestamp;
+} __packed;
+
 struct netc_tag_th_subtype1 {
 	struct netc_tag_cmn cmn;
 	u8 host_reason;
@@ -129,17 +139,69 @@ static void netc_fill_tp_tag_subtype2(struct sk_buff *skb,
 	tag->ts_req_id = FIELD_PREP(NETC_TAG_TS_REQ_ID, ts_req_id);
 }
 
+static void netc_fill_tp_tag_subtype3(struct sk_buff *skb,
+				      struct net_device *ndev)
+{
+	u32 ts = FIELD_PREP(NETC_TAG_TIMESTAMP, NETC_SKB_CB(skb)->tstamp);
+	u8 ts_req_id = NETC_SKB_CB(skb)->ts_req_id;
+	struct netc_tag_tp_subtype3 *tag;
+
+	tag = netc_fill_common_tp_tag(skb, ndev, NETC_TAG_TP_SUBTYPE3,
+				      NETC_TAG_TP_SUBTYPE3_LEN);
+	tag->ts_req_id = FIELD_PREP(NETC_TAG_TS_REQ_ID, ts_req_id);
+	tag->timestamp = htonl(ts);
+}
+
+static void netc_onestep_sync_enqueue(struct sk_buff *skb,
+				      struct net_device *ndev)
+{
+	struct dsa_port *dp = dsa_user_to_port(ndev);
+	struct netc_tagger_data *tagger_data;
+
+	tagger_data = dp->ds->tagger_data;
+	if (unlikely(!tagger_data->onestep_sync_enqueue)) {
+		kfree_skb(skb);
+		return;
+	}
+
+	/* Hand the one-step Sync to the switch driver, which takes ownership
+	 * and queues it for deferred transmission from its work. The tagger
+	 * must not touch the skb after this point.
+	 */
+	tagger_data->onestep_sync_enqueue(dp->ds, dp->index, skb);
+}
+
+static netdev_tx_t netc_onestep_sync_xmit(struct sk_buff *skb,
+					  struct net_device *dev)
+{
+	netc_fill_tp_tag_subtype3(skb, dev);
+
+	return dsa_enqueue_skb(skb, dev);
+}
+
 static struct sk_buff *netc_xmit(struct sk_buff *skb,
 				 struct net_device *ndev)
 {
 	u8 ptp_flag = NETC_SKB_CB(skb)->ptp_flag;
 
 	/* Fast path: the overwhelming majority of frames are not PTP frames */
-	if (likely(!ptp_flag))
+	if (likely(!ptp_flag)) {
 		netc_fill_tp_tag_subtype0(skb, ndev);
-	else
-		/* ptp_flag == NETC_PTP_FLAG_TWOSTEP */
+	} else if (ptp_flag == NETC_PTP_FLAG_TWOSTEP) {
 		netc_fill_tp_tag_subtype2(skb, ndev);
+	} else if (ptp_flag == NETC_PTP_FLAG_ONESTEP) {
+		/* The switch driver takes ownership of the one-step Sync and
+		 * queues it for deferred TX; the deferred work tags it subtype
+		 * 3 and transmits it directly to the conduit. Return NULL so
+		 * dsa_user_xmit() stops processing this skb.
+		 */
+		netc_onestep_sync_enqueue(skb, ndev);
+		skb = NULL;
+	} else {
+		/* NETC_PTP_FLAG_DROP */
+		kfree_skb(skb);
+		skb = NULL;
+	}
 
 	return skb;
 }
@@ -326,6 +388,7 @@ static int netc_connect(struct dsa_switch *ds)
 	if (!tagger_data)
 		return -ENOMEM;
 
+	tagger_data->onestep_sync_xmit = netc_onestep_sync_xmit;
 	ds->tagger_data = tagger_data;
 
 	return 0;
-- 
2.34.1

Re: [PATCH v4 net-next 3/8] ptp: netc: export netc_timer_get_current_time() for cross-driver use

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 3

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] netc_timer_get_current_time()
  (drivers/ptp/ptp_netc.c:1050-1082) takes the module-global…
- [Medium] netc_timer_get_current_time() treats membership in
  netc_timer_list as proof that TMR_CUR_TIME can be read, and returns 0…
- [Low] Concern that the exported netc_timer_get_current_time() and its
  list/lock infrastructure have no in-tree consumer.
- [Low] Concern that netc_timer_get_current_time() keys its lookup on an
  unreferenced caller-supplied 'struct pci_dev *' (tmp->pdev == pdev at…
- [Low] include/linux/fsl/netc_global.h — a header that otherwise
  contains only trivial MMIO accessor inlines…

--

ptp: netc: export netc_timer_get_current_time() for cross-driver use

This adds netc_timer_get_current_time() to the NETC Timer driver and exports
it so the NETC Switch driver, which is a separate PCIe function with its own
driver, can read TMR_CUR_TIME. Each probed Timer is tracked in a
module-private list keyed by its pci_dev, and one global spinlock covers both
the list lookup and the register read so that an unbind cannot free priv
under an in-flight read.

A few questions about the new helper and the header change below.

> diff --git a/drivers/ptp/ptp_netc.c b/drivers/ptp/ptp_netc.c
> index aa9be8e2a630..1211aa9a37d3 100644
> --- a/drivers/ptp/ptp_netc.c
> +++ b/drivers/ptp/ptp_netc.c

[ ... ]

> @@ -123,8 +123,12 @@ struct netc_timer {
>  	u8 fs_alarm_num;
>  	u8 fs_alarm_bitmap;
>  	struct netc_pp pp[NETC_TMR_FIPER_NUM]; /* periodic pulse */
> +	struct list_head node;
>  };
>  
> +static LIST_HEAD(netc_timer_list);
> +static DEFINE_SPINLOCK(netc_timer_list_lock);
> +
>  #define netc_timer_rd(p, o)		netc_read((p)->base + (o))
>  #define netc_timer_wr(p, o, v)		netc_write((p)->base + (o), v)
>  
> @@ -985,6 +989,10 @@ static int netc_timer_probe(struct pci_dev *pdev,
>  
>  	enable_irq(priv->irq);
>  
> +	spin_lock_bh(&netc_timer_list_lock);
> +	list_add(&priv->node, &netc_timer_list);
> +	spin_unlock_bh(&netc_timer_list_lock);
> +
>  	return 0;
>  
>  free_msix_irq:
> @@ -999,6 +1007,10 @@ static void netc_timer_remove(struct pci_dev *pdev)
>  {
>  	struct netc_timer *priv = pci_get_drvdata(pdev);
>  
> +	spin_lock_bh(&netc_timer_list_lock);
> +	list_del(&priv->node);
> +	spin_unlock_bh(&netc_timer_list_lock);
> +
>  	disable_irq(priv->irq);
>  	ptp_clock_unregister(priv->clock);
>  	netc_timer_wr(priv, NETC_TMR_TEMASK, 0);

[ ... ]

> @@ -1021,5 +1033,54 @@ static struct pci_driver netc_timer_driver = {
>  };
>  module_pci_driver(netc_timer_driver);
>  
> +/**
> + * netc_timer_get_current_time - read the current PTP time from the NETC Timer
> + * @pdev: PCI device of the NETC Timer
> + * @ns: Output, the current PTP clock time in nanoseconds
> + *
> + * Read TMR_CUR_TIME from the NETC Timer bound to @pdev. The lookup and read
> + * run under netc_timer_list_lock, so the Timer cannot be unbound and its priv
> + * freed during the read.
> + *
> + * Context: Process or softirq context. Must not be called from hardirq.
> + *
> + * Return: 0 on success, -ENODEV if the Timer is not present (not yet probed
> + *         or already removed).
> + */
> +int netc_timer_get_current_time(struct pci_dev *pdev, u64 *ns)
> +{
> +	struct netc_timer *priv = NULL;
> +	struct netc_timer *tmp;
> +	unsigned long flags;
> +	int err = 0;
> +
> +	/* Serialize against driver unbind, so holding it here ensures that
> +	 * priv remains valid for the entire duration of the register read.
> +	 */
> +	spin_lock_bh(&netc_timer_list_lock);

[Severity: Medium]
Is it necessary to hold the module-global netc_timer_list_lock across the
MMIO access, rather than only across the lookup?

priv->lock is taken from hardirq context with a plain spin_lock() in
netc_timer_isr(), and that handler can spin in the register drain loop in
netc_timer_handle_etts_event():

drivers/ptp/ptp_netc.c:netc_timer_handle_etts_event() {
	while (netc_timer_rd(priv, NETC_TMR_STAT) & TMR_STAT_ETS_VLD(index))
		etts = netc_timer_rd64(priv, NETC_TMR_ETTS_L(index));
}

So while CPU A sits in netc_timer_get_current_time() waiting on
spin_lock_irqsave(&priv->lock) with softirqs disabled and the global list lock
held, CPU B can be inside netc_timer_isr() holding priv->lock for a period
bounded only by hardware. During that window every other Timer's probe(),
remove() and every other time query is serialized behind
netc_timer_list_lock. The same applies if the target function is unreachable
and the non-posted read stalls until the PCIe completion timeout.

Would an RCU-protected lookup (with the priv free deferred in remove()), or
taking a reference on the Timer, give the same lifetime guarantee without
holding a global lock across the register access?

Related to that: the BH-only requirement makes the exported symbol illegal
from hardirq or with interrupts disabled, but that constraint appears only in
the kerneldoc and is not visible at the prototype in
include/linux/fsl/netc_global.h, and nothing but an eventual WARN from
__local_bh_enable_ip() catches a violation. Is there a reason not to drop the
context restriction by using a lookup scheme that does not disable BH?

> +
> +	list_for_each_entry(tmp, &netc_timer_list, node) {
> +		if (tmp->pdev == pdev) {
> +			priv = tmp;
> +			break;
> +		}
> +	}

[Severity: Low]
This isn't a bug, but the lookup is a bare pointer comparison on a
caller-supplied struct pci_dev *, so its soundness depends on the caller
holding a reference on @pdev. The in-tree consumer does hold one
(netc_get_ptp_timer() obtains it with pci_get_domain_bus_and_slot() and
drops it with pci_dev_put()), so nothing is broken today.

Could the kerneldoc and the exported prototype state that the caller must
hold a reference on @pdev? Without it, a future caller that caches a raw
pci_dev pointer has no hint that a freed and recycled pointer could match a
different Timer's entry.

> +
> +	if (!priv) {
> +		err = -ENODEV;
> +		goto netc_timer_list_unlock;
> +	}
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	*ns = netc_timer_cur_time_read(priv);
> +	spin_unlock_irqrestore(&priv->lock, flags);

[Severity: Medium]
Can this return 0 together with a meaningless timestamp when the Timer
function is not accessible?

List membership only proves the pci_dev is bound to this driver: the entry is
added at the end of netc_timer_probe() and removed only in
netc_timer_remove(). netc_timer_driver registers no PM callbacks, so system
suspend takes the PCI core default path and the function is disabled and put
into D3 while the list entry stays live, and nothing re-programs
TMR_CTRL[TE], CK_SEL, TMR_ADD or TMR_CNT on resume. The driver already
documents the constraint in netc_timer_init():

drivers/ptp/ptp_netc.c:netc_timer_init() {
	/* Software must enable timer first and the clock selected must be
	 * active, otherwise, the registers which are in the timer clock
	 * domain are not accessible.
	 */
}

The same holds for a surprise-removed function or one contained by DPC/AER,
where the read returns all-ones. In those cases the helper still reports
success, and the switch side consumes the value verbatim as the one-step Sync
originTimestamp in netc_port_onestep_work():

drivers/net/dsa/netc/netc_ptp.c:netc_port_onestep_work() {
	NETC_SKB_CB(skb)->tstamp = tstamp;
}

Would it make sense to gate the read on pdev->current_state == PCI_D0 and
!pci_channel_offline(pdev), or on TMR_CTRL[TE], and return an error instead
of a successful-looking value? The missing PM support is not introduced by
this patch, but the new export is what turns an inaccessible-device read into
a timestamp handed to another driver.

> +
> +netc_timer_list_unlock:
> +	spin_unlock_bh(&netc_timer_list_lock);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(netc_timer_get_current_time);
> +
>  MODULE_DESCRIPTION("NXP NETC Timer PTP Driver");
>  MODULE_LICENSE("Dual BSD/GPL");
> diff --git a/include/linux/fsl/netc_global.h b/include/linux/fsl/netc_global.h
> index 5b8ff528d369..d4a26c17f99a 100644
> --- a/include/linux/fsl/netc_global.h
> +++ b/include/linux/fsl/netc_global.h
> @@ -6,6 +6,7 @@
>  
>  #include <linux/io.h>
>  #include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/pci.h>

[Severity: Low]
This isn't a bug, but is the full <linux/pci.h> needed here? The prototype
and the stub only name struct pci_dev *, so a forward declaration of
struct pci_dev plus <linux/errno.h> for -ENODEV would be enough.

netc_global.h otherwise holds only the netc_read()/netc_write()/netc_read64()
inlines, and it is included by drivers/net/dsa/netc/netc_switch.h,
drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c,
drivers/net/ethernet/freescale/enetc/ntmp.c and drivers/ptp/ptp_netc.c, so
all four now pull in the PCI API.

Would a separate header such as include/linux/fsl/netc_timer.h be a better
home for an inter-driver service API than the register accessor header?

>  
>  static inline u32 netc_read(void __iomem *reg)
>  {
> @@ -22,4 +23,13 @@ static inline u64 netc_read64(void __iomem *reg)
>  	return ioread64(reg);
>  }
>  
> +#if IS_REACHABLE(CONFIG_PTP_NETC_V4_TIMER)
> +int netc_timer_get_current_time(struct pci_dev *pdev, u64 *ns);
> +#else
> +static inline int netc_timer_get_current_time(struct pci_dev *pdev, u64 *ns)
> +{
> +	return -ENODEV;
> +}
> +#endif
> +
>  #endif

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918072852.501420-1-wei.fang%40oss.nxp.com

Re: [PATCH v4 net-next 5/8] net: dsa: netc: check return value of ntmp_ipft_delete_entry()

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 3

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] The commit message's impact assessment is factually wrong for
  the very path this patch reorders.
- [Medium] netc_port_set_host_flood()
  (drivers/net/dsa/netc/netc_main.c:1807-1821) now destroys the working…
- [Low] The patch fixes a real, user-visible defect (a failed
  ntmp_ipft_delete_entry() during host-flood rule replacement left an…
- [Low] The new error message added in netc_port_bridge_join() prints the
  port index with %u although `port` is declared `int` by the…
- [Low] netc_port_set_host_flood() now deletes the existing host-flood
  IPFT entry before installing the replacement (netc_main.c:1807-1821),…

--

net: dsa: netc: check return value of ntmp_ipft_delete_entry()

This makes netc_port_remove_host_flood() propagate the error from
ntmp_ipft_delete_entry() and only clear np->ipft_hf_eid, np->uc, np->mc
and NETC_PIPFCR once the entry has actually been removed.  As a result
netc_port_set_host_flood() now deletes the old IPFT host flood entry
before adding the new one, and netc_port_bridge_join() removes the rule
early and restores it on the error path.

A few questions below about the changelog and about the new ordering.

> net: dsa: netc: check return value of ntmp_ipft_delete_entry()
>
> ntmp_ipft_delete_entry() may fail, but the driver ignored its return
> value, so a failed deletion left the IPFT entry untracked and
> impossible to clean up later. This is harmless today because the port
> also disables its ingress port filter table lookup, so the leftover
> entry is never matched.

[Severity: Medium]
Is the "harmless today" statement accurate for the replacement path that
this patch reorders?

In the pre-patch netc_port_set_host_flood(), the new rule was added
first, which sets np->ipft_hf_eid to the new entry ID and writes
NETC_PIPFCR = PIPFCR_EN, and only afterwards was the old ID deleted:

	old_entry_id = np->ipft_hf_eid;

	if (netc_port_add_host_flood_rule(np, uc, mc)) {
	...
	/* Remove the old host flood entry */
	netc_port_remove_host_flood(np, old_entry_id);

The old helper only disabled the lookup when the deleted ID matched the
tracked one:

	if (np->ipft_hf_eid == entry_id)
		disable_host_flood = true;

Since np->ipft_hf_eid had already been updated to the new entry, that
condition was false here, so NETC_PIPFCR = 0 was never written.  Doesn't
that mean a failed delete left an active, untracked IPFT redirect entry
with the lookup still enabled, rather than an entry that "is never
matched"?

For the promisc to allmulti-only transition (uc=1,mc=1 to uc=0,mc=1) the
stale entry has an empty DMAC mask, so it matches every frame from that
source port and redirects it to the host.  Would the port then stay
effectively promiscuous after userspace disabled promiscuous mode, with
the ternary IPFT entry leaked permanently because its ID is no longer
recorded anywhere?

The delete can fail in practice: ntmp_ipft_delete_entry() ->
ntmp_delete_entry_by_id() -> ntmp_alloc_data_mem() can return -ENOMEM
from dma_alloc_coherent(GFP_KERNEL), and netc_xmit_ntmp_cmd() can return
-EBUSY, -ETIMEDOUT (read_poll_timeout() on cbdr->regs.cir) or -EIO.

Could the changelog be updated to describe the actual impact?

> Signed-off-by: Wei Fang <wei.fang@nxp.com>

[Severity: Low]
Should this carry a Fixes: tag?  The ignored return value together with
the add-before-delete ordering came in with commit 46d6407692c80b258ecba3af831130a6f6e2feea ("net: dsa: netc: add FDB,
STP, MTU, port setup and host flooding support"), which is already in
the tree, and the effect described above (a port keeping a redirect the
current RX flags no longer request, plus a leaked TCAM entry) is user
visible.

> diff --git a/drivers/net/dsa/netc/netc_main.c b/drivers/net/dsa/netc/netc_main.c
> index c590931a4e1b..082ed9405868 100644
> --- a/drivers/net/dsa/netc/netc_main.c
> +++ b/drivers/net/dsa/netc/netc_main.c

[ ... ]

> @@ -1761,32 +1755,31 @@ static int netc_port_add_host_flood_rule(struct netc_port *np,
>  	return err;
>  }
>  
> -static void netc_port_remove_host_flood(struct netc_port *np, u32 entry_id)
> +static int netc_port_remove_host_flood(struct netc_port *np)
>  {
>  	struct netc_switch *priv = np->switch_priv;
> -	bool disable_host_flood = false;
> +	u32 entry_id = np->ipft_hf_eid;
> +	int err;
>  
>  	if (entry_id == NTMP_NULL_ENTRY_ID)
> -		return;
> +		return 0;
>  
> -	if (np->ipft_hf_eid == entry_id)
> -		disable_host_flood = true;
> +	err = ntmp_ipft_delete_entry(&priv->ntmp, entry_id);
> +	if (err)
> +		return err;
>  
> -	ntmp_ipft_delete_entry(&priv->ntmp, entry_id);
> +	np->ipft_hf_eid = NTMP_NULL_ENTRY_ID;
> +	np->uc = false;
> +	np->mc = false;
> +	netc_port_wr(np, NETC_PIPFCR, 0);

[ ... ]

> @@ -1803,19 +1801,24 @@ static void netc_port_set_host_flood(struct dsa_switch *ds, int port,
>  		return;
>  
>  	/* IPFT does not support in-place updates to the KEYE element,
> -	 * we need to add a new entry and then delete the old one. So
> -	 * save the old entry ID first.
> +	 * we need to delete the old one and then add the new rule. If
> +	 * the deletion fails, return immediately.
>  	 */
> -	old_entry_id = np->ipft_hf_eid;
> -
> -	if (netc_port_add_host_flood_rule(np, uc, mc)) {
> -		dev_err(ds->dev, "Failed to add host flood rule on port %d\n",
> +	if (netc_port_remove_host_flood(np)) {
> +		dev_err(ds->dev,
> +			"Failed to delete old host flood rule on port %d\n",
>  			port);
> +
>  		return;
>  	}
>  
> -	/* Remove the old host flood entry */
> -	netc_port_remove_host_flood(np, old_entry_id);
> +	/* Restoring the previous configuration is pointless because
> +	 * .port_set_host_flood() returns void, so the upper layer cannot
> +	 * detect the error and the RX flags have changed.
> +	 */
> +	if (netc_port_add_host_flood_rule(np, uc, mc))
> +		dev_err(ds->dev,
> +			"Failed to add host flood rule on port %d\n", port);

[Severity: Medium]
With the new ordering, what happens to host flooding when the add fails?

netc_port_remove_host_flood() has already deleted the entry and cleared
the tracked state:

	np->ipft_hf_eid = NTMP_NULL_ENTRY_ID;
	np->uc = false;
	np->mc = false;
	netc_port_wr(np, NETC_PIPFCR, 0);

netc_port_add_host_flood_rule() can then fail on kzalloc_obj() returning
NULL, or on ntmp_ipft_add_entry() returning -ENOMEM/-EBUSY/-ETIMEDOUT/
-EIO from the command BD transaction.  In that case the port ends up
with no host flood entry at all while the netdev still has IFF_PROMISC
or IFF_ALLMULTI set, and only a dev_err() is emitted.

Before this change the add ran first, so an add failure left the
previous working rule installed and np->uc/np->mc untouched.

Since .port_set_host_flood() is void and is only reached from
dsa_user_change_rx_flags() -> dsa_user_manage_host_flood(), nothing
reports or retries the failure.  Does host unicast/multicast flooding
for that port then stay silently off until userspace toggles the RX
flags again?

The comment says restoring the previous configuration is pointless.  For
the common promisc to allmulti transition the old rule is a superset of
what is now requested, so wouldn't keeping it still satisfy the new
request and be better than delivering nothing?

[Severity: Low]
Separately, is the window with no rule installed a concern even when
both operations succeed?  Between the delete and the add the port has no
host flood redirect, and the gap covers a full synchronous NTMP command
BD transaction for the delete (dma_alloc_coherent(GFP_KERNEL) plus
netc_xmit_ntmp_cmd() polling through read_poll_timeout()), a
kzalloc_obj() that may enter direct reclaim, and a second command BD
transaction for the add, which only re-arms the redirect at the end:

	err = ntmp_ipft_add_entry(&priv->ntmp, host_flood);
	if (err)
		goto free_host_flood;
	...
	/* Enable ingress port filter table lookup */
	netc_port_wr(np, NETC_PIPFCR, PIPFCR_EN);

During that interval, frames that only reach the CPU through the host
flood redirect (multicast under IFF_ALLMULTI when going from
(uc=0,mc=1) to (uc=1,mc=1), or unknown-DA unicast under IFF_PROMISC)
are dropped.  The old add-then-delete ordering kept a matching entry
armed at every instant.

[ ... ]

> @@ -2015,6 +2024,11 @@ static int netc_port_bridge_join(struct dsa_switch *ds, int port,
>  disable_mlo:
>  	netc_port_set_mlo(np, MLO_DISABLE);
>  
> +	if (netc_port_add_host_flood_rule(np, uc, mc))
> +		dev_err(ds->dev,
> +			"Failed to restore host flood rule on port %u\n",
> +			port);

[Severity: Low]
This isn't a bug, but port is declared int by the .port_bridge_join
prototype, and the two other messages added by this patch use %d for
the same value ("Failed to delete old host flood rule on port %d" and
"Failed to add host flood rule on port %d").  Should this one use %d as
well?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918072852.501420-1-wei.fang%40oss.nxp.com

Re: [PATCH v4 net-next 7/8] net: dsa: netc: add PTP two-step timestamping support

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 3 · Low: 3

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] A timed-out timestamp-request ID becomes immediately reusable,
  so a late hardware response can complete a newer skb with the wrong…
- [Medium] Partial IPFT deletion or a failed rollback leaves the reported
  RX timestamp configuration inconsistent with the rules actually…
- [Medium] The PTP ingress-port-filter entries programmed by
  netc_port_hwtstamp_set() are never removed when the switch is torn…
- [Low] Commit message names a callback that does not exist.
- [Low] netc_get_ts_info() advertises HWTSTAMP_TX_ON and
  netc_port_hwtstamp_set() accepts it (even together with…
- [Low] The new TX-timestamp timeout bookkeeping reads the global 64-bit
  `jiffies_64` variable directly instead of using get_jiffies_64():…

--

net: dsa: netc: add PTP two-step timestamping support

This adds RX timestamping via IPFT rules that trap PTP frames to the CPU
port, and two-step TX timestamping by cloning the skb, assigning a 4-bit
per-port request ID and matching the hardware response frame back to the
queued clone.  It also adds the per-switch netc_tagger_data and grabs the
PTP timer's pci_dev so get_ts_info() can report the PHC index.

[Severity: Low]
The changelog says:

> The tagger dispatches both to the driver through the
> twostep_tstamp_handler callback in netc_tagger_data, which matches the
> ID to the queued clone and completes it via skb_complete_tx_timestamp().

Is that the right member name?  The struct added by this patch in
include/linux/dsa/tag_netc.h is:

struct netc_tagger_data {
	void (*txtstamp_handler)(struct dsa_switch *ds, int port,
				 u8 ts_req_id, u64 ts);
};

and net/dsa/tag_netc.c:netc_twostep_tstamp_process() calls
tagger_data->txtstamp_handler(), so grepping for twostep_tstamp_handler
finds nothing.

The same paragraph mentions netc_connect()/netc_disconnect() on the tagger
side, but not the new .connect_tag_protocol op
netc_connect_tag_protocol(), which is what actually installs the handler.
Could that be mentioned as well so the dispatch path is followable from
the changelog?

> diff --git a/drivers/net/dsa/netc/netc_main.c b/drivers/net/dsa/netc/netc_main.c
> index 336f5a4633e9..03200fc63a09 100644
> --- a/drivers/net/dsa/netc/netc_main.c
> +++ b/drivers/net/dsa/netc/netc_main.c

[ ... ]

> @@ -949,6 +993,21 @@ static void netc_destroy_all_lists(struct netc_switch *priv)
>  	mutex_destroy(&priv->vft_lock);
>  }
>  
> +static void netc_free_ports_resources(struct netc_switch *priv)
> +{
> +	struct dsa_port *dp;
> +
> +	dsa_switch_for_each_available_port(dp, priv->ds) {
> +		struct netc_port *np = priv->ports[dp->index];
> +
> +		if (!dsa_port_is_user(dp))
> +			continue;
> +
> +		disable_delayed_work_sync(&np->tstamp_timeout_work);
> +		netc_port_purge_tstamp_queue(np);
> +	}
> +}
> +
>  static void netc_teardown(struct dsa_switch *ds)
>  {
>  	struct netc_switch *priv = ds->priv;
> @@ -956,6 +1015,8 @@ static void netc_teardown(struct dsa_switch *ds)
>  	disable_delayed_work_sync(&priv->fdbt_ageing_work);
>  	netc_destroy_all_lists(priv);
>  	netc_free_ntmp_user(priv);
> +	netc_free_ports_resources(priv);
> +	pci_dev_put(priv->tmr_dev);
>  }

[Severity: Medium]
Do the PTP IPFT entries survive teardown here?

netc_free_ports_resources() cancels the timeout work and purges the clone
queue, but nothing calls netc_port_del_ptp_filter(), which is the only
release path for np->ptp_ipft_eid[].  It is static in netc_ptp.c and
reachable only from netc_port_set_ptp_filter(), i.e. only from a user-space
hwtstamp request.

So after unbind, up to five entries per user port are still live in the
IPFT with FLTFA=REDIRECT, host reason NETC_HR_PTP_TRAP and IPFT_TIMECAPE,
while struct netc_port (holding the entry IDs) is devm-freed.

There is also an ordering question: netc_teardown() calls
netc_free_ntmp_user() before netc_free_ports_resources():

	netc_free_ntmp_user(priv);	/* destroys the command BD rings */
	netc_free_ports_resources(priv);

Since ntmp_ipft_delete_entry() needs those rings, can the table cleanup be
added at the natural place with this ordering, or should
netc_free_ports_resources() move before netc_free_ntmp_user()?

In a multi-switch tree this looks observable without a PCI re-probe:
unbinding one switch runs dsa_tree_teardown() (hence netc_teardown() for
the sibling), and re-binding runs netc_setup() again on the surviving priv,
where netc_port_ptp_init() resets ptp_ipft_eid[] to NTMP_NULL_ENTRY_ID
while the old entries are still programmed.  Would PTP frames then keep
being trapped and time-captured with HWTSTAMP_FILTER_NONE, with no way to
delete the stale entries?  For the single-switch case the pcie_flr() in
netc_switch_pci_init() at probe seems to hide it.

[ ... ]

> diff --git a/drivers/net/dsa/netc/netc_ptp.c b/drivers/net/dsa/netc/netc_ptp.c
> new file mode 100644
> index 000000000000..2e743443ca03
> --- /dev/null
> +++ b/drivers/net/dsa/netc/netc_ptp.c
> @@ -0,0 +1,492 @@

[ ... ]

> +static void netc_port_tstamp_timeout_work(struct work_struct *work)
> +{
> +	struct netc_port *np = container_of(work, struct netc_port,
> +					    tstamp_timeout_work.work);
> +	struct sk_buff_head free_list;
> +	struct sk_buff *skb, *skb_tmp;
> +
> +	__skb_queue_head_init(&free_list);
> +
> +	spin_lock_bh(&np->tstamp_lock);
> +	skb_queue_walk_safe(&np->tstamp_queue, skb, skb_tmp) {
> +		if (time_before64(jiffies_64, NETC_SKB_CB(skb)->ptp_tx_time +
> +				  NETC_TSTAMP_TIMEOUT))
> +			continue;

[Severity: Low]
Should this use get_jiffies_64() rather than reading jiffies_64 directly?
The same raw read is used in netc_port_prepare_twostep():

	NETC_SKB_CB(clone)->ptp_tx_time = jiffies_64;

include/linux/jiffies.h says:

 * The 64-bit value is not atomic on 32-bit systems - you MUST NOT read it
 * without sampling the sequence number in jiffies_lock.
 * get_jiffies_64() will do this for you as appropriate.

Since this comparison is the only exit criterion for a queued clone, a torn
read on a 32-bit build could either purge it immediately or never purge it
(retaining the clone, its socket reference from skb_clone_sk() and its
ts_req_id).  The Kconfig is "depends on ARM64 || COMPILE_TEST", so only
32-bit COMPILE_TEST builds compile the racy read today.  ocelot uses plain
jiffies with time_is_before_jiffies() for the same pattern.

> +
> +		dev_dbg_ratelimited(np->switch_priv->dev,
> +				    "Port %d ts_req_id %u which seems lost\n",
> +				    np->dp->index, NETC_SKB_CB(skb)->ts_req_id);
> +
> +		__skb_unlink(skb, &np->tstamp_queue);
> +		__skb_queue_tail(&free_list, skb);
> +	}

[ ... ]

> +static int netc_get_ts_req_id(struct netc_port *np)
> +{
> +	DECLARE_BITMAP(ts_req_id_bitmap, NETC_NUM_TS_REQ_ID);
> +	struct sk_buff *skb, *skb_tmp;
> +	unsigned long ts_req_id;
> +
> +	bitmap_zero(ts_req_id_bitmap, NETC_NUM_TS_REQ_ID);
> +
> +	skb_queue_walk_safe(&np->tstamp_queue, skb, skb_tmp)
> +		__set_bit(NETC_SKB_CB(skb)->ts_req_id, ts_req_id_bitmap);
> +
> +	ts_req_id = find_first_zero_bit(ts_req_id_bitmap, NETC_NUM_TS_REQ_ID);

[Severity: Medium]
Can a timed-out request ID be handed out again while the hardware response
for it is still in flight?

Availability here is derived purely from the clones currently queued, and
netc_port_tstamp_timeout_work() frees an ID by unlinking the clone without
knowing whether the frame has even been transmitted.  The timeout clock
starts before the frame reaches the conduit:

netc_port_prepare_twostep()
	NETC_SKB_CB(clone)->ptp_tx_time = jiffies_64;
	__skb_queue_tail(&np->tstamp_queue, clone);

so egress queueing delay (link down/flapping, or sustained PAUSE from the
link partner) counts against the 5 second budget.  If that delay exceeds
NETC_TSTAMP_TIMEOUT:

netc_port_tstamp_timeout_work()	-> drops clone, frees ID
netc_get_ts_req_id()		-> hands the same ID to a new transmit
netc_port_txtstamp_handler()	-> late response matches the new clone

and netc_port_txtstamp_handler() matches on nothing but port and the 4-bit
ID:

	if (NETC_SKB_CB(skb)->ts_req_id != ts_req_id)
		continue;

	__skb_unlink(skb, &np->tstamp_queue);

Would that report the old frame's transmit time for the new frame, and then
discard the correct response as lost?  ocelot_port_dequeue_ptp_tx_skb()
additionally compares the PTP sequenceId before completing a queued clone;
since the NETC response tag only carries the 4-bit ID, would a generation
counter or an ID quarantine be needed here?

[ ... ]

> +int netc_get_ts_info(struct dsa_switch *ds, int port,
> +		     struct kernel_ethtool_ts_info *info)
> +{

[ ... ]

> +	info->tx_types = BIT(HWTSTAMP_TX_OFF) | BIT(HWTSTAMP_TX_ON);
> +

[ ... ]

> +static int netc_port_del_ptp_filter(struct netc_port *np)
> +{
> +	struct netc_switch *priv = np->switch_priv;
> +	int ret = 0;
> +	int err;
> +
> +	for (int i = 0; i < NETC_PTP_MAX; i++) {
> +		if (np->ptp_ipft_eid[i] == NTMP_NULL_ENTRY_ID)
> +			continue;
> +
> +		/* No -ETIMEDOUT here: with the command BD ring enabled, the
> +		 * hardware never times out on a command. Any remaining error
> +		 * means the entry is still present in the table.
> +		 */
> +		err = ntmp_ipft_delete_entry(&priv->ntmp,
> +					     np->ptp_ipft_eid[i]);
> +		if (likely(!err)) {
> +			np->ptp_ipft_eid[i] = NTMP_NULL_ENTRY_ID;
> +			continue;
> +		}
> +
> +		ret = err;

[Severity: Medium]
When one of the up to five deletions fails, the loop continues and the
last error is returned, so the hardware can end up with a subset of the
rules still installed.  What does user space then see?

netc_port_set_ptp_filter()
	err = netc_port_del_ptp_filter(np);
	if (err)
		return err;

	np->ptp_rx_filter = HWTSTAMP_FILTER_NONE;

On the delete failure path np->ptp_rx_filter keeps the full old value, so
netc_port_hwtstamp_get() reports a filter that is only partly programmed.

On the add path the opposite happens: np->ptp_rx_filter is already
HWTSTAMP_FILTER_NONE, and both netc_port_add_l4_ptp_filter() and
netc_port_add_l2_l4_ptp_filter() discard the rollback return value:

del_ptp_filter:
	netc_port_del_ptp_filter(np);

	return err;

so a failed rollback can leave live REDIRECT+TIMECAPE entries while
software reports NONE, and PTP frames from that port keep being trapped to
the CPU port instead of being forwarded.

Is the failure reachable?  ntmp_ipft_delete_entry() -> 
ntmp_delete_entry_by_id() first does:

	err = ntmp_alloc_data_mem(user->dev, &swcbd, (void **)&req);
	if (err)
		return err;

which is a dma_alloc_coherent(..., GFP_KERNEL) that can return -ENOMEM
before the command is issued, and netc_xmit_ntmp_cmd() can return -EBUSY,
-EIO or -ETIMEDOUT.

[ ... ]

> +void netc_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
> +{
> +	struct netc_port *np = NETC_PORT(ds, port);
> +	u32 ptp_class;
> +	int tx_type;
> +
> +	NETC_SKB_CB(skb)->ptp_flag = 0;
> +	ptp_class = ptp_classify_raw(skb);
> +	if (ptp_class == PTP_CLASS_NONE)
> +		return;
> +
> +	/* The rx_filters in netc_get_ts_info() has already declared that
> +	 * it only supports PTP v2, so TX only supports v2 as well.
> +	 */
> +	if (unlikely(ptp_class & PTP_CLASS_V1))
> +		return;

[Severity: Low]
This isn't a bug for real PTP stacks, but is the comment's reasoning right?
The advertised rx_filters describe what the ingress filter can match; the
uapi definition of the TX mode makes no protocol statement:

include/uapi/linux/net_tstamp.h
	/*
	 * Enables hardware time stamping for outgoing packets;
	 * the sender of the packet decides which are to be
	 * time stamped ...
	 */
	HWTSTAMP_TX_ON,

netc_get_ts_info() advertises HWTSTAMP_TX_ON unconditionally and
netc_port_hwtstamp_set() accepts it even with HWTSTAMP_FILTER_NONE, yet a
socket asking for SOF_TIMESTAMPING_TX_HARDWARE on a non-PTP or PTPv1 frame
silently gets no timestamp here.  Could the comment be reworded to state
the hardware/driver TX restriction directly instead of deriving it from the
RX filters?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918072852.501420-1-wei.fang%40oss.nxp.com

Re: [PATCH v4 net-next 8/8] net: dsa: netc: add PTP one-step timestamping support

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 · Medium: 4 · Low: 2

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).

- [High] Lost-wakeup / incomplete state machine: once
  netc_port_onestep_work() commits np->onestep_state =…
- [High] In netc_port_program_onestep()
  (drivers/net/dsa/netc/netc_ptp.c:99-163), when…
- [Medium] The one-step Sync path silently discards TX frames on at least
  seven paths with no tx_dropped/tx_errors accounting and only two…
- [Medium] In netc_port_onestep_work() the 'no ts_req_id available'
  branch drops the dequeued frame AND unconditionally re-arms itself…
- [Medium] ABA race across the split tstamp_lock/onestep_lock:
  netc_port_txtstamp_handler() unlinks the matching clone under…
- [Medium] PM_SINGLE_STEP is a single per-port MAC register that the
  hardware consults at SFD time of the one-step frame (it supplies the…
- [Low] drivers/net/dsa/netc/Kconfig gains 'depends on PTP_NETC_V4_TIMER
  || PTP_NETC_V4_TIMER=n' with no mention in the commit message.
- [Low] netc_port_set_onestep_control() programs PM_SINGLE_STEP through
  netc_mac_port_wr(), which silently returns without touching hardware…

--

net: dsa: netc: add PTP one-step timestamping support

This adds one-step Sync offload for NETC switch ports and advertises
HWTSTAMP_TX_ONESTEP_SYNC.  One-step Sync frames are handed from the tagger
to the switch driver, queued per port and drained one at a time from a work
item that programs the shared PM_SINGLE_STEP register.  Non one-step Sync
PTP frames keep using two-step timestamping.

A few questions below about the new one-step path.

> diff --git a/drivers/net/dsa/netc/Kconfig b/drivers/net/dsa/netc/Kconfig
> index 8770b65d0f62..7c930579f1c9 100644
> --- a/drivers/net/dsa/netc/Kconfig
> +++ b/drivers/net/dsa/netc/Kconfig
> @@ -5,6 +5,7 @@ config NET_DSA_NETC_SWITCH
>  	depends on NET_DSA && PCI
>  	depends on NET_VENDOR_FREESCALE
>  	depends on PTP_1588_CLOCK_OPTIONAL
> +	depends on PTP_NETC_V4_TIMER || PTP_NETC_V4_TIMER=n

[Severity: Low]
This new tristate dependency isn't mentioned in the changelog, and it does
change which existing configurations stay valid.

CONFIG_NET_DSA_NETC_SWITCH=y together with CONFIG_PTP_NETC_V4_TIMER=m was a
legal combination before; with this line make oldconfig silently downgrades
the switch driver to =m.

The motivation looks right, since netc_timer_get_current_time() is behind
IS_REACHABLE(CONFIG_PTP_NETC_V4_TIMER) in include/linux/fsl/netc_global.h
and otherwise resolves to a -ENODEV stub, which would make netc_get_phc_time()
fail for every one-step frame.  Could the changelog describe this restriction,
or could it be split into its own patch?

>  	select NET_DSA_TAG_NETC
>  	select FSL_ENETC_MDIO
>  	select NXP_NTMP
> diff --git a/drivers/net/dsa/netc/netc_main.c b/drivers/net/dsa/netc/netc_main.c
> index 03200fc63a09..af39a71aece0 100644
> --- a/drivers/net/dsa/netc/netc_main.c
> +++ b/drivers/net/dsa/netc/netc_main.c

[ ... ]

> @@ -94,7 +95,7 @@ static void netc_port_rmw(struct netc_port *np, u32 reg,
>  	netc_port_wr(np, reg, new);
>  }
>  
> -static void netc_mac_port_wr(struct netc_port *np, u32 reg, u32 val)
> +void netc_mac_port_wr(struct netc_port *np, u32 reg, u32 val)
>  {
>  	if (is_netc_pseudo_port(np))
>  		return;

[ ... ]

> diff --git a/drivers/net/dsa/netc/netc_ptp.c b/drivers/net/dsa/netc/netc_ptp.c
> index 2e743443ca03..1bc0f0956206 100644
> --- a/drivers/net/dsa/netc/netc_ptp.c
> +++ b/drivers/net/dsa/netc/netc_ptp.c
> @@ -11,6 +11,15 @@
>  
>  #define NETC_NUM_TS_REQ_ID		16
>  #define NETC_TSTAMP_TIMEOUT		(5 * HZ)
> +#define NETC_MAX_STEP_OFFSET		0x1ff
> +#define NETC_ONESTEP_QTH		512
> +/* The 30-bit timestamp of the To_Port subtype 3 tag lets the hardware
> + * account for a single wrap, so the correction field of a one-step Sync
> + * frame is only correct if it is sent out within 2^30 ns after the
> + * software timestamp is read. Past this window the frame is beyond
> + * repair, and PM_SINGLE_STEP becomes safe to reprogram.
> + */

[Severity: Medium]
This comment argues that the correction value is beyond repair after 2^30 ns,
but PM_SINGLE_STEP also carries the correction-field offset and the
"update UDP checksum" bit, which the hardware still applies at SFD time to
whatever frame is being transmitted.

If the previously programmed frame is still sitting in the switch or port
egress path (egress congestion, flow control, TAS gating), the next
netc_port_set_onestep_control() call can change offset and CH under it.  For
an L2 PTP frame the correction offset is around 22, for UDP/IPv6 around 74,
so the MAC would write 8 bytes of correction data at the wrong position
inside the pending frame and may recompute its UDP checksum.

The "no ts_req_id available" branch of netc_port_onestep_work() has the same
shape: it overwrites np->onestep_state with NETC_ONESTEP_SCHEDULED, which
erases the record that a frame is in flight at all.

Would it be safer to only reprogram the register when the new frame's
offset/CH match the pending one, or to hold off until the pending frame is
known to be gone?

> +#define NETC_ONESTEP_VALID_WINDOW	0x40000000 /* ns */
>  
>  static void netc_port_tstamp_timeout_work(struct work_struct *work)
>  {
> @@ -68,12 +77,276 @@ static int netc_get_ts_req_id(struct netc_port *np)
>  	return ts_req_id;
>  }
>  
> +static int netc_get_phc_time(struct netc_switch *priv, u64 *ns)
> +{
> +	if (unlikely(!priv->tmr_dev))
> +		return -ENODEV;
> +
> +	return netc_timer_get_current_time(priv->tmr_dev, ns);
> +}
> +
> +static void netc_port_set_onestep_control(struct netc_port *np,
> +					  bool csum_update, int offset)
> +{
> +	u32 val;
> +
> +	val = PM_SINGLE_STEP_EN | FIELD_PREP(PM_SINGLE_STEP_OFFSET, offset);
> +	if (csum_update)
> +		val |= PM_SINGLE_STEP_CH;
> +	netc_mac_port_wr(np, NETC_PM_SINGLE_STEP(0), val);

[Severity: Low]
This isn't a bug on today's hardware, but netc_mac_port_wr() returns without
touching the register when is_netc_pseudo_port(np) is true, and it reports
nothing back to the caller.

The one-step machinery is gated only on dsa_port_is_user(), and
netc_get_ts_info() advertises HWTSTAMP_TX_ONESTEP_SYNC unconditionally, so a
user port with caps.pseudo_link set would get its originTimestamp rewritten
and a subtype-3 tag attached while the MAC single-step engine was never
enabled.

That combination looks impossible right now: imx94_switch_phylink_get_caps()
only marks port 3 (the CPU port) as internal, and the binding requires
ethernet-port@3 to carry the ethernet = <&enetc3> phandle.  Would it still be
worth refusing one-step on pseudo-link ports explicitly so a future platform
cannot silently drop the register write?

> +}
> +
> +static void netc_port_program_onestep(struct netc_port *np,
> +				      struct sk_buff *skb)
> +{
> +	u16 correction_offset = NETC_SKB_CB(skb)->correction_offset;
> +	u16 tstamp_offset = NETC_SKB_CB(skb)->timestamp_offset;
> +	u64 tstamp = NETC_SKB_CB(skb)->tstamp;
> +	u8 *hdr = skb_mac_header(skb);

[ ... ]

> +	if (NETC_SKB_CB(skb)->is_udp) {
> +		__be32 old_sec_l, old_ns;
> +		struct udphdr *uh;
> +		__be16 old_sec_h;
> +
> +		if (skb->ip_summed == CHECKSUM_PARTIAL) {
> +			csum_update = true;
> +			goto update_timestamp;
> +		}
> +
> +		if (unlikely(!skb_transport_header_was_set(skb)))
> +			uh = (struct udphdr *)(hdr + tstamp_offset -
> +					       sizeof(struct ptp_header) -
> +					       sizeof(struct udphdr));
> +		else
> +			uh = udp_hdr(skb);

[Severity: High]
Can udp_hdr(skb) be trusted here?  netc_port_prepare_onestep_sync() derives
and bounds-checks the PTP offsets itself:

	if (unlikely(tstamp_offset + 10 > skb_headlen(skb) ||
		     correction_offset > NETC_MAX_STEP_OFFSET))
		goto set_ptp_flag_drop;

but skb->transport_header is never validated against skb_headlen(), and the
safe computed expression is only used when the transport header was not set.
The code then reads uh->check at offset 6 and writes 2 bytes there through
inet_proto_csum_replace2()/inet_proto_csum_replace4().

Two AF_PACKET paths can leave transport_header at or past the tail while the
frame still parses as a valid PTPv2 Sync over UDP with ip_summed ==
CHECKSUM_NONE:

packet_snd()
  packet_parse_headers()
    skb_probe_transport_header()
      __skb_flow_dissect()
        key_control->thoff = min_t(u16, nhoff, skb ? skb->len : hlen);

The dissector runs with skb->protocol taken from the user-supplied
sll_protocol and nhoff = 0, so a frame whose declared ethertype disagrees
with its real headers can walk attacker-chosen IPv6 option hdrlen bytes and
end with thoff clamped to skb->len, i.e. transport_header == tail.

The other path is PACKET_VNET_HDR:

virtio_net_hdr_to_skb()
  skb_partial_csum_set()   /* csum_start = headlen - 2 accepted */
...
skb_checksum_help()        /* flips ip_summed to CHECKSUM_NONE,
                              transport_header left as-is */

With the frame length tuned so tailroom is under 8 bytes, wouldn't the
uh->check read go past skb_tail_pointer() and the csum-replace write land
inside struct skb_shared_info (flags/meta_len/nr_frags/tx_flags/gso_size)?

Since the validated expression is already computed just above, could it be
used unconditionally instead of udp_hdr(skb)?  The same udp_hdr() pattern
exists in drivers/net/ethernet/freescale/enetc/enetc.c, but this path is new
here.

> +
> +		/* For IPv4, a UDP checksum of zero on the wire means "no
> +		 * checksum". For IPv6, its UDP checksum is mandatory and
> +		 * never zero.
> +		 */
> +		if (!uh->check)
> +			goto update_timestamp;
> +
> +		old_sec_h = __get_unaligned_t(__be16, hdr + tstamp_offset);
> +		old_sec_l = __get_unaligned_t(__be32, hdr + tstamp_offset + 2);
> +		old_ns = __get_unaligned_t(__be32, hdr + tstamp_offset + 6);
> +		inet_proto_csum_replace2(&uh->check, skb, old_sec_h,
> +					 new_sec_h, false);
> +		inet_proto_csum_replace4(&uh->check, skb, old_sec_l,
> +					 new_sec_l, false);
> +		inet_proto_csum_replace4(&uh->check, skb, old_ns,
> +					 new_ns, false);
> +		csum_update = true;
> +	}
> +
> +update_timestamp:

[ ... ]

> +static void netc_port_onestep_work(struct work_struct *work)
> +{

[ ... ]

> +skb_dequeue:
> +	skb = __skb_dequeue(&np->onestep_queue);
> +	if (!skb)
> +		goto set_onestep_state_idle;
> +
> +	/* Clone is a ts_req_id token only; its payload is never read, so
> +	 * sharing the buffer with the mutated original is fine.
> +	 */
> +	clone = skb_clone(skb, GFP_ATOMIC);
> +	if (unlikely(!clone)) {
> +		kfree_skb(skb);
> +		goto skb_dequeue;
> +	}
> +
> +	spin_lock_bh(&np->tstamp_lock);
> +	ts_req_id = netc_get_ts_req_id(np);
> +	if (unlikely(ts_req_id < 0)) {
> +		spin_unlock_bh(&np->tstamp_lock);
> +
> +		/* Re-queuing the frame and immediately rescheduling the work
> +		 * would busy-loop on system_percpu_wq and burn CPU until an
> +		 * ID is freed, so drop this frame and move on to the next one
> +		 * in the queue instead.
> +		 */
> +		np->onestep_state = NETC_ONESTEP_SCHEDULED;
> +		schedule_work(&np->onestep_work);
> +
> +		goto onestep_unlock;
> +	}

[Severity: Medium]
The comment says re-queuing plus rescheduling would busy-loop, but the work
does reschedule itself here unconditionally, and each pass destroys one
queued one-step Sync frame at onestep_unlock.  With NETC_ONESTEP_QTH at 512,
can this discard the entire backlog in quick succession?

The 16-entry ts_req_id pool is shared with the two-step path:
netc_port_prepare_twostep() takes an ID for every PTP frame that is not a
one-step Sync (Announce, Follow_Up, Delay_Resp still flow in one-step mode),
and holds it for up to NETC_TSTAMP_TIMEOUT when a response is lost.  So a
burst of two-step requests can drive this branch.

The only trace is the dev_dbg_ratelimited() inside netc_get_ts_req_id().
Should ID exhaustion apply backpressure (leave the frame queued and arm a
delayed retry) rather than dropping frames, and should the changelog mention
that exhaustion means one-step Sync frames are discarded?

> +
> +	/* PHC is unavailable, drop the whole queue */
> +	if (unlikely(netc_get_phc_time(priv, &tstamp))) {
> +		spin_unlock_bh(&np->tstamp_lock);
> +		goto set_onestep_state_idle;
> +	}
> +
> +	NETC_SKB_CB(skb)->tstamp = tstamp;
> +	NETC_SKB_CB(skb)->ts_req_id = ts_req_id;
> +	NETC_SKB_CB(skb)->ptp_flag = NETC_PTP_FLAG_ONESTEP;
> +	NETC_SKB_CB(clone)->ts_req_id = ts_req_id;
> +	NETC_SKB_CB(clone)->ptp_tx_time = jiffies_64;
> +	NETC_SKB_CB(clone)->ptp_flag = NETC_PTP_FLAG_ONESTEP;
> +	np->onestep_tx_time = NETC_SKB_CB(clone)->ptp_tx_time;
> +	np->onestep_ts_req_id = ts_req_id;
> +
> +	__skb_queue_tail(&np->tstamp_queue, clone);
> +	if (!delayed_work_pending(&np->tstamp_timeout_work))
> +		schedule_delayed_work(&np->tstamp_timeout_work,
> +				      NETC_TSTAMP_TIMEOUT);
> +
> +	spin_unlock_bh(&np->tstamp_lock);
> +
> +	np->onestep_state = NETC_ONESTEP_IN_FLIGHT;
> +	spin_unlock_bh(&np->onestep_lock);
> +
> +	netc_port_program_onestep(np, skb);
> +	tagger_data = priv->ds->tagger_data;
> +	tagger_data->onestep_sync_xmit(skb, np->dp->user);
> +
> +	return;
> +
> +set_onestep_state_idle:
> +	np->onestep_state = NETC_ONESTEP_IDLE;
> +purge_onestep_queue:
> +	__skb_queue_purge(&np->onestep_queue);
> +onestep_unlock:
> +	spin_unlock_bh(&np->onestep_lock);
> +	kfree_skb(skb);
> +	kfree_skb(clone);
> +}
> +
> +static bool netc_onestep_timeout(struct netc_port *np)
> +{
> +	u64 expire_time;
> +
> +	/* Use monotonic jiffies_64, as the PHC may be stepped backwards.
> +	 * Add one tick since the ns-to-jiffies conversion rounds down, so
> +	 * the software window is never shorter than the hardware window.
> +	 */
> +	expire_time = np->onestep_tx_time + 1 +
> +		      nsecs_to_jiffies64(NETC_ONESTEP_VALID_WINDOW);
> +	if (np->onestep_state == NETC_ONESTEP_IN_FLIGHT &&
> +	    time_after64(jiffies_64, expire_time))
> +		return true;
> +
> +	return false;
> +}
> +
> +void netc_port_onestep_sync_enqueue(struct dsa_switch *ds, int port,
> +				    struct sk_buff *skb)
> +{
> +	struct netc_port *np = NETC_PORT(ds, port);
> +
> +	spin_lock_bh(&np->onestep_lock);
> +	if (unlikely(np->onestep_state == NETC_ONESTEP_PORT_INACTIVE)) {
> +		kfree_skb(skb);
> +		goto onestep_unlock;
> +	}
> +
> +	if (unlikely(skb_queue_len(&np->onestep_queue) >= NETC_ONESTEP_QTH)) {
> +		dev_dbg_ratelimited(np->switch_priv->dev,
> +				    "The onestep_queue of port %d is full\n",
> +				    port);
> +		kfree_skb(skb);
> +		goto onestep_unlock;
> +	}
> +
> +	__skb_queue_tail(&np->onestep_queue, skb);
> +	if (likely(np->onestep_state == NETC_ONESTEP_IDLE) ||
> +	    netc_onestep_timeout(np)) {
> +		np->onestep_state = NETC_ONESTEP_SCHEDULED;
> +		schedule_work(&np->onestep_work);
> +	}
> +
> +onestep_unlock:
> +	spin_unlock_bh(&np->onestep_lock);
> +}

[Severity: High]
Can the port get stuck in NETC_ONESTEP_IN_FLIGHT forever?

Once netc_port_onestep_work() sets IN_FLIGHT and hands the frame to
onestep_sync_xmit(), only three things can resume the drain:

  1. a matching response in netc_port_txtstamp_handler()
  2. netc_onestep_timeout(), which is evaluated only from
     netc_port_onestep_sync_enqueue()
  3. the ts_req_id exhaustion self-reschedule

The changelog says:

    A dropped frame produces no response, so reuse tstamp_timeout_work to
    reclaim its request ID after 5 seconds

but netc_port_tstamp_timeout_work() only unlinks and frees the timed-out
clone:

	__skb_unlink(skb, &np->tstamp_queue);
	__skb_queue_tail(&free_list, skb);
	...
	if (!skb_queue_empty(&np->tstamp_queue))
		schedule_delayed_work(&np->tstamp_timeout_work,
				      NETC_TSTAMP_TIMEOUT);

It never touches np->onestep_state and never schedules np->onestep_work.
And once the clone has been reaped, a late response takes the !skb_match
early return in netc_port_txtstamp_handler(), so it cannot clear IN_FLIGHT
either.

Concretely: frame A goes IN_FLIGHT with id X, frame B is enqueued inside the
window and only appended, A is dropped in the switch so no response arrives,
5 seconds later the timeout work frees A's clone and leaves IN_FLIGHT set.
If the sender then stops, or falls below one frame per window, doesn't B and
everything behind it stay queued and unfreed until link-down, port disable
or a hwtstamp reconfiguration?  These are the original TX skbs and still
hold skb->sk, so the owning socket stays charged.

The queue-full branch above also returns before the netc_onestep_timeout()
check, so once 512 frames have accumulated even continued traffic can no
longer recover the port.

Would it work to reset onestep_state and schedule onestep_work from the
timeout path, or to arm a delayed work for the validity deadline, so
recovery does not depend on new traffic arriving?

Separately, the changelog says the work is scheduled "when the PTP time has
advanced beyond NETC_ONESTEP_VALID_WINDOW", while netc_onestep_timeout()
compares monotonic jiffies_64 and only at enqueue time.

[ ... ]

> @@ -395,6 +673,66 @@ int netc_port_hwtstamp_get(struct dsa_switch *ds, int port,
>  	return 0;
>  }
>  
> +static void netc_port_prepare_onestep_sync(struct netc_port *np,
> +					   struct sk_buff *skb,
> +					   u32 ptp_class, bool *twostep)
> +{

[ ... ]

> +	if (unlikely(skb_linearize_cow(skb)))
> +		goto set_ptp_flag_drop;
> +
> +	ptp_hdr = ptp_parse_header(skb, ptp_class);
> +	if (unlikely(!ptp_hdr))
> +		goto set_ptp_flag_drop;
> +
> +	msg_type = ptp_get_msgtype(ptp_hdr, ptp_class);

[ ... ]

> +set_ptp_flag_drop:
> +	/* Drop instead of falling back to two-step: if it is a Sync,
> +	 * one-step offload will not be executed, the timestamp in the
> +	 * frame is inaccurate, which may affect PTP synchronization.
> +	 */
> +	NETC_SKB_CB(skb)->ptp_flag = NETC_PTP_FLAG_DROP;
> +	dev_dbg_ratelimited(priv->dev,
> +			    "Port %d: PTP frame dropped in error\n",
> +			    np->dp->index);
> +}

[Severity: Medium]
The skb_linearize_cow() failure is evaluated before the message type is
known, so a plain ENOMEM destroys any PTP frame on the port, including a
Delay_Req that this code would otherwise have sent two-step.  Should the
linearize failure fall back to two-step, or at least be handled after the
Sync check?

None of the new drop paths are accounted anywhere.  dsa_user_xmit() has
already counted the frame as transmitted via dev_sw_netstats_tx_add() before
the tagger frees it, so from userspace the frames just vanish:

  netc_port_prepare_onestep_sync() -> NETC_PTP_FLAG_DROP -> kfree_skb() in
    netc_xmit()                               (linearize/parse/offset checks)
  netc_port_onestep_sync_enqueue()            (queue full, port inactive)
  netc_port_onestep_work()                    (clone failure, no ts_req_id,
                                               PHC read failure purges all)
  netc_port_purge_onestep_queue()             (hwtstamp reconfiguration)

netc has no .get_stats64, so dsa_user_get_stats64() falls back to
dev_get_tstats64(), which folds dev->stats into rtnl_link_stats64 - could
these paths bump dev->stats.tx_dropped?

The changelog only says "Frames that are not one-step Sync still fall back
to two-step timestamping", which does not tell a reader that enabling
one-step turns a degraded timestamp into a frame that is never transmitted.

[ ... ]

> @@ -449,6 +787,21 @@ void netc_port_txtstamp_handler(struct dsa_switch *ds, int port,
>  		dev_dbg_ratelimited(np->switch_priv->dev,
>  				    "Port %d ts_req_id %u which seems lost\n",
>  				    port, ts_req_id);
> +
> +		return;
> +	}
> +
> +	if (NETC_SKB_CB(skb_match)->ptp_flag == NETC_PTP_FLAG_ONESTEP) {
> +		spin_lock_bh(&np->onestep_lock);
> +		if (likely(np->onestep_state == NETC_ONESTEP_IN_FLIGHT &&
> +			   np->onestep_ts_req_id == ts_req_id) ||
> +		    np->onestep_state == NETC_ONESTEP_IDLE) {
> +			np->onestep_state = NETC_ONESTEP_SCHEDULED;
> +			schedule_work(&np->onestep_work);
> +		}
> +		spin_unlock_bh(&np->onestep_lock);
> +		consume_skb(skb_match);
> +
>  		return;
>  	}

[Severity: Medium]
Can the ts_req_id comparison here match a different frame?  The clone is
unlinked from np->tstamp_queue under tstamp_lock earlier in this function,
which immediately makes the 4-bit id reusable, and onestep_lock is only
taken afterwards:

CPU0 (work, frame A response overdue past the window)
  netc_port_onestep_sync_enqueue()
     netc_onestep_timeout() -> true, schedule_work()
  netc_port_onestep_work()
     spin_lock_bh(&np->onestep_lock);

CPU1 (A's response arrives)
  netc_port_txtstamp_handler()
     __skb_unlink(clone_A, &np->tstamp_queue);   /* id X now free */
     spin_unlock_bh(&np->tstamp_lock);
     spin_lock_bh(&np->onestep_lock);            /* blocks */

CPU0 continues
     netc_get_ts_req_id() -> X again for frame B
     np->onestep_ts_req_id = X;
     np->onestep_state = NETC_ONESTEP_IN_FLIGHT;
     spin_unlock_bh(&np->onestep_lock);
     onestep_sync_xmit(B)

CPU1 then sees IN_FLIGHT with onestep_ts_req_id == X and treats A's
response as B's completion, sets SCHEDULED and re-arms the work, which can
transmit frame C and reprogram PM_SINGLE_STEP while B is still pending in
hardware.  Workqueue non-reentrancy serializes the worker, not frame
completion.

Would retiring the id and moving the one-step state under a single lock
order (onestep_lock then tstamp_lock), or matching on a generation counter
instead of the reusable 4-bit id, close this?

[ ... ]

> diff --git a/drivers/net/dsa/netc/netc_switch_hw.h b/drivers/net/dsa/netc/netc_switch_hw.h
> index 1404ae41c7bc..37d1dd7ec2c7 100644
> --- a/drivers/net/dsa/netc/netc_switch_hw.h
> +++ b/drivers/net/dsa/netc/netc_switch_hw.h
> @@ -203,6 +203,11 @@ enum netc_stg_stage {
>  #define   SSP_10M			1
>  #define   SSP_1G			2
>  
> +#define NETC_PM_SINGLE_STEP(a)		(0x10c0 + (a) * 0x400)
> +#define  PM_SINGLE_STEP_CH		BIT(6)
> +#define  PM_SINGLE_STEP_OFFSET		GENMASK(15, 7)
> +#define  PM_SINGLE_STEP_EN		BIT(31)
> +

[ ... ]

> diff --git a/net/dsa/tag_netc.c b/net/dsa/tag_netc.c
> index 6b492451d092..c1c0ffee75c1 100644
> --- a/net/dsa/tag_netc.c
> +++ b/net/dsa/tag_netc.c

[ ... ]

> @@ -129,17 +139,69 @@ static void netc_fill_tp_tag_subtype2(struct sk_buff *skb,
>  static struct sk_buff *netc_xmit(struct sk_buff *skb,
>  				 struct net_device *ndev)
>  {
>  	u8 ptp_flag = NETC_SKB_CB(skb)->ptp_flag;
>  
>  	/* Fast path: the overwhelming majority of frames are not PTP frames */
> -	if (likely(!ptp_flag))
> +	if (likely(!ptp_flag)) {
>  		netc_fill_tp_tag_subtype0(skb, ndev);
> -	else
> -		/* ptp_flag == NETC_PTP_FLAG_TWOSTEP */
> +	} else if (ptp_flag == NETC_PTP_FLAG_TWOSTEP) {
>  		netc_fill_tp_tag_subtype2(skb, ndev);
> +	} else if (ptp_flag == NETC_PTP_FLAG_ONESTEP) {
> +		/* The switch driver takes ownership of the one-step Sync and
> +		 * queues it for deferred TX; the deferred work tags it subtype
> +		 * 3 and transmits it directly to the conduit. Return NULL so
> +		 * dsa_user_xmit() stops processing this skb.
> +		 */
> +		netc_onestep_sync_enqueue(skb, ndev);
> +		skb = NULL;
> +	} else {
> +		/* NETC_PTP_FLAG_DROP */
> +		kfree_skb(skb);
> +		skb = NULL;
> +	}
>  
>  	return skb;
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918072852.501420-1-wei.fang%40oss.nxp.com

RE: [PATCH v4 net-next 3/8] ptp: netc: export netc_timer_get_current_time() for cross-driver use

[Wei Fang]

> > +int netc_timer_get_current_time(struct pci_dev *pdev, u64 *ns)
> > +{
> > +	struct netc_timer *priv = NULL;
> > +	struct netc_timer *tmp;
> > +	unsigned long flags;
> > +	int err = 0;
> > +
> > +	/* Serialize against driver unbind, so holding it here ensures that
> > +	 * priv remains valid for the entire duration of the register read.
> > +	 */
> > +	spin_lock_bh(&netc_timer_list_lock);
> 
> [Severity: Medium]
> Is it necessary to hold the module-global netc_timer_list_lock across the
> MMIO access, rather than only across the lookup?
> 
> priv->lock is taken from hardirq context with a plain spin_lock() in
> netc_timer_isr(), and that handler can spin in the register drain loop in
> netc_timer_handle_etts_event():
> 
> drivers/ptp/ptp_netc.c:netc_timer_handle_etts_event() {
> 	while (netc_timer_rd(priv, NETC_TMR_STAT) & TMR_STAT_ETS_VLD(index))
> 		etts = netc_timer_rd64(priv, NETC_TMR_ETTS_L(index));
> }
> 
> So while CPU A sits in netc_timer_get_current_time() waiting on
> spin_lock_irqsave(&priv->lock) with softirqs disabled and the global list lock
> held, CPU B can be inside netc_timer_isr() holding priv->lock for a period
> bounded only by hardware. During that window every other Timer's probe(),
> remove() and every other time query is serialized behind
> netc_timer_list_lock. The same applies if the target function is unreachable
> and the non-posted read stalls until the PCIe completion timeout.
> 
> Would an RCU-protected lookup (with the priv free deferred in remove()), or
> taking a reference on the Timer, give the same lifetime guarantee without
> holding a global lock across the register access?
> 
> Related to that: the BH-only requirement makes the exported symbol illegal
> from hardirq or with interrupts disabled, but that constraint appears only in
> the kerneldoc and is not visible at the prototype in
> include/linux/fsl/netc_global.h, and nothing but an eventual WARN from
> __local_bh_enable_ip() catches a violation. Is there a reason not to drop the
> context restriction by using a lookup scheme that does not disable BH?
> 

There is effectively no contention on the global lock. NETC has at most 3 Timer
instances, and the Switch can only use Timer0. netc_timer_get_current_time()
is used solely by the Switch driver - the ENETC driver never calls it. At runtime,
only the Switch driver ever takes netc_timer_list_lock; the other Timer instances
take it only during their own probe()/remove(). So in practice the lock has no
competing runtime users. On top of that, the drain loop in
netc_timer_handle_etts_event() is bounded - the hardware time-stamp FIFO
holds at most 16 entries, so the ISR holds priv->lock for at most 16 register reads,
not an unbounded time.

With no real contender for the lock, switching to RCU would only add complexity
without a meaningful gain.

> > +
> > +	list_for_each_entry(tmp, &netc_timer_list, node) {
> > +		if (tmp->pdev == pdev) {
> > +			priv = tmp;
> > +			break;
> > +		}
> > +	}
> 
> [Severity: Low]
> This isn't a bug, but the lookup is a bare pointer comparison on a
> caller-supplied struct pci_dev *, so its soundness depends on the caller
> holding a reference on @pdev. The in-tree consumer does hold one
> (netc_get_ptp_timer() obtains it with pci_get_domain_bus_and_slot() and
> drops it with pci_dev_put()), so nothing is broken today.
> 
> Could the kerneldoc and the exported prototype state that the caller must
> hold a reference on @pdev? Without it, a future caller that caches a raw
> pci_dev pointer has no hint that a freed and recycled pointer could match a
> different Timer's entry.

The only caller is the NETC switch driver, I do not think we need to update 
the kernel-doc.

> 
> > +
> > +	if (!priv) {
> > +		err = -ENODEV;
> > +		goto netc_timer_list_unlock;
> > +	}
> > +
> > +	spin_lock_irqsave(&priv->lock, flags);
> > +	*ns = netc_timer_cur_time_read(priv);
> > +	spin_unlock_irqrestore(&priv->lock, flags);
> 
> [Severity: Medium]
> Can this return 0 together with a meaningless timestamp when the Timer
> function is not accessible?
> 
> List membership only proves the pci_dev is bound to this driver: the entry is
> added at the end of netc_timer_probe() and removed only in
> netc_timer_remove(). netc_timer_driver registers no PM callbacks, so system
> suspend takes the PCI core default path and the function is disabled and put
> into D3 while the list entry stays live, and nothing re-programs
> TMR_CTRL[TE], CK_SEL, TMR_ADD or TMR_CNT on resume. The driver already
> documents the constraint in netc_timer_init():
> 
> drivers/ptp/ptp_netc.c:netc_timer_init() {
> 	/* Software must enable timer first and the clock selected must be
> 	 * active, otherwise, the registers which are in the timer clock
> 	 * domain are not accessible.
> 	 */
> }
> 
> The same holds for a surprise-removed function or one contained by DPC/AER,
> where the read returns all-ones. In those cases the helper still reports
> success, and the switch side consumes the value verbatim as the one-step Sync
> originTimestamp in netc_port_onestep_work():
> 
> drivers/net/dsa/netc/netc_ptp.c:netc_port_onestep_work() {
> 	NETC_SKB_CB(skb)->tstamp = tstamp;
> }
> 
> Would it make sense to gate the read on pdev->current_state == PCI_D0 and
> !pci_channel_offline(pdev), or on TMR_CTRL[TE], and return an error instead
> of a successful-looking value? The missing PM support is not introduced by
> this patch, but the new export is what turns an inaccessible-device read into
> a timestamp handed to another driver.
> 

NETC is a Root Complex integrated Endpoint (RCiEP). The Switch, ENETC and
Timer are all on-chip integrated functions with no external PCIe link and no
physical connector. So surprise removal and AER/DPC containment simply
cannot occur here.

Suspend/resume is not supported anywhere in the NETC subsystem - the Switch,
ENETC and Timer drivers have no PM callbacks. When PM support is added later,
it will guarantee that the Timer suspends after the Switch and resumes before it,
so the Timer is always programmed and accessible whenever the Switch may
query it. 

RE: [PATCH v4 net-next 5/8] net: dsa: netc: check return value of ntmp_ipft_delete_entry()

[Wei Fang]

> > ntmp_ipft_delete_entry() may fail, but the driver ignored its return
> > value, so a failed deletion left the IPFT entry untracked and
> > impossible to clean up later. This is harmless today because the port
> > also disables its ingress port filter table lookup, so the leftover
> > entry is never matched.
> 
> [Severity: Medium]
> Is the "harmless today" statement accurate for the replacement path that
> this patch reorders?

The harmless meaning here is that when host flooding is disabled, even if
hardware entries are not deleted, the flooding rules that have not been
deleted will not take effect because IPFT lookup is disabled. I will improve
the commit message to make it clear.

> 
> In the pre-patch netc_port_set_host_flood(), the new rule was added
> first, which sets np->ipft_hf_eid to the new entry ID and writes
> NETC_PIPFCR = PIPFCR_EN, and only afterwards was the old ID deleted:
> 
> 	old_entry_id = np->ipft_hf_eid;
> 
> 	if (netc_port_add_host_flood_rule(np, uc, mc)) {
> 	...
> 	/* Remove the old host flood entry */
> 	netc_port_remove_host_flood(np, old_entry_id);
> 
> The old helper only disabled the lookup when the deleted ID matched the
> tracked one:
> 
> 	if (np->ipft_hf_eid == entry_id)
> 		disable_host_flood = true;
> 
> Since np->ipft_hf_eid had already been updated to the new entry, that
> condition was false here, so NETC_PIPFCR = 0 was never written.  Doesn't
> that mean a failed delete left an active, untracked IPFT redirect entry
> with the lookup still enabled, rather than an entry that "is never
> matched"?
> 
> For the promisc to allmulti-only transition (uc=1,mc=1 to uc=0,mc=1) the
> stale entry has an empty DMAC mask, so it matches every frame from that
> source port and redirects it to the host.  Would the port then stay
> effectively promiscuous after userspace disabled promiscuous mode, with
> the ternary IPFT entry leaked permanently because its ID is no longer
> recorded anywhere?
> 
> The delete can fail in practice: ntmp_ipft_delete_entry() ->
> ntmp_delete_entry_by_id() -> ntmp_alloc_data_mem() can return -ENOMEM
> from dma_alloc_coherent(GFP_KERNEL), and netc_xmit_ntmp_cmd() can
> return
> -EBUSY, -ETIMEDOUT (read_poll_timeout() on cbdr->regs.cir) or -EIO.
> 
> Could the changelog be updated to describe the actual impact?
> 
> > Signed-off-by: Wei Fang <wei.fang@nxp.com>
> 
> [Severity: Low]
> Should this carry a Fixes: tag?  The ignored return value together with
> the add-before-delete ordering came in with commit
> 46d6407692c80b258ecba3af831130a6f6e2feea ("net: dsa: netc: add FDB,
> STP, MTU, port setup and host flooding support"), which is already in
> the tree, and the effect described above (a port keeping a redirect the
> current RX flags no longer request, plus a leaked TCAM entry) is user
> visible.

This is only a theoretical issue; we haven't encountered this problem in
practice. I think it's acceptable as an improvement rather than a bug fix.

> 
> > diff --git a/drivers/net/dsa/netc/netc_main.c
> b/drivers/net/dsa/netc/netc_main.c
> > index c590931a4e1b..082ed9405868 100644
> > --- a/drivers/net/dsa/netc/netc_main.c
> > +++ b/drivers/net/dsa/netc/netc_main.c
> 
> [ ... ]
> 
> > @@ -1761,32 +1755,31 @@ static int netc_port_add_host_flood_rule(struct
> netc_port *np,
> >  	return err;
> >  }
> >
> > -static void netc_port_remove_host_flood(struct netc_port *np, u32 entry_id)
> > +static int netc_port_remove_host_flood(struct netc_port *np)
> >  {
> >  	struct netc_switch *priv = np->switch_priv;
> > -	bool disable_host_flood = false;
> > +	u32 entry_id = np->ipft_hf_eid;
> > +	int err;
> >
> >  	if (entry_id == NTMP_NULL_ENTRY_ID)
> > -		return;
> > +		return 0;
> >
> > -	if (np->ipft_hf_eid == entry_id)
> > -		disable_host_flood = true;
> > +	err = ntmp_ipft_delete_entry(&priv->ntmp, entry_id);
> > +	if (err)
> > +		return err;
> >
> > -	ntmp_ipft_delete_entry(&priv->ntmp, entry_id);
> > +	np->ipft_hf_eid = NTMP_NULL_ENTRY_ID;
> > +	np->uc = false;
> > +	np->mc = false;
> > +	netc_port_wr(np, NETC_PIPFCR, 0);
> 
> [ ... ]
> 
> > @@ -1803,19 +1801,24 @@ static void netc_port_set_host_flood(struct
> dsa_switch *ds, int port,
> >  		return;
> >
> >  	/* IPFT does not support in-place updates to the KEYE element,
> > -	 * we need to add a new entry and then delete the old one. So
> > -	 * save the old entry ID first.
> > +	 * we need to delete the old one and then add the new rule. If
> > +	 * the deletion fails, return immediately.
> >  	 */
> > -	old_entry_id = np->ipft_hf_eid;
> > -
> > -	if (netc_port_add_host_flood_rule(np, uc, mc)) {
> > -		dev_err(ds->dev, "Failed to add host flood rule on port %d\n",
> > +	if (netc_port_remove_host_flood(np)) {
> > +		dev_err(ds->dev,
> > +			"Failed to delete old host flood rule on port %d\n",
> >  			port);
> > +
> >  		return;
> >  	}
> >
> > -	/* Remove the old host flood entry */
> > -	netc_port_remove_host_flood(np, old_entry_id);
> > +	/* Restoring the previous configuration is pointless because
> > +	 * .port_set_host_flood() returns void, so the upper layer cannot
> > +	 * detect the error and the RX flags have changed.
> > +	 */
> > +	if (netc_port_add_host_flood_rule(np, uc, mc))
> > +		dev_err(ds->dev,
> > +			"Failed to add host flood rule on port %d\n", port);
> 
> [Severity: Medium]
> With the new ordering, what happens to host flooding when the add fails?
> 
> netc_port_remove_host_flood() has already deleted the entry and cleared
> the tracked state:
> 
> 	np->ipft_hf_eid = NTMP_NULL_ENTRY_ID;
> 	np->uc = false;
> 	np->mc = false;
> 	netc_port_wr(np, NETC_PIPFCR, 0);
> 
> netc_port_add_host_flood_rule() can then fail on kzalloc_obj() returning
> NULL, or on ntmp_ipft_add_entry() returning -ENOMEM/-EBUSY/-ETIMEDOUT/
> -EIO from the command BD transaction.  In that case the port ends up
> with no host flood entry at all while the netdev still has IFF_PROMISC
> or IFF_ALLMULTI set, and only a dev_err() is emitted.
> 
> Before this change the add ran first, so an add failure left the
> previous working rule installed and np->uc/np->mc untouched.
> 
> Since .port_set_host_flood() is void and is only reached from
> dsa_user_change_rx_flags() -> dsa_user_manage_host_flood(), nothing
> reports or retries the failure.  Does host unicast/multicast flooding
> for that port then stay silently off until userspace toggles the RX
> flags again?
> 
> The comment says restoring the previous configuration is pointless.  For
> the common promisc to allmulti transition the old rule is a superset of
> what is now requested, so wouldn't keeping it still satisfy the new
> request and be better than delivering nothing?

The failure mode you describe is real, but it's a deliberate trade-off:

IPFT does not support in-place updates to the KEYE element, so the old
entry must be deleted before the new one is added. Keeping the "add-first,
delete-later" order would force each port to track two or more host flood
rules when the old deletion fails, which this patch intentionally moves away
from - especially since IPFT will soon serve other features (PTP trapping,
flow policing) where disabling the port's whole lookup is no longer acceptable.

The impact is bounded. An add failure only happens under memory pressure
or a command-BD error (rare), and the effect is that the port's host flooding is
temporarily off until userspace toggles the RX flags again - a recoverable
functional degradation, not a memory-safety issue.

> 
> [Severity: Low]
> Separately, is the window with no rule installed a concern even when
> both operations succeed?  Between the delete and the add the port has no
> host flood redirect, and the gap covers a full synchronous NTMP command
> BD transaction for the delete (dma_alloc_coherent(GFP_KERNEL) plus
> netc_xmit_ntmp_cmd() polling through read_poll_timeout()), a
> kzalloc_obj() that may enter direct reclaim, and a second command BD
> transaction for the add, which only re-arms the redirect at the end:
> 
> 	err = ntmp_ipft_add_entry(&priv->ntmp, host_flood);
> 	if (err)
> 		goto free_host_flood;
> 	...
> 	/* Enable ingress port filter table lookup */
> 	netc_port_wr(np, NETC_PIPFCR, PIPFCR_EN);
> 
> During that interval, frames that only reach the CPU through the host
> flood redirect (multicast under IFF_ALLMULTI when going from
> (uc=0,mc=1) to (uc=1,mc=1), or unknown-DA unicast under IFF_PROMISC)
> are dropped.  The old add-then-delete ordering kept a matching entry
> armed at every instant.

Yes, the window is real, but I suppose it is acceptable when switching the
configuration. As stated above, the "delete-first, add-later" order is a
trade-off. Otherwise, each port may need to track two or more host flood
rules when the old deletion fails.

> 
> [ ... ]
> 
> > @@ -2015,6 +2024,11 @@ static int netc_port_bridge_join(struct
> dsa_switch *ds, int port,
> >  disable_mlo:
> >  	netc_port_set_mlo(np, MLO_DISABLE);
> >
> > +	if (netc_port_add_host_flood_rule(np, uc, mc))
> > +		dev_err(ds->dev,
> > +			"Failed to restore host flood rule on port %u\n",
> > +			port);
> 
> [Severity: Low]
> This isn't a bug, but port is declared int by the .port_bridge_join
> prototype, and the two other messages added by this patch use %d for
> the same value ("Failed to delete old host flood rule on port %d" and
> "Failed to add host flood rule on port %d").  Should this one use %d as
> well?

Yes, this should be improved.

RE: [PATCH v4 net-next 7/8] net: dsa: netc: add PTP two-step timestamping support

[Wei Fang]

> This adds RX timestamping via IPFT rules that trap PTP frames to the CPU
> port, and two-step TX timestamping by cloning the skb, assigning a 4-bit
> per-port request ID and matching the hardware response frame back to the
> queued clone.  It also adds the per-switch netc_tagger_data and grabs the
> PTP timer's pci_dev so get_ts_info() can report the PHC index.
> 
> [Severity: Low]
> The changelog says:
> 
> > The tagger dispatches both to the driver through the
> > twostep_tstamp_handler callback in netc_tagger_data, which matches the
> > ID to the queued clone and completes it via skb_complete_tx_timestamp().
> 
> Is that the right member name?  The struct added by this patch in
> include/linux/dsa/tag_netc.h is:

Sorry, this is an error, I will fix it.

> 
> struct netc_tagger_data {
> 	void (*txtstamp_handler)(struct dsa_switch *ds, int port,
> 				 u8 ts_req_id, u64 ts);
> };
> 
> and net/dsa/tag_netc.c:netc_twostep_tstamp_process() calls
> tagger_data->txtstamp_handler(), so grepping for twostep_tstamp_handler
> finds nothing.
> 
> The same paragraph mentions netc_connect()/netc_disconnect() on the tagger
> side, but not the new .connect_tag_protocol op
> netc_connect_tag_protocol(), which is what actually installs the handler.
> Could that be mentioned as well so the dispatch path is followable from
> the changelog?
> 
> > diff --git a/drivers/net/dsa/netc/netc_main.c
> b/drivers/net/dsa/netc/netc_main.c
> > index 336f5a4633e9..03200fc63a09 100644
> > --- a/drivers/net/dsa/netc/netc_main.c
> > +++ b/drivers/net/dsa/netc/netc_main.c
> 
> [ ... ]
> 
> > @@ -949,6 +993,21 @@ static void netc_destroy_all_lists(struct
> netc_switch *priv)
> >  	mutex_destroy(&priv->vft_lock);
> >  }
> >
> > +static void netc_free_ports_resources(struct netc_switch *priv)
> > +{
> > +	struct dsa_port *dp;
> > +
> > +	dsa_switch_for_each_available_port(dp, priv->ds) {
> > +		struct netc_port *np = priv->ports[dp->index];
> > +
> > +		if (!dsa_port_is_user(dp))
> > +			continue;
> > +
> > +		disable_delayed_work_sync(&np->tstamp_timeout_work);
> > +		netc_port_purge_tstamp_queue(np);
> > +	}
> > +}
> > +
> >  static void netc_teardown(struct dsa_switch *ds)
> >  {
> >  	struct netc_switch *priv = ds->priv;
> > @@ -956,6 +1015,8 @@ static void netc_teardown(struct dsa_switch *ds)
> >  	disable_delayed_work_sync(&priv->fdbt_ageing_work);
> >  	netc_destroy_all_lists(priv);
> >  	netc_free_ntmp_user(priv);
> > +	netc_free_ports_resources(priv);
> > +	pci_dev_put(priv->tmr_dev);
> >  }
> 
> [Severity: Medium]
> Do the PTP IPFT entries survive teardown here?
> 
> netc_free_ports_resources() cancels the timeout work and purges the clone
> queue, but nothing calls netc_port_del_ptp_filter(), which is the only
> release path for np->ptp_ipft_eid[].  It is static in netc_ptp.c and
> reachable only from netc_port_set_ptp_filter(), i.e. only from a user-space
> hwtstamp request.
> 
> So after unbind, up to five entries per user port are still live in the
> IPFT with FLTFA=REDIRECT, host reason NETC_HR_PTP_TRAP and
> IPFT_TIMECAPE,
> while struct netc_port (holding the entry IDs) is devm-freed.
> 
> There is also an ordering question: netc_teardown() calls
> netc_free_ntmp_user() before netc_free_ports_resources():
> 
> 	netc_free_ntmp_user(priv);	/* destroys the command BD rings */
> 	netc_free_ports_resources(priv);
> 
> Since ntmp_ipft_delete_entry() needs those rings, can the table cleanup be
> added at the natural place with this ordering, or should
> netc_free_ports_resources() move before netc_free_ntmp_user()?
> 
> In a multi-switch tree this looks observable without a PCI re-probe:
> unbinding one switch runs dsa_tree_teardown() (hence netc_teardown() for
> the sibling), and re-binding runs netc_setup() again on the surviving priv,
> where netc_port_ptp_init() resets ptp_ipft_eid[] to NTMP_NULL_ENTRY_ID
> while the old entries are still programmed.  Would PTP frames then keep
> being trapped and time-captured with HWTSTAMP_FILTER_NONE, with no way
> to
> delete the stale entries?  For the single-switch case the pcie_flr() in
> netc_switch_pci_init() at probe seems to hide it.

Currently we do not support multi-switch, so this is not an issue in current
stage, but this is a good point, I will improve the setup/teardown logic to
facilitate future expansion to support a multi-switch tree.

> 
> [ ... ]
> 
> > diff --git a/drivers/net/dsa/netc/netc_ptp.c
> b/drivers/net/dsa/netc/netc_ptp.c
> > new file mode 100644
> > index 000000000000..2e743443ca03
> > --- /dev/null
> > +++ b/drivers/net/dsa/netc/netc_ptp.c
> > @@ -0,0 +1,492 @@
> 
> [ ... ]
> 
> > +static void netc_port_tstamp_timeout_work(struct work_struct *work)
> > +{
> > +	struct netc_port *np = container_of(work, struct netc_port,
> > +					    tstamp_timeout_work.work);
> > +	struct sk_buff_head free_list;
> > +	struct sk_buff *skb, *skb_tmp;
> > +
> > +	__skb_queue_head_init(&free_list);
> > +
> > +	spin_lock_bh(&np->tstamp_lock);
> > +	skb_queue_walk_safe(&np->tstamp_queue, skb, skb_tmp) {
> > +		if (time_before64(jiffies_64, NETC_SKB_CB(skb)->ptp_tx_time +
> > +				  NETC_TSTAMP_TIMEOUT))
> > +			continue;
> 
> [Severity: Low]
> Should this use get_jiffies_64() rather than reading jiffies_64 directly?

i.MX94 and future SoCs are all arm64 architecture, and we have added
"depends on ARM64" dependency to the Kconfig. So this is not an issue.

> The same raw read is used in netc_port_prepare_twostep():
> 
> 	NETC_SKB_CB(clone)->ptp_tx_time = jiffies_64;
> 
> include/linux/jiffies.h says:
> 
>  * The 64-bit value is not atomic on 32-bit systems - you MUST NOT read it
>  * without sampling the sequence number in jiffies_lock.
>  * get_jiffies_64() will do this for you as appropriate.
> 
> Since this comparison is the only exit criterion for a queued clone, a torn
> read on a 32-bit build could either purge it immediately or never purge it
> (retaining the clone, its socket reference from skb_clone_sk() and its
> ts_req_id).  The Kconfig is "depends on ARM64 || COMPILE_TEST", so only
> 32-bit COMPILE_TEST builds compile the racy read today.  ocelot uses plain
> jiffies with time_is_before_jiffies() for the same pattern.
> 
> > +
> > +		dev_dbg_ratelimited(np->switch_priv->dev,
> > +				    "Port %d ts_req_id %u which seems lost\n",
> > +				    np->dp->index, NETC_SKB_CB(skb)->ts_req_id);
> > +
> > +		__skb_unlink(skb, &np->tstamp_queue);
> > +		__skb_queue_tail(&free_list, skb);
> > +	}
> 
> [ ... ]
> 
> > +static int netc_get_ts_req_id(struct netc_port *np)
> > +{
> > +	DECLARE_BITMAP(ts_req_id_bitmap, NETC_NUM_TS_REQ_ID);
> > +	struct sk_buff *skb, *skb_tmp;
> > +	unsigned long ts_req_id;
> > +
> > +	bitmap_zero(ts_req_id_bitmap, NETC_NUM_TS_REQ_ID);
> > +
> > +	skb_queue_walk_safe(&np->tstamp_queue, skb, skb_tmp)
> > +		__set_bit(NETC_SKB_CB(skb)->ts_req_id, ts_req_id_bitmap);
> > +
> > +	ts_req_id = find_first_zero_bit(ts_req_id_bitmap, NETC_NUM_TS_REQ_ID);
> 
> [Severity: Medium]
> Can a timed-out request ID be handed out again while the hardware response
> for it is still in flight?
> 
> Availability here is derived purely from the clones currently queued, and
> netc_port_tstamp_timeout_work() frees an ID by unlinking the clone without
> knowing whether the frame has even been transmitted.  The timeout clock
> starts before the frame reaches the conduit:
> 
> netc_port_prepare_twostep()
> 	NETC_SKB_CB(clone)->ptp_tx_time = jiffies_64;
> 	__skb_queue_tail(&np->tstamp_queue, clone);
> 
> so egress queueing delay (link down/flapping, or sustained PAUSE from the
> link partner) counts against the 5 second budget.  If that delay exceeds
> NETC_TSTAMP_TIMEOUT:
> 
> netc_port_tstamp_timeout_work()	-> drops clone, frees ID
> netc_get_ts_req_id()		-> hands the same ID to a new transmit
> netc_port_txtstamp_handler()	-> late response matches the new clone
> 
> and netc_port_txtstamp_handler() matches on nothing but port and the 4-bit
> ID:
> 
> 	if (NETC_SKB_CB(skb)->ts_req_id != ts_req_id)
> 		continue;
> 
> 	__skb_unlink(skb, &np->tstamp_queue);
> 
> Would that report the old frame's transmit time for the new frame, and then
> discard the correct response as lost?  ocelot_port_dequeue_ptp_tx_skb()
> additionally compares the PTP sequenceId before completing a queued clone;
> since the NETC response tag only carries the 4-bit ID, would a generation
> counter or an ID quarantine be needed here?

This is an unavoidable problem and a limitation of the current hardware; the
response frame returned by the hardware only contains ts_req_id and timestamp.
We can only match the skb in the queue based on ts_req_id. However, this problem
doesn't have much impact because timeouts only occur in extreme cases. In such
cases, PTP synchronization itself will be affected, but once the extreme situation
disappears, subsequent PTP synchronization will return to normal.

> > +void netc_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
> > +{
> > +	struct netc_port *np = NETC_PORT(ds, port);
> > +	u32 ptp_class;
> > +	int tx_type;
> > +
> > +	NETC_SKB_CB(skb)->ptp_flag = 0;
> > +	ptp_class = ptp_classify_raw(skb);
> > +	if (ptp_class == PTP_CLASS_NONE)
> > +		return;
> > +
> > +	/* The rx_filters in netc_get_ts_info() has already declared that
> > +	 * it only supports PTP v2, so TX only supports v2 as well.
> > +	 */
> > +	if (unlikely(ptp_class & PTP_CLASS_V1))
> > +		return;
> 
> [Severity: Low]
> This isn't a bug for real PTP stacks, but is the comment's reasoning right?
> The advertised rx_filters describe what the ingress filter can match; the
> uapi definition of the TX mode makes no protocol statement:
> 
> include/uapi/linux/net_tstamp.h
> 	/*
> 	 * Enables hardware time stamping for outgoing packets;
> 	 * the sender of the packet decides which are to be
> 	 * time stamped ...
> 	 */
> 	HWTSTAMP_TX_ON,
> 
> netc_get_ts_info() advertises HWTSTAMP_TX_ON unconditionally and
> netc_port_hwtstamp_set() accepts it even with HWTSTAMP_FILTER_NONE, yet
> a
> socket asking for SOF_TIMESTAMPING_TX_HARDWARE on a non-PTP or PTPv1
> frame
> silently gets no timestamp here.  Could the comment be reworded to state
> the hardware/driver TX restriction directly instead of deriving it from the
> RX filters?
> 

PTP is bidirectional, not unidirectional; there is no situation where TX uses v1
and RX uses v2.

RE: [PATCH v4 net-next 8/8] net: dsa: netc: add PTP one-step timestamping support

[Wei Fang]

> > diff --git a/drivers/net/dsa/netc/netc_ptp.c b/drivers/net/dsa/netc/netc_ptp.c
> > index 2e743443ca03..1bc0f0956206 100644
> > --- a/drivers/net/dsa/netc/netc_ptp.c
> > +++ b/drivers/net/dsa/netc/netc_ptp.c
> > @@ -11,6 +11,15 @@
> >
> >  #define NETC_NUM_TS_REQ_ID		16
> >  #define NETC_TSTAMP_TIMEOUT		(5 * HZ)
> > +#define NETC_MAX_STEP_OFFSET		0x1ff
> > +#define NETC_ONESTEP_QTH		512
> > +/* The 30-bit timestamp of the To_Port subtype 3 tag lets the hardware
> > + * account for a single wrap, so the correction field of a one-step Sync
> > + * frame is only correct if it is sent out within 2^30 ns after the
> > + * software timestamp is read. Past this window the frame is beyond
> > + * repair, and PM_SINGLE_STEP becomes safe to reprogram.
> > + */
> 
> [Severity: Medium]
> This comment argues that the correction value is beyond repair after 2^30 ns,
> but PM_SINGLE_STEP also carries the correction-field offset and the
> "update UDP checksum" bit, which the hardware still applies at SFD time to
> whatever frame is being transmitted.
> 
> If the previously programmed frame is still sitting in the switch or port
> egress path (egress congestion, flow control, TAS gating), the next
> netc_port_set_onestep_control() call can change offset and CH under it.  For
> an L2 PTP frame the correction offset is around 22, for UDP/IPv6 around 74,
> so the MAC would write 8 bytes of correction data at the wrong position
> inside the pending frame and may recompute its UDP checksum.
> 
> The "no ts_req_id available" branch of netc_port_onestep_work() has the same
> shape: it overwrites np->onestep_state with NETC_ONESTEP_SCHEDULED,
> which
> erases the record that a frame is in flight at all.
> 
> Would it be safer to only reprogram the register when the new frame's
> offset/CH match the pending one, or to hold off until the pending frame is
> known to be gone?

As stated in the comment, "Past this window the frame is beyond repair",
that is to say, when the sync frame leaves the port, the correction field
is not correct. When PM_SINGLE_STEP is programmed after a timeout,
there are essentially two possibilities: either the value of PM_SINGLE_STEP
remains unchanged, with no impact; or PM_SINGLE_STEP changes, in this
case the frame will be corrupted and either discarded by the sender or
receiver, which is as expected.

> 
> > +#define NETC_ONESTEP_VALID_WINDOW	0x40000000 /* ns */
> >
> >  static void netc_port_tstamp_timeout_work(struct work_struct *work)
> >  {
> > @@ -68,12 +77,276 @@ static int netc_get_ts_req_id(struct netc_port *np)
> >  	return ts_req_id;
> >  }
> >
> > +static int netc_get_phc_time(struct netc_switch *priv, u64 *ns)
> > +{
> > +	if (unlikely(!priv->tmr_dev))
> > +		return -ENODEV;
> > +
> > +	return netc_timer_get_current_time(priv->tmr_dev, ns);
> > +}
> > +
> > +static void netc_port_set_onestep_control(struct netc_port *np,
> > +					  bool csum_update, int offset)
> > +{
> > +	u32 val;
> > +
> > +	val = PM_SINGLE_STEP_EN | FIELD_PREP(PM_SINGLE_STEP_OFFSET,
> offset);
> > +	if (csum_update)
> > +		val |= PM_SINGLE_STEP_CH;
> > +	netc_mac_port_wr(np, NETC_PM_SINGLE_STEP(0), val);
> 
> [Severity: Low]
> This isn't a bug on today's hardware, but netc_mac_port_wr() returns without
> touching the register when is_netc_pseudo_port(np) is true, and it reports
> nothing back to the caller.
> 
> The one-step machinery is gated only on dsa_port_is_user(), and
> netc_get_ts_info() advertises HWTSTAMP_TX_ONESTEP_SYNC unconditionally,
> so a
> user port with caps.pseudo_link set would get its originTimestamp rewritten
> and a subtype-3 tag attached while the MAC single-step engine was never
> enabled.
> 
> That combination looks impossible right now: imx94_switch_phylink_get_caps()
> only marks port 3 (the CPU port) as internal, and the binding requires
> ethernet-port@3 to carry the ethernet = <&enetc3> phandle.  Would it still be
> worth refusing one-step on pseudo-link ports explicitly so a future platform
> cannot silently drop the register write?

Currently, for i.MX94 only the CPU port is pseudo MAC, so no need to add
a such check in this stage. It is better to be added when future NETCs support
pseudo MAC on user port.

> 
> > +}
> > +
> > +static void netc_port_program_onestep(struct netc_port *np,
> > +				      struct sk_buff *skb)
> > +{
> > +	u16 correction_offset = NETC_SKB_CB(skb)->correction_offset;
> > +	u16 tstamp_offset = NETC_SKB_CB(skb)->timestamp_offset;
> > +	u64 tstamp = NETC_SKB_CB(skb)->tstamp;
> > +	u8 *hdr = skb_mac_header(skb);
> 
> [ ... ]
> 
> > +	if (NETC_SKB_CB(skb)->is_udp) {
> > +		__be32 old_sec_l, old_ns;
> > +		struct udphdr *uh;
> > +		__be16 old_sec_h;
> > +
> > +		if (skb->ip_summed == CHECKSUM_PARTIAL) {
> > +			csum_update = true;
> > +			goto update_timestamp;
> > +		}
> > +
> > +		if (unlikely(!skb_transport_header_was_set(skb)))
> > +			uh = (struct udphdr *)(hdr + tstamp_offset -
> > +					       sizeof(struct ptp_header) -
> > +					       sizeof(struct udphdr));
> > +		else
> > +			uh = udp_hdr(skb);
> 
> [Severity: High]
> Can udp_hdr(skb) be trusted here?  netc_port_prepare_onestep_sync() derives
> and bounds-checks the PTP offsets itself:
> 
> 	if (unlikely(tstamp_offset + 10 > skb_headlen(skb) ||
> 		     correction_offset > NETC_MAX_STEP_OFFSET))
> 		goto set_ptp_flag_drop;
> 
> but skb->transport_header is never validated against skb_headlen(), and the
> safe computed expression is only used when the transport header was not set.
> The code then reads uh->check at offset 6 and writes 2 bytes there through
> inet_proto_csum_replace2()/inet_proto_csum_replace4().
> 
> Two AF_PACKET paths can leave transport_header at or past the tail while the
> frame still parses as a valid PTPv2 Sync over UDP with ip_summed ==
> CHECKSUM_NONE:
> 
> packet_snd()
>   packet_parse_headers()
>     skb_probe_transport_header()
>       __skb_flow_dissect()
>         key_control->thoff = min_t(u16, nhoff, skb ? skb->len : hlen);
> 
> The dissector runs with skb->protocol taken from the user-supplied
> sll_protocol and nhoff = 0, so a frame whose declared ethertype disagrees
> with its real headers can walk attacker-chosen IPv6 option hdrlen bytes and
> end with thoff clamped to skb->len, i.e. transport_header == tail.
> 
> The other path is PACKET_VNET_HDR:
> 
> virtio_net_hdr_to_skb()
>   skb_partial_csum_set()   /* csum_start = headlen - 2 accepted */
> ...
> skb_checksum_help()        /* flips ip_summed to CHECKSUM_NONE,
>                               transport_header left as-is */
> 
> With the frame length tuned so tailroom is under 8 bytes, wouldn't the
> uh->check read go past skb_tail_pointer() and the csum-replace write land
> inside struct skb_shared_info (flags/meta_len/nr_frags/tx_flags/gso_size)?
> 
> Since the validated expression is already computed just above, could it be
> used unconditionally instead of udp_hdr(skb)?  The same udp_hdr() pattern
> exists in drivers/net/ethernet/freescale/enetc/enetc.c, but this path is new
> here.

The AF_PACKET + flow-dissector path is real (transport_header can be clamped
to tail), but the OOB write can't happen:

All real reads/writes are mac-header relative, not transport_header relative.
ptp_parse_header() + ptp_classify_raw() work on actual content, and
tstamp_offset + 10 > skb_headlen() after skb_linearize_cow() bounds the whole
region. The correct UDP header is at ptp_hdr - sizeof(udphdr), so uh->check is
always in-bounds.

Hitting the udp_hdr(skb) branch as an exploit needs a frame that both passes
the mac-header bounds check and has transport_header at tail with a non-zero
uh->check - mutually inconsistent. So: not a High OOB write.

That said, your suggestion is right. The skb_transport_header_was_set() branch
is redundant and trusts an unvalidated transport_header. I'll drop it and use the
already-validated expression unconditionally.

> 
> > +
> > +		/* For IPv4, a UDP checksum of zero on the wire means "no
> > +		 * checksum". For IPv6, its UDP checksum is mandatory and
> > +		 * never zero.
> > +		 */
> > +		if (!uh->check)
> > +			goto update_timestamp;
> > +
> > +		old_sec_h = __get_unaligned_t(__be16, hdr + tstamp_offset);
> > +		old_sec_l = __get_unaligned_t(__be32, hdr + tstamp_offset + 2);
> > +		old_ns = __get_unaligned_t(__be32, hdr + tstamp_offset + 6);
> > +		inet_proto_csum_replace2(&uh->check, skb, old_sec_h,
> > +					 new_sec_h, false);
> > +		inet_proto_csum_replace4(&uh->check, skb, old_sec_l,
> > +					 new_sec_l, false);
> > +		inet_proto_csum_replace4(&uh->check, skb, old_ns,
> > +					 new_ns, false);
> > +		csum_update = true;
> > +	}
> > +
> > +update_timestamp:
> 
> [ ... ]
> 
> > +static void netc_port_onestep_work(struct work_struct *work)
> > +{
> 
> [ ... ]
> 
> > +skb_dequeue:
> > +	skb = __skb_dequeue(&np->onestep_queue);
> > +	if (!skb)
> > +		goto set_onestep_state_idle;
> > +
> > +	/* Clone is a ts_req_id token only; its payload is never read, so
> > +	 * sharing the buffer with the mutated original is fine.
> > +	 */
> > +	clone = skb_clone(skb, GFP_ATOMIC);
> > +	if (unlikely(!clone)) {
> > +		kfree_skb(skb);
> > +		goto skb_dequeue;
> > +	}
> > +
> > +	spin_lock_bh(&np->tstamp_lock);
> > +	ts_req_id = netc_get_ts_req_id(np);
> > +	if (unlikely(ts_req_id < 0)) {
> > +		spin_unlock_bh(&np->tstamp_lock);
> > +
> > +		/* Re-queuing the frame and immediately rescheduling the work
> > +		 * would busy-loop on system_percpu_wq and burn CPU until an
> > +		 * ID is freed, so drop this frame and move on to the next one
> > +		 * in the queue instead.
> > +		 */
> > +		np->onestep_state = NETC_ONESTEP_SCHEDULED;
> > +		schedule_work(&np->onestep_work);
> > +
> > +		goto onestep_unlock;
> > +	}
> 
> [Severity: Medium]
> The comment says re-queuing plus rescheduling would busy-loop, but the work
> does reschedule itself here unconditionally, and each pass destroys one
> queued one-step Sync frame at onestep_unlock.  With NETC_ONESTEP_QTH at
> 512,
> can this discard the entire backlog in quick succession?

That is intention if there is no available ts_req_id. If we do not discard the skb,
the work will be busy-loop until there is an available ts_req_id. But the issue is
that we do not know how long it will take to an available ts_req_id. So drop the
skb is a better choice.

> 
> The 16-entry ts_req_id pool is shared with the two-step path:
> netc_port_prepare_twostep() takes an ID for every PTP frame that is not a
> one-step Sync (Announce, Follow_Up, Delay_Resp still flow in one-step mode),
> and holds it for up to NETC_TSTAMP_TIMEOUT when a response is lost.  So a
> burst of two-step requests can drive this branch.
> 
> The only trace is the dev_dbg_ratelimited() inside netc_get_ts_req_id().
> Should ID exhaustion apply backpressure (leave the frame queued and arm a
> delayed retry) rather than dropping frames, and should the changelog mention
> that exhaustion means one-step Sync frames are discarded?

I don't think this makes much sense. Under normal circumstances, the frame is
sent out quickly, and if a situation arises where ts_req_id is unavailable, it will
recover quickly. Discarding the sync frame won't cause any problems, and
subsequent sync frames will still maintain PTP synchronization. If a timeout
occurs, PTP synchronization is already affected, so discarding the message is
even less of a problem.

> 
> > +
> > +	/* PHC is unavailable, drop the whole queue */
> > +	if (unlikely(netc_get_phc_time(priv, &tstamp))) {
> > +		spin_unlock_bh(&np->tstamp_lock);
> > +		goto set_onestep_state_idle;
> > +	}
> > +
> > +	NETC_SKB_CB(skb)->tstamp = tstamp;
> > +	NETC_SKB_CB(skb)->ts_req_id = ts_req_id;
> > +	NETC_SKB_CB(skb)->ptp_flag = NETC_PTP_FLAG_ONESTEP;
> > +	NETC_SKB_CB(clone)->ts_req_id = ts_req_id;
> > +	NETC_SKB_CB(clone)->ptp_tx_time = jiffies_64;
> > +	NETC_SKB_CB(clone)->ptp_flag = NETC_PTP_FLAG_ONESTEP;
> > +	np->onestep_tx_time = NETC_SKB_CB(clone)->ptp_tx_time;
> > +	np->onestep_ts_req_id = ts_req_id;
> > +
> > +	__skb_queue_tail(&np->tstamp_queue, clone);
> > +	if (!delayed_work_pending(&np->tstamp_timeout_work))
> > +		schedule_delayed_work(&np->tstamp_timeout_work,
> > +				      NETC_TSTAMP_TIMEOUT);
> > +
> > +	spin_unlock_bh(&np->tstamp_lock);
> > +
> > +	np->onestep_state = NETC_ONESTEP_IN_FLIGHT;
> > +	spin_unlock_bh(&np->onestep_lock);
> > +
> > +	netc_port_program_onestep(np, skb);
> > +	tagger_data = priv->ds->tagger_data;
> > +	tagger_data->onestep_sync_xmit(skb, np->dp->user);
> > +
> > +	return;
> > +
> > +set_onestep_state_idle:
> > +	np->onestep_state = NETC_ONESTEP_IDLE;
> > +purge_onestep_queue:
> > +	__skb_queue_purge(&np->onestep_queue);
> > +onestep_unlock:
> > +	spin_unlock_bh(&np->onestep_lock);
> > +	kfree_skb(skb);
> > +	kfree_skb(clone);
> > +}
> > +
> > +static bool netc_onestep_timeout(struct netc_port *np)
> > +{
> > +	u64 expire_time;
> > +
> > +	/* Use monotonic jiffies_64, as the PHC may be stepped backwards.
> > +	 * Add one tick since the ns-to-jiffies conversion rounds down, so
> > +	 * the software window is never shorter than the hardware window.
> > +	 */
> > +	expire_time = np->onestep_tx_time + 1 +
> > +		      nsecs_to_jiffies64(NETC_ONESTEP_VALID_WINDOW);
> > +	if (np->onestep_state == NETC_ONESTEP_IN_FLIGHT &&
> > +	    time_after64(jiffies_64, expire_time))
> > +		return true;
> > +
> > +	return false;
> > +}
> > +
> > +void netc_port_onestep_sync_enqueue(struct dsa_switch *ds, int port,
> > +				    struct sk_buff *skb)
> > +{
> > +	struct netc_port *np = NETC_PORT(ds, port);
> > +
> > +	spin_lock_bh(&np->onestep_lock);
> > +	if (unlikely(np->onestep_state == NETC_ONESTEP_PORT_INACTIVE)) {
> > +		kfree_skb(skb);
> > +		goto onestep_unlock;
> > +	}
> > +
> > +	if (unlikely(skb_queue_len(&np->onestep_queue) >= NETC_ONESTEP_QTH))
> {
> > +		dev_dbg_ratelimited(np->switch_priv->dev,
> > +				    "The onestep_queue of port %d is full\n",
> > +				    port);
> > +		kfree_skb(skb);
> > +		goto onestep_unlock;
> > +	}
> > +
> > +	__skb_queue_tail(&np->onestep_queue, skb);
> > +	if (likely(np->onestep_state == NETC_ONESTEP_IDLE) ||
> > +	    netc_onestep_timeout(np)) {
> > +		np->onestep_state = NETC_ONESTEP_SCHEDULED;
> > +		schedule_work(&np->onestep_work);
> > +	}
> > +
> > +onestep_unlock:
> > +	spin_unlock_bh(&np->onestep_lock);
> > +}
> 
> [Severity: High]
> Can the port get stuck in NETC_ONESTEP_IN_FLIGHT forever?

Yes, this is a potential issue, I will fix it.

> 
> Once netc_port_onestep_work() sets IN_FLIGHT and hands the frame to
> onestep_sync_xmit(), only three things can resume the drain:
> 
>   1. a matching response in netc_port_txtstamp_handler()
>   2. netc_onestep_timeout(), which is evaluated only from
>      netc_port_onestep_sync_enqueue()
>   3. the ts_req_id exhaustion self-reschedule
> 
> The changelog says:
> 
>     A dropped frame produces no response, so reuse tstamp_timeout_work to
>     reclaim its request ID after 5 seconds
> 
> but netc_port_tstamp_timeout_work() only unlinks and frees the timed-out
> clone:
> 
> 	__skb_unlink(skb, &np->tstamp_queue);
> 	__skb_queue_tail(&free_list, skb);
> 	...
> 	if (!skb_queue_empty(&np->tstamp_queue))
> 		schedule_delayed_work(&np->tstamp_timeout_work,
> 				      NETC_TSTAMP_TIMEOUT);
> 
> It never touches np->onestep_state and never schedules np->onestep_work.
> And once the clone has been reaped, a late response takes the !skb_match
> early return in netc_port_txtstamp_handler(), so it cannot clear IN_FLIGHT
> either.
> 
> Concretely: frame A goes IN_FLIGHT with id X, frame B is enqueued inside the
> window and only appended, A is dropped in the switch so no response arrives,
> 5 seconds later the timeout work frees A's clone and leaves IN_FLIGHT set.
> If the sender then stops, or falls below one frame per window, doesn't B and
> everything behind it stay queued and unfreed until link-down, port disable
> or a hwtstamp reconfiguration?  These are the original TX skbs and still
> hold skb->sk, so the owning socket stays charged.
> 
> The queue-full branch above also returns before the netc_onestep_timeout()
> check, so once 512 frames have accumulated even continued traffic can no
> longer recover the port.
> 
> Would it work to reset onestep_state and schedule onestep_work from the
> timeout path, or to arm a delayed work for the validity deadline, so
> recovery does not depend on new traffic arriving?
> 
> Separately, the changelog says the work is scheduled "when the PTP time has
> advanced beyond NETC_ONESTEP_VALID_WINDOW", while
> netc_onestep_timeout()
> compares monotonic jiffies_64 and only at enqueue time.
> 
> [ ... ]
> 
> > @@ -395,6 +673,66 @@ int netc_port_hwtstamp_get(struct dsa_switch *ds,
> int port,
> >  	return 0;
> >  }
> >
> > +static void netc_port_prepare_onestep_sync(struct netc_port *np,
> > +					   struct sk_buff *skb,
> > +					   u32 ptp_class, bool *twostep)
> > +{
> 
> [ ... ]
> 
> > +	if (unlikely(skb_linearize_cow(skb)))
> > +		goto set_ptp_flag_drop;
> > +
> > +	ptp_hdr = ptp_parse_header(skb, ptp_class);
> > +	if (unlikely(!ptp_hdr))
> > +		goto set_ptp_flag_drop;
> > +
> > +	msg_type = ptp_get_msgtype(ptp_hdr, ptp_class);
> 
> [ ... ]
> 
> > +set_ptp_flag_drop:
> > +	/* Drop instead of falling back to two-step: if it is a Sync,
> > +	 * one-step offload will not be executed, the timestamp in the
> > +	 * frame is inaccurate, which may affect PTP synchronization.
> > +	 */
> > +	NETC_SKB_CB(skb)->ptp_flag = NETC_PTP_FLAG_DROP;
> > +	dev_dbg_ratelimited(priv->dev,
> > +			    "Port %d: PTP frame dropped in error\n",
> > +			    np->dp->index);
> > +}
> 
> [Severity: Medium]
> The skb_linearize_cow() failure is evaluated before the message type is
> known, so a plain ENOMEM destroys any PTP frame on the port, including a
> Delay_Req that this code would otherwise have sent two-step.  Should the
> linearize failure fall back to two-step, or at least be handled after the
> Sync check?

No, first, the frame might be a sync frame, if fall back to two-step, that means
one-step offload will not take effect, so the timestamp of the sync frame is not
correct. If the PTP clocks are already synced, a frame with an incorrect timestamp
may cause the clocks desynchronization. In this case, drop the frame is better
than sending it.

Second, the purpose of skb_linearize_cow() is to ensure that the required
data is in the linear buffer. If a check is performed first, it defeats the purpose
of using skb_linearize_cow(), because it cannot be guaranteed that the
required data is in the linear buffer at this time.

> 
> None of the new drop paths are accounted anywhere.  dsa_user_xmit() has
> already counted the frame as transmitted via dev_sw_netstats_tx_add() before
> the tagger frees it, so from userspace the frames just vanish:
> 
>   netc_port_prepare_onestep_sync() -> NETC_PTP_FLAG_DROP -> kfree_skb()
> in
>     netc_xmit()                               (linearize/parse/offset
> checks)
>   netc_port_onestep_sync_enqueue()            (queue full, port inactive)
>   netc_port_onestep_work()                    (clone failure, no
> ts_req_id,
>                                                PHC read failure
> purges all)
>   netc_port_purge_onestep_queue()             (hwtstamp
> reconfiguration)
> 
> netc has no .get_stats64, so dsa_user_get_stats64() falls back to
> dev_get_tstats64(), which folds dev->stats into rtnl_link_stats64 - could
> these paths bump dev->stats.tx_dropped?
> 
> The changelog only says "Frames that are not one-step Sync still fall back
> to two-step timestamping", which does not tell a reader that enabling
> one-step turns a degraded timestamp into a frame that is never transmitted.
> 
> [ ... ]
> 
> > @@ -449,6 +787,21 @@ void netc_port_txtstamp_handler(struct dsa_switch
> *ds, int port,
> >  		dev_dbg_ratelimited(np->switch_priv->dev,
> >  				    "Port %d ts_req_id %u which seems lost\n",
> >  				    port, ts_req_id);
> > +
> > +		return;
> > +	}
> > +
> > +	if (NETC_SKB_CB(skb_match)->ptp_flag == NETC_PTP_FLAG_ONESTEP) {
> > +		spin_lock_bh(&np->onestep_lock);
> > +		if (likely(np->onestep_state == NETC_ONESTEP_IN_FLIGHT &&
> > +			   np->onestep_ts_req_id == ts_req_id) ||
> > +		    np->onestep_state == NETC_ONESTEP_IDLE) {
> > +			np->onestep_state = NETC_ONESTEP_SCHEDULED;
> > +			schedule_work(&np->onestep_work);
> > +		}
> > +		spin_unlock_bh(&np->onestep_lock);
> > +		consume_skb(skb_match);
> > +
> >  		return;
> >  	}
> 
> [Severity: Medium]
> Can the ts_req_id comparison here match a different frame?  The clone is
> unlinked from np->tstamp_queue under tstamp_lock earlier in this function,
> which immediately makes the 4-bit id reusable, and onestep_lock is only
> taken afterwards:
> 
> CPU0 (work, frame A response overdue past the window)
>   netc_port_onestep_sync_enqueue()
>      netc_onestep_timeout() -> true, schedule_work()
>   netc_port_onestep_work()
>      spin_lock_bh(&np->onestep_lock);
> 
> CPU1 (A's response arrives)
>   netc_port_txtstamp_handler()
>      __skb_unlink(clone_A, &np->tstamp_queue);   /* id X now free */
>      spin_unlock_bh(&np->tstamp_lock);
>      spin_lock_bh(&np->onestep_lock);            /* blocks */
> 
> CPU0 continues
>      netc_get_ts_req_id() -> X again for frame B
>      np->onestep_ts_req_id = X;
>      np->onestep_state = NETC_ONESTEP_IN_FLIGHT;
>      spin_unlock_bh(&np->onestep_lock);
>      onestep_sync_xmit(B)
> 
> CPU1 then sees IN_FLIGHT with onestep_ts_req_id == X and treats A's
> response as B's completion, sets SCHEDULED and re-arms the work, which can
> transmit frame C and reprogram PM_SINGLE_STEP while B is still pending in
> hardware.  Workqueue non-reentrancy serializes the worker, not frame
> completion.
> 
> Would retiring the id and moving the one-step state under a single lock
> order (onestep_lock then tstamp_lock), or matching on a generation counter
> instead of the reusable 4-bit id, close this?
> 

This is a hardware limitation, the response frame only contains ts_req_id
and timestamp. the driver can only use the ts_req_id to match the skb from
the queue. The incorrect matches can occur, but only in the event of a
timeout. In the event of a timeout, PTP synchronization has already been
affected, so incorrect matches will not cause any worse consequences.
Once the link recovers, the timeout disappears, and subsequent PTP
synchronization resumes. Therefore, it's a trade-off.