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