diff mbox series

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

Message ID 20230221125217.20775-4-ihuguet@redhat.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series sfc: support unicast PTP | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning WARNING: line length of 88 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Íñigo Huguet Feb. 21, 2023, 12:52 p.m. UTC 
When sending a PTP event packet, add the correct filters that will make
that future incoming unicast PTP event packets will be timestamped.
The unicast address for the filter is gotten from the outgoing skb
before sending it.

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

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

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

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

--
2.34.3

Comments

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

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

> +		if (rc < 0)
> +			goto fail;
> +	} else if (efx_ptp_use_mac_tx_timestamps(efx)) {
> +		/* IPv6 PTP only supported by devices with MAC hw timestamp */
> +		struct in6_addr *addr = &ipv6_hdr(skb)->saddr;
> +
> +		rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
> +						addr, PTP_EVENT_PORT);
> +		if (rc < 0)
> +			goto fail;
> +
> +		rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
> +						addr, PTP_GENERAL_PORT);
> +		if (rc < 0)
> +			goto fail;
> +	} else {
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +
> +fail:
> +	efx_ptp_remove_filters(efx, &ptp->rxfilters_ucast);
> +	return rc;
> +}
> +
>   static int efx_ptp_start(struct efx_nic *efx)
>   {
>   	struct efx_ptp_data *ptp = efx->ptp_data;
> --
> 2.34.3
>
Íñigo Huguet Feb. 22, 2023, 2:41 p.m. UTC | #2 
On Wed, Feb 22, 2023 at 3:22 PM Vadim Fedorenko
<vadim.fedorenko@linux.dev> wrote:
> Not sure why do you want general packets to go to the queue for
> timestamping? There is no need for timestamp them in the protocol.
> The same question is for multicast version.

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

So the reason is not that we want the timestamp, but we want those
packets to go to the same RX queue. As a side effect, they will be
timestamped, yes, but as far as I know, there is no way to avoid that.
Richard Cochran Feb. 22, 2023, 4:52 p.m. UTC | #3 
On Wed, Feb 22, 2023 at 03:41:51PM +0100, Íñigo Huguet wrote:

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

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

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

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

That's good to know, thanks.

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

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

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

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

True, but they usually are.

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

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

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

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

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

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

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

Thanks,
Richard
diff mbox series

Patch

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

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

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

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

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

 	skb_tstamp_tx(skb, &timestamps);

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

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

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

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

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

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

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