[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <IA3PR11MB8986359263651CCAEF537CAFE5DBA@IA3PR11MB8986.namprd11.prod.outlook.com>
Date: Mon, 1 Dec 2025 07:29:39 +0000
From: "Loktionov, Aleksandr" <aleksandr.loktionov@...el.com>
To: Mina Almasry <almasrymina@...gle.com>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "bpf@...r.kernel.org"
<bpf@...r.kernel.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, YiFei Zhu <zhuyifei@...gle.com>, "Alexei
Starovoitov" <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, "David
S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, "Jesper
Dangaard Brouer" <hawk@...nel.org>, John Fastabend
<john.fastabend@...il.com>, Stanislav Fomichev <sdf@...ichev.me>, "Nguyen,
Anthony L" <anthony.l.nguyen@...el.com>, "Kitszel, Przemyslaw"
<przemyslaw.kitszel@...el.com>, Andrew Lunn <andrew+netdev@...n.ch>, "Eric
Dumazet" <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, "Lobakin,
Aleksander" <aleksander.lobakin@...el.com>, Richard Cochran
<richardcochran@...il.com>, "intel-wired-lan@...ts.osuosl.org"
<intel-wired-lan@...ts.osuosl.org>
Subject: RE: [Intel-wired-lan] [PATCH net-next v1] idpf: export RX hardware
timestamping information to XDP
> -----Original Message-----
> From: Mina Almasry <almasrymina@...gle.com>
> Sent: Monday, December 1, 2025 2:27 AM
> To: Loktionov, Aleksandr <aleksandr.loktionov@...el.com>
> Cc: netdev@...r.kernel.org; bpf@...r.kernel.org; linux-
> kernel@...r.kernel.org; YiFei Zhu <zhuyifei@...gle.com>; Alexei
> Starovoitov <ast@...nel.org>; Daniel Borkmann <daniel@...earbox.net>;
> David S. Miller <davem@...emloft.net>; Jakub Kicinski
> <kuba@...nel.org>; Jesper Dangaard Brouer <hawk@...nel.org>; John
> Fastabend <john.fastabend@...il.com>; Stanislav Fomichev
> <sdf@...ichev.me>; Nguyen, Anthony L <anthony.l.nguyen@...el.com>;
> Kitszel, Przemyslaw <przemyslaw.kitszel@...el.com>; Andrew Lunn
> <andrew+netdev@...n.ch>; Eric Dumazet <edumazet@...gle.com>; Paolo
> Abeni <pabeni@...hat.com>; Lobakin, Aleksander
> <aleksander.lobakin@...el.com>; Richard Cochran
> <richardcochran@...il.com>; intel-wired-lan@...ts.osuosl.org
> Subject: Re: [Intel-wired-lan] [PATCH net-next v1] idpf: export RX
> hardware timestamping information to XDP
>
> On Mon, Nov 24, 2025 at 2:33 AM Loktionov, Aleksandr
> <aleksandr.loktionov@...el.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Intel-wired-lan <intel-wired-lan-bounces@...osl.org> On
> Behalf
> > > Of Mina Almasry
> > > Sent: Saturday, November 22, 2025 3:09 PM
> > > To: netdev@...r.kernel.org; bpf@...r.kernel.org; linux-
> > > kernel@...r.kernel.org
> > > Cc: YiFei Zhu <zhuyifei@...gle.com>; Alexei Starovoitov
> > > <ast@...nel.org>; Daniel Borkmann <daniel@...earbox.net>; David S.
> > > Miller <davem@...emloft.net>; Jakub Kicinski <kuba@...nel.org>;
> > > Jesper Dangaard Brouer <hawk@...nel.org>; John Fastabend
> > > <john.fastabend@...il.com>; Stanislav Fomichev <sdf@...ichev.me>;
> > > Nguyen, Anthony L <anthony.l.nguyen@...el.com>; Kitszel,
> Przemyslaw
> > > <przemyslaw.kitszel@...el.com>; Andrew Lunn
> <andrew+netdev@...n.ch>;
> > > Eric Dumazet <edumazet@...gle.com>; Paolo Abeni
> <pabeni@...hat.com>;
> > > Lobakin, Aleksander <aleksander.lobakin@...el.com>; Richard
> Cochran
> > > <richardcochran@...il.com>; intel-wired-lan@...ts.osuosl.org; Mina
> > > Almasry <almasrymina@...gle.com>
> > > Subject: [Intel-wired-lan] [PATCH net-next v1] idpf: export RX
> > > hardware timestamping information to XDP
> > >
> > > From: YiFei Zhu <zhuyifei@...gle.com>
> > >
> > > 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@...ts.osuosl.org
> > >
> > > Signed-off-by: YiFei Zhu <zhuyifei@...gle.com>
> > > Signed-off-by: Mina Almasry <almasrymina@...gle.com>
> > > ---
> > > 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?
>
Oh, yes. You are right. For u8 byte endianness has no sense. Sorry.
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@...el.com>
Powered by blists - more mailing lists