[PATCH net-next v4 0/4] sfc: support unicast PTP

[PATCH net-next v4 0/4] sfc: support unicast PTP

[Íñigo Huguet]

Unicast PTP was not working with sfc NICs.

The reason was that these NICs don't timestamp all incoming packets,
but instead they only timestamp packets of the queues that are selected
for that. Currently, only one RX queue is configured for timestamp: the
RX queue of the PTP channel. The packets that are put in the PTP RX
queue are selected according to firmware filters configured from the
driver.

Multicast PTP was already working because the needed filters are known
in advance, so they're inserted when PTP is enabled. This patches
add the ability to dynamically add filters for unicast addresses,
extracted from the TX PTP-event packets.

Since we don't know in advance how many filters we'll need, some info
about the filters need to be saved. This will allow to check if a filter
already exists or if a filter is too old and should be removed.

Note that the previous point is unnecessary for multicast filters, but
I've opted to change how they're handled to match the new unicast's
filters to avoid having duplicate insert/remove_filters functions,
once for each type of filter.

Tested: With ptp4l, all combinations of IPv4/IPv6, master/slave and
unicast/multicast

Reported-by: Yalin Li <yalli@redhat.com>
Signed-off-by: Íñigo Huguet <ihuguet@redhat.com>

v2:
 - fixed missing IS_ERR
 - added doc of missing fields in efx_ptp_rxfilter
v3:
 - dropped pointless static inline in source file
 - removed the now unused PTP_RXFILTERS_LEN
 - follow reverse xmas tree convention in xmit_skb_mc
 - pass expiry as argument to the insert_filter functions and keep returning an
   integer error code from them, and not pointers, as suggested by Martin
 - moved the unicast filters expiration check to the end of the worker function
   to avoid increasing TX latency, as suggested by Martin
 - added check to avoid inserting unicast filters when doing multicast PTP
v4:
 - fixed filter leak, catched by Edward

Íñigo Huguet (4):
  sfc: store PTP filters in a list
  sfc: allow insertion of filters for unicast PTP
  sfc: support unicast PTP
  sfc: remove expired unicast PTP filters

 drivers/net/ethernet/sfc/ptp.c | 272 +++++++++++++++++++++++++++------
 1 file changed, 223 insertions(+), 49 deletions(-)

--
2.34.3

[PATCH net-next v4 1/4] sfc: store PTP filters in a list

[Íñigo Huguet]

Instead of using a fixed sized array for the PTP filters, use a list.

This is not actually necessary at this point because the filters for
multicast PTP are a fixed number, but this is a preparation for the
following patches adding support for unicast PTP.

To avoid confusion with the new struct type efx_ptp_rxfilter, change the
name of some local variables from rxfilter to spec, given they're of the
type efx_filter_spec.

Reported-by: Yalin Li <yalli@redhat.com>
Signed-off-by: Íñigo Huguet <ihuguet@redhat.com>
---
 drivers/net/ethernet/sfc/ptp.c | 81 ++++++++++++++++++++++------------
 1 file changed, 52 insertions(+), 29 deletions(-)

diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
index 9f07e1ba7780..8418a25ed724 100644
--- a/drivers/net/ethernet/sfc/ptp.c
+++ b/drivers/net/ethernet/sfc/ptp.c
@@ -33,6 +33,7 @@
 #include <linux/ip.h>
 #include <linux/udp.h>
 #include <linux/time.h>
+#include <linux/errno.h>
 #include <linux/ktime.h>
 #include <linux/module.h>
 #include <linux/pps_kernel.h>
@@ -118,8 +119,6 @@
 
 #define	PTP_MIN_LENGTH		63
 
-#define PTP_RXFILTERS_LEN	5
-
 #define PTP_ADDR_IPV4		0xe0000181	/* 224.0.1.129 */
 #define PTP_ADDR_IPV6		{0xff, 0x0e, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, \
 				0, 0x01, 0x81}	/* ff0e::181 */
@@ -213,6 +212,16 @@ struct efx_ptp_timeset {
 	u32 window;	/* Derived: end - start, allowing for wrap */
 };
 
+/**
+ * struct efx_ptp_rxfilter - Filter for PTP packets
+ * @list: Node of the list where the filter is added
+ * @handle: Handle ID for the MCDI filters table
+ */
+struct efx_ptp_rxfilter {
+	struct list_head list;
+	int handle;
+};
+
 /**
  * struct efx_ptp_data - Precision Time Protocol (PTP) state
  * @efx: The NIC context
@@ -229,8 +238,7 @@ struct efx_ptp_timeset {
  * @work: Work task
  * @reset_required: A serious error has occurred and the PTP task needs to be
  *                  reset (disable, enable).
- * @rxfilters: Receive filters when operating
- * @rxfilters_count: Num of installed rxfilters, should be == PTP_RXFILTERS_LEN
+ * @rxfilters_mcast: Receive filters for multicast PTP packets
  * @config: Current timestamp configuration
  * @enabled: PTP operation enabled
  * @mode: Mode in which PTP operating (PTP version)
@@ -299,8 +307,7 @@ struct efx_ptp_data {
 	struct workqueue_struct *workwq;
 	struct work_struct work;
 	bool reset_required;
-	u32 rxfilters[PTP_RXFILTERS_LEN];
-	size_t rxfilters_count;
+	struct list_head rxfilters_mcast;
 	struct hwtstamp_config config;
 	bool enabled;
 	unsigned int mode;
@@ -1292,11 +1299,13 @@ static inline void efx_ptp_process_rx(struct efx_nic *efx, struct sk_buff *skb)
 static void efx_ptp_remove_multicast_filters(struct efx_nic *efx)
 {
 	struct efx_ptp_data *ptp = efx->ptp_data;
+	struct efx_ptp_rxfilter *rxfilter, *tmp;
 
-	while (ptp->rxfilters_count) {
-		ptp->rxfilters_count--;
+	list_for_each_entry_safe(rxfilter, tmp, &ptp->rxfilters_mcast, list) {
 		efx_filter_remove_id_safe(efx, EFX_FILTER_PRI_REQUIRED,
-					  ptp->rxfilters[ptp->rxfilters_count]);
+					  rxfilter->handle);
+		list_del(&rxfilter->list);
+		kfree(rxfilter);
 	}
 }
 
@@ -1311,48 +1320,60 @@ static void efx_ptp_init_filter(struct efx_nic *efx,
 }
 
 static int efx_ptp_insert_filter(struct efx_nic *efx,
-				 struct efx_filter_spec *rxfilter)
+				 struct efx_filter_spec *spec)
 {
 	struct efx_ptp_data *ptp = efx->ptp_data;
+	struct efx_ptp_rxfilter *rxfilter;
+	int rc;
+
+	rxfilter = kzalloc(sizeof(*rxfilter), GFP_KERNEL);
+	if (!rxfilter)
+		return -ENOMEM;
 
-	int rc = efx_filter_insert_filter(efx, rxfilter, true);
+	rc = efx_filter_insert_filter(efx, spec, true);
 	if (rc < 0)
-		return rc;
-	ptp->rxfilters[ptp->rxfilters_count] = rc;
-	ptp->rxfilters_count++;
+		goto fail;
+
+	rxfilter->handle = rc;
+	list_add(&rxfilter->list, &ptp->rxfilters_mcast);
+
 	return 0;
+
+fail:
+	kfree(rxfilter);
+	return rc;
 }
 
 static int efx_ptp_insert_ipv4_filter(struct efx_nic *efx, u16 port)
 {
-	struct efx_filter_spec rxfilter;
+	struct efx_filter_spec spec;
 
-	efx_ptp_init_filter(efx, &rxfilter);
-	efx_filter_set_ipv4_local(&rxfilter, IPPROTO_UDP, htonl(PTP_ADDR_IPV4),
+	efx_ptp_init_filter(efx, &spec);
+	efx_filter_set_ipv4_local(&spec, IPPROTO_UDP, htonl(PTP_ADDR_IPV4),
 				  htons(port));
-	return efx_ptp_insert_filter(efx, &rxfilter);
+	return efx_ptp_insert_filter(efx, &spec);
 }
 
 static int efx_ptp_insert_ipv6_filter(struct efx_nic *efx, u16 port)
 {
 	const struct in6_addr addr = {{PTP_ADDR_IPV6}};
-	struct efx_filter_spec rxfilter;
+	struct efx_filter_spec spec;
 
-	efx_ptp_init_filter(efx, &rxfilter);
-	efx_filter_set_ipv6_local(&rxfilter, IPPROTO_UDP, &addr, htons(port));
-	return efx_ptp_insert_filter(efx, &rxfilter);
+	efx_ptp_init_filter(efx, &spec);
+	efx_filter_set_ipv6_local(&spec, IPPROTO_UDP, &addr, htons(port));
+	return efx_ptp_insert_filter(efx, &spec);
 }
 
 static int efx_ptp_insert_eth_filter(struct efx_nic *efx)
 {
 	const u8 addr[ETH_ALEN] = PTP_ADDR_ETHER;
-	struct efx_filter_spec rxfilter;
+	struct efx_filter_spec spec;
 
-	efx_ptp_init_filter(efx, &rxfilter);
-	efx_filter_set_eth_local(&rxfilter, EFX_FILTER_VID_UNSPEC, addr);
-	rxfilter.match_flags |= EFX_FILTER_MATCH_ETHER_TYPE;
-	rxfilter.ether_type = htons(ETH_P_1588);
-	return efx_ptp_insert_filter(efx, &rxfilter);
+	efx_ptp_init_filter(efx, &spec);
+	efx_filter_set_eth_local(&spec, EFX_FILTER_VID_UNSPEC, addr);
+	spec.match_flags |= EFX_FILTER_MATCH_ETHER_TYPE;
+	spec.ether_type = htons(ETH_P_1588);
+	return efx_ptp_insert_filter(efx, &spec);
 }
 
 static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
@@ -1360,7 +1381,7 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
 	struct efx_ptp_data *ptp = efx->ptp_data;
 	int rc;
 
-	if (!ptp->channel || ptp->rxfilters_count)
+	if (!ptp->channel || !list_empty(&ptp->rxfilters_mcast))
 		return 0;
 
 	/* Must filter on both event and general ports to ensure
@@ -1566,6 +1587,8 @@ int efx_ptp_probe(struct efx_nic *efx, struct efx_channel *channel)
 	for (pos = 0; pos < MAX_RECEIVE_EVENTS; pos++)
 		list_add(&ptp->rx_evts[pos].link, &ptp->evt_free_list);
 
+	INIT_LIST_HEAD(&ptp->rxfilters_mcast);
+
 	/* Get the NIC PTP attributes and set up time conversions */
 	rc = efx_ptp_get_attributes(efx);
 	if (rc < 0)
-- 
2.34.3

[PATCH net-next v4 2/4] sfc: allow insertion of filters for unicast PTP

[Íñigo Huguet]

Add a second list for unicast filters and generalize the
efx_ptp_insert/remove_filters functions to allow acting in any of the 2
lists.

No filters for unicast are inserted yet. That will be done in the next
patch.

The reason to use 2 different lists instead of a single one is that, in
next patches, we will want to check if unicast filters are already added
and if they're expired. We don't need that for multicast filters.

Reported-by: Yalin Li <yalli@redhat.com>
Signed-off-by: Íñigo Huguet <ihuguet@redhat.com>
---
 drivers/net/ethernet/sfc/ptp.c | 58 ++++++++++++++++++++--------------
 1 file changed, 35 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
index 8418a25ed724..478dd5c06b22 100644
--- a/drivers/net/ethernet/sfc/ptp.c
+++ b/drivers/net/ethernet/sfc/ptp.c
@@ -239,6 +239,7 @@ struct efx_ptp_rxfilter {
  * @reset_required: A serious error has occurred and the PTP task needs to be
  *                  reset (disable, enable).
  * @rxfilters_mcast: Receive filters for multicast PTP packets
+ * @rxfilters_ucast: Receive filters for unicast PTP packets
  * @config: Current timestamp configuration
  * @enabled: PTP operation enabled
  * @mode: Mode in which PTP operating (PTP version)
@@ -308,6 +309,7 @@ struct efx_ptp_data {
 	struct work_struct work;
 	bool reset_required;
 	struct list_head rxfilters_mcast;
+	struct list_head rxfilters_ucast;
 	struct hwtstamp_config config;
 	bool enabled;
 	unsigned int mode;
@@ -1296,12 +1298,12 @@ static inline void efx_ptp_process_rx(struct efx_nic *efx, struct sk_buff *skb)
 	local_bh_enable();
 }
 
-static void efx_ptp_remove_multicast_filters(struct efx_nic *efx)
+static void efx_ptp_remove_filters(struct efx_nic *efx,
+				   struct list_head *ptp_list)
 {
-	struct efx_ptp_data *ptp = efx->ptp_data;
 	struct efx_ptp_rxfilter *rxfilter, *tmp;
 
-	list_for_each_entry_safe(rxfilter, tmp, &ptp->rxfilters_mcast, list) {
+	list_for_each_entry_safe(rxfilter, tmp, ptp_list, list) {
 		efx_filter_remove_id_safe(efx, EFX_FILTER_PRI_REQUIRED,
 					  rxfilter->handle);
 		list_del(&rxfilter->list);
@@ -1320,9 +1322,9 @@ static void efx_ptp_init_filter(struct efx_nic *efx,
 }
 
 static int efx_ptp_insert_filter(struct efx_nic *efx,
+				 struct list_head *ptp_list,
 				 struct efx_filter_spec *spec)
 {
-	struct efx_ptp_data *ptp = efx->ptp_data;
 	struct efx_ptp_rxfilter *rxfilter;
 	int rc;
 
@@ -1335,7 +1337,7 @@ static int efx_ptp_insert_filter(struct efx_nic *efx,
 		goto fail;
 
 	rxfilter->handle = rc;
-	list_add(&rxfilter->list, &ptp->rxfilters_mcast);
+	list_add(&rxfilter->list, ptp_list);
 
 	return 0;
 
@@ -1344,27 +1346,29 @@ static int efx_ptp_insert_filter(struct efx_nic *efx,
 	return rc;
 }
 
-static int efx_ptp_insert_ipv4_filter(struct efx_nic *efx, u16 port)
+static int efx_ptp_insert_ipv4_filter(struct efx_nic *efx,
+				      struct list_head *ptp_list,
+				      __be32 addr, u16 port)
 {
 	struct efx_filter_spec spec;
 
 	efx_ptp_init_filter(efx, &spec);
-	efx_filter_set_ipv4_local(&spec, IPPROTO_UDP, htonl(PTP_ADDR_IPV4),
-				  htons(port));
-	return efx_ptp_insert_filter(efx, &spec);
+	efx_filter_set_ipv4_local(&spec, IPPROTO_UDP, addr, htons(port));
+	return efx_ptp_insert_filter(efx, ptp_list, &spec);
 }
 
-static int efx_ptp_insert_ipv6_filter(struct efx_nic *efx, u16 port)
+static int efx_ptp_insert_ipv6_filter(struct efx_nic *efx,
+				      struct list_head *ptp_list,
+				      struct in6_addr *addr, u16 port)
 {
-	const struct in6_addr addr = {{PTP_ADDR_IPV6}};
 	struct efx_filter_spec spec;
 
 	efx_ptp_init_filter(efx, &spec);
-	efx_filter_set_ipv6_local(&spec, IPPROTO_UDP, &addr, htons(port));
-	return efx_ptp_insert_filter(efx, &spec);
+	efx_filter_set_ipv6_local(&spec, IPPROTO_UDP, addr, htons(port));
+	return efx_ptp_insert_filter(efx, ptp_list, &spec);
 }
 
-static int efx_ptp_insert_eth_filter(struct efx_nic *efx)
+static int efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx)
 {
 	const u8 addr[ETH_ALEN] = PTP_ADDR_ETHER;
 	struct efx_filter_spec spec;
@@ -1373,7 +1377,7 @@ static int efx_ptp_insert_eth_filter(struct efx_nic *efx)
 	efx_filter_set_eth_local(&spec, EFX_FILTER_VID_UNSPEC, addr);
 	spec.match_flags |= EFX_FILTER_MATCH_ETHER_TYPE;
 	spec.ether_type = htons(ETH_P_1588);
-	return efx_ptp_insert_filter(efx, &spec);
+	return efx_ptp_insert_filter(efx, &efx->ptp_data->rxfilters_mcast, &spec);
 }
 
 static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
@@ -1387,11 +1391,13 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
 	/* Must filter on both event and general ports to ensure
 	 * that there is no packet re-ordering.
 	 */
-	rc = efx_ptp_insert_ipv4_filter(efx, PTP_EVENT_PORT);
+	rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_mcast,
+					htonl(PTP_ADDR_IPV4), PTP_EVENT_PORT);
 	if (rc < 0)
 		goto fail;
 
-	rc = efx_ptp_insert_ipv4_filter(efx, PTP_GENERAL_PORT);
+	rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_mcast,
+					htonl(PTP_ADDR_IPV4), PTP_GENERAL_PORT);
 	if (rc < 0)
 		goto fail;
 
@@ -1399,15 +1405,19 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
 	 * PTP over IPv6 and Ethernet
 	 */
 	if (efx_ptp_use_mac_tx_timestamps(efx)) {
-		rc = efx_ptp_insert_ipv6_filter(efx, PTP_EVENT_PORT);
+		struct in6_addr ipv6_addr = {{PTP_ADDR_IPV6}};
+
+		rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_mcast,
+						&ipv6_addr, PTP_EVENT_PORT);
 		if (rc < 0)
 			goto fail;
 
-		rc = efx_ptp_insert_ipv6_filter(efx, PTP_GENERAL_PORT);
+		rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_mcast,
+						&ipv6_addr, PTP_GENERAL_PORT);
 		if (rc < 0)
 			goto fail;
 
-		rc = efx_ptp_insert_eth_filter(efx);
+		rc = efx_ptp_insert_eth_multicast_filter(efx);
 		if (rc < 0)
 			goto fail;
 	}
@@ -1415,7 +1425,7 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
 	return 0;
 
 fail:
-	efx_ptp_remove_multicast_filters(efx);
+	efx_ptp_remove_filters(efx, &ptp->rxfilters_mcast);
 	return rc;
 }
 
@@ -1440,7 +1450,7 @@ static int efx_ptp_start(struct efx_nic *efx)
 	return 0;
 
 fail:
-	efx_ptp_remove_multicast_filters(efx);
+	efx_ptp_remove_filters(efx, &ptp->rxfilters_mcast);
 	return rc;
 }
 
@@ -1456,7 +1466,8 @@ static int efx_ptp_stop(struct efx_nic *efx)
 
 	rc = efx_ptp_disable(efx);
 
-	efx_ptp_remove_multicast_filters(efx);
+	efx_ptp_remove_filters(efx, &ptp->rxfilters_mcast);
+	efx_ptp_remove_filters(efx, &ptp->rxfilters_ucast);
 
 	/* Make sure RX packets are really delivered */
 	efx_ptp_deliver_rx_queue(&efx->ptp_data->rxq);
@@ -1588,6 +1599,7 @@ int efx_ptp_probe(struct efx_nic *efx, struct efx_channel *channel)
 		list_add(&ptp->rx_evts[pos].link, &ptp->evt_free_list);
 
 	INIT_LIST_HEAD(&ptp->rxfilters_mcast);
+	INIT_LIST_HEAD(&ptp->rxfilters_ucast);
 
 	/* Get the NIC PTP attributes and set up time conversions */
 	rc = efx_ptp_get_attributes(efx);
-- 
2.34.3

[PATCH net-next v4 3/4] sfc: support unicast PTP

[Íñigo Huguet]

When sending a PTP event packet, add the correct filters that will make
that future incoming unicast PTP event packets will be timestamped.
The unicast address for the filter is gotten from the outgoing skb
before sending it.

Until now they were not timestamped because only filters that match with
the PTP multicast addressed were being configured into the NIC for the
PTP special channel. Packets received through different channels are not
timestamped, getting "received SYNC without timestamp" error in ptp4l.

Note that the inserted filters are never removed unless the NIC is stopped
or reconfigured, so efx_ptp_stop is called. Removal of old filters will
be handled by the next patch.

Additionally, cleanup a bit efx_ptp_xmit_skb_mc to use the reverse xmas
tree convention and remove an unnecessary assignment to rc variable in
void function.

Reported-by: Yalin Li <yalli@redhat.com>
Signed-off-by: Íñigo Huguet <ihuguet@redhat.com>
---
 drivers/net/ethernet/sfc/ptp.c | 108 ++++++++++++++++++++++++++++++++-
 1 file changed, 105 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
index 478dd5c06b22..16686aa5bfb4 100644
--- a/drivers/net/ethernet/sfc/ptp.c
+++ b/drivers/net/ethernet/sfc/ptp.c
@@ -215,10 +215,16 @@ struct efx_ptp_timeset {
 /**
  * struct efx_ptp_rxfilter - Filter for PTP packets
  * @list: Node of the list where the filter is added
+ * @ether_type: Network protocol of the filter (ETHER_P_IP / ETHER_P_IPV6)
+ * @loc_port: UDP port of the filter (PTP_EVENT_PORT / PTP_GENERAL_PORT)
+ * @loc_host: IPv4/v6 address of the filter
  * @handle: Handle ID for the MCDI filters table
  */
 struct efx_ptp_rxfilter {
 	struct list_head list;
+	__be16 ether_type;
+	__be16 loc_port;
+	__be32 loc_host[4];
 	int handle;
 };

@@ -367,6 +373,8 @@ static int efx_phc_settime(struct ptp_clock_info *ptp,
 			   const struct timespec64 *e_ts);
 static int efx_phc_enable(struct ptp_clock_info *ptp,
 			  struct ptp_clock_request *request, int on);
+static int efx_ptp_insert_unicast_filter(struct efx_nic *efx,
+					 struct sk_buff *skb);

 bool efx_ptp_use_mac_tx_timestamps(struct efx_nic *efx)
 {
@@ -1112,6 +1120,8 @@ static void efx_ptp_xmit_skb_queue(struct efx_nic *efx, struct sk_buff *skb)

 	tx_queue = efx_channel_get_tx_queue(ptp_data->channel, type);
 	if (tx_queue && tx_queue->timestamping) {
+		skb_get(skb);
+
 		/* This code invokes normal driver TX code which is always
 		 * protected from softirqs when called from generic TX code,
 		 * which in turn disables preemption. Look at __dev_queue_xmit
@@ -1135,6 +1145,13 @@ static void efx_ptp_xmit_skb_queue(struct efx_nic *efx, struct sk_buff *skb)
 		local_bh_disable();
 		efx_enqueue_skb(tx_queue, skb);
 		local_bh_enable();
+
+		/* We need to add the filters after enqueuing the packet.
+		 * Otherwise, there's high latency in sending back the
+		 * timestamp, causing ptp4l timeouts
+		 */
+		efx_ptp_insert_unicast_filter(efx, skb);
+		dev_consume_skb_any(skb);
 	} else {
 		WARN_ONCE(1, "PTP channel has no timestamped tx queue\n");
 		dev_kfree_skb_any(skb);
@@ -1144,11 +1161,11 @@ static void efx_ptp_xmit_skb_queue(struct efx_nic *efx, struct sk_buff *skb)
 /* Transmit a PTP packet, via the MCDI interface, to the wire. */
 static void efx_ptp_xmit_skb_mc(struct efx_nic *efx, struct sk_buff *skb)
 {
+	MCDI_DECLARE_BUF(txtime, MC_CMD_PTP_OUT_TRANSMIT_LEN);
 	struct efx_ptp_data *ptp_data = efx->ptp_data;
 	struct skb_shared_hwtstamps timestamps;
-	int rc = -EIO;
-	MCDI_DECLARE_BUF(txtime, MC_CMD_PTP_OUT_TRANSMIT_LEN);
 	size_t len;
+	int rc;

 	MCDI_SET_DWORD(ptp_data->txbuf, PTP_IN_OP, MC_CMD_PTP_OP_TRANSMIT);
 	MCDI_SET_DWORD(ptp_data->txbuf, PTP_IN_PERIPH_ID, 0);
@@ -1182,7 +1199,10 @@ static void efx_ptp_xmit_skb_mc(struct efx_nic *efx, struct sk_buff *skb)

 	skb_tstamp_tx(skb, &timestamps);

-	rc = 0;
+	/* Add the filters after sending back the timestamp to avoid delaying it
+	 * or ptp4l may timeout.
+	 */
+	efx_ptp_insert_unicast_filter(efx, skb);

 fail:
 	dev_kfree_skb_any(skb);
@@ -1298,6 +1318,21 @@ static inline void efx_ptp_process_rx(struct efx_nic *efx, struct sk_buff *skb)
 	local_bh_enable();
 }

+static bool efx_ptp_filter_exists(struct list_head *ptp_list,
+				  struct efx_filter_spec *spec)
+{
+	struct efx_ptp_rxfilter *rxfilter;
+
+	list_for_each_entry(rxfilter, ptp_list, list) {
+		if (rxfilter->ether_type == spec->ether_type &&
+		    rxfilter->loc_port == spec->loc_port &&
+		    !memcmp(rxfilter->loc_host, spec->loc_host, sizeof(spec->loc_host)))
+			return true;
+	}
+
+	return false;
+}
+
 static void efx_ptp_remove_filters(struct efx_nic *efx,
 				   struct list_head *ptp_list)
 {
@@ -1328,6 +1363,9 @@ static int efx_ptp_insert_filter(struct efx_nic *efx,
 	struct efx_ptp_rxfilter *rxfilter;
 	int rc;

+	if (efx_ptp_filter_exists(ptp_list, spec))
+		return 0;
+
 	rxfilter = kzalloc(sizeof(*rxfilter), GFP_KERNEL);
 	if (!rxfilter)
 		return -ENOMEM;
@@ -1337,6 +1375,9 @@ static int efx_ptp_insert_filter(struct efx_nic *efx,
 		goto fail;

 	rxfilter->handle = rc;
+	rxfilter->ether_type = spec->ether_type;
+	rxfilter->loc_port = spec->loc_port;
+	memcpy(rxfilter->loc_host, spec->loc_host, sizeof(spec->loc_host));
 	list_add(&rxfilter->list, ptp_list);

 	return 0;
@@ -1429,6 +1470,67 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
 	return rc;
 }

+static bool efx_ptp_valid_unicast_event_pkt(struct sk_buff *skb)
+{
+	if (skb->protocol == htons(ETH_P_IP)) {
+		return ip_hdr(skb)->daddr != htonl(PTP_ADDR_IPV4) &&
+			ip_hdr(skb)->protocol == IPPROTO_UDP &&
+			udp_hdr(skb)->source == htons(PTP_EVENT_PORT);
+	} else if (skb->protocol == htons(ETH_P_IPV6)) {
+		struct in6_addr mcast_addr = {{PTP_ADDR_IPV6}};
+
+		return !ipv6_addr_equal(&ipv6_hdr(skb)->daddr, &mcast_addr) &&
+			ipv6_hdr(skb)->nexthdr == IPPROTO_UDP &&
+			udp_hdr(skb)->source == htons(PTP_EVENT_PORT);
+	}
+	return false;
+}
+
+static int efx_ptp_insert_unicast_filter(struct efx_nic *efx,
+					 struct sk_buff *skb)
+{
+	struct efx_ptp_data *ptp = efx->ptp_data;
+	int rc;
+
+	if (!efx_ptp_valid_unicast_event_pkt(skb))
+		return -EINVAL;
+
+	if (skb->protocol == htons(ETH_P_IP)) {
+		__be32 addr = ip_hdr(skb)->saddr;
+
+		rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast,
+						addr, PTP_EVENT_PORT);
+		if (rc < 0)
+			goto fail;
+
+		rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast,
+						addr, PTP_GENERAL_PORT);
+		if (rc < 0)
+			goto fail;
+	} else if (efx_ptp_use_mac_tx_timestamps(efx)) {
+		/* IPv6 PTP only supported by devices with MAC hw timestamp */
+		struct in6_addr *addr = &ipv6_hdr(skb)->saddr;
+
+		rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
+						addr, PTP_EVENT_PORT);
+		if (rc < 0)
+			goto fail;
+
+		rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
+						addr, PTP_GENERAL_PORT);
+		if (rc < 0)
+			goto fail;
+	} else {
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+
+fail:
+	efx_ptp_remove_filters(efx, &ptp->rxfilters_ucast);
+	return rc;
+}
+
 static int efx_ptp_start(struct efx_nic *efx)
 {
 	struct efx_ptp_data *ptp = efx->ptp_data;
--
2.34.3

[PATCH net-next v4 4/4] sfc: remove expired unicast PTP filters

[Íñigo Huguet]

Filters inserted to support unicast PTP mode might become unused after
some time, so we need to remove them to avoid accumulating many of them.

Actually, it would be a very unusual situation that many different
addresses are used, normally only a small set of predefined
addresses are tried. Anyway, some cleanup is necessary because
maintaining old filters forever makes very little sense.

Reported-by: Yalin Li <yalli@redhat.com>
Signed-off-by: Íñigo Huguet <ihuguet@redhat.com>
---
 drivers/net/ethernet/sfc/ptp.c | 83 ++++++++++++++++++++++++----------
 1 file changed, 60 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
index 16686aa5bfb4..1447683dce31 100644
--- a/drivers/net/ethernet/sfc/ptp.c
+++ b/drivers/net/ethernet/sfc/ptp.c
@@ -75,6 +75,9 @@
 /* How long an unmatched event or packet can be held */
 #define PKT_EVENT_LIFETIME_MS		10
 
+/* How long unused unicast filters can be held */
+#define UCAST_FILTER_EXPIRY_JIFFIES	msecs_to_jiffies(30000)
+
 /* Offsets into PTP packet for identification.  These offsets are from the
  * start of the IP header, not the MAC header.  Note that neither PTP V1 nor
  * PTP V2 permit the use of IPV4 options.
@@ -218,6 +221,7 @@ struct efx_ptp_timeset {
  * @ether_type: Network protocol of the filter (ETHER_P_IP / ETHER_P_IPV6)
  * @loc_port: UDP port of the filter (PTP_EVENT_PORT / PTP_GENERAL_PORT)
  * @loc_host: IPv4/v6 address of the filter
+ * @expiry: time when the filter expires, in jiffies
  * @handle: Handle ID for the MCDI filters table
  */
 struct efx_ptp_rxfilter {
@@ -225,6 +229,7 @@ struct efx_ptp_rxfilter {
 	__be16 ether_type;
 	__be16 loc_port;
 	__be32 loc_host[4];
+	unsigned long expiry;
 	int handle;
 };
 
@@ -1318,8 +1323,8 @@ static inline void efx_ptp_process_rx(struct efx_nic *efx, struct sk_buff *skb)
 	local_bh_enable();
 }
 
-static bool efx_ptp_filter_exists(struct list_head *ptp_list,
-				  struct efx_filter_spec *spec)
+static struct efx_ptp_rxfilter *
+efx_ptp_find_filter(struct list_head *ptp_list, struct efx_filter_spec *spec)
 {
 	struct efx_ptp_rxfilter *rxfilter;
 
@@ -1327,10 +1332,19 @@ static bool efx_ptp_filter_exists(struct list_head *ptp_list,
 		if (rxfilter->ether_type == spec->ether_type &&
 		    rxfilter->loc_port == spec->loc_port &&
 		    !memcmp(rxfilter->loc_host, spec->loc_host, sizeof(spec->loc_host)))
-			return true;
+			return rxfilter;
 	}
 
-	return false;
+	return NULL;
+}
+
+static void efx_ptp_remove_one_filter(struct efx_nic *efx,
+				      struct efx_ptp_rxfilter *rxfilter)
+{
+	efx_filter_remove_id_safe(efx, EFX_FILTER_PRI_REQUIRED,
+				  rxfilter->handle);
+	list_del(&rxfilter->list);
+	kfree(rxfilter);
 }
 
 static void efx_ptp_remove_filters(struct efx_nic *efx,
@@ -1339,10 +1353,7 @@ static void efx_ptp_remove_filters(struct efx_nic *efx,
 	struct efx_ptp_rxfilter *rxfilter, *tmp;
 
 	list_for_each_entry_safe(rxfilter, tmp, ptp_list, list) {
-		efx_filter_remove_id_safe(efx, EFX_FILTER_PRI_REQUIRED,
-					  rxfilter->handle);
-		list_del(&rxfilter->list);
-		kfree(rxfilter);
+		efx_ptp_remove_one_filter(efx, rxfilter);
 	}
 }
 
@@ -1358,13 +1369,17 @@ static void efx_ptp_init_filter(struct efx_nic *efx,
 
 static int efx_ptp_insert_filter(struct efx_nic *efx,
 				 struct list_head *ptp_list,
-				 struct efx_filter_spec *spec)
+				 struct efx_filter_spec *spec,
+				 unsigned long expiry)
 {
 	struct efx_ptp_rxfilter *rxfilter;
 	int rc;
 
-	if (efx_ptp_filter_exists(ptp_list, spec))
+	rxfilter = efx_ptp_find_filter(ptp_list, spec);
+	if (rxfilter) {
+		rxfilter->expiry = expiry;
 		return 0;
+	}
 
 	rxfilter = kzalloc(sizeof(*rxfilter), GFP_KERNEL);
 	if (!rxfilter)
@@ -1378,6 +1393,7 @@ static int efx_ptp_insert_filter(struct efx_nic *efx,
 	rxfilter->ether_type = spec->ether_type;
 	rxfilter->loc_port = spec->loc_port;
 	memcpy(rxfilter->loc_host, spec->loc_host, sizeof(spec->loc_host));
+	rxfilter->expiry = expiry;
 	list_add(&rxfilter->list, ptp_list);
 
 	return 0;
@@ -1389,28 +1405,31 @@ static int efx_ptp_insert_filter(struct efx_nic *efx,
 
 static int efx_ptp_insert_ipv4_filter(struct efx_nic *efx,
 				      struct list_head *ptp_list,
-				      __be32 addr, u16 port)
+				      __be32 addr, u16 port,
+				      unsigned long expiry)
 {
 	struct efx_filter_spec spec;
 
 	efx_ptp_init_filter(efx, &spec);
 	efx_filter_set_ipv4_local(&spec, IPPROTO_UDP, addr, htons(port));
-	return efx_ptp_insert_filter(efx, ptp_list, &spec);
+	return efx_ptp_insert_filter(efx, ptp_list, &spec, expiry);
 }
 
 static int efx_ptp_insert_ipv6_filter(struct efx_nic *efx,
 				      struct list_head *ptp_list,
-				      struct in6_addr *addr, u16 port)
+				      struct in6_addr *addr, u16 port,
+				      unsigned long expiry)
 {
 	struct efx_filter_spec spec;
 
 	efx_ptp_init_filter(efx, &spec);
 	efx_filter_set_ipv6_local(&spec, IPPROTO_UDP, addr, htons(port));
-	return efx_ptp_insert_filter(efx, ptp_list, &spec);
+	return efx_ptp_insert_filter(efx, ptp_list, &spec, expiry);
 }
 
 static int efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx)
 {
+	struct efx_ptp_data *ptp = efx->ptp_data;
 	const u8 addr[ETH_ALEN] = PTP_ADDR_ETHER;
 	struct efx_filter_spec spec;
 
@@ -1418,7 +1437,7 @@ static int efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx)
 	efx_filter_set_eth_local(&spec, EFX_FILTER_VID_UNSPEC, addr);
 	spec.match_flags |= EFX_FILTER_MATCH_ETHER_TYPE;
 	spec.ether_type = htons(ETH_P_1588);
-	return efx_ptp_insert_filter(efx, &efx->ptp_data->rxfilters_mcast, &spec);
+	return efx_ptp_insert_filter(efx, &ptp->rxfilters_mcast, &spec, 0);
 }
 
 static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
@@ -1433,12 +1452,14 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
 	 * that there is no packet re-ordering.
 	 */
 	rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_mcast,
-					htonl(PTP_ADDR_IPV4), PTP_EVENT_PORT);
+					htonl(PTP_ADDR_IPV4), PTP_EVENT_PORT,
+					0);
 	if (rc < 0)
 		goto fail;
 
 	rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_mcast,
-					htonl(PTP_ADDR_IPV4), PTP_GENERAL_PORT);
+					htonl(PTP_ADDR_IPV4), PTP_GENERAL_PORT,
+					0);
 	if (rc < 0)
 		goto fail;
 
@@ -1449,12 +1470,12 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
 		struct in6_addr ipv6_addr = {{PTP_ADDR_IPV6}};
 
 		rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_mcast,
-						&ipv6_addr, PTP_EVENT_PORT);
+						&ipv6_addr, PTP_EVENT_PORT, 0);
 		if (rc < 0)
 			goto fail;
 
 		rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_mcast,
-						&ipv6_addr, PTP_GENERAL_PORT);
+						&ipv6_addr, PTP_GENERAL_PORT, 0);
 		if (rc < 0)
 			goto fail;
 
@@ -1490,21 +1511,24 @@ static int efx_ptp_insert_unicast_filter(struct efx_nic *efx,
 					 struct sk_buff *skb)
 {
 	struct efx_ptp_data *ptp = efx->ptp_data;
+	unsigned long expiry;
 	int rc;
 
 	if (!efx_ptp_valid_unicast_event_pkt(skb))
 		return -EINVAL;
 
+	expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
+
 	if (skb->protocol == htons(ETH_P_IP)) {
 		__be32 addr = ip_hdr(skb)->saddr;
 
 		rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast,
-						addr, PTP_EVENT_PORT);
+						addr, PTP_EVENT_PORT, expiry);
 		if (rc < 0)
 			goto fail;
 
 		rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast,
-						addr, PTP_GENERAL_PORT);
+						addr, PTP_GENERAL_PORT, expiry);
 		if (rc < 0)
 			goto fail;
 	} else if (efx_ptp_use_mac_tx_timestamps(efx)) {
@@ -1512,12 +1536,12 @@ static int efx_ptp_insert_unicast_filter(struct efx_nic *efx,
 		struct in6_addr *addr = &ipv6_hdr(skb)->saddr;
 
 		rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
-						addr, PTP_EVENT_PORT);
+						addr, PTP_EVENT_PORT, expiry);
 		if (rc < 0)
 			goto fail;
 
 		rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
-						addr, PTP_GENERAL_PORT);
+						addr, PTP_GENERAL_PORT, expiry);
 		if (rc < 0)
 			goto fail;
 	} else {
@@ -1531,6 +1555,17 @@ static int efx_ptp_insert_unicast_filter(struct efx_nic *efx,
 	return rc;
 }
 
+static void efx_ptp_drop_expired_unicast_filters(struct efx_nic *efx)
+{
+	struct efx_ptp_data *ptp = efx->ptp_data;
+	struct efx_ptp_rxfilter *rxfilter, *tmp;
+
+	list_for_each_entry_safe(rxfilter, tmp, &ptp->rxfilters_ucast, list) {
+		if (time_is_before_jiffies(rxfilter->expiry))
+			efx_ptp_remove_one_filter(efx, rxfilter);
+	}
+}
+
 static int efx_ptp_start(struct efx_nic *efx)
 {
 	struct efx_ptp_data *ptp = efx->ptp_data;
@@ -1631,6 +1666,8 @@ static void efx_ptp_worker(struct work_struct *work)
 
 	while ((skb = __skb_dequeue(&tempq)))
 		efx_ptp_process_rx(efx, skb);
+
+	efx_ptp_drop_expired_unicast_filters(efx);
 }
 
 static const struct ptp_clock_info efx_phc_clock_info = {
-- 
2.34.3

Re: [PATCH net-next v4 0/4] sfc: support unicast PTP

[Jakub Kicinski]

On Tue, 21 Feb 2023 13:52:13 +0100 Íñigo Huguet wrote:
> Unicast PTP was not working with sfc NICs.
> 
> The reason was that these NICs don't timestamp all incoming packets,
> but instead they only timestamp packets of the queues that are selected
> for that. Currently, only one RX queue is configured for timestamp: the
> RX queue of the PTP channel. The packets that are put in the PTP RX
> queue are selected according to firmware filters configured from the
> driver.
> 
> Multicast PTP was already working because the needed filters are known
> in advance, so they're inserted when PTP is enabled. This patches
> add the ability to dynamically add filters for unicast addresses,
> extracted from the TX PTP-event packets.
> 
> Since we don't know in advance how many filters we'll need, some info
> about the filters need to be saved. This will allow to check if a filter
> already exists or if a filter is too old and should be removed.
> 
> Note that the previous point is unnecessary for multicast filters, but
> I've opted to change how they're handled to match the new unicast's
> filters to avoid having duplicate insert/remove_filters functions,
> once for each type of filter.
> 
> Tested: With ptp4l, all combinations of IPv4/IPv6, master/slave and
> unicast/multicast


# Form letter - net-next is closed

The merge window for v6.3 has begun and therefore net-next is closed
for new drivers, features, code refactoring and optimizations.
We are currently accepting bug fixes only.

Please repost when net-next reopens after Mar 6th.

RFC patches sent for review only are obviously welcome at any time.

Re: [PATCH net-next v4 0/4] sfc: support unicast PTP

[Martin Habets]

On Tue, Feb 21, 2023 at 01:52:13PM +0100, Íñigo Huguet wrote:
> Unicast PTP was not working with sfc NICs.
> 
> The reason was that these NICs don't timestamp all incoming packets,
> but instead they only timestamp packets of the queues that are selected
> for that. Currently, only one RX queue is configured for timestamp: the
> RX queue of the PTP channel. The packets that are put in the PTP RX
> queue are selected according to firmware filters configured from the
> driver.
> 
> Multicast PTP was already working because the needed filters are known
> in advance, so they're inserted when PTP is enabled. This patches
> add the ability to dynamically add filters for unicast addresses,
> extracted from the TX PTP-event packets.
> 
> Since we don't know in advance how many filters we'll need, some info
> about the filters need to be saved. This will allow to check if a filter
> already exists or if a filter is too old and should be removed.
> 
> Note that the previous point is unnecessary for multicast filters, but
> I've opted to change how they're handled to match the new unicast's
> filters to avoid having duplicate insert/remove_filters functions,
> once for each type of filter.
> 
> Tested: With ptp4l, all combinations of IPv4/IPv6, master/slave and
> unicast/multicast
> 
> Reported-by: Yalin Li <yalli@redhat.com>
> Signed-off-by: Íñigo Huguet <ihuguet@redhat.com>

When you repost after net-next is open again please add my
Acked-by: Martin Habets <habetsm.xilinx@gmail.com>

Thanks!

> 
> v2:
>  - fixed missing IS_ERR
>  - added doc of missing fields in efx_ptp_rxfilter
> v3:
>  - dropped pointless static inline in source file
>  - removed the now unused PTP_RXFILTERS_LEN
>  - follow reverse xmas tree convention in xmit_skb_mc
>  - pass expiry as argument to the insert_filter functions and keep returning an
>    integer error code from them, and not pointers, as suggested by Martin
>  - moved the unicast filters expiration check to the end of the worker function
>    to avoid increasing TX latency, as suggested by Martin
>  - added check to avoid inserting unicast filters when doing multicast PTP
> v4:
>  - fixed filter leak, catched by Edward
> 
> Íñigo Huguet (4):
>   sfc: store PTP filters in a list
>   sfc: allow insertion of filters for unicast PTP
>   sfc: support unicast PTP
>   sfc: remove expired unicast PTP filters
> 
>  drivers/net/ethernet/sfc/ptp.c | 272 +++++++++++++++++++++++++++------
>  1 file changed, 223 insertions(+), 49 deletions(-)
> 
> --
> 2.34.3

Re: [PATCH net-next v4 3/4] sfc: support unicast PTP

[Vadim Fedorenko]

On 21/02/2023 12:52, Íñigo Huguet wrote:
> When sending a PTP event packet, add the correct filters that will make
> that future incoming unicast PTP event packets will be timestamped.
> The unicast address for the filter is gotten from the outgoing skb
> before sending it.
> 
> Until now they were not timestamped because only filters that match with
> the PTP multicast addressed were being configured into the NIC for the
> PTP special channel. Packets received through different channels are not
> timestamped, getting "received SYNC without timestamp" error in ptp4l.
> 
> Note that the inserted filters are never removed unless the NIC is stopped
> or reconfigured, so efx_ptp_stop is called. Removal of old filters will
> be handled by the next patch.
> 
> Additionally, cleanup a bit efx_ptp_xmit_skb_mc to use the reverse xmas
> tree convention and remove an unnecessary assignment to rc variable in
> void function.
> 
> Reported-by: Yalin Li <yalli@redhat.com>
> Signed-off-by: Íñigo Huguet <ihuguet@redhat.com>
> ---
>   drivers/net/ethernet/sfc/ptp.c | 108 ++++++++++++++++++++++++++++++++-
>   1 file changed, 105 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
> index 478dd5c06b22..16686aa5bfb4 100644
> --- a/drivers/net/ethernet/sfc/ptp.c
> +++ b/drivers/net/ethernet/sfc/ptp.c
> @@ -215,10 +215,16 @@ struct efx_ptp_timeset {
>   /**
>    * struct efx_ptp_rxfilter - Filter for PTP packets
>    * @list: Node of the list where the filter is added
> + * @ether_type: Network protocol of the filter (ETHER_P_IP / ETHER_P_IPV6)
> + * @loc_port: UDP port of the filter (PTP_EVENT_PORT / PTP_GENERAL_PORT)
> + * @loc_host: IPv4/v6 address of the filter
>    * @handle: Handle ID for the MCDI filters table
>    */
>   struct efx_ptp_rxfilter {
>   	struct list_head list;
> +	__be16 ether_type;
> +	__be16 loc_port;
> +	__be32 loc_host[4];
>   	int handle;
>   };
> 
> @@ -367,6 +373,8 @@ static int efx_phc_settime(struct ptp_clock_info *ptp,
>   			   const struct timespec64 *e_ts);
>   static int efx_phc_enable(struct ptp_clock_info *ptp,
>   			  struct ptp_clock_request *request, int on);
> +static int efx_ptp_insert_unicast_filter(struct efx_nic *efx,
> +					 struct sk_buff *skb);
> 
>   bool efx_ptp_use_mac_tx_timestamps(struct efx_nic *efx)
>   {
> @@ -1112,6 +1120,8 @@ static void efx_ptp_xmit_skb_queue(struct efx_nic *efx, struct sk_buff *skb)
> 
>   	tx_queue = efx_channel_get_tx_queue(ptp_data->channel, type);
>   	if (tx_queue && tx_queue->timestamping) {
> +		skb_get(skb);
> +
>   		/* This code invokes normal driver TX code which is always
>   		 * protected from softirqs when called from generic TX code,
>   		 * which in turn disables preemption. Look at __dev_queue_xmit
> @@ -1135,6 +1145,13 @@ static void efx_ptp_xmit_skb_queue(struct efx_nic *efx, struct sk_buff *skb)
>   		local_bh_disable();
>   		efx_enqueue_skb(tx_queue, skb);
>   		local_bh_enable();
> +
> +		/* We need to add the filters after enqueuing the packet.
> +		 * Otherwise, there's high latency in sending back the
> +		 * timestamp, causing ptp4l timeouts
> +		 */
> +		efx_ptp_insert_unicast_filter(efx, skb);
> +		dev_consume_skb_any(skb);
>   	} else {
>   		WARN_ONCE(1, "PTP channel has no timestamped tx queue\n");
>   		dev_kfree_skb_any(skb);
> @@ -1144,11 +1161,11 @@ static void efx_ptp_xmit_skb_queue(struct efx_nic *efx, struct sk_buff *skb)
>   /* Transmit a PTP packet, via the MCDI interface, to the wire. */
>   static void efx_ptp_xmit_skb_mc(struct efx_nic *efx, struct sk_buff *skb)
>   {
> +	MCDI_DECLARE_BUF(txtime, MC_CMD_PTP_OUT_TRANSMIT_LEN);
>   	struct efx_ptp_data *ptp_data = efx->ptp_data;
>   	struct skb_shared_hwtstamps timestamps;
> -	int rc = -EIO;
> -	MCDI_DECLARE_BUF(txtime, MC_CMD_PTP_OUT_TRANSMIT_LEN);
>   	size_t len;
> +	int rc;
> 
>   	MCDI_SET_DWORD(ptp_data->txbuf, PTP_IN_OP, MC_CMD_PTP_OP_TRANSMIT);
>   	MCDI_SET_DWORD(ptp_data->txbuf, PTP_IN_PERIPH_ID, 0);
> @@ -1182,7 +1199,10 @@ static void efx_ptp_xmit_skb_mc(struct efx_nic *efx, struct sk_buff *skb)
> 
>   	skb_tstamp_tx(skb, &timestamps);
> 
> -	rc = 0;
> +	/* Add the filters after sending back the timestamp to avoid delaying it
> +	 * or ptp4l may timeout.
> +	 */
> +	efx_ptp_insert_unicast_filter(efx, skb);
> 
>   fail:
>   	dev_kfree_skb_any(skb);
> @@ -1298,6 +1318,21 @@ static inline void efx_ptp_process_rx(struct efx_nic *efx, struct sk_buff *skb)
>   	local_bh_enable();
>   }
> 
> +static bool efx_ptp_filter_exists(struct list_head *ptp_list,
> +				  struct efx_filter_spec *spec)
> +{
> +	struct efx_ptp_rxfilter *rxfilter;
> +
> +	list_for_each_entry(rxfilter, ptp_list, list) {
> +		if (rxfilter->ether_type == spec->ether_type &&
> +		    rxfilter->loc_port == spec->loc_port &&
> +		    !memcmp(rxfilter->loc_host, spec->loc_host, sizeof(spec->loc_host)))
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
>   static void efx_ptp_remove_filters(struct efx_nic *efx,
>   				   struct list_head *ptp_list)
>   {
> @@ -1328,6 +1363,9 @@ static int efx_ptp_insert_filter(struct efx_nic *efx,
>   	struct efx_ptp_rxfilter *rxfilter;
>   	int rc;
> 
> +	if (efx_ptp_filter_exists(ptp_list, spec))
> +		return 0;
> +
>   	rxfilter = kzalloc(sizeof(*rxfilter), GFP_KERNEL);
>   	if (!rxfilter)
>   		return -ENOMEM;
> @@ -1337,6 +1375,9 @@ static int efx_ptp_insert_filter(struct efx_nic *efx,
>   		goto fail;
> 
>   	rxfilter->handle = rc;
> +	rxfilter->ether_type = spec->ether_type;
> +	rxfilter->loc_port = spec->loc_port;
> +	memcpy(rxfilter->loc_host, spec->loc_host, sizeof(spec->loc_host));
>   	list_add(&rxfilter->list, ptp_list);
> 
>   	return 0;
> @@ -1429,6 +1470,67 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
>   	return rc;
>   }
> 
> +static bool efx_ptp_valid_unicast_event_pkt(struct sk_buff *skb)
> +{
> +	if (skb->protocol == htons(ETH_P_IP)) {
> +		return ip_hdr(skb)->daddr != htonl(PTP_ADDR_IPV4) &&
> +			ip_hdr(skb)->protocol == IPPROTO_UDP &&
> +			udp_hdr(skb)->source == htons(PTP_EVENT_PORT);
> +	} else if (skb->protocol == htons(ETH_P_IPV6)) {
> +		struct in6_addr mcast_addr = {{PTP_ADDR_IPV6}};
> +
> +		return !ipv6_addr_equal(&ipv6_hdr(skb)->daddr, &mcast_addr) &&
> +			ipv6_hdr(skb)->nexthdr == IPPROTO_UDP &&
> +			udp_hdr(skb)->source == htons(PTP_EVENT_PORT);
> +	}
> +	return false;
> +}
> +
> +static int efx_ptp_insert_unicast_filter(struct efx_nic *efx,
> +					 struct sk_buff *skb)
> +{
> +	struct efx_ptp_data *ptp = efx->ptp_data;
> +	int rc;
> +
> +	if (!efx_ptp_valid_unicast_event_pkt(skb))
> +		return -EINVAL;
> +
> +	if (skb->protocol == htons(ETH_P_IP)) {
> +		__be32 addr = ip_hdr(skb)->saddr;
> +
> +		rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast,
> +						addr, PTP_EVENT_PORT);
> +		if (rc < 0)
> +			goto fail;
> +
> +		rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast,
> +						addr, PTP_GENERAL_PORT);

Not sure why do you want general packets to go to the queue for 
timestamping? There is no need for timestamp them in the protocol.
The same question is for multicast version.

> +		if (rc < 0)
> +			goto fail;
> +	} else if (efx_ptp_use_mac_tx_timestamps(efx)) {
> +		/* IPv6 PTP only supported by devices with MAC hw timestamp */
> +		struct in6_addr *addr = &ipv6_hdr(skb)->saddr;
> +
> +		rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
> +						addr, PTP_EVENT_PORT);
> +		if (rc < 0)
> +			goto fail;
> +
> +		rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
> +						addr, PTP_GENERAL_PORT);
> +		if (rc < 0)
> +			goto fail;
> +	} else {
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +
> +fail:
> +	efx_ptp_remove_filters(efx, &ptp->rxfilters_ucast);
> +	return rc;
> +}
> +
>   static int efx_ptp_start(struct efx_nic *efx)
>   {
>   	struct efx_ptp_data *ptp = efx->ptp_data;
> --
> 2.34.3
> 

Re: [PATCH net-next v4 3/4] sfc: support unicast PTP

[Íñigo Huguet]

On Wed, Feb 22, 2023 at 3:22 PM Vadim Fedorenko
<vadim.fedorenko@linux.dev> wrote:
> Not sure why do you want general packets to go to the queue for
> timestamping? There is no need for timestamp them in the protocol.
> The same question is for multicast version.

The reason is explained in a comment in efx_ptp_insert_multicast filters:
   Must filter on both event and general ports to ensure
   that there is no packet re-ordering

So the reason is not that we want the timestamp, but we want those
packets to go to the same RX queue. As a side effect, they will be
timestamped, yes, but as far as I know, there is no way to avoid that.

-- 
Íñigo Huguet

Re: [PATCH net-next v4 3/4] sfc: support unicast PTP

[Richard Cochran]

On Wed, Feb 22, 2023 at 03:41:51PM +0100, Íñigo Huguet wrote:

> The reason is explained in a comment in efx_ptp_insert_multicast filters:
>    Must filter on both event and general ports to ensure
>    that there is no packet re-ordering

There is nothing wrong with re-ordering.  Nothing guarantees that
datagrams are received in the order they are sent.

The user space PTP stack must be handle out of order messages correct
(which ptp4l does do BTW).

Thanks,
Richard

Re: [PATCH net-next v4 3/4] sfc: support unicast PTP

[Íñigo Huguet]

On Wed, Feb 22, 2023 at 5:52 PM Richard Cochran
<richardcochran@gmail.com> wrote:
>
> On Wed, Feb 22, 2023 at 03:41:51PM +0100, Íñigo Huguet wrote:
>
> > The reason is explained in a comment in efx_ptp_insert_multicast filters:
> >    Must filter on both event and general ports to ensure
> >    that there is no packet re-ordering
>
> There is nothing wrong with re-ordering.  Nothing guarantees that
> datagrams are received in the order they are sent.
>
> The user space PTP stack must be handle out of order messages correct
> (which ptp4l does do BTW).

That's good to know, thanks.

Anyway,this patch set addresses the addition of Unicast PTP support,
and regarding the timestamping of PTP-general packets it has
maintained the existing driver behaviour. So I hope this is not a
blocker for the current patches, I think it should be addressed in a
different patch set if the time permits.

>
> Thanks,
> Richard
>


-- 
Íñigo Huguet

Re: [PATCH net-next v4 3/4] sfc: support unicast PTP

[Martin Habets]

On Wed, Feb 22, 2023 at 08:52:45AM -0800, Richard Cochran wrote:
> On Wed, Feb 22, 2023 at 03:41:51PM +0100, Íñigo Huguet wrote:
> 
> > The reason is explained in a comment in efx_ptp_insert_multicast filters:
> >    Must filter on both event and general ports to ensure
> >    that there is no packet re-ordering
> 
> There is nothing wrong with re-ordering.

I disagree. If re-ordering can be avoided that is a good thing.

> Nothing guarantees that
> datagrams are received in the order they are sent.

True, but they usually are.

> The user space PTP stack must be handle out of order messages correct
> (which ptp4l does do BTW).

This takes CPU time. If it can be avoided that is a good thing, as
it puts less pressure on the host. It is not just about CPU load, it
is also about latency.

Martin

Re: [PATCH net-next v4 3/4] sfc: support unicast PTP

[Richard Cochran]

On Fri, Feb 24, 2023 at 09:01:59AM +0000, Martin Habets wrote:
> On Wed, Feb 22, 2023 at 08:52:45AM -0800, Richard Cochran wrote:
> > The user space PTP stack must be handle out of order messages correct
> > (which ptp4l does do BTW).
> 
> This takes CPU time. If it can be avoided that is a good thing, as
> it puts less pressure on the host. It is not just about CPU load, it
> is also about latency.

It neither takes more CPU nor induces additional latency to handle
messages out of order.  The stack simply uses an event based state
machine.  In between events, the stack is sleeping on input.

Thanks,
Richard

Re: [PATCH net-next v4 3/4] sfc: support unicast PTP

[Richard Cochran]

On Fri, Feb 24, 2023 at 07:43:35AM -0800, Richard Cochran wrote:
> On Fri, Feb 24, 2023 at 09:01:59AM +0000, Martin Habets wrote:
> > On Wed, Feb 22, 2023 at 08:52:45AM -0800, Richard Cochran wrote:
> > > The user space PTP stack must be handle out of order messages correct
> > > (which ptp4l does do BTW).
> > 
> > This takes CPU time. If it can be avoided that is a good thing, as
> > it puts less pressure on the host. It is not just about CPU load, it
> > is also about latency.
> 
> It neither takes more CPU nor induces additional latency to handle
> messages out of order.  The stack simply uses an event based state
> machine.  In between events, the stack is sleeping on input.

If you are talking about CPU cycles and latency *in the driver* then,
by all means, choose your queues to implement the best solution.

But that wasn't the argument given in the original post.

Thanks,
Richard