Re: [Intel-wired-lan] [PATCH net-next v1] idpf: export RX hardware timestamping information to XDP

[Mina Almasry]

On Mon, Nov 24, 2025 at 2:33 AM Loktionov, Aleksandr
<aleksandr.loktionov@xxxxxxxxx> wrote:
>
>
>
> > -----Original Message-----
> > From: Intel-wired-lan <intel-wired-lan-bounces@xxxxxxxxxx> On Behalf
> > Of Mina Almasry
> > Sent: Saturday, November 22, 2025 3:09 PM
> > To: netdev@xxxxxxxxxxxxxxx; bpf@xxxxxxxxxxxxxxx; linux-
> > kernel@xxxxxxxxxxxxxxx
> > Cc: YiFei Zhu <zhuyifei@xxxxxxxxxx>; Alexei Starovoitov
> > <ast@xxxxxxxxxx>; Daniel Borkmann <daniel@xxxxxxxxxxxxx>; David S.
> > Miller <davem@xxxxxxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>; Jesper
> > Dangaard Brouer <hawk@xxxxxxxxxx>; John Fastabend
> > <john.fastabend@xxxxxxxxx>; Stanislav Fomichev <sdf@xxxxxxxxxxx>;
> > Nguyen, Anthony L <anthony.l.nguyen@xxxxxxxxx>; Kitszel, Przemyslaw
> > <przemyslaw.kitszel@xxxxxxxxx>; Andrew Lunn <andrew+netdev@xxxxxxx>;
> > Eric Dumazet <edumazet@xxxxxxxxxx>; Paolo Abeni <pabeni@xxxxxxxxxx>;
> > Lobakin, Aleksander <aleksander.lobakin@xxxxxxxxx>; Richard Cochran
> > <richardcochran@xxxxxxxxx>; intel-wired-lan@xxxxxxxxxxxxxxxx; Mina
> > Almasry <almasrymina@xxxxxxxxxx>
> > Subject: [Intel-wired-lan] [PATCH net-next v1] idpf: export RX
> > hardware timestamping information to XDP
> >
> > From: YiFei Zhu <zhuyifei@xxxxxxxxxx>
> >
> > The logic is similar to idpf_rx_hwtstamp, but the data is exported as
> > a BPF kfunc instead of appended to an skb.
> >
> > A idpf_queue_has(PTP, rxq) condition is added to check the queue
> > supports PTP similar to idpf_rx_process_skb_fields.
> >
> > Cc: intel-wired-lan@xxxxxxxxxxxxxxxx
> >
> > Signed-off-by: YiFei Zhu <zhuyifei@xxxxxxxxxx>
> > Signed-off-by: Mina Almasry <almasrymina@xxxxxxxxxx>
> > ---
> >  drivers/net/ethernet/intel/idpf/xdp.c | 27
> > +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/intel/idpf/xdp.c
> > b/drivers/net/ethernet/intel/idpf/xdp.c
> > index 21ce25b0567f..850389ca66b6 100644
> > --- a/drivers/net/ethernet/intel/idpf/xdp.c
> > +++ b/drivers/net/ethernet/intel/idpf/xdp.c
> > @@ -2,6 +2,7 @@
> >  /* Copyright (C) 2025 Intel Corporation */
> >
> >  #include "idpf.h"
> > +#include "idpf_ptp.h"
> >  #include "idpf_virtchnl.h"
> >  #include "xdp.h"
> >  #include "xsk.h"
> > @@ -369,6 +370,31 @@ int idpf_xdp_xmit(struct net_device *dev, int n,
> > struct xdp_frame **frames,
> >                                      idpf_xdp_tx_finalize);
> >  }
> >
> > +static int idpf_xdpmo_rx_timestamp(const struct xdp_md *ctx, u64
> > +*timestamp) {
> > +     const struct virtchnl2_rx_flex_desc_adv_nic_3 *rx_desc;
> > +     const struct libeth_xdp_buff *xdp = (typeof(xdp))ctx;
> > +     const struct idpf_rx_queue *rxq;
> > +     u64 cached_time, ts_ns;
> > +     u32 ts_high;
> > +
> > +     rx_desc = xdp->desc;
> > +     rxq = libeth_xdp_buff_to_rq(xdp, typeof(*rxq), xdp_rxq);
> > +
> > +     if (!idpf_queue_has(PTP, rxq))
> > +             return -ENODATA;
> > +     if (!(rx_desc->ts_low & VIRTCHNL2_RX_FLEX_TSTAMP_VALID))
> > +             return -ENODATA;
> RX flex desc fields are little‑endian.
> You already convert ts_high with le32_to_cpu(), but test ts_low directly against the mask.
> On big‑endian this can misdetect the bit and spuriously return -ENODATA.
> Please convert ts_low to host order before the bit test.
> See existing IDPF/ICE patterns where descriptor words are leXX_to_cpu()‑converted prior to FIELD_GET() / bit checks.
> Also, per the XDP RX metadata kfunc docs, -ENODATA must reflect true absence of per‑packet metadata; endianness‑correct testing is required to uphold the semantic.
>

Hey, sorry for the late reply. Initially when I read the reply, I
thought: "why not, lets add a leXX_to_cpu".

But now that I look closer to implement the change and submit v2, it
looks correct as written. ts_low is defined as a u8:

```
struct virtchnl2_rx_flex_desc_adv_nic_3 {
...
u8 ts_low;
```

So it should not be fed into any leXX_to_cpu() functions, no?

I also looked at other u8 members in this struct like `u8
status_err0_qw0` and `u8 status_err0_qw1`, and both are used in
existing code without a conversion. So it seems correct as written.
Can you reconsdirer?

If you insist some change is required, can you elaborate more on what
needs to be changed? There is no le8_to_cpu, unless a trivial one that
does nothing (one byte struct cannot be little or big endian).