[PATCH net-next v1 0/4] misc drivers' sw timestamp changes

[PATCH net-next v1 0/4] misc drivers' sw timestamp changes

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

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/681b9d2210879_1f6aad294bc@willemb.c.googlers.com.notmuch/

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(-)

-- 
2.43.5

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

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

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

Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 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(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_main.c b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
index c1d1673c5749..b565189e5913 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_main.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
@@ -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);
 }
 
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
index bf3aa46887a1..e71cd10e4e1f 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
@@ -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);
-- 
2.43.5

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

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

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

Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 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(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/sge.c b/drivers/net/ethernet/chelsio/cxgb4/sge.c
index f991a28a71c3..ee5075b252fd 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/sge.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/sge.c
@@ -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);
 
diff --git a/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c b/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
index e8e460a92e0e..4e2096e49684 100644
--- a/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
+++ b/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
@@ -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;
-- 
2.43.5

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

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

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 <kernelxing@tencent.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 28b62bd73e23..e7266e517edb 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -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);
 
-- 
2.43.5

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

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

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

Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
index 502670718104..e030f23e5145 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
@@ -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;
-- 
2.43.5

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

[Horatiu Vultur]

The 05/08/2025 11:33, Jason Xing wrote:
> 
> From: Jason Xing <kernelxing@tencent.com>
> 
> Make sure the call of skb_tx_timestamp as close to the doorbell.
> 
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
>  drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> index 502670718104..e030f23e5145 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> @@ -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 <horatiu.vultur@microchip.com>

>         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
> 

-- 
/Horatiu

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

[Jason Xing]

Hi Horatiu,

On Thu, May 8, 2025 at 3:08 PM Horatiu Vultur
<horatiu.vultur@microchip.com> wrote:
>
> The 05/08/2025 11:33, Jason Xing wrote:
> >
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > Make sure the call of skb_tx_timestamp as close to the doorbell.
> >
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > ---
> >  drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> > index 502670718104..e030f23e5145 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> > @@ -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.

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

>
> Nacked-by: Horatiu Vultur <horatiu.vultur@microchip.com>
>
> >         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
> >
>
> --
> /Horatiu

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

[Horatiu Vultur]

The 05/08/2025 16:40, Jason Xing wrote:
> Hi Horatiu,

Hi Jason,

> 
> On Thu, May 8, 2025 at 3:08 PM Horatiu Vultur
> <horatiu.vultur@microchip.com> wrote:
> >
> > The 05/08/2025 11:33, Jason Xing wrote:
> > >
> > > From: Jason Xing <kernelxing@tencent.com>
> > >
> > > Make sure the call of skb_tx_timestamp as close to the doorbell.
> > >
> > > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > > ---
> > >  drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> > > index 502670718104..e030f23e5145 100644
> > > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> > > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> > > @@ -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.
> 
> 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.

> 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.

> 
> Thanks,
> Jason
> 
> >
> > Nacked-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> >
> > >         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
> > >
> >
> > --
> > /Horatiu

-- 
/Horatiu

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

[Jason Xing]

On Thu, May 8, 2025 at 5:44 PM Horatiu Vultur
<horatiu.vultur@microchip.com> wrote:
>
> The 05/08/2025 16:40, Jason Xing wrote:
> > Hi Horatiu,
>
> Hi Jason,
>
> >
> > On Thu, May 8, 2025 at 3:08 PM Horatiu Vultur
> > <horatiu.vultur@microchip.com> wrote:
> > >
> > > The 05/08/2025 11:33, Jason Xing wrote:
> > > >
> > > > From: Jason Xing <kernelxing@tencent.com>
> > > >
> > > > Make sure the call of skb_tx_timestamp as close to the doorbell.
> > > >
> > > > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > > > ---
> > > >  drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> > > > index 502670718104..e030f23e5145 100644
> > > > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> > > > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> > > > @@ -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.
> >
> > 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.

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

>
> > 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.
>
> >
> > Thanks,
> > Jason
> >
> > >
> > > Nacked-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > >
> > > >         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
> > > >
> > >
> > > --
> > > /Horatiu
>
> --
> /Horatiu

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

[Vladimir Oltean]

On Thu, May 08, 2025 at 08:22:39PM +0800, Jason Xing wrote:
> 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)

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

[Jason Xing]

On Thu, May 8, 2025 at 8:40 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Thu, May 08, 2025 at 08:22:39PM +0800, Jason Xing wrote:
> > 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)

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

Thanks,
Jason

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

[Vladimir Oltean]

Horatiu,

On Thu, May 08, 2025 at 11:41:56AM +0200, Horatiu Vultur wrote:
> > > > -       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.
> 
> > 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().

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

[Horatiu Vultur]

The 05/08/2025 17:21, Vladimir Oltean wrote:
> 
> Horatiu,

Hi Vladimir,

> 
> On Thu, May 08, 2025 at 11:41:56AM +0200, Horatiu Vultur wrote:
> > > > > -       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.
> >
> > > 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?

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

> 
> 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.

-- 
/Horatiu