misc drivers' sw timestamp changes

PATCH Superseded Jason Xing 3 participants 13 messages May 8, 2025 – May 12, 2025

Subsystems (8)

Networking Network drivers Arm STM32 stmmac Ethernet driver +4 more subsystems

lore

Share & tools

lore

Jason Xing

author

2025-05-08 03:33 UTC #0 lore raw

cover misc drivers' sw timestamp changes

From: Jason Xing <[email protected]>

I filtered more than 100 drivers and only modified four outstanding drivers among them because the software timestamp generation is too early. The idea of this series is derived from the brief talk[1] with Willem. In conclusion, this series makes the generation of software timestamp near/before kicking the doorbell for drivers.

[1]: https://lore.kernel.org/all/[email protected]/

Jason Xing (4):
net: atlantic: generate software timestamp just before the doorbell
net: cxgb4: generate software timestamp just before the doorbell
net: stmmac: generate software timestamp just before the doorbell
net: lan966x: generate software timestamp just before the doorbell

 drivers/net/ethernet/aquantia/atlantic/aq_main.c            | 1 -
 drivers/net/ethernet/aquantia/atlantic/aq_nic.c             | 2 ++
 drivers/net/ethernet/chelsio/cxgb4/sge.c                    | 5 ++---
 .../net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c  | 2 +-
 drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c       | 2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c           | 6 ++----
 6 files changed, 8 insertions(+), 10 deletions(-)

Signature · 1 line

2.43.5

Jason Xing

author

2025-05-08 03:33 UTC #1 lore raw

PATCH 1/4 net: atlantic: generate software timestamp just before the doorbell

From: Jason Xing <[email protected]>

Make sure the call of skb_tx_timestamp as close to the doorbell.

Signed-off-by Jason Xing <[email protected]>

---
 drivers/net/ethernet/aquantia/atlantic/aq_main.c | 1 -
 drivers/net/ethernet/aquantia/atlantic/aq_nic.c  | 2 ++
 2 files changed, 2 insertions(+), 1 deletion(-)

drivers/net/ethernet/aquantia/atlantic/aq_main.c +0 -1

index c1d1673c5749..b565189e5913 100644
@@ -123,7 +123,6 @@ static netdev_tx_t aq_ndev_start_xmit(struct sk_buff *skb, struct net_device *nd
 	}
 #endif
 
-	skb_tx_timestamp(skb);
 	return aq_nic_xmit(aq_nic, skb);
 }

drivers/net/ethernet/aquantia/atlantic/aq_nic.c +2 -0

index bf3aa46887a1..e71cd10e4e1f 100644
@@ -898,6 +898,8 @@ int aq_nic_xmit(struct aq_nic_s *self, struct sk_buff *skb)
 
 	frags = aq_nic_map_skb(self, skb, ring);
 
+	skb_tx_timestamp(skb);
+
 	if (likely(frags)) {
 		err = self->aq_hw_ops->hw_ring_tx_xmit(self->aq_hw,
 						       ring, frags);

Signature · 1 line

2.43.5

Jason Xing

author

2025-05-08 03:33 UTC #2 lore raw

PATCH 2/4 net: cxgb4: generate software timestamp just before the doorbell

In reply to Jason Xing

From: Jason Xing <[email protected]>

Make sure the call of skb_tx_timestamp as close to the doorbell.

Signed-off-by Jason Xing <[email protected]>

---
 drivers/net/ethernet/chelsio/cxgb4/sge.c                     | 5 ++---
 .../net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c   | 2 +-
 2 files changed, 3 insertions(+), 4 deletions(-)

drivers/net/ethernet/chelsio/cxgb4/sge.c +2 -3

index f991a28a71c3..ee5075b252fd 100644
@@ -1533,7 +1533,6 @@ static netdev_tx_t cxgb4_eth_xmit(struct sk_buff *skb, struct net_device *dev)
 	} else {
 		q = &adap->sge.ethtxq[qidx + pi->first_qset];
 	}
-	skb_tx_timestamp(skb);
 
 	reclaim_completed_tx(adap, &q->q, -1, true);
 	cntrl = TXPKT_L4CSUM_DIS_F | TXPKT_IPCSUM_DIS_F;

@@ -1717,7 +1716,7 @@ static netdev_tx_t cxgb4_eth_xmit(struct sk_buff *skb, struct net_device *dev)
 	}
 
 	txq_advance(&q->q, ndesc);
-
+	skb_tx_timestamp(skb);
 	cxgb4_ring_tx_db(adap, &q->q, ndesc);
 	return NETDEV_TX_OK;

@@ -2268,7 +2267,6 @@ static int ethofld_hard_xmit(struct net_device *dev,
 
 	d = &eosw_txq->desc[eosw_txq->last_pidx];
 	skb = d->skb;
-	skb_tx_timestamp(skb);
 
 	wr = (struct fw_eth_tx_eo_wr *)&eohw_txq->q.desc[eohw_txq->q.pidx];
 	if (unlikely(eosw_txq->state != CXGB4_EO_STATE_ACTIVE &&

@@ -2373,6 +2371,7 @@ static int ethofld_hard_xmit(struct net_device *dev,
 		eohw_txq->vlan_ins++;
 
 	txq_advance(&eohw_txq->q, ndesc);
+	skb_tx_timestamp(skb);
 	cxgb4_ring_tx_db(adap, &eohw_txq->q, ndesc);
 	eosw_txq_advance_index(&eosw_txq->last_pidx, 1, eosw_txq->ndesc);

drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c +1 -1

index e8e460a92e0e..4e2096e49684 100644
@@ -1640,6 +1640,7 @@ static int chcr_ktls_tunnel_pkt(struct chcr_ktls_info *tx_info,
 	cxgb4_write_sgl(skb, &q->q, pos, end, 0, sgl_sdesc->addr);
 	sgl_sdesc->skb = skb;
 	chcr_txq_advance(&q->q, ndesc);
+	skb_tx_timestamp(skb);
 	cxgb4_ring_tx_db(tx_info->adap, &q->q, ndesc);
 	return 0;
 }

@@ -1903,7 +1904,6 @@ static int chcr_ktls_sw_fallback(struct sk_buff *skb,
 	th = tcp_hdr(nskb);
 	skb_offset = skb_tcp_all_headers(nskb);
 	data_len = nskb->len - skb_offset;
-	skb_tx_timestamp(nskb);
 
 	if (chcr_ktls_tunnel_pkt(tx_info, nskb, q))
 		goto out;

Signature · 1 line

2.43.5

Jason Xing

author

2025-05-08 03:33 UTC #3 lore raw

PATCH 3/4 net: stmmac: generate software timestamp just before the doorbell

In reply to Jason Xing

From: Jason Xing <[email protected]>

Make sure the call of skb_tx_timestamp as close to the doorbell.

In stmmac_tso_xmit(), adjust the order of setting SKBTX_IN_PROGRESS and generating software timestamp so that without SOF_TIMESTAMPING_OPT_TX_SWHW being set the software and hardware timestamps will not appear at the same time (Please see __skb_tstamp_tx()).

Signed-off-by Jason Xing <[email protected]>

---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +2 -4

index 28b62bd73e23..e7266e517edb 100644
@@ -4497,8 +4497,6 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 	if (priv->sarc_type)
 		stmmac_set_desc_sarc(priv, first, priv->sarc_type);
 
-	skb_tx_timestamp(skb);
-
 	if (unlikely((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
 		     priv->hwts_tx_en)) {
 		/* declare that device is doing timestamping */

@@ -4531,6 +4529,7 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 	}
 
 	netdev_tx_sent_queue(netdev_get_tx_queue(dev, queue), skb->len);
+	skb_tx_timestamp(skb);
 
 	stmmac_flush_tx_descriptors(priv, queue);
 	stmmac_tx_timer_arm(priv, queue);

@@ -4774,8 +4773,6 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 	if (priv->sarc_type)
 		stmmac_set_desc_sarc(priv, first, priv->sarc_type);
 
-	skb_tx_timestamp(skb);
-
 	/* Ready to fill the first descriptor and set the OWN bit w/o any
 	 * problems because all the descriptors are actually ready to be
 	 * passed to the DMA engine.

@@ -4820,6 +4817,7 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 	stmmac_set_tx_owner(priv, first);
 
 	netdev_tx_sent_queue(netdev_get_tx_queue(dev, queue), skb->len);
+	skb_tx_timestamp(skb);
 
 	stmmac_enable_dma_transmission(priv, priv->ioaddr, queue);

Signature · 1 line

2.43.5

Jason Xing

author

2025-05-08 03:34 UTC #4 lore raw

PATCH 4/4 net: lan966x: generate software timestamp just before the doorbell

In reply to Jason Xing

From: Jason Xing <[email protected]>

Make sure the call of skb_tx_timestamp as close to the doorbell.

Signed-off-by Jason Xing <[email protected]>

---
 drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c +1 -1

index 502670718104..e030f23e5145 100644
@@ -730,7 +730,6 @@ int lan966x_fdma_xmit(struct sk_buff *skb, __be32 *ifh, struct net_device *dev)
 		}
 	}
 
-	skb_tx_timestamp(skb);
 	skb_push(skb, IFH_LEN_BYTES);
 	memcpy(skb->data, ifh, IFH_LEN_BYTES);
 	skb_put(skb, 4);

@@ -768,6 +767,7 @@ int lan966x_fdma_xmit(struct sk_buff *skb, __be32 *ifh, struct net_device *dev)
 		next_dcb_buf->ptp = true;
 
 	/* Start the transmission */
+	skb_tx_timestamp(skb);
 	lan966x_fdma_tx_start(tx);
 
 	return NETDEV_TX_OK;

Signature · 1 line

2.43.5

Horatiu Vultur

maintainer Nacked-by on v1 4/4 NAK

2025-05-08 07:08 UTC #5 lore raw

Re: [PATCH net-next v1 4/4] net: lan966x: generate software timestamp just before the doorbell

Quoted 15 lines · Jason Xing

From: Jason Xing <[email protected]>
Make sure the call of skb_tx_timestamp as close to the doorbell.
Signed-off-by Jason Xing <[email protected]>
---
 drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c +0 -0
index 502670718104..e030f23e5145 100644

drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c int lan966x_fdma_xmit(struct sk_buff *skb, __be32 *ifh, struct net_device *dev) · line 733

Quoted 4 lines

drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c +0 -1

@@ -730,7 +730,6 @@ int lan966x_fdma_xmit(struct sk_buff *skb, __be32 *ifh, struct net_device *dev)
                }
        }
 
-       skb_tx_timestamp(skb);

Changing this will break the PHY timestamping because the frame gets modified in the next line, meaning that the classify function will always return PTP_CLASS_NONE.

Nacked-by Horatiu Vultur <[email protected]>

Quoted 14 lines

drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c +1 -1

        skb_push(skb, IFH_LEN_BYTES);
        memcpy(skb->data, ifh, IFH_LEN_BYTES);
        skb_put(skb, 4);

@@ -768,6 +767,7 @@ int lan966x_fdma_xmit(struct sk_buff *skb, __be32 *ifh, struct net_device *dev)
                next_dcb_buf->ptp = true;
 
        /* Start the transmission */
+       skb_tx_timestamp(skb);
        lan966x_fdma_tx_start(tx);
 
        return NETDEV_TX_OK;
--

2.43.5

Signature · 1 line

/Horatiu

Jason Xing

author

2025-05-08 08:41 UTC #6 lore raw

Re: [PATCH net-next v1 4/4] net: lan966x: generate software timestamp just before the doorbell

Hi Horatiu,

Quoted 20 lines · Horatiu Vultur

Quoted 18 lines · Jason Xing
From: Jason Xing <[email protected]>
Make sure the call of skb_tx_timestamp as close to the doorbell.
Signed-off-by Jason Xing <[email protected]>
---
 drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c +0 -0
index 502670718104..e030f23e5145 100644
@@ -730,7 +730,6 @@ int lan966x_fdma_xmit(struct sk_buff *skb, __be32 *ifh, struct net_device *dev)
                }
        }
 

Quoted 5 lines

drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c int lan966x_fdma_xmit(struct sk_buff *skb, __be32 *ifh, struct net_device *dev) · line 733
Quoted 1 line
drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c +0 -1
-       skb_tx_timestamp(skb);
Changing this will break the PHY timestamping because the frame gets modified in the next line, meaning that the classify function will always return PTP_CLASS_NONE.

Sorry that I'm not that familiar with the details. I will remove it from this series, but still trying to figure out what cases could be.

Do you mean it can break when bpf prog is loaded because 'skb_push(skb, IFH_LEN_BYTES);' expands the skb->data area? May I ask how the modified data of skb breaks the PHY timestamping feature?

Thanks,
Jason

Quoted 20 lines

Nacked-by Horatiu Vultur <[email protected]>

Quoted 12 lines

drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c +1 -1

        skb_push(skb, IFH_LEN_BYTES);
        memcpy(skb->data, ifh, IFH_LEN_BYTES);
        skb_put(skb, 4);

@@ -768,6 +767,7 @@ int lan966x_fdma_xmit(struct sk_buff *skb, __be32 *ifh, struct net_device *dev)
                next_dcb_buf->ptp = true;
 
        /* Start the transmission */
+       skb_tx_timestamp(skb);
        lan966x_fdma_tx_start(tx);
 
        return NETDEV_TX_OK;
--

Quoted 2 lines

2.43.5

--
/Horatiu

Horatiu Vultur

maintainer

2025-05-08 09:44 UTC #7 lore raw

Re: [PATCH net-next v1 4/4] net: lan966x: generate software timestamp just before the doorbell

Quoted 1 line · Jason Xing

Hi Horatiu,

Hi Jason,

Quoted 29 lines

Quoted 20 lines · Horatiu Vultur
Quoted 18 lines · Jason Xing
From: Jason Xing <[email protected]>
Make sure the call of skb_tx_timestamp as close to the doorbell.
Signed-off-by Jason Xing <[email protected]>
---
 drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c +0 -0
index 502670718104..e030f23e5145 100644
@@ -730,7 +730,6 @@ int lan966x_fdma_xmit(struct sk_buff *skb, __be32 *ifh, struct net_device *dev)
                }
        }
 
Quoted 5 lines
drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c int lan966x_fdma_xmit(struct sk_buff *skb, __be32 *ifh, struct net_device *dev) · line 733
Quoted 1 line
drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c +0 -1
-       skb_tx_timestamp(skb);
Changing this will break the PHY timestamping because the frame gets modified in the next line, meaning that the classify function will always return PTP_CLASS_NONE.

Quoted 5 lines

Sorry that I'm not that familiar with the details. I will remove it from this series, but still trying to figure out what cases could be.
Do you mean it can break when bpf prog is loaded because 'skb_push(skb, IFH_LEN_BYTES);' expands the skb->data area?

Well, the bpf program will check if it is a PTP frame that needs to be timestamp when it runs ptp_classify_raw, and as we push some data in front of the frame, the bpf will run from that point meaning that it would failed to detect the PTP frames.

Quoted 2 lines

May I ask
how the modified data of skb breaks the PHY timestamping feature?

If it fails to detect that it is a PTP frame, then the frame will not be passed to the PHY using the callback txtstamp. So the PHY will timestamp the frame but it doesn't have the frame to attach the timestamp.

Quoted 24 lines

Thanks,
Jason

Quoted 20 lines

Nacked-by Horatiu Vultur <[email protected]>

Quoted 12 lines

drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c +1 -1

        skb_push(skb, IFH_LEN_BYTES);
        memcpy(skb->data, ifh, IFH_LEN_BYTES);
        skb_put(skb, 4);

@@ -768,6 +767,7 @@ int lan966x_fdma_xmit(struct sk_buff *skb, __be32 *ifh, struct net_device *dev)
                next_dcb_buf->ptp = true;
 
        /* Start the transmission */
+       skb_tx_timestamp(skb);
        lan966x_fdma_tx_start(tx);
 
        return NETDEV_TX_OK;
--

Quoted 2 lines

2.43.5

--
/Horatiu

Signature · 1 line

/Horatiu

Jason Xing

author

2025-05-08 12:23 UTC #8 lore raw

Re: [PATCH net-next v1 4/4] net: lan966x: generate software timestamp just before the doorbell

Quoted 38 lines · Horatiu Vultur

Quoted 1 line · Jason Xing
Hi Horatiu,
Hi Jason,
Quoted 32 lines
Quoted 20 lines · Horatiu Vultur
Quoted 18 lines · Jason Xing
From: Jason Xing <[email protected]>
Make sure the call of skb_tx_timestamp as close to the doorbell.
Signed-off-by Jason Xing <[email protected]>
---
 drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c +0 -0
index 502670718104..e030f23e5145 100644
@@ -730,7 +730,6 @@ int lan966x_fdma_xmit(struct sk_buff *skb, __be32 *ifh, struct net_device *dev)
                }
        }
 
Quoted 5 lines
drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c int lan966x_fdma_xmit(struct sk_buff *skb, __be32 *ifh, struct net_device *dev) · line 733
Quoted 1 line
drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c +0 -1
-       skb_tx_timestamp(skb);
Changing this will break the PHY timestamping because the frame gets modified in the next line, meaning that the classify function will always return PTP_CLASS_NONE.
Sorry that I'm not that familiar with the details. I will remove it from this series, but still trying to figure out what cases could be.

Quoted 7 lines

Quoted 2 lines
Do you mean it can break when bpf prog is loaded because 'skb_push(skb, IFH_LEN_BYTES);' expands the skb->data area?
Well, the bpf program will check if it is a PTP frame that needs to be timestamp when it runs ptp_classify_raw, and as we push some data in front of the frame, the bpf will run from that point meaning that it would failed to detect the PTP frames.

Thanks for the kind reply.

It looks like how to detect depends on how the bpf prog is written? Mostly depends on how the writer handles this data part. Even though we don't guarantee on how to ask users/admins to write/adjust their bpf codes, it's not that convenient for them if this patch is applied, to be frank. I'm not pushing you to accept this patch, just curious on "how and why". Now I can guess why you're opposed to it....

Thanks,
Jason

Quoted 35 lines

Quoted 3 lines
May I ask
how the modified data of skb breaks the PHY timestamping feature?
If it fails to detect that it is a PTP frame, then the frame will not be passed to the PHY using the callback txtstamp. So the PHY will timestamp the frame but it doesn't have the frame to attach the timestamp.
Quoted 24 lines
Thanks,
Jason
Quoted 20 lines
Nacked-by Horatiu Vultur <[email protected]>
Quoted 12 lines
drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c +1 -1
        skb_push(skb, IFH_LEN_BYTES);
        memcpy(skb->data, ifh, IFH_LEN_BYTES);
        skb_put(skb, 4);
@@ -768,6 +767,7 @@ int lan966x_fdma_xmit(struct sk_buff *skb, __be32 *ifh, struct net_device *dev)
                next_dcb_buf->ptp = true;
 
        /* Start the transmission */
+       skb_tx_timestamp(skb);
        lan966x_fdma_tx_start(tx);
 
        return NETDEV_TX_OK;
--
Quoted 2 lines
2.43.5
--
/Horatiu
--
/Horatiu

Vladimir Oltean

2025-05-08 12:40 UTC #9 lore raw

Re: [PATCH net-next v1 4/4] net: lan966x: generate software timestamp just before the doorbell

Quoted 8 lines · Jason Xing

Thanks for the kind reply.
It looks like how to detect depends on how the bpf prog is written? Mostly depends on how the writer handles this data part. Even though we don't guarantee on how to ask users/admins to write/adjust their bpf codes, it's not that convenient for them if this patch is applied, to be frank. I'm not pushing you to accept this patch, just curious on "how and why". Now I can guess why you're opposed to it....

The BPF program is not user-generated, it is run in the context of the function you're moving.

skb_tx_timestamp()
-> skb_clone_tx_timestamp()
-> classify()

      -> ptp_classify_raw()
         -> bpf_prog_run(ptp_insns, skb)

Jason Xing

author

2025-05-08 14:16 UTC #10 lore raw

Re: [PATCH net-next v1 4/4] net: lan966x: generate software timestamp just before the doorbell

Quoted 11 lines · Vladimir Oltean

Quoted 8 lines · Jason Xing
Thanks for the kind reply.
It looks like how to detect depends on how the bpf prog is written? Mostly depends on how the writer handles this data part. Even though we don't guarantee on how to ask users/admins to write/adjust their bpf codes, it's not that convenient for them if this patch is applied, to be frank. I'm not pushing you to accept this patch, just curious on "how and why". Now I can guess why you're opposed to it....

Quoted 8 lines

The BPF program is not user-generated, it is run in the context of the function you're moving.
skb_tx_timestamp()
-> skb_clone_tx_timestamp()
-> classify()
      -> ptp_classify_raw()
         -> bpf_prog_run(ptp_insns, skb)

Right, I'll drop this patch from the series then.

Thanks,
Jason

Vladimir Oltean

2025-05-08 14:22 UTC #11 lore raw

Re: [PATCH net-next v1 4/4] net: lan966x: generate software timestamp just before the doorbell

In reply to Horatiu Vultur

Horatiu,

Quoted 17 lines · Horatiu Vultur

Quoted 11 lines
Quoted 5 lines
Quoted 1 line
- skb_tx_timestamp(skb);
Changing this will break the PHY timestamping because the frame gets modified in the next line, meaning that the classify function will always return PTP_CLASS_NONE.
Sorry that I'm not that familiar with the details. I will remove it from this series, but still trying to figure out what cases could be.
Do you mean it can break when bpf prog is loaded because 'skb_push(skb, IFH_LEN_BYTES);' expands the skb->data area?
Well, the bpf program will check if it is a PTP frame that needs to be timestamp when it runs ptp_classify_raw, and as we push some data in front of the frame, the bpf will run from that point meaning that it would failed to detect the PTP frames.

Quoted 6 lines

Quoted 2 lines
May I ask
how the modified data of skb breaks the PHY timestamping feature?
If it fails to detect that it is a PTP frame, then the frame will not be passed to the PHY using the callback txtstamp. So the PHY will timestamp the frame but it doesn't have the frame to attach the timestamp.

While I was further discussing this in private with Jason, a thought popped up in my head.

Shouldn't skb_tx_timestamp(skb); be done _before_ this section?

	/* skb processing */
	needed_headroom = max_t(int, IFH_LEN_BYTES - skb_headroom(skb), 0);
	needed_tailroom = max_t(int, ETH_FCS_LEN - skb_tailroom(skb), 0);
	if (needed_headroom || needed_tailroom || skb_header_cloned(skb)) {
		err = pskb_expand_head(skb, needed_headroom, needed_tailroom,
				       GFP_ATOMIC);
		if (unlikely(err)) {
			dev->stats.tx_dropped++;
			err = NETDEV_TX_OK;
			goto release;
		}
	}

The idea is that skb_tx_timestamp() calls skb_clone_tx_timestamp(), and that should require skb_unshare() before you make any further modification like insert an IFH here, so that the IFH is not visible to clones (and thus to user space, on the socket error queue).

I think pskb_expand_head() serves the role of skb_unshare(), because I see skb_header_cloned() is one of the conditions on which it is called.

But the problem is that skb_header_cloned() may have been false, then skb_tx_timestamp() makes skb_header_cloned() true, but pskb_expand_head() has already run. So the IFH is shared with the clone made for TX timestamping purposes, I guess.

Am I completely off?

Also, I believe you can set dev->needed_headroom = IFH_LEN_BYTES, dev->needed_tailroom = ETH_FCS_LEN, and then just call skb_ensure_writable_head_tail().

Horatiu Vultur

maintainer

2025-05-12 07:24 UTC #12 lore raw

Re: [PATCH net-next v1 4/4] net: lan966x: generate software timestamp just before the doorbell

Quoted 2 lines · Vladimir Oltean

Horatiu,

Hi Vladimir,

Quoted 52 lines · Horatiu Vultur

Quoted 17 lines
Quoted 11 lines
Quoted 5 lines
Quoted 1 line
- skb_tx_timestamp(skb);
Changing this will break the PHY timestamping because the frame gets modified in the next line, meaning that the classify function will always return PTP_CLASS_NONE.
Sorry that I'm not that familiar with the details. I will remove it from this series, but still trying to figure out what cases could be.
Do you mean it can break when bpf prog is loaded because 'skb_push(skb, IFH_LEN_BYTES);' expands the skb->data area?
Well, the bpf program will check if it is a PTP frame that needs to be timestamp when it runs ptp_classify_raw, and as we push some data in front of the frame, the bpf will run from that point meaning that it would failed to detect the PTP frames.
Quoted 6 lines
Quoted 2 lines
May I ask
how the modified data of skb breaks the PHY timestamping feature?
If it fails to detect that it is a PTP frame, then the frame will not be passed to the PHY using the callback txtstamp. So the PHY will timestamp the frame but it doesn't have the frame to attach the timestamp.
While I was further discussing this in private with Jason, a thought popped up in my head.
Shouldn't skb_tx_timestamp(skb); be done _before_ this section?
        /* skb processing */
        needed_headroom = max_t(int, IFH_LEN_BYTES - skb_headroom(skb), 0);
        needed_tailroom = max_t(int, ETH_FCS_LEN - skb_tailroom(skb), 0);
        if (needed_headroom || needed_tailroom || skb_header_cloned(skb)) {
                err = pskb_expand_head(skb, needed_headroom, needed_tailroom,
                                       GFP_ATOMIC);
                if (unlikely(err)) {
                        dev->stats.tx_dropped++;
                        err = NETDEV_TX_OK;
                        goto release;
                }
        }
The idea is that skb_tx_timestamp() calls skb_clone_tx_timestamp(), and that should require skb_unshare() before you make any further modification like insert an IFH here, so that the IFH is not visible to clones (and thus to user space, on the socket error queue).
I think pskb_expand_head() serves the role of skb_unshare(), because I see skb_header_cloned() is one of the conditions on which it is called.

Quoted 6 lines

But the problem is that skb_header_cloned() may have been false, then skb_tx_timestamp() makes skb_header_cloned() true, but pskb_expand_head() has already run. So the IFH is shared with the clone made for TX timestamping purposes, I guess.
Am I completely off?

Sorry for late reply.
I think you are right!. I just want to double check by actually trying it.

Quoted 4 lines

Also, I believe you can set dev->needed_headroom = IFH_LEN_BYTES, dev->needed_tailroom = ETH_FCS_LEN, and then just call skb_ensure_writable_head_tail().

Thanks for the advice. I will look also into this.

Signature · 1 line

/Horatiu