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

[PATCH net 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 master/slave and unicast/multicast
Reported-by: Yalin Li <yalli@redhat.com>
Signed-off-by: Íñigo Huguet <ihuguet@redhat.com>

Íñ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, 217 insertions(+), 55 deletions(-)

--
2.34.3

[PATCH net 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 | 72 ++++++++++++++++++++++------------
 1 file changed, 46 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
index 9f07e1ba7780..53817b4350a5 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>
@@ -213,6 +214,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 +240,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 +309,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 +1301,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 +1322,55 @@ 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 = efx_filter_insert_filter(efx, rxfilter, true);
+	int rc = efx_filter_insert_filter(efx, spec, true);
 	if (rc < 0)
 		return rc;
-	ptp->rxfilters[ptp->rxfilters_count] = rc;
-	ptp->rxfilters_count++;
+
+	rxfilter = kzalloc(sizeof(*rxfilter), GFP_KERNEL);
+	if (!rxfilter)
+		return -ENOMEM;
+
+	rxfilter->handle = rc;
+	list_add(&rxfilter->list, &ptp->rxfilters_mcast);
+
 	return 0;
 }
 
 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 +1378,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 +1584,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 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 53817b4350a5..5d5f70c56048 100644
--- a/drivers/net/ethernet/sfc/ptp.c
+++ b/drivers/net/ethernet/sfc/ptp.c
@@ -241,6 +241,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)
@@ -310,6 +311,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;
@@ -1298,12 +1300,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);
@@ -1322,9 +1324,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 = efx_filter_insert_filter(efx, spec, true);
@@ -1336,32 +1338,34 @@ static int efx_ptp_insert_filter(struct efx_nic *efx,
 		return -ENOMEM;
 
 	rxfilter->handle = rc;
-	list_add(&rxfilter->list, &ptp->rxfilters_mcast);
+	list_add(&rxfilter->list, ptp_list);
 
 	return 0;
 }
 
-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;
@@ -1370,7 +1374,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)
@@ -1384,11 +1388,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;
 
@@ -1396,15 +1402,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;
 	}
@@ -1412,7 +1422,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;
 }
 
@@ -1437,7 +1447,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;
 }
 
@@ -1453,7 +1463,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);
@@ -1585,6 +1596,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 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.

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

diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
index 5d5f70c56048..f9342e21bf31 100644
--- a/drivers/net/ethernet/sfc/ptp.c
+++ b/drivers/net/ethernet/sfc/ptp.c
@@ -217,10 +217,15 @@ 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_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;
 };
 
@@ -369,6 +374,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)
 {
@@ -1114,6 +1121,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
@@ -1137,6 +1146,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);
@@ -1148,7 +1164,7 @@ static void efx_ptp_xmit_skb_mc(struct efx_nic *efx, struct sk_buff *skb)
 {
 	struct efx_ptp_data *ptp_data = efx->ptp_data;
 	struct skb_shared_hwtstamps timestamps;
-	int rc = -EIO;
+	int rc;
 	MCDI_DECLARE_BUF(txtime, MC_CMD_PTP_OUT_TRANSMIT_LEN);
 	size_t len;
 
@@ -1184,7 +1200,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);
@@ -1300,6 +1319,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,8 +1362,12 @@ static int efx_ptp_insert_filter(struct efx_nic *efx,
 				 struct efx_filter_spec *spec)
 {
 	struct efx_ptp_rxfilter *rxfilter;
+	int rc;
+
+	if (efx_ptp_filter_exists(ptp_list, spec))
+		return 0;
 
-	int rc = efx_filter_insert_filter(efx, spec, true);
+	rc = efx_filter_insert_filter(efx, spec, true);
 	if (rc < 0)
 		return rc;
 
@@ -1338,6 +1376,9 @@ static int efx_ptp_insert_filter(struct efx_nic *efx,
 		return -ENOMEM;
 
 	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;
@@ -1426,6 +1467,63 @@ 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)->protocol == IPPROTO_UDP &&
+			udp_hdr(skb)->source == htons(PTP_EVENT_PORT);
+	} else if (skb->protocol == htons(ETH_P_IPV6)) {
+		return 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 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 | 120 +++++++++++++++++++++------------
 1 file changed, 76 insertions(+), 44 deletions(-)

diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
index f9342e21bf31..fd37a61058e7 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.
@@ -226,6 +229,7 @@ struct efx_ptp_rxfilter {
 	__be16 ether_type;
 	__be16 loc_port;
 	__be32 loc_host[4];
+	unsigned long expiry;
 	int handle;
 };
 
@@ -1319,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;
 
@@ -1328,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 inline 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,
@@ -1340,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);
 	}
 }
 
@@ -1357,23 +1367,24 @@ static void efx_ptp_init_filter(struct efx_nic *efx,
 			   efx_rx_queue_index(queue));
 }
 
-static int efx_ptp_insert_filter(struct efx_nic *efx,
-				 struct list_head *ptp_list,
-				 struct efx_filter_spec *spec)
+static struct efx_ptp_rxfilter *
+efx_ptp_insert_filter(struct efx_nic *efx, struct list_head *ptp_list,
+		      struct efx_filter_spec *spec)
 {
 	struct efx_ptp_rxfilter *rxfilter;
 	int rc;
 
-	if (efx_ptp_filter_exists(ptp_list, spec))
-		return 0;
+	rxfilter = efx_ptp_find_filter(ptp_list, spec);
+	if (rxfilter)
+		return rxfilter;
 
 	rc = efx_filter_insert_filter(efx, spec, true);
 	if (rc < 0)
-		return rc;
+		return ERR_PTR(rc);
 
 	rxfilter = kzalloc(sizeof(*rxfilter), GFP_KERNEL);
 	if (!rxfilter)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	rxfilter->handle = rc;
 	rxfilter->ether_type = spec->ether_type;
@@ -1381,12 +1392,12 @@ static int efx_ptp_insert_filter(struct efx_nic *efx,
 	memcpy(rxfilter->loc_host, spec->loc_host, sizeof(spec->loc_host));
 	list_add(&rxfilter->list, ptp_list);
 
-	return 0;
+	return rxfilter;
 }
 
-static int efx_ptp_insert_ipv4_filter(struct efx_nic *efx,
-				      struct list_head *ptp_list,
-				      __be32 addr, u16 port)
+static struct efx_ptp_rxfilter *
+efx_ptp_insert_ipv4_filter(struct efx_nic *efx, struct list_head *ptp_list,
+			   __be32 addr, u16 port)
 {
 	struct efx_filter_spec spec;
 
@@ -1395,9 +1406,9 @@ static int efx_ptp_insert_ipv4_filter(struct efx_nic *efx,
 	return efx_ptp_insert_filter(efx, ptp_list, &spec);
 }
 
-static int efx_ptp_insert_ipv6_filter(struct efx_nic *efx,
-				      struct list_head *ptp_list,
-				      struct in6_addr *addr, u16 port)
+static struct efx_ptp_rxfilter *
+efx_ptp_insert_ipv6_filter(struct efx_nic *efx, struct list_head *ptp_list,
+			   struct in6_addr *addr, u16 port)
 {
 	struct efx_filter_spec spec;
 
@@ -1406,7 +1417,8 @@ static int efx_ptp_insert_ipv6_filter(struct efx_nic *efx,
 	return efx_ptp_insert_filter(efx, ptp_list, &spec);
 }
 
-static int efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx)
+static struct efx_ptp_rxfilter *
+efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx)
 {
 	const u8 addr[ETH_ALEN] = PTP_ADDR_ETHER;
 	struct efx_filter_spec spec;
@@ -1421,7 +1433,7 @@ static int efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx)
 static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
 {
 	struct efx_ptp_data *ptp = efx->ptp_data;
-	int rc;
+	struct efx_ptp_rxfilter *rc;
 
 	if (!ptp->channel || !list_empty(&ptp->rxfilters_mcast))
 		return 0;
@@ -1431,12 +1443,12 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
 	 */
 	rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_mcast,
 					htonl(PTP_ADDR_IPV4), PTP_EVENT_PORT);
-	if (rc < 0)
+	if (IS_ERR(rc))
 		goto fail;
 
 	rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_mcast,
 					htonl(PTP_ADDR_IPV4), PTP_GENERAL_PORT);
-	if (rc < 0)
+	if (IS_ERR(rc))
 		goto fail;
 
 	/* if the NIC supports hw timestamps by the MAC, we can support
@@ -1447,16 +1459,16 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
 
 		rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_mcast,
 						&ipv6_addr, PTP_EVENT_PORT);
-		if (rc < 0)
+		if (IS_ERR(rc))
 			goto fail;
 
 		rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_mcast,
 						&ipv6_addr, PTP_GENERAL_PORT);
-		if (rc < 0)
+		if (IS_ERR(rc))
 			goto fail;
 
 		rc = efx_ptp_insert_eth_multicast_filter(efx);
-		if (rc < 0)
+		if (IS_ERR(rc))
 			goto fail;
 	}
 
@@ -1464,7 +1476,7 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
 
 fail:
 	efx_ptp_remove_filters(efx, &ptp->rxfilters_mcast);
-	return rc;
+	return PTR_ERR(rc);
 }
 
 static bool efx_ptp_valid_unicast_event_pkt(struct sk_buff *skb)
@@ -1483,7 +1495,7 @@ 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;
+	struct efx_ptp_rxfilter *rxfilter;
 
 	if (!efx_ptp_valid_unicast_event_pkt(skb))
 		return -EINVAL;
@@ -1491,28 +1503,36 @@ static int efx_ptp_insert_unicast_filter(struct efx_nic *efx,
 	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)
+		rxfilter = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast,
+						      addr, PTP_EVENT_PORT);
+		if (IS_ERR(rxfilter))
 			goto fail;
 
-		rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast,
-						addr, PTP_GENERAL_PORT);
-		if (rc < 0)
+		rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
+
+		rxfilter = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast,
+						      addr, PTP_GENERAL_PORT);
+		if (rxfilter < 0)
 			goto fail;
+
+		rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
 	} 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)
+		rxfilter = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
+						      addr, PTP_EVENT_PORT);
+		if (IS_ERR(rxfilter))
 			goto fail;
 
-		rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
-						addr, PTP_GENERAL_PORT);
-		if (rc < 0)
+		rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
+
+		rxfilter = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
+						      addr, PTP_GENERAL_PORT);
+		if (IS_ERR(rxfilter))
 			goto fail;
+
+		rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
 	} else {
 		return -EOPNOTSUPP;
 	}
@@ -1521,7 +1541,18 @@ static int efx_ptp_insert_unicast_filter(struct efx_nic *efx,
 
 fail:
 	efx_ptp_remove_filters(efx, &ptp->rxfilters_ucast);
-	return rc;
+	return PTR_ERR(rxfilter);
+}
+
+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)
@@ -1615,6 +1646,7 @@ static void efx_ptp_worker(struct work_struct *work)
 	}
 
 	efx_ptp_drop_time_expired_events(efx);
+	efx_ptp_drop_expired_unicast_filters(efx);
 
 	__skb_queue_head_init(&tempq);
 	efx_ptp_process_events(efx, &tempq);
-- 
2.34.3

Re: [PATCH net 4/4] sfc: remove expired unicast PTP filters

[kernel test robot]

Hi Íñigo,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net/master]

url:    https://github.com/intel-lab-lkp/linux/commits/igo-Huguet/sfc-store-PTP-filters-in-a-list/20230201-000831
patch link:    https://lore.kernel.org/r/20230131160506.47552-5-ihuguet%40redhat.com
patch subject: [PATCH net 4/4] sfc: remove expired unicast PTP filters
config: ia64-allyesconfig (https://download.01.org/0day-ci/archive/20230201/202302010101.qyLPmkHu-lkp@intel.com/config)
compiler: ia64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/d631c3b59ac7ba7f62da245114156866ea74a15b
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review igo-Huguet/sfc-store-PTP-filters-in-a-list/20230201-000831
        git checkout d631c3b59ac7ba7f62da245114156866ea74a15b
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=ia64 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=ia64 SHELL=/bin/bash drivers/net/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/net/ethernet/sfc/ptp.c: In function 'efx_ptp_insert_unicast_filter':
>> drivers/net/ethernet/sfc/ptp.c:1515:30: warning: ordered comparison of pointer with integer zero [-Wextra]
    1515 |                 if (rxfilter < 0)
         |                              ^


vim +1515 drivers/net/ethernet/sfc/ptp.c

  1493	
  1494	static int efx_ptp_insert_unicast_filter(struct efx_nic *efx,
  1495						 struct sk_buff *skb)
  1496	{
  1497		struct efx_ptp_data *ptp = efx->ptp_data;
  1498		struct efx_ptp_rxfilter *rxfilter;
  1499	
  1500		if (!efx_ptp_valid_unicast_event_pkt(skb))
  1501			return -EINVAL;
  1502	
  1503		if (skb->protocol == htons(ETH_P_IP)) {
  1504			__be32 addr = ip_hdr(skb)->saddr;
  1505	
  1506			rxfilter = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast,
  1507							      addr, PTP_EVENT_PORT);
  1508			if (IS_ERR(rxfilter))
  1509				goto fail;
  1510	
  1511			rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
  1512	
  1513			rxfilter = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast,
  1514							      addr, PTP_GENERAL_PORT);
> 1515			if (rxfilter < 0)
  1516				goto fail;
  1517	
  1518			rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
  1519		} else if (efx_ptp_use_mac_tx_timestamps(efx)) {
  1520			/* IPv6 PTP only supported by devices with MAC hw timestamp */
  1521			struct in6_addr *addr = &ipv6_hdr(skb)->saddr;
  1522	
  1523			rxfilter = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
  1524							      addr, PTP_EVENT_PORT);
  1525			if (IS_ERR(rxfilter))
  1526				goto fail;
  1527	
  1528			rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
  1529	
  1530			rxfilter = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
  1531							      addr, PTP_GENERAL_PORT);
  1532			if (IS_ERR(rxfilter))
  1533				goto fail;
  1534	
  1535			rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
  1536		} else {
  1537			return -EOPNOTSUPP;
  1538		}
  1539	
  1540		return 0;
  1541	
  1542	fail:
  1543		efx_ptp_remove_filters(efx, &ptp->rxfilters_ucast);
  1544		return PTR_ERR(rxfilter);
  1545	}
  1546	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

[PATCH net-next v2 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 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

For the missing IS_ERR:
Reported-by: kernel test robot <lkp@intel.com>

Íñ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 | 274 ++++++++++++++++++++++++++-------
 1 file changed, 219 insertions(+), 55 deletions(-)

--
2.34.3

[PATCH net-next v2 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 | 72 ++++++++++++++++++++++------------
 1 file changed, 46 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
index 9f07e1ba7780..53817b4350a5 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>
@@ -213,6 +214,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 +240,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 +309,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 +1301,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 +1322,55 @@ 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 = efx_filter_insert_filter(efx, rxfilter, true);
+	int rc = efx_filter_insert_filter(efx, spec, true);
 	if (rc < 0)
 		return rc;
-	ptp->rxfilters[ptp->rxfilters_count] = rc;
-	ptp->rxfilters_count++;
+
+	rxfilter = kzalloc(sizeof(*rxfilter), GFP_KERNEL);
+	if (!rxfilter)
+		return -ENOMEM;
+
+	rxfilter->handle = rc;
+	list_add(&rxfilter->list, &ptp->rxfilters_mcast);
+
 	return 0;
 }
 
 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 +1378,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 +1584,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 v2 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 53817b4350a5..5d5f70c56048 100644
--- a/drivers/net/ethernet/sfc/ptp.c
+++ b/drivers/net/ethernet/sfc/ptp.c
@@ -241,6 +241,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)
@@ -310,6 +311,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;
@@ -1298,12 +1300,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);
@@ -1322,9 +1324,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 = efx_filter_insert_filter(efx, spec, true);
@@ -1336,32 +1338,34 @@ static int efx_ptp_insert_filter(struct efx_nic *efx,
 		return -ENOMEM;
 
 	rxfilter->handle = rc;
-	list_add(&rxfilter->list, &ptp->rxfilters_mcast);
+	list_add(&rxfilter->list, ptp_list);
 
 	return 0;
 }
 
-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;
@@ -1370,7 +1374,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)
@@ -1384,11 +1388,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;
 
@@ -1396,15 +1402,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;
 	}
@@ -1412,7 +1422,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;
 }
 
@@ -1437,7 +1447,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;
 }
 
@@ -1453,7 +1463,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);
@@ -1585,6 +1596,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 v2 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.

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

diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
index 5d5f70c56048..a3e827cd84a8 100644
--- a/drivers/net/ethernet/sfc/ptp.c
+++ b/drivers/net/ethernet/sfc/ptp.c
@@ -217,10 +217,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;
 };
 
@@ -369,6 +375,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)
 {
@@ -1114,6 +1122,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
@@ -1137,6 +1147,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);
@@ -1148,7 +1165,7 @@ static void efx_ptp_xmit_skb_mc(struct efx_nic *efx, struct sk_buff *skb)
 {
 	struct efx_ptp_data *ptp_data = efx->ptp_data;
 	struct skb_shared_hwtstamps timestamps;
-	int rc = -EIO;
+	int rc;
 	MCDI_DECLARE_BUF(txtime, MC_CMD_PTP_OUT_TRANSMIT_LEN);
 	size_t len;
 
@@ -1184,7 +1201,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);
@@ -1300,6 +1320,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,8 +1363,12 @@ static int efx_ptp_insert_filter(struct efx_nic *efx,
 				 struct efx_filter_spec *spec)
 {
 	struct efx_ptp_rxfilter *rxfilter;
+	int rc;
+
+	if (efx_ptp_filter_exists(ptp_list, spec))
+		return 0;
 
-	int rc = efx_filter_insert_filter(efx, spec, true);
+	rc = efx_filter_insert_filter(efx, spec, true);
 	if (rc < 0)
 		return rc;
 
@@ -1338,6 +1377,9 @@ static int efx_ptp_insert_filter(struct efx_nic *efx,
 		return -ENOMEM;
 
 	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;
@@ -1426,6 +1468,63 @@ 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)->protocol == IPPROTO_UDP &&
+			udp_hdr(skb)->source == htons(PTP_EVENT_PORT);
+	} else if (skb->protocol == htons(ETH_P_IPV6)) {
+		return 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 v2 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 | 121 +++++++++++++++++++++------------
 1 file changed, 77 insertions(+), 44 deletions(-)

diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
index a3e827cd84a8..dd46ca6c070e 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.
@@ -220,6 +223,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 {
@@ -227,6 +231,7 @@ struct efx_ptp_rxfilter {
 	__be16 ether_type;
 	__be16 loc_port;
 	__be32 loc_host[4];
+	unsigned long expiry;
 	int handle;
 };
 
@@ -1320,8 +1325,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;
 
@@ -1329,10 +1334,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 inline 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,
@@ -1341,10 +1355,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,23 +1369,24 @@ static void efx_ptp_init_filter(struct efx_nic *efx,
 			   efx_rx_queue_index(queue));
 }
 
-static int efx_ptp_insert_filter(struct efx_nic *efx,
-				 struct list_head *ptp_list,
-				 struct efx_filter_spec *spec)
+static struct efx_ptp_rxfilter *
+efx_ptp_insert_filter(struct efx_nic *efx, struct list_head *ptp_list,
+		      struct efx_filter_spec *spec)
 {
 	struct efx_ptp_rxfilter *rxfilter;
 	int rc;
 
-	if (efx_ptp_filter_exists(ptp_list, spec))
-		return 0;
+	rxfilter = efx_ptp_find_filter(ptp_list, spec);
+	if (rxfilter)
+		return rxfilter;
 
 	rc = efx_filter_insert_filter(efx, spec, true);
 	if (rc < 0)
-		return rc;
+		return ERR_PTR(rc);
 
 	rxfilter = kzalloc(sizeof(*rxfilter), GFP_KERNEL);
 	if (!rxfilter)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	rxfilter->handle = rc;
 	rxfilter->ether_type = spec->ether_type;
@@ -1382,12 +1394,12 @@ static int efx_ptp_insert_filter(struct efx_nic *efx,
 	memcpy(rxfilter->loc_host, spec->loc_host, sizeof(spec->loc_host));
 	list_add(&rxfilter->list, ptp_list);
 
-	return 0;
+	return rxfilter;
 }
 
-static int efx_ptp_insert_ipv4_filter(struct efx_nic *efx,
-				      struct list_head *ptp_list,
-				      __be32 addr, u16 port)
+static struct efx_ptp_rxfilter *
+efx_ptp_insert_ipv4_filter(struct efx_nic *efx, struct list_head *ptp_list,
+			   __be32 addr, u16 port)
 {
 	struct efx_filter_spec spec;
 
@@ -1396,9 +1408,9 @@ static int efx_ptp_insert_ipv4_filter(struct efx_nic *efx,
 	return efx_ptp_insert_filter(efx, ptp_list, &spec);
 }
 
-static int efx_ptp_insert_ipv6_filter(struct efx_nic *efx,
-				      struct list_head *ptp_list,
-				      struct in6_addr *addr, u16 port)
+static struct efx_ptp_rxfilter *
+efx_ptp_insert_ipv6_filter(struct efx_nic *efx, struct list_head *ptp_list,
+			   struct in6_addr *addr, u16 port)
 {
 	struct efx_filter_spec spec;
 
@@ -1407,7 +1419,8 @@ static int efx_ptp_insert_ipv6_filter(struct efx_nic *efx,
 	return efx_ptp_insert_filter(efx, ptp_list, &spec);
 }
 
-static int efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx)
+static struct efx_ptp_rxfilter *
+efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx)
 {
 	const u8 addr[ETH_ALEN] = PTP_ADDR_ETHER;
 	struct efx_filter_spec spec;
@@ -1422,7 +1435,7 @@ static int efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx)
 static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
 {
 	struct efx_ptp_data *ptp = efx->ptp_data;
-	int rc;
+	struct efx_ptp_rxfilter *rc;
 
 	if (!ptp->channel || !list_empty(&ptp->rxfilters_mcast))
 		return 0;
@@ -1432,12 +1445,12 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
 	 */
 	rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_mcast,
 					htonl(PTP_ADDR_IPV4), PTP_EVENT_PORT);
-	if (rc < 0)
+	if (IS_ERR(rc))
 		goto fail;
 
 	rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_mcast,
 					htonl(PTP_ADDR_IPV4), PTP_GENERAL_PORT);
-	if (rc < 0)
+	if (IS_ERR(rc))
 		goto fail;
 
 	/* if the NIC supports hw timestamps by the MAC, we can support
@@ -1448,16 +1461,16 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
 
 		rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_mcast,
 						&ipv6_addr, PTP_EVENT_PORT);
-		if (rc < 0)
+		if (IS_ERR(rc))
 			goto fail;
 
 		rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_mcast,
 						&ipv6_addr, PTP_GENERAL_PORT);
-		if (rc < 0)
+		if (IS_ERR(rc))
 			goto fail;
 
 		rc = efx_ptp_insert_eth_multicast_filter(efx);
-		if (rc < 0)
+		if (IS_ERR(rc))
 			goto fail;
 	}
 
@@ -1465,7 +1478,7 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
 
 fail:
 	efx_ptp_remove_filters(efx, &ptp->rxfilters_mcast);
-	return rc;
+	return PTR_ERR(rc);
 }
 
 static bool efx_ptp_valid_unicast_event_pkt(struct sk_buff *skb)
@@ -1484,7 +1497,7 @@ 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;
+	struct efx_ptp_rxfilter *rxfilter;
 
 	if (!efx_ptp_valid_unicast_event_pkt(skb))
 		return -EINVAL;
@@ -1492,28 +1505,36 @@ static int efx_ptp_insert_unicast_filter(struct efx_nic *efx,
 	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)
+		rxfilter = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast,
+						      addr, PTP_EVENT_PORT);
+		if (IS_ERR(rxfilter))
 			goto fail;
 
-		rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast,
-						addr, PTP_GENERAL_PORT);
-		if (rc < 0)
+		rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
+
+		rxfilter = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast,
+						      addr, PTP_GENERAL_PORT);
+		if (IS_ERR(rxfilter))
 			goto fail;
+
+		rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
 	} 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)
+		rxfilter = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
+						      addr, PTP_EVENT_PORT);
+		if (IS_ERR(rxfilter))
 			goto fail;
 
-		rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
-						addr, PTP_GENERAL_PORT);
-		if (rc < 0)
+		rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
+
+		rxfilter = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
+						      addr, PTP_GENERAL_PORT);
+		if (IS_ERR(rxfilter))
 			goto fail;
+
+		rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
 	} else {
 		return -EOPNOTSUPP;
 	}
@@ -1522,7 +1543,18 @@ static int efx_ptp_insert_unicast_filter(struct efx_nic *efx,
 
 fail:
 	efx_ptp_remove_filters(efx, &ptp->rxfilters_ucast);
-	return rc;
+	return PTR_ERR(rxfilter);
+}
+
+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)
@@ -1616,6 +1648,7 @@ static void efx_ptp_worker(struct work_struct *work)
 	}
 
 	efx_ptp_drop_time_expired_events(efx);
+	efx_ptp_drop_expired_unicast_filters(efx);
 
 	__skb_queue_head_init(&tempq);
 	efx_ptp_process_events(efx, &tempq);
-- 
2.34.3

Re: [PATCH net 4/4] sfc: remove expired unicast PTP filters

[kernel test robot]

Hi Íñigo,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net/master]

url:    https://github.com/intel-lab-lkp/linux/commits/igo-Huguet/sfc-store-PTP-filters-in-a-list/20230201-000831
patch link:    https://lore.kernel.org/r/20230131160506.47552-5-ihuguet%40redhat.com
patch subject: [PATCH net 4/4] sfc: remove expired unicast PTP filters
config: parisc-randconfig-s033-20230129 (https://download.01.org/0day-ci/archive/20230202/202302020019.2MT9fEOy-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 12.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/d631c3b59ac7ba7f62da245114156866ea74a15b
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review igo-Huguet/sfc-store-PTP-filters-in-a-list/20230201-000831
        git checkout d631c3b59ac7ba7f62da245114156866ea74a15b
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=parisc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=parisc SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

sparse warnings: (new ones prefixed by >>)
>> drivers/net/ethernet/sfc/ptp.c:1515:30: sparse: sparse: incompatible types for operation (<):
>> drivers/net/ethernet/sfc/ptp.c:1515:30: sparse:    struct efx_ptp_rxfilter *[assigned] rxfilter
>> drivers/net/ethernet/sfc/ptp.c:1515:30: sparse:    int

vim +1515 drivers/net/ethernet/sfc/ptp.c

  1493	
  1494	static int efx_ptp_insert_unicast_filter(struct efx_nic *efx,
  1495						 struct sk_buff *skb)
  1496	{
  1497		struct efx_ptp_data *ptp = efx->ptp_data;
  1498		struct efx_ptp_rxfilter *rxfilter;
  1499	
  1500		if (!efx_ptp_valid_unicast_event_pkt(skb))
  1501			return -EINVAL;
  1502	
  1503		if (skb->protocol == htons(ETH_P_IP)) {
  1504			__be32 addr = ip_hdr(skb)->saddr;
  1505	
  1506			rxfilter = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast,
  1507							      addr, PTP_EVENT_PORT);
  1508			if (IS_ERR(rxfilter))
  1509				goto fail;
  1510	
  1511			rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
  1512	
  1513			rxfilter = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast,
  1514							      addr, PTP_GENERAL_PORT);
> 1515			if (rxfilter < 0)
  1516				goto fail;
  1517	
  1518			rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
  1519		} else if (efx_ptp_use_mac_tx_timestamps(efx)) {
  1520			/* IPv6 PTP only supported by devices with MAC hw timestamp */
  1521			struct in6_addr *addr = &ipv6_hdr(skb)->saddr;
  1522	
  1523			rxfilter = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
  1524							      addr, PTP_EVENT_PORT);
  1525			if (IS_ERR(rxfilter))
  1526				goto fail;
  1527	
  1528			rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
  1529	
  1530			rxfilter = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
  1531							      addr, PTP_GENERAL_PORT);
  1532			if (IS_ERR(rxfilter))
  1533				goto fail;
  1534	
  1535			rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
  1536		} else {
  1537			return -EOPNOTSUPP;
  1538		}
  1539	
  1540		return 0;
  1541	
  1542	fail:
  1543		efx_ptp_remove_filters(efx, &ptp->rxfilters_ucast);
  1544		return PTR_ERR(rxfilter);
  1545	}
  1546	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

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

[Jakub Kicinski]

On Wed,  1 Feb 2023 09:08:45 +0100 Íñigo Huguet wrote:
> v2: fixed missing IS_ERR
>     added doc of missing fields in efx_ptp_rxfilter

1. don't repost within 24h, *especially* if you're reposting
because of compilation problems

https://www.kernel.org/doc/html/next/process/maintainer-netdev.html

2. please don't repost in a thread, it makes it harder for me 
to maintain a review queue

3. drop the pointless inline in the source file in patch 4

+static inline void efx_ptp_remove_one_filter(struct efx_nic *efx,
+					     struct efx_ptp_rxfilter *rxfilter)

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

[Íñigo Huguet]

On Wed, Feb 1, 2023 at 8:05 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed,  1 Feb 2023 09:08:45 +0100 Íñigo Huguet wrote:
> > v2: fixed missing IS_ERR
> >     added doc of missing fields in efx_ptp_rxfilter
>
> 1. don't repost within 24h, *especially* if you're reposting
> because of compilation problems
>
> https://www.kernel.org/doc/html/next/process/maintainer-netdev.html

Sorry, I wasn't aware of this.

> 2. please don't repost in a thread, it makes it harder for me
> to maintain a review queue

What do you mean? I sent it with `git send-email --in-reply-to`, I
thought this was the canonical way to send a v2 and superseed the
previous version.

> 3. drop the pointless inline in the source file in patch 4
>
> +static inline void efx_ptp_remove_one_filter(struct efx_nic *efx,
> +                                            struct efx_ptp_rxfilter *rxfilter)

This is the second time I get pushback because of this. Could you
please explain the rationale of not allowing it?

I understand that the compiler probably will do the right thing with
or without the `inline`, and more being in the same translation unit.
Actually, I checked the style guide [1] and I thought it was fine like
this: it says that `inline` should not be abused, but it's fine in
cases like this one. Quotes from the guide:
  "Generally, inline functions are preferable to macros resembling functions"
  "A reasonable rule of thumb is to not put inline at functions that
have more than 3 lines of code in them"

I have the feeling that if I had made it as a macro it had been
accepted, but inline not, despite the "prefer inline over macro".

I don't mind changing it, but I'd like to understand it so I can
remember the next time. And if it's such a hard rule that it's
considered a "fail" in the patchwork checks, maybe it should be
documented somewhere.

Thanks!

[1] https://www.kernel.org/doc/html/latest/process/coding-style.html


-- 
Íñigo Huguet

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

[Leon Romanovsky]

On Thu, Feb 02, 2023 at 08:08:10AM +0100, Íñigo Huguet wrote:
> On Wed, Feb 1, 2023 at 8:05 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Wed,  1 Feb 2023 09:08:45 +0100 Íñigo Huguet wrote:
> > > v2: fixed missing IS_ERR
> > >     added doc of missing fields in efx_ptp_rxfilter
> >
> > 1. don't repost within 24h, *especially* if you're reposting
> > because of compilation problems
> >
> > https://www.kernel.org/doc/html/next/process/maintainer-netdev.html
> 
> Sorry, I wasn't aware of this.
> 
> > 2. please don't repost in a thread, it makes it harder for me
> > to maintain a review queue
> 
> What do you mean? I sent it with `git send-email --in-reply-to`, I
> thought this was the canonical way to send a v2 and superseed the
> previous version.

It was never canonical way. I'm second to Jakub, it messes review and
acceptance flow so badly that I prefer to do not take such patches due
to possible confusion.

> 
> > 3. drop the pointless inline in the source file in patch 4
> >
> > +static inline void efx_ptp_remove_one_filter(struct efx_nic *efx,
> > +                                            struct efx_ptp_rxfilter *rxfilter)
> 
> This is the second time I get pushback because of this. Could you
> please explain the rationale of not allowing it?
> 
> I understand that the compiler probably will do the right thing with
> or without the `inline`, and more being in the same translation unit.
> Actually, I checked the style guide [1] and I thought it was fine like
> this: it says that `inline` should not be abused, but it's fine in
> cases like this one. Quotes from the guide:
>   "Generally, inline functions are preferable to macros resembling functions"
>   "A reasonable rule of thumb is to not put inline at functions that
> have more than 3 lines of code in them"
> 
> I have the feeling that if I had made it as a macro it had been
> accepted, but inline not, despite the "prefer inline over macro".
> 
> I don't mind changing it, but I'd like to understand it so I can
> remember the next time. And if it's such a hard rule that it's
> considered a "fail" in the patchwork checks, maybe it should be
> documented somewhere.

GCC will automatically inline your not-inline function anyway.

Documentation/process/coding-style.rst
   958 15) The inline disease
...
   978 Often people argue that adding inline to functions that are static and used
   979 only once is always a win since there is no space tradeoff. While this is
   980 technically correct, gcc is capable of inlining these automatically without
   981 help, and the maintenance issue of removing the inline when a second user
   982 appears outweighs the potential value of the hint that tells gcc to do
   983 something it would have done anyway.


> 
> Thanks!
> 
> [1] https://www.kernel.org/doc/html/latest/process/coding-style.html
> 
> 
> -- 
> Íñigo Huguet
> 

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

[Íñigo Huguet]

On Thu, Feb 2, 2023 at 9:38 AM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Thu, Feb 02, 2023 at 08:08:10AM +0100, Íñigo Huguet wrote:
> > On Wed, Feb 1, 2023 at 8:05 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > >
> > > On Wed,  1 Feb 2023 09:08:45 +0100 Íñigo Huguet wrote:
> > > > v2: fixed missing IS_ERR
> > > >     added doc of missing fields in efx_ptp_rxfilter
> > >
> > > 1. don't repost within 24h, *especially* if you're reposting
> > > because of compilation problems
> > >
> > > https://www.kernel.org/doc/html/next/process/maintainer-netdev.html
> >
> > Sorry, I wasn't aware of this.
> >
> > > 2. please don't repost in a thread, it makes it harder for me
> > > to maintain a review queue
> >
> > What do you mean? I sent it with `git send-email --in-reply-to`, I
> > thought this was the canonical way to send a v2 and superseed the
> > previous version.
>
> It was never canonical way. I'm second to Jakub, it messes review and
> acceptance flow so badly that I prefer to do not take such patches due
> to possible confusion.

I was so sure about this that it has confused me a lot. But I've found
where my mistake came from: in the past I made a few contributions to
the Buildroot project, and there they explicitly request to do it
because they say that patchwork automatically marks the previous
version as superseded. But yes, of course you're right: in kernel's
documentation it explicitly says not to do it.

>
> >
> > > 3. drop the pointless inline in the source file in patch 4
> > >
> > > +static inline void efx_ptp_remove_one_filter(struct efx_nic *efx,
> > > +                                            struct efx_ptp_rxfilter *rxfilter)
> >
> > This is the second time I get pushback because of this. Could you
> > please explain the rationale of not allowing it?
> >
> > I understand that the compiler probably will do the right thing with
> > or without the `inline`, and more being in the same translation unit.
> > Actually, I checked the style guide [1] and I thought it was fine like
> > this: it says that `inline` should not be abused, but it's fine in
> > cases like this one. Quotes from the guide:
> >   "Generally, inline functions are preferable to macros resembling functions"
> >   "A reasonable rule of thumb is to not put inline at functions that
> > have more than 3 lines of code in them"
> >
> > I have the feeling that if I had made it as a macro it had been
> > accepted, but inline not, despite the "prefer inline over macro".
> >
> > I don't mind changing it, but I'd like to understand it so I can
> > remember the next time. And if it's such a hard rule that it's
> > considered a "fail" in the patchwork checks, maybe it should be
> > documented somewhere.
>
> GCC will automatically inline your not-inline function anyway.
>
> Documentation/process/coding-style.rst
>    958 15) The inline disease
> ...
>    978 Often people argue that adding inline to functions that are static and used
>    979 only once is always a win since there is no space tradeoff. While this is
>    980 technically correct, gcc is capable of inlining these automatically without
>    981 help, and the maintenance issue of removing the inline when a second user
>    982 appears outweighs the potential value of the hint that tells gcc to do
>    983 something it would have done anyway.

I also saw that, but this paragraph seems to talk about functions of
*any* size, for which many people think that it's good to add `inline`
if they're used *only once*. That's not this case, but instead this
case seems to fit well in the cases where the guide says that it's OK
to use them:
"Generally, inline functions are preferable to macros resembling functions"
"A reasonable rule of thumb is to not put inline at functions that
have more than 3 lines of code in them".

Just to be clear: there are a lot of discussions and opinions about
how to use inline, and some rules about its usage are needed (mainly
limiting it). What I mean is that we have some written rules, but if
there are additional rules that are being applied, maybe they should
be written too. That way we would avoid work in reviews and resends
(because I checked the coding-style regarding this topic before
sending the patch), and we the developers would understand better the
technical reasons behind it.

> > Thanks!
> >
> > [1] https://www.kernel.org/doc/html/latest/process/coding-style.html
> >
> >
> > --
> > Íñigo Huguet
> >
>


-- 
Íñigo Huguet

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

[Martin Habets]

On Wed, Feb 01, 2023 at 09:08:46AM +0100, Íñigo Huguet wrote:
> 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 | 72 ++++++++++++++++++++++------------
>  1 file changed, 46 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
> index 9f07e1ba7780..53817b4350a5 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>
> @@ -213,6 +214,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 +240,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 +309,7 @@ struct efx_ptp_data {
>  	struct workqueue_struct *workwq;
>  	struct work_struct work;
>  	bool reset_required;
> -	u32 rxfilters[PTP_RXFILTERS_LEN];

This is the onlu place PTP_RXFILTERS_LEN is used, so remove that
define in this patch.

Martin

> -	size_t rxfilters_count;
> +	struct list_head rxfilters_mcast;
>  	struct hwtstamp_config config;
>  	bool enabled;
>  	unsigned int mode;
> @@ -1292,11 +1301,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 +1322,55 @@ 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 = efx_filter_insert_filter(efx, rxfilter, true);
> +	int rc = efx_filter_insert_filter(efx, spec, true);
>  	if (rc < 0)
>  		return rc;
> -	ptp->rxfilters[ptp->rxfilters_count] = rc;
> -	ptp->rxfilters_count++;
> +
> +	rxfilter = kzalloc(sizeof(*rxfilter), GFP_KERNEL);
> +	if (!rxfilter)
> +		return -ENOMEM;
> +
> +	rxfilter->handle = rc;
> +	list_add(&rxfilter->list, &ptp->rxfilters_mcast);
> +
>  	return 0;
>  }
>  
>  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 +1378,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 +1584,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

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

[Martin Habets]

On Wed, Feb 01, 2023 at 09:08:48AM +0100, Íñ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.
> 
> Reported-by: Yalin Li <yalli@redhat.com>
> Signed-off-by: Íñigo Huguet <ihuguet@redhat.com>
> ---
>  drivers/net/ethernet/sfc/ptp.c | 105 ++++++++++++++++++++++++++++++++-
>  1 file changed, 102 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
> index 5d5f70c56048..a3e827cd84a8 100644
> --- a/drivers/net/ethernet/sfc/ptp.c
> +++ b/drivers/net/ethernet/sfc/ptp.c
> @@ -217,10 +217,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;
>  };
>  
> @@ -369,6 +375,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)
>  {
> @@ -1114,6 +1122,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
> @@ -1137,6 +1147,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);
> @@ -1148,7 +1165,7 @@ static void efx_ptp_xmit_skb_mc(struct efx_nic *efx, struct sk_buff *skb)
>  {
>  	struct efx_ptp_data *ptp_data = efx->ptp_data;
>  	struct skb_shared_hwtstamps timestamps;
> -	int rc = -EIO;
> +	int rc;
>  	MCDI_DECLARE_BUF(txtime, MC_CMD_PTP_OUT_TRANSMIT_LEN);
>  	size_t len;

While I'm grateful for this unrelated cleanup, it exposes that our
existing code does not obey the reverse Christmas convention. And
now the line with rc is also shorter than the one for len.
Can you fix this please? For me this is ok as part of this series.

Martin

>  
> @@ -1184,7 +1201,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);
> @@ -1300,6 +1320,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,8 +1363,12 @@ static int efx_ptp_insert_filter(struct efx_nic *efx,
>  				 struct efx_filter_spec *spec)
>  {
>  	struct efx_ptp_rxfilter *rxfilter;
> +	int rc;
> +
> +	if (efx_ptp_filter_exists(ptp_list, spec))
> +		return 0;
>  
> -	int rc = efx_filter_insert_filter(efx, spec, true);
> +	rc = efx_filter_insert_filter(efx, spec, true);
>  	if (rc < 0)
>  		return rc;
>  
> @@ -1338,6 +1377,9 @@ static int efx_ptp_insert_filter(struct efx_nic *efx,
>  		return -ENOMEM;
>  
>  	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;
> @@ -1426,6 +1468,63 @@ 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)->protocol == IPPROTO_UDP &&
> +			udp_hdr(skb)->source == htons(PTP_EVENT_PORT);
> +	} else if (skb->protocol == htons(ETH_P_IPV6)) {
> +		return 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

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

[Martin Habets]

On Wed, Feb 01, 2023 at 09:08:49AM +0100, Íñigo Huguet wrote:
> 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 | 121 +++++++++++++++++++++------------
>  1 file changed, 77 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
> index a3e827cd84a8..dd46ca6c070e 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)

This seems like something that should be tunable, with this as a
default value.

> +
>  /* 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.
> @@ -220,6 +223,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 {
> @@ -227,6 +231,7 @@ struct efx_ptp_rxfilter {
>  	__be16 ether_type;
>  	__be16 loc_port;
>  	__be32 loc_host[4];
> +	unsigned long expiry;
>  	int handle;
>  };
>  
> @@ -1320,8 +1325,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;
>  
> @@ -1329,10 +1334,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 inline void efx_ptp_remove_one_filter(struct efx_nic *efx,
> +					     struct efx_ptp_rxfilter *rxfilter)

As others noted, don't use inline in .c files.

> +{
> +	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,
> @@ -1341,10 +1355,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,23 +1369,24 @@ static void efx_ptp_init_filter(struct efx_nic *efx,
>  			   efx_rx_queue_index(queue));
>  }
>  
> -static int efx_ptp_insert_filter(struct efx_nic *efx,
> -				 struct list_head *ptp_list,
> -				 struct efx_filter_spec *spec)
> +static struct efx_ptp_rxfilter *
> +efx_ptp_insert_filter(struct efx_nic *efx, struct list_head *ptp_list,
> +		      struct efx_filter_spec *spec)

This API change and the following ones are all for very little gain,
they are just do set the new expiry attribute. And in the end the
pointers all get converted back to an integer return code.
In stead, just pass expiry as a new argument to efx_ptp_insert_ipv4_filter()
and efx_ptp_insert_ipv6_filter().

>  {
>  	struct efx_ptp_rxfilter *rxfilter;
>  	int rc;
>  
> -	if (efx_ptp_filter_exists(ptp_list, spec))
> -		return 0;
> +	rxfilter = efx_ptp_find_filter(ptp_list, spec);
> +	if (rxfilter)
> +		return rxfilter;
>  
>  	rc = efx_filter_insert_filter(efx, spec, true);
>  	if (rc < 0)
> -		return rc;
> +		return ERR_PTR(rc);
>  
>  	rxfilter = kzalloc(sizeof(*rxfilter), GFP_KERNEL);
>  	if (!rxfilter)
> -		return -ENOMEM;
> +		return ERR_PTR(-ENOMEM);
>  
>  	rxfilter->handle = rc;
>  	rxfilter->ether_type = spec->ether_type;
> @@ -1382,12 +1394,12 @@ static int efx_ptp_insert_filter(struct efx_nic *efx,
>  	memcpy(rxfilter->loc_host, spec->loc_host, sizeof(spec->loc_host));
>  	list_add(&rxfilter->list, ptp_list);
>  
> -	return 0;
> +	return rxfilter;
>  }
>  
> -static int efx_ptp_insert_ipv4_filter(struct efx_nic *efx,
> -				      struct list_head *ptp_list,
> -				      __be32 addr, u16 port)
> +static struct efx_ptp_rxfilter *
> +efx_ptp_insert_ipv4_filter(struct efx_nic *efx, struct list_head *ptp_list,
> +			   __be32 addr, u16 port)
>  {
>  	struct efx_filter_spec spec;
>  
> @@ -1396,9 +1408,9 @@ static int efx_ptp_insert_ipv4_filter(struct efx_nic *efx,
>  	return efx_ptp_insert_filter(efx, ptp_list, &spec);
>  }
>  
> -static int efx_ptp_insert_ipv6_filter(struct efx_nic *efx,
> -				      struct list_head *ptp_list,
> -				      struct in6_addr *addr, u16 port)
> +static struct efx_ptp_rxfilter *
> +efx_ptp_insert_ipv6_filter(struct efx_nic *efx, struct list_head *ptp_list,
> +			   struct in6_addr *addr, u16 port)
>  {
>  	struct efx_filter_spec spec;
>  
> @@ -1407,7 +1419,8 @@ static int efx_ptp_insert_ipv6_filter(struct efx_nic *efx,
>  	return efx_ptp_insert_filter(efx, ptp_list, &spec);
>  }
>  
> -static int efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx)
> +static struct efx_ptp_rxfilter *
> +efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx)
>  {
>  	const u8 addr[ETH_ALEN] = PTP_ADDR_ETHER;
>  	struct efx_filter_spec spec;
> @@ -1422,7 +1435,7 @@ static int efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx)
>  static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
>  {
>  	struct efx_ptp_data *ptp = efx->ptp_data;
> -	int rc;
> +	struct efx_ptp_rxfilter *rc;
>  
>  	if (!ptp->channel || !list_empty(&ptp->rxfilters_mcast))
>  		return 0;
> @@ -1432,12 +1445,12 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
>  	 */
>  	rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_mcast,
>  					htonl(PTP_ADDR_IPV4), PTP_EVENT_PORT);
> -	if (rc < 0)
> +	if (IS_ERR(rc))
>  		goto fail;
>  
>  	rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_mcast,
>  					htonl(PTP_ADDR_IPV4), PTP_GENERAL_PORT);
> -	if (rc < 0)
> +	if (IS_ERR(rc))
>  		goto fail;
>  
>  	/* if the NIC supports hw timestamps by the MAC, we can support
> @@ -1448,16 +1461,16 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
>  
>  		rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_mcast,
>  						&ipv6_addr, PTP_EVENT_PORT);
> -		if (rc < 0)
> +		if (IS_ERR(rc))
>  			goto fail;
>  
>  		rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_mcast,
>  						&ipv6_addr, PTP_GENERAL_PORT);
> -		if (rc < 0)
> +		if (IS_ERR(rc))
>  			goto fail;
>  
>  		rc = efx_ptp_insert_eth_multicast_filter(efx);
> -		if (rc < 0)
> +		if (IS_ERR(rc))
>  			goto fail;
>  	}
>  
> @@ -1465,7 +1478,7 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
>  
>  fail:
>  	efx_ptp_remove_filters(efx, &ptp->rxfilters_mcast);
> -	return rc;
> +	return PTR_ERR(rc);
>  }
>  
>  static bool efx_ptp_valid_unicast_event_pkt(struct sk_buff *skb)
> @@ -1484,7 +1497,7 @@ 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;
> +	struct efx_ptp_rxfilter *rxfilter;
>  
>  	if (!efx_ptp_valid_unicast_event_pkt(skb))
>  		return -EINVAL;
> @@ -1492,28 +1505,36 @@ static int efx_ptp_insert_unicast_filter(struct efx_nic *efx,
>  	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)
> +		rxfilter = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast,
> +						      addr, PTP_EVENT_PORT);
> +		if (IS_ERR(rxfilter))
>  			goto fail;
>  
> -		rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast,
> -						addr, PTP_GENERAL_PORT);
> -		if (rc < 0)
> +		rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
> +
> +		rxfilter = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast,
> +						      addr, PTP_GENERAL_PORT);
> +		if (IS_ERR(rxfilter))
>  			goto fail;
> +
> +		rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
>  	} 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)
> +		rxfilter = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
> +						      addr, PTP_EVENT_PORT);
> +		if (IS_ERR(rxfilter))
>  			goto fail;
>  
> -		rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
> -						addr, PTP_GENERAL_PORT);
> -		if (rc < 0)
> +		rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
> +
> +		rxfilter = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
> +						      addr, PTP_GENERAL_PORT);
> +		if (IS_ERR(rxfilter))
>  			goto fail;
> +
> +		rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
>  	} else {
>  		return -EOPNOTSUPP;
>  	}
> @@ -1522,7 +1543,18 @@ static int efx_ptp_insert_unicast_filter(struct efx_nic *efx,
>  
>  fail:
>  	efx_ptp_remove_filters(efx, &ptp->rxfilters_ucast);
> -	return rc;
> +	return PTR_ERR(rxfilter);
> +}
> +
> +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))

Shouldn't this be time_is_after_jiffies?

> +			efx_ptp_remove_one_filter(efx, rxfilter);
> +	}
>  }
>  
>  static int efx_ptp_start(struct efx_nic *efx)
> @@ -1616,6 +1648,7 @@ static void efx_ptp_worker(struct work_struct *work)
>  	}
>  
>  	efx_ptp_drop_time_expired_events(efx);
> +	efx_ptp_drop_expired_unicast_filters(efx);

This grabs locks in efx_mcdi_filter_remove_safe(), which is bad because
that will delay processing that is done below. So do this at the end of
the function.

Martin

>  
>  	__skb_queue_head_init(&tempq);
>  	efx_ptp_process_events(efx, &tempq);
> -- 
> 2.34.3

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

[Íñigo Huguet]

On Thu, Feb 2, 2023 at 3:12 PM Martin Habets <habetsm.xilinx@gmail.com> wrote:
>
> On Wed, Feb 01, 2023 at 09:08:49AM +0100, Íñigo Huguet wrote:
> > 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 | 121 +++++++++++++++++++++------------
> >  1 file changed, 77 insertions(+), 44 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
> > index a3e827cd84a8..dd46ca6c070e 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)
>
> This seems like something that should be tunable, with this as a
> default value.
>
> > +
> >  /* 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.
> > @@ -220,6 +223,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 {
> > @@ -227,6 +231,7 @@ struct efx_ptp_rxfilter {
> >       __be16 ether_type;
> >       __be16 loc_port;
> >       __be32 loc_host[4];
> > +     unsigned long expiry;
> >       int handle;
> >  };
> >
> > @@ -1320,8 +1325,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;
> >
> > @@ -1329,10 +1334,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 inline void efx_ptp_remove_one_filter(struct efx_nic *efx,
> > +                                          struct efx_ptp_rxfilter *rxfilter)
>
> As others noted, don't use inline in .c files.
>
> > +{
> > +     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,
> > @@ -1341,10 +1355,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,23 +1369,24 @@ static void efx_ptp_init_filter(struct efx_nic *efx,
> >                          efx_rx_queue_index(queue));
> >  }
> >
> > -static int efx_ptp_insert_filter(struct efx_nic *efx,
> > -                              struct list_head *ptp_list,
> > -                              struct efx_filter_spec *spec)
> > +static struct efx_ptp_rxfilter *
> > +efx_ptp_insert_filter(struct efx_nic *efx, struct list_head *ptp_list,
> > +                   struct efx_filter_spec *spec)
>
> This API change and the following ones are all for very little gain,
> they are just do set the new expiry attribute. And in the end the
> pointers all get converted back to an integer return code.
> In stead, just pass expiry as a new argument to efx_ptp_insert_ipv4_filter()
> and efx_ptp_insert_ipv6_filter().

I wanted to save computation time when inserting multicast filters,
but given that it's done only once, it's a bit pointless. I also
wanted to show clearly in the code that only unicast filters care
about expiration, but maybe just passing a constant 0 to your expiry
argument clearly shows that multicast doesn't care about it.

I also had the feeling that I was overcomplicating the API for little
gain, I like your approach more.

Also, reviewing this has made me to realize that I'm inserting
pointless unicast filters when using multicast PTP. Probably I should
add a check `ip_hdr(skb)->daddr != htnonl(PTP_ADDR_IPV4)` to
efx_ptp_valid_unicast_event_pkt (and the equivalent for IPv6).

>
> >  {
> >       struct efx_ptp_rxfilter *rxfilter;
> >       int rc;
> >
> > -     if (efx_ptp_filter_exists(ptp_list, spec))
> > -             return 0;
> > +     rxfilter = efx_ptp_find_filter(ptp_list, spec);
> > +     if (rxfilter)
> > +             return rxfilter;
> >
> >       rc = efx_filter_insert_filter(efx, spec, true);
> >       if (rc < 0)
> > -             return rc;
> > +             return ERR_PTR(rc);
> >
> >       rxfilter = kzalloc(sizeof(*rxfilter), GFP_KERNEL);
> >       if (!rxfilter)
> > -             return -ENOMEM;
> > +             return ERR_PTR(-ENOMEM);
> >
> >       rxfilter->handle = rc;
> >       rxfilter->ether_type = spec->ether_type;
> > @@ -1382,12 +1394,12 @@ static int efx_ptp_insert_filter(struct efx_nic *efx,
> >       memcpy(rxfilter->loc_host, spec->loc_host, sizeof(spec->loc_host));
> >       list_add(&rxfilter->list, ptp_list);
> >
> > -     return 0;
> > +     return rxfilter;
> >  }
> >
> > -static int efx_ptp_insert_ipv4_filter(struct efx_nic *efx,
> > -                                   struct list_head *ptp_list,
> > -                                   __be32 addr, u16 port)
> > +static struct efx_ptp_rxfilter *
> > +efx_ptp_insert_ipv4_filter(struct efx_nic *efx, struct list_head *ptp_list,
> > +                        __be32 addr, u16 port)
> >  {
> >       struct efx_filter_spec spec;
> >
> > @@ -1396,9 +1408,9 @@ static int efx_ptp_insert_ipv4_filter(struct efx_nic *efx,
> >       return efx_ptp_insert_filter(efx, ptp_list, &spec);
> >  }
> >
> > -static int efx_ptp_insert_ipv6_filter(struct efx_nic *efx,
> > -                                   struct list_head *ptp_list,
> > -                                   struct in6_addr *addr, u16 port)
> > +static struct efx_ptp_rxfilter *
> > +efx_ptp_insert_ipv6_filter(struct efx_nic *efx, struct list_head *ptp_list,
> > +                        struct in6_addr *addr, u16 port)
> >  {
> >       struct efx_filter_spec spec;
> >
> > @@ -1407,7 +1419,8 @@ static int efx_ptp_insert_ipv6_filter(struct efx_nic *efx,
> >       return efx_ptp_insert_filter(efx, ptp_list, &spec);
> >  }
> >
> > -static int efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx)
> > +static struct efx_ptp_rxfilter *
> > +efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx)
> >  {
> >       const u8 addr[ETH_ALEN] = PTP_ADDR_ETHER;
> >       struct efx_filter_spec spec;
> > @@ -1422,7 +1435,7 @@ static int efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx)
> >  static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
> >  {
> >       struct efx_ptp_data *ptp = efx->ptp_data;
> > -     int rc;
> > +     struct efx_ptp_rxfilter *rc;
> >
> >       if (!ptp->channel || !list_empty(&ptp->rxfilters_mcast))
> >               return 0;
> > @@ -1432,12 +1445,12 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
> >        */
> >       rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_mcast,
> >                                       htonl(PTP_ADDR_IPV4), PTP_EVENT_PORT);
> > -     if (rc < 0)
> > +     if (IS_ERR(rc))
> >               goto fail;
> >
> >       rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_mcast,
> >                                       htonl(PTP_ADDR_IPV4), PTP_GENERAL_PORT);
> > -     if (rc < 0)
> > +     if (IS_ERR(rc))
> >               goto fail;
> >
> >       /* if the NIC supports hw timestamps by the MAC, we can support
> > @@ -1448,16 +1461,16 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
> >
> >               rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_mcast,
> >                                               &ipv6_addr, PTP_EVENT_PORT);
> > -             if (rc < 0)
> > +             if (IS_ERR(rc))
> >                       goto fail;
> >
> >               rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_mcast,
> >                                               &ipv6_addr, PTP_GENERAL_PORT);
> > -             if (rc < 0)
> > +             if (IS_ERR(rc))
> >                       goto fail;
> >
> >               rc = efx_ptp_insert_eth_multicast_filter(efx);
> > -             if (rc < 0)
> > +             if (IS_ERR(rc))
> >                       goto fail;
> >       }
> >
> > @@ -1465,7 +1478,7 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
> >
> >  fail:
> >       efx_ptp_remove_filters(efx, &ptp->rxfilters_mcast);
> > -     return rc;
> > +     return PTR_ERR(rc);
> >  }
> >
> >  static bool efx_ptp_valid_unicast_event_pkt(struct sk_buff *skb)
> > @@ -1484,7 +1497,7 @@ 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;
> > +     struct efx_ptp_rxfilter *rxfilter;
> >
> >       if (!efx_ptp_valid_unicast_event_pkt(skb))
> >               return -EINVAL;
> > @@ -1492,28 +1505,36 @@ static int efx_ptp_insert_unicast_filter(struct efx_nic *efx,
> >       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)
> > +             rxfilter = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast,
> > +                                                   addr, PTP_EVENT_PORT);
> > +             if (IS_ERR(rxfilter))
> >                       goto fail;
> >
> > -             rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast,
> > -                                             addr, PTP_GENERAL_PORT);
> > -             if (rc < 0)
> > +             rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
> > +
> > +             rxfilter = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast,
> > +                                                   addr, PTP_GENERAL_PORT);
> > +             if (IS_ERR(rxfilter))
> >                       goto fail;
> > +
> > +             rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
> >       } 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)
> > +             rxfilter = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
> > +                                                   addr, PTP_EVENT_PORT);
> > +             if (IS_ERR(rxfilter))
> >                       goto fail;
> >
> > -             rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
> > -                                             addr, PTP_GENERAL_PORT);
> > -             if (rc < 0)
> > +             rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
> > +
> > +             rxfilter = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
> > +                                                   addr, PTP_GENERAL_PORT);
> > +             if (IS_ERR(rxfilter))
> >                       goto fail;
> > +
> > +             rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
> >       } else {
> >               return -EOPNOTSUPP;
> >       }
> > @@ -1522,7 +1543,18 @@ static int efx_ptp_insert_unicast_filter(struct efx_nic *efx,
> >
> >  fail:
> >       efx_ptp_remove_filters(efx, &ptp->rxfilters_ucast);
> > -     return rc;
> > +     return PTR_ERR(rxfilter);
> > +}
> > +
> > +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))
>
> Shouldn't this be time_is_after_jiffies?
>
> > +                     efx_ptp_remove_one_filter(efx, rxfilter);
> > +     }
> >  }
> >
> >  static int efx_ptp_start(struct efx_nic *efx)
> > @@ -1616,6 +1648,7 @@ static void efx_ptp_worker(struct work_struct *work)
> >       }
> >
> >       efx_ptp_drop_time_expired_events(efx);
> > +     efx_ptp_drop_expired_unicast_filters(efx);
>
> This grabs locks in efx_mcdi_filter_remove_safe(), which is bad because
> that will delay processing that is done below. So do this at the end of
> the function.
>
> Martin
>
> >
> >       __skb_queue_head_init(&tempq);
> >       efx_ptp_process_events(efx, &tempq);
> > --
> > 2.34.3
>


-- 
Íñigo Huguet

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

[Íñigo Huguet]

On Thu, Feb 2, 2023 at 3:12 PM Martin Habets <habetsm.xilinx@gmail.com> wrote:
>
> On Wed, Feb 01, 2023 at 09:08:49AM +0100, Íñigo Huguet wrote:
> > 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 | 121 +++++++++++++++++++++------------
> >  1 file changed, 77 insertions(+), 44 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
> > index a3e827cd84a8..dd46ca6c070e 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)
>
> This seems like something that should be tunable, with this as a
> default value.

Not sure how valuable it would be. The filter is not going to expire
while it's in use, and it will expire 30s (or whatever) after last
usage. If it expires, if used again it's reinserted, with only the
penalty of a MCDI call.

I wouldn't make it tunable, but if you insist, how would you do it?
modparam? Other?

> > +
> >  /* 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.
> > @@ -220,6 +223,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 {
> > @@ -227,6 +231,7 @@ struct efx_ptp_rxfilter {
> >       __be16 ether_type;
> >       __be16 loc_port;
> >       __be32 loc_host[4];
> > +     unsigned long expiry;
> >       int handle;
> >  };
> >
> > @@ -1320,8 +1325,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;
> >
> > @@ -1329,10 +1334,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 inline void efx_ptp_remove_one_filter(struct efx_nic *efx,
> > +                                          struct efx_ptp_rxfilter *rxfilter)
>
> As others noted, don't use inline in .c files.
>
> > +{
> > +     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,
> > @@ -1341,10 +1355,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,23 +1369,24 @@ static void efx_ptp_init_filter(struct efx_nic *efx,
> >                          efx_rx_queue_index(queue));
> >  }
> >
> > -static int efx_ptp_insert_filter(struct efx_nic *efx,
> > -                              struct list_head *ptp_list,
> > -                              struct efx_filter_spec *spec)
> > +static struct efx_ptp_rxfilter *
> > +efx_ptp_insert_filter(struct efx_nic *efx, struct list_head *ptp_list,
> > +                   struct efx_filter_spec *spec)
>
> This API change and the following ones are all for very little gain,
> they are just do set the new expiry attribute. And in the end the
> pointers all get converted back to an integer return code.
> In stead, just pass expiry as a new argument to efx_ptp_insert_ipv4_filter()
> and efx_ptp_insert_ipv6_filter().
>
> >  {
> >       struct efx_ptp_rxfilter *rxfilter;
> >       int rc;
> >
> > -     if (efx_ptp_filter_exists(ptp_list, spec))
> > -             return 0;
> > +     rxfilter = efx_ptp_find_filter(ptp_list, spec);
> > +     if (rxfilter)
> > +             return rxfilter;
> >
> >       rc = efx_filter_insert_filter(efx, spec, true);
> >       if (rc < 0)
> > -             return rc;
> > +             return ERR_PTR(rc);
> >
> >       rxfilter = kzalloc(sizeof(*rxfilter), GFP_KERNEL);
> >       if (!rxfilter)
> > -             return -ENOMEM;
> > +             return ERR_PTR(-ENOMEM);
> >
> >       rxfilter->handle = rc;
> >       rxfilter->ether_type = spec->ether_type;
> > @@ -1382,12 +1394,12 @@ static int efx_ptp_insert_filter(struct efx_nic *efx,
> >       memcpy(rxfilter->loc_host, spec->loc_host, sizeof(spec->loc_host));
> >       list_add(&rxfilter->list, ptp_list);
> >
> > -     return 0;
> > +     return rxfilter;
> >  }
> >
> > -static int efx_ptp_insert_ipv4_filter(struct efx_nic *efx,
> > -                                   struct list_head *ptp_list,
> > -                                   __be32 addr, u16 port)
> > +static struct efx_ptp_rxfilter *
> > +efx_ptp_insert_ipv4_filter(struct efx_nic *efx, struct list_head *ptp_list,
> > +                        __be32 addr, u16 port)
> >  {
> >       struct efx_filter_spec spec;
> >
> > @@ -1396,9 +1408,9 @@ static int efx_ptp_insert_ipv4_filter(struct efx_nic *efx,
> >       return efx_ptp_insert_filter(efx, ptp_list, &spec);
> >  }
> >
> > -static int efx_ptp_insert_ipv6_filter(struct efx_nic *efx,
> > -                                   struct list_head *ptp_list,
> > -                                   struct in6_addr *addr, u16 port)
> > +static struct efx_ptp_rxfilter *
> > +efx_ptp_insert_ipv6_filter(struct efx_nic *efx, struct list_head *ptp_list,
> > +                        struct in6_addr *addr, u16 port)
> >  {
> >       struct efx_filter_spec spec;
> >
> > @@ -1407,7 +1419,8 @@ static int efx_ptp_insert_ipv6_filter(struct efx_nic *efx,
> >       return efx_ptp_insert_filter(efx, ptp_list, &spec);
> >  }
> >
> > -static int efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx)
> > +static struct efx_ptp_rxfilter *
> > +efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx)
> >  {
> >       const u8 addr[ETH_ALEN] = PTP_ADDR_ETHER;
> >       struct efx_filter_spec spec;
> > @@ -1422,7 +1435,7 @@ static int efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx)
> >  static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
> >  {
> >       struct efx_ptp_data *ptp = efx->ptp_data;
> > -     int rc;
> > +     struct efx_ptp_rxfilter *rc;
> >
> >       if (!ptp->channel || !list_empty(&ptp->rxfilters_mcast))
> >               return 0;
> > @@ -1432,12 +1445,12 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
> >        */
> >       rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_mcast,
> >                                       htonl(PTP_ADDR_IPV4), PTP_EVENT_PORT);
> > -     if (rc < 0)
> > +     if (IS_ERR(rc))
> >               goto fail;
> >
> >       rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_mcast,
> >                                       htonl(PTP_ADDR_IPV4), PTP_GENERAL_PORT);
> > -     if (rc < 0)
> > +     if (IS_ERR(rc))
> >               goto fail;
> >
> >       /* if the NIC supports hw timestamps by the MAC, we can support
> > @@ -1448,16 +1461,16 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
> >
> >               rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_mcast,
> >                                               &ipv6_addr, PTP_EVENT_PORT);
> > -             if (rc < 0)
> > +             if (IS_ERR(rc))
> >                       goto fail;
> >
> >               rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_mcast,
> >                                               &ipv6_addr, PTP_GENERAL_PORT);
> > -             if (rc < 0)
> > +             if (IS_ERR(rc))
> >                       goto fail;
> >
> >               rc = efx_ptp_insert_eth_multicast_filter(efx);
> > -             if (rc < 0)
> > +             if (IS_ERR(rc))
> >                       goto fail;
> >       }
> >
> > @@ -1465,7 +1478,7 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
> >
> >  fail:
> >       efx_ptp_remove_filters(efx, &ptp->rxfilters_mcast);
> > -     return rc;
> > +     return PTR_ERR(rc);
> >  }
> >
> >  static bool efx_ptp_valid_unicast_event_pkt(struct sk_buff *skb)
> > @@ -1484,7 +1497,7 @@ 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;
> > +     struct efx_ptp_rxfilter *rxfilter;
> >
> >       if (!efx_ptp_valid_unicast_event_pkt(skb))
> >               return -EINVAL;
> > @@ -1492,28 +1505,36 @@ static int efx_ptp_insert_unicast_filter(struct efx_nic *efx,
> >       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)
> > +             rxfilter = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast,
> > +                                                   addr, PTP_EVENT_PORT);
> > +             if (IS_ERR(rxfilter))
> >                       goto fail;
> >
> > -             rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast,
> > -                                             addr, PTP_GENERAL_PORT);
> > -             if (rc < 0)
> > +             rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
> > +
> > +             rxfilter = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast,
> > +                                                   addr, PTP_GENERAL_PORT);
> > +             if (IS_ERR(rxfilter))
> >                       goto fail;
> > +
> > +             rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
> >       } 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)
> > +             rxfilter = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
> > +                                                   addr, PTP_EVENT_PORT);
> > +             if (IS_ERR(rxfilter))
> >                       goto fail;
> >
> > -             rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
> > -                                             addr, PTP_GENERAL_PORT);
> > -             if (rc < 0)
> > +             rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
> > +
> > +             rxfilter = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
> > +                                                   addr, PTP_GENERAL_PORT);
> > +             if (IS_ERR(rxfilter))
> >                       goto fail;
> > +
> > +             rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
> >       } else {
> >               return -EOPNOTSUPP;
> >       }
> > @@ -1522,7 +1543,18 @@ static int efx_ptp_insert_unicast_filter(struct efx_nic *efx,
> >
> >  fail:
> >       efx_ptp_remove_filters(efx, &ptp->rxfilters_ucast);
> > -     return rc;
> > +     return PTR_ERR(rxfilter);
> > +}
> > +
> > +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))
>
> Shouldn't this be time_is_after_jiffies?

No, if time_is_before_jiffies means that jiffies are higher than the
value of expiry, so filter needs to be removed.

>
> > +                     efx_ptp_remove_one_filter(efx, rxfilter);
> > +     }
> >  }
> >
> >  static int efx_ptp_start(struct efx_nic *efx)
> > @@ -1616,6 +1648,7 @@ static void efx_ptp_worker(struct work_struct *work)
> >       }
> >
> >       efx_ptp_drop_time_expired_events(efx);
> > +     efx_ptp_drop_expired_unicast_filters(efx);
>
> This grabs locks in efx_mcdi_filter_remove_safe(), which is bad because
> that will delay processing that is done below. So do this at the end of
> the function.
>
> Martin
>
> >
> >       __skb_queue_head_init(&tempq);
> >       efx_ptp_process_events(efx, &tempq);
> > --
> > 2.34.3
>


-- 
Íñigo Huguet