RE: [PATCH v4 net-next 7/8] 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:

quoted

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?

quoted

diff --git a/drivers/net/dsa/netc/netc_main.c

b/drivers/net/dsa/netc/netc_main.c

quoted

index 336f5a4633e9..03200fc63a09 100644

--- a/drivers/net/dsa/netc/netc_main.c
+++ b/drivers/net/dsa/netc/netc_main.c

[ ... ]

quoted

@@ -949,6 +993,21 @@ static void netc_destroy_all_lists(struct

netc_switch *priv)

quoted

 	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.

[ ... ]

quoted

diff --git a/drivers/net/dsa/netc/netc_ptp.c

b/drivers/net/dsa/netc/netc_ptp.c

quoted

new file mode 100644
index 000000000000..2e743443ca03

--- /dev/null
+++ b/drivers/net/dsa/netc/netc_ptp.c

@@ -0,0 +1,492 @@

[ ... ]

quoted

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

quoted

+
+		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);
+	}

[ ... ]

quoted

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

quoted

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