Re: [PATCH net-next 08/12] net: enetc: save the parsed information of PTP packet to skb->cb

[Vadim Fedorenko <vfedorenko@novek.ru>]

On 11.07.2025 07:57, Wei Fang wrote:
> Currently, the Tx PTP packets are parsed twice in the enetc driver, once
> in enetc_xmit() and once in enetc_map_tx_buffs(). The latter is duplicate
> and is unnecessary, since the parsed information can be saved to skb->cb
> so that enetc_map_tx_buffs() can get the previously parsed data from
> skb->cb. Therefore, we add struct enetc_skb_cb as the format of the data
> in the skb->cb buffer to save the parsed information of PTP packet.
> 
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---
>   drivers/net/ethernet/freescale/enetc/enetc.c | 65 ++++++++++----------
>   drivers/net/ethernet/freescale/enetc/enetc.h |  9 +++
>   2 files changed, 43 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index e4287725832e..c1373163a096 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -225,13 +225,12 @@ static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb)
>   {
>   	bool do_vlan, do_onestep_tstamp = false, do_twostep_tstamp = false;
>   	struct enetc_ndev_priv *priv = netdev_priv(tx_ring->ndev);
> +	struct enetc_skb_cb *enetc_cb = ENETC_SKB_CB(skb);
>   	struct enetc_hw *hw = &priv->si->hw;
>   	struct enetc_tx_swbd *tx_swbd;
>   	int len = skb_headlen(skb);
>   	union enetc_tx_bd temp_bd;
> -	u8 msgtype, twostep, udp;
>   	union enetc_tx_bd *txbd;
> -	u16 offset1, offset2;
>   	int i, count = 0;
>   	skb_frag_t *frag;
>   	unsigned int f;
> @@ -280,16 +279,10 @@ static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb)
>   	count++;
>   
>   	do_vlan = skb_vlan_tag_present(skb);
> -	if (skb->cb[0] & ENETC_F_TX_ONESTEP_SYNC_TSTAMP) {
> -		if (enetc_ptp_parse(skb, &udp, &msgtype, &twostep, &offset1,
> -				    &offset2) ||
> -		    msgtype != PTP_MSGTYPE_SYNC || twostep)
> -			WARN_ONCE(1, "Bad packet for one-step timestamping\n");
> -		else
> -			do_onestep_tstamp = true;
> -	} else if (skb->cb[0] & ENETC_F_TX_TSTAMP) {
> +	if (enetc_cb->flag & ENETC_F_TX_ONESTEP_SYNC_TSTAMP)
> +		do_onestep_tstamp = true;
> +	else if (enetc_cb->flag & ENETC_F_TX_TSTAMP)
>   		do_twostep_tstamp = true;
> -	}
>   
>   	tx_swbd->do_twostep_tstamp = do_twostep_tstamp;
>   	tx_swbd->qbv_en = !!(priv->active_offloads & ENETC_F_QBV);
> @@ -333,6 +326,8 @@ static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb)
>   		}
>   
>   		if (do_onestep_tstamp) {
> +			u16 tstamp_off = enetc_cb->origin_tstamp_off;
> +			u16 corr_off = enetc_cb->correction_off;
>   			__be32 new_sec_l, new_nsec;
>   			u32 lo, hi, nsec, val;
>   			__be16 new_sec_h;
> @@ -362,32 +357,32 @@ static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb)
>   			new_sec_h = htons((sec >> 32) & 0xffff);
>   			new_sec_l = htonl(sec & 0xffffffff);
>   			new_nsec = htonl(nsec);
> -			if (udp) {
> +			if (enetc_cb->udp) {
>   				struct udphdr *uh = udp_hdr(skb);
>   				__be32 old_sec_l, old_nsec;
>   				__be16 old_sec_h;
>   
> -				old_sec_h = *(__be16 *)(data + offset2);
> +				old_sec_h = *(__be16 *)(data + tstamp_off);
>   				inet_proto_csum_replace2(&uh->check, skb, old_sec_h,
>   							 new_sec_h, false);
>   
> -				old_sec_l = *(__be32 *)(data + offset2 + 2);
> +				old_sec_l = *(__be32 *)(data + tstamp_off + 2);
>   				inet_proto_csum_replace4(&uh->check, skb, old_sec_l,
>   							 new_sec_l, false);
>   
> -				old_nsec = *(__be32 *)(data + offset2 + 6);
> +				old_nsec = *(__be32 *)(data + tstamp_off + 6);
>   				inet_proto_csum_replace4(&uh->check, skb, old_nsec,
>   							 new_nsec, false);
>   			}
>   
> -			*(__be16 *)(data + offset2) = new_sec_h;
> -			*(__be32 *)(data + offset2 + 2) = new_sec_l;
> -			*(__be32 *)(data + offset2 + 6) = new_nsec;
> +			*(__be16 *)(data + tstamp_off) = new_sec_h;
> ++			*(__be32 *)(data + tstamp_off + 2) = new_sec_l;
> ++			*(__be32 *)(data + tstamp_off + 6) = new_nsec;

This looks like merge conflict artifact...

>   
>   			/* Configure single-step register */
>   			val = ENETC_PM0_SINGLE_STEP_EN;
> -			val |= ENETC_SET_SINGLE_STEP_OFFSET(offset1);
> -			if (udp)
> +			val |= ENETC_SET_SINGLE_STEP_OFFSET(corr_off);
> +			if (enetc_cb->udp)
>   				val |= ENETC_PM0_SINGLE_STEP_CH;
>   
>   			enetc_port_mac_wr(priv->si, ENETC_PM0_SINGLE_STEP,
> @@ -938,12 +933,13 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb
>   static netdev_tx_t enetc_start_xmit(struct sk_buff *skb,
>   				    struct net_device *ndev)
>   {
> +	struct enetc_skb_cb *enetc_cb = ENETC_SKB_CB(skb);
>   	struct enetc_ndev_priv *priv = netdev_priv(ndev);
>   	struct enetc_bdr *tx_ring;
>   	int count;
>   
>   	/* Queue one-step Sync packet if already locked */
> -	if (skb->cb[0] & ENETC_F_TX_ONESTEP_SYNC_TSTAMP) {
> +	if (enetc_cb->flag & ENETC_F_TX_ONESTEP_SYNC_TSTAMP) {
>   		if (test_and_set_bit_lock(ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS,
>   					  &priv->flags)) {
>   			skb_queue_tail(&priv->tx_skbs, skb);
> @@ -1005,24 +1001,29 @@ static netdev_tx_t enetc_start_xmit(struct sk_buff *skb,
>   
>   netdev_tx_t enetc_xmit(struct sk_buff *skb, struct net_device *ndev)
>   {
> +	struct enetc_skb_cb *enetc_cb = ENETC_SKB_CB(skb);
>   	struct enetc_ndev_priv *priv = netdev_priv(ndev);
>   	u8 udp, msgtype, twostep;
>   	u16 offset1, offset2;
>   
> -	/* Mark tx timestamp type on skb->cb[0] if requires */
> +	/* Mark tx timestamp type on enetc_cb->flag if requires */
>   	if ((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
> -	    (priv->active_offloads & ENETC_F_TX_TSTAMP_MASK)) {
> -		skb->cb[0] = priv->active_offloads & ENETC_F_TX_TSTAMP_MASK;
> -	} else {
> -		skb->cb[0] = 0;
> -	}
> +	    (priv->active_offloads & ENETC_F_TX_TSTAMP_MASK))
> +		enetc_cb->flag = priv->active_offloads & ENETC_F_TX_TSTAMP_MASK;
> +	else
> +		enetc_cb->flag = 0;
>   
>   	/* Fall back to two-step timestamp if not one-step Sync packet */
> -	if (skb->cb[0] & ENETC_F_TX_ONESTEP_SYNC_TSTAMP) {
> +	if (enetc_cb->flag & ENETC_F_TX_ONESTEP_SYNC_TSTAMP) {
>   		if (enetc_ptp_parse(skb, &udp, &msgtype, &twostep,
>   				    &offset1, &offset2) ||
> -		    msgtype != PTP_MSGTYPE_SYNC || twostep != 0)
> -			skb->cb[0] = ENETC_F_TX_TSTAMP;
> +		    msgtype != PTP_MSGTYPE_SYNC || twostep != 0) {
> +			enetc_cb->flag = ENETC_F_TX_TSTAMP;
> +		} else {
> +			enetc_cb->udp = !!udp;
> +			enetc_cb->correction_off = offset1;
> +			enetc_cb->origin_tstamp_off = offset2;
> +		}
>   	}
>   
>   	return enetc_start_xmit(skb, ndev);
> @@ -1214,7 +1215,9 @@ static bool enetc_clean_tx_ring(struct enetc_bdr *tx_ring, int napi_budget)
>   		if (xdp_frame) {
>   			xdp_return_frame(xdp_frame);
>   		} else if (skb) {
> -			if (unlikely(skb->cb[0] & ENETC_F_TX_ONESTEP_SYNC_TSTAMP)) {
> +			struct enetc_skb_cb *enetc_cb = ENETC_SKB_CB(skb);
> +
> +			if (unlikely(enetc_cb->flag & ENETC_F_TX_ONESTEP_SYNC_TSTAMP)) {
>   				/* Start work to release lock for next one-step
>   				 * timestamping packet. And send one skb in
>   				 * tx_skbs queue if has.
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h
> index 62e8ee4d2f04..ce3fed95091b 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.h
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.h
> @@ -54,6 +54,15 @@ struct enetc_tx_swbd {
>   	u8 qbv_en:1;
>   };
>   
> +struct enetc_skb_cb {
> +	u8 flag;
> +	bool udp;
> +	u16 correction_off;
> +	u16 origin_tstamp_off;
> +};
> +
> +#define ENETC_SKB_CB(skb) ((struct enetc_skb_cb *)((skb)->cb))
> +
>   struct enetc_lso_t {
>   	bool	ipv6;
>   	bool	tcp;