[PATCH net] gve: Check valid ts bit on RX descriptor before hw timestamping

[PATCH net] gve: Check valid ts bit on RX descriptor before hw timestamping

[Harshitha Ramamurthy]

From: Tim Hostetler <thostet@google.com>

The device returns a valid bit in the LSB of the low timestamp byte in
the completion descriptor that the driver should check before
setting the SKB's hardware timestamp. If the timestamp is not valid, do not
hardware timestamp the SKB.

Cc: stable@vger.kernel.org
Fixes: b2c7aeb49056 ("gve: Implement ndo_hwtstamp_get/set for RX timestamping")
Reviewed-by: Joshua Washington <joshwash@google.com>
Signed-off-by: Tim Hostetler <thostet@google.com>
Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
---
 drivers/net/ethernet/google/gve/gve.h          |  2 ++
 drivers/net/ethernet/google/gve/gve_desc_dqo.h |  3 ++-
 drivers/net/ethernet/google/gve/gve_rx_dqo.c   | 18 ++++++++++++------
 3 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
index bceaf9b05cb4..4cc6dcbfd367 100644
--- a/drivers/net/ethernet/google/gve/gve.h
+++ b/drivers/net/ethernet/google/gve/gve.h
@@ -100,6 +100,8 @@
  */
 #define GVE_DQO_QPL_ONDEMAND_ALLOC_THRESHOLD 96
 
+#define GVE_DQO_RX_HWTSTAMP_VALID 0x1
+
 /* Each slot in the desc ring has a 1:1 mapping to a slot in the data ring */
 struct gve_rx_desc_queue {
 	struct gve_rx_desc *desc_ring; /* the descriptor ring */
diff --git a/drivers/net/ethernet/google/gve/gve_desc_dqo.h b/drivers/net/ethernet/google/gve/gve_desc_dqo.h
index d17da841b5a0..f7786b03c744 100644
--- a/drivers/net/ethernet/google/gve/gve_desc_dqo.h
+++ b/drivers/net/ethernet/google/gve/gve_desc_dqo.h
@@ -236,7 +236,8 @@ struct gve_rx_compl_desc_dqo {
 
 	u8 status_error1;
 
-	__le16 reserved5;
+	u8 reserved5;
+	u8 ts_sub_nsecs_low;
 	__le16 buf_id; /* Buffer ID which was sent on the buffer queue. */
 
 	union {
diff --git a/drivers/net/ethernet/google/gve/gve_rx_dqo.c b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
index 7380c2b7a2d8..02e25be8a50d 100644
--- a/drivers/net/ethernet/google/gve/gve_rx_dqo.c
+++ b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
@@ -456,14 +456,20 @@ static void gve_rx_skb_hash(struct sk_buff *skb,
  * Note that this means if the time delta between packet reception and the last
  * clock read is greater than ~2 seconds, this will provide invalid results.
  */
-static void gve_rx_skb_hwtstamp(struct gve_rx_ring *rx, u32 hwts)
+static void gve_rx_skb_hwtstamp(struct gve_rx_ring *rx,
+				const struct gve_rx_compl_desc_dqo *desc)
 {
 	u64 last_read = READ_ONCE(rx->gve->last_sync_nic_counter);
 	struct sk_buff *skb = rx->ctx.skb_head;
-	u32 low = (u32)last_read;
-	s32 diff = hwts - low;
-
-	skb_hwtstamps(skb)->hwtstamp = ns_to_ktime(last_read + diff);
+	u32 ts, low;
+	s32 diff;
+
+	if (desc->ts_sub_nsecs_low & GVE_DQO_RX_HWTSTAMP_VALID) {
+		ts = le32_to_cpu(desc->ts);
+		low = (u32)last_read;
+		diff = ts - low;
+		skb_hwtstamps(skb)->hwtstamp = ns_to_ktime(last_read + diff);
+	}
 }
 
 static void gve_rx_free_skb(struct napi_struct *napi, struct gve_rx_ring *rx)
@@ -919,7 +925,7 @@ static int gve_rx_complete_skb(struct gve_rx_ring *rx, struct napi_struct *napi,
 		gve_rx_skb_csum(rx->ctx.skb_head, desc, ptype);
 
 	if (rx->gve->ts_config.rx_filter == HWTSTAMP_FILTER_ALL)
-		gve_rx_skb_hwtstamp(rx, le32_to_cpu(desc->ts));
+		gve_rx_skb_hwtstamp(rx, desc);
 
 	/* RSC packets must set gso_size otherwise the TCP stack will complain
 	 * that packets are larger than MTU.
-- 
2.51.0.740.g6adb054d12-goog

Re: [PATCH net] gve: Check valid ts bit on RX descriptor before hw timestamping

[Simon Horman]

On Tue, Oct 14, 2025 at 12:47:39AM +0000, Harshitha Ramamurthy wrote:
> From: Tim Hostetler <thostet@google.com>
> 
> The device returns a valid bit in the LSB of the low timestamp byte in
> the completion descriptor that the driver should check before
> setting the SKB's hardware timestamp. If the timestamp is not valid, do not
> hardware timestamp the SKB.
> 
> Cc: stable@vger.kernel.org
> Fixes: b2c7aeb49056 ("gve: Implement ndo_hwtstamp_get/set for RX timestamping")
> Reviewed-by: Joshua Washington <joshwash@google.com>
> Signed-off-by: Tim Hostetler <thostet@google.com>
> Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net] gve: Check valid ts bit on RX descriptor before hw timestamping

[Willem de Bruijn]

Harshitha Ramamurthy wrote:
> From: Tim Hostetler <thostet@google.com>
> 
> The device returns a valid bit in the LSB of the low timestamp byte in
> the completion descriptor that the driver should check before
> setting the SKB's hardware timestamp. If the timestamp is not valid, do not

nit: weird line wrap. if setting had been on the line above, no line over 70.

> hardware timestamp the SKB.
> 
> Cc: stable@vger.kernel.org
> Fixes: b2c7aeb49056 ("gve: Implement ndo_hwtstamp_get/set for RX timestamping")
> Reviewed-by: Joshua Washington <joshwash@google.com>
> Signed-off-by: Tim Hostetler <thostet@google.com>
> Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>

Reviewed-by: Willem de Bruijn <willemb@google.com>

Re: [PATCH net] gve: Check valid ts bit on RX descriptor before hw timestamping

[Eric Dumazet]

On Mon, Oct 13, 2025 at 5:47 PM Harshitha Ramamurthy
<hramamurthy@google.com> wrote:
>
> From: Tim Hostetler <thostet@google.com>
>
> The device returns a valid bit in the LSB of the low timestamp byte in
> the completion descriptor that the driver should check before
> setting the SKB's hardware timestamp. If the timestamp is not valid, do not
> hardware timestamp the SKB.
>
> Cc: stable@vger.kernel.org
> Fixes: b2c7aeb49056 ("gve: Implement ndo_hwtstamp_get/set for RX timestamping")
> Reviewed-by: Joshua Washington <joshwash@google.com>
> Signed-off-by: Tim Hostetler <thostet@google.com>
> Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
> ---
>  drivers/net/ethernet/google/gve/gve.h          |  2 ++
>  drivers/net/ethernet/google/gve/gve_desc_dqo.h |  3 ++-
>  drivers/net/ethernet/google/gve/gve_rx_dqo.c   | 18 ++++++++++++------
>  3 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
> index bceaf9b05cb4..4cc6dcbfd367 100644
> --- a/drivers/net/ethernet/google/gve/gve.h
> +++ b/drivers/net/ethernet/google/gve/gve.h
> @@ -100,6 +100,8 @@
>   */
>  #define GVE_DQO_QPL_ONDEMAND_ALLOC_THRESHOLD 96
>
> +#define GVE_DQO_RX_HWTSTAMP_VALID 0x1
> +
>  /* Each slot in the desc ring has a 1:1 mapping to a slot in the data ring */
>  struct gve_rx_desc_queue {
>         struct gve_rx_desc *desc_ring; /* the descriptor ring */
> diff --git a/drivers/net/ethernet/google/gve/gve_desc_dqo.h b/drivers/net/ethernet/google/gve/gve_desc_dqo.h
> index d17da841b5a0..f7786b03c744 100644
> --- a/drivers/net/ethernet/google/gve/gve_desc_dqo.h
> +++ b/drivers/net/ethernet/google/gve/gve_desc_dqo.h
> @@ -236,7 +236,8 @@ struct gve_rx_compl_desc_dqo {
>
>         u8 status_error1;
>
> -       __le16 reserved5;
> +       u8 reserved5;
> +       u8 ts_sub_nsecs_low;
>         __le16 buf_id; /* Buffer ID which was sent on the buffer queue. */
>
>         union {
> diff --git a/drivers/net/ethernet/google/gve/gve_rx_dqo.c b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> index 7380c2b7a2d8..02e25be8a50d 100644
> --- a/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> +++ b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> @@ -456,14 +456,20 @@ static void gve_rx_skb_hash(struct sk_buff *skb,
>   * Note that this means if the time delta between packet reception and the last
>   * clock read is greater than ~2 seconds, this will provide invalid results.
>   */
> -static void gve_rx_skb_hwtstamp(struct gve_rx_ring *rx, u32 hwts)
> +static void gve_rx_skb_hwtstamp(struct gve_rx_ring *rx,
> +                               const struct gve_rx_compl_desc_dqo *desc)
>  {
>         u64 last_read = READ_ONCE(rx->gve->last_sync_nic_counter);
>         struct sk_buff *skb = rx->ctx.skb_head;
> -       u32 low = (u32)last_read;
> -       s32 diff = hwts - low;
> -
> -       skb_hwtstamps(skb)->hwtstamp = ns_to_ktime(last_read + diff);
> +       u32 ts, low;
> +       s32 diff;
> +
> +       if (desc->ts_sub_nsecs_low & GVE_DQO_RX_HWTSTAMP_VALID) {
> +               ts = le32_to_cpu(desc->ts);
> +               low = (u32)last_read;
> +               diff = ts - low;
> +               skb_hwtstamps(skb)->hwtstamp = ns_to_ktime(last_read + diff);
> +       }

If (desc->ts_sub_nsecs_low & GVE_DQO_RX_HWTSTAMP_VALID) can vary among
all packets received on this queue,
I will suggest you add an

        else {
                 skb_hwtstamps(skb)->hwtstamp = 0;
        }

This is because napi_reuse_skb() does not currently clear this field.

So if a prior skb had hwtstamp set to a timestamp, then merged in GRO,
and recycled, we have the risk of reusing an old timestamp
if GVE_DQO_RX_HWTSTAMP_VALID is unset.

We could also handle this generically, at the cost of one extra
instruction in the fast path.


>  }
>
>  static void gve_rx_free_skb(struct napi_struct *napi, struct gve_rx_ring *rx)
> @@ -919,7 +925,7 @@ static int gve_rx_complete_skb(struct gve_rx_ring *rx, struct napi_struct *napi,
>                 gve_rx_skb_csum(rx->ctx.skb_head, desc, ptype);
>
>         if (rx->gve->ts_config.rx_filter == HWTSTAMP_FILTER_ALL)
> -               gve_rx_skb_hwtstamp(rx, le32_to_cpu(desc->ts));
> +               gve_rx_skb_hwtstamp(rx, desc);
>
>         /* RSC packets must set gso_size otherwise the TCP stack will complain
>          * that packets are larger than MTU.
> --
> 2.51.0.740.g6adb054d12-goog
>

Re: [PATCH net] gve: Check valid ts bit on RX descriptor before hw timestamping

[Vadim Fedorenko]

On 14/10/2025 01:47, Harshitha Ramamurthy wrote:
> From: Tim Hostetler <thostet@google.com>
> 
> The device returns a valid bit in the LSB of the low timestamp byte in
> the completion descriptor that the driver should check before
> setting the SKB's hardware timestamp. If the timestamp is not valid, do not
> hardware timestamp the SKB.
> 
> Cc: stable@vger.kernel.org
> Fixes: b2c7aeb49056 ("gve: Implement ndo_hwtstamp_get/set for RX timestamping")
> Reviewed-by: Joshua Washington <joshwash@google.com>
> Signed-off-by: Tim Hostetler <thostet@google.com>
> Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>

Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

Re: [PATCH net] gve: Check valid ts bit on RX descriptor before hw timestamping

[Willem de Bruijn]

Eric Dumazet wrote:
> On Mon, Oct 13, 2025 at 5:47 PM Harshitha Ramamurthy
> <hramamurthy@google.com> wrote:
> >
> > From: Tim Hostetler <thostet@google.com>
> >
> > The device returns a valid bit in the LSB of the low timestamp byte in
> > the completion descriptor that the driver should check before
> > setting the SKB's hardware timestamp. If the timestamp is not valid, do not
> > hardware timestamp the SKB.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: b2c7aeb49056 ("gve: Implement ndo_hwtstamp_get/set for RX timestamping")
> > Reviewed-by: Joshua Washington <joshwash@google.com>
> > Signed-off-by: Tim Hostetler <thostet@google.com>
> > Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
> > ---
> >  drivers/net/ethernet/google/gve/gve.h          |  2 ++
> >  drivers/net/ethernet/google/gve/gve_desc_dqo.h |  3 ++-
> >  drivers/net/ethernet/google/gve/gve_rx_dqo.c   | 18 ++++++++++++------
> >  3 files changed, 16 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
> > index bceaf9b05cb4..4cc6dcbfd367 100644
> > --- a/drivers/net/ethernet/google/gve/gve.h
> > +++ b/drivers/net/ethernet/google/gve/gve.h
> > @@ -100,6 +100,8 @@
> >   */
> >  #define GVE_DQO_QPL_ONDEMAND_ALLOC_THRESHOLD 96
> >
> > +#define GVE_DQO_RX_HWTSTAMP_VALID 0x1
> > +
> >  /* Each slot in the desc ring has a 1:1 mapping to a slot in the data ring */
> >  struct gve_rx_desc_queue {
> >         struct gve_rx_desc *desc_ring; /* the descriptor ring */
> > diff --git a/drivers/net/ethernet/google/gve/gve_desc_dqo.h b/drivers/net/ethernet/google/gve/gve_desc_dqo.h
> > index d17da841b5a0..f7786b03c744 100644
> > --- a/drivers/net/ethernet/google/gve/gve_desc_dqo.h
> > +++ b/drivers/net/ethernet/google/gve/gve_desc_dqo.h
> > @@ -236,7 +236,8 @@ struct gve_rx_compl_desc_dqo {
> >
> >         u8 status_error1;
> >
> > -       __le16 reserved5;
> > +       u8 reserved5;
> > +       u8 ts_sub_nsecs_low;
> >         __le16 buf_id; /* Buffer ID which was sent on the buffer queue. */
> >
> >         union {
> > diff --git a/drivers/net/ethernet/google/gve/gve_rx_dqo.c b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> > index 7380c2b7a2d8..02e25be8a50d 100644
> > --- a/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> > +++ b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> > @@ -456,14 +456,20 @@ static void gve_rx_skb_hash(struct sk_buff *skb,
> >   * Note that this means if the time delta between packet reception and the last
> >   * clock read is greater than ~2 seconds, this will provide invalid results.
> >   */
> > -static void gve_rx_skb_hwtstamp(struct gve_rx_ring *rx, u32 hwts)
> > +static void gve_rx_skb_hwtstamp(struct gve_rx_ring *rx,
> > +                               const struct gve_rx_compl_desc_dqo *desc)
> >  {
> >         u64 last_read = READ_ONCE(rx->gve->last_sync_nic_counter);
> >         struct sk_buff *skb = rx->ctx.skb_head;
> > -       u32 low = (u32)last_read;
> > -       s32 diff = hwts - low;
> > -
> > -       skb_hwtstamps(skb)->hwtstamp = ns_to_ktime(last_read + diff);
> > +       u32 ts, low;
> > +       s32 diff;
> > +
> > +       if (desc->ts_sub_nsecs_low & GVE_DQO_RX_HWTSTAMP_VALID) {
> > +               ts = le32_to_cpu(desc->ts);
> > +               low = (u32)last_read;
> > +               diff = ts - low;
> > +               skb_hwtstamps(skb)->hwtstamp = ns_to_ktime(last_read + diff);
> > +       }
> 
> If (desc->ts_sub_nsecs_low & GVE_DQO_RX_HWTSTAMP_VALID) can vary among
> all packets received on this queue,
> I will suggest you add an
> 
>         else {
>                  skb_hwtstamps(skb)->hwtstamp = 0;
>         }
> 
> This is because napi_reuse_skb() does not currently clear this field.
> 
> So if a prior skb had hwtstamp set to a timestamp, then merged in GRO,
> and recycled, we have the risk of reusing an old timestamp
> if GVE_DQO_RX_HWTSTAMP_VALID is unset.
> 
> We could also handle this generically, at the cost of one extra
> instruction in the fast path.

That would be safest. This may not be limited to GVE.

NICs supporting line rate timestamping is not universal. Older devices
predominantly aim to support low rate PTP messages AFAIK.

On the Tx path there are known rate limits to the number of packets
that some can timestamp, e.g., because of using PHY registers.

On the Rx path packets are selected by filters such as
HWTSTAMP_FILTER_PTP_V2_L2_SYNC. But its not guaranteed that these can
be matched and timestamped at any rate? Essentially trusting that no
more PTP packets arrive than the device can timestamp.

e1000e_rx_hwtstamp is a good example. It has a descriptor bit whether
a packet was timestamped, similar to GVE. And only supports a single
outstanding request:

        /* The Rx time stamp registers contain the time stamp.  No other
         * received packet will be time stamped until the Rx time stamp
         * registers are read.  Because only one packet can be time stamped
         * at a time, the register values must belong to this packet and
         * therefore none of the other additional attributes need to be
         * compared.
         */

Perhaps not the best example as it does not use napi_reuse_skb. I
thought of too late ;) But there are quite likely more.
 
> 
> >  }
> >
> >  static void gve_rx_free_skb(struct napi_struct *napi, struct gve_rx_ring *rx)
> > @@ -919,7 +925,7 @@ static int gve_rx_complete_skb(struct gve_rx_ring *rx, struct napi_struct *napi,
> >                 gve_rx_skb_csum(rx->ctx.skb_head, desc, ptype);
> >
> >         if (rx->gve->ts_config.rx_filter == HWTSTAMP_FILTER_ALL)
> > -               gve_rx_skb_hwtstamp(rx, le32_to_cpu(desc->ts));
> > +               gve_rx_skb_hwtstamp(rx, desc);
> >
> >         /* RSC packets must set gso_size otherwise the TCP stack will complain
> >          * that packets are larger than MTU.
> > --
> > 2.51.0.740.g6adb054d12-goog
> >

Re: [PATCH net] gve: Check valid ts bit on RX descriptor before hw timestamping

[Eric Dumazet]

On Tue, Oct 14, 2025 at 1:19 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Eric Dumazet wrote:
> > On Mon, Oct 13, 2025 at 5:47 PM Harshitha Ramamurthy
> > <hramamurthy@google.com> wrote:
> > >
> > > From: Tim Hostetler <thostet@google.com>
> > >
> > > The device returns a valid bit in the LSB of the low timestamp byte in
> > > the completion descriptor that the driver should check before
> > > setting the SKB's hardware timestamp. If the timestamp is not valid, do not
> > > hardware timestamp the SKB.
> > >
> > > Cc: stable@vger.kernel.org
> > > Fixes: b2c7aeb49056 ("gve: Implement ndo_hwtstamp_get/set for RX timestamping")
> > > Reviewed-by: Joshua Washington <joshwash@google.com>
> > > Signed-off-by: Tim Hostetler <thostet@google.com>
> > > Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
> > > ---
> > >  drivers/net/ethernet/google/gve/gve.h          |  2 ++
> > >  drivers/net/ethernet/google/gve/gve_desc_dqo.h |  3 ++-
> > >  drivers/net/ethernet/google/gve/gve_rx_dqo.c   | 18 ++++++++++++------
> > >  3 files changed, 16 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
> > > index bceaf9b05cb4..4cc6dcbfd367 100644
> > > --- a/drivers/net/ethernet/google/gve/gve.h
> > > +++ b/drivers/net/ethernet/google/gve/gve.h
> > > @@ -100,6 +100,8 @@
> > >   */
> > >  #define GVE_DQO_QPL_ONDEMAND_ALLOC_THRESHOLD 96
> > >
> > > +#define GVE_DQO_RX_HWTSTAMP_VALID 0x1
> > > +
> > >  /* Each slot in the desc ring has a 1:1 mapping to a slot in the data ring */
> > >  struct gve_rx_desc_queue {
> > >         struct gve_rx_desc *desc_ring; /* the descriptor ring */
> > > diff --git a/drivers/net/ethernet/google/gve/gve_desc_dqo.h b/drivers/net/ethernet/google/gve/gve_desc_dqo.h
> > > index d17da841b5a0..f7786b03c744 100644
> > > --- a/drivers/net/ethernet/google/gve/gve_desc_dqo.h
> > > +++ b/drivers/net/ethernet/google/gve/gve_desc_dqo.h
> > > @@ -236,7 +236,8 @@ struct gve_rx_compl_desc_dqo {
> > >
> > >         u8 status_error1;
> > >
> > > -       __le16 reserved5;
> > > +       u8 reserved5;
> > > +       u8 ts_sub_nsecs_low;
> > >         __le16 buf_id; /* Buffer ID which was sent on the buffer queue. */
> > >
> > >         union {
> > > diff --git a/drivers/net/ethernet/google/gve/gve_rx_dqo.c b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> > > index 7380c2b7a2d8..02e25be8a50d 100644
> > > --- a/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> > > +++ b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> > > @@ -456,14 +456,20 @@ static void gve_rx_skb_hash(struct sk_buff *skb,
> > >   * Note that this means if the time delta between packet reception and the last
> > >   * clock read is greater than ~2 seconds, this will provide invalid results.
> > >   */
> > > -static void gve_rx_skb_hwtstamp(struct gve_rx_ring *rx, u32 hwts)
> > > +static void gve_rx_skb_hwtstamp(struct gve_rx_ring *rx,
> > > +                               const struct gve_rx_compl_desc_dqo *desc)
> > >  {
> > >         u64 last_read = READ_ONCE(rx->gve->last_sync_nic_counter);
> > >         struct sk_buff *skb = rx->ctx.skb_head;
> > > -       u32 low = (u32)last_read;
> > > -       s32 diff = hwts - low;
> > > -
> > > -       skb_hwtstamps(skb)->hwtstamp = ns_to_ktime(last_read + diff);
> > > +       u32 ts, low;
> > > +       s32 diff;
> > > +
> > > +       if (desc->ts_sub_nsecs_low & GVE_DQO_RX_HWTSTAMP_VALID) {
> > > +               ts = le32_to_cpu(desc->ts);
> > > +               low = (u32)last_read;
> > > +               diff = ts - low;
> > > +               skb_hwtstamps(skb)->hwtstamp = ns_to_ktime(last_read + diff);
> > > +       }
> >
> > If (desc->ts_sub_nsecs_low & GVE_DQO_RX_HWTSTAMP_VALID) can vary among
> > all packets received on this queue,
> > I will suggest you add an
> >
> >         else {
> >                  skb_hwtstamps(skb)->hwtstamp = 0;
> >         }
> >
> > This is because napi_reuse_skb() does not currently clear this field.
> >
> > So if a prior skb had hwtstamp set to a timestamp, then merged in GRO,
> > and recycled, we have the risk of reusing an old timestamp
> > if GVE_DQO_RX_HWTSTAMP_VALID is unset.
> >
> > We could also handle this generically, at the cost of one extra
> > instruction in the fast path.
>
> That would be safest. This may not be limited to GVE.

Yes, I started counting the bugs yesterday.

But considering this is going to be a patch that is separate and could
be missed in various old kernels,
I think a belt and suspender mode is safer :

Make sure each individual driver does not assume the field is zero
(or add a check about this assertion, which is going to be more
expensive anyway)

>
> NICs supporting line rate timestamping is not universal. Older devices
> predominantly aim to support low rate PTP messages AFAIK.
>
> On the Tx path there are known rate limits to the number of packets
> that some can timestamp, e.g., because of using PHY registers.
>
> On the Rx path packets are selected by filters such as
> HWTSTAMP_FILTER_PTP_V2_L2_SYNC. But its not guaranteed that these can
> be matched and timestamped at any rate? Essentially trusting that no
> more PTP packets arrive than the device can timestamp.
>
> e1000e_rx_hwtstamp is a good example. It has a descriptor bit whether
> a packet was timestamped, similar to GVE. And only supports a single
> outstanding request:
>
>         /* The Rx time stamp registers contain the time stamp.  No other
>          * received packet will be time stamped until the Rx time stamp
>          * registers are read.  Because only one packet can be time stamped
>          * at a time, the register values must belong to this packet and
>          * therefore none of the other additional attributes need to be
>          * compared.
>          */
>
> Perhaps not the best example as it does not use napi_reuse_skb. I
> thought of too late ;) But there are quite likely more.
>

I took a look at various uses in RX path in drivers/net/ethernet, I
found many bugs

drivers/net/ethernet/amd/xgbe/xgbe-drv.c:2374
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c:1098
drivers/net/ethernet/broadcom/tg3.c:6898
drivers/net/ethernet/cavium/liquidio/lio_core.c
drivers/net/ethernet/freescale/enetc/enetc.c
drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
drivers/net/ethernet/meta/fbnic/fbnic_txrx.c...

Then I stopped chasing.

I will send a fix in napi_reuse_skb()

diff --git a/net/core/gro.c b/net/core/gro.c
index 5ba4504cfd28ec26f487bfb96645e25c4845d720..76f9c3712422109ad00f15f6804abf6a8b00db43
100644
--- a/net/core/gro.c
+++ b/net/core/gro.c
@@ -639,6 +639,8 @@ EXPORT_SYMBOL(gro_receive_skb);

 static void napi_reuse_skb(struct napi_struct *napi, struct sk_buff *skb)
 {
+       struct skb_shared_info *shinfo;
+
        if (unlikely(skb->pfmemalloc)) {
                consume_skb(skb);
                return;
@@ -655,8 +657,12 @@ static void napi_reuse_skb(struct napi_struct
*napi, struct sk_buff *skb)

        skb->encapsulation = 0;
        skb->ip_summed = CHECKSUM_NONE;
-       skb_shinfo(skb)->gso_type = 0;
-       skb_shinfo(skb)->gso_size = 0;
+
+       shinfo = skb_shinfo(skb);
+       shinfo->gso_type = 0;
+       shinfo->gso_size = 0;
+       shinfo->hwtstamps.hwtstamp = 0;
+
        if (unlikely(skb->slow_gro)) {
                skb_orphan(skb);
                skb_ext_reset(skb);

Re: [PATCH net] gve: Check valid ts bit on RX descriptor before hw timestamping

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 14 Oct 2025 00:47:39 +0000 you wrote:
> From: Tim Hostetler <thostet@google.com>
> 
> The device returns a valid bit in the LSB of the low timestamp byte in
> the completion descriptor that the driver should check before
> setting the SKB's hardware timestamp. If the timestamp is not valid, do not
> hardware timestamp the SKB.
> 
> [...]

Here is the summary with links:
  - [net] gve: Check valid ts bit on RX descriptor before hw timestamping
    https://git.kernel.org/netdev/net/c/bfdd74166a63

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html