1. Patchew
  2. linux
  3. View series

[PATCH net] net: lan966x: Fix 1-step timestamping over ipv4 or ipv6

Horatiu Vultur posted 1 patch 6 months, 3 weeks ago 
.../ethernet/microchip/lan966x/lan966x_main.c |  6 +++
.../ethernet/microchip/lan966x/lan966x_main.h |  5 ++
.../ethernet/microchip/lan966x/lan966x_ptp.c  | 49 ++++++++++++++-----
3 files changed, 47 insertions(+), 13 deletions(-)
[PATCH net] net: lan966x: Fix 1-step timestamping over ipv4 or ipv6
Posted by Horatiu Vultur 6 months, 3 weeks ago 
When enabling 1-step timestamping for ptp frames that are over udpv4 or
udpv6 then the inserted timestamp is added at the wrong offset in the
frame, meaning that will modify the frame at the wrong place, so the
frame will be malformed.
To fix this, the HW needs to know which kind of frame it is to know
where to insert the timestamp. For that there is a field in the IFH that
says the PDU_TYPE, which can be NONE  which is the default value,
IPV4 or IPV6. Therefore make sure to set the PDU_TYPE so the HW knows
where to insert the timestamp.
Like I mention before the issue is not seen with L2 frames because by
default the PDU_TYPE has a value of 0, which represents the L2 frames.

Fixes: 77eecf25bd9d2f ("net: lan966x: Update extraction/injection for timestamping")
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 .../ethernet/microchip/lan966x/lan966x_main.c |  6 +++
 .../ethernet/microchip/lan966x/lan966x_main.h |  5 ++
 .../ethernet/microchip/lan966x/lan966x_ptp.c  | 49 ++++++++++++++-----
 3 files changed, 47 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
index 0af143ec0f869..427bdc0e4908c 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
@@ -353,6 +353,11 @@ static void lan966x_ifh_set_rew_op(void *ifh, u64 rew_op)
 	lan966x_ifh_set(ifh, rew_op, IFH_POS_REW_CMD, IFH_WID_REW_CMD);
 }
 
+static void lan966x_ifh_set_oam_type(void *ifh, u64 oam_type)
+{
+	lan966x_ifh_set(ifh, oam_type, IFH_POS_PDU_TYPE, IFH_WID_PDU_TYPE);
+}
+
 static void lan966x_ifh_set_timestamp(void *ifh, u64 timestamp)
 {
 	lan966x_ifh_set(ifh, timestamp, IFH_POS_TIMESTAMP, IFH_WID_TIMESTAMP);
@@ -380,6 +385,7 @@ static netdev_tx_t lan966x_port_xmit(struct sk_buff *skb,
 			return err;
 
 		lan966x_ifh_set_rew_op(ifh, LAN966X_SKB_CB(skb)->rew_op);
+		lan966x_ifh_set_oam_type(ifh, LAN966X_SKB_CB(skb)->pdu_type);
 		lan966x_ifh_set_timestamp(ifh, LAN966X_SKB_CB(skb)->ts_id);
 	}
 
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
index 1efa584e71077..1f9df67f05044 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
@@ -75,6 +75,10 @@
 #define IFH_REW_OP_ONE_STEP_PTP		0x3
 #define IFH_REW_OP_TWO_STEP_PTP		0x4
 
+#define IFH_PDU_TYPE_NONE		0
+#define IFH_PDU_TYPE_IPV4		7
+#define IFH_PDU_TYPE_IPV6		8
+
 #define FDMA_RX_DCB_MAX_DBS		1
 #define FDMA_TX_DCB_MAX_DBS		1
 
@@ -254,6 +258,7 @@ struct lan966x_phc {
 
 struct lan966x_skb_cb {
 	u8 rew_op;
+	u8 pdu_type;
 	u16 ts_id;
 	unsigned long jiffies;
 };
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c b/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
index 63905bb5a63a8..87e5e81d40dc6 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
@@ -322,34 +322,55 @@ void lan966x_ptp_hwtstamp_get(struct lan966x_port *port,
 	*cfg = phc->hwtstamp_config;
 }
 
-static int lan966x_ptp_classify(struct lan966x_port *port, struct sk_buff *skb)
+static void lan966x_ptp_classify(struct lan966x_port *port, struct sk_buff *skb,
+				 u8 *rew_op, u8 *pdu_type)
 {
 	struct ptp_header *header;
 	u8 msgtype;
 	int type;
 
-	if (port->ptp_tx_cmd == IFH_REW_OP_NOOP)
-		return IFH_REW_OP_NOOP;
+	if (port->ptp_tx_cmd == IFH_REW_OP_NOOP) {
+		*rew_op = IFH_REW_OP_NOOP;
+		*pdu_type = IFH_PDU_TYPE_NONE;
+		return;
+	}
 
 	type = ptp_classify_raw(skb);
-	if (type == PTP_CLASS_NONE)
-		return IFH_REW_OP_NOOP;
+	if (type == PTP_CLASS_NONE) {
+		*rew_op = IFH_REW_OP_NOOP;
+		*pdu_type = IFH_PDU_TYPE_NONE;
+		return;
+	}
 
 	header = ptp_parse_header(skb, type);
-	if (!header)
-		return IFH_REW_OP_NOOP;
+	if (!header) {
+		*rew_op = IFH_REW_OP_NOOP;
+		*pdu_type = IFH_PDU_TYPE_NONE;
+		return;
+	}
 
-	if (port->ptp_tx_cmd == IFH_REW_OP_TWO_STEP_PTP)
-		return IFH_REW_OP_TWO_STEP_PTP;
+	if (type & PTP_CLASS_L2)
+		*pdu_type = IFH_PDU_TYPE_NONE;
+	if (type & PTP_CLASS_IPV4)
+		*pdu_type = IFH_PDU_TYPE_IPV4;
+	if (type & PTP_CLASS_IPV6)
+		*pdu_type = IFH_PDU_TYPE_IPV6;
+
+	if (port->ptp_tx_cmd == IFH_REW_OP_TWO_STEP_PTP) {
+		*rew_op = IFH_REW_OP_TWO_STEP_PTP;
+		return;
+	}
 
 	/* If it is sync and run 1 step then set the correct operation,
 	 * otherwise run as 2 step
 	 */
 	msgtype = ptp_get_msgtype(header, type);
-	if ((msgtype & 0xf) == 0)
-		return IFH_REW_OP_ONE_STEP_PTP;
+	if ((msgtype & 0xf) == 0) {
+		*rew_op = IFH_REW_OP_ONE_STEP_PTP;
+		return;
+	}
 
-	return IFH_REW_OP_TWO_STEP_PTP;
+	*rew_op = IFH_REW_OP_TWO_STEP_PTP;
 }
 
 static void lan966x_ptp_txtstamp_old_release(struct lan966x_port *port)
@@ -374,10 +395,12 @@ int lan966x_ptp_txtstamp_request(struct lan966x_port *port,
 {
 	struct lan966x *lan966x = port->lan966x;
 	unsigned long flags;
+	u8 pdu_type;
 	u8 rew_op;
 
-	rew_op = lan966x_ptp_classify(port, skb);
+	lan966x_ptp_classify(port, skb, &rew_op, &pdu_type);
 	LAN966X_SKB_CB(skb)->rew_op = rew_op;
+	LAN966X_SKB_CB(skb)->pdu_type = pdu_type;
 
 	if (rew_op != IFH_REW_OP_TWO_STEP_PTP)
 		return 0;
-- 
2.34.1
Re: [PATCH net] net: lan966x: Fix 1-step timestamping over ipv4 or ipv6
Posted by Vadim Fedorenko 6 months, 3 weeks ago 
On 21/05/2025 13:41, Horatiu Vultur wrote:
> When enabling 1-step timestamping for ptp frames that are over udpv4 or
> udpv6 then the inserted timestamp is added at the wrong offset in the
> frame, meaning that will modify the frame at the wrong place, so the
> frame will be malformed.
> To fix this, the HW needs to know which kind of frame it is to know
> where to insert the timestamp. For that there is a field in the IFH that
> says the PDU_TYPE, which can be NONE  which is the default value,
> IPV4 or IPV6. Therefore make sure to set the PDU_TYPE so the HW knows
> where to insert the timestamp.
> Like I mention before the issue is not seen with L2 frames because by
> default the PDU_TYPE has a value of 0, which represents the L2 frames.
> 
> Fixes: 77eecf25bd9d2f ("net: lan966x: Update extraction/injection for timestamping")
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>   .../ethernet/microchip/lan966x/lan966x_main.c |  6 +++
>   .../ethernet/microchip/lan966x/lan966x_main.h |  5 ++
>   .../ethernet/microchip/lan966x/lan966x_ptp.c  | 49 ++++++++++++++-----
>   3 files changed, 47 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> index 0af143ec0f869..427bdc0e4908c 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> @@ -353,6 +353,11 @@ static void lan966x_ifh_set_rew_op(void *ifh, u64 rew_op)
>   	lan966x_ifh_set(ifh, rew_op, IFH_POS_REW_CMD, IFH_WID_REW_CMD);
>   }
>   
> +static void lan966x_ifh_set_oam_type(void *ifh, u64 oam_type)
> +{
> +	lan966x_ifh_set(ifh, oam_type, IFH_POS_PDU_TYPE, IFH_WID_PDU_TYPE);
> +}
> +
>   static void lan966x_ifh_set_timestamp(void *ifh, u64 timestamp)
>   {
>   	lan966x_ifh_set(ifh, timestamp, IFH_POS_TIMESTAMP, IFH_WID_TIMESTAMP);
> @@ -380,6 +385,7 @@ static netdev_tx_t lan966x_port_xmit(struct sk_buff *skb,
>   			return err;
>   
>   		lan966x_ifh_set_rew_op(ifh, LAN966X_SKB_CB(skb)->rew_op);
> +		lan966x_ifh_set_oam_type(ifh, LAN966X_SKB_CB(skb)->pdu_type);
>   		lan966x_ifh_set_timestamp(ifh, LAN966X_SKB_CB(skb)->ts_id);
>   	}
>   
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> index 1efa584e71077..1f9df67f05044 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> @@ -75,6 +75,10 @@
>   #define IFH_REW_OP_ONE_STEP_PTP		0x3
>   #define IFH_REW_OP_TWO_STEP_PTP		0x4
>   
> +#define IFH_PDU_TYPE_NONE		0
> +#define IFH_PDU_TYPE_IPV4		7
> +#define IFH_PDU_TYPE_IPV6		8
> +
>   #define FDMA_RX_DCB_MAX_DBS		1
>   #define FDMA_TX_DCB_MAX_DBS		1
>   
> @@ -254,6 +258,7 @@ struct lan966x_phc {
>   
>   struct lan966x_skb_cb {
>   	u8 rew_op;
> +	u8 pdu_type;
>   	u16 ts_id;
>   	unsigned long jiffies;
>   };
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c b/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
> index 63905bb5a63a8..87e5e81d40dc6 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
> @@ -322,34 +322,55 @@ void lan966x_ptp_hwtstamp_get(struct lan966x_port *port,
>   	*cfg = phc->hwtstamp_config;
>   }
>   
> -static int lan966x_ptp_classify(struct lan966x_port *port, struct sk_buff *skb)
> +static void lan966x_ptp_classify(struct lan966x_port *port, struct sk_buff *skb,
> +				 u8 *rew_op, u8 *pdu_type)
>   {
>   	struct ptp_header *header;
>   	u8 msgtype;
>   	int type;
>   
> -	if (port->ptp_tx_cmd == IFH_REW_OP_NOOP)
> -		return IFH_REW_OP_NOOP;
> +	if (port->ptp_tx_cmd == IFH_REW_OP_NOOP) {
> +		*rew_op = IFH_REW_OP_NOOP;
> +		*pdu_type = IFH_PDU_TYPE_NONE;
> +		return;
> +	}
>   
>   	type = ptp_classify_raw(skb);
> -	if (type == PTP_CLASS_NONE)
> -		return IFH_REW_OP_NOOP;
> +	if (type == PTP_CLASS_NONE) {
> +		*rew_op = IFH_REW_OP_NOOP;
> +		*pdu_type = IFH_PDU_TYPE_NONE;
> +		return;
> +	}
>   
>   	header = ptp_parse_header(skb, type);
> -	if (!header)
> -		return IFH_REW_OP_NOOP;
> +	if (!header) {
> +		*rew_op = IFH_REW_OP_NOOP;
> +		*pdu_type = IFH_PDU_TYPE_NONE;
> +		return;
> +	}
>   
> -	if (port->ptp_tx_cmd == IFH_REW_OP_TWO_STEP_PTP)
> -		return IFH_REW_OP_TWO_STEP_PTP;
> +	if (type & PTP_CLASS_L2)
> +		*pdu_type = IFH_PDU_TYPE_NONE;
> +	if (type & PTP_CLASS_IPV4)
> +		*pdu_type = IFH_PDU_TYPE_IPV4;
> +	if (type & PTP_CLASS_IPV6)
> +		*pdu_type = IFH_PDU_TYPE_IPV6;

ptp_classify_raw() will also return PTP_CLASS_IPV4 or PTP_CLASS_IPV6
flags set for (PTP_CLASS_VLAN|PTP_CLASS_IPV4) and
(PTP_CLASS_VLAN|PTP_CLASS_IPV6) cases. Will the hardware support proper
timestamp placing in these cases?

> +
> +	if (port->ptp_tx_cmd == IFH_REW_OP_TWO_STEP_PTP) {
> +		*rew_op = IFH_REW_OP_TWO_STEP_PTP;
> +		return;
> +	}
>   
>   	/* If it is sync and run 1 step then set the correct operation,
>   	 * otherwise run as 2 step
>   	 */
>   	msgtype = ptp_get_msgtype(header, type);
> -	if ((msgtype & 0xf) == 0)
> -		return IFH_REW_OP_ONE_STEP_PTP;
> +	if ((msgtype & 0xf) == 0) {
> +		*rew_op = IFH_REW_OP_ONE_STEP_PTP;
> +		return;
> +	}
>   
> -	return IFH_REW_OP_TWO_STEP_PTP;
> +	*rew_op = IFH_REW_OP_TWO_STEP_PTP;
>   }
>   
>   static void lan966x_ptp_txtstamp_old_release(struct lan966x_port *port)
> @@ -374,10 +395,12 @@ int lan966x_ptp_txtstamp_request(struct lan966x_port *port,
>   {
>   	struct lan966x *lan966x = port->lan966x;
>   	unsigned long flags;
> +	u8 pdu_type;
>   	u8 rew_op;
>   
> -	rew_op = lan966x_ptp_classify(port, skb);
> +	lan966x_ptp_classify(port, skb, &rew_op, &pdu_type);
>   	LAN966X_SKB_CB(skb)->rew_op = rew_op;
> +	LAN966X_SKB_CB(skb)->pdu_type = pdu_type;
>   
>   	if (rew_op != IFH_REW_OP_TWO_STEP_PTP)
>   		return 0;
Re: [PATCH net] net: lan966x: Fix 1-step timestamping over ipv4 or ipv6
Posted by Horatiu Vultur 6 months, 3 weeks ago 
The 05/22/2025 11:47, Vadim Fedorenko wrote:

Hi Vadim,


> > -static int lan966x_ptp_classify(struct lan966x_port *port, struct sk_buff *skb)
> > +static void lan966x_ptp_classify(struct lan966x_port *port, struct sk_buff *skb,
> > +                              u8 *rew_op, u8 *pdu_type)
> >   {
> >       struct ptp_header *header;
> >       u8 msgtype;
> >       int type;
> > 
> > -     if (port->ptp_tx_cmd == IFH_REW_OP_NOOP)
> > -             return IFH_REW_OP_NOOP;
> > +     if (port->ptp_tx_cmd == IFH_REW_OP_NOOP) {
> > +             *rew_op = IFH_REW_OP_NOOP;
> > +             *pdu_type = IFH_PDU_TYPE_NONE;
> > +             return;
> > +     }
> > 
> >       type = ptp_classify_raw(skb);
> > -     if (type == PTP_CLASS_NONE)
> > -             return IFH_REW_OP_NOOP;
> > +     if (type == PTP_CLASS_NONE) {
> > +             *rew_op = IFH_REW_OP_NOOP;
> > +             *pdu_type = IFH_PDU_TYPE_NONE;
> > +             return;
> > +     }
> > 
> >       header = ptp_parse_header(skb, type);
> > -     if (!header)
> > -             return IFH_REW_OP_NOOP;
> > +     if (!header) {
> > +             *rew_op = IFH_REW_OP_NOOP;
> > +             *pdu_type = IFH_PDU_TYPE_NONE;
> > +             return;
> > +     }
> > 
> > -     if (port->ptp_tx_cmd == IFH_REW_OP_TWO_STEP_PTP)
> > -             return IFH_REW_OP_TWO_STEP_PTP;
> > +     if (type & PTP_CLASS_L2)
> > +             *pdu_type = IFH_PDU_TYPE_NONE;
> > +     if (type & PTP_CLASS_IPV4)
> > +             *pdu_type = IFH_PDU_TYPE_IPV4;
> > +     if (type & PTP_CLASS_IPV6)
> > +             *pdu_type = IFH_PDU_TYPE_IPV6;
> 
> ptp_classify_raw() will also return PTP_CLASS_IPV4 or PTP_CLASS_IPV6
> flags set for (PTP_CLASS_VLAN|PTP_CLASS_IPV4) and
> (PTP_CLASS_VLAN|PTP_CLASS_IPV6) cases. Will the hardware support proper
> timestamp placing in these cases?

Yes, the HW seems to be working also in that case.
I just created a vlan interface and then start ptp4l on that interface
and I could see that the frames were updated correctly.

> 
> > +
> > +     if (port->ptp_tx_cmd == IFH_REW_OP_TWO_STEP_PTP) {
> > +             *rew_op = IFH_REW_OP_TWO_STEP_PTP;
> > +             return;
> > +     }
> > 
> >       /* If it is sync and run 1 step then set the correct operation,
> >        * otherwise run as 2 step
> >        */
> >       msgtype = ptp_get_msgtype(header, type);
> > -     if ((msgtype & 0xf) == 0)
> > -             return IFH_REW_OP_ONE_STEP_PTP;
> > +     if ((msgtype & 0xf) == 0) {
> > +             *rew_op = IFH_REW_OP_ONE_STEP_PTP;
> > +             return;
> > +     }
> > 
> > -     return IFH_REW_OP_TWO_STEP_PTP;
> > +     *rew_op = IFH_REW_OP_TWO_STEP_PTP;
> >   }
> > 
> >   static void lan966x_ptp_txtstamp_old_release(struct lan966x_port *port)
> > @@ -374,10 +395,12 @@ int lan966x_ptp_txtstamp_request(struct lan966x_port *port,
> >   {
> >       struct lan966x *lan966x = port->lan966x;
> >       unsigned long flags;
> > +     u8 pdu_type;
> >       u8 rew_op;
> > 
> > -     rew_op = lan966x_ptp_classify(port, skb);
> > +     lan966x_ptp_classify(port, skb, &rew_op, &pdu_type);
> >       LAN966X_SKB_CB(skb)->rew_op = rew_op;
> > +     LAN966X_SKB_CB(skb)->pdu_type = pdu_type;
> > 
> >       if (rew_op != IFH_REW_OP_TWO_STEP_PTP)
> >               return 0;
> 

-- 
/Horatiu

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.