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

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

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;